* Make the methods of `CThreadInterrupt` virtual and store a pointer to
it in `CConnman`, thus making it possible to override with a mocked
instance.
* Initialize `CConnman::m_interrupt_net` from the constructor, making it
possible for callers to supply mocked version.
* Introduce `FuzzedThreadInterrupt` and `ConsumeThreadInterrupt()` and
use them in `src/test/fuzz/connman.cpp` and `src/test/fuzz/i2p.cpp`.
This improves the CPU utilization of the `connman` fuzz test.
As a nice side effect, the `std::shared_ptr` used for
`CConnman::m_interrupt_net` resolves the possible lifetime issues with
it (see the removed comment for that variable).
Now that all network calls done by `CConnman::OpenNetworkConnection()`
are done via `Sock` they can be redirected (mocked) to `FuzzedSocket`
for testing.
`FuzzedSock::Accept()` properly returns a new socket, but it forgot to
set the output argument `addr`, like `accept(2)` is expected to.
This could lead to reading uninitialized data during testing when we
read it, e.g. from `CService::SetSockAddr()` which reads the `sa_family`
member.
Set `addr` to a fuzzed IPv4 or IPv6 address.
12ff4be9c7 test: ensure -rpcallowip is compatible with RFC4193 (Matthew Zipkin)
c02bd3c187 config: Explain RFC4193 and CJDNS interaction in help and init error (Matthew Zipkin)
f728b6b111 init: Configure reachable networks before we start the RPC server (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/32433
`MaybeFlipIPv6toCJDNS()` relies on `g_reachable_nets` to distinguish between CJDNS addresses and other IPv6 addresses. In particular, [RFC4193](https://www.rfc-editor.org/rfc/rfc4193#section-3.1) address or "Unique Local Address" with the L-bit unset also begins with the `fc` prefix. #32433 highlights a use case for these addresses that have nothing to do with CJDNS.
On master we don't parse init flags like `-cjdnsreachable` until *after* the HTTP server has started, causing conflicts with `-rpcallowip` because CJDNS doesn't support subnets.
This PR ensures that `NET_CJDNS` is only present in the reachable networks list if set by `-cjdnsreachable` *before* `-rpcallowip` is checked. If it is set all `fc` addresses are assumed to be CJDNS, can not have subnets, and can't be set for `-rpcallowip`.
I also noted this specific parameter interaction in the init help as well as the error message if configured incorrectly.
This can be tested locally:
`bitcoind -regtest -rpcallowip=fc00:dead:beef::/64 -rpcuser=u -rpcpassword=p`
On master this will just throw an error that doesn't even mention IPv6 at all.
On the branch, this will succeed and can be tested by adding the ULA to a local interface.
On linux: `sudo ip -6 addr add fc00:dead:beef::1/64 dev lo`
On macos: `sudo ifconfig lo0 inet6 fc00:dead:beef::1/128 add`
then: `curl -v -g -6 --interface fc00:dead:beef::1 u:p@[::1]:18443 --data '{"method":"getblockcount"}'`
If the `rpcallowip` option is removed, the RPC request will fail to authorize.
Finally, adding `-cjdnsreachable` to the start up command will throw an error and specify the incompatibility:
> RFC4193 is allowed only if -cjdnsreachable=0.
ACKs for top commit:
achow101:
ACK 12ff4be9c7
tapcrafter:
tACK 12ff4be9c7
ryanofsky:
Code review ACK 12ff4be9c7
willcl-ark:
ACK 12ff4be9c7
Tree-SHA512: a4dd70ca2bb9f6ec2c0a9463fd73985d1ed80552c674a9067ac9a86662d1c018cc275ba757cebb2993c5f3971ecf4778b95d35fe7a7178fb41b1d18b601c9960
cfc42ae5b7 fuzz: add a target for the coins database (Antoine Poinsot)
46e14630f7 fuzz: move the coins_view target's body into a standalone function (Antoine Poinsot)
56d878c465 fuzz: avoid underflow in coins_view target (Antoine Poinsot)
Pull request description:
This reopens https://github.com/bitcoin/bitcoin/pull/28216.
The current `coins_view` target only tests `CCoinsViewCache` using a basic `CCoinsView` instance. The addition of the `coins_view_db` target enables testing with an actual `CCoinsViewDB` as the backend.
ACKs for top commit:
maflcko:
lgtm ACK cfc42ae5b7
l0rinc:
code review ACK cfc42ae5b7
TheCharlatan:
ACK cfc42ae5b7
Tree-SHA512: d3a92f122629f075767453a1abd9819a1c9716db53b997418993fef62d27683324740d0a8f84df76d8a7a45e508ccadeb69553b6f69e29a1238cd7c0be5276ca
f98e1aaf34 rpc: Note in fundrawtransaction doc, fee rate is for package (benthecarman)
Pull request description:
Accidentally made some transactions with a much higher fee rate than I wanted because I did not know this would do it for the package rather than the individual tx.
ACKs for top commit:
achow101:
ACK f98e1aaf34
rkrux:
re-ACK f98e1aaf34
danielabrozzoni:
ACK f98e1aaf34
Tree-SHA512: 9f961de1200803ec4d1c6901fd606bb6cf707ffd03942d9dc0d4b6554c827075f99d693b93e892f728679d67e63e12c71da4426dab091b3311d1605bc37251a2
83bfe1485c build: add -Wthread-safety-pointer (fanquake)
240a4fb95d Squashed 'src/leveldb/' changes from 113db4962b..aba469ad6a (fanquake)
Pull request description:
This will become available in Clang 21:
> ThreadSafetyAnalysis now supports -Wthread-safety-pointer, which
> enables warning on passing or returning pointers to guarded variables
> as function arguments or return value respectively. Note that
> ThreadSafetyAnalysis still does not perform alias analysis. The
> feature will be default-enabled with -Wthread-safety in a future release.
See https://github.com/llvm/llvm-project/blob/main/clang/docs/ReleaseNotes.rst.
Also updates the leveldb subtree to pull: https://github.com/bitcoin-core/leveldb-subtree/pull/54.
ACKs for top commit:
davidgumberg:
Tested ACK 83bfe1485c
maflcko:
lgtm ACK 83bfe1485c
theuni:
utACK 83bfe1485c
Tree-SHA512: 9bc80bd04a9cebed8aca20bc23a17e52a6a89a1fb042993322f43dbf7bd93de509c091ebb69255063833b098ab11a64285eccf61e17b9f94f974c734a20ad8da
86e1111239 test: verify node skips loading legacy wallets during startup (furszy)
9f94de5bb5 wallet: init, don't error out when loading legacy wallets (furszy)
Pull request description:
Instead of failing during initialization and shutting down the app when encountering a legacy wallet, skip loading the wallet and notify the user accordingly.
This allows users to access migration functionalities without needing to manually remove the wallet from settings.json or resort to using the bitcoin-wallet utility.
This means that GUI users will be able to use the migration button, and bitcoin-cli users will be able to call the migratewallet RPC directly after init.
ACKs for top commit:
achow101:
ACK 86e1111239
w0xlt:
ACK 86e1111239
Tree-SHA512: 85d594a503ee7a833a23754b71b6ba4869ca34ed802c9ac0cd7b2fa56978f5fcad84ee4bd3acdcc61cf8e7f08f0789336febc5d76beae1eebf7bd51462512b78
ad9a13fc42 walletdb: Log additional exception error messages for corrupted wallets (Ava Chow)
Pull request description:
Many exceptions thrown for corruption are `std::runtime_error`; we should catch those and log the message to help with debugging.
Split from #32489
ACKs for top commit:
davidgumberg:
ACK ad9a13fc42
furszy:
ACK ad9a13fc42
rkrux:
ACK ad9a13fc42
Sjors:
utACK ad9a13fc42
Tree-SHA512: 107b938d67346804733ea27c44ed38822db0e020e5b1ac889ee35280d812ec56dcc9af7b3eab7a521d72cdd9cb4a8d6d35f3a3dfbcb2a6fd170a981f34fbdfc2
f3a444c45f gui: Disallow loading legacy wallets (Ava Chow)
09955172f3 wallet, rpc: Give warning in listwalletdir for legacy wallets (Ava Chow)
Pull request description:
A new field `warnings` is added for each wallet in `listwalletdir`. If a legacy wallet is detected, the warning will contain a message that the wallet is a legacy wallet and will need to be migrated before it can be loaded.
In the GUI, the "Open Wallet" menu is changed to show legacy wallets greyed out with "(needs migration)" appended to their name to indicate to the user that the legacy wallet will need to be migrated.
ACKs for top commit:
maflcko:
lgtm ACK f3a444c45f
adyshimony:
Test ACK [f3a444c](f3a444c45f)
furszy:
Code review ACK f3a444c45f
w0xlt:
Code Review ACK f3a444c45f
Tree-SHA512: 496caec0ca37845487bd709e592240315eb23461fbd697e68a7fde8e4d9b74b48aab1212c88dbbcc8a107a896b824c2e1f69691068641812ae903f873fa2f22b
24e5fd3bed fs: remove _POSIX_C_SOURCE defining (fanquake)
Pull request description:
On Linux systems, `_POSIX_C_SOURCE` will default to `200809L` (since glibc 2.10). There's currently no reason for us to undefine it, and then set it to an earlier value. Also tested with musl libc.
I think if anything, the project should be settings macros like `_POSIX_C_SOURCE`, globally.
ACKs for top commit:
hebasto:
re-ACK 24e5fd3bed, only rebased since my recent [review](https://github.com/bitcoin/bitcoin/pull/32460#pullrequestreview-2854183748).
Tree-SHA512: 920d60058821992193616e0c73c2f7e4230a9e3ccb9d71d16493ae69696c868f4325d3dd2d4e8388749080c187aa7b205493b3e2c6986ad37440e591ebe107e1
It reuses the logic from the `coins_view` target, except it uses an
in-memory CCoinsViewDB as the backend.
Note `CCoinsViewDB` will assert the best block hash is never null, so we
slightly modify the coins_view fuzz logic to take care of this.
83df64d749 log: Stats when fulfilling GETBLOCKTXN (David Gumberg)
3733ed2dae log: Size of missing tx'es when reconstructing compact block (David Gumberg)
36bcee05dc log: Log start of compact block initialization. (David Gumberg)
Pull request description:
This PR adds some additional logging to help measure performance of compact block reconstruction.
1. Adds a message to the beginning of `PartiallyDownloadedBlock::InitData()` so that that the logs indicate the amount of time it takes to populate a compact block from mempool transactions.
2. Logs the size of the transactions which a node did not have in its mempool and was forced to request.
3. Logs the size and number of transactions that a node sends to it's peer in a `BLOCKTXN` to fulfill a compact block `GETBLOCKTXN` request.
Relevant to this discussion on delving bitcoin: https://delvingbitcoin.org/t/stats-on-compact-block-reconstructions/1052
ACKs for top commit:
instagibbs:
reACK 83df64d749
w0xlt:
reACK 83df64d749
1440000bytes:
ACK 83df64d749
Tree-SHA512: 92c3c7d55005dd47dad90ddb54e4127482260cea5390f7696e8b3b9defb337f5fb09166af6b12eb2ab8151d04dae08b0a570e3509a86509b0ab3151d84387e06
fab1e02086 refactor: Pass verification_progress into block tip notifications (MarcoFalke)
fa76b378e4 rpc: Round verificationprogress to exactly 1 for a recent tip (MarcoFalke)
faf6304bdf test: Use mockable time in GuessVerificationProgress (MarcoFalke)
Pull request description:
Some users really seem to care about this. While it shouldn't matter much, the diff is so trivial that it is probably worth doing.
Fixes#31127
One could also consider to split the field into two dedicated ones (https://github.com/bitcoin/bitcoin/issues/28847#issuecomment-1807115357), but this is left for a more involved follow-up and may also be controversial.
ACKs for top commit:
achow101:
ACK fab1e02086
pinheadmz:
ACK fab1e02086
sipa:
utACK fab1e02086
Tree-SHA512: a3c24e3c446d38fbad9399c1e7f1ffa7904490a3a7d12623b44e583b435cc8b5f1ba83b84d29c7ffaf22028bc909c7cec07202b825480449c6419d2a190938f5
3e6ac5bf77 refactor: validation: mark CheckBlockIndex as const (stickies-v)
61a51eccbb validation: don't use GetAll() in CheckBlockIndex() (stickies-v)
d05481df64 refactor: validation: mark SnapshotBase as const (stickies-v)
Pull request description:
While reviewing another PR, I [noticed](https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2056509235) that `ChainstateManager::CheckBlockIndex()` is not a `const` method. To try and assert that this method was not causing any side-effects, I modified the method to make it `const`. It did not surface any errors, but I think it would be good to merge this change regardless, even if `CheckBlockIndex` is only used in regtest.
This PR removes `CheckBlockIndex()`'s calls to non-const `ChainstateManager` methods by marking `SnapshotBase` `const` and ~inlining the `GetAll()` calls (thereby also performing consistency checks on invalid or fully validated `m_disabled==true` chainstates, as slight behaviour change), and finally marks `CheckBlockIndex()` as `const`.
ACKs for top commit:
achow101:
ACK 3e6ac5bf77
mzumsande:
Code Review ACK 3e6ac5bf77
TheCharlatan:
ACK 3e6ac5bf77
Tree-SHA512: 3d3cd351f5af1fab9a9498218ec62dba6e397fc7b5f4868ae0a77dc2b7c813d12c4f53f253f209101a3f6523695014e20c82dfac27cf0035611d5dd29feb80b5
09ee8b7f27 node: avoid recomputing block hash in `ReadBlock` (Lőrinc)
2bf173210f test: exercise `ReadBlock` hash‑mismatch path (Lőrinc)
Pull request description:
Eliminate one block header hash calculation per block-read by reusing the hash for:
* proof‑of‑work verification;
* (optional) integrity check against the supplied hash.
This part of the code wasn't covered by tests either, so the first commit exercises this part first, before pushing the validation to the delegate method.
ACKs for top commit:
maflcko:
lgtm ACK 09ee8b7f27
achow101:
ACK 09ee8b7f27
jonatack:
ACK 09ee8b7f27
pinheadmz:
ACK 09ee8b7f27
Tree-SHA512: 43fe51b478ea574b6d4c952684b13ca54fb8cbd67c3b6c136f460122d9ee953cc70b88778537117eecea71ccb8d88311faeac21b866e11d117f1145973204ed4
a5ac43d98d doc: Add release notes describing bitcoin wrapper executable (Ryan Ofsky)
258bda80c0 doc: Mention bitcoin wrapper executable in documentation (Ryan Ofsky)
d2739d75c9 build: add bitcoin.exe to windows installer (Sjors Provoost)
ba649c0006 ci: Run multiprocess tests through wrapper executable (Ryan Ofsky)
29bdd743bb test: Support BITCOIN_CMD environment variable (Ryan Ofsky)
9c8c68891b multiprocess: Add bitcoin wrapper executable (Ryan Ofsky)
5076d20fdb util: Add cross-platform ExecVp and GetExePath functions (Ryan Ofsky)
Pull request description:
Intended to make bitcoin command line features more discoverable and allow installing new multiprocess binaries in libexec/ instead of bin/ so they don't cause confusion.
Idea and implementation of this were discussed in https://github.com/bitcoin/bitcoin/issues/30983.
---
Initial implementation of this feature is deliberately minimal so the UX can evolve in response to feedback and there are not too many details to debate and discuss in a single PR. But many improvements are possible or planned:
- Adding manpage and bash completions.
- Showing nicer error messages that detect if an executable isn't installed and suggest how to fix [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2073194474)
- Showing wrapper command lines in subcommand in help output [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077800405). This could be done conditionally as suggested in the comment or be unconditional.
- Showing wrapper command lines in subcommand error output. There is a bitcoin-cli error pointed out in [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091152243) that is needlessly confusing.
- Integrating help so `bitcoin help subcommand` invokes `bitcoin subcommand -h`. `bitcoin -h subcommand` should also be supported and be equivalent [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2093116725)
- Adding support for `bitcoin-util` subcommands. Ideal interface would probably be more like `bitcoin grind` not `bitcoin util grind` but this has been punted for now. Supporting subcommands directly would require some ArgsManager modifications
- Adding a dedicated python functional test for the wrapper. Right now there is some CI coverage by setting the `BITCOIN_CMD` variable, but this doesn't cover things like the help output and version output, and support for different directory layouts.
- Better `--multiprocess` (`-m`) / `--monolithic` (`-M`) default selection. Right now, default is monolithic but it probably makes sense to chose more intelligently depending on whether -ipc options are enabled and what binaries are available.
- Maybe parsing `bitcoin.conf` and supporting options to control wrapper behavior like custom locations or preferences or aliases.
- Better command command line usability. Allow combining short options like (`-ah`). Allow fuzzy matching of subcommands or suggestions if you misspell. (suggested by stickies in review club)
- Not directly related to this PR but `bitcoin-cli named` implementation used by the wrapper should do a better job disambiguating named arguments from base64 arguments ending in = as pointed out in [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091886628)
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722). A review club meeting for it took place in https://bitcoincore.reviews/31375
ACKs for top commit:
Sjors:
utACK a5ac43d98d
achow101:
ACK a5ac43d98d
vasild:
ACK a5ac43d98d
theStack:
ACK a5ac43d98d
ismaelsadeeq:
fwiw my last review implied an ACK a5ac43d98d
hodlinator:
ACK a5ac43d98d
Tree-SHA512: 570e6a4ff8bd79ef6554da3d01f36c0a7c6d2dd7dace8f8732eca98f4a8bc2284474a9beadeba783114fe2f3dd08b2041b3da7753bae0b7f881ec50668cb821f
Instead of allowing users to load a legacy wallet from the "Open Wallet"
menu, show the legacy wallet greyed out with a message that the wallet
needs to be migrated.
Eliminate one SHA‑256 double‑hash computation of the header per block read by reusing the hash for:
* proof‑of‑work verification;
* (optional) integrity check against the supplied hash.
This is the RPC example counterpart to commit
86de8c1668 (PR #32544).
Since the recent legacy wallet removal this parameter *must* be
true, so providing it in the examples doesn't contain valuable
information anymore and it seems best to remove them.
Now that we only ever operate on descriptor wallets, mentioning
that a faster rescan is only available for them is redundant and
can be removed.
These texts were originally introduced in commit
ca48a4694f (PR #25957).
This `getwalletinfo()` result field was only ever returned for
legacy wallets and is hence not relevant anymore, so we can
delete it and the corresponding CWallet/ScriptPubKeyMan code
behind it.
fd290730f5 validation: clean up and clarify CheckInputScripts logic (Cory Fields)
1a37507895 validation: use a lock for CCheckQueueControl (Cory Fields)
c3b0e6c7f4 validation: make CCheckQueueControl's CCheckQueue non-optional (Cory Fields)
4c8c90b556 validation: only create a CCheckQueueControl if it's actually going to be used (Cory Fields)
11fed833b3 threading: add LOCK_ARGS macro (Cory Fields)
Pull request description:
As part of an effort to cleanup our threading primitives and add safe `SharedMutex`/`SharedLock` impls, I'd like to get rid of the last of our legacy `ENTER_CRITICAL_SECTION`/`LEAVE_CRITICAL_SECTION` usage. This, along with a follow-up [after fixing REVERSE_LOCK](https://github.com/bitcoin/bitcoin/pull/32465) will allow us to do that.
This replaces the old macros with an RAII lock, while simplifying `CCheckQueueControl`. It now requires a `CCheckQueue`, and optionality is handled externally. In the case of validation, it is wrapped in a `std::optional`.
It also adds an `LOCK_ARGS` macro for `UniqueLock` initialization which may be helpful elsewhere.
ACKs for top commit:
fjahr:
re-ACK fd290730f5
ryanofsky:
Code review ACK fd290730f5, just removing assert since last review. Thanks for considering all the comments and feedback!
TheCharlatan:
Re-ACK fd290730f5
Tree-SHA512: 54b9db604ee1bda7d11bce1653a88d3dcbc4f525eed6a85abdd4d6409138674af4bb8b00afa4e0d3d29dadd045a3a39de253a45f0ef9c05f56cba1aac5b59303
