Block, BlockHeader and Block messages #21
Conversation
…action> This is a general placeholder for now.
Should we serialize out `Block` or leave explicit like so? ¯\_(ツ)_/¯
|
|
||
| /// The hash value of the previous block (header) this | ||
| /// particular block references. | ||
| prev_block: BlockHeaderHash, |
There was a problem hiding this comment.
All these fields except the txns come out of BlockHeader, is there a better way to do this without straight-up serializing BlockHeader etc as the Block message?
There was a problem hiding this comment.
Hmm, what if the Block variant of the Message enum just contains a Block object, and the message codec calls Block::zcash_serialize?
zebra-chain/src/block.rs
Outdated
|
|
||
| impl BlockHeader { | ||
| /// Get the SHA-256d hash in internal byte order of this block header. | ||
| pub fn hash(&self) -> [u8; 32] { |
There was a problem hiding this comment.
Since we have impl From<BlockHeader> for BlockHeaderHash, I think we don't need this function? We can just use BlockHeaderHash::from(block_header) or even block_header.into().
There was a problem hiding this comment.
You're right, removing. I think this was a leftover artifact before we decided to do BlockHeaderHash::from() instead.
zebra-chain/src/block.rs
Outdated
| .expect("The merkle tree of transactions must serialize."); | ||
| Self(hash_writer.finish()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Should these impls (L29-L54) live with the MerkleTree definition in merkle_tree.rs?
There was a problem hiding this comment.
Yeah I think you are correct.
zebra-chain/src/merkle_tree.rs
Outdated
| } | ||
|
|
||
| impl<T> MerkleTree<T> { | ||
| pub fn get_root(&self) -> Sha256 { |
There was a problem hiding this comment.
Is this a duplicate of impl From<MerkleTree> for MerkleRootHash?
There was a problem hiding this comment.
I guess it is, hm.
| use crate::serialization::{SerializationError, ZcashDeserialize, ZcashSerialize}; | ||
|
|
||
| // XXX: Depending on if we implement SproutNoteCommitmentTree or | ||
| // similar, it may be worth it to define a NoteCommitmentTree trait. |
There was a problem hiding this comment.
not a comment scoped to this PR, but branching point for further discussion in a separate issue: we may need a sprout note commitment tree, even if we checkpoint on sapling activation, in order to do sprout-on-groth16 proofs to move old sprout value.
|
I think some of the |
Co-Authored-By: Henry de Valence <hdevalence@hdevalence.ca>
| Block { ref block } => { | ||
| block | ||
| .zcash_serialize(&mut writer) | ||
| .expect("Blocks must serialize."); |
There was a problem hiding this comment.
I think this one can error (unlike the hash-writer case, where we know the writer is infallible) if there's an error in the underlying writer, so maybe this bubble the error with ??
Fix bugs identified in test/stage of big refactor
Resolves #7