feat(consensus): eliminate block ties #673
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
msbrogli
force-pushed
the
feat/consensus-change
branch
3 times, most recently
from
5598803 to
8abec23
Compare
msbrogli
force-pushed
the
feat/consensus-change
branch
from
8abec23 to
5026f34
Compare
msbrogli
force-pushed
the
feat/consensus-change
branch
from
5026f34 to
d4dc39e
Compare
jansegre
force-pushed
the
feat/consensus-change
branch
from
d4dc39e to
807d778
Compare
jansegre
force-pushed
the
feat/consensus-change
branch
5 times, most recently
from
7e4b8b8 to
e516014
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #673 +/- ##
==========================================
- Coverage 84.70% 84.61% -0.10%
==========================================
Files 313 313
Lines 24249 24256 +7
Branches 3690 3693 +3
==========================================
- Hits 20541 20525 -16
- Misses 2997 3009 +12
- Partials 711 722 +11 ☔ View full report in Codecov by Sentry. |
msbrogli
commented
Aug 9, 2024
| for tx in txs2: | ||
| meta = tx.get_metadata(force_reload=True) | ||
| self.assertIsNone(meta.first_block) | ||
| # for tx in txs2: |
There was a problem hiding this comment.
Why is this part of the code commented out?
There was a problem hiding this comment.
I'm actually not too sure, I'll review this part.
| @@ -624,6 +624,11 @@ def get_best_block_tips(self, timestamp: Optional[float] = None, *, skip_cache: | |||
| elif meta.score > best_score: | |||
| best_score = meta.score | |||
| best_tip_blocks = [block_hash] | |||
|
|
|||
| # XXX: if there's more than one we filter it so it's the smallest hash | |||
| if len(best_tip_blocks) > 1: | |||
There was a problem hiding this comment.
Should we allow the caller to choose whether they want to get just one or all of them?
There was a problem hiding this comment.
I think that could make sense as a new method, like get_all_block_tips, instead of get_best_block_tips. But currently that method wouldn't be used anywhere else, and when removing the tips indexes, it would be more convenient if we didn't have to maintain that method.
| @@ -624,6 +624,11 @@ def get_best_block_tips(self, timestamp: Optional[float] = None, *, skip_cache: | |||
| elif meta.score > best_score: | |||
| best_score = meta.score | |||
| best_tip_blocks = [block_hash] | |||
|
|
|||
| # XXX: if there's more than one we filter it so it's the smallest hash | |||
| if len(best_tip_blocks) > 1: | |||
There was a problem hiding this comment.
Should we create an issue to refactor this method and simply store the current best block in the storage?
There was a problem hiding this comment.
#540 sort of does this by using the height-index, but because it's not using this PR as a base it does this incorrectly and hides other tips, after this PR that change would be correct
jansegre
changed the title
jansegre
force-pushed
the
feat/score-acc-weight-types
branch
from
17c1ffc to
93d9f89
Compare
jansegre
force-pushed
the
feat/consensus-change
branch
from
e516014 to
1b93f4d
Compare
jansegre
force-pushed
the
feat/score-acc-weight-types
branch
from
93d9f89 to
da4ac0d
Compare
jansegre
force-pushed
the
feat/score-acc-weight-types
branch
from
da4ac0d to
b74fa0d
Compare
jansegre
force-pushed
the
feat/consensus-change
branch
from
1b93f4d to
b9b2390
Compare
jansegre
force-pushed
the
feat/score-acc-weight-types
branch
from
b74fa0d to
45884e3
Compare
jansegre
force-pushed
the
feat/consensus-change
branch
from
b9b2390 to
2dddd20
Compare
jansegre
force-pushed
the
feat/score-acc-weight-types
branch
from
45884e3 to
b21d67c
Compare
jansegre
force-pushed
the
feat/score-acc-weight-types
branch
2 times, most recently
from
2f2d7a8 to
1a3e1d0
Compare
jansegre
force-pushed
the
feat/score-acc-weight-types
branch
2 times, most recently
from
4a182f4 to
d870605
Compare
jansegre
force-pushed
the
feat/score-acc-weight-types
branch
2 times, most recently
from
fb35bfb to
17eac7c
Compare
jansegre
force-pushed
the
feat/consensus-change
branch
from
2dddd20 to
3f18210
Compare
jansegre
force-pushed
the
feat/score-acc-weight-types
branch
from
17eac7c to
26a36c0
Compare
jansegre
force-pushed
the
feat/consensus-change
branch
from
3f18210 to
e7b3d46
Compare
jansegre
force-pushed
the
feat/consensus-change
branch
from
e7b3d46 to
2aa27e6
Compare
|
Bencher Report| Branch | feat/consensus-change |
| Testbed | ubuntu-22.04 |
🚨 1 Alert
| Benchmark | Measure Units | View | Benchmark Result (Result Δ%) | Lower Boundary (Limit %) |
|---|---|---|---|---|
| sync-v2 (up to 20000 blocks) | Latency minutes (m) | 📈 plot 🚨 alert (🔔) 🚷 threshold | 1.48 (-10.71%) | 1.49 (100.79%) |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result minutes (m) (Result Δ%) | Lower Boundary minutes (m) (Limit %) | Upper Boundary minutes (m) (Limit %) |
|---|---|---|---|---|
| sync-v2 (up to 20000 blocks) | 📈 view plot 🚨 view alert (🔔) 🚷 view threshold | 1.48 (-10.71%) | 1.49 (100.79%) | 1.82 (81.18%) |
jansegre
force-pushed
the
feat/consensus-change
branch
from
2aa27e6 to
ec17e5b
Compare
1 task
Co-authored-by: Jan Segre <jan@hathor.network>
jansegre
force-pushed
the
feat/consensus-change
branch
from
a0b589a to
4bf80a4
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
We've always supported "block ties", meaning that blocks that have the same score will tie and both void each other, while at the same time having to remain as "best block tips" (which always had to be a list). This pushes some unnecessary complexity throughout the code. There are no disadvantages to using the block hash as a tie-breaker and simplifying this complexity.
In practice there's a slight difference in the consensus of certain situations, the difference is that before some tied block chains would be voided all the way back until they weren't tied anymore, now one of them would be picked as best chain. This situation does not cause any incompatibility in how nodes sync and any new block with higher score will cause the same state on both chains.
This PR is the first step, that only focuses on removing ties at the consensus level. Subsequent PRs will focus on refactoring out the complexity of supporting multiple tips.
Acceptance criteria
update_voided_infoso when testing if a new block might be a winner, and its score is neither higher or lower than the current winner (that is, a tie), the hash is used as a tie breakerupdate_voided_infoso when a new block is not a winner, only the tip with highest score and lowest hash is stored as "best block tips" cacheget_best_block_tipsso it only ever returns a list of size 1, when the block tips index returns more than one tip with the highest score, the one if lowest hash is selectedtest_split_brain_only_blocks_different_heightso it checks the node will correctly not void everything back to the genesis and tie the two blocks (when before it used to)test_single_fork_not_bestsimilarly, so it checks that the best chain is considered