9954aa7728 http: don't parse any new requests from a client if m_req_busy = true (Matthew Zipkin)
c7db3ae1f9 test: cover HTTPRequest state machine (Matthew Zipkin)
90676e24ad Add state to HTTPRequest to avoid duplicate work over I/O cycles (Matthew Zipkin)
507e528e84 http: reuse HTTPHeaders to parse chunked trailer (Matthew Zipkin)
902d8908c9 http: only read one HTTPRequest at a time per client (Matthew Zipkin)
Pull request description:
This PR reduces the memory consumption of the HTTP Server when reading data from connected clients, and improves performance especially when requests are large (i.e. requiring multiple TCP packets).
In https://github.com/bitcoin/bitcoin/pull/35182 the server copies as much data as it can from the socket into application memory, and then tries to parse as many complete HTTP requests as possible from that data. If a request is discovered to be incomplete, the in-progress request is abandoned. The server tries again on the next I/O cycle to read the same data from the buffer, duplicating work as many times as it takes before the client finishes sending the request (or times out).
This PR implements two improvements to this:
1. Only parse one request at a time from the receive buffer. The server processes requests from each client in series anyway.
2. Add state to `HTTPRequest` so it can be filled with data from the receive buffer over multiple I/O loop iterations without losing progress.
If a client sends large or multiple requests, that data will sit in the kernel's socket buffer instead of the application memory. Eventually the socket buffer will fill up and TCP backpressure will kick in, dropping the TCP window to 0 and blocking the client from sending any more.
A state machine for `HTTPRemoteClient` was [discussed previously](https://github.com/bitcoin/bitcoin/pull/35182#pullrequestreview-4322490068) to control resource consumption. Another nice benefit of this model (for a follow-up PR) will be to insert the RPC authentication check after reading 8kB-limited headers but before the 32MB-limited request body.
ACKs for top commit:
winterrdog:
re-ACK 9954aa7728
janb84:
re ACK 9954aa7728
frankomosh:
ACK 9954aa7728.
fjahr:
ACK 9954aa7728
Tree-SHA512: b7c913114283fbf1f360b40f6c65a01390a26731bf3b166f460ec260f9206f25d738b3a06887bfa839911c1c6aaf634448181da47a752a9a881aebd907e44868
fabe100c2b test: Use throwing config parser getters without fallback (MarcoFalke)
fa8acd57cd test: Write true/false values in config.ini (MarcoFalke)
Pull request description:
Currently, the called `getboolean` member function is *not* the throwing https://docs.python.org/3/library/configparser.html#configparser.ConfigParser.getboolean, but a non-throwing member function on a dict-like proxy object.
This is confusing and brittle, because tests shouldn't silently skip when a config key is missing. Instead, tests should loudly fail, e.g. when the config key is renamed in one place, but not the other.
ACKs for top commit:
jeanpablojp:
tACK fabe100c2b
willcl-ark:
ACK fabe100c2b
Tree-SHA512: a970d74ad285372b8adcce8e2a52b01f5a3b563899dfc5262e6ffbf3d8aba43e72f7b03111e8d5188924c7d3d789992407cddb5d182d9be5e42f07896d8ad4a3
b3d77ea027 test: Speedup fee estimation functional test with batching (sedited)
Pull request description:
The fee estimation functional test is currently the slowest one by a good margin. It is a bit annoying, because it also increases the total runtime of the functional tests.
It seems like most of the slowness comes from the transactions propagating between the nodes. This patch helps them do that by submitting them directly to all the nodes. Also take this opportunity to batch the transaction submissions.
On my machine this speeds up the fee estimation functional test from around 71 seconds to 25 seconds.
ACKs for top commit:
151henry151:
tACK b3d77ea027
maflcko:
review ACK b3d77ea027🐇
ismaelsadeeq:
ACK b3d77ea027
Tree-SHA512: f76415dca7997577ca39ac6b95dfdf32b930dd64b4311e3da34e34adb16107ff4ea2d9fa679f3ca50540e80c38af7f9390b44f21ad1b8107fbbc3586edb2ef19
25bed560be test: add forward-compat functional test for txindex (sedited)
703304ed8c doc: add release notes for txindex disk usage and downgrading (Andrew Toth)
8e5320a2d2 tests: cover txindex hash prefix collisions and legacy fallback (Andrew Toth)
b75efa19ba txindex: skip bloom filters and legacy lookups for new databases (Andrew Toth)
004d7c098c txindex: hash key prefixes and pack block positions (Andrew Toth)
5a255970fd refactor: move txindex db constants and legacy key to txindex_key.h (Andrew Toth)
327660134c txindex: pass the full block to DB::WriteTxs (Andrew Toth)
42771e7998 txindex: use a new block locator for downgrade safety (Andrew Toth)
4b08baed72 txindex: return optional tx and block hash from FindTx (Andrew Toth)
Pull request description:
The current txindex uses the full 32-byte txid as keys, which takes up about 66 GB of disk space today on mainnet. Using a 5-byte key prefix instead drops the disk usage to 26 GB - cutting the size to less than half.
Using the full 32-bytes is unnecessary since a 5-byte salted siphash will produce collisions in about 1 in 1.1 trillion. Some collisions will occur, but the penalty is just an extra disk read, deserialization and hash.
The tx position can be appended to the key instead of used as a value, and a LevelDB iterator can seek to the prefix and then scan for the correct tx. This is an almost identical approach to `txospenderindex`.
Also instead of storing the file position of the block, we can store only the sequence of the connected block and offset of the transaction in the block. This can be packed into a 6-byte key suffix using 3-byte representations of the sequence and offset in the block. The block file can be recovered by the CBlockIndex that is already in memory. The sequence is mapped to the block hash in the db, so we can lookup the block hash to find the CBlockIndex during reads.
If a tx is not found with this method, we fallback to looking up the legacy entry. With this method a user with an existing db can opt to erase the `indexes/txindex` folder and reindex, or keep the current index and new entries will be appended with the smaller footprint.
The time to index was faster on my machine with this method, 1h19m vs current 1h50m.
Lookups are roughly the same, around 0.2ms per lookup with `getrawtransaction`.
When testing on mainnet, I got 894,549 2-way collisions, 395 3-way collision, and 1 4-way collision that worst case could cause an extra 3 false positives when reading.
ACKs for top commit:
l0rinc:
diff reACK 25bed560be
sedited:
ACK 25bed560be
ajtowns:
ACK 25bed560be
Tree-SHA512: a25c79ca7e722e2f372b65f5fc11c8b194ad49f2240b4881c7e606306aabbd3604aede3f1c33606b467486affac3a3f503638f513c896935cebbc02709cb60d8
c3945bfd2b doc: use derivehdkey in multisig tutorial (Sjors Provoost)
3662e33669 test: use derivehdkey in M-of-N multisig demo (Sjors Provoost)
d9570f0838 rpc: add derivehdkey (Sjors Provoost)
62da9f9614 wallet: add GetExtKey helper (Sjors Provoost)
aaf1548475 wallet: generalize GetActiveHDPubKeys helper (Sjors Provoost)
3821452c4a refactor: add hardened derivation helper (Sjors Provoost)
0ab61caafd rpc: ParsePathBIP32 helper (Sjors Provoost)
e36c4b76e1 util: reject out-of-range BIP32 keypath indices (Sjors Provoost)
ba78c31a00 fuzz: check ParseHDKeypath/WriteHDKeypath round-trip (Sjors Provoost)
8cce969085 Have ParseHDKeypath handle h derivation marker (Sjors Provoost)
fc53077762 test: move parse_hd_keypath test to bip32_tests (Sjors Provoost)
dab525eb77 key: add DeriveExtKey() helper (Sjors Provoost)
Pull request description:
Adds a `derivehdkey` RPC that returns an xpub, or optionally the xprv, at an arbitrary BIP32 path (with at least one hardened step), derived from a wallet HD key.
The main use case is coordinating a multisig setup, where each participant shares an xpub derived at a hardened path (e.g. `m/87h/0h/0h`) distinct from their default single-signature descriptors. See the (updated) `doc/multisig-tutorial.md` and (updated) functional test to see how that workflow improves.
The first commits are some helpful helpers:
- _key: add DeriveExtKey() helper_ - performs the actual derivation
- _test: move parse_hd_keypath test to bip32_tests_ - from `psbt_wallet_tests`
- _Have ParseHDKeypath handle h derivation marker_
- _util: reject out-of-range BIP32 keypath indices_ - `ParseHDKeypath` would previously map overflowing values without `h` to hardened.
- _fuzz: check ParseHDKeypath/WriteHDKeypath round-trip_
- _rpc: ParsePathBIP32 helper_
- _refactor: add hardened derivation helper_ - `HasHardenedDerivation()`, to enforce the "at least one hardened step" rule
- _wallet: generalize GetActiveHDPubKeys helper_ - extracts code from `gethdkeys` which `derivehdkey` needs
- _wallet: add GetExtKey helper_ - reconstruct an xprv from a wallet xpub (analog of `GetKey()`); behavior-preserving prep, also simplifies `gethdkeys`.
Meat and potatoes:
- _rpc: add derivehdkey_ - the RPC itself, plus the `UnusedKey` filter on `GetHDPubKeys` that drives key selection.
- _test: use derivehdkey in M-of-N multisig demo_ - rewrites the functional multisig test to use the RPC and `<0;1>` syntax.
- _doc: use derivehdkey in multisig tutorial_ - same for the prose tutorial.
ACKs for top commit:
pseudoramdom:
code review ACK c3945bfd2b
achow101:
ACK c3945bfd2b
w0xlt:
That being the case, ACK c3945bfd2b
Tree-SHA512: 661f17c9bfe26017eb14c27ba7af37093387100d3baa25f5d29bba9c1aedc40d19afe1bdfc126a18d018857bb02f1fc84386f10b8f4f4b8e9d6f4b0691d9e302
f280f5eb47 wallet: rpc: deprecate removeprunedfunds (David Gumberg)
e5b7785447 test: wallet: resend: avoid internal behavior via removeprunedfunds (David Gumberg)
Pull request description:
Originally added in https://github.com/bitcoin/bitcoin/pull/7558 as a companion to `importprunedfunds`, this RPC has no known helpful use while being both dangerous and a maintenance burden.
Despite what the name says, it allows the deletion of arbitrary transactions, and `importprunedfunds` does not allow the importing of transactions not belonging to the user, and `listtransactions` does not list transactions not belonging to the wallet, so this RPC can only be used to delete transactions actually belonging to the wallet, and in the unlikely event that transactions not belonging to the wallet are present, they cause no harm except for occupying a few bytes on the users disk.
ACKs for top commit:
achow101:
ACK f280f5eb47
polespinasa:
ACK f280f5eb47
pablomartin4btc:
reACK f280f5eb47
Tree-SHA512: ed9c30c50be514d637999b4c8f3fa9b9b1446a5553e3974703638b45d8f55f1291f5cfb82dd2ead6d0743e424e9e3b1edbd56fc04dae3dcdee4d175e2a1ce061
4e8c4bc794 test: classify SOCKS5 peers via getpeerinfo addrbind (Henry Romp)
Pull request description:
p2p_private_broadcast.py classifies each SOCKS5 connection by scanning the node's debug log for `trying v. connection (...) to <addr>:<port>`, then attaches a fake peer for that type. The helper returned the first match in the whole log, so when a feeler selected a clearnet address that private broadcast had used earlier in the run (in the CI failure, `[50::1]:8333`, about 10 seconds apart), the feeler was labelled private-broadcast, was given the `NoRelayP2PInterface`, and disconnected as a feeler rather than with the expected "connected in vain" message.
Instead of relying on the debug log, identify the connection via the SOCKS5 proxy client socket's source address, which equals the node's `addrbind` for that peer, and read `connection_type` from getpeerinfo. The proxy replies to the SOCKS5 request before invoking `destinations_factory`, so the node has already registered the peer by the time classification runs. This also stops treating debug.log contents as a stable test interface. Dropping the log scrape removes a full re-read of debug.log per SOCKS5 connection; `p2p_private_broadcast.py` goes from ~23s to ~14s locally.
Fixes#35843
Tested with:
`build/test/functional/test_runner.py p2p_private_broadcast.py p2p_private_broadcast_retry_v1.py --timeout-factor=2`, and against the forced-feeler repro from the issue, which no longer mislabels the feeler.
ACKs for top commit:
jeanpablojp:
tACK 4e8c4bc794
andrewtoth:
ACK 4e8c4bc794
mzumsande:
Code Review ACK 4e8c4bc794
Tree-SHA512: ce2db418787d7ecf518bd49b37d7d664748fee5991a2924522dfaf42b27d90ca001caa0611011310636b453d3aada1061d086f20bb866eb66605645935f55c74
4ca182ca40 doc: clarify alternate_wtxids is empty when only one witness variant (pablomartin4btc)
fa48b5d28e test: assert listsinceblock "removed" reports current canonical wtxid (pablomartin4btc)
9b96ee1288 wallet, test: add unit test for variant txid validation in CWalletTx deserializer (pablomartin4btc)
9de6543cb5 wallet: post-#35501 cleanup in CWalletTx (pablomartin4btc)
Pull request description:
Follow-up cleanups and clarifications after #35501 was merged.
Commit breakdown:
1. _post-[#35501](https://github.com/bitcoin/bitcoin/pull/35501) cleanup in_ `CWalletTx`
- Rename `arg_state` → `new_state` in `Update()` for consistency
- Simplify `RecomputeCanonical()` using `std::ranges::min_element` with a projection lambda (14 lines → 3 lines)
- Add variant txid validation in the `CWalletTx` deserialise constructor: throws `std::runtime_error` if any variant's txid doesn't match the canonical txid deserialized from the stream
- Move `Init()` to `private` and extend it to clear `m_txs` and reset `m_canonical_wtxid`, so a full re-deserialise via `Unserialize()` starts from a clean state
All [suggested](https://github.com/bitcoin/bitcoin/pull/35501#pullrequestreview-4854519083) by ajtowns.
2. _add unit test for variant txid validation in_ `CWalletTx` _deserializer_
3. _assert_ `listsinceblock` "removed" _reports current canonical wtxid_
Documents that removed entries reflect the wallet's current `CWalletTx` state, not a snapshot of the detached block. A future followup could improve this (requires per-block tracking of which witness variant was included).
[Suggested](https://github.com/bitcoin/bitcoin/pull/35501#discussion_r3632044472) by w0xlt.
4. _clarify_ `alternate_wtxids` _is empty when only one witness variant_
[Suggested](https://github.com/bitcoin/bitcoin/pull/35501#discussion_r3632113003) by polespinasa.
ACKs for top commit:
jeanpablojp:
re-ACK 4ca182ca40
achow101:
ACK 4ca182ca40
polespinasa:
ACK 4ca182ca40
Tree-SHA512: 64eadeb11372d904c79edbfd264c4d8dc1b4fe4ce5e3acc301bfeba9e556efb5dce2c684f0e58c7b74e2c687cc7dd77389970662b3fd629ed034a97bcfdfb71c
0390338692 test: check MuSig import private key warnings (woltx)
5e62fbf09c wallet: check descriptor private key completeness on import (woltx)
cd8d01bf47 descriptors: require complete MuSig private keys (woltx)
Pull request description:
`importdescriptors` currently checks whether all private keys are present by expanding the descriptor and verifying that every expanded origin pubkey has a private key.
This is wrong for MuSig descriptors because expansion includes the synthetic aggregate pubkey. There is no individual private key for that aggregate pubkey, so importing a fully private MuSig descriptor such as `rawtr(musig(A_priv,B_priv))` incorrectly returns:
```
Not all private keys provided. Some wallet functionality may return unexpected errors
```
This PR fixes the issue by making descriptor private-key completeness account for MuSig participant keys, and by having `importdescriptors` use `Descriptor::HavePrivateKeys()` instead of duplicating its own manual completeness check.
The functional test covers both cases:
- `rawtr(musig(A_priv,B_priv))` imports without warnings.
- `rawtr(musig(A_priv,B_pub))` still warns that not all private keys were provided.
ACKs for top commit:
achow101:
ACK 0390338692
theStack:
Code-review ACK 0390338692
Tree-SHA512: a55fb084c63f725a0991556acdfb822f3a5a669f745a00b9f0bf0996b639986cdf5e2be2e8d3d0a2ee3fe5744355f20b40df576601792ef3db698e606629ad52
When a block is detached, listsinceblock "removed" entries reflect the
wallet's current CWalletTx rather than a snapshot of the variant that
was actually in the detached block. Add assertions to make this
behaviour explicit. A future followup could improve listsinceblock to
track and report the specific witness variant that was in the
disconnected block (requires per-block tracking of which witness variant was included).
Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>
ea59f17220 test: cover v0.14.3 wallet migration (w0xlt)
18b8afd093 test: support v0.14.x in dumb_sync_blocks (w0xlt)
Pull request description:
This PR adds test coverage for migrating legacy Bitcoin Core wallets from v0.14.3 (released in 2017) to the descriptor wallet format. The test validates that users can safely upgrade their wallets while preserving all funds, transaction history, and addresses.
This test was originally developed on top of #32977, as it was requested in reviews.
However, since it also increases test coverage, it can be merged independently.
The test covers two wallet migration scenarios:
* Non-HD Wallet Migration - Tests migration of non-HD wallets (created with `-usehd=0`)
* Single Chain HD Wallet Migration - Tests migration of HD wallets from v0.14.3 (`VERSION_HD_BASE`)
The node v0.14.3 cannot be synced using the normal test framework helpers because it does not have the `syncwithvalidationinterfacequeue` RPC, so the test uses `dumb_sync_blocks` to submit blocks from the ancient node to the modern node before migrating the wallet.
Each scenario uses its own dedicated old/new node pair, which keeps the setup isolated and makes this testing best managed in a separate file rather than in the existing migration test files.
On the Windows cross-built CI job, this test is excluded from the main functional test runner and re-run sequentially in an ASCII-only tmpdir, because the v0.14.3 binary cannot handle non-ASCII characters in the temporary directory path.
ACKs for top commit:
furszy:
utACK ea59f17220
pablomartin4btc:
reACK ea59f17220
Tree-SHA512: 35ef7173e10fe52f20db0d6d1f144c5a1343ff406d294ab7d0e346d79546797f3f145f2893d99bb7b57d71aa14580690ab80eae148ec94b91498eb0938b2d15e
089c883c55 test: Add coverage for listreceivedby* excluding "send" addresses (pablomartin4btc)
873c054805 wallet: Exclude non-owned addresses from listreceivedby* (pablomartin4btc)
Pull request description:
Fixes#16159.
`listreceivedbyaddress`/`listreceivedbylabel` with `include_empty=true` walk the entire address book and return every entry that has no matching `mapTally` record — including addresses with a "send" purpose (foreign addresses that got a label via `setlabel`, the GUI, or `addmultisigaddress`) that this wallet never received funds to and doesn't own.
This excludes those via `IsMine()` rather than the address book's `purpose` field, since `purpose` is set inconsistently across several code paths and `IsMine()` is the same check `mapTally` itself is already built from.
Picks up prior work by kouloumos in #25973 and BrandonOdiwuor in #30972, both closed for
inactivity:
- [#25973](https://github.com/bitcoin/bitcoin/pull/25973) filtered on `purpose == "send"` directly. ryanofsky pointed out purpose "is set pretty haphazardly in code" and [suggested](https://github.com/bitcoin/bitcoin/pull/25973#discussion_r1269477246) `IsMine()` instead.
- [#30972](https://github.com/bitcoin/bitcoin/pull/30972) implemented that, then furszy pointed out `IsMine()` only needs to run for addresses missing from `mapTally`, not every one. rkrux further suggested dropping the redundant re-lock in favor of `EXCLUSIVE_LOCKS_REQUIRED` directly on the lambda — matching the existing pattern in `wallet/interfaces.cpp` — and simplifying the branching.
This PR carries that final approach forward on current master. The regression test is a small, standalone addition rather than reviving the test-file "split into subtests" refactor from the earlier PRs, which achow101 [flagged](https://github.com/bitcoin/bitcoin/pull/30972#issuecomment-3688186614) on #30972 as unrelated stylistic churn.
ACKs for top commit:
polespinasa:
lgtm re-ACK 089c883c55
jeanpablojp:
ACK 089c883c55
achow101:
ACK 089c883c55
Tree-SHA512: d45488c93b9294258faaab5d1891ca5e8c4b8d0d4feb298403c7c3f20d6aa08989d548cddd25ccd47a1ed969e4a309ee68ad1541c6121fed39ed534c78c256e7
fad9ab714b test: Append print_suppressions=0 to LSAN_OPTIONS, and suppress bitcoin-qt (MarcoFalke)
Pull request description:
(see commit msg for rationale and background).
To test, one should be able to use the cmake options such as `-DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' --preset=dev-mode -DBUILD_GUI=ON -DSANITIZERS=address` on e.g. Fedora. Then see that the current suppressions file is insufficient, and also confirm that `print_suppressions=0` is required.
ACKs for top commit:
fanquake:
ACK fad9ab714b
Tree-SHA512: 1830b4aeb072fa18b76522a124a268073675da14255e469a6d86ee5de52cd08d5613d0c3bd8a66465b0c4636345c9e967923cd1fb516906a58b614fe0e700033
75f5851927 doc: add release note for REST cache-control headers (w0xlt)
bbe21ac29f doc: document REST cache-control defaults (w0xlt)
862a179556 http: add no-store to dispatcher-generated error responses (w0xlt)
acf45c44c0 rest: add Cache-Control headers to REST responses (w0xlt)
Pull request description:
This PR adds explicit Cache-Control headers to REST responses.
The policy is:
- Immutable data gets: `Cache-Control: public, immutable, max-age=86400`
- Mutable, node-local, and error responses get: `Cache-Control: no-store`
Important details:
- `/block` and `/block/notxdetails` bin/hex, `/blockpart`, `/blockfilter`, `/spenttxouts`, and `/deploymentinfo/<blockhash>.json` are treated as immutable.
- `/block` and `/block/notxdetails` JSON, all `/tx` formats, `/headers`, `/blockfilterheaders`, `/blockhashbyheight`, `/chaininfo`, `/mempool`, `/getutxos`, and `/deploymentinfo.json` are no-store.
- REST errors and HTTP dispatcher-generated errors are no-store.
- Unmatched `/rest` 404s also return no-store, including paths like `/rest/tx`, `/rest/does-not-exist`, and `/rest?x=1`.
Tests were added in `interface_rest.py` to cover successful responses, behavior across a newly mined block, REST errors, and unmatched REST 404s.
Docs were added to `REST-interface.md`, including guidance for overriding the defaults in a reverse proxy or CDN.
Closes#33809
ACKs for top commit:
stickies-v:
re-ACK 75f5851927
pinheadmz:
ACK 75f5851927
sedited:
ACK 75f5851927
Tree-SHA512: 292ccd06ddfc9272c17fa720ce1ea8bb05462337af6460488f70003d3daf31fcf262e68c264522a911bba65ae2b25fc88a1fd422e5664583daf64070231cb062
The print_suppressions=0 is required to avoid a CI failure when the
suppressions were used. E.g:
```
$ LSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/lsan:print_suppressions=1" ./bld-cmake/test/functional/interface_gui.py
2026-08-08T10:53:45.864160Z TestFramework (INFO): PRNG seed is: 8358096631255493262
2026-08-08T10:53:45.914748Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_5zx5343v
2026-08-08T10:53:47.029997Z TestFramework (INFO): Test that bitcoin-gui starts up and can be stopped via RPC
2026-08-08T10:53:47.431761Z TestFramework (ERROR): Unexpected exception:
File "./test/functional/test_framework/test_node.py", line 534, in is_node_stopped
raise AssertionError("Unexpected stderr {} != {}".format(stderr, expected_stderr))
AssertionError: Unexpected stderr -----------------------------------------------------
Suppressions used:
count bytes template
2 181 bitcoin-qt
----------------------------------------------------- !=
```
The general suppression of the qt executables is required to avoid CI
failures for i386 builds. E.g:
```
test 2026-08-05T08:54:08.370427Z TestFramework (ERROR): Unexpected exception:
Traceback (most recent call last):
File "/ci_container_base/ci/scratch_ ₿🧪_/build-i686-pc-linux-gnu/test/functional/interface_gui.py", line 34, in run_test
self.stop_node(0)
~~~~~~~~~~~~~~^^^
File "/ci_container_base/test/functional/test_framework/test_node.py", line 525, in is_node_stopped
assert return_code in expected_ret_code, self._node_msg(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: [node 0] Node returned unexpected exit code (1) vs ((0,)) when stopping
node0 stderr =================================================================
==73449==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 386 byte(s) in 17 object(s) allocated from:
#0 0x5df28a2d in malloc (/ci_container_base/ci/scratch_ ₿🧪_/build-i686-pc-linux-gnu/bin/bitcoin-qt+0x1d0da2d) (BuildId: 496a5df531df278fe395724ae917f266331b3f81)
#1 0xee03c1d1 (<unknown module>)
Indirect leak of 12 byte(s) in 1 object(s) allocated from:
#0 0x5df28a2d in malloc (/ci_container_base/ci/scratch_ ₿🧪_/build-i686-pc-linux-gnu/bin/bitcoin-qt+0x1d0da2d) (BuildId: 496a5df531df278fe395724ae917f266331b3f81)
#1 0xee03c1d1 (<unknown module>)
SUMMARY: AddressSanitizer: 398 byte(s) leaked in 18 allocation(s).
```
Regression test for #16159: an address labeled via setlabel by a
wallet that doesn't own it is assigned a "send" purpose and must not
appear in listreceivedbyaddress/listreceivedbylabel results, even
with include_empty=true.
Co-authored-by: Andreas Kouloumos <kouloumosa@gmail.com>
Use derivehdkey instead of extracting each participant xpub (and
derivation info) from the listdescriptors output.
Additionally use the new <0;1> descriptor syntax.
Finally this commits adds a few debug log lines, and expand the
explanation for why we use m/44h/1h/0h.
Add an UnusedKey filter to GetHDPubKeys() so the new RPC can prefer
unused(KEY) descriptors before falling back to active descriptors.
Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>
264555af3c rpc: avoid descriptor range counter overflow (Lőrinc)
143a13fb2b test: characterize descriptor range endpoint (Lőrinc)
Pull request description:
**Problem:** The authenticated `scantxoutset`, `scanblocks`, `getdescriptoractivity`, `utxoupdatepsbt`, and `descriptorprocesspsbt` RPCs share a descriptor expansion helper that iterates inclusive `int64_t` ranges with an `int` counter.
A ranged descriptor with an explicit `[begin, end]` range ending at `2^31 - 1` expands that valid position, then overflows when advancing the counter to exit the loop.
Trap-enabled builds terminate, while other builds invoke undefined behavior.
**Fix:** Use `int64_t` for loop control so the one-past-the-end value is representable and every position passed to `Descriptor::Expand()` remains within its existing `int` range.
Related: [#26275](https://github.com/bitcoin/bitcoin/pull/26275) fixed the same endpoint overflow in `deriveaddresses`.
ACKs for top commit:
achow101:
ACK 264555af3c
polespinasa:
ACK 264555af3c
sedited:
ACK 264555af3c
Tree-SHA512: 4326182b5897b6f6672e5f7c7296eafdbb6e3b5ed901d61e8fa2cff9b19d372bb8adb88902368dc520ed400e12dd5264ea68677d9ce0feec76fa2ef55fa0d2f4
1278a5970d net_processing: process unique tx INVs only (Gregory Sanders)
Pull request description:
There is no reason we should process conflicting
advertisements for transactions, as they cannot be both accepted into our mempool.
Avoid processing these and doing spurious work.
Should be no change in observable behavior.
ACKs for top commit:
ajtowns:
ACK 1278a5970d
fjahr:
ACK 1278a5970d
l0rinc:
ACK 1278a5970d
Tree-SHA512: c62ceed2cc634c8c99509a8495e5f9bb6d4d8d050942f709a6539ae4dfe1ec628ce7ab0ec1392656809e0d827d1fecf2e8fb9bb4600d13a5bf9a34c9e9e3ad6e
There is no reason we should process conflicting
advertisements for transactions, as they cannot be both
accepted into our mempool.
Avoid processing these and doing spurious work.
REST error responses normally go through RESTERR(), which sets
Cache-Control: no-store. Some errors are returned directly by the
generic HTTP dispatcher and bypass RESTERR(), including unmatched
paths, queue exhaustion, handler exceptions, and shutdown rejection.
Use a shared reply helper to set no-store on dispatcher-generated
error responses. This avoids caching transient errors and keeps the
behavior consistent across REST and RPC requests.
Test the work-queue rejection path by checking that its HTTP 503
response includes the header.
Test migratewallet on v0.14.3 non-HD and single-chain HD wallets in both
unencrypted and encrypted configurations.
Verify balances, transaction history, address ownership, descriptor
structure, encryption enforcement, backup creation, and the absence of
rescans or unexpected auxiliary wallets.
Run the test with an ASCII-only temporary directory in the Windows
cross-build job because the v0.14.3 binary cannot handle the Unicode
runner path.
The getblock RPC used a boolean verbose argument before v0.15. Use the
legacy named argument so the helper can synchronize blocks from v0.14.x
nodes while remaining compatible with current nodes.
3ac8b806a6 test: test the result order of a multiple import request is correct (Pol Espinasa)
e4732bf018 test: test invalid or missing timestamp throws importdescriptors (Pol Espinasa)
07fb58b9ef test: Test a locked wallet rejects an empty importdescriptors request (Pol Espinasa)
Pull request description:
In addition to #35179 (already merged) this adds more missing test coverage that was detected while rebasing #34861.
The three tests added checks:
- Locked wallet throws because of being locked if giving an empty importdescriptors request.
- Invalid or missing timestamp throws as a top level RPC error and not a per-item error.
- The order of the requests and the response is the same, even if failing or succeeding.
ACKs for top commit:
nebula-21:
ACK 3ac8b806a6
Bicaru20:
re-ACK 3ac8b806a6
brunoerg:
reACK 3ac8b806a6
Tree-SHA512: b6ba9e16bbdbefcab2529f49f9aab0ae8885bd2d381c6eec36ae442dea1aa2361e6fb339ab5bc2c51c3bef6216d8d939db53e57ec577f05fe54c07fc46f8f255
aeca061086 rpc: reject null for optional parameters (Ruslan Kasheparov)
Pull request description:
Treat explicitly passed `null` as missing for optional RPC parameters that are required in certain contexts.
ACKs for top commit:
achow101:
ACK aeca061086
maflcko:
review ACK aeca061086🥚
sedited:
ACK aeca061086
Tree-SHA512: 60f146085fd20e532ba3cbefdb76d430938168621706a20b2b62a34318499fd72a8c934b08f690f9b72d19ed26581094517a0986586f95bc4b23fa8743b24d11
ddddffda3a doc: Add doc/release-notes-35836.md (MarcoFalke)
fa7fe798c6 wallet: Remove meaningless bool fallback in FundTransaction (MarcoFalke)
Pull request description:
This mostly removes a no-op and meaningless bool fallback in the `fundrawtransaction` RPC.
This allows to remove a `skip_type_check`. This makes validating the JSON schema from https://github.com/bitcoin/bitcoin/pull/34683 more consistent.
Adding the type check here is useful, because:
* The fallback was added in af4fe7fd12 (more than a decade ago). Retaining backwards compat with more than 10-year old clients seems purely theoretical. There were other breaking RPC changes on shorter notice in the meantime. If someone really forgot to update this over the last 10 years, I don't see a downside of notifying them.
* The compat only works for positional args, which seems another small reason to drop it.
* The compat is now fully irrelevant and a no-op, given that watch-only wallet is not a concept
anymore after commit 1337c72198.
* Keeping the compat means that openrpc spec users do not get any type checks at all here.
ACKs for top commit:
polespinasa:
ACK ddddffda3a
sedited:
ACK ddddffda3a
Tree-SHA512: aa5113bd74159ae5a3bf189edc48d011761db98beaf701cd144ba94eac9f525bec2b8eeb53e588b2f20dc9965ebabef2c5ce55a732b8cef2874a95f398ecae8f
Add Cache-Control headers to REST API responses so standard HTTP caches
can cache safe responses by default without per-deployment proxy rules.
Cache policy summary:
- Immutable: /block binary and hex responses, /blockpart, /blockfilter,
and /spenttxouts in all formats, and blockhash-specific
/deploymentinfo/<blockhash>.json responses return
"public, immutable, max-age=86400".
- No-store: /block and /block/notxdetails JSON, /tx, /headers,
/blockfilterheaders, /blockhashbyheight, /chaininfo, /mempool,
/getutxos, tip-relative /deploymentinfo.json, and RESTERR error
responses return "no-store".
Mutable responses are not stored because REST does not provide cache
validators such as ETag or Last-Modified.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
fa5cbb8909 uint256: Workaround GCC-14 stringop-overread bug in Compare (Ava Chow)
6c9d76d589 doc: release note for alternate_wtxids in gettransaction (Ava Chow)
99bdcb064c test: compat, ensure downgrade preserves tx witness variants (furszy)
ef2afc6a0a test: Test for wallet txs with alternate wtxids (Ava Chow)
2d55c7a74d wallet: Show alternate wtxids in gettransaction (Ava Chow)
0b1af01bd4 wallet: Replace CWalletTx::SetTx with Update (Ava Chow)
56cf27db4d wallet: Store all witness variants of a transaction (furszy)
798ba6d04f wallet: Make CWalletTx::tx private and use CWalletTx::GetTx to access (Ava Chow)
72ebdd6364 wallet: Remove unused CWalletTx CopyFrom and copy constructor (Ava Chow)
19af439bdf wallet: Deserialize directly in CWalletTx's ctor (Ava Chow)
Pull request description:
When the wallet is presented with a transaction that has the same txid as one already known to the wallet, but has a different witness, instead of ignoring the transaction, store it alongside the known tx. This enables the wallet to be aware of all wtxid variants of its transactions. This also allows for the wallet to be able to calculate fees for replacements better as txs with different witnesses may have different feerates.
Specifically, the wallet stores these alternates in `CWalletTx` and extends the existing `tx` record type to essentially have a vector of transactions appended to the record. In `CWalletTx`, the single transaction is replaced with a map of wtxid to transaction so that all witness variants can still be represented by a single `CWalletTx`. For all of the various things that need the tx from a `CWalletTx`, a single witness variant is chosen to be the canonical tx and returned by `GetTx()`. This canonical tx is written into the same place as the previous single tx was written to in the `tx` record so that wallets can be loaded into previous versions.
To choose the canonical transaction, if any of the variants is confirmed, then that is the canonical one. Otherwise, the witness variant with the least weight is chosen.
An additional change I've included is to make `CWalletTx` RAII. This simplifies some of the implementation and enforces the assumption that a `CWalletTx` always has a transaction.
Lastly, `gettransaction` and `listtransaction` have a new field `alternate_wtxids` to inform users of the wtxids of the witness variants for a transaction, and of course, a test.
Closes#11240
ACKs for top commit:
furszy:
ACK fa5cbb8909
ajtowns:
ACK fa5cbb8909
w0xlt:
ACK fa5cbb8909
Tree-SHA512: ee303b395ab7a0843969f9491f876f4472c6301e968d9db87312edf44f7447245e707dd544356371d5f32fe6a619ee6937c24f3b7899f7a8108090b425f22d8e
fa7f553781 test: Suppress implicit-unsigned-integer-truncation:SaltedCoinsCacheHasher::operator() (MarcoFalke)
Pull request description:
The truncation of u64 to size_t is intentional here, but it would be nice to document that for ubsan.
Otherwise, ubsan will print warnings about this. E.g. on 32-bit platforms:
```
/ci_container_base/src/coins.h:255:16: runtime error: implicit conversion from type 'uint64_t' (aka 'unsigned long long') of value 18400304222395891501 (64-bit, unsigned) to type 'size_t' (aka 'unsigned int') changed the value to 2265382701 (32-bit, unsigned)
```
This is a bit tedious to test on 64-bit platforms, but one can use a diff like:
```diff
diff --git a/src/coins.h b/src/coins.h
index c854893bcb..906be9efae 100644
--- a/src/coins.h
+++ b/src/coins.h
@@ -246,3 +246,3 @@ public:
/** Hash a transaction ID, itself a cryptographic hash, as one jumbo block. */
- size_t operator()(const Txid& id) const noexcept
+ uint32_t operator()(const Txid& id) const noexcept
{
@@ -252,3 +252,3 @@ public:
/** Hash an outpoint as its txid jumbo block followed by the zero-extended index as one normal block. */
- size_t operator()(const COutPoint& id) const noexcept
+ uint32_t operator()(const COutPoint& id) const noexcept
{
```
and:
```
$ UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./bld-cmake/bin/test_bitcoin
./src/coins.h:255:16: runtime error: implicit conversion from type 'uint64_t' (aka 'unsigned long') of value 17092028281225243117 (64-bit, unsigned) to type 'uint32_t' (aka 'unsigned int') changed the value to 291269101 (32-bit, unsigned)
ACKs for top commit:
l0rinc:
ACK fa7f553781
sedited:
ACK fa7f553781
Tree-SHA512: 0798e09a1291c7e7e2799c586f416b9e92ddf3ad64af3888a01726f338935f735c4e36716a7c38426e75ddbbdc2854949c3f553ab67758a053a2e3ba6a3c3ec1
This RPC has no helpful use while being both dangerous and a maintenance
burden.
Despite what the name says, it allows the deletion of arbitrary
transactions, and `importprunedfunds` does not allow the importing of
transactions not belonging to the user, and `listtransactions` does not
list transactions not belonging to the wallet, so this RPC can only be
used to delete transactions actually belonging to the wallet, and in the
unlikely event that transactions not belonging to the wallet are
present, they cause no harm except for occupying a few bytes on the
users disk.
The SOCKS5 destinations factory classified connections by scanning
debug.log for connection attempts to the requested address and port.
The destination is not unique per connection, so the log cannot
identify which attempt is being served: first-match returned a stale
type when an automatic connection reused an address private broadcast
had already used, and latest-match still breaks if two attempts to the
same address overlap.
Match the exact connection instead: the source addr:port of the
proxy's client socket equals the node's addrbind for that peer, so
looking it up in getpeerinfo identifies precisely the connection being
served and returns its connection_type. This also stops treating
debug.log contents as a stable interface.
Co-authored-by: Greg Sanders <gsanders87@gmail.com>
f4a6d079c4 qa: Support `get_bind_addrs` and `feature_bind_extra` on illumos (Hennadii Stepanov)
5e96a8fd5a doc: Add `lsof` to Test Suite Dependencies on NetBSD (Hennadii Stepanov)
5d01aa4772 qa: Ignore `lsof` warnings on NetBSD (Hennadii Stepanov)
70352fda03 qa: Strip prefix length from NetBSD `ifconfig` output (Hennadii Stepanov)
1c1735567e doc: Add `lsof` to Test Suite Dependencies on FreeBSD (Hennadii Stepanov)
4cb7f39c2c qa: Drop OpenBSD from supported platforms in `get_bind_addrs` function (Hennadii Stepanov)
8a982eea85 qa: Add `skip_if_no_lsof_on_nonlinux` helper and use it where needed (Hennadii Stepanov)
Pull request description:
This PR is a follow-up to #34256. It extends functional test support to illumos-based OSes and fixes several related issues on the *BSDs.
Changes:
- Make `lsof` an optional functional test dependency via a new `skip_if_no_lsof` helper, consistent with other optional test deps.
- Strip the CIDR prefix length from NetBSD `ifconfig` output (no-op on other platforms).
- Suppress spurious `lsof` warnings on NetBSD.
- Drop OpenBSD from the platforms supported by `get_bind_addrs`.
- Document the `lsof` Test Suite Dependency for FreeBSD and NetBSD.
- Add support for `get_bind_addrs` and `feature_bind_extra` on illumos.
CI runs: https://github.com/hebasto/bitcoin-core-nightly/pull/280.
Addresses https://github.com/bitcoin/bitcoin/pull/34256#issuecomment-4361855749.
ACKs for top commit:
l0rinc:
Lightly tested code review ACK f4a6d079c4
sedited:
utACK f4a6d079c4
Tree-SHA512: 24d943d059f5fa3f5626017eff744836177a41724544355f34b3a31fdf287bd1916bc6e903b598c1c55b61da2ff0f931b4455542d9cff6cf399ef7963096dff4
28641fd195 p2p: reject empty getblocktxn requests (furszy)
9871fb726c p2p: reject filtered block inv early when bloom is disabled (furszy)
aaf9412026 refactor: split p2p_getdata.py in sub-cases (furszy)
Pull request description:
Reject requests that make the node read blocks from disk unnecessarily:
* `getblocktxn` is meant to request the txs a peer is missing. When a
peer sends a `getblocktxn` with an empty index vector, it isn't missing
anything, so it shouldn't have sent the message in the first place.
* The peer should not request a filtered block when the node does not
advertise the `NODE_BLOOM` service. Filtered blocks are built from
the bloom filter, which can be loaded only when the `NODE_BLOOM`
service is offered.
Both are disconnected now.
Note: can be split in two PRs if preferred.
ACKs for top commit:
151henry151:
ACK 28641fd195
l0rinc:
lightly tested ACK 28641fd195
mzumsande:
Code Review ACK 28641fd195
winterrdog:
tested ACK 28641fd195
sedited:
ACK 28641fd195
Tree-SHA512: 787ee0741fb797ea0898daab1bf3d7b3a21d91c9d940fc833e893806e95843e9d5a7a79a55ba67ac0f1550c08f2b6cee5ab2b625c082f56885ae795958cc608c
Descriptor ranges may end at `INT32_MAX`, but the expansion loop counts with `int`.
Incrementing after the final index overflows, terminating the node in `-ftrapv` builds and invoking undefined behavior otherwise.
Use `int64_t` so the final increment stays representable.
55d3cd51a4 doc: add release note describing change for forbidden clients (Matthew Zipkin)
d1ed2a6e25 http: check rpcallowip immediately after accepting connection (Matthew Zipkin)
Pull request description:
This is a follow-up to #35182 addressing a review comment from that PR: https://github.com/bitcoin/bitcoin/pull/35182#pullrequestreview-4322490068
This update to HTTPServer checks the IP subnet allowlist as soon as possible (immediately after receiving a connection from a client) before any data is received. This does not entirely protect the server from the "slow loris" attack or [CWE-400](https://cwe.mitre.org/data/definitions/400.html) but does restrict the attack surface to localhost and clients explicitly allowed by the user.
If a client is not allowed by the list, we disconnect as soon as possible. This is a behavior change from master branch (and previous release with libevent) where `403 Forbidden` was returned (after a potentially large amount request data was written to memory by the server).
To facilitate existing unit tests, this commit includes a refactor that moves the subnet allow list and relevant methods into the HTTPServer class instead of static file scope. This is needed because otherwise the allow list would be empty when the unit tests run.
There is still plenty of refactoring to do in order to modernize `HTTPServer` and de-globalize it, but since this specific issue has a resource allocation guard, I wanted to open it quickly on its own.
ACKs for top commit:
janb84:
ACK 55d3cd51a4
winterrdog:
ACK 55d3cd51a4
w0xlt:
ACK 55d3cd51a4
fjahr:
Code review ACK 55d3cd51a4
Tree-SHA512: 545911f2e4d2f97ab8bc854e9e57c39eb896428f8c349d34c8e8025a1f6bfb8cfd436f381e36af8b87592c07df3e16210819f3eef7943e23c6626030e615fdf5
