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 wrong param in prevalidate #19035

Merged
merged 1 commit into from
Dec 12, 2024
Merged

Conversation

almogdepaz
Copy link
Contributor

Purpose:

fix a bug where we pass the wrong blockchain data structure into get_next_sub_slot_iters_and_difficulty

Current Behavior:

we pass self.blockchain instead of the AugmentedBlockchain param passed into get_next_sub_slot_iters_and_difficulty
this means that we rely on having the blocks in the cache, which now happens when we call add blocks but is redundant since we also use AugmentedBlockchain to stage the new blocks before we switch to the new fork

New Behavior:

pass the correct parameter

Testing Notes:

@almogdepaz almogdepaz added full_node Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog labels Dec 12, 2024
@almogdepaz almogdepaz marked this pull request as ready for review December 12, 2024 11:08
@almogdepaz almogdepaz requested a review from a team as a code owner December 12, 2024 11:08
@almogdepaz almogdepaz requested a review from arvidn December 12, 2024 11:09
Copy link
Contributor

@arvidn arvidn left a comment

Choose a reason for hiding this comment

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

we really should have a test that exercise this. I'm a bit surprised the deep reorg tests don't actually. We have a case where we switch from lighter blocks to heavier ones, which I believe should affect the difficulty

@almogdepaz
Copy link
Contributor Author

we really should have a test that exercise this. I'm a bit surprised the deep reorg tests don't actually. We have a case where we switch from lighter blocks to heavier ones, which I believe should affect the difficulty

we added the AugmentedBlockchain but didn't all the code paths that support the old cache flow, so we have a safety net that allows this to work so i am not sure we can write a test that throws here but we do keep redundant data and this can lead to bugs if someone changes the code expecting it to work entirely with AugmentedBlockchain

@pmaslana pmaslana merged commit 2982667 into main Dec 12, 2024
364 of 365 checks passed
@pmaslana pmaslana deleted the fix_bug_in_prevalidated_blocks branch December 12, 2024 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog full_node
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants