thehubbleproject / hubble-contracts

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

BatchPubkeyRegistered's startID and endID are off by 1 #662

Closed jacque006 closed 2 years ago

jacque006 commented 2 years ago

Problem

When BatchPubkeyRegistered is emitted during BLSAccountRegistry.registerBatch, the startID and endID args are 1 based indexed instead of 0 based as they should be.

For example, when the first batch is registered with the default batch pubkey settings, the returned range should be 0 - 15, but is instead 1 -16.

Solution

Notes About Batch Pubkey Registration

https://github.com/thehubbleproject/hubble-contracts/blob/master/contracts/AccountTree.sol#L5-L8

The account tree for public keys is 32 levels deep, with 1 << 32 (4294967296) total nodes on the bottom. This is the maximum number of public keys that can be registered. This tree is then divided in half. The left side of the tree is for single registered pubkeys, the right for batch registered pubkeys. Both of these subtrees have 1 << 31 nodes (2147483648).

0             root
         /           \
1      rootLeft  rootRight
       /   \          /   \
       .....           ....
     /  \  /  \     /  \  /  \
32     single         batch

The ID (pubkeyID) for singly registered pubkeys starts at 0, and maxes out at (1 << 31) - 1 (2147483647). The ID for batch registered pubkeys starts at 1 << 31 (2147483648), and maxes out at (1 << 32) - 1 (4294967295). Batch pubkeys are registered in batches of 16 pubkeys so the range returned is the first and last one registered WITHOUT the left tree size taken into account.

So for the first batch will have a startID of 0 and an endID of 15 (range 0 - 15), but the IDs (pubkeyIDs) will be 2147483648 (1 << 31) - 2147483664 ((1 << 31) - 16). The last batch will have a range of 2147483632 ((1 << 31) - 16) - 2147483647 ((1 << 31) - 1) and the IDs will be 4294967280 ((1 << 32) - 16) - 4294967295 ((1 << 32 - 1)).

kautukkundan commented 2 years ago

Raised this concern over a bad observation. Correct value was returned from the event but the logged value was incremented by 1 due to incorrect code. The AccountRegistry works as expected 👍🏼

Apologies, closing this issue @jacque006 @ChihChengLiang