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.
This was breaking with `--disable-wallet`, likely due to some PR
removing a `using namespace std;` line from a header file that this code
was implicitly depending on.
Notable changes
===============
This hotfix remediates memory exhaustion vulnerabilities that zcashd inherited
as a fork of bitcoind. These bugs could allow an attacker to use peer-to-peer
messages to fill the memory of a node, resulting in a crash.
-----BEGIN PGP SIGNATURE-----
iQGzBAABCgAdFiEEX8Nd8pnYcf0pobEL9FXpuSAjoYsFAmP1DKQACgkQ9FXpuSAj
oYt2jQv+IurleLqEbtBW2ajDYDIkcMu+asEtHCk3B6GELykKaFMgHYTffFBmuyJ+
5GdoX685Rn6r8BxK6K1u129A9ztY7K0JpVeDA75tN5WLBj9twLdNaODfCLg5EjlZ
UtxTNnynQ0MX5Uv7pt1DAM+++OKYujUHypaajgZ9ttqpSHZl3Z2ye0/HFEc4023p
VH13CvU/3R4JLkFENi49rbS49LFfVuQrhAQoOPlCf3xoWbUYIdmWWZa/HOJV3g0e
3mqC+rhz97GVylLI4LJrm3v0tLeEUIuu+fdAziWfuWrBlB4jQ5p4L5trDHiQoqWB
5Qt5tjJKHHLnHHSyLcFFaes12tjPfrn9PBxYDPyfFAHIGf0WEiy36+6G5P7jpjYj
OXLAmBPBIRBgZf9LJIHrgvqQynfGe9vaWgCArWWgSC8wZ2hWlM7pZRhCe+uw35R1
e5AfZjvbXj6gR+1631Mhl84e6xkGXe24szzuZDrUCqLBJpTb1JNLRh1OtT8zDN0t
MfKtnE3W
=AXAQ
-----END PGP SIGNATURE-----
Merge tag 'v5.4.2' into hotfix-v5.4.2
zcashd release 5.4.2
Notable changes
===============
This hotfix remediates memory exhaustion vulnerabilities that zcashd inherited
as a fork of bitcoind. These bugs could allow an attacker to use peer-to-peer
messages to fill the memory of a node, resulting in a crash.
Notable changes
===============
This hotfix remediates memory exhaustion vulnerabilities that zcashd inherited
as a fork of bitcoind. These bugs could allow an attacker to use peer-to-peer
messages to fill the memory of a node, resulting in a crash.
-----BEGIN PGP SIGNATURE-----
iQGzBAABCgAdFiEEX8Nd8pnYcf0pobEL9FXpuSAjoYsFAmP1CD8ACgkQ9FXpuSAj
oYvGXQwAqaa1l5qC3VY/60jkH4xmWoSUEzeCOPUG7lYJrIEyzgXj8Ko0Cjr308jm
ISXDDTOxKb2hfnCbeqbZqRyFbzGzG5L6AkjHAvmQMiZwx2JlbH2k+jd5fggOZSMv
shL5KbxWN2YRftc8r+fDraJMbGULbKBWQooKaFyQmupT+bVsRf1Nh+lFIVG4FUwA
oWZot36wB6Y99Y57wlyN2m22+j1glyk5mKv2ttXYbdwSRTFB5W5L1US6Z8uxXQPb
Qa7sVO90QtzkHr+GPtMiTn513VuLFr+KArGn+qidU/PvblJI/vXuBf54g7JZw4Ot
gkKziatgaN3pO30I4rTij78LCgKJZ/WImLE7nwTl/bG2Ki1WfyolNLjS+1pXpcPL
xlXso5ioKlSIGhnlPouXwoxlaqTpDwRKLp0azNJl5hG/tXEHupToK2M61woi9LlP
4RB+a75OptFi9NMp/Sx0T8zNRn7OB8iP+3BgRP0+mzcLC1AHfjJk/IRo9CQh5jRZ
MMe98OrC
=6QYn
-----END PGP SIGNATURE-----
Merge tag 'v5.3.3' into hotfix-v5.4.2
zcashd release v5.3.3
Notable changes
===============
This hotfix remediates memory exhaustion vulnerabilities that zcashd inherited
as a fork of bitcoind. These bugs could allow an attacker to use peer-to-peer
messages to fill the memory of a node, resulting in a crash.
This reverts to the `bool` that was used previously. The independent change to use
`TransparentCoinbasePolicy as a parameter type eliminates some of the motivation for this, and there
is a better approach to solving the remaining issues in #6457.
Eliminates SelectsTransparentCoinbase in favor of a Disallow/Allow/Require ternary. Also asserts if
a selector requires transparent coinbase but doesn’t select transparent.
- Remove `std::optional` from a number of uses,
- simplify `GetUFVKMetadataForAddress` to `GetUFVKIdForAddress`, and
- add a new `GetUFVKMetadataForAddress` as a wrapper around
`GetUFVKMetadataForReceiver`.
Previously, all pools had to be listed in the selection order. This had a few problems
- if a pool was missing from selection, we’d `assert`
- a logic error could result in note selection from pools that we thought were not needed for
selection (and thus might subvert our strategy re: hiding sender, fewer pools, opportunistic
shielding, etc.)
Now, we only include pools that are intended to be part of the selection order, and all notes from
other pools are cleared before we do any selection.
little memory when we disconnect the node.
Co-authored-by: Kris Nuttycombe <kris@nutty.land>
Co-authored-by: Jack Grigg <str4d@z.cash>
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
While limitations on the influence of attackers on addrman already
exist (affected buckets are restricted to a subset based on incoming
IP / network group), there is no reason to permit them to let them
feed us addresses at more than a multiple of the normal network
rate.
This commit introduces a "token bucket" rate limiter for the
processing of addresses in incoming ADDR and ADDRV2 messages.
Every connection gets an associated token bucket. Processing an
address in an ADDR or ADDRV2 message from non-whitelisted peers
consumes a token from the bucket. If the bucket is empty, the
address is ignored (it is not forwarded or processed). The token
counter increases at a rate of 0.1 tokens per second, and will
accrue up to a maximum of 1000 tokens (the maximum we accept in a
single ADDR or ADDRV2). When a GETADDR is sent to a peer, it
immediately gets 1000 additional tokens, as we actively desire many
addresses from such peers (this may temporarily cause the token
count to exceed 1000).
The rate limit of 0.1 addr/s was chosen based on observation of
honest nodes on the network. Activity in general from most nodes
is either 0, or up to a maximum around 0.025 addr/s for recent
Bitcoin Core nodes. A few (self-identified, through subver) crawler
nodes occasionally exceed 0.1 addr/s.
(cherry-picked from commit bitcoin/bitcoin@0d64b8f709)
I thought we had removed this a long time ago, TBH, its really
confusing feedback to users that we display whether a tx was
broadcast to immediate neighbor nodes, given that has little
indication of whether the tx propagated very far.
We need to load these early so that it's possible for other
initialization steps to respect them. However, we were loading them
slightly too early, before the config file had been read, which meant
that only CLI arguments were being used.
We now load the `-allowdeprecated` settings just after the config file
is parsed and the chain parameters are prepared; neither of these are
features we would ever consider deprecating (at least while `zcashd`
exists in its Bitcoin Core-derived form).
Closeszcash/zcash#6420.
This should lower the main thread's likelihood to immediately reacquire
cs_main after dropping it, which should help ThreadNotifyWallets and the
RPC methods to acquire cs_main more quickly.
Co-authored-by: Daira Hopwood <daira@jacaranda.org>
Co-authored-by: Kris Nuttycombe <kris@nutty.land>
The general invariant in the wallet is that `CWallet::ChainTip` is only
called with sequential (either connecting or disconnecting) blocks. The
one exception to this is when starting `zcashd` with `-reindex`, which
creates a discontinuity: the node jumps back to the genesis block, and
`ThreadNotifyWallets` will similarly start notifying the wallet of the
entire chain again.
In Bitcoin Core, this behaviour was fine: there was no persistent cached
state that couldn't just be overwritten during the re-notification. For
Zcash however, wallets need to additionally maintain witnesses for notes
that are spendable, and these witnesses can generally only be amended by
sequential blocks.
For Sprout and Sapling, the discontinuity was handled by checking if a
reindex was occurring during `CWallet::InitLoadWallet`, and clearing the
witness caches in `CWallet::ClearNoteWitnessCache` if so. The witnesses
would then be rebuilt as the reindexed chain was re-connected during
`ActivateBestChain`.
The Orchard wallet stores its witnesses in a different structure on the
Rust side, so it wasn't being cleared at the same time. This meant that
when a full reindex was performed in one go, the sequentiality invariant
would be broken once `ThreadNotifyWallets` reached NU5 activation, and
the node would crash with a failed assertion (the issue at hand).
However, reindexing Zcash takes a long time, and has been historically
buggy for various reasons (e.g. crashing due to OOM). And due to a quirk
of how the `-rescan` behaviour is implemented, if a reindexing node is
restarted during its `ActivateBestChain` phase, on restart the node will
almost always trigger a rescan due to the wallet chain locator not
containing any hashes that match the on-start chain tip. And the first
thing that the rescan logic does is check whether the start of the
rescan is before NU5 activation, and reset the Orchard wallet if so.
We now reset the Orchard wallet unconditionally at the same time as we
clear the Sprout and Sapling witness caches. This additionally clears
spentness information that the Orchard wallet is storing, but that is
rebuilt during the reindex.
Closeszcash/zcash#5736.
Closeszcash/zcash#6004.
1000 blocks was selected as a balance between limiting the likely
maximum memory usage of the batch scanner, and avoiding
artificially restricting scanning throughput of small/fast blocks
due to the second-boundary lock synchronization point.
This also removes the `zcashd.wallet.batchscanner.usage.bytes` gague
value that was previously made available when `-prometheusport` was
specified.
Co-authored-by: Jack Grigg <jack@z.cash>
We no longer fetch updates from the mempool unless we have fetched all
updates from the chain, as we would otherwise notify the wallet of
mempool changes for which they have not observed parent transactions
in the chain.
Co-authored-by: Jack Grigg <jack@z.cash>
Wallet rescanning that happens on node startup was previously not
respecting Ctrl-C. We use an explicit check of `ShutdownRequested()`
instead of a `boost::interruption_point` because wallet startup does
not run in a separate thread.
Co-authored-by: Jack Grigg <jack@z.cash>
Kris Nuttycombe
Greg Pfeil
Greg Pfeil
Dimitris Apostolou
Daira Emma Hopwood
Greg Pfeil
Jack Grigg
str4d
Evan Klitzke
Kris Nuttycombe
Marius Kjærstad
Jon Atack
Pieter Wuille
Matt Corallo
sasha
Jack Grigg
Charlie O'Keefe
Alex Morcos
Alfredo Garcia
instagibbs
Suhas Daftuar