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>
C++ `auto`, as opposed to `auto&`, does not infer reference types (except when it does:
https://blog.petrzemek.net/2017/12/08/when-auto-seemingly-deduces-a-reference-in-cpp/
but that doesn't apply here). The inferred type of `orchard_bundle` was therefore
`const OrchardBundle`. This made the assignment a call to `OrchardBundle::operator=`
which calls `orchard_bundle_clone`. A reference assignment is sufficient, because `tx`
is in scope throughout `ContextualCheckTransaction`.
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
As mentioned in #5964 and #5977, `-reindex-chainstate` is currently broken. This
removes it from error messages so that we don't suggest its use, but rather a
full `-reindex`.
This change should be reverted if/when those issues are fixed.
This enables `cargo` to parallelize the library and binary builds
internally, reducing the Rust build time by the build time of the
binaries (because they are overall faster than the library build).
Part of zcash/zcash#6065.
And ci script output.
Identified via test/lint/lint-spelling
(cherry picked from commit bitcoin/bitcoin@9b0e16226e)
Zcash: Only the change to `test/lint/lint-shell.sh`
Jack Grigg
Daira Hopwood
str4d
Alex
Kris Nuttycombe
Greg Pfeil
Kris Nuttycombe
user
Nathan Wilcox
Greg Pfeil
Conrado Gouvea
Ben Woosley
Hennadii Stepanov