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.
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`.
This should hopefully ensure that we end up with a single Git repository
that has both branches in it, enabling `git merge-base --is-ancestor` to
work correctly.
The "recent base" check attempts to remove a label from the PR being
checked, which uses the `issues` API. But a `write` permission for the
`issues` API appears to be insufficient.
* sort the result summary (always)
* add a flag, `--deterministic` (`-d`), to enable other output changes
* don’t print progress `.` (with `-d`)
* don’t print durations/runtime (with `-d`)
This isn’t fully deterministic as tests can still complete out of order, but
even there these changes make it easier to diff regions. Adding `--jobs 1`
should make it fully deterministic at the cost of performance.
The `pull_request_target` event causes `actions/checkout` to check out
the target branch (e.g. the main repo's `master` branch) instead of the
PR's branch. This meant that after zcash/zcash#6487 merged, the check
would always pass (because the queried revision is always present in the
history of `master`). `github.head_ref` correctly points to the tip of
the PR's branch, ensuring that `git merge-base --is-ancestor` performs
the expected comparison.
Kris Nuttycombe
Daira Emma Hopwood
Greg Pfeil
Greg Pfeil
Jack Grigg
str4d