-
Notifications
You must be signed in to change notification settings - Fork 89
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
enable stake table epochs #2510
base: main
Are you sure you want to change the base?
Conversation
… and add tests checking epochs
If the blocks per epoch changes then the computation of current epoch becomes more complicated and we need to store the original value of blocks per epoch and the new value to compute epochs correctly as a function of the hotshot block height. I wonder if we should make the blockPerEpoch an array of |
great idea, we discussed this and concluded that we didn't need to support changing hotshot blocks per epoch at this moment. but we could do, @jbearer @bfish713 what do you think? |
uint64 epoch; | ||
uint64 queueSize; | ||
|
||
if (firstAvailableEpoch < currentEpoch() + 1) { |
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.
Add a comment as to why we don't need to worry about overflow?
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 added it as a todo in line 174, i want to start a discussion around this, will do so now
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.
so they don't handle overflow in the offchain code, so i think a revert if it overflows can work? though it's a check every time this is invoked. what do you think?
contracts/test/StakeTable.t.sol
Outdated
assertEq(currentBlockHeight, initialBlockHeight); | ||
|
||
// Calculate the expected epoch | ||
uint64 expectedEpoch = initialBlockHeight / hotShotBlocksPerEpoch; |
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 the test would be easier to read if it just hardcoded zeroes, wherever zero is expected.
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.
makes sense, updated here af8841c
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 meant more generally l if we use asserts in a test it's usually implied that we are asserting that some value is equal to an expected value. So instead of assert(epoch, expectedEpoch)
we can write assert(epoch, 0)
and end up with fewer indirections.
However, if the left and right hand size are some complicated expressions then defining some extra variables is helpful for reading.
@@ -323,7 +343,8 @@ contract StakeTable is AbstractStakeTable { | |||
// currentEpoch() + 1 (the start of the next full epoch), but in periods of high churn the | |||
// queue may fill up and it may be later. If the queue is so full that the wait time exceeds | |||
// the caller's desired maximum wait, abort. | |||
(uint64 registerEpoch, uint64 queueSize) = this.nextRegistrationEpoch(); | |||
(uint64 registerEpoch, uint64 queueSize) = | |||
this.nextAvailableEpoch(firstAvailableRegistrationEpoch, _numPendingRegistrations); |
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.
There is the comment above how _numPendingRegistrations
is not the total queue size.
espresso-sequencer/contracts/src/StakeTable.sol
Lines 89 to 91 in 0728068
/// @notice number of pending registrations in the `firstAvailableRegistrationEpoch` (not the | |
/// total pending queue size!) | |
uint64 private _numPendingRegistrations; |
But later we set it as such, although there is also a TODO in that comment
espresso-sequencer/contracts/src/StakeTable.sol
Lines 213 to 219 in 0728068
// @param queueSize current size of the registration queue (after insertion of new element in | |
// the queue) | |
/// TODO modify this according to the current spec | |
function appendRegistrationQueue(uint64 epoch, uint64 queueSize) private { | |
firstAvailableRegistrationEpoch = epoch; | |
_numPendingRegistrations = queueSize + 1; | |
} |
I think it would be nice if we named things a bit more explicitly. Since there are many epochs, queue sizes, etc. especially for those variables it's useful to be explicit.
Closes #2505
This PR:
hotShotBlocksPerEpoch
hotShotBlocksPerEpoch
is set at constructor timenextRegistraionEpoch()
andnextExitEpoch()
into one function,nextAvailableEpoch(...)
since the logic was the samecurrentEpoch()
andnextAvailableEpoch(...)
functionsThis PR does not:
withdrawFunds()
which depends on the epoch functionality, this will be handled in other github issues.Key places to review: