feat(HubPool): Second iteration on init, dispute and execute relayer refund bundles

[chrismaree]

This PR adds the second iteration of initiateRelayerRefund, disputeRelayerRefund and executeRelayerRefund.

[chrismaree]

this structure changed somewhat.

[chrismaree]

unfortunately this is the syntax we need to use because this struct contains a mapping for mapping(uint256 => uint256) claimedBitMap;. doing it like this leaves that mapping as uninitialized, giving it an empty mapping.

[chrismaree]

renamed these to be more consistant.

[chrismaree]

if we are deploying a HubPool then add the library linking

[mrice32]

This is awesome! I have some high level comments, but this is super cool so far.

[mrice32]

Is it possible to avoid emitting these large data structures? Only reason I ask is that when you add all of this up, we end up essentially emitting more data than the entire tree once we execute all of these. That might start getting expensive. Not at all against emitting data, but we might want to think about which pieces of data we will need offchain.

[chrismaree]

ye, that's a good point. we dont need it off-chain. I was just emitting everything I could and not really considering this. as a more meta point, we should generally consider the impact of emitting more complex structures vs less complex structures in events. is this largely impactful? I did not think it was.

[mrice32]

I agree with your broader point around emitting structs. Etherscan and other offchain parsers (like HAL) don't seem to be able to read this. I think we should generally avoid putting important information in structs within events.

[mrice32]

nit: event is called Requested, but method is called initiate. Might make sense to make them consistent?

[chrismaree]

yup yup. good point. will rename.

[mrice32]

What happens if the bondAmount changes mid liveness?

[chrismaree]

we assume it does not. I think that's fair assumption. we should pause the bridge if changing this number.

[mrice32]

Would pause-unpause work well within a governance process? Can we game this out a little bit? Could we make the setBond method detect whether there is an active, unresolved proposal to guarantee this doesn't result in broken accounting/lost funds?

[chrismaree]

ye, that can work! I can set that restriction on this method. I'll leave this for a separate PR.

[chrismaree]

I've added this in this PR: #14

[mrice32]

Awesome, sounds good!

[mrice32]

Do you think this is something we should optimistically verify? (I know, another thing that the bots would have to do)

I only ask because it looks like we have to store two variables on-chain to verify this (the number of relay refunds and the unclaimed rebalance leafs). Or would we have to store these variables anyway?

[chrismaree]

well we are not storing the number of relayer refunds, no? we are simply setting this (as the number of leaves) at the beginning. in my mind the big advantage with doing it this way is we know with certainty that there is no pending unclaimed leafs before initialising the next bundle of leafs. without this you can have a situation with inconsistant state on L1 vs what the bot sees if we do this optimistically (i.e I have not seen the final executed leafs for a set of repayments and therefore dispute the next bundle that comes in as I'm not up too date with master).

obviously, this is not that important if we design the bots well to avoid this but it just seems consistant to store this, with a marginally small gas cost.

put another way, storing this data and having this extra computation is likely less than 10k extra gas per relay refund in total. sending to one L2 costs at least 100k gas so this cost is small when compared to the other costs.

[mrice32]

That seems reasonable, and I think we should probably just not worry about this until we've nailed down how we're storing everything, as that probably changes a lot of the gas math here.

Also, totally agree that not doing this optimistically just removes a lot of bot edge cases, as you've pointed out.

[mrice32]

nit: push() returns the storage reference, so you can do:

MyStruct storage myStruct = array.push();
myStruct.someField = 1;
myStruct.otherField = 2;

This makes the syntax a bit cleaner and will save us from having to index into the array multiple times.

[mrice32]

Might not be something we want to do in this initial PR, but have you considered doing something similar to what we did in v1, where we hash all of this and just store the hash to verify the calldata that's passed in on later calls to reduce storage costs?

[chrismaree]

we can do that but we can only do it with the static data, right?

[mrice32]

Agreed. As discussed IRL, the savings may not make this worthwhile. It would save us around 15-20k gas, but require each subsequent proposal to pass in the previous proposal to prove that it was completed.

[nicholaspai]

Yeah too much complexity for the gas saved

[mrice32]

Do you think it's possible to somehow avoid storing all of these since only one is active at a time? We could reuse the same storage slot each time and always know if the prev one has been closed out when starting the next one because the storage slot isn't zero'd out. Note: only problem I see with this approach is that it might not work well with the mapping, since you can't really close out/delete a mapping.

[chrismaree]

we can do that. your goal is we'll get a refund if we re-use the same storage slots between refunds, right?

[chrismaree]

one option would to simply store one instance of RelayerRefundRequest within the hubPool. it tracks the exact variables we have in it now, but it's not within an array. When the unclaimedPoolRebalanceLeafs hits 0 and a data worker calls initiateRelayerRefund again we simply delete this instance of this struct and re-set it. this way we only ever have the storage of one instance of this data and will net many of the refunds. I'll give this a go and see how it looks.

[chrismaree]

I tried this out and it does not really work as you cant delete/reset the mapping in the way we'd need to to work. I tried creating this basic test:

pragma solidity ^0.8.0;

contract StructTest {
    struct RelayerRefundRequest {
        uint64 requestExpirationTimestamp;
        uint64 unclaimedPoolRebalanceLeafs;
        bytes32 poolRebalanceRoot;
        bytes32 destinationDistributionRoot;
        mapping(uint256 => uint256) claimedBitMap;
        address proposer;
        bool proposerBondRepaid;
    }

    RelayerRefundRequest public activeRelayerRefundRequest;

    function setValue() public {
        activeRelayerRefundRequest.proposer = msg.sender;
        activeRelayerRefundRequest.claimedBitMap[0] = 10;
    }

    function getClaim(uint256 index) public view returns (uint256) {
        return activeRelayerRefundRequest.claimedBitMap[index];
    }

    function resetStruct() public {
        delete activeRelayerRefundRequest;
    }
}

The delete call worked fine at resetting the proposer but after deleting the instance of RelayerRefundRequest the claimedBitMap was still initialised on the value set in the setValue call.

[chrismaree]

long story short you cant use delete on complex data types and so cant reset it. this means we either need:

1. to create a 3d mapping where we index some information to spectate out claimedBitMap for each proposal
2. do an array like I originally proposed
3. move away from a claimedBitMap to some other structure for tracking claims. I personally think leaving it like it is now is probs the cleanest and cheapest option.

[mrice32]

@chrismaree as discussed IRL, I think we landed on making the bitmap a single integer and limit us to supporting 256 chains.

[chrismaree]

That's right. I've implemented this in a separate PR to try minimise the changes here: #14

[mrice32]

Responded to all the comment threads! We discussed the most important ones offline, but I left a few comments on others as well.

[nicholaspai]

nit: change this name to end with LeafCounts instead of Leafs. I was a bit confused because without "counts" i expected this to be an array or a bitmap

[chrismaree]

sure.

[nicholaspai]

nit: should this be >= ?

[nicholaspai]

nit: same as as above, should this be >=

[nicholaspai]

pretty sure you can just return relayerRefundRequests.length here and the current implementation is incorrect

[chrismaree]

ye this is removed in the next PR

[nicholaspai]

LGTM after you review matt's comments!

[mrice32]

LGTM once @nicholaspai's comments are resolved.