Add structural test vectors for transaction formats #85
Conversation
We weren't generating negative value balances, which meant we missed bugs where value balances were being serialized as u64 instead of i64. The change to make some value balances negative causes some ZIP 243 and ZIP 244 test vector sighashes to change (as expected).
The test vectors include explicit values for simple fields like anchors and signatures, and lengths of variable-length composite fields like Sapling Spends and Orchard Actions. As the v5 transaction format always ends with either a simple field (`bindingSigOrchard`) or a zero-length composite field (`nActionsOrchard`), these test vectors are sufficient to ensure correct parsing of the entire transaction.
The test vectors include explicit values for simple fields like pubkeys and signatures, and lengths of variable-length composite fields like Sapling Spends and Sprout JSDescriptions.
| @@ -19,6 +19,7 @@ tv_scripts=( | |||
| sapling_note_encryption | |||
| sapling_signatures | |||
| sapling_zip32 | |||
| transaction_v5 | |||
There was a problem hiding this comment.
Why is transaction_legacy not included?
There was a problem hiding this comment.
Looks like that's in the third commit.
There was a problem hiding this comment.
@LarryRuane it should be included here as well.
| if valueBalanceRand & 0x8000000000000000: | ||
| self.valueBalanceOrchard = (valueBalanceRand % (MAX_MONEY + 1)) - MAX_MONEY | ||
| else: | ||
| self.valueBalanceOrchard = valueBalanceRand % (MAX_MONEY + 1) |
There was a problem hiding this comment.
Three times and you factor out:
def signed_amount(rand):
valueRand = rand.u64()
if valueRand & 0x8000000000000000:
return (valueRand % (MAX_MONEY + 1)) - MAX_MONEY
else:
return valueRand % (MAX_MONEY + 1)
There was a problem hiding this comment.
maybe?
return (valueRand % (MAX_MONEY + 1)) - (MAX_MONEY if valueRand & 0x8000000000000000 else 0)
There was a problem hiding this comment.
I like @daira's suggestion here: it's explicit as to the switch that you're making at the top level.
| test_vectors = [] | ||
| while len(test_vectors) < 30: | ||
| # Generate transactions with versions prior to ZIP 225. | ||
| tx = LegacyTransaction(rand, rand.u8() % NU5_TX_VERSION) |
There was a problem hiding this comment.
0 was never a valid version number. I see that line 327 above will replace a version other than 3 (Overwinter) and 4 (Sapling) with a random version:
self.nVersion = rand.u32() & ((1 << 31) - 1)
but I think that versions 1 and 2 (as version numbers that were actually used) should have high probability of occurrence, and 0 should be excluded. One option is to keep this line as it is, and only replace 0 with a random number in [1, 231) in the LegacyTransaction constructor.
| if valueBalanceRand & 0x8000000000000000: | ||
| self.valueBalanceOrchard = (valueBalanceRand % (MAX_MONEY + 1)) - MAX_MONEY | ||
| else: | ||
| self.valueBalanceOrchard = valueBalanceRand % (MAX_MONEY + 1) | ||
| self.anchorOrchard = PallasBase(leos2ip(rand.b(32))) | ||
| self.proofsOrchard = rand.b(rand.u8() + 32) # Proof will always contain at least one element | ||
| self.bindingSigOrchard = RedPallasSignature(rand) |
There was a problem hiding this comment.
Below here: version_bytes is confusingly named because it doesn't return bytes.
There was a problem hiding this comment.
Also, ZIP 225 has been updated to specify that all fields are little-endian, and so that TODO on line 504 can be removed.
| if valueBalanceRand & 0x8000000000000000: | ||
| self.valueBalanceOrchard = (valueBalanceRand % (MAX_MONEY + 1)) - MAX_MONEY | ||
| else: | ||
| self.valueBalanceOrchard = valueBalanceRand % (MAX_MONEY + 1) |
There was a problem hiding this comment.
maybe?
return (valueRand % (MAX_MONEY + 1)) - (MAX_MONEY if valueRand & 0x8000000000000000 else 0)
| @@ -19,6 +19,7 @@ tv_scripts=( | |||
| sapling_note_encryption | |||
| sapling_signatures | |||
| sapling_zip32 | |||
| transaction_v5 | |||
There was a problem hiding this comment.
Looks like that's in the third commit.
|
I'm trying to use the transaction test vectors here to test lightwalletd parsing. Maybe I'm interpreting things wrongly, but it seems like the |
|
@LarryRuane These are structural test vectors, and structurally the Incidentally, this is something I plan to change about the Rust transaction parser, which currently applies the additional restriction that |
Closes #84.