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.

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

Fix/4826 #4840

Merged
merged 28 commits into from
Jun 5, 2024
Merged

Fix/4826 #4840

merged 28 commits into from
Jun 5, 2024

Conversation

jcnelson
Copy link
Member

This fixes #4826 by consolidating the way in which the code loads a Nakamoto-epoch reward set. It now will load it from the Nakamoto chain state, where it is keyed to the PoX anchor block selected in the given burnchain fork, with one exception: the first-ever Nakamoto reward set must be loaded from the sortition DB, since that'll be the only place where it is available.

@jcnelson jcnelson requested review from kantai and obycode May 30, 2024 14:52
@jcnelson jcnelson requested a review from a team as a code owner May 30, 2024 14:52
…kamoto-epoch reward set from the Nakamoto chain state, except for the first-ever Nakamoto reward set which is necessarily a preprocessed reward set. Also, remove all calls to load a preprocessed sortition DB reward set from the Nakamoto coordinator
…oto test infrastructure, instead of loading it from a preprocessed reward set in the sortition DB
…Network to reference the reward cycle and its reward set
…he Nakamoto reward set from the Nakamoto chainstate instead of the sortition DB. Update the caching logic as well to key each cached reward set by anchor block ID
Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mostly looks good to me, my biggest comment is around the burn_height parameter: I think its kind of dangerous the way its currently used, and I'd much prefer at least the provider interface being more explicit about the reward cycle its fetching, but potentially all of the interfaces being more explicit as well (or, perhaps eliminating the parameter in load_nakamoto_reward_set and just using the sortition_tip to lookup the snapshot?)

stackslib/src/chainstate/burn/db/sortdb.rs Outdated Show resolved Hide resolved
stackslib/src/chainstate/nakamoto/coordinator/mod.rs Outdated Show resolved Hide resolved
stackslib/src/chainstate/nakamoto/coordinator/mod.rs Outdated Show resolved Hide resolved
stackslib/src/chainstate/nakamoto/coordinator/mod.rs Outdated Show resolved Hide resolved
@jcnelson jcnelson requested a review from kantai June 3, 2024 21:34
kantai
kantai previously approved these changes Jun 4, 2024
@jcnelson
Copy link
Member Author

jcnelson commented Jun 4, 2024

ping @obycode

obycode
obycode previously approved these changes Jun 4, 2024
Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! LGTM.

Base automatically changed from feat/header-signer-signatures to develop June 4, 2024 21:35
@kantai kantai dismissed stale reviews from obycode and themself June 4, 2024 21:35

The base branch was changed.

@jcnelson jcnelson enabled auto-merge June 4, 2024 22:02
@jcnelson jcnelson requested review from kantai and obycode June 4, 2024 22:02
@jcnelson jcnelson added this pull request to the merge queue Jun 5, 2024
Merged via the queue into develop with commit fe8ba12 Jun 5, 2024
1 check passed
Comment on lines +310 to +321
let reward_cycle = self
.burnchain
.pox_constants
.block_height_to_reward_cycle(
self.burnchain.first_block_height,
self.burn_block.block_height,
)
.ok_or_else(|| {
NakamotoNodeError::SigningCoordinatorFailure(
"Building on a burn block that is before the first burn block".into(),
)
})?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is already merged, but I just noticed that this reward_cycle is unused and wanted to make sure that was intentional. It seems correct to me using the method below to get the cycle.

@blockstack-devops
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Oct 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants