search

thehubbleproject / hubble-contracts

Hubble optimistic rollup
https://thehubbleproject.github.io/docs/
MIT License
133 stars 28 forks source link

Inconsistent deposit subtree IDs after rollback #671

Closed b-tarczynski closed 2 years ago

b-tarczynski commented 2 years ago

Right now rollbacks of deposit batches call reenqueue function on depositManager that creates new subtree with different subtreeID than was used in submitDeposits and emits new DepositSubTreeReady event with this ID. That is a problem in the case of syncing Deposits and DepositSubtrees from events. If we sync in commander DepositQueued events and add them to queue and we pop Deposits on DepositSubTreeReady events in case of rollback we wouldn't know which DepositQueued events assign to DepositSubTreeReady that was emitted for reenqueue function.

Way to fix that would be to implement the pushFront(bytes32 subtreeRoot) function that adds rollbacked Subtree at the beginning of the queue with the same SubtreeID. On the command side we would simply check if we already store subtree with this ID and in that case we would omit it. If we want to go with that approach then we should rename other SubtreeQueue functions, for example:

enqueue -> pushBack(bytes32 subtreeRoot)
dequeue -> popFront() returns (uint256 subtreeID, bytes32 subtreeRoot)

Also it would make deposits stay in original order after rollback, because right now submitted deposits are added at the end of the queue.

jacque006 commented 2 years ago

Overall this solution sounds good to me. The only confusing part is that there may now be multiple DepositSubTreeReady with the same args, but that can be pointed out/documented in SubtreeQueue.

If we want to go with that approach then we should rename other SubtreeQueue functions, for example: enqueue -> pushBack(bytes32 subtreeRoot) dequeue -> popFront() returns (uint256 subtreeID, bytes32 subtreeRoot)

I could see the new function just being called requeue to match original authors naming. If we wanted to follow ECMA/JS syntax which Solidity largely appears to, we would have push, shift, and unshift. But frankly, they all get the intent across :)

@ChihChengLiang feel free to chime in if you have any additional thoughts on this.

ChihChengLiang commented 2 years ago

The solution sounds good to me too. I like the pushBack, pushFront, and popFront namings too.