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/4793 #4817

Merged
merged 33 commits into from
May 28, 2024
Merged

Fix/4793 #4817

merged 33 commits into from
May 28, 2024

Conversation

jcnelson
Copy link
Member

This PR updates the Nakamoto chainstate and networking stack to use the reward cycle's signers instead of the aggregate public key to authenticate Nakamoto blocks (#4793). In order to do this, this PR also fixes #4813.

Note that the fix for #4813 does not need a schema migration for nodes today which have erroneously stored a SelectedAndUnknown reward set, because the impact of doing so is nonexistent. This would only have required a schema migration if the last reward 2.5 cycle was erroneously set to SelectedAndUnknown.

jcnelson added 21 commits May 22, 2024 15:39
…til a later date, and it's in the git history so we can fetch it later)
…ortition tip. Do not attempt to get the canonical stacks or burnchain tips.
…est module, where it is still required for test coverage
@jcnelson jcnelson requested a review from hstove May 22, 2024 21:32
@jcnelson jcnelson requested a review from a team as a code owner May 22, 2024 21:32
jcnelson added 7 commits May 22, 2024 17:38
…t the block in reward cycle N at reward cycle index 0 was signed by the signers of reward cycle N - 1
…t an extension of a previously-started tenure, or a newly-started tenure), and clarify that the existing API for getting the highest Nakamoto tenure only pertains to the highest *started* tenure (not extended)
@jcnelson jcnelson requested a review from kantai May 23, 2024 19:32
…ed reward set of the anchor block is not yet known
Copy link
Contributor

@hstove hstove left a comment

Choose a reason for hiding this comment

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

Overall LGTM! I appreciate the discrete commits, it made it easier to review. Also good work with the improvements of the API for fetching a reward set.

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.

Overall LGTM, just a few comments.

Comment on lines +5486 to +5487
self.current_reward_sets.insert(rc, reward_cycle_info);
self.current_reward_set_ids.insert(rc, reward_cycle_sort_id);
Copy link
Member

Choose a reason for hiding this comment

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

Would current_reward_sets: BTreeMap<u64, (SortitionId, RewardCycleInfo)> work for tracking the information? Or is there a case where current_reward_sets.get(x).is_some() && current_reward_set_ids.get(x).is_none()?

Copy link
Member Author

Choose a reason for hiding this comment

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

The downloader logic expects HashMap<u64, RewardCycleInfo> all over the place, and it's cleaner to just store these two data in separate tables than force the downloader to deconstruct the tuple each time it wants to access the reward cycle info.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, if its really easier, that's fine, but please add to both docstrings that there's a 1-1 correspondence between entries in current_reward_sets and current_reward_set_ids, which is an invariant that must be enforced by any method that adds or removes elements from those sets.

@jferrant
Copy link
Collaborator

I really only have non blocking nits. aside from that, if we don;t plan to keep v1 tests in working state, might want to remove filter_bad_transactions from CI. Otherwise, maybe we can maintain this test?

@jcnelson jcnelson requested a review from a team as a code owner May 24, 2024 18:38
@jcnelson jcnelson requested review from jferrant, kantai and hstove May 24, 2024 18:39
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 LGTM, just had a comment that the 1-1 invariant between current_reward_sets and current_reward_set_ids needs to be made explicit in the variables' docstrings.

@jcnelson jcnelson merged commit a70bfb5 into feat/header-signer-signatures May 28, 2024
1 check passed
@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.

5 participants