parse V5 (nu5) transactions #367

LarryRuane · 2021-10-07T21:01:18Z

todo:

add relevant V5 fields to CompactBlock (instead of skipping over)
add V5 test vectors to unit tests

parser/transaction.go

LarryRuane · 2021-10-08T02:57:04Z

It's easier to review this PR if you ignore whitespace: https://github.com/zcash/lightwalletd/pull/367/files?w=1
(The whitespace changes are necessary because when working with Go, the convention is to keep the code always formatted (gofmt).)

LarryRuane · 2021-10-08T14:09:07Z

Force-pushed to incorporate @daira's comment. @nuttycom, on further thought, let's just leave this as-is for now, we should merge this quickly, I could possibly break it by reorganizing the code, and I can clean it up separately.

nuttycom

I would prefer for the V4 and V5 parsers to be separate functions, which may delegate to functions that perform parsing of the common parts of the format. It's hard to check against the spec with the two formats interleaved, and this is the same reason that we wrote an entirely separate section in the protocol for the V5 format rather than trying to write it interleaved.

parser/transaction.go

LarryRuane · 2021-10-13T14:42:20Z

Force-pushed to incorporate @nuttycom's suggestions (thanks!). There are now separate functions to parse pre-V5 transactions and V5 transactions. In order to not duplicate too much code, I refactored the transparent parsing into a separate function (called by both top-level transaction parsers). This didn't work out so nicely for the Sapling spends and outputs, so I didn't do the same there.

It's easiest to review this code with whitespace ignored: https://github.com/zcash/lightwalletd/pull/367/files?w=1

nuttycom

One more small required change, the fOverwintered flag needs to be checked; otherwise just a nit about transaction construction that you can ignore at your discretion.

parser/transaction.go

LarryRuane · 2021-10-14T19:35:28Z

Force-pushed for @nuttycom's review comments (thanks!)

parser/transaction.go

LarryRuane · 2021-10-18T20:46:21Z

New commit 6f019a3 to address @nuttycom's suggestions, will squash before merging. I tested these changes by running testnet, although the orchard actions still haven't been tested (testnet has v5 transactions, but none of them contain orchard actions yet).

nuttycom

utACK

parser/transaction.go

LarryRuane · 2021-10-20T19:33:10Z

Pushed a new commit to address the latest review comments (make a separate commit for easier review; will squash the commit later).

nuttycom

re-utACK.

daira · 2021-10-20T22:22:01Z

parser/transaction.go

+		return nil, errors.New("could not read nVersionGroupId")
+	}
+	if tx.nVersionGroupID != 0x26A7270A {
+		return nil, errors.New(fmt.Sprintf("version number %d must be 0x26A7270A", tx.nVersionGroupID))


Suggested change

return nil, errors.New(fmt.Sprintf("version number %d must be 0x26A7270A", tx.nVersionGroupID))

return nil, errors.New(fmt.Sprintf("version group ID %d must be 0x26A7270A", tx.nVersionGroupID))

parser/transaction.go

daira · 2021-10-21T11:08:51Z

parser/transaction.go

+	}
+	if !s.ReadCompactSize(&spendCount) {
+		return nil, errors.New("could not read nShieldedSpend")
+	}


Suggested change

}

}

if spendCount >= (1 << 16) {

return nil, errors.New(fmt.Sprintf("spendCount (%d) must be less than 2^16", spendCount))

}

Although the consensus rule restricting the maximum number of spends and outputs to 2¹⁶ - 1 each was only added for NU5, they were already restricted to lower limits by the block size, so it is correct to check them unconditionally for both v4 and v5 transactions, as zcashd does.

daira · 2021-10-21T11:10:34Z

parser/transaction.go

-	if !s.ReadCompactSize(&txInCount) {
-		return nil, errors.New("could not read tx_in_count")
+	if !s.ReadCompactSize(&outputCount) {
+		return nil, errors.New("could not read nShieldedOutput")
 	}


Suggested change

}

}

if outputCount >= (1 << 16) {

return nil, errors.New(fmt.Sprintf("outputCount (%d) must be less than 2^16", outputCount))

}

daira · 2021-10-21T13:24:04Z

parser/transaction.go

@@ -325,148 +352,221 @@ func (tx *Transaction) ToCompact(index int) *walletrpc.CompactTx {
 	return ctx
 }

-// ParseFromSlice deserializes a single transaction from the given data.
-func (tx *Transaction) ParseFromSlice(data []byte) ([]byte, error) {
+// parse version 4


Suggested change

// parse version 4

// parse version 4 transaction data after the nVersionGroupId field.

LarryRuane · 2021-10-21T15:01:04Z

Force-pushed for latest review comments (enforcing spendCount and outputCount limits, fix version group ID error string, improve 2 comments), thank you, @daira.

daira · 2021-10-21T21:08:07Z

parser/transaction.go

-		var joinSplitCount int
-		if !s.ReadCompactSize(&joinSplitCount) {
-			return nil, errors.New("could not read nJoinSplit")
+		var proofsCount int


This is not a count of proofs, it's a size in bytes. Rename it to something like proofsSize.

daira · 2021-10-21T21:09:03Z

parser/transaction.go

-			if !s.ReadBytes(&tx.joinSplitPubKey, 32) {
-				return nil, errors.New("could not read joinSplitPubKey")
-			}
+	// declare here to prevent shadowing problems in cryptobyte assignments


I don't know what this means.

daira

utACK modulo comments.

TODO: - store, instead of just skip over, nu5 transaction fields - add relevant nu5 fields to CompactBlock - restore disabled V4 unit tests - add V5 test vectors to unit tests The reason most of the V4 transaction and block unit tests are removed is that they used V3 transactions, which lightwalletd never sees in production, since lightwalletd starts at Sapling activation (which has V4 transactions). So these tests were always wrong, in a way. This commit simplifies the parsing code by removing support for V3 (since it was never needed). The tests need to be updated to V4, but we'll do that in a later PR.

LarryRuane requested review from nuttycom and daira October 7, 2021 21:01

LarryRuane self-assigned this Oct 7, 2021

daira reviewed Oct 7, 2021

View reviewed changes

parser/transaction.go Show resolved Hide resolved

nuttycom reviewed Oct 8, 2021

View reviewed changes

parser/transaction.go Outdated Show resolved Hide resolved

parser/transaction.go Outdated Show resolved Hide resolved

LarryRuane force-pushed the 2021-10-v5-transaction-parsing branch from c6ae3df to 4971c6e Compare October 8, 2021 14:04

LarryRuane requested a review from str4d October 8, 2021 14:09

r3ld3v added this to the Core Sprint 2021-40 milestone Oct 11, 2021

nuttycom requested changes Oct 11, 2021

View reviewed changes

parser/transaction.go Outdated Show resolved Hide resolved

LarryRuane force-pushed the 2021-10-v5-transaction-parsing branch from 4971c6e to 589f710 Compare October 13, 2021 14:33

LarryRuane requested a review from nuttycom October 13, 2021 15:32

nuttycom requested changes Oct 14, 2021

View reviewed changes

parser/transaction.go Show resolved Hide resolved

parser/transaction.go Show resolved Hide resolved

parser/transaction.go Show resolved Hide resolved

parser/transaction.go Outdated Show resolved Hide resolved

LarryRuane force-pushed the 2021-10-v5-transaction-parsing branch from 589f710 to 25949b2 Compare October 14, 2021 19:34

nuttycom reviewed Oct 15, 2021

View reviewed changes

parser/transaction.go Outdated Show resolved Hide resolved

nuttycom approved these changes Oct 19, 2021

View reviewed changes

daira reviewed Oct 19, 2021

View reviewed changes

parser/transaction.go Show resolved Hide resolved

daira reviewed Oct 19, 2021

View reviewed changes

parser/transaction.go Outdated Show resolved Hide resolved

daira reviewed Oct 19, 2021

View reviewed changes

parser/transaction.go Show resolved Hide resolved

daira reviewed Oct 19, 2021

View reviewed changes

parser/transaction.go Outdated Show resolved Hide resolved

daira reviewed Oct 19, 2021

View reviewed changes

parser/transaction.go Show resolved Hide resolved

nuttycom approved these changes Oct 20, 2021

View reviewed changes

daira reviewed Oct 20, 2021

View reviewed changes

daira reviewed Oct 21, 2021

View reviewed changes

parser/transaction.go Show resolved Hide resolved

daira reviewed Oct 21, 2021

View reviewed changes

LarryRuane force-pushed the 2021-10-v5-transaction-parsing branch from 6281972 to e33780b Compare October 21, 2021 14:54

daira reviewed Oct 21, 2021

View reviewed changes

daira approved these changes Oct 21, 2021

View reviewed changes

r3ld3v removed this from the Core Sprint 2021-40 milestone Oct 22, 2021

LarryRuane force-pushed the 2021-10-v5-transaction-parsing branch from e33780b to f166d2c Compare October 24, 2021 19:29

LarryRuane merged commit ddf3781 into zcash:master Oct 24, 2021

LarryRuane deleted the 2021-10-v5-transaction-parsing branch October 26, 2021 14:47

teor2345 mentioned this pull request Feb 15, 2022

getblock JSON-RPC method ZcashFoundation/zebra#3144

Closed

str4d mentioned this pull request May 23, 2022

Implement ZIP 244 txid logic for v5 transactions #392

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

parse V5 (nu5) transactions #367

parse V5 (nu5) transactions #367

LarryRuane commented Oct 7, 2021

LarryRuane commented Oct 8, 2021 •

edited

Loading

LarryRuane commented Oct 8, 2021

nuttycom left a comment

LarryRuane commented Oct 13, 2021

nuttycom left a comment

LarryRuane commented Oct 14, 2021

LarryRuane commented Oct 18, 2021

nuttycom left a comment

LarryRuane commented Oct 20, 2021

nuttycom left a comment

daira Oct 20, 2021

daira Oct 21, 2021

daira Oct 21, 2021

daira Oct 21, 2021

LarryRuane commented Oct 21, 2021

daira Oct 21, 2021

daira Oct 21, 2021

daira left a comment

	return nil, errors.New(fmt.Sprintf("version number %d must be 0x26A7270A", tx.nVersionGroupID))
	return nil, errors.New(fmt.Sprintf("version group ID %d must be 0x26A7270A", tx.nVersionGroupID))

-	}
+	}
+	if spendCount >= (1 << 16) {
+		return nil, errors.New(fmt.Sprintf("spendCount (%d) must be less than 2^16", spendCount))
+	}

-	}
+	}
+	if outputCount >= (1 << 16) {
+		return nil, errors.New(fmt.Sprintf("outputCount (%d) must be less than 2^16", outputCount))
+	}

	// parse version 4
	// parse version 4 transaction data after the nVersionGroupId field.

parse V5 (nu5) transactions #367

parse V5 (nu5) transactions #367

Conversation

LarryRuane commented Oct 7, 2021

LarryRuane commented Oct 8, 2021 • edited Loading

LarryRuane commented Oct 8, 2021

nuttycom left a comment

Choose a reason for hiding this comment

LarryRuane commented Oct 13, 2021

nuttycom left a comment

Choose a reason for hiding this comment

LarryRuane commented Oct 14, 2021

LarryRuane commented Oct 18, 2021

nuttycom left a comment

Choose a reason for hiding this comment

LarryRuane commented Oct 20, 2021

nuttycom left a comment

Choose a reason for hiding this comment

daira Oct 20, 2021

Choose a reason for hiding this comment

daira Oct 21, 2021

Choose a reason for hiding this comment

daira Oct 21, 2021

Choose a reason for hiding this comment

daira Oct 21, 2021

Choose a reason for hiding this comment

LarryRuane commented Oct 21, 2021

daira Oct 21, 2021

Choose a reason for hiding this comment

daira Oct 21, 2021

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

LarryRuane commented Oct 8, 2021 •

edited

Loading