Skip to content
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.

Sign up for GitHub

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

Jump to bottom

enable stake table epochs #2510

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

enable stake table epochs #2510

wants to merge 9 commits into from

Conversation

alysiahuggins
Copy link
Contributor

Closes #2505

This PR:

  • re-adds epoch functionality so that validator registrations and exits can be handled on a epoch basis
  • ensures that the currentEpoch is a based on the latest hotshot block height and the hotShotBlocksPerEpoch
  • the hotShotBlocksPerEpoch is set at constructor time
  • consolidates nextRegistraionEpoch() and nextExitEpoch() into one function, nextAvailableEpoch(...) since the logic was the same
  • adds tests for the currentEpoch() and nextAvailableEpoch(...) functions

This PR does not:

  • provide a way to update hotshot blocks per epoch as it's assumed that this will not change in the near future, when it does change the function can be updated
  • handle integer overflows (e.g. currentEpoch()+1) - this is a discussion to have
  • modify epoch logic for functions such as withdrawFunds() which depends on the epoch functionality, this will be handled in other github issues.

Key places to review:

@alysiahuggins alysiahuggins linked an issue Jan 30, 2025 that may be closed by this pull request
set current epoch based on hotshot block height #2505
Open
@sveitser
Copy link
Collaborator

sveitser commented Feb 3, 2025

provide a way to update hotshot blocks per epoch as it's assumed that this will not change in the near future, when it does change the function can be updated

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 (hotshotBlockNumber, blocksPerEpoch) to enable this computation.

@alysiahuggins
Copy link
Contributor Author

provide a way to update hotshot blocks per epoch as it's assumed that this will not change in the near future, when it does change the function can be updated

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 (hotshotBlockNumber, blocksPerEpoch) to enable this computation.

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?

sveitser
sveitser reviewed Feb 5, 2025
View reviewed changes
contracts/src/StakeTable.sol Outdated
uint64 epoch;
uint64 queueSize;

if (firstAvailableEpoch < currentEpoch() + 1) {
Copy link
Collaborator

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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?

sveitser
sveitser reviewed Feb 5, 2025
View reviewed changes
contracts/test/StakeTable.t.sol Outdated
assertEq(currentBlockHeight, initialBlockHeight);

// Calculate the expected epoch
uint64 expectedEpoch = initialBlockHeight / hotShotBlocksPerEpoch;
Copy link
Collaborator

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.

Copy link
Contributor Author

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

Copy link
Collaborator

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.

alysiahuggins and others added 7 commits February 5, 2025 17:33
@alysiahuggins
set the expected epoch to be a constant instead of a calculation
af8841c
@sveitser
WIP: simplify data flow for epoch queues
229b454 
- Some renames to make it easier to read.
- Mutate the state of the queue in place relying on EVM reverts to
  restore the state of the contract when the transactions revert.

Would have to still update abstract contract and tests, so mainly
pushing this for discussion. We would also have a new function for
clients to determine available epochs, if that's actually required.

I think for clients it might be fine if we just return the max of the
registrationEpoch variable in the contract and currentEpoch() + 1. If
the registration epoch gets bumped by one by the time their transaction
is included that's just a race condition we can't avoid anyway. They can
set a reasonable `validUntilEpoch` to avoid having to wait too long.
@alysiahuggins
update tests, mark overiddden functions as virtual so they can be ove…
3f06c50 
…ridden again if need be, update parent contract
@alysiahuggins
Merge branch 'main' into 2316-stake-table-epochs
2b4ea47
@sveitser
remove public getters in favor of public members
32d1b60 
- Fix solhint.
- Add a test for an epoch lenght greater than one.
- Remove unnecessary return values from functions that push to queue.
@sveitser
Merge branch '2316-stake-table-epochs' into ma/2316-stake-table-epoch…
c908f4f 
…s-refactor
@sveitser
Simplify data flow for epoch queues (#2548)
1724050 
Note this PR is from reviewing #2316 and into that branch.

- Some renames to make it easier to read.
- Mutate the state of the queue in place relying on EVM reverts to
restore the state of the contract when the transactions revert.

Would have to still update abstract contract and tests, so mainly
pushing this for discussion. We would also have a new function for
clients to determine available epochs, if that's actually required.

I think for clients it might be fine if we just return the max of the
registrationEpoch variable in the contract and currentEpoch() + 1. If
the registration epoch gets bumped by one by the time their transaction
is included that's just a race condition we can't avoid anyway. They can
set a reasonable `validUntilEpoch` to avoid having to wait too long.
@sveitser
Copy link
Collaborator

sveitser commented Feb 10, 2025 •
edited
Loading

@alysiahuggins I think many of the tests in this PR currently only assert how the contract state changes. That is fine, but what is IMO more important and less of an implementation detail is if the events with correct epochs are emitted because this is what the application will actually consume (not the state of the contract). I think we should add some asserts to check that the epochs in the emitted events evolve as expected.

@alysiahuggins
Copy link
Contributor Author

@alysiahuggins I think many of the tests in this PR currently only assert how the contract state changes. That is fine, but what is IMO more important and less of an implementation detail is if the events with correct epochs are emitted because this is what the application will actually consume (not the state of the contract). I think we should add some asserts to check that the epochs in the emitted events evolve as expected.

agreed and will do

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sveitser sveitser sveitser left review comments

@philippecamacho philippecamacho Awaiting requested review from philippecamacho philippecamacho is a code owner

@ImJeremyHe ImJeremyHe Awaiting requested review from ImJeremyHe ImJeremyHe is a code owner

@jbearer jbearer Awaiting requested review from jbearer jbearer is a code owner

@tbro tbro Awaiting requested review from tbro tbro is a code owner

@imabdulbasit imabdulbasit Awaiting requested review from imabdulbasit imabdulbasit is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

set current epoch based on hotshot block height
2 participants
@alysiahuggins @sveitser