`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 was broken by a combination of several PRs that were apparently
merged with tests not passing (at least #6425 and #6479), due to a CI
fault.
Signed-off-by: Daira Emma Hopwood <daira@jacaranda.org>
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.
Kris Nuttycombe
Daira Emma Hopwood
Greg Pfeil
Sean Bowe
DeckerSU
Greg Pfeil
Miodrag Popović
Greg Pfeil
Dimitris Apostolou
Jack Grigg