feat(chain): add block merkle root to BlockHeaderInnerLite #2643
Conversation
bowenwang1996
requested review from
evgenykuzyakov,
maxzaver and
SkidanovAlex
as code owners
|
|
core/primitives/src/merkle.rs
Outdated
| @@ -98,6 +98,41 @@ pub fn verify_path<T: BorshSerialize>(root: MerkleHash, path: &MerklePath, item: | |||
| hash == root | |||
| } | |||
|
|
|||
| #[derive(Default, Clone, BorshSerialize, BorshDeserialize)] | |||
| pub struct MerkleTree { | |||
| @@ -2969,6 +2969,13 @@ impl<'a> ChainUpdate<'a> { | |||
| { | |||
| return Err(ErrorKind::InvalidFinalityInfo.into()); | |||
| } | |||
|
|
|||
| let mut block_merkle_tree = | |||
| self.chain_store_update.get_block_merkle_tree(&header.prev_hash)?.clone(); | |||
There was a problem hiding this comment.
It looks like we update merkle tree and then throw it away. Don't we store updated merkle tree somewhere? Should we either use the stored merkle tree to verify the root, or update the stored merkle tree in this code?
There was a problem hiding this comment.
We store it when we process the block. We could potentially do an optimization of storing the updated one when we produce the block, but I don't see the need to do that as it might introduce unnecessary bugs.
| @@ -143,6 +143,9 @@ pub enum ErrorKind { | |||
| /// Invalid VRF proof, or incorrect random_output in the header | |||
| #[fail(display = "Invalid Randomness Beacon Output")] | |||
| InvalidRandomnessBeaconOutput, | |||
| /// Invalid block merkle root. | |||
| #[fail(display = "Invalid Block Merkle Root")] | |||
There was a problem hiding this comment.
Could we elaborate when this error occurs? Does it occur when we already know the merkle tree but then receivd the block header such that the root of the updated merkle tree does not match the one in that we know from the block? Doesn't it imply that we know the block body before its block header? It what cases it is possible?
There was a problem hiding this comment.
This happens when the block merkle root contained in header is invalid -- that is, it doesn't match the one calculated from prev header. The only assumption we are making is that when we process a header its prev header is known. If that's the case the merkle tree can be constructed. If the prev header is not known then process header will fail earlier.
chain/chain/src/store.rs
Outdated
| fn update_and_save_block_merkle_tree(&mut self, header: &BlockHeader) -> Result<(), Error> { | ||
| let prev_hash = header.prev_hash; | ||
| if prev_hash == CryptoHash::default() { | ||
| self.save_block_merkle_tree(header.hash(), MerkleTree::default()); |
There was a problem hiding this comment.
Shouldn't it be a merkle tree with one node, instead of tree with no nodes? For consistency reasons.
There was a problem hiding this comment.
No. We store the merkle tree up to the current block (exclusive), which means for genesis block, there is nothing to store, so the tree is empty.
| @@ -352,8 +352,12 @@ impl Client { | |||
| }; | |||
|
|
|||
| // Get block extra from previous block. | |||
| let prev_block_extra = self.chain.get_block_extra(&head.last_block_hash)?.clone(); | |||
| let prev_block = self.chain.get_block(&head.last_block_hash)?; | |||
| let mut block_merkle_tree = | |||
There was a problem hiding this comment.
For the record. We should start thinking of: #2348
| @@ -155,6 +156,19 @@ impl Message for GetBlock { | |||
| type Result = Result<BlockView, String>; | |||
| } | |||
|
|
|||
| /// Get block with the block merkle tree. Used for testing | |||
| pub struct GetBlockWithMerkleTree(pub BlockIdOrFinality); | |||
There was a problem hiding this comment.
This type seems to exist only to workaround the actix framework. Can we rename it to something like: RequestForBlockWithMerkleTree?
There was a problem hiding this comment.
I would prefer it to be consistent with what we already have, i.e, GetBlock.
core/primitives/src/merkle.rs
Outdated
| @@ -98,6 +98,41 @@ pub fn verify_path<T: BorshSerialize>(root: MerkleHash, path: &MerklePath, item: | |||
| hash == root | |||
| } | |||
|
|
|||
| #[derive(Default, Clone, BorshSerialize, BorshDeserialize)] | |||
| pub struct MerkleTree { | |||
bowenwang1996
force-pushed
the
block-merkle-root
branch
from
70aabc9 to
b1a235a
Compare
Add block merkle root to block header so that we light client can verify transaction outcomes in blocks that are not known by the light client. Resolves #2632.
Test plan
test_merkle_treeandtest_invalid_block_merkle_root.