Amend hashForkNo and document fork number #22
Merged
Conversation
This file contains hidden or 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
ninioArtillero
force-pushed
the
ninioArtillero/chain-path-utility
branch
from
03ca80b to
9bee54f
Compare
ninioArtillero
changed the title
ninioArtillero
force-pushed
the
ninioArtillero/chain-path-utility
branch
2 times, most recently
from
c3142a0 to
e0fa207
Compare
ninioArtillero
force-pushed
the
ninioArtillero/chain-path-utility
branch
from
e0fa207 to
13e2f0e
Compare
isovector
reviewed
Jan 8, 2026
ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/PeerSimulator/StateDiagram.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/PeerSimulator/StateDiagram.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/PeerSimulator/StateDiagram.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/PeerSimulator/StateDiagram.hs
Outdated
Show resolved
Hide resolved
isovector
reviewed
Jan 8, 2026
ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/PeerSimulator/StateDiagram.hs
Outdated
Show resolved
Hide resolved
ninioArtillero
force-pushed
the
ninioArtillero/chain-path-utility
branch
3 times, most recently
from
648cc2b to
f525394
Compare
ninioArtillero
force-pushed
the
ninioArtillero/chain-path-utility
branch
from
f525394 to
cd80cc6
Compare
nbloomf
approved these changes
Jan 9, 2026
Member
nbloomf
left a comment
nbloomf
left a comment
There was a problem hiding this comment.
Makes sense to me. Thank you for tackling this and for the great documentation!
ninioArtillero
added a commit
that referenced
this pull request
Jan 9, 2026
This reverts commit 7449d2f.
isovector
pushed a commit
that referenced
this pull request
Jan 9, 2026
# Description This PR fixes `hashForkNo` as previous behavior uniquely identified branches by fork number and the new parameterized implementation did not. This seems relevant because otherwise `BlockTreeBranch`es forking from the same block in the trunk would end up with the same fork number. As it turns out, these _fork numbers_ are not related to the order in which forking takes place. Related documentation was added. This seem to be related to an apparent mismatch. On the one hand, the `Test.Util.TestBlock.TestHash` documentation states that multiple branches can steem from a single node in a tree and _suggests_ that the fork number indexes branches in an orderly way. On the other hand, the implementation of `GenesisTest` generation in `Test.Consensus.Genesis.Setup.GenChains` assigns fork numbers to branches in an arbitrary way.
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.
Description
This PR fixes
hashForkNoas previous behavior uniquely identified branches by fork number and the new parameterized implementation did not. This seems relevant because otherwiseBlockTreeBranches forking from the same block in the trunk would end up with the same fork number. As it turns out, these fork numbers are not related to the order in which forking takes place. Related documentation was added.This seem to be related to an apparent mismatch. On the one hand, the
Test.Util.TestBlock.TestHashdocumentation states that multiple branches can steem from a single node in a tree and suggests that the fork number indexes branches in an orderly way. On the other hand, the implementation ofGenesisTestgeneration inTest.Consensus.Genesis.Setup.GenChainsassigns fork numbers to branches in an arbitrary way.