Enforce max_block_size

[tbro]

Omit transactions that exceed max_block_size from payload. Use mocked max_block_size for now.

Depends on: EspressoSystems/HotShot#3096
Related to: EspressoSystems/hotshot-builder-core#122

[sveitser]

As this is a distinct / new feature for of our block building logic we should add a unittest that fails if we don't enforce the max block size.

[sveitser]

@tbro anything blocking more progress here?

[tbro]

@tbro anything blocking more progress here?

Yes I need to fix this: EspressoSystems/HotShot#3068. I've taken a few missteps on that, but making progress.

[jbearer]

This is not quite right as the payload contains other metadata, like a transaction table for each namespace, that could cause it to exceed the max block size even if the sum of transaction payloads doesn't.

Not sure of the best solution, maybe @ggutoski has some ideas. One idea could be to add a len() function to NamespaceInfo, which would return something like TxTableEntry::byte_len() + tx_table.len() + tx_bodies.len() (the first term is for the one word used to encode the length of the tx table). Then we would basically loop until namespaces.values().map(NamespaceInfo::len).sum() > chain_config.max_block_size().

[ggutoski]

I'm currently rewriting everything in block. I can definitely accommodate this. For this PR I suggest you do as little as possible to get it working.

It should be possible to keep a running count of the block byte length for each new transaction that includes tx table sizes, then just stop when we hit the limit.

(Note that a single large tx could cause a lot of wasted block space if it happens to be the one that triggers break. We might need to do something more intelligent in the future.)

Happy to hop on a call to nail this down.

[ggutoski]

Now that I think of it, maybe this is easy. Just do

block_size += tx.payload.len() + [size of a tx table entry];

[tbro]

like so?

tx.payload().len() + size_of::<TxTableEntry>()

[ggutoski]

Yes, actually. (Sorry I could have clarified that for you!)

You still need to account for the tx table header in each namespace. That's an additional size_of::<TxTableEntry>() bytes per namespace. So I guess at the end you could do something like

block_size += namespaces.len() * size_of::<TxTableEntry>()

[ggutoski]

that's a tiny amount of data, but it could in principle push you over the limit. You could account for it on-the-fly by doing block_size += size_of::<TxTableEntry>() each time you encounter a new namespace. Unfortunately that's wrapped up in update_namespace_with_tx, so you might need to hack that method to get what you want. I suggest you don't worry about ugly code for this PR though because it will all be replaced soon.

[tbro]

I'm still going over the tests to make sure they are still valid, but I think 61dd04f gets us what we need. Note that I removed a TODO that seemed relevant. cc @ggutoski @jbearer

[ggutoski]

i like it!

[jbearer]

If we break here we have already added the new transaction that pushes us over the limit. I think somehow we need to account for all the new size, including from update_namespace_with_tx before pushing the new tx

[tbro]

A yes. hummm its a complicated bit of spaghetti to try and reason about.

[tbro]

The only thing I can think of is just to check the hashmap and see if the namespace of the current tx is present or not. Its redundant b/c update_namespace_with_tx does that check again, but if @ggutoski is going to give all this an overhaul anyway maybe it doesn't matter.

[tbro]

fixed here 🤞 b849d44

[jbearer]

Cool, block size handling seems correct now