8deed0df06 doc: add release notes for PR 34911 (rkrux)
1a85ca1dff rpc, mempool: rpcdeprecate `bip125-replaceable` key in mempool RPCs reponses (rkrux)
f89d18c3b1 rpc, mempool: rpcdeprecate `fullrbf` key in getmempoolinfo RPC response (rkrux)
Pull request description:
mempoolfullrbf=1 behaviour has been the default since v28 and the argument has
been removed since v29 subsequently.
The getmempoolinfo RPC need not return the deprecated `fullrbf` key in the
response anymore unless the user requests it via -deprecatedrpc=fullrbf node
argument.
Also, remove the `bip125-replaceable` key from the mempool RPCs
responses (because it, too, has been deprecated since v29) unless the user
requests it via -deprecatedrpc=bip125 node argument.
ACKs for top commit:
optout21:
ACK 8deed0df06
sedited:
ACK 8deed0df06
instagibbs:
ACK 8deed0df06
Tree-SHA512: dfbbdf04ae25dafe81758bd85715af80d736e21bb913ab09ff8fd8225587b7a1bf1dd6b0f7ea05135504e11d43371d98e5f5fc6e3717d581e870e8c2bf97811b
2a90b6132a Add release notes for exportasmap (Fabian Jahr)
8cb2d926b4 rpc: Add exportasmap RPC (Fabian Jahr)
Pull request description:
This depends on the embedded data PR itself (#28792), until merge this will remain in draft status. All commit but the last one are from there.
There has been interest in exporting the embedded data back into a file. This is implemented here with a simple `exportasmap` RPC which provides this functionality. The exported file can be used to verify the data integrity or statistical analysis using e.g. `contrib/asmap-tool.py`.
ACKs for top commit:
achow101:
ACK 2a90b6132a
sedited:
ACK 2a90b6132a
seduless:
Tested ACK 2a90b6132a
Tree-SHA512: c8453078efe916ed95bff0695aabd9123d805970e037fad5e9317f4d43e2a4b21970030265bc82b00f3a222ee3db11346eef1fb6fc9393429b9bc6449f1830f1
RPCs: getrawmempool, getmempoolancestors, getmempooldescendants, getmempoolentry
This key has been marked deprecated since v29, it can be
removed from the RPC response now unless the user requests it via the
-deprecatedrpc=bip125 node argument.
mempoolfullrbf=1 behaviour has been the default since v28 and the argument has
been removed since v29 subsequently. The getmempoolinfo RPC need not return
the deprecated `fullrbf` key in the response anymore unless the user requests
it via -deprecatedrpc=fullrbf node argument.
32325d1777 tests: Add test for mempool-invalid wallet tx (Anthony Towns)
25e063d950 wallet: Add separate balance info for non-mempool wallet txs (Anthony Towns)
81e763f1e5 wallet: Have GetBalance report used amount directly without two calls (Anthony Towns)
Pull request description:
Changes `getbalances` to report the sum of txos spent by transactions that aren't confirmed nor in the mempool (eg due to being part of too long a mempool chain, or spending non-standard outputs, or having a datacarrier output that exceeds `-datacarriersize`, etc). Those values are added to the trusted/untrusted_pending/immature/used fields as appropriate (where previously they were skipped), and subtracted from the new nonmempool field, so that the sum of all fields remains the same.
For example:
```
$ bitcoin-cli -regtest getbalances
{
"mine": {
"trusted": 6049.99999220,
"untrusted_pending": 0.00000000,
"immature": 3200.00000780,
"nonmempool": -100.00000000
},
"lastprocessedblock": {
"hash": "3ab4582226d5e8ad76438db48d76e822c31bce2cdbc7ba82a5d974a277515d0d",
"height": 221
}
}
```
Closes#11887
ACKs for top commit:
achow101:
ACK 32325d1777
w0xlt:
lgtm ACK 32325d1777
musaHaruna:
Code Review ACK [32325d1](32325d1777)
Tree-SHA512: 142581944d1b3213067e219e3b8205f27b89007e545149c01b801bad38fe730c5b2bfdfe6a2064c3649889f66ec48ec7616982564d00e3d83837249e925d8f16
9fe5896a44 tor: torcontrol disconnect on too many lines to avoid OOM (David Gumberg)
8b68287bf9 test: Make torcontrol max line length test stricter and test boundaries. (David Gumberg)
ab5889796f refactor: torcontrol add connection checks to restart_with_mock (David Gumberg)
Pull request description:
LLM disclosure: Found with the help of Claude Opus 4.6, fix, test, description, and commit messages written by me.
------
This fixes a low-severity issue where a misbehaving Tor control daemon can cause
bitcoind to OOM by sending continuation lines without sending `250 OK` or
similar.
This issue is not that serious because if your tor control daemon is malicious you are already in all kinds of trouble, but as a matter of robustness this should be fixed.
The fix is to prevent the `TorControlConnection::m_message` buffer from growing
without bound by by limiting the number of lines handled by `TorControlConnection::ProcessBuffer()`
to `MAX_LINE_COUNT = 1000`. Now the most memory that can be occupied by
`m_message` is on the order of `MAX_LINE_LENGTH * MAX_LINE_COUNT= 100MB`
Although this is not compliant with the Tor control protocol in general,
where commands like `GETINFO ns/all` will likely return thousands of
lines, it is more than sufficient for handling the replies from the
commands that are used by a node:
<details>
<summary>
#### Tor control commands used by Bitcoin Core
</summary>
`AUTHENTICATE`: 1 line:
The server responds with 250 OK on success or 515 Bad
authentication if the authentication cookie is incorrect. Tor closes
the connection on an authentication failure.
https://spec.torproject.org/control-spec/commands.html#authenticate
`GETINFO net/listener/socks`: 2 lines
A quoted, space-separated list of the locations where Tor is
listening...
https://spec.torproject.org/control-spec/commands.html#getinfo
`AUTHCHALLENGE SAFECOOKIE`: 1 line
If the server accepts the command, the server reply format is:
```
"250 AUTHCHALLENGE" SP "SERVERHASH=" ServerHash SP "SERVERNONCE="
ServerNonce CRLF
```
https://spec.torproject.org/control-spec/commands.html#authenticate
`PROTOCOLINFO`: 4-5 lines
The server reply format is:
```
250-PROTOCOLINFO" SP PIVERSION CRLF \*InfoLine "250 OK" CRLF
InfoLine = AuthLine / VersionLine / OtherLine
```
(https://spec.torproject.org/control-spec/commands.html#protocolinfo)
`ADD_ONION`: 2-3 lines for Bitcoin Core's tor control client.
The server reply format is:
```
"250-ServiceID=" ServiceID CRLF
["250-PrivateKey=" KeyType ":" KeyBlob CRLF]
*("250-ClientAuth=" ClientName ":" ClientBlob CRLF)
"250 OK" CRLF
```
...
The server response will only include a private key if the server
was requested to generate a new keypair
...
If client authorization is enabled using the “BasicAuth” flag (which
is v2 only), the service will not be accessible to clients without
valid authorization data (configured with the “HidServAuth” option).
The list of authorized clients is specified with one or more
“ClientAuth” parameters. If “ClientBlob” is not specified for a
client, a new credential will be randomly generated and returned."
https://spec.torproject.org/control-spec/commands.html#add_onion
We don't set the `BasicAuth` flag, so the response will not include any
`ClientAuthLines`.
</details>
## Reproduce
To reproduce this issue, the following script or similar can be used as the
misbehaving Tor control daemon:
```python
#!/usr/bin/env python3
"""
A fake Tor control service that never finishes its reply. Sends unlimited
continuation lines ("250-...") without ever sending the final "250 ...".
Each line accumulates in m_message.lines with no cap. Bitcoind OOMs.
"""
import socket
import time
PORT = 19191
server = socket.create_server(("127.0.0.1", PORT))
conn, _ = server.accept()
conn.recv(4096) # Receive PROTOCOLINFO
time_start = time.time()
try:
while True:
conn.sendall(b"250-Ceaseless\r\n" * 10000)
except (BrokenPipeError, ConnectionResetError):
elapsed = time.time() - time_start
print(f"Node disconnected after {elapsed:.2f}s")
```
**🟡¡This will OOM, run in a container, VM, or some sandbox with memory limits!🟡**
Start a node with `-torcontrol=127.0.0.1=19191`.
E.g. with systemd:
```bash
systemd-run --user --scope -p MemoryMax=2G -p MemorySwapMax=0 bitcoind -regtest -torcontrol=127.0.0.1:19191
```
ACKs for top commit:
fjahr:
ACK 9fe5896a44
danielabrozzoni:
Code review ACK 9fe5896a44
janb84:
ACK. 9fe5896a44
sedited:
ACK 9fe5896a44
Tree-SHA512: ccbeba40c096e1fa3911c75c49e3a5c403712f646d77329de48017a19d1f0caa2ee4cc148b6c6473f68e55d7da04f17eb67748b5bf4dede3579b944ee5370cf5
5f36e0ff1e rpc: fix getblockstats UTXO overhead accounting (Lőrinc)
76190489e6 coins: pack `Coin` height/coinbase consistently (Lőrinc)
1f309d1aa2 coins: make `Coin::fCoinBase` a bool (Lőrinc)
Pull request description:
The [`getblockstats` RPC](https://github.com/bitcoin/bitcoin/pull/10757) currently overestimates UTXO overhead by treating the `fCoinBase` bitfield as an additional `bool` in `PER_UTXO_OVERHEAD`.
However, `fCoinBase` and `nHeight` are stored as bitfields and effectively packed into a single 32-bit value; counting an extra bool in the overhead calculation is unnecessary.
This PR introduces the following changes across three commits:
* Store `fCoinBase` as a `bool` bitfield to reduce implicit conversions at call sites.
* Use a consistent height/coinbase packing style across `Coin` serialization, undo serialization, and coinstats hashing (casting `nHeight` to `uint32_t` before shifting to avoid signed-promotion UB).
* Adjust UTXO overhead estimation to match the actual `Coin` layout and update the related tests accordingly.
ACKs for top commit:
achow101:
ACK 5f36e0ff1e
sedited:
ACK 5f36e0ff1e
vasild:
ACK 5f36e0ff1e
optout21:
crACK 5f36e0ff1e
Tree-SHA512: f4a44debed358e9e130da9d6fae5f89289daa34f0bdb7155edc3e9b691c219451f4c80b1e16aca5b011f0fa2fa975484ef1af4ca4563b7c6ba4ca9dd133f30be
6ac49373aa test: Add clean shutdown to Socks5Server (optout)
Pull request description:
Add clean shutdown to `Socks5Server` test utility, fix a sporadic CI failure.
Closes#34849.
The `Socks5Server` utility handles multiple incoming connections, which are handled in separate background threads. Its `stop()` method unblocks and waits for the main background thread cleanly, but it doesn't attempt to wait for the completion of handler threads. There is no guarantee that the handler threads are finished after `stop()` returns, which can lead to IO errors.
In the reported sporadic test failures, the test in `p2p_private_broadcast.py` uses the Socks5 server, and makes a node connect to/through it. Then it stops the Socks5 server, and then it stops the node. However, if a connection handler is still active, that can lead to errors, as the socket is closed.
The change attempts to fix this by storing all handler threads and connections, and attempting to shut them down before `stop()` returns.
Notes:
- ~~I was not able to reliably reproduce the failure locally.~~
Update: I could reproduce the failure, see https://github.com/bitcoin/bitcoin/issues/34849#issuecomment-4126331780 .
Running the relevant test:
```sh
build/test/functional/test_runner.py p2p_private_broadcast.py
```
ACKs for top commit:
andrewtoth:
re-ACK 6ac49373aa
achow101:
ACK 6ac49373aa
vasild:
ACK 6ac49373aa
w0xlt:
ACK 6ac49373aa
Tree-SHA512: c63a62a516925252c450c9b7931539aedac2e44566bd4fe217aa54e6ca1438c8f50a100c714166b7cc67786b00f42c1a36e4916d63311842a77293cd2e102356
c54f37c1ba cli, rpc: add -rpcid option for custom request IDs (Torkel Rogstad)
Pull request description:
Add a `-rpcid` CLI argument to `bitcoin-cli` that allows setting a custom string as the JSON-RPC request ID instead of the hardcoded default of 1. This enables correlating requests and responses when debugging or when multiple clients are making concurrent calls.
On the server side, include the request ID in the RPC debug log line.
Tests are added in `test/functional/interface_bitcoin_cli.py` for this.
ACKs for top commit:
maflcko:
lgtm ACK c54f37c1ba
stickies-v:
ACK c54f37c1ba
sedited:
ACK c54f37c1ba
Tree-SHA512: b693a50a2be5dc7797e5b61bddc4c10237888b52065d1b68029d7211542e0dd44f2b3dfb9b3bd7c5e4d9026766817de66a7ef4f29a8d97a268554ba775063e10
fa16bc53d7 test: Require named arg for create_block ntime arg (MarcoFalke)
fab352053d test: Remove unused create_coinbase imports (MarcoFalke)
fad6deb3cb scripted-diff: Use new create_block height option (MarcoFalke)
fa5eb74b96 test: Allow to set height in create_block (MarcoFalke)
Pull request description:
The `create_block` helper is often called with `create_coinbase`: `create_block(prev_hash, create_coinbase(new_height))`
This is fine, but a bit verbose and tedious to type each time. Also, it requires an additional import.
Improve this by allowing to set `height` in `create_block` directly, similar to other options, such as `ntime`, or `hashprev`.
ACKs for top commit:
l0rinc:
reACK fa16bc53d7
theStack:
re-ACK fa16bc53d7
Tree-SHA512: 41ca7327aac714b446d53b1a29f83e792f7fc13d567cd0f063f743d50b193fa0c71cc651454ae1f3c960b0b82cc8658932ea08b3be8632f69a18ccd09a052c17
fae807ed25 test: Remove unused, confusing and brittle connect_nodes.wait_for_connect (MarcoFalke)
fab2772647 test: Fix all races after a socket is closed gracefully (MarcoFalke)
fa21edddb2 test: Stricter checks in rpc_setban.py (MarcoFalke)
faa404e119 test: Add is_connected_to helper (MarcoFalke)
Pull request description:
Currently, functional tests may intermittently fail, due to a lack of synchronization after a node connection socket was closed gracefully: If node A is connected to node B, and node B closes the connection, node A *must* wait for the connection to be closed before continuing the test. Otherwise, subsequent re-connections may not work while a stale connection is still alive.
This can be reproduced locally via something like:
```diff
diff --git a/src/net.cpp b/src/net.cpp
index 6b79e913e8..32bd061500 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -2200,2 +2200,3 @@ void CConnman::SocketHandlerConnected(const std::vector<CNode*>& nodes,
}
+ UninterruptibleSleep(599ms);
pnode->CloseSocketDisconnect();
```
With this diff, the tests should fail on master, and pass after this fix.
The fix is placed inside the `connect_nodes` helper.
ACKs for top commit:
rkrux:
ACK fae807ed25
w0xlt:
ACK fae807ed25
davidgumberg:
(re-ish) crACK fae807ed25
sedited:
ACK fae807ed25
Tree-SHA512: 053825bbd319d7c49e08810bbabbf9c3a43d89897d697c0ca9232fd8a88ae475b4f9fbd79dff549b4e0a8941cf926ca4567e7fd43dc1749bf023d8ee7dd49608
fc736013a5 rpc: Add in_memory option to dumptxoutset with rollback (Fabian Jahr)
d0fd718948 test: Extend named pipe sqlite tool test to use rollback (Fabian Jahr)
ab9463efac test: Add dumptxoutset fork test (Fabian Jahr)
49d5e835a8 rpc: Don't invalidate blocks in dumptxoutset (Fabian Jahr)
fe58eb9850 blockstorage: Add DeletePruneLock (Fabian Jahr)
Pull request description:
This is an alternative approach to implement `dumptxoutset` with rollback that was discussed a few times. It does not rely on `invalidateblock` and `reconsiderblock` and instead creates a temporary copy of the coins DB, modifies this copy by rolling back as many blocks as necessary and then creating the dump from this temp copy DB. See also https://github.com/bitcoin/bitcoin/pull/29553#issuecomment-1978480989, https://github.com/bitcoin/bitcoin/issues/32817#issuecomment-3012406102 and #29565 discussions.
The nice side-effects of this are that forks can not interfere with the rollback and network activity does not have to be suspended. But there are also some downsides when comparing to the current approach: this does require some additional disk space for the copied coins DB and performance is slower (master took 3m 17s vs 9m 16s in my last test with the code here, rolling back ~1500 blocks). However, there is also not much code being added here, network can stay active throughout and performance would stay constant with this approach while it would impact master if there were forks that needed to be invalidated as well (see #33444 for the alternative approach), so this could still be considered a good trade-off.
ACKs for top commit:
stratospher:
tested ACK fc73601. very nice!
sedited:
Re-ACK fc736013a5
theStack:
re-ACK fc736013a5
Tree-SHA512: d3d674f68184ac3ada87d969d0fca7bc38203ee939853864adcd235ee3a954914c7e351b817800b885a495606e323392c27d88ba8d8e018eaf8567c098eb0e9c
This commit ensures the `TorControlConnection::m_message` buffer doesn't
grow unbounded and exhaust memory, by limiting the number of lines
handled by `TorControlConnection::ProcessBuffer()` to `MAX_LINE_COUNT =
1000`. Now the most memory that can be occupied by `m_message` is on the
order of `MAX_LINE_LENGTH * MAX_LINE_COUNT= 100MB`
Although this is not compliant with the tor control protocol in general,
where commands like `GETINFO ns/all` will likely return thousands of
lines, it is more than sufficient for handling the replies from the
commands that are used by a node:
`AUTHENTICATE`: 1 line:
The server responds with 250 OK on success or 515 Bad
authentication if the authentication cookie is incorrect. Tor closes
the connection on an authentication failure.
https://spec.torproject.org/control-spec/commands.html#authenticate
`GETINFO net/listener/socks`: 2 lines
A quoted, space-separated list of the locations where Tor is
listening...
https://spec.torproject.org/control-spec/commands.html#getinfo
`AUTHCHALLENGE SAFECOOKIE`: 1 line
If the server accepts the command, the server reply format is:
```
"250 AUTHCHALLENGE" SP "SERVERHASH=" ServerHash SP "SERVERNONCE="
ServerNonce CRLF
```
https://spec.torproject.org/control-spec/commands.html#authenticate
`PROTOCOLINFO`: 4-5 lines
The server reply format is:
```
250-PROTOCOLINFO" SP PIVERSION CRLF \*InfoLine "250 OK" CRLF
InfoLine = AuthLine / VersionLine / OtherLine
```
(https://spec.torproject.org/control-spec/commands.html#protocolinfo)
`ADD_ONION`: 2-3 lines for Bitcoin Core's tor control client.
The server reply format is:
```
"250-ServiceID=" ServiceID CRLF
["250-PrivateKey=" KeyType ":" KeyBlob CRLF]
*("250-ClientAuth=" ClientName ":" ClientBlob CRLF)
"250 OK" CRLF
```
...
The server response will only include a private key if the server
was requested to generate a new keypair
...
If client authorization is enabled using the “BasicAuth” flag (which
is v2 only), the service will not be accessible to clients without
valid authorization data (configured with the “HidServAuth” option).
The list of authorized clients is specified with one or more
“ClientAuth” parameters. If “ClientBlob” is not specified for a
client, a new credential will be randomly generated and returned."
https://spec.torproject.org/control-spec/commands.html#add_onion
We don't set the `BasicAuth` flag, so the response will not include any
`ClientAuthLines`.
Adds a check that at the boundary of MAX_LINE_LENGTH, no disconnect
occurs.
Also makes the overlength test message exactly MAX_LINE_LENGTH + 1 to
test the boundary.
Drops the redundant node liveness check, which is covered by the later
check that the node reconnects.
- Only one node needed for test
- Use ascii encoding instead of utf-8
- Make tests independent of each other
- Expect HTTP error code 413 for too-large request
- Clarify python client race condition in comment
Previously, it was only possible to set the height indirectly by calling
create_coinbase outside the create_block function. This is fine, but
verbose and not needed.
Just like hashprev can be set directly (instead of using the value from
tmpl), allow height to be set directly.
Also, use it in one place. The other places are done in a scripted-diff.
Also, add a unit test for the new feature.
Add a -rpcid CLI argument to bitcoin-cli that allows setting a custom
string as the JSON-RPC request ID instead of the hardcoded default of 1.
This enables correlating requests and responses when debugging or when
multiple clients are making concurrent calls.
On the server side, include the request ID in the RPC debug log line
when it differs from the default value of 1, so that custom IDs are
visible in the debug.log output.
422ca211ec test: ensure HTTP server enforces limits on headers and body size (Matthew Zipkin)
0c1a07e890 test: ensure HTTP server timeout is not caused by a delayed response (Matthew Zipkin)
f06de5c1ea test: clean up and modernize interface_http (Matthew Zipkin)
Pull request description:
This is a follow-up to https://github.com/bitcoin/bitcoin/pull/32408 and a new prerequisite for #32061 in response to a few review comments there.
### New test: `check_server_busy_idle_timeout()`
In https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2872150590 it is pointed out that the idle timeout set by `-rpcservertimeout` could disconnect a client unfairly if it was the server that was taking too long to process a response. That misbehavior was confirmed and #32061 was updated to match current libevent behavior. This PR asserts the current libevent behavior by adding another test using RPC `waitforblock` past the `-rpcservertimeout` value and then verifying that the HTTP connection is still open.
### Expanded tests: `check_excessive_request_size()` and `check_chunked_transfer()`
https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2941508964 made me realize that I was not testing HTTPRequest body size at all, and that headers size was being tested one line at a time but not in total. The libevent server does both things so that behavior is asserted in these expanded tests, and the mechanism in #32061 was improved to match.
### Clean up `interface_http.py`
Since I am extending this test module again I refactored the monolithic test to resemble the current functional test style in the repository, de-duplicating HTTP connection code with a helper class and separating individual test cases into functions.
ACKs for top commit:
fjahr:
ACK 422ca211ec
Bortlesboat:
re-ACK 422ca211ec. Checked out the rebased branch and ran interface_http.py locally, passes clean. Went through all three commits -- the helper class deduplication reads well and check_server_busy_idle_timeout is a nice addition from the #32061 discussion. One minor note inline.
hodlinator:
Concept ACK 422ca211ec
theStack:
ACK 422ca211ec
Tree-SHA512: 1165c9c49c8b1ae5a40a15e1d0f8275bb4d5e08a5eea7ae8b9d900bb34a85b29ba89c728b5e0866fbf289dd49aa5ece0af63e410e64aee70c89a19759c5ea3ff
The `Socks5Server` utility handles multiple incoming connections,
which are handled in separate background threads.
The `stop()` method unblocks and waits for the main background thread
cleanly, but it doesn't attempt to wait for any handler threads.
This change stores handler threads and connections, and attempts
to shut them down before `stop()` returns.
Co-authored-by: vasild <vd@FreeBSD.org>
Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>
3fd68a95e6 scripted-diff: replace remaining Python test equality asserts (Lőrinc)
301b1d7b1f test: add missing `assert_equal` imports (Lőrinc)
06a4176c42 test: convert truthy asserts in `wallet_miniscript` and `rpc_psbt` (Lőrinc)
d9a3cf20a4 test: convert simple equality asserts in excluded files (Lőrinc)
23c06d4e6d test: convert equality asserts with comments or special chars (Lőrinc)
dcd90fbe54 test: prep manual equality assert conversions (Lőrinc)
4f4516e3f6 test: split equality asserts joined by `and` (Lőrinc)
76a5570b36 test: use `in` for two-value equality asserts (Lőrinc)
Pull request description:
### Problem
Plain `assert x == y` is a poor fit for this test framework, `assert_equal()` gives more useful failure output and keeps equality checks consistent with the rest of the functional tests.
### Design
A simple scripted diff cannot safely rewrite all of them because many files use `==` inside larger expressions, such as chained conditions, comprehensions, and other compound assertions. That makes a one-shot mechanical conversion either incorrect or harder to review.
### Fix
This series first rewrites the non-mechanical cases into standalone assertions so patterns are easier to identify, then applies a scripted diff to the remaining cases that are safe to convert mechanically.
Partially fixes https://github.com/bitcoin/bitcoin/issues/23119 by adjusting the `==` case only (which is similar to the `BOOST` alternatives so it's expected to be uncontroversial).
ACKs for top commit:
maflcko:
review ACK 3fd68a95e6🚆
rkrux:
lgtm ACK 3fd68a95e6
theStack:
ACK 3fd68a95e6
Tree-SHA512: 6950ffa044db72d6235305f4c0247254e7e8f57ee1c8300e553953963914a6360ca71569fe315ecb333cf0e62f78b3a24603717f64229783761f8c1b5958fc12
bb00fd2142 test: use dynamic ports and add coverage in feature_bind_port_discover (b-l-u-e)
4f19508ae7 test: dont connect nodes in feature_bind_port_discover (b-l-u-e)
b8827ce619 net: Fix Discover() not running when using -bind=0.0.0.0:port (b-l-u-e)
Pull request description:
This PR fixes two related issues with address discovery when using explicit bind addresses in Bitcoin Core:
- #31293
- #31336
When using `-bind=0.0.0.0:port` (or `-bind=::`), the `Discover()` function was not being executed because the code only checked the `bind_on_any` flag. This led to two problems:
- The node would not discover its own local addresses if an explicit "any" bind was used.
- The functional test `feature_bind_port_discover.py` would fail, as it expects local addresses to be discovered in these cases.
This PR:
1. Checks both `bind_on_any` and any bind addresses using `IsBindAny()`
Ensures `Discover()` runs when binding to 0.0.0.0 or ::, even if specified explicitly.
2. Ensures correct address discovery
The node will now discover its own addresses when using explicit "any" binds, matching user expectations and fixing the test.
3. Maintains backward compatibility
The semantic meaning of `bind_on_any` is preserved as defined in `net.h`:
> "True if the user did not specify -bind= or -whitebind= and thus we should bind on 0.0.0.0 (IPv4) and :: (IPv6)"
4. Updates the test to use dynamic ports
The functional test `feature_bind_port_discover.py` is updated to use dynamic ports instead of hardcoded ones, improving reliability.
References
- Implementation follows the approach proposed in my [review comment on #31492](https://github.com/bitcoin/bitcoin/pull/31492#issuecomment-2941773645).
Closes#31293
The #31336 has the overall test failure and requires both this PR and #33362 to be fully resolved.
ACKs for top commit:
NicolaLS:
tested ACK bb00fd2142
pinheadmz:
ACK bb00fd2142
vasild:
ACK bb00fd2142
achow101:
ACK bb00fd2142
Tree-SHA512: 6f1b0b29cc539e4b49b3594f9ad70be90cfff28e31497a8b85e11d8a2509faaa8ca803e542ea3280588ece5a065c4c54193cec87cad221f1abae34d8b13cfdc5
1401011f71 test: Add test for exceeding max line length in torcontrol (Fabian Jahr)
84c1f32071 test: Add torcontrol coverage for PoW defense enablement (Fabian Jahr)
7dff9ec298 test: Add test for partial message handling in torcontrol (Fabian Jahr)
569383356e test: Add simple functional test for torcontrol (Fabian Jahr)
4117b92e67 fuzz: Improve torcontrol fuzz test (Fabian Jahr)
b1869e9a2d torcontrol: Move tor controller into node context (Fabian Jahr)
eae193e750 torcontrol: Remove libevent usage (Fabian Jahr)
8444efbd4a refactor: Get rid of unnecessary newlines in logs (Fabian Jahr)
6bcb60354e refactor: Modernize member variable names in torcontrol (Fabian Jahr)
a36591d194 refactor: Use constexpr in torcontrol where possible (Fabian Jahr)
Pull request description:
This is part of the effort to remove the libevent dependency from our code base: https://github.com/bitcoin/bitcoin/issues/31194
The current approach tries to reuse existing code and follows roughly similar design decisions. It replaces the libevent-based async I/O with blocking I/O utilizing the existing `Sock` and `CThreadInterrupt`. The controller runs in a dedicated thread.
There are some optional code modernizations thrown in made along the way (namings, constexpr etc.). These are not strictly necessary but make the end result with the new code more consistent.
ACKs for top commit:
achow101:
ACK 1401011f71
janb84:
re ACK 1401011f71
pinheadmz:
ACK 1401011f71
Tree-SHA512: 167f1d98a634524568cb1d723e7bdb7234bade2c5686586caf2accea58c3308f83a32e0705edc570d6db691ae578a91e474ae4773f126ec2e1619d3adf7df622
Some Python functional tests still use plain `assert x == y`.
The earlier commits convert the ambiguous assert patterns by hand, so this commit can rewrite the remaining safe cases mechanically.
The verify script excludes `wallet_bumpfee.py`, `test_framework/netutil.py`, and `test_framework/authproxy.py`, which still contain assert forms that the plain line-based substitution would misidentify.
-BEGIN VERIFY SCRIPT-
perl -pi -e 's/^(\s*)assert (.+?) == ([^,#]+?)$/\1assert_equal(\2, \3)/' $(git ls-files -- 'test/functional' \
':(exclude)test/functional/wallet_bumpfee.py' ':(exclude)test/functional/test_framework/netutil.py' ':(exclude)test/functional/test_framework/authproxy.py')
-END VERIFY SCRIPT-
The later scripted diff only rewrites call sites.
Add `assert_equal` imports in the remaining files that still need them so the mechanical replacement can apply cleanly.
Replace the last remaining truthy asserts (and a remaining `is False`) in `wallet_miniscript.py` and `rpc_psbt.py` with `assert_equal`.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
A few files still need to stay out of the later scripted diff because they contain more complicated assert shapes.
Convert the straightforward equality checks in those files by hand first.
Use a local import in `authproxy.py` so the change does not create an import cycle.
Some remaining `assert x == y` lines include inline comments, list literals, or descriptor strings with `#`.
Convert those sites by hand so the later mechanical rewrite can stay simple.
The later scripted diff only handles plain `assert x == y` lines.
Some remaining tests still use equality inside comprehensions, parenthesized asserts, and other shapes that the line-based rewrite would misread.
Rewrite those sites by hand first so the later mechanical conversion stays safe.
The commit also simplifies the dead `len(["errors"]) == 0` branch in `blocktools.py`, which can never be true.
Some tests combine multiple equality checks in one `assert`.
Split those checks into separate assertions so failures point at the exact mismatch.
This also removes mixed `==` expressions that the later cleanup should not touch.
Some tests spell a two-value membership check as `assert x == a or x == b`.
Rewrite those sites as `assert x in (a, b)`.
This keeps the check the same and removes `==` forms that the later cleanup should not touch.
Instead this new approach uses a temporary coins db to roll back the
UTXO set.
This new approach also prevents the node from pruning necessary blocks
during dumptxoutset execution by using prune locks.
43c528aba9 wallet, test: update `gethdkeys` functional test (rkrux)
6e3a0afc2f wallet: fix `gethdkeys` RPC for descriptors with partial xprvs (rkrux)
Pull request description:
Fixes#34378
A non-watch-only wallet allows to import descriptors with partial private keys, eg: a multisig descriptor with one private key and one public key. In case an xpub is imported in any such descriptors whose private key the wallet doesn't have, then the `gethdkeys` RPC throws an unhandled error like below when the private keys are requested.
This fix ensures that such calls are properly handled by conditionally finding the corresponding xprv and the related functional test is accordingly updated.
```
➜ bitcoincli -named gethdkeys private=true
error code: -1
error message:
map::at: key not found
```
ACKs for top commit:
achow101:
ACK 43c528aba9
w0xlt:
ACK 43c528aba9
Eunovo:
Tested ACK 43c528aba9
Tree-SHA512: 106e02ee368a3fa94d116f54f2fa71f9886e4753e331b91ce13a346559d5497ef6cd7ddaa8736fcfe1a7ac427a44d647fb75e523eb72f917fa866adbe0dbad30
