Block header parsing and transaction ID computation #66

str4d · 2019-02-26T21:04:34Z

No description provided.

ebfull

Just fun complaints, nothing urgent. Nice PR!

ebfull · 2019-02-27T02:24:02Z

Cargo.lock

@@ -501,6 +576,7 @@ dependencies = [
 "checksum num-traits 0.2.5 (registry+https://github.com/rust-lang/crates.io-index)" = "630de1ef5cc79d0cdd78b7e33b81f083cbfe90de0f4b2b2f07f905867c70e9fe"
 "checksum num_cpus 1.8.0 (registry+https://github.com/rust-lang/crates.io-index)" = "c51a3322e4bca9d212ad9a158a02abc6934d005490c054a2778df73a70aa0a30"
 "checksum opaque-debug 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)" = "d620c9c26834b34f039489ac0dfdb12c7ac15ccaf818350a64c9b5334a452ad7"
+"checksum opaque-debug 0.2.2 (registry+https://github.com/rust-lang/crates.io-index)" = "93f5bb2e8e8dec81642920ccff6b61f1eb94fa3020c5a325c9851ff604152409"


We're bringing in two different versions of this crate apparently.

The older version is coming in through the aes crate, which is used via the fpe by zip32. We can bump fpe to the latest aes crate, which will resolve this discrepancy.

I've opened str4d/fpe#9 with this change, along with some minor cleanups and improvements. I've confirmed that it reduces the number of dependencies by three (two of which were older versions of crates we still depend on).

ebfull · 2019-02-27T02:31:38Z

zcash_primitives/Cargo.toml

@@ -8,10 +8,12 @@ authors = [
 [dependencies]
 byteorder = "1"
 ff = { path = "../ff" }
+hex = "0.3"


Sigh! Rust should have added a hex literal.

ebfull · 2019-02-27T02:34:10Z

zcash_primitives/src/block.rs

+
+impl fmt::Display for BlockHash {
+    fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
+        let mut data = self.0.to_vec();


Instead, you could initialize a local clone of the self.0: [u8; 32] and then [T]::reverse() on that, rather than allocating.

(Not blocking.)

ebfull · 2019-02-27T02:38:13Z

zcash_primitives/src/transaction/mod.rs

+
+impl fmt::Display for TxId {
+    fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
+        let mut data = self.0.to_vec();


Same as the other review comment about something similar to this.

Fix comment

str4d · 2019-03-07T23:28:39Z

Rebased on master to fix merge conflicts. Now the only merge conflicts will be trivial ones with #68 and #69, due to adding modules to the same file in zcash_primitives.

str4d · 2019-03-07T23:43:00Z

Rebased the last few commits again because I forgot to correct the Cargo.lock after rebasing.

Eirik0 · 2019-03-19T16:41:25Z

zcash_primitives/src/block.rs

 use std::ops::Deref;

 #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
 pub struct BlockHash(pub [u8; 32]);

+impl fmt::Display for BlockHash {


Aside from the name, BlockHash is the same as TxId and in this commit we write the same fn fmt twice. This makes me wonder if there should be a single type (named ByteArray32 or something like that) rather than two.

I'm not convinced that the extra indirection to the underlying bytes is worth it yet. If we find the reverse-hex format is bleeding in too much from zcashd then we can revisit this.

zcash_primitives/src/transaction/mod.rs

Eirik0 · 2019-03-19T16:54:36Z

zcash_primitives/src/transaction/mod.rs

    }
 }

 impl Transaction {
+    fn from_data(data: TransactionData) -> io::Result<Self> {
+        let mut tx = Transaction(data, TxId([0; 32]));


It feels we are going back and forth a bit having to construct the transaction and write it's data to an array before we can set the txid. It would be possible to do this all at once, but maybe it's not worth the trouble.

By design, I chose to only allow serialization of complete transactions (so that the transaction format is not used for storing "partially-complete transactions", which greatly complicates the job of the transaction parser). Thus it is necessary either to do what we do here, or to have the transaction serializer be a standalone function that takes all the transaction components as inputs. I don't think the complexity of the latter is worth it; the txid field of Transaction is private, so we know that only this module (and submodules) will be able to see and/or modify it directly.

This could be done by moving the write<W: Write>(&self, mut writer: W) function from Transaction to TransactionData, but I could see this being beyond the scope of this PR and we can revisit it later.

zcash_primitives/src/transaction/mod.rs

str4d · 2019-04-02T00:36:20Z

Addressed @Eirik0's comments (and I'd addressed @ebfull's before that).

hdevalence · 2019-04-02T19:47:59Z

zcash_primitives/src/block.rs

+    pub version: i32,
+    pub prev_block: BlockHash,
+    pub merkle_root: [u8; 32],
+    pub final_sapling_root: [u8; 32],


Not sure if this is out of scope for the PR, but the BlockHash is a typed wrapper for a [u8; 32]; are the merkle_root and final_sapling_root comparable 32-byte arrays, or semantically-different types? Should they also be newtype wrappers?

They are all hash outputs, but of at least two different hash functions. In zcashd they are represented by the same 256-bit binary blob type, which has the "feature" that when rendered in hex in zcashd RPC output, the bytes are reversed (behaviour inherited from Bitcoin Core).

I use separate types for block hashes and transaction IDs because those are the only two that external code is likely to actually interact with, whereas other block header fields (thus far) will only be used internally. Once the refactor is further along and we get to the stage of cleaning up the crate APIs, we should revisit this.

Opened #73.

hdevalence · 2019-04-02T20:03:28Z

zcash_primitives/src/block.rs

+        reader.read_exact(&mut merkle_root)?;
+
+        let mut final_sapling_root = [0; 32];
+        reader.read_exact(&mut final_sapling_root)?;


I don't think there's anything wrong with this code, but I get a little anxious about the let mut bytes = zeros; read bytes; pattern, since (considering the reading, not the zeroing, as the "real" initialization) the variable initialization is unlinked from its declaration, and the resulting variable is left mutable even if it doesn't need to be.

Since reading 32 bytes is a pretty common idiom, maybe it would make sense to define some extension behaviour like so:

pub trait ReadExt { fn read_32_bytes(&mut self) -> io::Result<[u8; 32]>; } impl<R: Read> ReadExt for R { #[inline] fn read_32_bytes(&mut self) -> io::Result<[u8; 32]> { let mut bytes = [0; 32]; self.read_exact(&mut bytes)?; Ok(bytes) } }

so that this byte-handling code can become, e.g.,

let prev_block = BlockHash(reader.read_32_bytes()?); let merkle_root = reader.read_32_bytes()?; let final_sapling_root = reader.read_32_bytes()?;

This is a commonly-used idiom throughout the existing codebase, so we should address this comprehensively. Opened #72

Eirik0

utACK. My comments are non-blocking.

ebfull approved these changes Feb 27, 2019

View reviewed changes

CryptoForge pushed a commit to CryptoForge/librustzcash that referenced this pull request Mar 3, 2019

Merge pull request zcash#66 from str4d/fix-spend-nf-comment

6e08a5a

Fix comment

str4d added 2 commits March 7, 2019 23:26

Block header representation

e21be37

TxId struct

20d5cdc

str4d force-pushed the block-header branch from 9d1da20 to 6a06389 Compare March 7, 2019 23:27

str4d added 3 commits March 7, 2019 23:41

impl Display for BlockHash and TxId

a1664c6

Block header serialisation

670bb27

Compute TxId for Transaction

4289843

str4d force-pushed the block-header branch from 6a06389 to 4289843 Compare March 7, 2019 23:42

Reverse a clone of [u8; 32] instead of allocating

b856d23

Eirik0 reviewed Mar 19, 2019

View reviewed changes

str4d added 3 commits April 2, 2019 01:29

Use named fields in Transaction struct

663f9d6

Test Transaction::txid()

3501365

cargo fmt

6c99d71

hdevalence reviewed Apr 2, 2019

View reviewed changes

Eirik0 approved these changes Apr 2, 2019

View reviewed changes

This was referenced Apr 5, 2019

Refactor to extract common read operations #72

Open

Consider type semantics of 32-byte arrays in crate APIs #73

Open

str4d merged commit d7ba310 into zcash:master Apr 5, 2019

str4d deleted the block-header branch April 5, 2019 19:54

str4d added this to the v0.1.0 milestone Aug 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block header parsing and transaction ID computation #66

Block header parsing and transaction ID computation #66

str4d commented Feb 26, 2019

ebfull left a comment

ebfull Feb 27, 2019

str4d Mar 7, 2019

str4d Mar 8, 2019

ebfull Feb 27, 2019

ebfull Feb 27, 2019

ebfull Feb 27, 2019

str4d commented Mar 7, 2019

str4d commented Mar 7, 2019

Eirik0 Mar 19, 2019

str4d Apr 2, 2019

Eirik0 Mar 19, 2019

str4d Apr 1, 2019

Eirik0 Apr 2, 2019

str4d commented Apr 2, 2019

hdevalence Apr 2, 2019

str4d Apr 5, 2019

str4d Apr 5, 2019

hdevalence Apr 2, 2019 •

edited

Loading

str4d Apr 5, 2019

Eirik0 left a comment

Block header parsing and transaction ID computation #66

Block header parsing and transaction ID computation #66

Conversation

str4d commented Feb 26, 2019

ebfull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

str4d commented Mar 7, 2019

str4d commented Mar 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

str4d commented Apr 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hdevalence Apr 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eirik0 left a comment

Choose a reason for hiding this comment

hdevalence Apr 2, 2019 •

edited

Loading