Extract the part of `p2p_private_broadcast.py` that starts
listening on a `P2PConnection` object (or its children classes)
and put it into `test_framework/p2p.py`.
Extract the part of `p2p_private_broadcast.py` that configures and
starts the SOCKS5 server into a reusable function and put it into
`test_framework/socks5.py`.
Use bind port 0 to let the OS pick an available port instead of
hackishly assuming that `p2p_port(N)` is available where N is more
than the number of the nodes the test uses.
89af67d79f tests: Add some fuzz test coverage for command-specific args (Anthony Towns)
92df785859 tests: Add some test coverage for ArgsManager::AddCommand (Anthony Towns)
33c8090be9 ArgsManager: automate checking for correct command options (Anthony Towns)
186354a0d8 bitcoin-wallet: use command-specific options (Anthony Towns)
d21e82b7d6 ArgsManager: support command-specific options (Anthony Towns)
Pull request description:
Adds the ability to link particular options to one or more (`OptionsCategory::COMMANDS`) commands, and uses this feature
in `bitcoin-wallet`. Separates out the help information for these command-specific options (duplicating it if an option applies to multiple commands), and provides a function for checking at runtime if some options have been specified by the user that only apply to other commands.
#### Motivation
Currently, `ArgsManager` supports commands like `bitcoin-wallet dump` but while some of the options are command-specific (like `-dumpfile`), `ArgsManager` itself doesn't know that. As a result, `-dumpfile` is listed in the global help rather than under the relevant commands, and if you use `-dumpfile` with a different command that doesn't support it, `ArgsManager` cannot automatically report that as an error, resulting in the commands that don't support the option having to have error-handling specific to all the options they don't support.
#### Changes
Help output moves command-specific options under their associated commands:
Before:
```
Options:
-dumpfile=<file name>
When used with 'dump', writes out the records to this file. When used
with 'createfromdump', loads the records into a new wallet.
...
Commands:
createfromdump
Create new wallet file from dumped records
dump
Print out all of the wallet key-value records
```
After:
```
Commands:
createfromdump
Create new wallet file from dumped records
-dumpfile=<file name>
When used with 'dump', writes out the records to this file. When
used with 'createfromdump', loads the records into a new wallet.
dump
Print out all of the wallet key-value records
-dumpfile=<file name>
When used with 'dump', writes out the records to this file. When
used with 'createfromdump', loads the records into a new wallet.
```
Error messages are now generated automatically by `ArgsManager` rather than ad-hoc wallet code for each option:
Before:
```c++
if (args.IsArgSet("-dumpfile") && command != "dump" && command != "createfromdump") {
tfm::format(std::cerr, "The -dumpfile option can only be used with the \"dump\" and \"createfromdump\" commands.\n");
return false;
}
```
After:
```c++
std::vector<std::string> details;
if (!args.CheckCommandOptions(command, &details)) {
tfm::format(std::cerr, "Error: Invalid arguments provided:\n%s\n", util::MakeUnorderedList(details));
return false;
}
```
#### Limitations
- If an option applies to multiple commands, it shares the same help text. There's no way to provide per-command descriptions.
- Option parsing rules are unchanged — options still cannot appear after the command.
ACKs for top commit:
achow101:
ACK 89af67d79f
sedited:
Re-ACK 89af67d79f
ryanofsky:
Code review ACK 89af67d79f. Since last review: rebase, integration with ClearArgs and fuzz test, and Assume -> Assert switch
Tree-SHA512: 7ae7c3b74d0c8c4db8459e9f0b9c7498b2fa4758954ec49983decbba177877b039779f0f7b55e60c3a0ed74c5e9e4ac4734ba9e049bf3a7743280ef8300869fa
da769855d0 test: add PSBT proprietary merge regression coverage (w0xlt)
3f5b3c7a80 psbt: preserve proprietary fields when combining PSBTs (w0xlt)
Pull request description:
BIP 174 proprietary fields are currently parsed, serialized, and exposed by `decodepsbt`, but they are not preserved by `combinepsbt`.
The reason is that the merge paths in `PartiallySignedTransaction::Merge()`, `PSBTInput::Merge()`, and `PSBTOutput::Merge()` union `unknown`, but never union `m_proprietary`.
This means application-specific PSBT metadata can be lost during combination, even though BIP 174 treats proprietary records as normal PSBT key-value pairs for private or application-specific use.
This PR fixes that by preserving proprietary fields in all three merge paths.
ACKs for top commit:
nervana21:
re-ACK da769855d0
Bicaru20:
re-ACK da769855d0
achow101:
ACK da769855d0
theStack:
Code-review ACK da769855d0
Tree-SHA512: 4474674ac00c3155fd7d3d777bdeb70d4ca2006c8fe62eb84a888ef2f7aa02739bf31d8e9f2cb59e324be1085c77897f0bbf6673254c12408ab09a1582e28b83
8544537f41 mining: drop unused include_dummy_extranonce option (Sjors Provoost)
58eeab790d mining: only pad with OP_0 at heights <= 16 (Sjors Provoost)
00d22328b0 mining: pad coinbase to fix createNewBlock at heights <=16 (Sjors Provoost)
605ff37403 test: bad-cb-length for createNewBlock() at low heights (Sjors Provoost)
1966621b76 test: refactor IPC mining test to use script_BIP34_coinbase_height (Sjors Provoost)
Pull request description:
Blocks 0-16 on any new chain require mining code to be careful not to violate the `bad-cb-length` rule, which states the coinbase transaction scriptSig must be at least 2 bytes.
Our mining code deals with that by padding the `scriptSig` with a 0 `extraNonce`. It does this for every height. As a result IPC clients would get an unnecessary `0` in the `scriptSigPrefix` field of `CoinbaseTx`. #32420 fixed that by introducing a `include_dummy_extranonce` option in `BlockCreateOptions` and turning that off for IPC clients.
A minor issue was missed though: `createNewBlock()` now fails with `bad-cb-length`. An easy workaround is to use the `generate` RPC for the first 16 blocks, as demonstrated in the 2nd commit.
The real fix is to have the miner code always pad the `scriptSig` at lower heights, but to _not_ include that in the `scriptSigPrefix` field of `CoinbaseTx` (introduced in #33819). This is what the 3rd commit implements.
Now that we set `scriptSigPrefix` independent of what our internal miner code does - to get past `CheckBlock()` - the original motivation for `include_dummy_extranonce` goes away and we can just drop it entirely. The last commit drops it, while the 4th commit adjusts the tests and hardcoded block and assume utxo hashes.
This last change does not break IPC clients, because `include_dummy_extranonce` was never exposed in `mining.capnp`.
Instead of adjusting the hardcoded hashes, an alternative approach would be to just always pad the `scriptSig` internally, since we exclude the padding from `scriptSigPrefix` anyway. However, IPC clients can also call `getBlock()` to get the raw block and might be confused about the difference. The miner code is also easier to understand if we limit the exception (`coinbase_tx.script_sig_prefix != coinbaseTx.vin[0].scriptSig`) to `nHeight <= 16`, where the explanation is based purely on consensus rules rather than historical test suite reasons.
The first two commits are preperation test changes:
- extract `assert_capnp_failed` helper for macOS (also part of #34727)
- use `script_BIP34_coinbase_height` in IPC mining test (existing code in `interface_ipc_mining.py` was incorrect for low height
Fixes#35126
ACKs for top commit:
ryanofsky:
Code review ACK 8544537f41. Just rebased to fix silent conflict and applied some minor suggestions since last review. As part of rereviewing I left some more minor suggestions that are fine to ignore.
sedited:
Re-ACK 8544537f41
Tree-SHA512: a01d48842bf4bcc1a9c51a89ef9d750766db7d04edb4dcd6b3a8bf195c6b4fa07445256a49367ff0db00ab489a52a3d7ff6a5c3ab9290ecb1fcb82f532552e9b
a39cc16b43 doc: Release note for addhdkey (Ava Chow)
89b9a01b4e wallet, rpc: Disallow importing unused() to wallets without privkeys (Ava Chow)
35bbee6374 wallet, rpc: Disallow import of unused() if key already exists (Ava Chow)
f3f8bcbd1d wallet: Add addhdkey RPC (Ava Chow)
82bc280de4 test: Simple test for importing unused(KEY) (Ava Chow)
80c29bc6f1 descriptor: Add unused(KEY) descriptor (Ava Chow)
Pull request description:
It is sometimes useful for the wallet to have keys that it can sign with but are not (initially) involved in any scripts, e.g. for setting up a multisig. Ryanofsky [suggested](https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1867721948) A `unused(KEY)` descriptor which allows for a key to be specified, but produces no scripts. These can be imported into the wallet, and subsequently retrieved with `gethdkeys`. Additionally, `listdescriptors` will output these descriptors so that they can be easily backed up.
In order to make it easier for people to add HD keys to their wallet, and to generate a new one if they want to rotate their descriptors, an `addhdkey` RPC is also added. Without arguments, it will generate a new HD key and add it to the wallet via a `unused(KEY)` descriptor. If provided a private key, it will construct the descriptor and add it to the wallet.
See also: https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1866961865
Based on #29130 as `gethdkeys` is useful for testing this.
ACKs for top commit:
Sjors:
utACK a39cc16
rkrux:
lgtm ACK a39cc16b43
Tree-SHA512: c1288c792ab01ca2eaddd24b0e7d11c259cd59e79042465d0d1eb656fd559c1200dc19750b4d84acc762b5b599935a06df214c18226e662087842ea91ec3011b
6d86184a8b rpc: combinerawtransaction now rejects unmergeable transactions (Adam Andrews)
Pull request description:
Previously, combinerawtransaction would silently return the first tx when asked to combine unrelated txs. Now, it will check tx mergeability and throws a descriptive error if tx cannot be merged.
fixes #25980
ACKs for top commit:
nervana21:
tACK 6d86184a8b
achow101:
ACK 6d86184a8b
rkrux:
ACK [6d86184](6d86184a8b)
Tree-SHA512: 5caf983c5ab618a000f40b9ad698439d3e6217ec2dc593740443f47d90f8804a895f3054dd29bbcecdb48a61992b0d0afda7ec89591d768a44918648bbb6e20d
Drop the include_dummy_extranonce branch from the OP_0 padding
condition in CreateNewBlock(), so that the dummy extraNonce is
only appended when consensus actually requires it (heights <= 16,
where the BIP34 height push alone would yield a 1-byte scriptSig
and trigger bad-cb-length).
The include_dummy_extranonce option struct field is now unused by
the miner and is removed in the next commit. Callers still set it,
so that this commit compiles.
Regenerate the hardcoded coinbase / block hashes throughout the
unit and functional test suites and update the regtest assumeutxo
snapshot in chainparams.
Additional side-effects:
- Without the dummy extranonce, coinbase scriptSigs are 1 byte
shorter at heights > 16, making every block 1 byte smaller.
This shifts where block files wrap and therefore where pruning
boundaries land.
- feature_assumeutxo malleation cases:
- case 1: error message changes due to UTXO reordering, similar
to 8f2078af6a
- case 4: the corruption byte is swapped from \x82 to \x83
because \x82 happened to be the actual value at that
offset in the new snapshot.
Since #32420, createNewBlock has thrown `bad-cb-length` errors when called at
low block heights because `OP_0` padding stopped being added to coinbase
transactions. (#32420 did add an `include_dummy_extranonce` option which could
bypass this, but it was not exposed to IPC clients.) Fix the problem by padding
coinbase transactions with `OP_0` when necessary to produce valid blocks.
Additionally this commit stops adding `OP_0` padding to the template
`script_sig_prefix` field when `include_dummy_extranonce` is true. This is safe
because non-IPC clients don't use this field, and IPC clients could never set
the option to true, and are expected to add their own nonces in any case.
This also improves documentation about the `script_sig_prefix` field and
`getCoinbaseTx` method.
On new regtest / signet chains (heights <= 16), createBlock() fails
internally with bad-cb-length. Since mining.capnp does not expose
include_dummy_extranonce, IPC clients can't work around this.
Add a functional test to illustrate this issue and use RPC to
work around it. The next commit introduces a fix.
Needed in a later commit to correctly derive the BIP34 prefix
for heights <= 16.
Add a padding parameter to script_BIP34_coinbase_height() that
controls whether the OP_0 dummy extranonce is appended for
heights <= 16.
Use this helper with padding=False in the IPC mining test's
build_coinbase_test().
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
Previously, combinerawtransaction would silently return the first tx when
asked to combine unrelated txs. Now, it will check tx mergeability and
throws a descriptive error if tx cannot be merged.
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
