48df0939e7 fuzz: Remove unnecessary thread pool mutexes (marcofleon)
a4c3b003f8 fuzz: Speed up dbwrapper_concurrent_reads harness (marcofleon)
Pull request description:
Limiting how many read queries each worker executes significantly speeds up this test, especially when running with sanitizers. This still builds the full query list, and then takes the first 128/2000 after each worker shuffles it. I think that keeps some more operation diversity vs just lowering the query max directly. It also allowed me to reuse and test against a corpus I already had. Let me know if I'm wrong, but I don't think this test needs every worker to execute an identical query list to be effective.
This PR also reverts the `num_entries` max from 3000 back to 5000, as that didn't have much effect on input speed and restores a bit of lost coverage.
Lastly, as https://github.com/bitcoin/bitcoin/pull/35455#discussion_r3394357385 points out, remove the unnecessary `Mutex` from `StartReadPoolIfNeeded()`. Fuzz targets are entered sequentially within a process and parallel fuzzing uses separate processes/forks, so a mutex to prevent two in-process callers from racing to start the pool isn't needed.
ACKs for top commit:
sedited:
ACK 48df0939e7
brunoerg:
ACK 48df0939e7
Tree-SHA512: 24c35e13fa26790b36e5190283b78dd8511165d576841cb701f541a36c5b7c0a73f9bc265e71598a879fa1df9a9438e7841736f72b967214196fa6fe68569eff
61020b36c5 doc: add release note for #35182 replace libevent HTTP server (Matthew Zipkin)
39e9099da5 logging: deprecate libevent category (Matthew Zipkin)
8c1eea0777 http: remove libevent usage from this subsystem (Matthew Zipkin)
e427c227fa fuzz: switch http_libevent::HTTPRequest to http_bitcoin::HTTPRequest (Matthew Zipkin)
21c7542cf8 http: switch servers from libevent to bitcoin (Matthew Zipkin)
cbb8d1fb33 HTTPServer: disconnect after idle timeout (-rpcservertimeout) (Matthew Zipkin)
e5f242eef3 HTTPServer: implement control methods to match legacy API (Matthew Zipkin)
2ca645c2e4 refactor: split HTTPBindAddresses into config parse and libevent setup (Matthew Zipkin)
fec6b6bca8 refactor: split http_request_cb into libevent callback and dispatch (Matthew Zipkin)
f946ff5a0b Add helper methods to HTTPRequest to match original API (Matthew Zipkin)
dd11b5e01b define HTTP request methods at module level outside of class (Matthew Zipkin)
7ee7df988e HTTPServer: use a queue to pipeline requests from each connected client (Matthew Zipkin)
5ef1b80a09 Allow http workers to send data optimistically as an optimization (Matthew Zipkin)
a69bb9e1e6 HTTPServer: disconnect clients (Matthew Zipkin)
cdf71998e5 HTTPServer: compose and send replies to connected clients (Matthew Zipkin)
6734bcdeff HTTPserver: support "chunked" Transfer-Encoding (Matthew Zipkin)
80e1cfe5a2 HTTPServer: read requests from connected clients (Matthew Zipkin)
3c5226ab96 HTTPServer: start an I/O loop in a new thread and accept connections (Matthew Zipkin)
4ef4ebdc0c http: Introduce HTTPRemoteClient class (Matthew Zipkin)
a85286c5c7 HTTPServer: generate sequential Ids for each newly accepted connection (Matthew Zipkin)
5a3aa1af28 HTTPServer: implement and test AcceptConnection() (Matthew Zipkin)
f5bc018948 http: Introduce HTTPServer class and implement binding to listening socket (Matthew Zipkin)
9463e98781 http: Implement HTTPRequest class (Matthew Zipkin)
ad50aa4a0f http: Implement HTTPResponse class (Matthew Zipkin)
68b5d289d1 http: Implement HTTPHeaders class (Matthew Zipkin)
89c54ae4cb http: enclose libevent-dependent code in a namespace (Matthew Zipkin)
5aa3629b48 util/string: LineReader should only trim \r or \r\n (Matthew Zipkin)
0cdbb191b5 util/string: use string_view in LineReader (Matthew Zipkin)
881d4b6c75 test: cover common HTTP attacks and common malformed requests (Matthew Zipkin)
Pull request description:
Continued from #32061.
This is a major component of [removing libevent as a dependency of the project](https://github.com/bitcoin/bitcoin/issues/31194), by replacing the HTTP server used for RPC and REST with one implemented entirely within the Bitcoin Core codebase. The new `HTTPServer` class runs its own I/O thread, handling socket connections with code based on #30988, but tailored specifically for HTTP.
Commit strategy:
- Isolate the existing libevent-based HTTP server in a namespace `http_libevent`
- Implement HTTP in a new namespace `http_bitcoin` (classes like `HTTPRequest`, `HTTPClient`, etc...)
- Switch bitcoind from the libevent server to the new server
- Clean up (delete `http_libevent`)
## RFC compliance
I ran https://www.http-probe.com/ against v31.0 release and this PR branch at commit ddefe4263b42. While both HTTP servers failed several of the very picky compliance tests, the only differences in test results between the two branches is an *improvement* in the test results of the PR branch.
Raw test results:
- [v31.0](https://gist.github.com/pinheadmz/bb0debd49e8556417e9f0072a4463b31)
- [PR Branch](https://gist.github.com/pinheadmz/7267cdd90ff9deee8034788b5d1148de)
Test result comparison:
- https://gist.github.com/pinheadmz/08a5814345e02bf1ac1304e8e5b163a1
- Summary: `Changed results: 33 (Fail→Pass: 32, Pass→Fail: 0, other: 1)`
## Fuzz testing
### libfuzzer
- 4 days x8 cores at ddefe4263b
- generated corpus: https://github.com/pinheadmz/qa-assets/tree/httprequest-pr35182-ddefe4263b
- coverage report: https://thebitcoinblockclock.com/demo/libfuzzer_pr35182_ddefe4263b/index.html
### fuzzamoto
- 4 days x8 cores at commit ddefe4263b with a [patched fuzzer](https://github.com/pinheadmz/fuzzamoto/tree/http)
- coverage report: https://thebitcoinblockclock.com/demo/fuzzamoto_pr35182_ddefe4263b/coverage/bitcoin/index.html
## Integration testing:
I am testing the new HTTP server by forking projects that integrate with bitcoin via HTTP and running their integration tests with bitcoind built from this branch (on Github actions). I will continue adding integrations over time, and re-running these CI tests as this branch gets rebased:
- ✅ [lnd](https://github.com/pinheadmz/lnd/pull/7) master at `f21262dd6` tested with this branch at `ddefe4263b`
- ✅ [NodeJS `rpc-bitcoin`](https://github.com/pinheadmz/rpc-bitcoin/pull/4) master at `eb0c5da7d1094629cb4a381733cb01c9bb9a4608` tested with this branch at `ddefe4263b`
- ✅ [electrs](https://github.com/pinheadmz/electrs/pull/5) master at `d81dc35ae8f71c22b96cf23ee279b69683f29919` tested with this branch at `ddefe4263b`
- ✅ [eclair](https://github.com/pinheadmz/eclair/pull/4) master at `2dda79468a8b69a2acf7962cdac63245f7cc3ee8` tested with this branch at `ddefe4263b`
- ✅ [corepc](https://github.com/pinheadmz/corepc/pull/4) master at `818e5f2929ea5113176150b2d9f7c0248992d99c` tested with this branch at `ddefe4263b`
- ✅ [bitcoinjs-lib](https://github.com/pinheadmz/bitcoinjs-lib/pull/4) master at `ab9fad5978bc1f4fb6542d1cde903d4427c3344e` tested with this branch at `ddefe4263b`
- ✅ [NodeJS `bitcoin-core`](https://github.com/pinheadmz/bitcoin-core/pull/2) master at `24f3d0be7ab73089542de42cc8e31903727dbdf8` tested with this branch at `ddefe4263b`
## Benchmarks
To get a general idea of server speed vs. master I ran the functional test suite on all three platforms, three trials each, from the first and last commit of this branch. The first commit represents master, but adds a few extra functional tests, so the overall suite is the same in the last commit.
[Full results](https://docs.google.com/spreadsheets/d/1X8JI3k_mjKm9qUYNCV14TezTUBGOFTqHW-mucZOYRn4/edit?usp=sharing)
Summary:
Tested at 37ff331f60
There appears to be a 1-2% slow down on Ubuntu and MacOS but a 5% speed up on Windows
|| Ubuntu - master | Ubuntu - branch | MacOS - master | Macos - branch | Windows - master | Windows - branch |
|-|-|-|-|-|-|-|
|AVERAGE| 253| 260| 187| 190| 521| 493|
|% diff to master| | 2.766798419| | 1.604278075| | -5.37428023 |
ACKs for top commit:
vasild:
ACK 61020b36c5
theStack:
re-ACK 61020b36c5
Tree-SHA512: d39f782b9e6c7d67fdba5638fe9a1018b83062668c8b2ab0aa3fe363801f3c36a0ae97b8da0ea0395195e03a2c248a5d1511e5fa27c438c9bd7fca5c83d1ae4c
Remove the `Mutex` from the `threadpool` and `dbwrapper_concurrent_reads`
pool startup helpers. Fuzz targets are entered sequentially within a
process and parallel fuzzing uses separate processes/forks, which each
have their own copy of the global thread pool. Therefore, a mutex to
prevent two in-process callers from racing to start the pool isn't needed.
Limit how many read queries each worker executes. This significantly
speeds up the test, as each worker runs >90% fewer (2000 to 128)
expensive LevelDB operations (like `IteratorSeek`) but still
ends up hitting the intended target code.
Revert the `num_entries` max from 3000 back to 5000, as that didn't
have much effect on input speed and restores a bit of lost coverage.
Creates logic to deprecate logging categories but still
"support" them so the software doesn't quit with unknown category
on startup. Deprecated categories are always false and attempts
to switch them are logged as warnings.
The original function was already naturally split into two chunks:
First, we parse and validate the users' RPC configuration for IPs and
ports. Next we bind libevent's http server to the appropriate
endpoints.
This commit splits these chunks into two separate functions, leaving
the argument parsing in the common space of the module and moving the
libevent-specific binding into the http_libevent namespace.
A future commit will implement http_bitcoin::HTTPBindAddresses to
bind the validate list of endpoints by the new HTTP server.
The original function is passed to libevent as a callback when HTTP
requests are received and processed. It wrapped the libevent request
object in a http_libevent::HTTPRequest and then handed that off to
bitcoin for basic checks and finally dispatch to worker threads.
In this commit we split the function after the
http_libevent::HTTPRequest is created, and pass that object to a new
function that maintains the logic of checking and dispatching.
This will be the merge point for http_libevent and http_bitcoin,
where HTTPRequest objects from either namespace have the same
downstream lifecycle.
This is a refactor to prepare for matching the API of HTTPRequest
definitions in both namespaces http_bitcoin and http_libevent. In
particular, to provide a consistent return type for GetRequestMethod()
in both classes.
See https://www.rfc-editor.org/rfc/rfc7230#section-6.3.2
> A server MAY process a sequence of pipelined requests in
parallel if they all have safe methods (Section 4.2.1 of [RFC7231]),
but it MUST send the corresponding responses in the same order that
the requests were received.
We choose NOT to process requests in parallel. They are executed in
the order recevied as well as responded to in the order received.
This prevents race conditions where old state may get sent in response
to requests that are very quick to process but were requested later on
in the queue.
Sockets-touching bits copied and adapted from `CConnman::SocketSendData()`
Testing this requires adding a new feature to the SocketTestingSetup,
returning the DynSock I/O pipes from the mock socket so the received
data can be checked.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
`SocketHandlerConnected()` adapted from CConnman
Testing this requires adding a new feature to the SocketTestingSetup,
inserting a "request" payload into the mock client that connects
to us.
This commit also moves IOErrorIsPermanent() from sock.cpp to sock.h
so it can be called from the socket handler in httpserver.cpp
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
Socket handling methods are copied from CConnMan:
`CConnman::GenerateWaitSockets()`
`CConnman::SocketHandlerListening()`
`CConnman::ThreadSocketHandler()` and `CConnman::SocketHandler()` are combined into ThreadSocketHandler()`.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
Introduce a new low-level socket managing class `HTTPServer`.
BindAndStartListening() was copied from CConnMan's BindListenPort()
in net.cpp and modernized.
Unit-test it with a new class `SocketTestingSetup` which mocks
`CreateSock()` and will enable mock client I/O in future commits.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
This commit is a no-op to isolate HTTP methods and objects that
depend on libevent. Following commits will add replacement objects
and methods in a new namespace for testing and review before
switching over the server.
The utility can not be opinionated about CR or SP on either end of a
line it reads. That decision is up to the caller and in fact in the
case of HTTP should be allowed in some places and rejected in others.
Replace TrimStringView() with more surgical operation.
394e473d42 coins: compact chainstate in background (Lőrinc)
aa021b26f3 validation: randomly compact chainstate (Lőrinc)
b10889d107 coins: test chainstate flush baseline (Lőrinc)
Pull request description:
**Problem:** https://github.com/bitcoin-core/leveldb-subtree/pull/61 disabled read-triggered seek compactions to avoid large chainstate write amplification from random UTXO lookups.
That avoids repeated read-driven rewrites, but it also removes opportunistic cleanup that previously helped compact old chainstate data.
After IBD, normal chainstate churn can leave obsolete entries behind until ordinary LevelDB compaction naturally reaches the affected levels, keeping the chainstate database larger than necessary.
Also, chainstates created by pre-29 nodes can contain thousands of files from the old 2 MiB LevelDB table target.
After the mmap limit dropped back to 1000 and seek compaction was disabled, continuing from such a chainstate can leave many table reads on the non-mmap path until the database is compacted.
**Fix:** After each completed post-IBD full chainstate flush, give the chainstate a 1/320 chance to compact.
With roughly hourly full flushes, this averages about once every two weeks and makes a six-month stretch without compaction about a one-in-a-million event.
The randomized recurring trigger spreads compactions across nodes and keeps maintenance stateless, without storing last-compaction height or timestamp metadata in the chainstate database.
Compaction runs on a background thread (`utxocompact`) so validation only schedules the work.
Partially fixes#35298 and #35457
ACKs for top commit:
sipa:
ACK 394e473d42
optout21:
ACK 394e473d42
andrewtoth:
ACK 394e473d42
sedited:
ACK 394e473d42
Tree-SHA512: 536a298f86ca7d87a2dd5428354b05b693931c090c7a4afd4d09b95ce53e2bd8319e01c940489a1520d8cb1010a7ea9724d2c18d2e12887bd5ec70cb68c03517
855a3fee88 scripted-diff: Rename SteadyClockContext to FakeSteadyClock (Hao Xu)
Pull request description:
See: https://github.com/bitcoin/bitcoin/pull/35536#issuecomment-4715945494
`SteadyClockContext` and `FakeNodeClock` are both LimitOne RAII helpers that mock a clock in tests -- the steady clock and the node clock, respectively. Rename the former so the two follow a consistent FakeXClock naming scheme.
ACKs for top commit:
maflcko:
lgtm ACK 855a3fee88
w0xlt:
ACK 855a3fee88
sedited:
ACK 855a3fee88
Tree-SHA512: 72fd8dd3217d19f4b9f587e379a0aea2b7760ca6d61be6c363e7c06757cba40487df8b40cf3d62f9f9256fb4d0373610dd6f62c6e9ef3edde02265fc967a6bdf
fab2874269 lint: Require scripted-diff script to succeed (MarcoFalke)
Pull request description:
Currently, scripted diffs may silently pass with errors.
Fix this issue by calling the script from a Bash instance with error checking enabled: `bash -o errexit -o nounset -o pipefail -c "$SCRIPT"`.
Also, use Bash (not sh) when launching the script itself, because Bash is required anyway.
Can be tested by running something like this and observing the behavior before and after:
```
git commit --allow-empty -m $'scripted-diff: foo\n\n-BEGIN VERIFY SCRIPT-\n false;falseasfsafsaf;true;false|cat; echo "${NO_UN_SET}"|cat \n-END VERIFY SCRIPT-\n' && ./test/lint/commit-script-check.sh HEAD~..HEAD ; echo $?
```
Alternatively, an ancient brittle script can be tested:
```
./test/lint/commit-script-check.sh fb65dde147f63422c4148b089c2f5be0bf5ba80f~..fb65dde147f63422c4148b089c2f5be0bf5ba80f
ACKs for top commit:
hodlinator:
ACK fab2874269
sedited:
ACK fab2874269
Tree-SHA512: e3e8167e150be45a096d4883057640eb5624456f21b134cfa901fe490e5afb192855e55a752cb6121399314f65db155544a60645a101c39a084a62de4af23298
8f0354995b depends: latest config.guess & config.sub (fanquake)
Pull request description:
Pull the latest versions from upstream.
ACKs for top commit:
hebasto:
ACK 8f0354995b, verified using the upstream [repo](https://cgit.git.savannah.gnu.org/cgit/config.git).
Tree-SHA512: 6ea7ce70a7db614d32f0aea47006892766f6abb41e31fef25cadd4423a0f6e42f8500f03ea70a3990c2628ad1b534b7c52c19914289375f87289ca34349075b2
735b25519a support: clamp RLIMIT_MEMLOCK to size_t (Sjors Provoost)
8ab4b9fc85 init: clamp fd limits to int (Sjors Provoost)
4afbabdcef Fix startup failure with RLIM_INFINITY fd limits (Sjors Provoost)
Pull request description:
When setting the fd limit to unlimited, the node fails to start:
```sh
ulimit -n unlimited
build/bin/bitcoind
Error: Not enough file descriptors available. -1 available, 160 required.
```
This was caused by `RaiseFileDescriptorLimit()` (introduced in #2568) casting `limitFD.rlim_cur` to `int`, which for `RLIM_INFINITY` overflows to `-1`. Fix it by returning `std::numeric_limits<int>::max()` instead.
Some platforms implement `RLIM_INFINITY` as the maximum uint64, others as int64 (-1). So simply changing the return type to `uint64_t` wouldn't work.
Similarly, though unlikely to actually happen:
```sh
ulimit -n 214748364
build/bin/bitcoind
Error: Not enough file descriptors available. -2147483648 available, 160 required.
```
The second commit expands the fix by clamping all values above `std::numeric_limits<int>::max()` instead of letting them overflow.
This PR also expands `test/functional/feature_init.py` to cover these, using `resource.setrlimit`. The check is skipped on environments with a hard limit below infinity (or that don't have the Python [Resource module](https://docs.python.org/3/library/resource.html)).
macOS by default has a hard limit of infinity, but on e.g. Ubuntu the default hard limit is 524288.
The third commit applies a similar fix to `PosixLockedPageAllocator::GetLimit()` for 32-bit systems, but without a test.
ACKs for top commit:
winterrdog:
Re-ACK 735b25519a
achow101:
ACK 735b25519a
sedited:
Re-ACK 735b25519a
pinheadmz:
ACK 735b25519a
Tree-SHA512: 0ce0292ecd61456bdec6943b06cbb9ecfc5180ee6dce850f8496ef54af22c1fae6ea473085202f5ba6f72e4dc51a29247620c9a0eae31e96658adc77b293129f
2447385f47 rpc: remove unused RPCResult::Type::ELISION (satsfy (Renato Britto))
7a85118005 rpc: expand decodepsbt output script with explicit fields (satsfy (Renato Britto))
88e2a6ae89 rpc: expand getaddressinfo embedded with explicit fields (Renato Britto)
a9f9e7d17e rpc: extract fee estimate result helpers (Renato Britto)
8a615a8800 rpc: extract ListSinceBlockTxFields() helper (Renato Britto)
372ac283ac rpc: extend TxDoc() for getblock verbosity 2/3 (Renato Britto)
0380a1c46b rpc: extend TxDoc() for getrawtransaction verbosity 2 (Renato Britto)
44fc3a290d rpc: introduce HelpElision variant and ElideGroup helper (Renato Britto)
Pull request description:
Partially addresses #29912. Motivated by #34683, which exports OpenRPC from existing `RPCHelpMan` metadata. [Sample OpenRPC](https://gist.github.com/natobritto/8c4a1da04968d2325082ac4bca7d2408).
Some RPC help definitions rely on `RPCResult::Type::ELISION` entries whose structure is only described in prose. This keeps human-readable help concise, but leaves parts of the result layout implicit and prevents tools from deriving complete machine-readable schemas from `RPCHelpMan` metadata.
This PR replaces ELISION-based reuse with shared structured definitions, so result layouts are represented directly in metadata rather than only in text. At the same time, human-readable help remains compact via explicit help-rendering elision using `HelpElision`, so previously elided sections stay abbreviated without losing schema completeness.
Affected RPCs: `getrawtransaction`, `getblock`, `listsinceblock`, `estimaterawfee`, `getaddressinfo`.
RPC return values are unchanged. Human-readable help remains compact, while structured result metadata becomes explicit enough to derive complete machine-readable schemas.
A related `RPCResult::Type::ELISION` use in `importdescriptors` was split out into the follow-up PR #34867 because it changes the generated help output, per [this review comment](https://github.com/bitcoin/bitcoin/pull/34764#discussion_r2960236111).
Changes:
- Introduce `HelpElision` (`NONE`, `START`, `SKIP`) and `ElideGroup()`, replacing the tri-state `print_elision`
- Add an `RPCResult` copy-with-replacement-options constructor to support applying elision while keeping `m_opts` const
- Extend `TxDoc()` / `TxDocOptions` to support reusable transaction layouts with optional `prevout`, `fee`, `hex`, and elision behavior
- Replace ELISION-based reuse in `getrawtransaction` and `getblock` with explicit structured definitions
- Factor shared result layouts into `GetBlockFields()`, `ListSinceBlockTxFields()`, `FeeRateBucketDoc()`, `GetAddressInfoEmbeddedFields()` and `FeeEstimateHorizonDoc()`
- Expand `listsinceblock.removed`, `estimaterawfee` horizons/buckets and `getaddressinfo.embedded` into explicit metadata while preserving concise help output
ACKs for top commit:
achow101:
ACK 2447385f47
w0xlt:
reACK 2447385f47
janb84:
re ACK 2447385f47
willcl-ark:
ACK 2447385f47
Tree-SHA512: 8dc03c45c388ebdb4f8f1613af2576fc127a8d5425efe518cd0d0ed2439a38b2ed1236413471672c85f52ee22c4a17677c18fd4689bc6570496fc3af4cd4112f
2818a171c0 test: add abortscan unit test (Pol Espinasa)
bc30e95163 wallet: move fAbortRescan reset into WalletRescanReserver reserve() (Pol Espinasa)
Pull request description:
Follow-up of https://github.com/bitcoin/bitcoin/pull/35179
For extra context refer to the conversations https://github.com/bitcoin/bitcoin/pull/35179#discussion_r3378136210 and comments bellow it.
Long story short: currently `ScanForWalletTransactions()` resets the value of `fAbortRescan` before starting the rescan loop. This can cause a race condition where some function (e.g. `importdescriptors`) starts a rescan and at the same time the user aborts it manually. Could happen that the `abortrescan` call returns True (success) but the rescan continues running as the value is overwritten.
This PR fixes this by resetting the value of `fAbortRescan` at the very beginning, when the wallet rescan is reserved, removing the race condition. Also adds a test for it.
ACKs for top commit:
achow101:
ACK 2818a171c0
w0xlt:
ACK 2818a171c0
nebula-21:
ACK 2818a171c0
pablomartin4btc:
ACK 2818a17
pinheadmz:
ACK 2818a171c0
Tree-SHA512: 5b64b9a16a209dd145ccf7b2d7c3a7205038b3b214b02c3d9f1cbda75a6a1c9a41ad5c42cbeda43af6141793965613c47e44300d758149be9bfdf458e85a5520
0654511e1b util: Check write failures before renaming settings.json (Shrey)
Pull request description:
This PR Fixes#35373.
The error message was updated from: "This is probably caused by disk corruption or a crash" to: "This is probably caused by a full disk, disk corruption or a crash"
The following files were updated:
1. src/common/settings.cpp -- Updated the main string formatting for the parse failure.
2. src/test/settings_tests.cpp -- Updated the exact string expectation in the C++ unit test.
3. test/functional/feature_settings.py -- Updated the expected error message in the Python test suite.
Configured the Bitcoin Core project natively on Windows using Visual Studio (cmake --preset vs2026-static -DBUILD_GUI=OFF) and it compiled with 100% of the C++ tests passed successfully (347 out of 347).
ACKs for top commit:
maflcko:
review ACK 0654511e1b💧
winterrdog:
ACK 0654511e1b
sedited:
ACK 0654511e1b
ryanofsky:
Code review ACK 0654511e1b with commit message and error message improved since last review
Tree-SHA512: d7b49860f2081a5a9d1d44917b0cf372a77f10cf21773c9b3291871c788a122ec5dde063fdf9e1052cf45b8d2667e15585b952e9f037e84f98fb41546bdd1fa9
de92208c2b migrate: Handle HD chains that have identical seeds but different IDs (Ava Chow)
Pull request description:
The seed ID is calculated from a pubkey produced by treating the seed as a private key. This calculation includes a pubkey compression parameter, even thought that compression is completely irrelevant for the usage of the seed as a BIP 32 seed. Thus migration should detect if a seed has been used multiple times by checking if the computed master key was already processed.
The spkm_migration fuzzer needs to have it's added descriptors accounting updated for this fix.
It should not be possible for users to actually run into this problem as all HD chains use seeds with the pubkey compression option set.
Fixes#35434
ACKs for top commit:
kevkevinpal:
crACK [de92208](de92208c2b)
marcofleon:
crACK de92208c2b
rkrux:
code review ACK de92208
Tree-SHA512: c420a24722fd6a94bf6656f195bad3432ba54c38b3c49a02750577281d0864988fd6d44cd9594b57cfaf33061a1e250e21378e3637b4e9a45f2d7aad6045884d
SteadyClockContext and FakeNodeClock are both LimitOne RAII helpers that mock a
clock in tests -- the steady clock and the node clock, respectively. Rename the
former so the two follow a consistent FakeXClock naming scheme.
-BEGIN VERIFY SCRIPT-
sed -i 's/SteadyClockContext/FakeSteadyClock/g' $(git grep -l SteadyClockContext)
-END VERIFY SCRIPT-
f963f2b675 argsman: allow duplicate registration between HIDDEN and other categories (Pablo Martin)
Pull request description:
Follow-up to #35470.
The assertion added in #35470 to prevent duplicate option registration across categories was too strict, it also fired when an option was registered in `OptionsCategory::HIDDEN` and then again in a real category (or vice versa).
That is intentional behavior introduced in #13441: options unavailable in a given binary (e.g. `GUI` args in `bitcoind`) are pre-registered as hidden so shared `bitcoin.conf` files don't fail. In `bitcoin-qt`, `SetupServerArgs` registers `GUI` args as hidden, then `SetupUIArgs` registers them properly under `OptionsCategory::GUI`, triggering the assertion and crashing on startup (e.g. `bitcoin-qt` crashes now that #35470 has been merged into `master`).
The fix relaxes the assertion to exclude `HIDDEN` from the cross-category duplicate check, preserving the original intent of #13441 while still catching unintentional duplicates between real categories.
<details>
<summary>Alternative approach considered</summary>
An alternative fix would have been to make `AddHiddenArgs` skip args already registered in any category:
```cpp
void ArgsManager::AddHiddenArgs(const std::vector<std::string>& names)
{
for (const std::string& name : names) {
size_t eq_index = name.find('=');
std::string arg_name = name.substr(0, eq_index == std::string::npos ? name.size() : eq_index);
LOCK(cs_args);
bool already_registered = std::ranges::any_of(m_available_args, [&](const auto& arg_map) {
return arg_map.second.contains(arg_name);
});
if (!already_registered) {
AddArg(name, "", ArgsManager::ALLOW_ANY, OptionsCategory::HIDDEN);
}
}
}
```
This would fix the crash but obscures the intent — silently skipping registrations in `AddHiddenArgs` makes it harder to reason about what's registered. The chosen approach of relaxing the assertion specifically for `HIDDEN` is more explicit about why the duplicate is allowed.
</details>
ACKs for top commit:
ryanofsky:
Code review ACK f963f2b675 and confirmed this fixes the problem.
sedited:
ACK f963f2b675
Tree-SHA512: a7606d66c99f3d9ff485abcd7c6e3266ce42a3ec716863a6c756a752c6c92a3c0e5a969d2c3646ade9f13032a42fc364b0bbfa9692e723ae61327284ed1caa2e
The assertion added in #35470 to prevent duplicate option registration
across categories was too strict — it also fired when an option was
registered in OptionsCategory::HIDDEN and then again in a real category
(or vice versa).
This is intentional behavior introduced in #13441: options unavailable
in a given binary (e.g. GUI args in bitcoind) are pre-registered as
hidden so shared bitcoin.conf files don't fail. In bitcoin-qt,
SetupServerArgs registers GUI args as hidden, then SetupUIArgs registers
them properly under OptionsCategory::GUI, triggering the assertion and
crashing on startup.
The fix relaxes the assertion to exclude HIDDEN from the cross-category
duplicate check, preserving the original intent of #13441 while still
catching unintentional duplicates between real categories.
