Skip to content

Fix fork choice rule to prefer the heaviest chain#1513

Merged
nazar-pc merged 1 commit intomainfrom
fix-fork-choice-rule
Jun 8, 2023
Merged

Fix fork choice rule to prefer the heaviest chain#1513
nazar-pc merged 1 commit intomainfrom
fix-fork-choice-rule

Conversation

@nazar-pc
Copy link
Copy Markdown
Member

@nazar-pc nazar-pc commented Jun 7, 2023

Previously we were deriving block weight from solution quality, which was actually wrong, it should have been derived from corresponding solution range instead.

Also in case node is presented with two forks that have the same weight, it'll now prefer the first block seen, rather the higher block number.

Resolves #1505

Code contributor checklist:

vedhavyas
vedhavyas previously approved these changes Jun 7, 2023
@nazar-pc
Copy link
Copy Markdown
Member Author

nazar-pc commented Jun 7, 2023

@vedhavyas I forgot to update fork choice rules in sp-lightclient which caused tests to fail. If you ignore whitespace changes, changes are quite straightforward. I also reused the function to compute weight, logic was duplicated in sp-lightclient, if it wasn't, it would fail to compile from the beginning.

I have renamed test_chain_reorg_to_longer_chain to test_chain_reorg_to_heavier_chain, but then also noticed it is not actually doing reorgs, it just extends existing chain. Did you mean to do it like that? I see that the chain is not marked as longest, but when I read reorg, I expect switch from one tip to the other tip, typically the same height. In that test we have one branch only.

@nazar-pc nazar-pc requested a review from vedhavyas June 7, 2023 15:41
@vedhavyas
Copy link
Copy Markdown
Contributor

I have renamed test_chain_reorg_to_longer_chain to test_chain_reorg_to_heavier_chain, but then also noticed it is not actually doing reorgs, it just extends existing chain. Did you mean to do it like that?

It seems to re-orgin as expected. In the test here it will create a fork chain at block height 5 till height 8 but not the canonical yet.
And we confirm that here and there are indeed two forks at that height.

Then we extend the fork by another block, height 9, here. Notice the is_best parameter is set to true so that add_headers_to_chain will make necessary overrides to make block 9 win the forck choice rule and there by becoming best header. We verify that here and also ensure there are no forks at block 5 which would be finalized as per K-deep rule.

@nazar-pc
Copy link
Copy Markdown
Member Author

nazar-pc commented Jun 8, 2023

I have missed hash_of_5 somehow when reading it, thanks for explaining!

@nazar-pc nazar-pc merged commit a92f469 into main Jun 8, 2023
@nazar-pc nazar-pc deleted the fix-fork-choice-rule branch June 8, 2023 08:19
i1i1 added a commit to autonomys/subspace-pulsar-sdk that referenced this pull request Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Heaviest chain vs longest chain

2 participants