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.
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.
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>
ThreadNotifyWallets was created in order to decouple wallet scanning of
transactions from the main chain updates, avoiding timing leaks to
network peers. To further ensure that not even lock contention could be
used to extract timing information, ThreadNotifyWallets collects update
information from the main chain on integer second boundaries.
In general this means that the wallet is processing the last second's
worth of blocks between each synchronization point. However, when there
are sequences of blocks that are costly for the wallet to scan, it may
take the wallet longer than a second to process a second's worth of
blocks connected to the main chain. This means that ThreadNotifyWallets
needs to wait until the next integer second boundary before collecting
the next set of updates, which means it will be processing at least two
seconds' worth of blocks. For extended periods where the chain contains
many outputs, the wallet will get progressively further behind the main
chain.
At the time that ThreadNotifyWallets was created, the above behaviour
was fine, because while the wallet scanning process consumed a lot of
CPU time, it did not consume much memory (as blocks are stored on disk).
However, we recently added batch scanning, which requires allocating
memory for each output that is being scanned. For a wallet with a
growing gap between its scanned-to height and the chain tip height, the
size of the necessary allocation will grow with each integer second
boundary crossed, until the node reaches OOM.
The solution is to implement backpressure: if we reach a memory limit
before we've finished adding blocks to the batch scanners, then we start
consuming the results from the batch scanner to free up memory space for
subsequent batches.
This commit implements the logic necessary to interleave batch creation
and batch result consumption. It does not apply any batch memory limits,
and as such should be effectively a no-op refactor.
Co-authored-by: Daira Hopwood <daira@jacaranda.org>
We were (most likely) copying potentially large structures in places
where we only required references to the data, for the same reason as
the bug fixed in zcash/zcash#6169: in most cases, `auto` does not infer
a reference, causing assignments to `auto` variables to invoke the copy
constructor.
Co-authored-by: Daira Hopwood <daira@jacaranda.org>
Many error messages would say "see debug.log" or similar, without
indicating where the debug log actually lives. This now prints the
actual path in those cases.
It also changes more general uses of "debug.log" to "debug log", since
the file name may not even be "debug.log" if the user has specified it.
Previously when a transaction was queried for batch trial decryption, we
identified it by its txid. This is sufficient to uniquely identify the
transaction within the wallet, but was _not_ sufficient to uniquely
identify it within a `ThreadNotifyWallets` loop. In particular, when a
reorg occurs, and the same transaction is present in blocks on both
sides of the reorg (or is reorged into the mempool and then conflicted
out):
- The first occurrence would batch the transaction's outputs and store a
result receiver.
- The second occurrence would overwrite the first occurrence's result
receiver with its own.
- The first occurrence would read the second's result receiver (which
has identical results to the first batch), removing it from the
`pending_results` map.
- The second occurrence would not find any receiver in the map, and
would mark the transaction as having no decrypted results.
We fix this by annotating each batched transaction with the hash of the
block that triggered it being trial-decrypted: either the block being
disconnected, the block being connected, or the null hash to indicate
a new transaction in the mempool. This is sufficient to domain-separate
all possible sources of duplicate txids:
- If a transaction is moved to the mempool via a block disconnection, or
from the mempool (either mined or conflicted) via a block connection,
its txid will appear twice: once with the block in question's hash,
and once with the null hash.
- If a transaction is present in both a disconnected and a connected
block (mined on both sides of the fork), its txid will appear twice:
once each with the two block's txids.
Both of the above rely on the assumption that block hashes are collision
resistant, which in turn relies on SHA-256 being collision resistant.
`CValidationInterface` listeners can either listen directly to
`CValidationInterface::SyncTransaction` as they currently do, or they
can listen to `CValidationInterface::InitBatchScanner` and then process
transactions via `BatchScanner::SyncTransaction`. The latter approach
allows listeners to perform trial decryption via whatever strategy is
most optimal for them.
When the wallet notification logic was moved into a separate thread,
most wallet notifications were transferred across. This one was missed,
and it is particularly pernicious: all it does is ask the wallet to tell
the UI that a particular transaction had been updated. We don't actually
_have_ any UI connected in zcashd, but there is a side-effect: the
callback blocks on acquiring `cs_wallet`, in the main thread that
already holds `cs_main`. For particularly large wallets, this can cause
the main thread to block on `ThreadNotifyWallets`, which in turn means
that anything waiting on `cs_main` (e.g. RPC calls) is blocked.
We solve this by moving the callback into `ThreadNotifyWallets`. We
don't technically need it for `zcashd`, but we maintain it in case a
downstream fork has reconnected a UI.
During testing of the previous fixes, the node entered a state where it
was possible to incorrectly dereference a map entry and crashed. These
changes address the most likely locations of the failed check.
In #5809, we attempted to fix the assumption that on startup, the
wallet's best chain was at the same position as the node's chain tip.
This did not account for a condition where the node's block index
might not contain the block corresponding to the wallet's best chain,
because the node had crashed before the block index containing that
block could be written to disk.
This commit adds handling so that the `ThreadNotifyWallets` thread
will not start until the wallet's best block has been restored
to the node's block index and we're able to obtain a pointer to
that state.
When initializing a new Orchard wallet after NU5 activation, it is
not valid to start from the empty note commitment tree; instead,
the note commitment tree needs to be initialized from the state of
the global Orchard Merkle frontier.
In addition, this change necessitates a change to how rewinds work,
such that in a rollback scenario with a newly initialized wallet
that does not have sufficient checkpoints to fully satisfy a requested
rewind, the rewind is allowed to proceed so long as it does not
invalidate any persisted witness data.
The previous code assumed that the last chain tip notified to the wallet
was equal to the node's chain tip at startup. However, this assumption
fails if the node shuts down uncleanly, or if a wallet file is moved
from one node to another.
We now try to start notifying from the wallet's best block, and if the
node doesn't have that block we fall back to the node's chain tip like
before.
Closeszcash/zcash#5805.
This mitigates a potential race condition between mutation of
`mapWallet` and writes to the wallet database that can result
in the state of the databse becoming inconsistent with the in-memory
set of transactions.
- The old patch is no longer necessary because of this upstream fix:
https://github.com/boostorg/build/pull/560
- Boost 1.72 removed a <deque> from an include, which exposed a missing
include in src/httpserver.cpp.
- Boost 1.73 moved function placeholders into the boost::placeholders
namespace.
- The new patch is a fix from just after Boost 1.74 was released, fixing
a warning that was missed.
After CWallet::ChainTipAdded() updates the witness data, it
may flush it to disk (SetBestChain()); make sure the locator
part is consistent with the witnesses (height).
ThreadNotifyWallets now collects all notification-related state at once,
and then executes wallet logic based on the collected state after
dropping any locks it is holding.
Continues Johnathan Corgan's work.
Publishing multipart messages
Bugfix: Add missing zmq header includes
Bugfix: Adjust build system to link ZeroMQ code for Qt binaries
Adds a regression test for the wallet's ResendWalletTransactions function, which uses a new, hidden RPC command "resendwallettransactions."
I refactored main's Broadcast signal so it is passed the best-block time, which let me remove a global variable shared between main.cpp and the wallet (nTimeBestReceived).
I also manually tested the "rebroadcast unconfirmed every half hour or so" functionality by:
1. Running bitcoind -connect=0.0.0.0:8333
2. Creating a couple of send-to-self transactions
3. Connect to a peer using -addnode
4. Waited a while, monitoring debug.log, until I see:
```2015-03-23 18:48:10 ResendWalletTransactions: rebroadcast 2 unconfirmed transactions```
One last change: don't bother putting ResendWalletTransactions messages in debug.log unless unconfirmed transactions were actually rebroadcast.
Kris Nuttycombe
Greg Pfeil
Matt Corallo
Jack Grigg
Greg Pfeil
str4d
Homu
Kris Nuttycombe
Larry Ruane
therealyingtong
Sean Bowe
Daira Hopwood
Jonas Schnelli
Eirik Ogilvie-Wigley
Jeff Garzik
João Barbosa
Gavin Andresen