Skip to content

fix(storage): Fix wrong update to the best_block_tips_cache#874

Merged
msbrogli merged 1 commit intomasterfrom
fix/best_block_tips-cache
Nov 13, 2023
Merged

fix(storage): Fix wrong update to the best_block_tips_cache#874
msbrogli merged 1 commit intomasterfrom
fix/best_block_tips-cache

Conversation

@msbrogli
Copy link
Member

@msbrogli msbrogli commented Nov 11, 2023

Motivation

The tests/simulation/test_simulator.py::SyncV2RandomSimulatorTestCase::test_many_miners_since_beginning was flaky because of this issue.

When the consensus algorithm updates the best_block_tips_cache, it does not include the current block being evaluated. Notice that the current block is not part of the heads.

Acceptance Criteria

  1. Include block to the list of best block tips used to update the best_block_tips_cache.
  2. In the assertTipsEqualSyncV2(), we shouldn't assert b1 == b2 because the height index keeps its previous entry when two or more blocks have the same score. So we can safely just check if b1 in s1 and b2 in s2.
  3. Notice that, when len(s1) == len(s2) == 1, checking if b1 in s1 and b2 in s2 is exactly the same as checking if b1 == b2.

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

@msbrogli msbrogli requested a review from jansegre as a code owner November 11, 2023 07:23
@msbrogli msbrogli self-assigned this Nov 11, 2023
@msbrogli msbrogli added the bug Something isn't working label Nov 11, 2023
@msbrogli msbrogli requested a review from glevco November 11, 2023 07:23
storage.update_best_block_tips_cache([not_none(blk.hash) for blk in heads])
best_block_tips = [not_none(blk.hash) for blk in heads]
best_block_tips.append(not_none(block.hash))
storage.update_best_block_tips_cache(best_block_tips)
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we just set to None and let the next call cache it?

Copy link
Member

Choose a reason for hiding this comment

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

Either way is fine for me. I wish we can get rid of this cache in the future. Will we even need it if we don't have best block ties, because even for sync-v1 the block height index would have a ready O(1) (or at least however fast best_blocks[-1] is) response?

@codecov
Copy link

codecov bot commented Nov 11, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (090bd71) 85.42% compared to head (31c98fb) 85.20%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #874      +/-   ##
==========================================
- Coverage   85.42%   85.20%   -0.23%     
==========================================
  Files         281      281              
  Lines       22353    22355       +2     
  Branches     3388     3388              
==========================================
- Hits        19096    19048      -48     
- Misses       2582     2622      +40     
- Partials      675      685      +10     
Files Coverage Δ
hathor/consensus/block_consensus.py 95.15% <100.00%> (+0.73%) ⬆️

... and 9 files with indirect coverage changes

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

@jansegre jansegre mentioned this pull request Nov 13, 2023
2 tasks
@msbrogli msbrogli force-pushed the fix/best_block_tips-cache branch from 31c98fb to 13e49bf Compare November 13, 2023 19:33
@msbrogli msbrogli merged commit 13e49bf into master Nov 13, 2023
@msbrogli msbrogli deleted the fix/best_block_tips-cache branch November 13, 2023 19:33
This was referenced Dec 20, 2023
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