Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement ZIP 244 txid logic for v5 transactions #392

Open
str4d opened this issue May 23, 2022 · 3 comments · Fixed by #393
Open

Implement ZIP 244 txid logic for v5 transactions #392

str4d opened this issue May 23, 2022 · 3 comments · Fixed by #393
Labels
bug Something isn't working

Comments

@str4d
Copy link
Collaborator

str4d commented May 23, 2022

Support for parsing v5 transactions (as specified in ZIP 225) was added in #367. However, the txid computation for v5 transactions (as specified in ZIP 244) was not implemented. This means that at present, lightwalletd returns incorrect txids for v5 transactions in compact blocks:

block := parser.NewBlock()
rest, err := block.ParseFromSlice(blockData)
if err != nil {
return nil, errors.Wrap(err, "error parsing block")
}
if len(rest) != 0 {
return nil, errors.New("received overlong message")
}
if block.GetHeight() != height {
return nil, errors.New("received unexpected height block")
}
return block.ToCompact(), nil

// ToCompact returns the compact representation of the full block.
func (b *Block) ToCompact() *walletrpc.CompactBlock {
compactBlock := &walletrpc.CompactBlock{
//TODO ProtoVersion: 1,
Height: uint64(b.GetHeight()),
PrevHash: b.hdr.HashPrevBlock,
Hash: b.GetEncodableHash(),
Time: b.hdr.Time,
}
// Only Sapling transactions have a meaningful compact encoding
saplingTxns := make([]*walletrpc.CompactTx, 0, len(b.vtx))
for idx, tx := range b.vtx {
if tx.HasShieldedElements() {
saplingTxns = append(saplingTxns, tx.ToCompact(idx))
}
}
compactBlock.Vtx = saplingTxns
return compactBlock
}

// ToCompact converts the given (full) transaction to compact format.
func (tx *Transaction) ToCompact(index int) *walletrpc.CompactTx {
ctx := &walletrpc.CompactTx{
Index: uint64(index), // index is contextual
Hash: tx.GetEncodableHash(),

// GetDisplayHash returns the transaction hash in big-endian display order.
func (tx *Transaction) GetDisplayHash() []byte {
if tx.cachedTxID != nil {
return tx.cachedTxID
}
// SHA256d
digest := sha256.Sum256(tx.rawBytes)
digest = sha256.Sum256(digest[:])
// Convert to big-endian
tx.cachedTxID = Reverse(digest[:])
return tx.cachedTxID
}
// GetEncodableHash returns the transaction hash in little-endian wire format order.
func (tx *Transaction) GetEncodableHash() []byte {
digest := sha256.Sum256(tx.rawBytes)
digest = sha256.Sum256(digest[:])
return digest[:]
}

@str4d str4d added the bug Something isn't working label May 23, 2022
@str4d
Copy link
Collaborator Author

str4d commented May 23, 2022

#367 had test vectors that include txids; these passed because the test conditional was backwards, and checking that the derived txid didn't match the test vector.

if bytes.Equal(tx.cachedTxID, []byte(txtestdata.Txid)) {
t.Fatal("txid")
}

LarryRuane pushed a commit to LarryRuane/lightwalletd that referenced this issue May 23, 2022
This is a shortcut fix. Instead of replicating the zip244 transaction
logic that's implemented in librustzcash:
zcash_primitives/src/transaction/components/orchard.rs

cheat by calling getblock with verbose level 1, which prints the txid
of each transaction. This currently requires calling getblock twice,
once for the raw hex (as we have always done), and again to get the
transaction IDs. An easy improvement would be to add the raw hex to
verbosity 1 result. (Or have a new verbosity that shows both.)
LarryRuane pushed a commit to LarryRuane/lightwalletd that referenced this issue May 24, 2022
This is a shortcut fix. Instead of replicating the zip244 transaction
logic that's implemented in librustzcash:
zcash_primitives/src/transaction/components/orchard.rs

cheat by calling getblock with verbose level 1, which prints the txid
of each transaction. This currently requires calling getblock twice,
once for the raw hex (as we have always done), and again to get the
transaction IDs. An easy improvement would be to add the raw hex to
verbosity 1 result. (Or have a new verbosity that shows both.)
LarryRuane pushed a commit to LarryRuane/lightwalletd that referenced this issue May 24, 2022
This is a shortcut fix. Instead of replicating the zip244 transaction
logic that's implemented in librustzcash:
zcash_primitives/src/transaction/components/orchard.rs

cheat by calling getblock with verbose level 1, which prints the txid
of each transaction. This currently requires calling getblock twice,
once for the raw hex (as we have always done), and again to get the
transaction IDs. An easy improvement would be to add the raw hex to
verbosity 1 result. (Or have a new verbosity that shows both.)
LarryRuane pushed a commit that referenced this issue May 24, 2022
This is a shortcut fix. Instead of replicating the zip244 transaction
logic that's implemented in librustzcash:
zcash_primitives/src/transaction/components/orchard.rs

cheat by calling getblock with verbose level 1, which prints the txid
of each transaction. This currently requires calling getblock twice,
once for the raw hex (as we have always done), and again to get the
transaction IDs. An easy improvement would be to add the raw hex to
verbosity 1 result. (Or have a new verbosity that shows both.)
LarryRuane pushed a commit to LarryRuane/lightwalletd that referenced this issue Jun 12, 2022
Now "go test ./..." passes, it had broken due to the V5 transaction
(NU5) changes in zcash#392. I didn't have time to fix the tests at the time.
Also, this fixes a few lint problems.
LarryRuane pushed a commit that referenced this issue Jun 12, 2022
Now "go test ./..." passes, it had broken due to the V5 transaction
(NU5) changes in #392. I didn't have time to fix the tests at the time.
Also, this fixes a few lint problems.
@str4d
Copy link
Collaborator Author

str4d commented Jul 9, 2023

Reopening because this shouldn't have been closed by #393 (which only partially fixed the problem, the internal code still derives the wrong txid and needs fixing).

@LarryRuane
Copy link
Collaborator

@str4d, prompted by your recent comment, I began looking into this and discovered that the Golang version of blake2b doesn't have the personalization interface. Investigating this, I found that you had already opened an issue against this 4 years ago: golang/go#32447 (comment)

Since this has still not been implemented, I went ahead and wrote a patch to zcashd's getblock RPC to add another verbosity level, 3, that returns exactly the information that lightwalletd needs. I know you don't like this, but it bothers me greatly that lightwalletd makes two RPC calls for each block it fetches (one at verbosity=1 to get the txids, and another at verbosity=0 to get the raw block hex). The lightwalletd sync now takes about 12 hours on my high-end desktop, and reducing the number of RPCs by half is sure to help.

Think of it as a short-term improvement until Golang can implement blake2 properly.

The lightwalletd PR is #449 and the corresponding zcashd PR is zcash/zcash#6747. (These can be merged and deployed in either order.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants