-
Notifications
You must be signed in to change notification settings - Fork 22
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
Revise hashing to support Merkle proofs #1110
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only very minor comments
concordium-consensus/src/Concordium/GlobalState/Basic/BlockState/LFMBTree.hs
Outdated
Show resolved
Hide resolved
concordium-consensus/src/Concordium/GlobalState/Persistent/Instances.hs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
I think we should add some tests for the provable properties of the block.
concordium-consensus/src/Concordium/GlobalState/Basic/BlockState/LFMBTree.hs
Show resolved
Hide resolved
concordium-consensus/src/Concordium/GlobalState/Basic/BlockState/LFMBTree.hs
Outdated
Show resolved
Hide resolved
concordium-consensus/src/Concordium/GlobalState/Basic/BlockState/LFMBTree.hs
Outdated
Show resolved
Hide resolved
concordium-consensus/src/Concordium/GlobalState/Basic/BlockState/LFMBTree.hs
Outdated
Show resolved
Hide resolved
concordium-consensus/src/Concordium/GlobalState/Basic/BlockState/LFMBTree.hs
Show resolved
Hide resolved
concordium-consensus/src/Concordium/Types/TransactionOutcomes.hs
Outdated
Show resolved
Hide resolved
concordium-consensus/tests/globalstate/GlobalStateTests/Instances.hs
Outdated
Show resolved
Hide resolved
concordium-consensus/tests/globalstate/GlobalStateTests/Instances.hs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this suffers from some of the same as the related PR in base.
Ideally this would be 2 pull requests, one changing the hashing scheme, and the other one doing the Merkle proof production.
Purpose
This involves several changes to support hashing blocks to support Merkle proofs. Primarily, these involves updating how
LFMBTree
hashes are computed for the new P7 protocol version.Changes
CapitalDistribution
is hashed differently depending onBlockHashVersion
, which determines the LFMBTree hashing scheme to use.PoolRewards
parametrised byBlockHashVersion
. The next and current capital distribution hashes are dependent on theBlockHashVersion
, as well as the hashing of thebakerPoolRewardDetails
LFMBTree.Concordium.GlobalState.Basic.BlockState.PoolRewards
was not used, the remaining useful definitions are lifted toConcordium.GlobalState.PoolRewards
.AccountsHash
,ModulesHash
andInstancesHash
types for the accounts, modules and instances tables, which are parametrised by the protocol version.BlockRewardDetails
is revised to parametrise by theBlockHashVersion
.TransactionOutcomes
are moved from base into a newConcordium.Types.TransactionOutcomes
.PersistentTransactionOutcomes
supports a newTransactionOutcomesVersion
that uses the new LFMBTree hashing scheme. ATransactionOutcomesHashV
that is parametrised by theTransactionOutcomesVersion
is introduced for hashing thePersistentTransactionOutcomes
.TransactionOutcomesHash
is still used in places where the version parameter is inconvenient or unnecessary.newContractInstanceIT
when inserting in a table with vacancies.newContractInstance
anddeleteContractInstance
.MerkleProvable
forBakedBlock
, supporting deriving Merkle proofs about a range of block attributes.Checklist
hard-to-understand areas.