-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor binary search logic for ValidatorsAccumulator.sol #353
Conversation
uint64 blockNumber = uint64(block.number); | ||
snapshots[blockNumber] = _getRoot(validatorsCount); | ||
uint256 blockNumbersLength = blockNumbers.length; | ||
if ( | ||
blockNumbersLength == 0 || | ||
blockNumbers[blockNumbersLength - 1] != blockNumber | ||
) { | ||
blockNumbers.push(blockNumber); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better if we used a ring buffer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have discussed the idea of the ring buffer but because it is a very permissionless protocol we decided that someone could make multiple deposits and delete old data, maybe this assumption is extremely pessimistic as those deposits are not cheap and we learned later the contract is not entirely permissionless.
But apart from the ring buffer, I like the idea of using an array actually instead of a map I think it will make the solution a bit cleaner
DepositData[] snapshots;
And if the deposit is for the same block we will just overwrite the deposit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using mapping + array costs less in both deposit
and verify
transactions. Therefore, we will stick to the current approach (discussed offline).
accumulator: _getRoot(validatorsCount) | ||
}); | ||
uint64 blockNumber = uint64(block.number); | ||
snapshots[blockNumber] = _getRoot(validatorsCount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for not using getValidatorsAccumulator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getValidatorsAccumulator()
is external and reads from the storage variable which reminds me that we can reuse the already read from the storage variable.
@@ -15,10 +15,9 @@ contract ValidatorsAccumulator is IValidatorsAccumulator { | |||
|
|||
// A counter for the total number of validators | |||
uint256 internal validatorsCount; | |||
// Start index of validators map | |||
uint256 internal startIndex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should do that in another PR as it was reported as separate issue by audtiors
return 0; | ||
uint64 lowerBlockNumber = blockNumbers[lower]; | ||
if (lowerBlockNumber > blockNumber) { | ||
return lowerBlockNumber; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, it is better to return type(uint64).max as this is a special case where no block was found. And will make the code a bit easier to follow
72c04ec
to
717ebd4
Compare
Remove validators count and therefore refactor the storage layout and storage reading