Skip to content

Commit 3f508c3

Browse files
davidkaplanbitgobitgobot
authored andcommitted
fix(utxo-lib): serialize Zcash Sapling valueBalance as signed int64
Zcash's Sapling valueBalance is a signed 64-bit field — it is negative whenever value flows from the transparent pool into the shielded pool (t->z shielding). The previous code in fromBufferV4 threw UnsupportedTransactionError for any non-zero valueBalance, preventing deposit-detection pipelines from computing txids for shielding transactions even when the shielded bundle vectors are empty. In toBufferV4 and hashForSignatureByNetwork the field was always written as eight zero bytes (VALUE_INT64_ZERO), so even if parsing had succeeded the re-serialized transaction would have had the wrong bytes and produced a wrong txid. Changes: - ZcashTransaction gains a `saplingValueBalance: Buffer` field, initialised to VALUE_INT64_ZERO and preserved through clone(). - fromBufferV4 stores the raw 8-byte slice in saplingValueBalance instead of throwing when the field is non-zero. Non-empty shielded bundles still throw UnsupportedTransactionError via readEmptySaplingBundle, keeping the existing restriction against fully-shielded transactions. - toBufferV4 writes tx.saplingValueBalance back verbatim, enabling exact round-tripping of fromBuffer→toBuffer/getId for any Sapling tx whose shielded bundle vectors are empty. - hashForSignatureByNetwork uses tx.saplingValueBalance so sighash computation is also correct. - Add test/bitgo/zcash/ZcashTransaction.ts with round-trip tests for a minimal Sapling v4 tx with valueBalance = -5. Security: WCN-1961 / FND-002 (funds-loss, medium severity). A custody pipeline calling fromBuffer(shieldingTx).getId() would previously crash or silently produce a wrong txid; with this fix it correctly round-trips for the case of empty shielded bundles, and throws UnsupportedTransactionError for populated shielded bundles. Ticket: WCN-1961 Session-Id: e7382333-471f-41ac-9109-8dbfc6acb8ea Task-Id: ffe9c655-62ea-463d-a151-7d634b267c81
1 parent 8e1f0fc commit 3f508c3

3 files changed

Lines changed: 54 additions & 7 deletions

File tree

modules/utxo-lib/src/bitgo/zcash/ZcashBufferutils.ts

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -91,11 +91,10 @@ export function fromBufferV4<TNumber extends number | bigint>(
9191
}
9292

9393
if (tx.isSaplingCompatible()) {
94-
const valueBalance = bufferReader.readSlice(8);
95-
if (!valueBalance.equals(VALUE_INT64_ZERO)) {
96-
/* istanbul ignore next */
97-
throw new UnsupportedTransactionError(`valueBalance must be zero`);
98-
}
94+
// valueBalance is a signed int64; negative for t->z shielding transactions.
95+
// Store raw bytes so toBuffer/getId can round-trip the tx without mutation.
96+
// https://github.com/zcash/zcash/blob/v4.5.1/src/primitives/transaction.h#L283
97+
tx.saplingValueBalance = bufferReader.readSlice(8);
9998

10099
// https://github.com/zcash/zcash/blob/v4.5.1/src/primitives/transaction.h#L863
101100
readEmptySaplingBundle(bufferReader);
@@ -167,7 +166,7 @@ export function toBufferV4<TNumber extends number | bigint>(
167166
}
168167

169168
if (tx.isSaplingCompatible()) {
170-
bufferWriter.writeSlice(VALUE_INT64_ZERO);
169+
bufferWriter.writeSlice(tx.saplingValueBalance ?? VALUE_INT64_ZERO);
171170
bufferWriter.writeVarInt(0); // vShieldedSpendLength
172171
bufferWriter.writeVarInt(0); // vShieldedOutputLength
173172
}

modules/utxo-lib/src/bitgo/zcash/ZcashTransaction.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,10 @@ export class ZcashTransaction<TNumber extends number | bigint = number> extends
114114
// Block height after which this transactions will expire, or 0 to disable expiry
115115
expiryHeight = 0;
116116
consensusBranchId: number;
117+
// Raw 8-byte little-endian signed int64 valueBalance field for Sapling v4 txs.
118+
// Negative when value flows from the transparent pool into the shielded pool
119+
// (t->z shielding). Stored verbatim so toBuffer/getId round-trip correctly.
120+
saplingValueBalance: Buffer = VALUE_INT64_ZERO;
117121

118122
constructor(public network: ZcashNetwork, tx?: ZcashTransaction<bigint | number>, amountType?: 'bigint' | 'number') {
119123
super(network, tx, amountType);
@@ -123,6 +127,7 @@ export class ZcashTransaction<TNumber extends number | bigint = number> extends
123127
this.overwintered = tx.overwintered;
124128
this.versionGroupId = tx.versionGroupId;
125129
this.expiryHeight = tx.expiryHeight;
130+
this.saplingValueBalance = tx.saplingValueBalance;
126131

127132
if (tx.consensusBranchId !== undefined) {
128133
consensusBranchId = tx.consensusBranchId;
@@ -378,7 +383,7 @@ export class ZcashTransaction<TNumber extends number | bigint = number> extends
378383
bufferWriter.writeUInt32(this.locktime);
379384
bufferWriter.writeUInt32(this.expiryHeight);
380385
if (this.isSaplingCompatible()) {
381-
bufferWriter.writeSlice(VALUE_INT64_ZERO);
386+
bufferWriter.writeSlice(this.saplingValueBalance);
382387
}
383388
bufferWriter.writeInt32(hashType);
384389

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
import * as assert from 'assert';
2+
import { networks } from '../../../src';
3+
import { ZcashTransaction } from '../../../src/bitgo';
4+
5+
// Minimal Sapling v4 transaction with valueBalance = -5 (t->z shielding).
6+
// Built as: header(0x80000004) | versionGroupId(0x892F2085) | vin(0) | vout(0) |
7+
// locktime(0) | expiryHeight(0) | valueBalance(-5 as int64le) |
8+
// vSpendsSapling(0) | vOutputsSapling(0) | vJoinSplit(0)
9+
const SHIELDING_TX_HEX = '0400008085202f8900000000000000000000fbffffffffffffff000000';
10+
11+
describe('ZcashTransaction (Sapling valueBalance)', function () {
12+
describe('round-trip for t->z shielding tx (negative valueBalance)', function () {
13+
it('parses without throwing', function () {
14+
const tx = ZcashTransaction.fromBuffer(Buffer.from(SHIELDING_TX_HEX, 'hex'), false, 'number', networks.zcash);
15+
// saplingValueBalance stores raw bytes; -5 in int64le is fb ff ff ff ff ff ff ff
16+
assert.strictEqual(tx.saplingValueBalance.toString('hex'), 'fbffffffffffffff');
17+
});
18+
19+
it('re-serializes to the same hex (toBuffer round-trip)', function () {
20+
const tx = ZcashTransaction.fromBuffer(Buffer.from(SHIELDING_TX_HEX, 'hex'), false, 'number', networks.zcash);
21+
assert.strictEqual(tx.toBuffer().toString('hex'), SHIELDING_TX_HEX);
22+
});
23+
24+
it('computes getId() without throwing', function () {
25+
const tx = ZcashTransaction.fromBuffer(Buffer.from(SHIELDING_TX_HEX, 'hex'), false, 'number', networks.zcash);
26+
// Just verify it does not throw; the exact txid is deterministic from the hex above.
27+
assert.doesNotThrow(() => tx.getId());
28+
});
29+
30+
it('clones correctly', function () {
31+
const tx = ZcashTransaction.fromBuffer(Buffer.from(SHIELDING_TX_HEX, 'hex'), false, 'number', networks.zcash);
32+
const cloned = tx.clone();
33+
assert.strictEqual(cloned.toBuffer().toString('hex'), SHIELDING_TX_HEX);
34+
});
35+
});
36+
37+
describe('round-trip for transparent tx (zero valueBalance)', function () {
38+
it('saplingValueBalance defaults to all-zero bytes', function () {
39+
const tx = new ZcashTransaction(networks.zcash);
40+
assert.strictEqual(tx.saplingValueBalance.toString('hex'), '0000000000000000');
41+
});
42+
});
43+
});

0 commit comments

Comments
 (0)