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
7cfbb8575e test: wallet: cover wallet passphrase with a null char (brunoerg)
Pull request description:
This PR adds test coverage for the `walletpassphrase`/`walletpassphrasechange` RPC when the passphrase is incorrect due to a null character.
For reference: https://github.com/bitcoin/bitcoin/pull/27068 introduced the usage of `SecureString` to allow null characters.
ACKs for top commit:
maflcko:
lgtm ACK 7cfbb8575e
achow101:
ACK 7cfbb8575e
w0xlt:
Code review ACK 7cfbb8575e
BrandonOdiwuor:
Code Review ACK 7cfbb8575e
theStack:
ACK 7cfbb8575e
pablomartin4btc:
cr ACK 7cfbb8575e
Tree-SHA512: ecdb48662ceb6c55c4b301ca7f537c3159ece7b66ee40ea977583ffb74bd3d06e334ab3a5639a9cde3aa6443129f412f9aea0ee5a8b73b31dba0728d0890b7f1
9653ebc053 depends: remove support for Windows Qt LTO builds (fanquake)
Pull request description:
The related Windows patches were dropped in 5e794e6202, and "Cross-compiling does not support LTO." (from #30997).
ACKs for top commit:
maflcko:
lgtm ACK 9653ebc053
Tree-SHA512: 40ae7b17669bf87f2e848055e85e1a6c946f0bb0bc1674e18f1622ec4a0613fe955a4daf83928c9375035dac289ce2a72dd7f347b15f86d108157f9da9499945
4ce53495e5 doc: update tor docs to use bitcoind binary from path (ismaelsadeeq)
Pull request description:
I noticed this while trying to run a node over Tor.
Using `./bitcoind` as the executable path is incorrect.
This is a simple documentation update PR that fixes the path by removing the prefix and just
having `bitcoind` as the usage example targeting those who have Bitcoin Core in their PATH.
ACKs for top commit:
davidgumberg:
ACK 4ce53495e5
janb84:
ACK 4ce53495e5
jonatack:
ACK 4ce53495e5
Tree-SHA512: a23c94a175f77d66ee1a81599a15a809ad768090eebb619c8e4a67b8a020a2256da4f40cec3c00ec35775b265d3c53cdb70c09fbed48d399416fbc9156ebff31
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
ed179e0a65 test: apply microsecond precision to test framework logging (Martin Zumsande)
Pull request description:
When analyzing functional test logs (produced with `combine_logs.py`), entries sometimes sort slightly out of order because even though python prints 6 digits for microsecond precision, it fills up the last 3 digits with zeroes. For example, it may look like a message was received by the test framework before it was sent by the node.
Change this to actually use microsecond precision - this should make combined logs a little bit easier to analyze.
ACKs for top commit:
davidgumberg:
Tested ACK ed179e0a65
achow101:
ACK ed179e0a65
maflcko:
review ACK ed179e0a65 🗳
janb84:
ACK ed179e0a65
Tree-SHA512: 55cdb5024e8e910c5a5ce741ce512eb88f4f82f11f378ba0fe7a5a2b1c97d2e7b540bdf5603c76aab837d35798610b165f087fbeb7c9dc90aaad890bf4d0323d
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
df9ebbf659 depends: use "mkdir -p" when installing xproto (fanquake)
Pull request description:
It looks like the mkdir detection in xproto is broken on Alpine. Ensure we always use `mkdir -p`.
Fixes#32494.
ACKs for top commit:
hebasto:
ACK df9ebbf659, I have reviewed the code and it looks OK.
janb84:
ACK df9ebbf659
willcl-ark:
ACK df9ebbf659
Tree-SHA512: 0f23b1096ffdf5ffa13115665dc42b65835b78bb0ab04a8be8f210980356953ab518e1273302fe4c9239361201f4f9ac737c0ebf10625f4817f81b65b3b25572
e50312eab0 doc: fix typos (fanquake)
c797e50dda ci: update codespell to 2.4.1 (fanquake)
21ee656337 doc: Remove obselete link in notificator.cpp (strmfos)
ee4406c04a doc: update URLs (fanquake)
Pull request description:
A round up of #32629 + some other changes that had previously been PR'd.
ACKs for top commit:
maflcko:
review ACK e50312eab0🥗
Tree-SHA512: 8fa3e14fdfa0cf65a42debc9cbb1f8b379aba44aa185e2e27337431e884d169bf1e811655c3a884d918e65ea28c5767ddaabaf25c862ebd9b4b38a0229ec5a93
6ee32aaaca test: signet tool genpsbt and solvepsbt commands (Sjors Provoost)
0a99d99fe4 signet: miner skips PSBT step for OP_TRUE (Sjors Provoost)
cdfb70e5a6 signet: split decode_psbt miner helper (Sjors Provoost)
Pull request description:
[BIP325](https://github.com/bitcoin/bips/blob/master/bip-0325.mediawiki) mentions the following rule:
> In the special case where an empty solution is valid (ie scriptSig and scriptWitness are both empty) this additional commitment can optionally be left out. This special case is to allow non-signet-aware block generation code to be used to test a custom signet chain where the challenge is trivially true.
Such a signet can be created using e.g. `-signetchallenge=51` (`OP_TRUE`). However `contrib/signet/miner` won't omit the commitment.
This PR improves the miner by skipping the PSBT for known trivial scripts (just `OP_TRUE` and trivial pushes for now). This prevents it from appending the 4 byte signet header to the witness commitment, as allowed by the above rule.
---
Previously the script would fail with `PSBT signing failed`, making it difficult to mine. This is no longer the case.
ACKs for top commit:
achow101:
ACK 6ee32aaaca
theStack:
re-ACK 6ee32aaaca
danielabrozzoni:
ACK 6ee32aaaca
Tree-SHA512: e47fbf471f2909286a6c1c073799ea388b9c19551afcce96cf9af45cc48d25c02f1e48e08861a88b604361e2c107a759d5baf393da8a37360de419f31651758a
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
4b1b36acb4 doc: Remove build instruction for running `clang-tidy` (Hennadii Stepanov)
Pull request description:
One of the benefits of using a compilation database, which is available after the CMake build system generation step, is that it is not necessary to actually build the code in order to run `clang-tidy`.
ACKs for top commit:
TheCharlatan:
ACK 4b1b36acb4
janb84:
ACK 4b1b36acb4
Tree-SHA512: cf28fb1bcff83016b927522f1c719f3b91df7d107a310250c550308c8544b212fa6d2e8a5502d69fa424421acdf952469edd67504ac2a8465a2c1520593a1f26
One of the benefits of using a compilation database, which is available
after the CMake build system generation step, is that it is not
necessary to actually build the code in order to run `clang-tidy`.
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.
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
b1ea542ae6 test: test MAX_SCRIPT_SIZE for block validity (Greg Sanders)
Pull request description:
I don't believe there are direct tests for this.
ACKs for top commit:
achow101:
ACK b1ea542ae6
TheCharlatan:
ACK b1ea542ae6
theStack:
ACK b1ea542ae6
Tree-SHA512: 1d7d3eab9c54977844bf2ca1aa403b070aae0f818db2fb5cae367d1c4d12f1e403b6fdec224af769a2ebb648cbca8bfd0d7df5db2a89fccf256c9c244484eba2
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
84aa484d45 test: fix transaction_graph_test reorg test (Greg Sanders)
eaf44f3767 test: check chainlimits respects on reorg (Greg Sanders)
47894367b5 functional test: add MAX_DISCONNECTED_TX_POOL_BYTES coverage (Greg Sanders)
Pull request description:
`DisconnectedBlockTransactions::LimitMemoryUsage()` has unit test coverage, but the default value end to end doesn't have coverage.
This test adds exercised coverage of memory limiting of the disconnect pool, and some basic behavior sanity checks.
Another test added is making sure chainlimits are being respected on reorg, and the expected transactions pruned.
Lastly, fix the existing test case which was using a deficient test via directly inducing reorgs with `invalidateblock`
ACKs for top commit:
maflcko:
re-ACK 84aa484d45🚋
TheCharlatan:
ACK 84aa484d45
Tree-SHA512: f5cdb9647fadc8eb30352ce38de44064103825e5358787dfccd6416fa8faf6ceea42552fe2250b37d56271a6c3898b3912e1c028652da122f5c99304aafddb64
dbb2d4c3d5 windows: Add application manifest to `bitcoin.exe` (Hennadii Stepanov)
df82c2dc17 windows: Add resource file for `bitcoin.exe` (Hennadii Stepanov)
Pull request description:
This PR is a follow up to https://github.com/bitcoin/bitcoin/pull/31375, which:
1. Adds a resource file for `bitcoin.exe` for consistency with other Windows executables.
2. Adds an application manifest to `bitcoin.exe`, which has been required for release binaries since https://github.com/bitcoin/bitcoin/pull/32396.
ACKs for top commit:
davidgumberg:
ACK dbb2d4c3d5
hodlinator:
ACK dbb2d4c3d5
Tree-SHA512: 853c9e578bfd74bfd2e1f0fa39f978638723c8e061456caa165fca6f10497517f9503ae12dfb88e7229a02de593ccf22126f3362ca0d75c74becbb727e80c9ad
4df4df45d7 test: fix sync function in rpc_psbt.py (Martin Zumsande)
Pull request description:
Even though the block is created on `node2`, the sync is only between `node1` and `node0`. Accordingly the test fails if I put a sleep in `msg_type == NetMsgType::HEADERS` processing: In this case, `node1` and `node0` do not hear about the new block, the sync still passes because they are in sync with each other, and later on in the `test_input_confs_control` subtest, `node1` would generate a forked block instead of building on the previous one, leading to test failure.
Haven't seen this in the CI, but I ran into it on an experimental branch.
ACKs for top commit:
maflcko:
lgtm ACK 4df4df45d7
achow101:
ACK 4df4df45d7
Tree-SHA512: 1211ba0ad263ebcd0aa6ef7c856dec7ec6ca6010e1df705e7243f6c9d950ccca6df1275c36a73a83034f49ea8401e8f9800c05cdb74c39e860e7ebcaf2ce6ada
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
