Transaction primitives #39
Conversation
str4d
force-pushed
the
zcash-transaction-primitives
branch
from
3cdc0dd to
1f11c40
Compare
| writer.write_u8(254)?; | ||
| writer.write_u32::<LittleEndian>(s as u32) | ||
| } | ||
| s => { |
There was a problem hiding this comment.
Should this fail if s > 0x02000000?
There was a problem hiding this comment.
There was a problem hiding this comment.
Do we know what the rationale was for that?
(I'd be terrified to change it, mind :-) )
zcash_primitives/src/serialize.rs
Outdated
| n => Ok(n as usize), | ||
| } | ||
| }? { | ||
| s if s > 0x02000000 => Err(io::Error::new( |
There was a problem hiding this comment.
Perhaps make a constant for 0x02000000?
There was a problem hiding this comment.
It's called MAX_SIZE in src/serialize.h.
zcash_primitives/src/serialize.rs
Outdated
| n => Ok(n as usize), | ||
| } | ||
| }? { | ||
| s if s > 0x02000000 => Err(io::Error::new( |
There was a problem hiding this comment.
It's called MAX_SIZE in src/serialize.h.
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I reviewed that these exactly match the behaviour of the C++ code.
| writer.write_u8(254)?; | ||
| writer.write_u32::<LittleEndian>(s as u32) | ||
| } | ||
| s => { |
There was a problem hiding this comment.
Do we know what the rationale was for that?
(I'd be terrified to change it, mind :-) )
|
|
||
| impl TxOut { | ||
| pub fn read<R: Read>(mut reader: &mut R) -> io::Result<Self> { | ||
| let value = Amount(reader.read_i64::<LittleEndian>()?); |
There was a problem hiding this comment.
Where do we check that this is in {0..MAX_MONEY}?
There was a problem hiding this comment.
Nowhere yet. I'll add a function to perform the necessary range check.
|
|
||
| impl SpendDescription { | ||
| pub fn read<R: Read>(mut reader: &mut R) -> io::Result<Self> { | ||
| let cv = edwards::Point::<Bls12, Unknown>::read(&mut reader, &JUBJUB)?; |
There was a problem hiding this comment.
cv are rk are required to not be of small order (see section 4.4 of the spec). Where is that checked?
There was a problem hiding this comment.
It is eventually checked during proof validation: 1f07d94#diff-43fed083a323e370f8efa22a3cf1b0ccR45
However, if you'd prefer that we also check these during parsing, I can do so.
There was a problem hiding this comment.
I don't insist on rules being implemented in specific places, but for reviewability, it's important to be able to check that all (and only!) the consensus rules described in the spec are implemented somewhere.
Perhaps we can follow the following policy:
- every consensus check (including implicit checks in parsing) should be documented with a comment referencing the section of the spec describing it;
- if you might expect a consensus check but it's actually somewhere else in the code, add a comment pointing to the somewhere else.
One concern might be that the second category of comments could get stale, but I think that is manageable.
There was a problem hiding this comment.
(If rules are moved around in the spec, this requires updating the comments. Up to now they haven't moved around much, if at all.)
|
|
||
| impl OutputDescription { | ||
| pub fn read<R: Read>(mut reader: &mut R) -> io::Result<Self> { | ||
| let cv = edwards::Point::<Bls12, Unknown>::read(&mut reader, &JUBJUB)?; |
There was a problem hiding this comment.
cv are epk are required to not be of small order (see section 4.5 of the spec). Where is that checked?
There was a problem hiding this comment.
It is eventually checked during proof validation: 1f07d94#diff-43fed083a323e370f8efa22a3cf1b0ccR127
However, if you'd prefer that we also check these during parsing, I can do so.
| impl JSDescription { | ||
| pub fn read<R: Read>(mut reader: R, use_groth: bool) -> io::Result<Self> { | ||
| let vpub_old = Amount(reader.read_i64::<LittleEndian>()?); | ||
| let vpub_new = Amount(reader.read_i64::<LittleEndian>()?); |
There was a problem hiding this comment.
Technically these fields are unsigned (section 7.2 of the spec). More precisely they're in the range {0..MAX_MONEY}.
There was a problem hiding this comment.
Amount parallels CAmount in zcashd, handling the fact that amounts may be subtracted with negative results. I'll add a function to perform the necessary range check as well as converting from u64 to i64, to make things clearer (so we use read_u64() to more closely match the spec).
There was a problem hiding this comment.
Maybe there should be a RestrictedAmount or CheckedAmount trait that is guaranteed to be in the {0..MAX_MONEY} range, with checked arithmetic?
| 0xb0, 0x06, 0xc0, 0x62, 0x46, 0x8e, 0x4b, 0xd8, 0xf7, 0xdd, 0x9a, 0xf6, 0x98, 0xf5, | ||
| 0x2a, 0xe8, 0x14, 0x63, 0x4e, 0x81, 0xd7, 0xf3, 0xe0, 0xc4, 0x20, 0x31, 0x7c, 0xac, | ||
| 0xa9, 0xae, 0x48, 0x11, 0xc6, 0xaf, 0x06, 0xfe, 0x80, 0xa8, 0xc0, 0x2a, 0xb7, 0xa0, | ||
|
|
There was a problem hiding this comment.
I did not check these test vectors in my review.
| 0xe7, 0xe6, 0x15, 0x9c, 0x46, 0x69, 0x9f, 0x10, 0x07, 0x92, 0xd4, 0x67, 0x29, 0x50, | ||
| 0x34, 0x8a, 0x90, 0x55, 0x2e, 0x45, 0x94, 0x3b, 0xee, 0xac, 0xf0, 0x3f, 0x32, 0x16, | ||
| 0xf9, 0x4e, 0x27, 0x90, 0x6e, 0xdc, 0x63, 0x23, 0x19, 0xad, 0x8d, 0x37, 0x44, 0x7f, | ||
| 0x5c, 0x59, 0xcc, 0xde |
There was a problem hiding this comment.
I have not checked these test vectors either.
|
Addressed @daira's remaining comments. |
|
ut(ACK+cov) c9b23df. |
Implements support for parsing all transaction versions, and the transaction digest algorithms from ZIP 143 and ZIP 243.