fix(hub-pool): Enable data worker to ensure that roots are not redundantly relayed to SpokePools

[nicholaspai]

Data worker can mark in PoolRebalanceLeaf whether it should send roots to the SpokePool. We assume that valid root bundles would only send roots to a SpokePool on a specific chainId once.

[chrismaree]

lgtm once the nits are resolved.

[mrice32]

What was the reasoning for using a bool over an index? Would something like perChainLeafIndex be more clear to someone creating a bundle?

[nicholaspai]

I just don't see any utility for the index. If you could give me an example of how an index would be useful to the off-chain data worker, then I'd change to an index. But I can't think of any examples where the index is useful.

[mrice32]

I think it's just easier to understand since an index obviously increments for each leaf for a chain. Whereas you have to know that this bool goes true on the first element and then false on every following element. I think the bool makes a lot of sense in the context of the HubPool, but to a relayer, this bool might have less meaning since they aren't working from within the HubPool's send logic. Maybe to reverse the question, what's the reasoning for using a bool over an index?

Not strongly opinionated on this, however. I'm cool with whatever you think!

[nicholaspai]

bool saves calldata gas right? thats the main reasoning

[mrice32]

It would not. See the first example here: https://docs.soliditylang.org/en/v0.8.11/abi-spec.html#examples. All smaller types are padded to 32 bytes in normal abi encoding (which is what we're using here and elsewhere). It's only in packed encoding (and in storage) that they are not padded.

Ironically, an index (or inverted bool) actually saves gas. This is because a nonzero byte costs 4x as much gas as a 0 byte. Since usually the index would be 0, this would result in all 0 bytes as opposed to the case where the bool is set to true and one of the bytes would be nonzero. This is a very small difference, though.

[nicholaspai]

Hm ok i'll change to an index since it doesn't seem to matter too much and it gives us a bit more flexibility

[nicholaspai]

Ok changed @mrice32 but I don't love the name groupIndex nor the perChainleafIndex. Any other ideas?

[mrice32]

Yeah, the naming is difficult. I think groupIndex is totally fine, though, especially since you documented it so well.

[mrice32]

#85 (comment)

[mrice32]

#85 (comment)

[mrice32]

LGTM!