-
Notifications
You must be signed in to change notification settings - Fork 712
Support abritrary epochs in TestChainstate #6619
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
base: develop
Are you sure you want to change the base?
Support abritrary epochs in TestChainstate #6619
Conversation
86e4e60 to
58cee2e
Compare
c3a3c2f to
c6ce521
Compare
Signed-off-by: Jacinta Ferrant <jacinta@stackslabs.com>
c6ce521 to
1eb48de
Compare
… into feat/expand-consensus-test-to-support-pre-nakamoto-epochs
Signed-off-by: Jacinta Ferrant <jacinta@stackslabs.com>
… into feat/expand-consensus-test-to-support-pre-nakamoto-epochs
Signed-off-by: Jacinta Ferrant <jacinta@stackslabs.com>
…t to use all epochs GTE epoch 2.0 Signed-off-by: Jacinta Ferrant <jacinta@stackslabs.com>
… into feat/expand-consensus-test-to-support-pre-nakamoto-epochs
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (68.99%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #6619 +/- ##
===========================================
+ Coverage 61.40% 68.99% +7.59%
===========================================
Files 574 574
Lines 354916 355501 +585
===========================================
+ Hits 217939 245284 +27345
+ Misses 136977 110217 -26760
... and 328 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
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 still need to finish the review of the changes in the consensus.rs file, but GREAT work!
| let epoch_at_height = SortitionDB::get_stacks_epoch(sort_db.conn(), cycle_start_height)? | ||
| .unwrap_or_else(|| { | ||
| panic!( | ||
| "FATAL: no epoch defined for burn height {}", | ||
| cycle_start_height | ||
| ) | ||
| }); | ||
|
|
||
| // Find the first Stacks block in this reward cycle's preceding prepare phase. | ||
| // This block will have invoked `.signers.stackerdb-set-signer-slots()` with the reward set. | ||
| // Note that we may not have processed it yet. But, if we do find it, then it's | ||
| // unique (and since Nakamoto Stacks blocks are processed in order, the anchor block | ||
| // cannot change later). | ||
| let first_epoch30_reward_cycle = burnchain | ||
| .block_height_to_reward_cycle(epoch_at_height.start_height) | ||
| .expect("FATAL: no reward cycle for epoch 3.0 start height"); | ||
|
|
||
| if !epoch_at_height | ||
| .epoch_id | ||
| .uses_nakamoto_reward_set(reward_cycle, first_epoch30_reward_cycle) | ||
| { | ||
| .unwrap_or_else(|| panic!("FATAL: no epoch defined for burn height {cycle_start_height}")); | ||
| let is_pre_naka_epoch = if epoch_at_height.epoch_id < StacksEpochId::Epoch30 { | ||
| true | ||
| } else { | ||
| let epoch_30 = | ||
| SortitionDB::get_stacks_epoch_by_epoch_id(sort_db.conn(), &StacksEpochId::Epoch30)? | ||
| .unwrap_or_else(|| panic!("FATAL: no Nakamoto epoch defined")); | ||
| // Find the first Stacks block in this reward cycle's preceding prepare phase. | ||
| // This block will have invoked `.signers.stackerdb-set-signer-slots()` with the reward set. | ||
| // Note that we may not have processed it yet. But, if we do find it, then it's | ||
| // unique (and since Nakamoto Stacks blocks are processed in order, the anchor block | ||
| // cannot change later). | ||
| let first_epoch30_reward_cycle = burnchain | ||
| .block_height_to_reward_cycle(epoch_30.start_height) | ||
| .expect("FATAL: no reward cycle for epoch 3.0 start height"); | ||
| !epoch_at_height | ||
| .epoch_id | ||
| .uses_nakamoto_reward_set(reward_cycle, first_epoch30_reward_cycle) | ||
| }; | ||
| if is_pre_naka_epoch { |
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.
qq for my own understanding: is this change for readibility purposes (I definitely find more more explicit the new code, instead of checking if it uses a nakamoto_reward_set) or did the old logic have a bug?
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.
Old logic actually had a bug. It wasn't a problem because we happened to choose our naka activation such that it didn't trigger, but with my experimenting with basically deploying at random heights...it would hit the issue and not correctly grab the first_epoch30_reward_cycle. EDIT: this actually is a bigger bug. see @aaronb-stacks 's thread: https://stackslabs.slack.com/archives/C09K44MQDS6/p1761873520836729
Also it is not fully fixed I don't think in this PR...
...ts/snapshots/blockstack_lib__chainstate__tests__consensus__successfully_deploy_and_call.snap
Outdated
Show resolved
Hide resolved
… into feat/expand-consensus-test-to-support-pre-nakamoto-epochs
…ion handling pre epoch 2.0 Signed-off-by: Jacinta Ferrant <jacinta@stackslabs.com>
Signed-off-by: Jacinta Ferrant <jacinta@stackslabs.com>
Signed-off-by: Jacinta Ferrant <jacinta@stackslabs.com>
| pub fn new( | ||
| test_name: &str, | ||
| initial_balances: Vec<(PrincipalData, u64)>, | ||
| epoch_blocks: HashMap<StacksEpochId, Vec<TestBlock>>, |
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.
maybe it's just me, but I find epoch_blocks little bit measleading to follow.
If I understood well it has different meaning depending on the test case:
- for
ContractConsensusTest(contract tests) it just serves as block counter to compute theEpochList - for
ConsensusTest(token transfer tests) it serves both as block counter and also effective blocks to execute
One more thing that started bugging me (not introduced in this PR) is that ContractConsensuTest depends on ConsesunsTest for sharing features but than both have different run() (and only ConsensusTest use the epoch_blocks` there)
So, I'm wondering if having a different design like the following, could help to make things (as the one above) clearer:
ConsensusTest: just have common feature for executing tests (or may even represent what for us would be the "Chain" on where we can do operations)ContractConsensutTest: is responsible for instantiating and running contract tests and can useConsensusTestTokenTransferTest: is responsible for instantiating and running token transfer tests and can useConsensusTest
What do you think guys?
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 agree that the way it is right now is a bit convoluted as a result of my need to precompute to some degree the epoch_list. I think it deserves a bit of a rethink and I like your suggestion (was so concerned with getting the working code up that I think I didn't give proper thought to its overall design)
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.
Cleaned up code mostly in c37946e.
There may be some better improvements possible but I think its a lot better with the frontloaded new() followed by run() call on the ContractConsensusTest.
| .unwrap_or_else(|| panic!("FATAL: no epoch defined for burn height {cycle_start_height}")); | ||
| let is_pre_naka_epoch = if epoch_at_height.epoch_id < StacksEpochId::Epoch30 { | ||
| true | ||
| } else { | ||
| let epoch_30 = | ||
| SortitionDB::get_stacks_epoch_by_epoch_id(sort_db.conn(), &StacksEpochId::Epoch30)? | ||
| .unwrap_or_else(|| panic!("FATAL: no Nakamoto epoch defined")); | ||
| // Find the first Stacks block in this reward cycle's preceding prepare phase. | ||
| // This block will have invoked `.signers.stackerdb-set-signer-slots()` with the reward set. | ||
| // Note that we may not have processed it yet. But, if we do find it, then it's | ||
| // unique (and since Nakamoto Stacks blocks are processed in order, the anchor block | ||
| // cannot change later). | ||
| let first_epoch30_reward_cycle = burnchain | ||
| .block_height_to_reward_cycle(epoch_30.start_height) | ||
| .expect("FATAL: no reward cycle for epoch 3.0 start height"); | ||
| !epoch_at_height | ||
| .epoch_id | ||
| .uses_nakamoto_reward_set(reward_cycle, first_epoch30_reward_cycle) | ||
| }; | ||
| if is_pre_naka_epoch { |
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 am pretty sure that this check could just be simplified to something like:
// if this reward cycle started before 3.0, we need to lookup the
// reward set using 2.x rules.
if epoch_at_height.epoch_id < StacksEpochId::Epoch3_0 {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.
Do we not use the pre-naka reward set calculation in the FIRST epoch 3.0 reward set? Specifically see this comment:
/// Does this epoch use the nakamoto reward set, or the epoch2 reward set?
/// We use the epoch2 reward set in all pre-3.0 epochs.
/// We also use the epoch2 reward set in the first 3.0 reward cycle.
/// After that, we use the nakamoto reward set.
pub fn uses_nakamoto_reward_set(
&self,
cur_reward_cycle: u64,
first_epoch30_reward_cycle: u64,
) -> bool {
match self {
StacksEpochId::Epoch10
| StacksEpochId::Epoch20
| StacksEpochId::Epoch2_05
| StacksEpochId::Epoch21
| StacksEpochId::Epoch22
| StacksEpochId::Epoch23
| StacksEpochId::Epoch24
| StacksEpochId::Epoch25 => false,
StacksEpochId::Epoch30
| StacksEpochId::Epoch31
| StacksEpochId::Epoch32
| StacksEpochId::Epoch33 => cur_reward_cycle > first_epoch30_reward_cycle,
}
}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.
Yep -- but that would be the case because the epoch_at_height variable corresponds to the active epoch of the reward cycle's start block: in the first cycle of Epoch 3.0's case, that block is in 2.5.
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 don't think that is necessarily true though no? I mean the epoch at height 3.0 activation...is epoch 3.0. However...its prepare phase actually happens in Epoch 2.5 meaning by epoch 2.5 reward set calculation standards. So that means it would need to use this specialty function, no?
Signed-off-by: Jacinta Ferrant <jacinta@stackslabs.com>
… into feat/expand-consensus-test-to-support-pre-nakamoto-epochs
3af95b7 to
76edefd
Compare
Replaces #6608