`std::visit(match {...}, specimen)`, improving code readability.
For ease of review, this commit includes only obviously correct
transformations that all follow the same pattern.
Signed-off-by: Daira Emma Hopwood <daira@jacaranda.org>
InsertBlockIndex should take a const reference to a uint256 instead of
just a uint256.
LoadBlockIndexGuts also assume std::function<CBlockIndex*(const uint256&)> insertBlockIndex
as an argument:
2d456afebe/src/txdb.h (L150)
This fixes an RPC test failure that tests specifically for this with
z_shieldcoinbase. This also exposed an issue where an overly-high fee resulted
in a negative Payment causing an exception too deep. Added an assert when
creating a Payment and guarded against it for z_shieldcoinbase.
Fixes#2621 and #5654 (but does not handle Orchard locking, which is tracked in
a separate issue).
Extracted from z_sendmany to be used across multiple transaction
operations. Previously, it also checked a `bool` to decide what “LegacyCompat”
means, but bools are too easy to concoct, so it now expects the sender and
recipients to be checked internally. Also, z_sendmany was generating the bool
only from the sender, illustrating how easy it is to miss something when you try
to precompute.
Do the check deeper, preventing test_bitcoin from being able to bypass it. This
also moves it out of z_sendmany-specific code, which will be helpful when we add
other operations, like sendfromaccount.
This resolves a conflict where most usage is `const`, but some modifies the
wallet. Previously it held a const member and then used `pwalletMain` directly
for the mutating calls. This now passes `CWallet` explicitly where necessary,
using `const` when possible.
This also benefits a follow-up PR (#6408) that introduces locking, which also
mutates the wallet.
As a pre-check inside `z_sendmany` we estimate the size of the
transaction that would be created, to confirm it won't exceed any
limits. We do this by creating a fake transaction with fake outputs and
measuring its size. In the case of Sapling recipients, we'd push an
empty `OutputDescription`.
In zcash/zcash#6459 we pulled in changes that improved type safety in
the Rust types. One of these changes was that the `cv` field in a
Sapling Output Description is now enforced at parsing time to be not
small order (where previously we enforced this at proof verification
time).
The two above paragraphs collide because when measuring the size of the
fake transaction, we convert a `CMutableTransaction` into a
`CTransaction`; this calls `UpdateHash` to pin its txid, and that causes
the transaction to be serialized and then parsed across the FFI. This
causes the null `OutputDescription` to reach the Rust parser which
treats it as invalid.
There are two solutions to this, which are used in various contexts:
- Avoid pushing a null `OutputDescription` into a `CMutableTransaction`.
This is the fix implemented in this PR for `z_sendmany`: we now call
`RandomInvalidOutputDescription()` which gives us a consensus-invalid
but parser-valid `OutputDescription`, suitable for estimating tx size.
- Use `UNSAFE_CTransaction` to avoid having `UpdateHash` be called on
construction. This type is used in tests where we explicitly want to
construct an invalid type in C++, for consensus checking purposes. One
of the `OutputDescription()` uses was in a test, but didn't trigger
the issue because the test was checking a different part of the
transaction being invalid. Technically no change is needed here;
however we now also call `RandomInvalidOutputDescription()` here for
uniformity.
Part of zcash/zcash#6509.
This inverts the structure, dispatching on the selector pattern first, then only checking
`SpendableInputs` if the selector is multi-pool. This shortens the code, eliminates some failure
cases, and caught a bug where `SaplingExtendedFullViewingKey` selectors were not
supported (previously hidden by `match { ..., [](const auto&) … }`).
This also removes the code that stops adding spends if they ever go
`>= targetAmount`. The included note limiting and change calculation should
ensure that it’s always `==` at the end, and we don’t want paper over a mistake
in those earlier calculations.
There are existing tests that fail if either
- the newly-added Orchard increment is missing or
- the assertion is applied when there’s Sprout change.
The `Payment` type had an `isInternal` field, but it is (and should always be) `false`.
`ResolvedPayment` is the corresponding type internal to the transaction builder that can be either
internal (for change) or external (for user-requested payments).
Some of the more significant changes are
- remove release note entry for already-released feature;
- rephrase some error messages and comments;
- add a missing case to `EstimateTxSize`;
- don’t return a selector when we don’t have a UFVK for a UA, which allows some
simplifications (and elimination of a failure case) to happen; and
- remove a redundant `InsufficientFundsError`.
`CWalletTx::DecryptSaplingNote` and `CWalletTx::RecoverSaplingNote` were
previously unused, and the variants that skipped the plaintext version
check were only used by `z_viewtransaction` to inspect details about
transactions that had already been added to the wallet.
We now merge the two back together, and use the activation height of the
Canopy NU as the checked height. This re-checks the plaintext version in
`z_viewtransaction` but permits either valid version.
The `zcashd` impl of `consensus::Parameters` is moved into a new
`params` module. It still uses the `cxx::bridge` in `wallet_scanner`
because `cxx` doesn't support Rust type aliases yet.
Jack Grigg
Kris Nuttycombe
Daira Emma Hopwood
Greg Pfeil
DeckerSU
Greg Pfeil
Dimitris Apostolou
Greg Pfeil
Jack Grigg
str4d