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(feature-activation): fix get ancestor #930

Merged
merged 1 commit into from
Jan 26, 2024

Conversation

glevco
Copy link
Contributor

@glevco glevco commented Jan 19, 2024

Motivation

I noticed there was a bug in the _get_ancestor_at_height() method of Feature Activation. It receives a block and an ancestor height, and returns the ancestor block with that height. In current code, it checks whether the block is voided, and if it's not, it's in the best blockchain, so we can use the height index. Otherwise, it would iterate over all parents using block.get_block_parent(), which is less performatic than using the index.

The issue is that this method may be called before the consensus runs for this block. This means that we do not know if the block is part of the best blockchain, and checking not meta.voided_by is wrong. This PR fixes this issue simply by forwarding the existing rule to the first parent block, which is guaranteed to have gone through the consensus already.

This indicates that it would be beneficial to refactor metadata in a way that this bug would not be possible. We shouldn't be able to check meta.voided_by for a vertex that hasn't been through the consensus. We should have different metadata types depending on the state of the vertex.

Note: Even if the loop ends up running, which happens only once for each Evaluation Interval (1 week) and only if the block is not in the best chain, it's not that slow. I've tested it informally inside a running local testnet node, with 20160 iterations (the maximum possible):

Screenshot 2024-01-19 at 14 22 03

Acceptance Criteria

  • Change the _get_ancestor_at_height() method so it gets the ancestor from the first parent block, instead of from the block itself.
  • Change the _get_ancestor_iteratively() function to a method to add an assertion.

Checklist

  • If you are requesting a merge into master, confirm this code is production-ready and can be included in future releases as soon as it gets merged

@glevco glevco added the bug Something isn't working label Jan 19, 2024
@glevco glevco self-assigned this Jan 19, 2024
@glevco glevco force-pushed the fix/feature-activation/fix-get-ancestor branch from fbfe2db to c786568 Compare January 19, 2024 17:48
@glevco glevco marked this pull request as ready for review January 19, 2024 17:50
Copy link

codecov bot commented Jan 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8cc0f3f) 85.27% compared to head (b003af4) 85.36%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #930      +/-   ##
==========================================
+ Coverage   85.27%   85.36%   +0.08%     
==========================================
  Files         288      288              
  Lines       22392    22397       +5     
  Branches     3372     3373       +1     
==========================================
+ Hits        19095    19119      +24     
+ Misses       2619     2608      -11     
+ Partials      678      670       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@glevco glevco force-pushed the fix/feature-activation/fix-get-ancestor branch from c786568 to b066d05 Compare January 19, 2024 22:08
@glevco glevco force-pushed the fix/feature-activation/fix-get-ancestor branch from b066d05 to b003af4 Compare January 26, 2024 14:47
@glevco glevco merged commit 2be371f into master Jan 26, 2024
12 checks passed
@glevco glevco deleted the fix/feature-activation/fix-get-ancestor branch January 26, 2024 15:40
@jansegre jansegre mentioned this pull request Jan 30, 2024
2 tasks
@jansegre jansegre mentioned this pull request Feb 26, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants