75cf9708a0 ci: add one more routable address to the VMs (docker containers) (Vasil Dimov)
1b93983bf5 test: make feature_bind_port_(discover|externalip).py auto-detect the skip condition (Vasil Dimov)
Pull request description:
`feature_bind_port_discover.py` and `feature_bind_port_externalip.py` require a routable address on the machine to run. Since that was not predictably available on CI, those tests required a manual setting up of IP addresses (e.g. using `ifconfig`) and then running the tests with a command line option telling them that the addresses are set up. The tests were not run in CI and [got rot](https://github.com/bitcoin/bitcoin/issues/31293#issuecomment-2497792487).
Change that to auto-detect, from the tests, whether the needed IP addresses are present and if yes, run the test, otherwise skip it. Also change the CI to configure the needed addresses when running the functional tests. This way the tests will be run regularly on CI.
Fixes: https://github.com/bitcoin/bitcoin/issues/31336
ACKs for top commit:
willcl-ark:
ACK 75cf9708a0
frankomosh:
Tested ACK 75cf9708a0. Built from source.
ryanofsky:
Code review ACK 75cf9708a0. Tested locally with and without the special addresses, and the detection seems to work well.
Tree-SHA512: 252911a37a06764f644a1a83c808f5255ac3bc74919426afa5d082c59e1ea924196354735f229d381cb5aff2340e001c2240bbadc8b5f27e5321fb4cfaef0fdb
81348576cc psbt, test: remove address type restrictions in test (rkrux)
Pull request description:
Because the corresponding Taproot fields were added in PSBT in #22558,
so these restrictions are no longer necessary.
ACKs for top commit:
kevkevinpal:
ACK [8134857](81348576cc)
polespinasa:
lgtm ACK 81348576cc
furszy:
utACK 81348576cc
Tree-SHA512: 5621509e103674ee5f39454871ca5acb2bf4c16b85dbbdc38abac22fbaea44cafe4bbd91dc3a5c6435b3859a4dc58f12bf7aeec2952c9f80fbbdad2adb52847c
1c500b1709 test: avoid non-loopback network traffic from node_init_tests/init_test (Vasil Dimov)
Pull request description:
The test `node_init_tests/init_test` calls:
`AppInitMain()` -> `StartMapPort()` -> `StartThreadMapPort()` -> `ThreadMapPort()` -> `ProcessPCP()` -> `PCPRequestPortMap()` -> `CreateSock()` and on the returned value from `CreateSock()` it calls the `Connect()` method.
Thus, change `BasicTestingSetup::BasicTestingSetup()` to set `-natpmp` to 0. This way `node_init_tests/init_test` or other tests will not do network activity due to `ThreadMapPort()`.
Also add a comment about `natpmp=0` in
`test/functional/test_framework/util.py`.
Also set `-dnsseed=0` in `BasicTestingSetup::BasicTestingSetup()` to
avoid DNS queries.
ACKs for top commit:
fjahr:
re-ACK 1c500b1709
ryanofsky:
Code review ACK 1c500b1709, just disabling -dnsseed since previous review, which makes sense.
Tree-SHA512: 3b275d91361804da6d1dc109dffe741ea4b3dd3be916eb12fa63efa233e13ea4dab5a9f8448bd8bf99bc41817f3b7a768abe91531a3f63eae8b4c912bfcbd13e
89af67d79f tests: Add some fuzz test coverage for command-specific args (Anthony Towns)
92df785859 tests: Add some test coverage for ArgsManager::AddCommand (Anthony Towns)
33c8090be9 ArgsManager: automate checking for correct command options (Anthony Towns)
186354a0d8 bitcoin-wallet: use command-specific options (Anthony Towns)
d21e82b7d6 ArgsManager: support command-specific options (Anthony Towns)
Pull request description:
Adds the ability to link particular options to one or more (`OptionsCategory::COMMANDS`) commands, and uses this feature
in `bitcoin-wallet`. Separates out the help information for these command-specific options (duplicating it if an option applies to multiple commands), and provides a function for checking at runtime if some options have been specified by the user that only apply to other commands.
#### Motivation
Currently, `ArgsManager` supports commands like `bitcoin-wallet dump` but while some of the options are command-specific (like `-dumpfile`), `ArgsManager` itself doesn't know that. As a result, `-dumpfile` is listed in the global help rather than under the relevant commands, and if you use `-dumpfile` with a different command that doesn't support it, `ArgsManager` cannot automatically report that as an error, resulting in the commands that don't support the option having to have error-handling specific to all the options they don't support.
#### Changes
Help output moves command-specific options under their associated commands:
Before:
```
Options:
-dumpfile=<file name>
When used with 'dump', writes out the records to this file. When used
with 'createfromdump', loads the records into a new wallet.
...
Commands:
createfromdump
Create new wallet file from dumped records
dump
Print out all of the wallet key-value records
```
After:
```
Commands:
createfromdump
Create new wallet file from dumped records
-dumpfile=<file name>
When used with 'dump', writes out the records to this file. When
used with 'createfromdump', loads the records into a new wallet.
dump
Print out all of the wallet key-value records
-dumpfile=<file name>
When used with 'dump', writes out the records to this file. When
used with 'createfromdump', loads the records into a new wallet.
```
Error messages are now generated automatically by `ArgsManager` rather than ad-hoc wallet code for each option:
Before:
```c++
if (args.IsArgSet("-dumpfile") && command != "dump" && command != "createfromdump") {
tfm::format(std::cerr, "The -dumpfile option can only be used with the \"dump\" and \"createfromdump\" commands.\n");
return false;
}
```
After:
```c++
std::vector<std::string> details;
if (!args.CheckCommandOptions(command, &details)) {
tfm::format(std::cerr, "Error: Invalid arguments provided:\n%s\n", util::MakeUnorderedList(details));
return false;
}
```
#### Limitations
- If an option applies to multiple commands, it shares the same help text. There's no way to provide per-command descriptions.
- Option parsing rules are unchanged — options still cannot appear after the command.
ACKs for top commit:
achow101:
ACK 89af67d79f
sedited:
Re-ACK 89af67d79f
ryanofsky:
Code review ACK 89af67d79f. Since last review: rebase, integration with ClearArgs and fuzz test, and Assume -> Assert switch
Tree-SHA512: 7ae7c3b74d0c8c4db8459e9f0b9c7498b2fa4758954ec49983decbba177877b039779f0f7b55e60c3a0ed74c5e9e4ac4734ba9e049bf3a7743280ef8300869fa
da769855d0 test: add PSBT proprietary merge regression coverage (w0xlt)
3f5b3c7a80 psbt: preserve proprietary fields when combining PSBTs (w0xlt)
Pull request description:
BIP 174 proprietary fields are currently parsed, serialized, and exposed by `decodepsbt`, but they are not preserved by `combinepsbt`.
The reason is that the merge paths in `PartiallySignedTransaction::Merge()`, `PSBTInput::Merge()`, and `PSBTOutput::Merge()` union `unknown`, but never union `m_proprietary`.
This means application-specific PSBT metadata can be lost during combination, even though BIP 174 treats proprietary records as normal PSBT key-value pairs for private or application-specific use.
This PR fixes that by preserving proprietary fields in all three merge paths.
ACKs for top commit:
nervana21:
re-ACK da769855d0
Bicaru20:
re-ACK da769855d0
achow101:
ACK da769855d0
theStack:
Code-review ACK da769855d0
Tree-SHA512: 4474674ac00c3155fd7d3d777bdeb70d4ca2006c8fe62eb84a888ef2f7aa02739bf31d8e9f2cb59e324be1085c77897f0bbf6673254c12408ab09a1582e28b83
b6c3670442 i2p: clean up SAM error logging (takeshikurosawaa)
Pull request description:
Clean up the I2P SAM error path.
`SESSION CREATE` may contain the private key, so the generic SAM reply
error path now reports the redacted request text instead of the full
request. It also avoids echoing raw router replies in those generic
error messages.
No network behavior change intended.
ACKs for top commit:
davidgumberg:
crACK b6c3670442
vasild:
ACK b6c3670442
Tree-SHA512: 204c8b64c6d3dd2f94f92cdc6d3daefd7773c42066984b9da859ebc2912c2ed38079d9e82a2d1f09d8d720750047114a80189e688929d7a0af5da2c2ee4a88da
8544537f41 mining: drop unused include_dummy_extranonce option (Sjors Provoost)
58eeab790d mining: only pad with OP_0 at heights <= 16 (Sjors Provoost)
00d22328b0 mining: pad coinbase to fix createNewBlock at heights <=16 (Sjors Provoost)
605ff37403 test: bad-cb-length for createNewBlock() at low heights (Sjors Provoost)
1966621b76 test: refactor IPC mining test to use script_BIP34_coinbase_height (Sjors Provoost)
Pull request description:
Blocks 0-16 on any new chain require mining code to be careful not to violate the `bad-cb-length` rule, which states the coinbase transaction scriptSig must be at least 2 bytes.
Our mining code deals with that by padding the `scriptSig` with a 0 `extraNonce`. It does this for every height. As a result IPC clients would get an unnecessary `0` in the `scriptSigPrefix` field of `CoinbaseTx`. #32420 fixed that by introducing a `include_dummy_extranonce` option in `BlockCreateOptions` and turning that off for IPC clients.
A minor issue was missed though: `createNewBlock()` now fails with `bad-cb-length`. An easy workaround is to use the `generate` RPC for the first 16 blocks, as demonstrated in the 2nd commit.
The real fix is to have the miner code always pad the `scriptSig` at lower heights, but to _not_ include that in the `scriptSigPrefix` field of `CoinbaseTx` (introduced in #33819). This is what the 3rd commit implements.
Now that we set `scriptSigPrefix` independent of what our internal miner code does - to get past `CheckBlock()` - the original motivation for `include_dummy_extranonce` goes away and we can just drop it entirely. The last commit drops it, while the 4th commit adjusts the tests and hardcoded block and assume utxo hashes.
This last change does not break IPC clients, because `include_dummy_extranonce` was never exposed in `mining.capnp`.
Instead of adjusting the hardcoded hashes, an alternative approach would be to just always pad the `scriptSig` internally, since we exclude the padding from `scriptSigPrefix` anyway. However, IPC clients can also call `getBlock()` to get the raw block and might be confused about the difference. The miner code is also easier to understand if we limit the exception (`coinbase_tx.script_sig_prefix != coinbaseTx.vin[0].scriptSig`) to `nHeight <= 16`, where the explanation is based purely on consensus rules rather than historical test suite reasons.
The first two commits are preperation test changes:
- extract `assert_capnp_failed` helper for macOS (also part of #34727)
- use `script_BIP34_coinbase_height` in IPC mining test (existing code in `interface_ipc_mining.py` was incorrect for low height
Fixes#35126
ACKs for top commit:
ryanofsky:
Code review ACK 8544537f41. Just rebased to fix silent conflict and applied some minor suggestions since last review. As part of rereviewing I left some more minor suggestions that are fine to ignore.
sedited:
Re-ACK 8544537f41
Tree-SHA512: a01d48842bf4bcc1a9c51a89ef9d750766db7d04edb4dcd6b3a8bf195c6b4fa07445256a49367ff0db00ab489a52a3d7ff6a5c3ab9290ecb1fcb82f532552e9b
CombinePSBTs currently preserves unknown records but drops proprietary records at the global, input, and output levels because the Merge() paths never union m_proprietary.
Preserve proprietary records in PartiallySignedTransaction::Merge(), PSBTInput::Merge(), and PSBTOutput::Merge() so combine/merge keeps all PSBT key-value data.
096bb0b5c0 bench: add benchmark for GetMappedAS() (0xb10c)
Pull request description:
With #28792 merged, we can use the embedded ASMap file to benchmark the IP -> ASN lookups. Before, this would have been cumbersome as it required having a real, external ASMap file. We want to benchmark against a real file, as a smaller test file has significantly faster lookups.
The benchmarks cover individual IP address lookups of mapped and unmapped IPv4 and IPv6 addresses along with a multi-IP lookup. For the IPs we assume to be mapped, we assert that they are mapped. Updating the embedded ASMap file might change the benchmark results slightly as some lookups will be a bit faster and others slower.
```
$ ./build/bin/bench_bitcoin --filter=ASMapGetMappedAS.* -min-time=5000
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 460.41 | 2,171,994.61 | 0.2% | 5.48 | `ASMapGetMappedASCloudflarev4`
| 356.96 | 2,801,460.45 | 0.0% | 5.33 | `ASMapGetMappedASCloudflarev6`
| 476.58 | 2,098,304.01 | 0.1% | 5.51 | `ASMapGetMappedASGooglev4`
| 359.17 | 2,784,224.15 | 0.0% | 5.33 | `ASMapGetMappedASGooglev6`
| 398.82 | 2,507,410.35 | 0.1% | 5.50 | `ASMapGetMappedASMulti`
| 346.11 | 2,889,237.31 | 0.3% | 5.32 | `ASMapGetMappedASQuad9v4`
| 274.21 | 3,646,832.70 | 0.1% | 5.50 | `ASMapGetMappedASQuad9v6`
| 5.32 | 187,875,440.96 | 0.1% | 5.50 | `ASMapGetMappedASUnmappedv4`
| 65.19 | 15,339,680.04 | 0.1% | 5.51 | `ASMapGetMappedASUnmappedv6`
```
_LLM disclosure: while I wrote the initial benchmarks, I had a LLM review it and point out style nits and typos to me._
ACKs for top commit:
l0rinc:
ACK 096bb0b5c0
fjahr:
ACK 096bb0b5c0
sipa:
ACK 096bb0b5c0
achow101:
ACK 096bb0b5c0
Tree-SHA512: 417427fd546fe7ec0903dde6f1a6557479a2e353eaf26be61dacc0031adae1a9970263b143fe9e2b83f489da01031cc83eff8e11ccedb00f5852e44c40189da2
8ba5f68b1d refactor, key: move `CreateMuSig2PartialSig` to `musig.{h,cpp}` module (Sebastian Falbesoner)
d087f266fc refactor, key: move `CreateMuSig2Nonce` to `musig.{h,cpp}` module (Sebastian Falbesoner)
f36d89f436 key: add `GetSecp256k1SignContext` access function (w0xlt)
Pull request description:
This PR is a follow-up of #29675, see https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2265077463. It moves all MuSig2 functions that currently live in `CKey` and call secp256k1 musig module API functions (i.e. `secp256k1_musig_...`) to the `musig.{h,cpp}` module, as this seems to be a better place. For accessing the `secp256k1_context_signing` object from the outside, a new function `GetSecp256k1SignContext` is added in the third commit.
As the patch is mostly move-only, it can be best reviewed via the git option `--color-moved=dimmed-zebra`
ACKs for top commit:
achow101:
ACK 8ba5f68b1d
w0xlt:
reACK 8ba5f68b1d
rkrux:
lgtm ACK 8ba5f68b1d
furszy:
ACK 8ba5f68b1d
Tree-SHA512: 95fcaa5d7a09037a0dce0053b8c640a7372a1251a2a3615c565f4dacc5aad5cf0ee8bfc43aa0d0def628465c16330d69f6ea9fcc07bbadc971863248f60d1878
With #28792 merged, we can use the embedded ASMap file to benchmark
the IP -> ASN lookups. Before, this would have been cumbersome as it
required having a real, external ASMap file. We want to benchmark
against a real file, as a smaller test file has significantly faster
lookups.
The benchmarks cover individual IP address lookups of mapped and
unmapped IPv4 and IPv6 addresses along with a multi-IP lookup.
For the IPs we assume to be mapped, we assert that they are mapped.
Updating the embedded ASMap file might change the benchmark results
slightly as some lookups will be a bit faster and others slower.
```
$ ./build/bin/bench_bitcoin --filter=ASMapGetMappedAS.* -min-time=5000
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 460.41 | 2,171,994.61 | 0.2% | 5.48 | `ASMapGetMappedASCloudflarev4`
| 356.96 | 2,801,460.45 | 0.0% | 5.33 | `ASMapGetMappedASCloudflarev6`
| 476.58 | 2,098,304.01 | 0.1% | 5.51 | `ASMapGetMappedASGooglev4`
| 359.17 | 2,784,224.15 | 0.0% | 5.33 | `ASMapGetMappedASGooglev6`
| 398.82 | 2,507,410.35 | 0.1% | 5.50 | `ASMapGetMappedASMulti`
| 346.11 | 2,889,237.31 | 0.3% | 5.32 | `ASMapGetMappedASQuad9v4`
| 274.21 | 3,646,832.70 | 0.1% | 5.50 | `ASMapGetMappedASQuad9v6`
| 5.32 | 187,875,440.96 | 0.1% | 5.50 | `ASMapGetMappedASUnmappedv4`
| 65.19 | 15,339,680.04 | 0.1% | 5.51 | `ASMapGetMappedASUnmappedv6`
```
Co-authored-by: Fabian Jahr <fjahr@protonmail.com>
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
SESSION CREATE requests can contain the I2P private key. Keep using the redacted request text in error messages, and avoid echoing raw SAM replies in the generic reply error path.
This keeps the error useful while avoiding logging either the private-key-bearing request or unescaped router-controlled reply bytes. No network behavior change is intended.
The test calls:
`AppInitMain()` -> `StartMapPort()` -> `StartThreadMapPort()` ->
`ThreadMapPort()` -> `ProcessPCP()` -> `PCPRequestPortMap()` ->
`CreateSock()` and on the returned value from `CreateSock()` it calls
the `Connect()` method.
Thus, change `BasicTestingSetup::BasicTestingSetup()` to set `-natpmp`
to 0. This way `node_init_tests/init_test` or other tests will not do
network activity due to `ThreadMapPort()`.
Also add a comment about `natpmp=0` in
`test/functional/test_framework/util.py`.
Also set `-dnsseed=0` in `BasicTestingSetup::BasicTestingSetup()` to
avoid DNS queries.
004a7e3cfb fuzz: Fix txorphan timeout by limiting block weight (marcofleon)
Pull request description:
The `EraseForBlock` branch in the `txorphan` harness could produce a block with 1000 transactions in it, each with potentially up to 200,000 inputs, resulting in way too many [map lookups](3cab711d69/src/node/txorphanage.cpp (L625)). This was producing inputs that were taking 2 seconds or longer per iteration, which is too long.
Fix by only adding transactions to the block up to the block weight limit. This matches production behavior, as `EraseForBlock` is only called on a newly [connected block](3cab711d69/src/net_processing.cpp (L2090)).
ACKs for top commit:
maflcko:
lgtm ACK 004a7e3cfb
instagibbs:
ACK 004a7e3cfb
sedited:
ACK 004a7e3cfb
Tree-SHA512: 465504402358e1bed629104b21e05301139f1590884de21e77d566a45e422eef6d4380c5714692f33f5398e4e299b8c9f84b82f58c56a98e410c5c841184aee5
801d36f55b fuzz: use ImmediateBackgroundTaskRunner to silence DEBUG_LOCKORDER (Eugene Siegel)
Pull request description:
DEBUG_LOCKORDER was reporting a false positive deadlock with the cmpctblock fuzz harness when using ImmediateTaskRunner. Since it is single-threaded, ImmediateTaskRunner callbacks added LockOrders that could never happen outside of a fuzz test.
First a block would get connected:
* LOCK(mempool.cs)
* BlockConnected (fuzz test runs in same thread)
* LOCK(m_tx_download_mutex)
Then a later iteration of the LIMITED_WHILE would send a TX:
* LOCK(m_tx_download_mutex)
* LOCK(mempool.cs)
causing a false positive deadlock. Normally, the BlockConnected callback would run in a different thread and no deadlock is reported.
Fix this by launching a thread that runs the callback and is immediately joined. I compared this PR to another branch (0028847c6b) that uses a ThreadPool and found this PR to be more stable and ~4% quicker.
ACKs for top commit:
maflcko:
lgtm ACK 801d36f55b
sedited:
ACK 801d36f55b
Tree-SHA512: 5e1f27354c484a90ffd74a1a11e5fed68d7b38d95c06dd39529217f07d535eb9a5b7befb42b2829d79c52b5eb43542bd1079c81d6eb0e56476352ffe98e0177e
a39cc16b43 doc: Release note for addhdkey (Ava Chow)
89b9a01b4e wallet, rpc: Disallow importing unused() to wallets without privkeys (Ava Chow)
35bbee6374 wallet, rpc: Disallow import of unused() if key already exists (Ava Chow)
f3f8bcbd1d wallet: Add addhdkey RPC (Ava Chow)
82bc280de4 test: Simple test for importing unused(KEY) (Ava Chow)
80c29bc6f1 descriptor: Add unused(KEY) descriptor (Ava Chow)
Pull request description:
It is sometimes useful for the wallet to have keys that it can sign with but are not (initially) involved in any scripts, e.g. for setting up a multisig. Ryanofsky [suggested](https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1867721948) A `unused(KEY)` descriptor which allows for a key to be specified, but produces no scripts. These can be imported into the wallet, and subsequently retrieved with `gethdkeys`. Additionally, `listdescriptors` will output these descriptors so that they can be easily backed up.
In order to make it easier for people to add HD keys to their wallet, and to generate a new one if they want to rotate their descriptors, an `addhdkey` RPC is also added. Without arguments, it will generate a new HD key and add it to the wallet via a `unused(KEY)` descriptor. If provided a private key, it will construct the descriptor and add it to the wallet.
See also: https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1866961865
Based on #29130 as `gethdkeys` is useful for testing this.
ACKs for top commit:
Sjors:
utACK a39cc16
rkrux:
lgtm ACK a39cc16b43
Tree-SHA512: c1288c792ab01ca2eaddd24b0e7d11c259cd59e79042465d0d1eb656fd559c1200dc19750b4d84acc762b5b599935a06df214c18226e662087842ea91ec3011b
6d86184a8b rpc: combinerawtransaction now rejects unmergeable transactions (Adam Andrews)
Pull request description:
Previously, combinerawtransaction would silently return the first tx when asked to combine unrelated txs. Now, it will check tx mergeability and throws a descriptive error if tx cannot be merged.
fixes #25980
ACKs for top commit:
nervana21:
tACK 6d86184a8b
achow101:
ACK 6d86184a8b
rkrux:
ACK [6d86184](6d86184a8b)
Tree-SHA512: 5caf983c5ab618a000f40b9ad698439d3e6217ec2dc593740443f47d90f8804a895f3054dd29bbcecdb48a61992b0d0afda7ec89591d768a44918648bbb6e20d
dc84a31014 wallet: remove fUpdate argument from AddToWalletIfInvolvingMe (rkrux)
94845df073 wallet: remove update_tx argument from SyncTransaction (rkrux)
6e796e1f47 wallet: remove fUpdate argument from ScanForWalletTransactions (rkrux)
54e4c0be8f wallet: remove update argument from RescanFromTime method (rkrux)
Pull request description:
This caught my attention while going through #32993.
The corresponding boolean arguments from CWallet methods related to
updating transactions during the blockchain scanning have been removed
because effectively these arguments were always passed as true making
the need for the boolean arguments unnecessary. The only falsy call sites
were in the unit tests that don't need to test scenarios that never happen
in actuality.
ACKs for top commit:
Bicaru20:
ACK dc84a31014. I also think that makes sense to remove the update argument if it is not used.
achow101:
ACK dc84a31014
sedited:
ACK dc84a31014
l0rinc:
untested code review ACK dc84a31014
Tree-SHA512: dfee793c88cc94d4a5b554e29710133ec4ad5e49455413245c7d7004ef71b618b0b2f821fa085415c34d80aeccd182c98e55223d84afbe8b408f64acac0571bb
0065f354a7 doc: clarify libfuzzer-nosan preset uses build_fuzz_nosan dir (ImMike)
Pull request description:
Adds a clarifying note next to the first mention of the `libfuzzer-nosan` preset in the Quickstart, pointing out that it uses a different build directory (`build_fuzz_nosan`, per [`CMakePresets.json` L54](https://github.com/bitcoin/bitcoin/blob/master/CMakePresets.json#L54)).
A reader following the quickstart with `--preset=libfuzzer-nosan` and then running `cmake --build build_fuzz` as shown would otherwise operate against the wrong (or empty) directory.
Pure docs; no code changes.
ACKs for top commit:
l0rinc:
ACK 0065f354a7
maflcko:
lgtm ACK 0065f354a7
sedited:
ACK 0065f354a7
Tree-SHA512: d73901112d259cec58746dff50fe3f9409e5b9826f0759f45478fe039bca851eb163036c60bdb215bfc66be79428b790742bbe8bc32b1ceaa2d6f80c17faf6d0
ca93ab808c doc: mention -DWITH_ZMQ=ON in BSD build guides (junbyjun1238)
Pull request description:
The BSD build guides currently state:
> If the package is installed, support will be compiled in.
Since `WITH_ZMQ` defaults to `OFF`, this is inaccurate: installing the dependency alone does not enable ZMQ support. Update the wording to mention the required `-DWITH_ZMQ=ON` CMake option, matching `doc/zmq.md`.
Docs-only change; no tests run.
ACKs for top commit:
maflcko:
lgtm ACK ca93ab808c
sedited:
ACK ca93ab808c
Tree-SHA512: d07b1b9748d8b6aa555c992608f8659b7e93d6587bbbb2170352342003e90a81592ddfa2abf3a26b8ebf1341142d8628cfc3faba0c4759655a91446afa5fc22a
The previous commit made CreateNewBlock() ignore the
include_dummy_extranonce flag (OP_0 is now appended only at heights
<= 16 to satisfy bad-cb-length, regardless of caller-supplied options).
Remove the now-unused field from BlockCreateOptions and clean up all
the call sites that still set it (RPC, bench, fuzz, test setups).
Drop the include_dummy_extranonce branch from the OP_0 padding
condition in CreateNewBlock(), so that the dummy extraNonce is
only appended when consensus actually requires it (heights <= 16,
where the BIP34 height push alone would yield a 1-byte scriptSig
and trigger bad-cb-length).
The include_dummy_extranonce option struct field is now unused by
the miner and is removed in the next commit. Callers still set it,
so that this commit compiles.
Regenerate the hardcoded coinbase / block hashes throughout the
unit and functional test suites and update the regtest assumeutxo
snapshot in chainparams.
Additional side-effects:
- Without the dummy extranonce, coinbase scriptSigs are 1 byte
shorter at heights > 16, making every block 1 byte smaller.
This shifts where block files wrap and therefore where pruning
boundaries land.
- feature_assumeutxo malleation cases:
- case 1: error message changes due to UTXO reordering, similar
to 8f2078af6a
- case 4: the corruption byte is swapped from \x82 to \x83
because \x82 happened to be the actual value at that
offset in the new snapshot.
Since #32420, createNewBlock has thrown `bad-cb-length` errors when called at
low block heights because `OP_0` padding stopped being added to coinbase
transactions. (#32420 did add an `include_dummy_extranonce` option which could
bypass this, but it was not exposed to IPC clients.) Fix the problem by padding
coinbase transactions with `OP_0` when necessary to produce valid blocks.
Additionally this commit stops adding `OP_0` padding to the template
`script_sig_prefix` field when `include_dummy_extranonce` is true. This is safe
because non-IPC clients don't use this field, and IPC clients could never set
the option to true, and are expected to add their own nonces in any case.
This also improves documentation about the `script_sig_prefix` field and
`getCoinbaseTx` method.
DEBUG_LOCKORDER was reporting a false positive deadlock with the
cmpctblock fuzz harness when using ImmediateTaskRunner. Since it is
single-threaded, ImmediateTaskRunner callbacks added LockOrders that
could never happen outside of a fuzz test.
First a block would get connected:
* LOCK(mempool.cs)
* BlockConnected (fuzz test runs in same thread)
* LOCK(m_tx_download_mutex)
Then a later iteration of the LIMITED_WHILE would send a TX:
* LOCK(m_tx_download_mutex)
* LOCK(mempool.cs)
causing a false positive deadlock. Normally, the BlockConnected
callback would run in a different thread and no deadlock is reported.
Fix this by launching a thread that runs the callback and is
immediately joined.
The FreeBSD, NetBSD, and OpenBSD build guides state that ZMQ support is compiled in when the package is installed. Since WITH_ZMQ defaults to OFF, update the wording to mention the required CMake option.
On new regtest / signet chains (heights <= 16), createBlock() fails
internally with bad-cb-length. Since mining.capnp does not expose
include_dummy_extranonce, IPC clients can't work around this.
Add a functional test to illustrate this issue and use RPC to
work around it. The next commit introduces a fix.
Needed in a later commit to correctly derive the BIP34 prefix
for heights <= 16.
Add a padding parameter to script_BIP34_coinbase_height() that
controls whether the OP_0 dummy extranonce is appended for
heights <= 16.
Use this helper with padding=False in the IPC mining test's
build_coinbase_test().
Compared to `CreateMuSig2Nonce`, creating a partial signature
has a stronger link to the secret key used, but for consistency
reasons it still makes sense to move all functionality that call
the secp256k1 musig API functions to the `musig.{h,cpp}` module
for consistency.
Can be reviewed via the git option `--color-moved=dimmed-zebra`.
Nonce creation is mainly derived by randomness, and the secret
key merely serves as (optional) additional data for increasing
misuse-resistance, rather than being a central part that would
justify an own CKey method, so move it to the musig.cpp module.
Can be reviewed via the git option `--color-moved=dimmed-zebra`.
The fuzzing quickstart documents the libfuzzer preset with build_fuzz
as the build directory, then mentions libfuzzer-nosan as an alternative
without noting that this preset uses a different binary directory
(build_fuzz_nosan, per CMakePresets.json line 54). A reader following
the quickstart with --preset=libfuzzer-nosan and then running
'cmake --build build_fuzz' as shown will operate against the wrong
(or empty) directory.
Add a clarifying note at the first mention in the Quickstart.
701bc2dc02 contrib: Fix NameError in signet miner gbt() (Torkel Rogstad)
Pull request description:
The logging.warning call referenced `bci["bestblockhash"]`, a variable from the calling scope `do_generate()` that is not available inside the `Generate.gbt()` method. This would crash with a NameError when getblocktemplate returned a template based on an unexpected previous block.
Use the `bestblockhash` parameter that was already being passed in and used correctly in the comparison on the line above.
The bug was introduced in 7b31332370 when the gbt logic was extracted into its own method — the if-condition was updated but the logging call was not.
ACKs for top commit:
kevkevinpal:
ACK [701bc2d](701bc2dc02)
sedited:
ACK 701bc2dc02
Tree-SHA512: 53764db954dd40b68d2f92279fd609732402fdbac4a1838aeee81d366b2b7fbc8da86b07f6f05cee62cb8ecdc514f6e0af1e59c65380eec38cbeaa58ec3c10ed
This refactor makes the ruff.toml easier to read, because it lists the
ignored checks instead of having to enumerate all checks to select in a
large collection of rules.
2424e52836 lint: doc: detail lint tool install methods (will)
5fefa5a654 Don't pin Python patch version (Sjors Provoost)
fd15b55c2e lint: use requirements.txt (will)
5f4d3383da lint: switch to ruff for formatting and linting (will)
a53b81ce4e lint: switch to uv for python management in linter (will)
Pull request description:
Modernise our lint tooling by:
\- Replacing pyenv + pip with [uv](https://docs.astral.sh/uv/) for better Python environment and dependency management
\- Move uv ruff and ty to install via `COPY --from` multi-stage Docker image imports
\- Moving ruff lint rules from hardcoded Rust array (in lint_py.rs) into a top-level ruff.toml
\- Extracting all remaining pip dependencies into dedicated ci/lint/requirements.txt
Extra rationale:
`COPY --from` pulls pre-built binaries from upstream images instead of compiling/downloading at runtime. Containerfile layer optimisations reduce rebuild frequency further.
Pinning tool versions in the dockerfile makes it more excplicit and easier to find.
The tradeoff we make here is that there is no longer a single install script to install tooling on a local machine. However I think this is OK, as it currently only works for `apt`-based OSes anyway, and I don't think running the linter outside of the container is such a valuable use-case as it is with some of the other CI jobs.
ACKs for top commit:
maflcko:
review ACK 2424e52836🗿
sedited:
ACK 2424e52836
Tree-SHA512: 32ef989c1e241cebe5f13da10abd23f6f63306591fd1f81880d688b886082bca17987591dc592c41fbb72278eba57b3cc6e786de7cfa80eb490ab34465d0119b
ac1ccc5bd9 build: Add CTAD feature check (Pol Espinasa)
9f273f1c1c build: Add path to doc recommended versions for CLANG, GCC and MSVC (Pol Espinasa)
Pull request description:
Adds a compiler features check. If failed, it returns a fatal error.
This early stop at configure time will avoid contributors losing time trying to fix compilation errors because of not fitting the requirements.
Example of the output for GCC:
Version requirement satisfied:
```
$ cmake -B build
-- The CXX compiler identification is GNU 13.3.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
...
-- Checking for required C++ features
-- Checking for required C++ features - done
....
```
Compiler feature requirement not satisfied:
```
$ cmake -B build -DCMAKE_C_COMPILER=/usr/bin/gcc-10 -DCMAKE_CXX_COMPILER=/usr/bin/g++-10
-- The CXX compiler identification is GNU 10.5.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/g++-10 - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Setting build type to "RelWithDebInfo" as none was specified
-- Performing Test CXX_SUPPORTS__WERROR
-- Performing Test CXX_SUPPORTS__WERROR - Success
-- Found SQLite3: /usr/include (found suitable version "3.45.1", minimum required is "3.7.17")
-- Checking for required C++ features
CMake Error at cmake/module/CheckCXXFeatures.cmake:32 (message):
Compiler lacks Class Template Argument Deduction (CTAD) for aggregates.
This C++ feature is required for src/util/overloaded.h.
You are probably using an old compiler version
The recommended compiler versions can be checked in:
- GCC -> doc/dependencies.md#compiler
- Clang -> doc/dependencies.md#compiler
- MSVC -> doc/build-windows-msvc.md
Call Stack (most recent call first):
CMakeLists.txt:198 (check_cxx_features)
-- Configuring incomplete, errors occurred!
```
Example of the output for Clang:
Compiler feature requirement satisfied:
```
$ cmake -B build -DCMAKE_C_COMPILER=clang-20 -DCMAKE_CXX_COMPILER=clang++-20
-- The CXX compiler identification is Clang 20.1.2
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/clang++-20 - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
...
-- Checking for required C++ features
-- Checking for required C++ features - done
...
```
Compiler feature requirement not satisfied:
```
$ cmake -B build -DCMAKE_C_COMPILER=clang-16 -DCMAKE_CXX_COMPILER=clang++-16
-- The CXX compiler identification is Clang 16.0.6
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/clang++-16 - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Setting build type to "RelWithDebInfo" as none was specified
-- Performing Test CXX_SUPPORTS__WERROR
-- Performing Test CXX_SUPPORTS__WERROR - Success
-- Performing Test CXX_SUPPORTS__G3
-- Performing Test CXX_SUPPORTS__G3 - Success
-- Performing Test LINKER_SUPPORTS__G3
-- Performing Test LINKER_SUPPORTS__G3 - Success
-- Performing Test CXX_SUPPORTS__FTRAPV
-- Performing Test CXX_SUPPORTS__FTRAPV - Success
-- Performing Test LINKER_SUPPORTS__FTRAPV
-- Performing Test LINKER_SUPPORTS__FTRAPV - Success
-- Found SQLite3: /usr/include (found suitable version "3.45.1", minimum required is "3.7.17")
-- Checking for required C++ features
CMake Error at cmake/module/CheckCXXFeatures.cmake:32 (message):
Compiler lacks Class Template Argument Deduction (CTAD) for aggregates.
This C++ feature is required for src/util/overloaded.h.
You are probably using an old compiler version
The recommended compiler versions can be checked in:
- GCC -> doc/dependencies.md#compiler
- Clang -> doc/dependencies.md#compiler
- MSVC -> doc/build-windows-msvc.md
Call Stack (most recent call first):
CMakeLists.txt:198 (check_cxx_features)
-- Configuring incomplete, errors occurred!
```
Edit: The first version of the PR was to check hardcoded compiler versions. See previous PR description for context here.
<details>
Adds a compiler minimum version check. If failed, it returns a fatal error.
This early stop at configure time will avoid contributors losing time trying to fix compilation errors because of not fitting the requirements.
Example of the output for GCC:
Version requirement satisfied:
```
sliv3r@sliv3r-tuxedo:~/Documentos/Projectes/BitcoinCore/bitcoin$ cmake -B BUILD
-- The CXX compiler identification is GNU 13.3.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
....
```
Version requirement not satisfied:
```
$ cmake -B build -S . -DCMAKE_C_COMPILER=/usr/bin/gcc-11 -DCMAKE_CXX_COMPILER=/usr/bin/g++-11
-- The CXX compiler identification is GNU 11.5.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/g++-11 - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
CMake Error at CMakeLists.txt:91 (message):
GCC >= 12.1 required.
-- Configuring incomplete, errors occurred!
```
Example of the output for Clang:
Version requirement satisfied:
```
sliv3r@sliv3r-tuxedo:~/Documentos/Projectes/BitcoinCore/bitcoin$ cmake -B build
-- The CXX compiler identification is Clang 18.1.3
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
...
```
Version requirement not satisfied:
```
$ cmake -B build -DCMAKE_C_COMPILER=clang-16 -DCMAKE_CXX_COMPILER=clang++-16
-- The CXX compiler identification is Clang 16.0.6
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/clang++-16 - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
CMake Error at CMakeLists.txt:86 (message):
Clang >= 17.0 required.
-- Configuring incomplete, errors occurred!
```
<\detail>
ACKs for top commit:
davidgumberg:
crACK ac1ccc5bd9
achow101:
ACK ac1ccc5bd9
vasild:
ACK ac1ccc5bd9
w0xlt:
ACK ac1ccc5bd9
ryanofsky:
Code review ACK ac1ccc5bd9. Very much agree with all the previous comments to not check specific compiler versions, and glad this PR is in better shape now.
Tree-SHA512: 15a12f56016846427ed8273406c3e9a52a6435bf7033c8fef82bb0e6c9fd75d40a62c79d33f9dd6da8717500f21a629da07620f160b3d43368dd7579f773b4bc
f24a7b5f75 doc: recommend script_flags instead of deployments.taproot (Sjors Provoost)
Pull request description:
#26201 removed `taproot` from `getdeploymentinfo` (not yet in a release, slated for v32), which it turns out Lnd relies on: https://github.com/lightningnetwork/lnd/pull/10683
Expand the release doc note to recommend the same solution they used: check for `TAPROOT` in the `script_flags` array. This was added in v31.
ACKs for top commit:
maflcko:
Stakeholder-aligned SLA-compliant production-ready enterprise-grade lgtm ACK f24a7b5f75
sedited:
ACK f24a7b5f75
Tree-SHA512: 5c21300ce3140eb1dd122e007b2f2cfafa3de83db4fb7c699312d6886d4c8fea56d9602390c1931fc640bf9364b2274dde85b38da4957311ddc07721cbc924a7
d084bc88be doc: clarify IWYU workflow (Lőrinc)
7c7cec4567 ci: update IWYU patch reference (Lőrinc)
Pull request description:
### Problem
This was prompted by https://github.com/bitcoin/bitcoin/pull/34435#discussion_r3123255248, where it was not clear to me how (and where) exceptional IWYU cases should be documented.
### Fix
This PR documents the IWYU CI wrapper as the reproducible local entrypoint.
The developer notes now recommend reducing suspected IWYU false positives to a minimal upstream reproducer, treat `IWYU pragma` as a narrow workarounds, and ask for nearby rationale comments on non-obvious IWYU pragma use. An example comment was also added.
The IWYU patch comment is also updated to point at the current `clang_22` include picker reference.
### Reproducer
Create a dummy commit on top that adds an unused include, then run the command from the developer notes.
Without the dummy commit, the command should pass.
<details><summary>IWYU demo commit</summary>
```diff
diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
--- a/src/kernel/bitcoinkernel.cpp(revision c92b329e7b7d49476b5977d26c24d7c4982c6024)
+++ b/src/kernel/bitcoinkernel.cpp(revision ad2c5ba2ba69156e77061c1e6c098b725c28f322)
@@ -43,6 +43,7 @@
#include <functional>
#include <list>
#include <memory>
+#include <vector>
#include <span>
#include <stdexcept>
#include <string>
```
</details>
> [!NOTE]
> After repeated failing runs, `docker container rm -f ci_native_iwyu` may be needed because the local CI wrapper can leave the detached container running when the inner test command fails.
ACKs for top commit:
hebasto:
ACK d084bc88be.
sedited:
ACK d084bc88be
Tree-SHA512: 0aac42d468a1fdfa9f4a3856372e05fb43ec9f0973aeb3a4194fff948fc61e8e72e3b280cde10e74b8da88b6cff93962b3a7f7390eb042113ef92aa6b51d6d8f
e2b0984f99 wallet: check BDB last page LSN (Lőrinc)
Pull request description:
### Problem
Legacy wallet migration uses the read-only BDB parser to verify that every page LSN is reset before reading records without BDB log files.
The BDB `last_page` metadata field stores the last valid page number, but the parser treated it like a page count and scanned only `0..<last_page`:
e2b0984f99/src/wallet/migrate.cpp (L87)
This skipped the final page, so a database whose last page still depended on BDB logs could be accepted.
### Fix
Scan LSNs through `last_page` inclusively.
ACKs for top commit:
achow101:
ACK e2b0984f99
w0xlt:
ACK e2b0984f99
sedited:
ACK e2b0984f99
Tree-SHA512: 26fade6cdb4747d299b6e620646aa14751cd91fbb7e40ab6e35c1ca796fb589a2340d66108b812611f2924136a8f12c4f911efe6346fffaf04b2d3d288101cda
4defc466a2 cmake: Set `CTEST_NIGHTLY_START_TIME` for CDash Nightly pipelines (Hennadii Stepanov)
Pull request description:
This PR follows up on https://github.com/bitcoin/bitcoin/pull/35222.
According to the [CMake documentation](https://cmake.org/cmake/help/latest/variable/CTEST_NIGHTLY_START_TIME.html) for `CTEST_NIGHTLY_START_TIME`:
> ... this variable must always be set for a nightly build in a dashboard script.
Examples of nightly build reports utilizing this configuration can be found on the [Bitcoin Core CDash board](https://my.cdash.org/index.php?project=bitcoin-core), based on [this commit](98aad4f72f).
ACKs for top commit:
ferminquant:
ACK 4defc466a2
purpleKarrot:
ACK 4defc466a2
Tree-SHA512: 7d3f5dc9f9f1336fc03f565d36750c66bdbae5a3916dabf4ab02c3c2584fc7135a8839e94d7fa08bcbb530913cf0eea31fc402dbdc1c22e207402c991dac044d
9f7a2293c4 depends: Unset `SOURCE_DATE_EPOCH` in `gen_id` script (Hennadii Stepanov)
Pull request description:
When performing Guix builds for `{x86_64,arm64}-apple-darwin` hosts across different commits, all packages in `depends` are rebuilt even if there are no changes in either the `depends` or `contrib/guix` subdirectories.
This occurs because the `SOURCE_DATE_EPOCH` environment variable enables Clang's `-source-date-epoch` option, which then appears in the output of `clang -v -E -xc -o /dev/null - < /dev/null`. For example:
```
$ SOURCE_DATE_EPOCH=1767855465 clang -v -E -xc++ -o /dev/null - < /dev/null
clang version 21.1.7 (Fedora 21.1.7-1.fc43)
Target: x86_64-redhat-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
Configuration file: /etc/clang/x86_64-redhat-linux-gnu-clang.cfg
System configuration file directory: /etc/clang/
Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-redhat-linux/14
Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-redhat-linux/15
Selected GCC installation: /usr/bin/../lib/gcc/x86_64-redhat-linux/15
Candidate multilib: .;@m64
Candidate multilib: 32;@m32
Selected multilib: .;@m64
(in-process)
"/usr/bin/clang-21" -cc1 -triple x86_64-redhat-linux-gnu -E -disable-free -clear-ast-before-backend -disable-llvm-verifier -discard-value-names -main-file-name - -mrelocation-model static -mframe-pointer=all -fmath-errno -ffp-contract=on -fno-rounding-math -mconstructor-aliases -funwind-tables=2 -target-cpu x86-64 -tune-cpu generic -debugger-tuning=gdb -fdebug-compilation-dir=/home/hebasto -v -fcoverage-compilation-dir=/home/hebasto -resource-dir /usr/bin/../lib/clang/21 -internal-isystem /usr/bin/../lib/gcc/x86_64-redhat-linux/15/../../../../include/c++/15 -internal-isystem /usr/bin/../lib/gcc/x86_64-redhat-linux/15/../../../../include/c++/15/x86_64-redhat-linux -internal-isystem /usr/bin/../lib/gcc/x86_64-redhat-linux/15/../../../../include/c++/15/backward -internal-isystem /usr/bin/../lib/clang/21/include -internal-isystem /usr/local/include -internal-isystem /usr/bin/../lib/gcc/x86_64-redhat-linux/15/../../../../x86_64-redhat-linux/include -internal-externc-isystem /include -internal-externc-isystem /usr/include -source-date-epoch 1767855465 -fdeprecated-macro -ferror-limit 19 -fmessage-length=180 -fgnuc-version=4.2.1 -fskip-odr-check-in-gmf -fcxx-exceptions -fexceptions -fcolor-diagnostics -faddrsig -D__GCC_HAVE_DWARF2_CFI_ASM=1 -o /dev/null -x c++ -
clang -cc1 version 21.1.7 based upon LLVM 21.1.7 default target x86_64-redhat-linux-gnu
ignoring nonexistent directory "/usr/bin/../lib/gcc/x86_64-redhat-linux/15/../../../../x86_64-redhat-linux/include"
ignoring nonexistent directory "/include"
#include "..." search starts here:
#include <...> search starts here:
/usr/bin/../lib/gcc/x86_64-redhat-linux/15/../../../../include/c++/15
/usr/bin/../lib/gcc/x86_64-redhat-linux/15/../../../../include/c++/15/x86_64-redhat-linux
/usr/bin/../lib/gcc/x86_64-redhat-linux/15/../../../../include/c++/15/backward
/usr/bin/../lib/clang/21/include
/usr/local/include
/usr/include
End of search list.
```
As a result, each package id differs between builds, which causes the script to treat the toolchain as changed and triggers unnecessary rebuilds.
This PR resolves this issue by [clobbering](https://github.com/bitcoin/bitcoin/pull/34228#issuecomment-4387135450) the `SOURCE_DATE_EPOCH` value in the `gen_id` script.
---
Suggested testing scenario:
```
$ env HOSTS=arm64-apple-darwin ./contrib/guix/guix-build
$ git commit --allow-empty -m "Trigger rebuild"
$ env HOSTS=arm64-apple-darwin ./contrib/guix/guix-build
````
The last command will rebuild depends on the master branch, but will successfully use the cached built packages on this PR.
ACKs for top commit:
maflcko:
lgtm ACK 9f7a2293c4
fanquake:
ACK 9f7a2293c4
Tree-SHA512: d5fa90100edfd88024ad949a9d75c8af274a054c0926ae4ddea6ecf1c38fae833670b360c56c740a7565cf393a92597823749fe75c85a27ae1ebebead4093853
