The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from code that is not strictly required by it.
The settings code belongs into the common library and namespace, since
the kernel library should not depend on it. See doc/design/libraries.md
for more information on this rationale.
Changing the namespace of the moved functions is scripted in the
following commit.
To avoid wasting processing power, we can skip blocks that occurred
before the wallet's creation time, since these blocks are guaranteed
not to contain any relevant wallet data.
This has direct implications (an speed improvement) on the underlying
blockchain synchronization process as well.
The reason is that the validation interface queue is limited to
10 tasks per time. This means that no more than 10 blocks can be
waiting for the wallet(s) to be processed while we are synchronizing
the chain (activating the best chain to be more precise).
Which can be a bottleneck if blocks arrive and are processed faster
from the network than what they are processed by the wallet(s).
55696a0ac30bcfbd555f71cbc8eac23b725f7dcf wallet: remove `mempool_sequence` from `transactionRemovedFromMempool` (w0xlt)
bf19069c53501231a2f3ba59afa067913ec4d3b2 wallet: remove `mempool_sequence` from `transactionAddedToMempool` (w0xlt)
Pull request description:
This PR removes `mempool_sequence` from `transactionRemovedFromMempool` and `transactionAddedToMempool`.
`mempool_sequence` is not used in these methods, only in ZMQ notifications.
ACKs for top commit:
instagibbs:
ACK 55696a0ac3
Tree-SHA512: 621e89230bcb6edfed83e2758601a2b093822fc2dc4e9bfb00487e340f2bc4c5ac3bf6df3ca00b7fe55bb3df15858820f2bf698f403d2e48b915dd9eb47b63e0
This is useful for speeding up wallet rescans and is based on an
earlier version from PR #15845 ("wallet: Fast rescan with BIP157 block
filters"), which was never merged.
Co-authored-by: MacroFake <falke.marco@gmail.com>
Replace CommitInternal method with CustomCommit and use interfaces::Chain
instead of CChainState to generate block locator.
This commit does not change behavior in any way, except in the
(m_best_block_index == nullptr) case, which was added recently in
https://github.com/bitcoin/bitcoin/pull/24117 as part of an ongoing attempt to
prevent index corruption if bitcoind is interrupted during startup. New
behavior in that case should be slightly better than the old behavior (skipping
the entire custom+base commit now vs only skipping the base commit previously)
and this might avoid more cases of corruption.
Add new interfaces::BlockInfo struct to be able to pass extra block
information (file and undo information) to indexes which they are
updated to use high level interfaces::Chain notifications.
This commit does not change behavior in any way.
817326a828d6148dc63d9ef08f641b9c0c522411 wallet: avoid rescans if under the snapshot (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: #15606)
---
Refuse to load a wallet if it requires a rescan lower than the height of assumed-valid blocks.
Of course in live code right now, `BLOCK_ASSUMED_VALID` block index entries don't exist since they're a unique flag introduced by the use of UTXO snapshots, so this is prophylactic code exercised only by unittests.
ACKs for top commit:
achow101:
ACK 817326a828d6148dc63d9ef08f641b9c0c522411
ryanofsky:
Code review ACK 817326a828d6148dc63d9ef08f641b9c0c522411. This seems like the simplest change we can make to avoid wallet problems when an assumeutxo snapshot is loaded.
Tree-SHA512: cfa44b2eb33d1818d30df45210d0dde1e9b78cc9b7c88cb985054dc28427bba9e0905debe4196065d1d3a5ce7bca7e605e629d5ce5f0225b25395746e6d3d596
Currently, the wallet scan progress is not saved.
If it is interrupted, it will be necessary to start from
scratch on the next load.
With this change, progress is saved every 60 seconds.
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
Co-authored-by: Jon Atack <jon@atack.com>
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Refuse to load a wallet if it requires a rescan lower than the height of
an unvalidated snapshot we're running -- in more general terms, if we
don't have data for the blocks.
7e88f61b285e6a356ea4f6ba384858f109559985 multiprocess: Make interfaces::Chain::isTaprootActive non-const (Russell Yanofsky)
Pull request description:
`interfaces::Chain` is an abstract class, so declaring the method const would be exposing internal implementation details of subclasses to interface callers. And specifically this doesn't work because the multiprocess implementation of the `interfaces::Chain::isTaprootActive` method can't be const because IPC connection state and request state is not constant during the call.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
ACKs for top commit:
jamesob:
ACK 7e88f61b28
Tree-SHA512: 1c5ed89870aeb7170b9048c41299ab650dfa3d0978088e08c4c866fa0babb292722710b16f25540f26667220cb4747b1c256c4bd42893c552291eccc155346a3
interfaces::Chain is an abstract class, so declaring the method const
would be exposing internal implementation details of subclasses to
interface callers. And specifically this doesn't work because the
multiprocess implementation of the interfaces::Chain::isTaprootActive
method can't be const because IPC connection state and request state is
not constant during the call.
49ee2a0ad88e0e656234b769d806987784ff1e28 Avoid wallet code writing node settings file (Russell Yanofsky)
Pull request description:
Change wallet loading code to access settings through the Chain interface instead of writing settings.json directly. This is for running wallet and node in separate processes, since multiprocess code wouldn't easily work with different processes updating the same file.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). The commit was first part of larger PR #10102.
ACKs for top commit:
jamesob:
ACK 49ee2a0ad88e0e656234b769d806987784ff1e28 ([`jamesob/ackr/22217.1.ryanofsky.refactor_avoid_wallet_co`](https://github.com/jamesob/bitcoin/tree/ackr/22217.1.ryanofsky.refactor_avoid_wallet_co))
ryanofsky:
> ACK [49ee2a0](49ee2a0ad8) ([`jamesob/ackr/22217.1.ryanofsky.refactor_avoid_wallet_co`](https://github.com/jamesob/bitcoin/tree/ackr/22217.1.ryanofsky.refactor_avoid_wallet_co))
Zero-1729:
crACK 49ee2a0ad88e0e656234b769d806987784ff1e28
meshcollider:
Code review ACK 49ee2a0ad88e0e656234b769d806987784ff1e28
Tree-SHA512: a81c63b87816f739e02e3992808f314294d6c7213babaafdaaf3c4650ebc97ee4f98f9a4684ce4ff87372df59989b8ad5929159c5686293a7cce04e97e2fabba
5c5d0b62648e1b144b7b93c199f45265dac100e5 Add FoundBlock.found member (Russell Yanofsky)
Pull request description:
This change lets IPC serialization code handle FoundBlock arguments more simply and efficiently. Without this change there was no way to determine from a FoundBlock object whether a block was found or not. So in order to correctly implement behavior of leaving FoundBlock output variables unmodified when a block was not found, IPC code would have to read preexisting output variable values from the local process, send them to the remote process, receive output values back from the remote process, and save them to output variables unconditionally. With FoundBlock.found method, the process is simpler. There's no need to read or send preexisting local output variable values, just to read final output values from the remote process and set them conditionally if the block was found.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). The commit was first part of larger PR #10102.
ACKs for top commit:
fjahr:
Code review ACK 5c5d0b62648e1b144b7b93c199f45265dac100e5
theStack:
Concept and code review ACK 5c5d0b62648e1b144b7b93c199f45265dac100e5
jamesob:
ACK 5c5d0b62648e1b144b7b93c199f45265dac100e5 ([`jamesob/ackr/22215.1.ryanofsky.refactor_add_foundblock`](https://github.com/jamesob/bitcoin/tree/ackr/22215.1.ryanofsky.refactor_add_foundblock))
Zero-1729:
crACK 5c5d0b6
Tree-SHA512: d906e1b7100ff72c3aa06d80bd77673887b2db670ebd52dce7c4f6f557a23a1744c6109308228a37fda6c6ea74f05ba0efecff0ef235ab06ea8acd861fbb8675
This change lets IPC serialization code handle FoundBlock arguments more
simply and efficiently. Without this change there was no way to
determine from a FoundBlock object whether a block was found or not. So
in order to correctly implement behavior of leaving FoundBlock output
variables unmodified when a block was not found, IPC code would have to
read preexisting output variable values from the local process, send
them to the remote process, receive output values back from the remote
process, and save them to output variables unconditionally. With
FoundBlock.found method, the process is simpler. There's no need to read
or send preexisting local output variable values, just to read final
output values from the remote process and set them conditionally if the
block was found.
Using the zmq notifications to avoid excessive mempool polling can be difficult
given the current notifications available. It announces all transactions
being added to mempool or included in blocks, but announces no evictions
and gives no indication if the transaction is in the mempool or a block.
Block notifications for zmq are also substandard, in that it only announces
block tips, while all block transactions are still announced.
This commit adds a unified stream which can be used to closely track mempool:
1) getrawmempool to fill out mempool knowledge
2) if txhash is announced, add or remove from set
based on add/remove flag
3) if blockhash is announced, get block txn list,
remove from those transactions local view of mempool
4) if we drop a sequence number, go to (1)
The mempool sequence number starts at the value 1, and
increments each time a transaction enters the mempool,
or is evicted from the mempool for any reason, including
block inclusion. The mempool sequence number is published
via ZMQ for any transaction-related notification.
These features allow for ZMQ/RPC consumer to track mempool
state in a more exacting way, without unnecesarily polling
getrawmempool. See interface_zmq.py::test_mempool_sync for
example usage.
Add WalletClient interface so node interface is cleaner and don't need
wallet-specific methods.
The new NodeContext::wallet_client pointer will also be needed to eliminate
global wallet variables like ::vpwallets, because createWallet(), loadWallet(),
getWallets(), etc methods called by the GUI need a way to get a reference to
the list of open wallets if it is no longer a global variable.
Also tweaks splash screen registration for load wallet events to be delayed
until after wallet client is created.
This maintains a persistent list of wallets stored in settings that will
automatically be loaded on startup. Being able to load a wallet automatically
on startup will be more useful in the GUI when the option to create wallets is
added in #15006, but it's reasonable to expose this feature by RPC as well.
fs.cpp:35:17: error: no member named 'strerror' in namespace 'std'
return std::strerror(errno);
~~~~~^
fs.cpp:49:9: error: use of undeclared identifier 'close'
close(fd);
^
2 errors generated.
./interfaces/chain.h:265:55: error: ‘std::function’ has not been declared
virtual void rpcRunLater(const std::string& name, std::function<void()> fn, int64_t seconds) = 0;
^~~
This fix is a based on the fix by Antoine Riard <ariard@student.42.fr> in
https://github.com/bitcoin/bitcoin/pull/18600.
Unlike that PR, which implements some new behavior, this just restores previous
wallet notification and status behavior for transactions removed from the
mempool because they conflict with transactions in a block. The behavior was
accidentally changed in two `CWallet::BlockConnected` updates:
a31be09bfd77eed497a8e251d31358e16e2f2eb1 and
7e89994133725125dddbfa8d45484e3b9ed51c6e from
https://github.com/bitcoin/bitcoin/pull/16624, causing issue
https://github.com/bitcoin/bitcoin/issues/18325.
The change here could be improved and replaced with a more comprehensive
cleanup, so it includes a detailed comment explaining future considerations.
Fixes#18325
Co-authored-by: Antoine Riard <ariard@student.42.fr>
This change is intended to make the bitcoin node and its rpc, network
and gui interfaces more responsive while the wallet is in use. Currently
because the node's cs_main mutex is always locked before the wallet's
cs_wallet mutex (to prevent deadlocks), cs_main currently stays locked
while the wallet does relatively slow things like creating and listing
transactions.
This commit only remmove chain lock tacking in wallet code, and invert
lock order from cs_main, cs_wallet to cs_wallet, cs_main.
must happen at once to avoid any deadlock. Previous commit were only
removing Chain::Lock methods to Chain interface and enforcing they
take cs_main.
Remove LockChain method from CWallet and Chain::Lock interface.
