a49bc1e24e ci: add --extended when using --usecli (Pol Espinasa)
5603ae0ffa test: fix send_batch_request to pass callables when using --usecli (Pol Espinasa)
Pull request description:
**First commit**
This commit fixes an error that made feature_index_prune.py fail if --usecli was used.
The test was failing because `node.batch(data)` was called with `data` being a dict. This worked in the normal scenario because `AuthServiceProxy.batch()` expects a list of petitions in the form of a dict. But `TestNodeCLI.batch()` expects callables and not a dict. When `--usecli` is used the test fails with an error `TypeError: 'dict' object is not callable`.
This is fixed by using `get_request()` which returns a lambda function if `--usecli` is used and returns a dict if not.
The `assert` is also changed because before this PR, the requests, constructed by hand were not specifiying the json-rpc version. By default if no version is specified we use version 1.0 which always returns `error: none` if there is no error. However, in version 2.0, it does not include an error key if there is no error. By using `get_request()` the requests are done in version 2.0 so there's no key error.
**Second commit**
The current CI doesn't cover the case of running all tests with --extended if using --usecli.
This led to a test failing (feature_index_prune.py) if run with --usecli and the CI not catching it.
See https://github.com/bitcoin/bitcoin/pull/34991 for context.
This commit improves the CI test coverage by also running now all functional tests (--extended) with the flag --usecli.
<details>
<summary>First "wrong" approach</summary>
Fixes two bugs that make `feature_index_prune.py` fail if `--usecli` is used.
1. Makes `TestNodeCLI.batch()` response equivalent to what a JSON-RPC response would look like by adding `error=None` in the response. The lack of `error` in the response was giving a `KeyError` message.
2. Makes `send_batch_request()` compatible with --usecli. Before the PR it was passing dicts to `node.batch()`, but `TestNodeCLI.batch()` expects callables, not dicts.
</details>
ACKs for top commit:
Bicaru20:
Re-ACK a49bc1e24e.
sedited:
Re-ACK a49bc1e24e
Tree-SHA512: 75fca26cf120638ced1fe38e86d8e3efa7addb6d97fc801e34783efd2cf6417f4ded2ec6247b6dcbcdb3cf4f48c4858f0932cbaa3e836a973d53581e75470a3f
9fa4076b20 test: Test merging implicit PSBTv0 with explicit PSBTv0 (w0xlt)
1660c18232 doc: Release notes for psbtv2 (Ava Chow)
470e52a5f8 fuzz: Enforce additional version invariants in PSBT fuzzer (Antoine Poinsot)
5bd0579c09 test: Tests for PSBT AddInput and AddOutput (Ava Chow)
b8b6e7f0c2 tests: Add PSBT unit test for ComputeTimeLock (Ava Chow)
0bc1c2e508 tests: Add test vectors from BIP 370 (Ava Chow)
e0e4dbdeb5 psbt: Change default psbt version to 2 (Ava Chow)
bcc1dca77b Add psbt_version to PSBT RPCs and default to v2 (Ava Chow)
ab38c30195 Implement PSBTv2 field merging (Ava Chow)
93e339e29f Implement PSBTv2 AddInput and AddOutput (Ava Chow)
b39c86ae60 Allow specifying PSBT version in constructor (Ava Chow)
dcc9a3c8df Implement PSBTv2 in decodepsbt (Ava Chow)
5770dbd39f Add PSBT::ComputeLockTime() (Ava Chow)
863cf47b33 Update test_framework/psbt.py for PSBTv2 (Ava Chow)
925161eaf0 Implement PSBTv2 fields de/ser (Ava Chow)
d9cf658ee0 Restrict joinpsbts to PSBTv0 only (Ava Chow)
3da0e16012 Replace PSBT.tx with PSBT::GetUnsignedTx and PSBT::GetUniqueID (Ava Chow)
c568624ff2 psbt: Return std::optional from PrecomputePSBTData (Ava Chow)
092de4f1f6 Replace PSBT::GetInputUTXO with PSBTInput::GetUTXO (Ava Chow)
82c9fe3179 psbt: Use PSBTInput and PSBTOutput fields instead of accessing global tx (Ava Chow)
95897507e9 psbt: AddInput and AddOutput should take only PSBTInput and PSBTOutput (Ava Chow)
1b7d323a72 Add PSBTInput::GetOutPoint (Ava Chow)
543d3e1cdc psbt: add PSBTv2 global tx fields (Ava Chow)
c01c7f068c psbt: Remove default constructor (Ava Chow)
9671aa08c2 psbt: add tx input and output fields in PSBTInput and PSBTOutput (Ava Chow)
990b084f11 Have PSBTInput and PSBTOutput know the PSBT's version (Ava Chow)
7eacc21ff6 psbt: make PSBT structs into classes (Ava Chow)
f926c326bb gui: Store PSBT in std::optional in PSBTOperationsDialog (Ava Chow)
1e2d146b47 psbt: Refactor duplicate key lookup and size checks (Ava Chow)
88384180d3 test: PSBTs should roundtrip through RPCs that do nothing (Ava Chow)
001877500d test: construct psbt with unknown field programmatically (David Gumberg)
0cb884e6df psbt: Fill hash preimages and taproot builder from SignatureData (Ava Chow)
Pull request description:
BIP 370 PSBTv2 introduces several new fields and different invariants for PSBT. This PR implements those new fields and restructures the PSBT implementation to match PSBTv2 but still remain compatible with PSBTv0.
ACKs for top commit:
nervana21:
re-ACK 9fa4076b20
theStack:
re-ACK 9fa4076b20
w0xlt:
ACK 9fa4076b20
Tree-SHA512: ab0a5ada4fa5fca27ba9ec9c291a44b30e69d6db11971957572d86c58c71c4caa4557dc25f403e1170ba4fac751306d074cc582defefc6e2fdd37be51c3d9dd0
eed7af666b doc: Add release note for disallowing some wallet path names (David Gumberg)
3d7f0e4ed5 wallettool: Use GetWalletPath to determine the wallet path (Ava Chow)
2b0dc0d228 wallet: Disallow . and .. from wallet names (Ava Chow)
Pull request description:
Wallet names including `..` and `.` are unintuitive and can lead to various issues, see #34497
This disallows creating or loading wallets that have any path elements that are `..` or `.`, including any present in an absolute path. This does not disallow relative paths altogether but rather limits them to only being subdirectories of the wallets directory.
ACKs for top commit:
davidgumberg:
crACK eed7af666b
w0xlt:
ACK eed7af666b
arejula27:
ACK eed7af666b
Tree-SHA512: bec5e54369061eb630d9afb94701badce09e8beb63686cf714016466fc01653d4841030fc10fcd14d44e6b1022c0994cb32253d80533807704d9e11eda2423ff
1950da94fc test: enable `rpc_bind` on macOS and BSD (Lőrinc)
7236a05503 test: enable `feature_bind_extra` on macOS and BSD (Lőrinc)
Pull request description:
### Problem
Some functional tests are shown as skipped when running on macOS & BSD because `test_framework/netutil.py` only implemented the Linux-specific logic for checking which TCP sockets a node is listening on.
### Fix
Add macOS and BSD implementations in `test/functional/test_framework/netutil.py` so tests can query:
* which TCP sockets a node is listening on (`get_bind_addrs()`, via `lsof`)
* a non-loopback interface address (`all_interfaces()`, via `ifconfig`)
Then enable the previously Linux-only tests by switching to a shared POSIX platform guard.
### Commands
<details>
<summary><code>get_bind_addrs()</code> (<code>lsof</code> + regex)</summary>
> Command used
```bash
lsof -nP -a -p <pid> -iTCP -sTCP:LISTEN -Ftn
```
> Flags
- -D: device cache warnings
- -n: no hostname resolution
- -P: no service/port-name resolution
- -a: AND all conditions
- -p <pid>: filter by process ID
- -iTCP: TCP sockets only
- -sTCP:LISTEN: listening sockets only
- -Ftn: machine-readable output (fields: type `t`, name `n`)
> Regex parser
```regex
t(IPv[46])\nn(\*|\[.+?]|[^:]+):(\d+)
```
> Captured groups
- group 1: IPv4 / IPv6 (used to disambiguate `*`)
- group 2: host (`*`, `[::1]`, `127.0.0.1`, ...)
- group 3: port
</details>
<details>
<summary><code>all_interfaces()</code> (<code>ifconfig</code> + regex)</summary>
> Command used
```bash
ifconfig -au
```
> Regex parsing
Interface blocks:
```regex
(?m)^(?P<iface>\S+):(?P<block>[^\n]*(?:\n[ \t]+[^\n]*)*)
```
IPv4 extraction within each block:
```regex
inet (\S+)
```
</details>
### Notes
The only remaining platform skips on macOS are the USDT/BPF tracing tests (`interface_usdt_*.py`).
ACKs for top commit:
Sjors:
ACK 1950da94fc
achow101:
ACK 1950da94fc
willcl-ark:
tACK 1950da94fc
Tree-SHA512: 4cecc88852623f3fe3a7dccceb0e71932824c1ed7f1d4ab89b953ff6b7991afbd0b016c819c17e966bed53082dd623a832752b8847711861009cd5ffc4677367
08925d5ee7 test: add coverage for loading a wallet in a non-writable directory (furszy)
0218966c0d test: add coverage for wallet creation in non-writable directory (furszy)
bc0090f1d6 wallet: handle non-writable db directories (furszy)
Pull request description:
Make wallet creation and load fail with a clear error when the db directory isn’t writable.
#### 1) For Wallet Creation
Before: creating a wallet would return a generic error:
"SQLiteDatabase: Failed to open database: unable to open database file"
After: creating a wallet returns:
"SQLiteDatabase: Failed to open database in directory <dir_path>: directory is not writable"
#### 2) For Wallet Loading
We currently allow loading wallets located on non-writable directories. This is problematic
because the node crashes on any subsequent write; generating a block is enough to trigger it.
Can be verified just by running the following test on master: 85fa4e2910
Also, to check directory writability, this creates a tmp file rather than relying on the
`permissions()` functions, since perms bits alone may not reliably reflect actual writability
in some systems.
Testing Note:
Pushed the tests in separate commits so they can be cherry-picked on master for comparison.
ACKs for top commit:
rkrux:
re-ACK 08925d5ee7
achow101:
ACK 08925d5ee7
seduless:
Tested ACK 08925d5ee7
Tree-SHA512: e480eab329a1d595fe0b191e83c97956e3ff1d1e335ada8ac6fe72bc4b2bb9b13b0d49db0254d34ad75f816db06d9cd0c21d3063d7d8ee6687a7ea2324c36288
walletcreatefundedpsbt, createpsbt, converttopsbt, and psbtbumpfee take
a psbt_version argument to set the version of the PSBT that they
produce. The default psbt_version is 2.
Every key has a duplicate key lookup check, and many keys have fixed
size checks. These can be refactored to reduce code duplication.
Co-Authored-By: David Gumberg <davidzgumberg@gmail.com>
Wallet names that are also paths that contain . and .. are unintuitive
and can result in unexpected behavior, particularly in migration.
Therefore we should disallow users from specifying wallet names that
contain . and .. as path elements.
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
242b0ebb5c btcsignals: use a single shared_ptr for liveness and callback (Cory Fields)
b12f43a0a8 signals: remove boost::signals2 from depends and vcpkg (Cory Fields)
a4b1607983 signals: remove boost::signals2 mentions in linters and docs (Cory Fields)
375397ebd9 signals: remove boost includes where possible (Cory Fields)
091736a153 signals: re-add forward-declares to interface headers (Cory Fields)
9958f4fe49 Revert "signals: Temporarily add boost headers to bitcoind and bitcoin-node builds" (Cory Fields)
34eabd77a2 signals: remove boost compatibility guards (Cory Fields)
e60a0b9a22 signals: Add a simplified boost-compatible implementation (Cory Fields)
63c68e2a3f signals: add signals tests (Cory Fields)
edc2978058 signals: use an alias for the boost::signals2 namespace (Cory Fields)
9ade3929aa signals: remove forward-declare for signals (Cory Fields)
037e58b57b signals: use forwarding header for boost signals (Cory Fields)
2150153f37 signals: Temporarily add boost headers to bitcoind and bitcoin-node builds (Cory Fields)
fd5e9d9904 signals: Use a lambda to avoid connecting a signal to another signal (Cory Fields)
Pull request description:
This drops our dependency on `boost::signals2`, leaving `boost::multi_index` as the only remaining boost dependency for bitcoind.
`boost::signals2` is a complex beast, but we only use a small portion of it. Namely: it's a way for multiple subscribers to connect to the same event, and the ability to later disconnect individual subscribers from that event.
`btcsignals` adheres to the subset of the `boost::signals2` API that we currently use, and thus is a drop-in replacement. Rather than implementing a complex `slot` tracking class that we never used anyway (and which was much more useful in the days before std::function existed), callbacks are simply wrapped directly in `std::function`s.
The new tests work with either `boost::signals2` or the new `btcsignals` implementation. Reviewers can verify
functional equivalency by running the tests in the commit that introduces them against `boost::signals2`, then again with `btcsignals`.
The majority of the commits in this PR are preparation and cleanup. Once `boost::signals2` is no longer needed, it is removed from depends. Additionally, a few CMake targets no longer need boost includes as they were previously only required for signals.
I think this is actually pretty straightforward to review. I kept things simple, including keeping types unmovable/uncopyable where possible rather than trying to define those semantics. In doing so, the new implementation has even fewer type requirements than boost, which I believe is due to a boost bug. I've opened a PR upstream for that to attempt to maintain parity between the implementations.
See individual commits for more details.
Closes#26442.
ACKs for top commit:
fjahr:
Code review ACK 242b0ebb5c
maflcko:
re-review ACK 242b0ebb5c🎯
w0xlt:
reACK 242b0ebb5c
Tree-SHA512: 9a472afa4f655624fa44493774a63b57509ad30fb61bf1d89b6d0b52000cb9a1409a5b8d515a99c76e0b26b2437c30508206c29a7dd44ea96eb1979d572cd4d4
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>
