Before merging 4.7.0-rc1 into the nu5-consensus branch, we were in a
split state:
- 4.7.0-rc1 included Orchard support in the transaction builder, which
required special handling of Orchard bundles when computing sighashes.
The `PrecomputedTransactionData` structure could be shared, because
its digests were only relevant to transparent signatures (as shielded
signatures signed the txid directly even in shielding transactions).
- nu5-consensus included the changes to ZIP 244, which required passing
around a `PrecomputedTransactionData` that contained the set of all
transparent inputs being spent, because shielding transactions now also
need to commit to transparent inputs.
In the merge commit, we incorrectly handled the resolution: we correctly
derived a fresh `PrecomputedTransactionData` when signing the Orchard
bundle, but we reused the `PrecomputedTransactionData` that was
previously derived before checking whether or not we even had an Orchard
bundle, for transparent inputs. This meant that its commitments didn't
commit to the Orchard bundle, and so transparent signatures on
transactions with Orchard bundles would fail to verify.
Incidentally, this is the exact inverse of a bug we encounted while
implementing the ZIP 244 changes on the nu5-consensus branch: we were
correctly computing the transparent sighash, but we were relying on the
initial `TxDigests` derived within `PrecomputedTransactionData` for the
Orchard sighash, even though we were actively rewriting the transaction
to include the Orchard bundle. The fix there was similarly to re-compute
the `TxDigests` before computing the sighash.
The merge commit includes changes to address direct merge conflicts.
This commit makes the remaining changes necessary to integrate the
Orchard wallet changes with the NU5 consensus changes.
The addition of `OrchardRawAddress` to `RecipientAddress` drives most of
the changes in this commit, which enable `z_sendmany` to send funds to
addresses in the Orchard pool once NU5 activates.
The ZIP 244 changes mean that we're going to need to alter every
callsite to pass through all of the transparent `CTxOut`s being spent.
Given that we need to pass it over to Rust, it makes more sense to just
have `PrecomputedTransactionData` be the vehicle for conveying this data
across.
This also alters `TransactionBuilder::SendChangeTo` to take a
`libzcash::ChangeAddress` value and thus avoids the invalid
t-addr case of CTxDestination.
This replaces the old implementation of asyncrpcoperation_sendmany
with one where all transaction construction is delegated to the
transaction builder. The capabilities of z_sendmany are somewhat
modified in the process:
* z_sendmany now permits sending funds from a Sprout address to
both transparent and Sapling addresses. PRIVACY NOTE: When
user sends a Sprout->Sapling transaction, the amount of the
transaction is publicly revealed.
* z_sendmany no longer supports transactions sending funds into
the Sprout pool, with the exception of change amounts when
sending from a Sprout address.
* When sending transparent coinbase funds to a set of shielded
addresses, the amount sent to recipients must fully consume
the input value and no change is permitted. This is a slightly
weaker constraint than was previously implemented; in the past,
only a single shielded recipient was allowed.
This is ~1.7x slower than the Lookup3-of-Xor-with-salt construct we were
using before, but it is a primitive designed for exactly this.
Zcash: Propagate CCoinsKeyHasher -> SaltedTxidHasher changes to where
we've used CCoinsKeyHasher outside of its original scope.
This matches the existing transaction builder structs:
- SpendDescriptionInfo
- OutputDescriptionInfo
- TransparentInputInfo
It also removes the dependency of the transaction format on the proving
system.
crypto_sign_verify_detached is still used within the consensus rules
until Canopy activation. ed25519-zebra generates signatures that are
valid under both pre- and post-Canopy rules (for our honest usage),
so we can use it to generate transaction signatures now. Then once
Canopy activates, we can remove the remaining usages of crypto_sign.
Refactor ProofVerifier
`ProofVerifier` was previously used to conditionally verify pre-Sapling Sprout
proofs (based on `ProofVerifier::Strict` or `ProofVerifier::Disabled` being
used), but hybrid Sprout proofs bypassed it (so were being verified multiple
times during block verification), and once `libsnark` was removed in
zcash/zcash#4060 `ProofVerifier::check` was doing nothing.
This PR refactors `ProofVerifier`, moving it out of the `libzcash` compilation
unit (so that it can depend on `primitives/transaction.h`), and moving Sprout
verification from `JSDescription::Verify` to `ProofVerifier::VerifySprout`.
Verification-skipping for Sprout proofs is re-introduced.
Additionally, the `ZCJoinSplit` global is removed from the codebase, and
`ZCJoinSplit::prove` is converted into a static function. We load the hybrid
Sprout parameters dynamically at proving time within the Rust code, and no
longer require a C++ global for any proving parameters.
As a side-effect, `libzcashconsensus.la` building with `--with-libs` is fixed,
as `primitives/transaction.cpp` no longer depends on `librustzcash.h`.
It needs to be closer to the root of our dependency tree, so that it can
depend on the transaction format. The libzcash compilation unit is
further from the dependency tree root than the transaction format.
We don't support making pre-Sapling JoinSplit proofs, and we load the
parameters for post-Sapling JoinSplit proofs at proving time, so there
is no need for a global ZCJoinSplit to be passed through the APIs.
SaplingNotePlaintext::decrypt() now has to be aware of consensus params and blockheight. Its callers in wallet, rpcwallet, and tests are updated accordingly.
TransactionBuilder is also modified to reject invalid leadBytes.
Co-authored by Daira Hopwood (daira@jacaranda.org)
This change improves usability across network upgrades, by informing
users when their new transactions are being created with the consensus
branch ID from the previous epoch.
We only check failing signatures against the previous epoch to minimise
the extra computational load on nodes.
The extra specifier meant that a runtime error would be thrown
during Sprout to Sapling migration if `zrpcunsafe` logging
was enabled:
"tinyformat: Too many conversion specifiers in format string"