552bc82b17961b86ae1964e817ba89ee7bfd985f doc: Use multipath descriptors in descriptors.md and linked test (Anurag chavan)
Pull request description:
Updates documentation and `wallet_miniscript_decaying_multisig_descriptor_psbt.py` to use single multipath descriptors with `<0;1>` syntax instead of separate external/internal descriptors.
## Changes
- **doc/descriptors.md**: Update examples (lines 70-71) to use `/<0;1>/*` multipath syntax
- **doc/descriptors.md**: Update Basic Multisig Example instructions (line 179) to use single multipath descriptor
- **test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py**: Refactor to use single multipath descriptor pattern matching `wallet_multisig_descriptor_psbt.py`
## Implementation
- `_get_xpub()` now extracts external descriptor and converts to multipath format
- `create_multisig()` imports single descriptor that expands to receive and change addresses
- Removed fake checksums from documentation examples
- Added clear comments documenting multipath convention
Fixes#34086
ACKs for top commit:
yashbhutwala:
ACK 552bc82b17961b86ae1964e817ba89ee7bfd985f
achow101:
ACK 552bc82b17961b86ae1964e817ba89ee7bfd985f
rkrux:
lgtm tACK 552bc82
Tree-SHA512: cc99271a3955daa475242d9f4ef8f09f4c94c64e48ec4647ecfd95dceb38bb0cdd91b78ec2d5f033b449d175eaecbdda49d6c766c8a1e1a01fed93be4eb0cfc0
6f7b4323cb46687ba9df7073a9cde427985d2dfc test: remove UNKNOWN_ERROR from script_tests (Bruno Garcia)
bd31a92d671610e1173a4e6f0c761d94724441ae script: use SCRIPT_ERR_SCRIPTNUM for CScriptNum errors (Bruno Garcia)
0ca4dcd78665bc5258a2b9cfcef0dcdf971a88c3 script: add SCRIPT_ERR_SCRIPTNUM error (Bruno Garcia)
Pull request description:
When evaluating a script, the current code is bad for analyzing some errors because it returns `SCRIPT_ERR_UNKNOWN_ERROR` for errors that are clearly known.
`CScriptNum` has two well defined errors: number overflow and non-minimally encoded number. However, for both errors we return as unknown. This PR changes it by adding a new ScriptError that is used for any `CScriptNum` error.
ACKs for top commit:
achow101:
ACK 6f7b4323cb46687ba9df7073a9cde427985d2dfc
w0xlt:
ACK 6f7b4323cb
darosior:
ACK 6f7b4323cb46687ba9df7073a9cde427985d2dfc
Tree-SHA512: e656d9992251fbc95d33966fa18ce64bf714179d51ba6a7f429e5a55bc58e7fc08827e4ab71ace0dd385dac7e1feaea621b49503387793a30eae7a7e44aa6b0f
fafdae46ff0b02d93d5fcff35f1185627d11d76a test: Check that redundant verack message is ignored (MarcoFalke)
Pull request description:
The code exists and is uncovered (ref https://maflcko.github.io/b-c-cov/total.coverage/src/net_processing.cpp.gcov.html#L3795), so add a trivial test to cover it.
ACKs for top commit:
brunoerg:
ACK fafdae46ff0b02d93d5fcff35f1185627d11d76a
sedited:
ACK fafdae46ff0b02d93d5fcff35f1185627d11d76a
Tree-SHA512: 157f434c2faa16243890b2344c4ee36bc359e56c80ba8a04f0bba71e9760cf9106c38ed755ff57eff8d1957f35516d20b3d010e0ecb8633b845f5314cc0d050a
7d9e1a810239a65a153c35f0f94490560441db49 test: Verify peer usage after assumeutxo validation completes (stringintech)
0067abe153298ce9f14262a15533033e6e907f2b p2p: Allow block downloads from peers without snapshot block after assumeutxo validation (stringintech)
Pull request description:
Currently, after assumeutxo background validation finishes, the node continues to skip peers that don't have the snapshot block in their best chain until restart. This unnecessarily excludes peers from block downloads even though the background sync has completed and undo data is available.
The restriction persists because `m_chainman.CurrentChainstate().SnapshotBase()` continues to return the snapshot base block until restart, even after validation completes. Added `m_chainman.CurrentChainstate().m_assumeutxo == Assumeutxo::UNVALIDATED` check to only apply the peer restriction while background validation is ongoing.
Also added test coverage in `feature_assumeutxo.py` that verifies peers without the snapshot block can be used for block downloads after background validation completes. The test fails without this fix.
ACKs for top commit:
fjahr:
Re-ACK 7d9e1a810239a65a153c35f0f94490560441db49
achow101:
ACK 7d9e1a810239a65a153c35f0f94490560441db49
sedited:
Re-ACK 7d9e1a810239a65a153c35f0f94490560441db49
Tree-SHA512: 5515971da7bf7efc55eecdf03686f44c20c9e52dd168e7cfa119032d6a8ebccee69df7143075e4e9d0a01426cd9ae7202dce5c00919a82478ebf49a15dc0fe19
c6ca2b85a3e6e73674e210aee4ed69c4af2848e4 validation: do not wipe utxo cache for stats/scans/snapshots (Pieter Wuille)
7099e93d0a80c65a547131d7bab977b09573310c refactor: rename `FlushStateMode::ALWAYS` to `FORCE_FLUSH` (Lőrinc)
Pull request description:
Revival of https://github.com/bitcoin/bitcoin/pull/30610#issuecomment-3432564955 with the remaining comments applied on top
> Since #28280, the cost of a non-wiping sync of the UTXO cache is only proportional to the number of dirty entries, rather than proportional to the size of the entire cache. Because of that, there is no reason to perform a wiping flush in case the contents of the cache is still useful.
>
> Split the `FlushStateMode::ALWAYS` mode into a FORCE_SYNC (non-wiping) and a FORCE_FLUSH (wiping), and then use the former in `scantxoutset`, `gettxoutsetinfo`, snapshot creation.
(slightly updated after #30214)
ACKs for top commit:
optout21:
reACK c6ca2b85a3e6e73674e210aee4ed69c4af2848e4
cedwies:
reACK c6ca2b8 (trivial)
achow101:
ACK c6ca2b85a3e6e73674e210aee4ed69c4af2848e4
sedited:
ACK c6ca2b85a3e6e73674e210aee4ed69c4af2848e4
Tree-SHA512: f3525a85dc512db4a0a9c749ad47c0d3fa44085a121aa54cd77646260a719c71f754ec6570ae77779c0ed68a24799116f79c686e7a17ce57a26f6a598f7bf926
2845f10a2be0fee13b2772d24e948052243782b8 test: extend FreeBSD ephemeral port range fix to P2P listeners (node)
34bed0ed8c449a3834927cec3447dbe6c74edf3d test: use IP_PORTRANGE_HIGH on FreeBSD for dynamic port allocation (woltx)
Pull request description:
Reopening #34336. I’ve now tested it on FreeBSD and confirmed it works.
On FreeBSD, the default ephemeral port range (10000-65535) overlaps with the test framework's static port range (11000-26000), possibly causing intermittent "address already in use" failures when tests use dynamic port allocation (`port=0`).
This PR adds a helper that sets `IP_PORTRANGE_HIGH` via `setsockopt()` before binding, requesting ports from 49152-65535 instead, which avoids the overlap, as suggested in https://github.com/bitcoin/bitcoin/issues/34331#issuecomment-3767161843 by @maflcko .
From FreeBSD's [sys/netinet/in.h](https://cgit.freebsd.org/src/tree/sys/netinet/in.h):
```c
#define IP_PORTRANGE 19
#define IP_PORTRANGE_HIGH 1
#define IPPORT_EPHEMERALFIRST 10000 /* default range start */
#define IPPORT_HIFIRSTAUTO 49152 /* high range start */
```
See also: FreeBSD https://man.freebsd.org/cgi/man.cgi?query=ip&sektion=4 man page.
Fixes#34331
ACKs for top commit:
vasild:
ACK 2845f10a2be0fee13b2772d24e948052243782b8
hebasto:
ACK 2845f10a2be0fee13b2772d24e948052243782b8, I have reviewed the code and it looks OK.
Tree-SHA512: ce501ce3e8a4023e07bad572df2b85d6829becf133813e4529aebba83e4eba59fa8b48e9d2197ebbb226adaf3054fad720775a787244d6b38c0078ee086102f4
1f60ca360eb83fa7982b1aac402eaaf477294197 wallet: fix removeprunedfunds bug with conflicting transactions (Martin Zumsande)
Pull request description:
`removeprunedfunds` removes all entries from `mapTxSpends` for the inputs of the pruned tx. However, this is incorrect, because there could be multiple entries from conflicting transactions (that shouldn't be removed as well). This could lead to the wallet creating invalid transactions, trying to double spend utxos.
The bug persists when the conflicting tx was mined, because the wallet trusts its internal accounting instead of calling `AddToSpends` again.
The added test should fail on master.
ACKs for top commit:
achow101:
ACK 1f60ca360eb83fa7982b1aac402eaaf477294197
fjahr:
tACK 1f60ca360eb83fa7982b1aac402eaaf477294197
furszy:
utACK 1f60ca360eb83fa7982b1aac402eaaf477294197
vasild:
ACK 1f60ca360eb83fa7982b1aac402eaaf477294197
Tree-SHA512: 3cc9ed547530fd53e25721177b76ab2e1eae16ce2c0e63fc01b20fdbf8bd02655dae51167ad56f9dec748d34c61ce65d38f993370820601f8257c73b876a3347
905dfdee86d679f8ea31d841bceb77a5724a6b1b test: use ModuleNotFoundError in interface_ipc.py (fanquake)
Pull request description:
Change this so we catch the case where the capnp shared libs have been updated, and can no-longer be loaded by the Python module, resulting in a skipped test, even though pycapnp is installed. i.e:
```bash
stderr:
Traceback (most recent call last):
File "/root/ci_scratch/build/test/functional/interface_ipc.py", line 20, in <module>
import capnp # type: ignore[import] # noqa: F401
^^^^^^^^^^^^
File "/usr/local/lib64/python3.14/site-packages/capnp/__init__.py", line 36, in <module>
from .version import version as __version__
File "/usr/local/lib64/python3.14/site-packages/capnp/version.py", line 1, in <module>
from .lib.capnp import _CAPNP_VERSION_MAJOR as LIBCAPNP_VERSION_MAJOR # noqa: F401
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ImportError: libcapnpc.so.1.0.1: cannot open shared object file: No such file or directory
```
Failing in this way should make it clear that `pycapnp` needs to be reinstalled/rebuilt.
If `pycapnp` is not installed, the test still skips as expected:
```bash
Remaining jobs: [interface_ipc.py]
1/1 - interface_ipc.py skipped (capnp module not available.)
TEST | STATUS | DURATION
interface_ipc.py | ○ Skipped | 0 s
```
Fixes: #34016.
ACKs for top commit:
maflcko:
lgtm ACK 905dfdee86d679f8ea31d841bceb77a5724a6b1b
hebasto:
ACK 905dfdee86d679f8ea31d841bceb77a5724a6b1b, I have reviewed the code and it looks OK. However, I'm [not able](https://github.com/bitcoin/bitcoin/issues/34016#issuecomment-3799532047) to reproduce https://github.com/bitcoin/bitcoin/issues/34016.
sedited:
ACK 905dfdee86d679f8ea31d841bceb77a5724a6b1b
Tree-SHA512: 3cedbe8fc51cc18f1c993f7747d20905f3bf94c736db99a9c4090f5823bf8c09dfbc19ef03c573d504dcdfba6ea0f7d088a7f4563b220742c9a441167c04cfd6
fa578d9434fdb090d27c7b5598dcd7f0ff0965cc lint: [move-only] Move python related lints to lint_py.rs (MarcoFalke)
fa392c31e7b9a487b61c881b06bb1e3787dcf4d9 lint: [move-only] Move repo related lints to lint_repo_hygiene.rs (MarcoFalke)
fab0cfa987c916a17c9cdd57636798866a56d74d lint: [move-only] Move cpp related lints to lint_cpp.rs (MarcoFalke)
fa3e48e3fd4d01dd8453994d214c24297aa83f62 lint: [move-only] Move docs related lints to lint_docs.rs (MarcoFalke)
fad09e77dbe521eb35206cd72536619225b18550 lint: [move-only] Move text related lints to text_format.rs (MarcoFalke)
faf40c2f848d7b5542862a4fd10b41cc6d418a90 lint: [move-only] Move util functions to util.rs (MarcoFalke)
Pull request description:
The single, large `main.rs` file is fine, but at some point it becomes harder to read.
So reduce the size by pulling functions out into modules.
This can be reviewed with the git option: `--color-moved=dimmed-zebra`
ACKs for top commit:
l0rinc:
Lightly tested code review ACK fa578d9434fdb090d27c7b5598dcd7f0ff0965cc
sedited:
ACK fa578d9434fdb090d27c7b5598dcd7f0ff0965cc
Tree-SHA512: f1e29fd3cf695fb6634d0b9f9e55508992b4b9885afee9dbe4d5d9e99cad3061e7141f39acbfe69d698422888169128cd7658a6dc991fd904b8520328b51586d
fad7bd9ba3eef03fcdd7cb17011ea0c6e483c767 noui: Remove always empty caption while formatting (MarcoFalke)
fa8ebeb332325604e8ca6080262543e10de4e46c refactor: [gui] Document that the title is always empty for node message (MarcoFalke)
fafe71b743a0637d16812d26430d99464cab0cee refactor: Remove empty caption from ThreadSafeMessageBox (MarcoFalke)
fa8d0088e76d4def59dff92bfb2ebbfc6cd4c195 refactor: Remove empty caption from ThreadSafeQuestion (MarcoFalke)
fa0195499ca611b513d9d1986d79c5e3a58cd0f2 refactor: [gui] Use lambdas over std::bind (MarcoFalke)
eeee1e341fa59b5b0b05f974105104fb2a0df9c3 refactor: Remove trailing semicolon after ADD_SIGNALS_DECL_WRAPPER (MarcoFalke)
Pull request description:
Currently, the user interface (noui, gui) has a caption for each message. However, the caption has many issues:
* It is always hard-coded to the empty string.
* This is confusing and tedious when reading or maintaining the code.
* It is redundant, because `noui` will ignore the caption and set the logging prefix (error, warning, info) based on the `style`.
* The gui does prefer to set the title based on the caption, but since it the caption is always empty, the fallback will always be used.
Fix all issues by removing it.
ACKs for top commit:
hebasto:
ACK fad7bd9ba3eef03fcdd7cb17011ea0c6e483c767, I have reviewed the code and it looks OK. Tested on Ubuntu 25.10.
sedited:
ACK fad7bd9ba3eef03fcdd7cb17011ea0c6e483c767
Tree-SHA512: 58ef538b9b3e1cfdcf2955f6de9b8cee335edbf6339723cb693cb4d584817904c962dac5199ee44d7e2860a5332dec1a6abf47e621eb5cf919aa1cdae271b55f
5aeaa71c77ac31a1b05f8361356d2810cfc5bc28 lint: pass args from lint.py to cargo run in container (will)
c17a2adb8dc0e9706319ddecc42737be4268ac6a lint: upgrade lint scripts for worktrees (will)
Pull request description:
Fixes#29972
Use a single script to run the linter locally or in CI.
Works from inside a worktree.
ACKs for top commit:
maflcko:
review ACK 5aeaa71c77ac31a1b05f8361356d2810cfc5bc28 🔒
davidgumberg:
code review and lightly tested reACK 5aeaa71
l0rinc:
Tested (+ lightly reviewed) ACK 5aeaa71c77ac31a1b05f8361356d2810cfc5bc28
Tree-SHA512: 7c11f649b4752739d31c4f9e6306a98bd2e615b27a0819bbb5e7d9284b9e28bd9f424e145f16361f672f1a63441a1ae2f901c4f99759e997b72a4bf2d56d8d39
14f99cfe53f07280b6f047844fc4fba0da8cd328 rpc: make `uptime` monotonic across NTP jumps (Lőrinc)
a9440b1595be7053b17895f7ee36652bac24be6e util: add `TicksSeconds` (Lőrinc)
Pull request description:
### Problem
`bitcoin-cli uptime` was derived from wall-clock time, so it could jump by large amounts when the system clock is corrected after `bitcoind` starts (e.g. on RTC-less systems syncing NTP).
This breaks the expectation that uptime reflects process runtime.
### Fix
Compute uptime from a [monotonic clock](https://en.cppreference.com/w/cpp/chrono/steady_clock.html) so it is immune to wall-clock jumps, and use that monotonic uptime for the RPC.
GUI startup time is derived from wall clock time minus monotonic uptime so it remains sensible after clock corrections.
### Reproducer
Revert the fix commit and run the `rpc_uptime` functional test (it should fail with `AssertionError: uptime should not jump with wall clock`):
Or alternatively:
```bash
cmake -B build && cmake --build build --target bitcoind bitcoin-cli -j$(nproc)
DATA_DIR=$(mktemp -d)
./build/bin/bitcoind -regtest -datadir="$DATA_DIR" -connect=0 -daemon
./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" -rpcwait uptime
sleep 1
./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" setmocktime $(( $(date +%s) + 20000000 ))
./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" uptime
./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" stop
```
<details>
<summary>Before (uptime jumps with wall clock)</summary>
```bash
Bitcoin Core starting
0
20000001
Bitcoin Core stopping
```
</details>
<details>
<summary>After (uptime stays monotonic)</summary>
```bash
Bitcoin Core starting
0
1
Bitcoin Core stopping
```
</details>
----------
Issue: https://github.com/bitcoin/bitcoin/issues/34326
ACKs for top commit:
maflcko:
review ACK 14f99cfe53f07280b6f047844fc4fba0da8cd328 🎦
willcl-ark:
tACK 14f99cfe53f07280b6f047844fc4fba0da8cd328
w0xlt:
ACK 14f99cfe53f07280b6f047844fc4fba0da8cd328
sedited:
ACK 14f99cfe53f07280b6f047844fc4fba0da8cd328
Tree-SHA512: 3909973f58666ffa0b784a6df087031b9e34d2022d354900a4dbb6cbe1d36285cd92770ee71350ebf64d6e8ab212d8ff0cd851f7dca1ec46ee2f19b417f53984
Add a ci/lint.py script to run the linter both locally or inside the CI
(replacing .github/ci-lint-exec.py) which supports running from a
worktree.
Determines whether we are in a worktree, and mounts the real `.git`
directory as a read-only volume if we are.
3f5211cba8e73e8eb03781e6ec32ba9c4a263782 test: remove child_one/child_two (w)txid variables (naiyoma)
7cfe790820cf247e8a27bb8091defc54c74d6aec test: replace ValidWitnessMalleatedTx class with function (naiyoma)
81675a781f3ab62a0576a9739d13b4997b63230d test: use pre-generated chain (naiyoma)
Pull request description:
This PR refactors ` ValidWitnessMalleatedTx` class into a `build_malleated_tx_package` function. As a result, two tests are updated: `mempool_accept_wtxid` and `p2p_p2p_private_broadcast`. Also included are a few small refactors in mempool_accept_wtxid , (switching to MiniWallet, using a pre-mined chain, using txid directly.)
Together, these changes reduce complexity and improve test runtime.
ACKs for top commit:
stratospher:
reACK 3f5211c.
cedwies:
reACK 3f5211c
maflcko:
review ACK 3f5211cba8e73e8eb03781e6ec32ba9c4a263782 👥
rkrux:
ACK 3f5211cba8e73e8eb03781e6ec32ba9c4a263782
Tree-SHA512: 1fd02be3432fef6b68e54fbe8b15ed56d2699580bb13d0777b21f9cbe4c6d33bbb710541e3ca2fc93eab771d17bf1c427e4b08fa216d561bdb320cc6b36ac8fc
Change this so we catch the case where the capnp shared libs have been
updated, and can no-longer be loaded by the Python module, resulting in
a skipped test, even though pycapnp is installed. i.e:
```bash
stderr:
Traceback (most recent call last):
File "/root/ci_scratch/build/test/functional/interface_ipc.py", line 20, in <module>
import capnp # type: ignore[import] # noqa: F401
^^^^^^^^^^^^
File "/usr/local/lib64/python3.14/site-packages/capnp/__init__.py", line 36, in <module>
from .version import version as __version__
File "/usr/local/lib64/python3.14/site-packages/capnp/version.py", line 1, in <module>
from .lib.capnp import _CAPNP_VERSION_MAJOR as LIBCAPNP_VERSION_MAJOR # noqa: F401
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ImportError: libcapnpc.so.1.0.1: cannot open shared object file: No such file or directory
```
Failing in this way should make it clear that `pycapnp` needs to be
reinstalled/rebuilt.
If `pycapnp` is not installed, the test still skips as expected:
```bash
Remaining jobs: [interface_ipc.py]
1/1 - interface_ipc.py skipped (capnp module not available.)
TEST | STATUS | DURATION
interface_ipc.py | ○ Skipped | 0 s
```
Fixes: #34016.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
removeprunedfunds removes all entries from mapTxSpends for the
inputs of the pruned tx. However, this is incorrect, because there could be
multiple entries from conflicting transactions (that shouldn't be
removed as well). This could lead to the wallet creating invalid
transactions, trying to double spend utxos.
The bug persists when the conflicting tx was mined, because
the wallet trusts its internal accounting instead of calling
AddToSpends again.
Add test coverage to ensure peers without the snapshot block in their chain can be used for block downloads after background validation completes. The test fails without the fix in the previous commit.
This prevents potential intermittend failures on windows when the wallet by the same name from the previous test case hasn't been cleaned up yet by it's process.
0aba464ce76522f1be3bb9e471b45438738de492 test: switch order of error code and message check (rkrux)
Pull request description:
I feel it'd be easier to debug intermittent test failures if the error message is present in the logs instead of error code. So, switching order of error code and message in the `try_rpc` function to aid error debugging.
Should help in debugging #34354 IMO. It's an intermittent failure on Windows that I can't reproduce and it's more difficult to figure out what could have gone wrong only by seeing the error code like below in the CI logs. Given that the functional tests pass, I don't see a harm in checking for error message first and throwing it in case of a mismatch.
```python
AssertionError: Unexpected JSONRPC error code -1
```
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improve coverage are always welcome.
* All other changes should have accompanying unit tests (see `src/test/`) or
functional tests (see `test/`). Contributors should note which tests cover
modified code. If no tests exist for a region of modified code, new tests
should accompany the change.
* Bug fixes are most welcome when they come with steps to reproduce or an
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
* Features are welcome, but might be rejected due to design or scope issues.
If a feature is based on a lot of dependencies, contributors should first
consider building the system outside of Bitcoin Core, if possible.
* Refactoring changes are only accepted if they are required for a feature or
bug fix or otherwise improve developer experience significantly. For example,
most "code style" refactoring changes require a thorough explanation why they
are useful, what downsides they have and why they *significantly* improve
developer experience or avoid serious programming bugs. Note that code style
is often a subjective matter. Unless they are explicitly mentioned to be
preferred in the [developer notes](/doc/developer-notes.md), stylistic code
changes are usually rejected.
-->
<!--
Bitcoin Core has a thorough review process and even the most trivial change
needs to pass a lot of eyes and requires non-zero or even substantial time
effort to review. There is a huge lack of active reviewers on the project, so
patches often sit for a long time.
-->
ACKs for top commit:
maflcko:
lgtm ACK 0aba464ce76522f1be3bb9e471b45438738de492
polespinasa:
lgtm ACK 0aba464ce76522f1be3bb9e471b45438738de492
fjahr:
utACK 0aba464ce76522f1be3bb9e471b45438738de492
brunoerg:
code review ACK 0aba464ce76522f1be3bb9e471b45438738de492
sedited:
ACK 0aba464ce76522f1be3bb9e471b45438738de492
Tree-SHA512: b09ba4b5d13a2c93a4a28a5c1b06af44a91295974236bb8326b74a988878c431e9ce0e19ec14bb98ac2b002da877abaa7da6a9851424453bcb494c0317b57227
75b704df9d5ccb262d19ffe95b7ca6d1a934f3fb wallettool: Disallow creating new unnamed wallets (Ava Chow)
5875a9c502632eb5c74df07e41af38582da6e884 wallet: disallow unnamed wallets in createwallet and restorewallet (Ava Chow)
d30ad4a9129da04e249e3938f643dc47bf060e7e wallet, rpc: Use HandleWalletError in createwallet (Ava Chow)
Pull request description:
We've been moving in the direction that all wallets must have a name. Therefore, we shouldn't allow creating new unnamed wallets. `createwallet`, `restorewallet`, and the wallet tool's `create` and `createfromdump` all now require the user to provide a non-empty wallet name when creating/restoring a wallet.
The GUI is already enforcing this, but we were not enforcing it for RPCs or in the underlying `CreateWallet` and `RestoreWallet` functions.
Wallet migration does still need to be able to restore unnamed wallets, so there is a new argument to `RestoreWallet` to explicitly allow that behavior for migration only.
ACKs for top commit:
rkrux:
lgtm ACK 75b704df9d5ccb262d19ffe95b7ca6d1a934f3fb
polespinasa:
re ACK 75b704df9d5ccb262d19ffe95b7ca6d1a934f3fb
Tree-SHA512: 8bde76d0b091e9276788c69412934af3426da2a7a69a00f94072d36c1a075cd41744ecdd5fef2b72870c1351b76aae061f124f716bb23f4839be20c464fc5ebd
The previous commit added set_ephemeral_port_range() to
avoid port conflicts on FreeBSD by requesting ports from the high
ephemeral range (49152-65535) instead of the default range
which overlaps with the test framework's static port range.
That fix was applied to the SOCKS5 server but not to P2P listeners
created via NetworkThread.create_listen_server(). This commit extends
the fix to cover P2P listeners as well.
When port=0 is requested (dynamic allocation), we now:
1. Manually create a socket with the appropriate address family
2. Call set_ephemeral_port_range() to configure the port range
3. Bind and listen on the socket
4. Pass the pre-configured socket to asyncio's create_server()
This ensures that dynamically allocated ports for P2P listeners also
come from the high range on FreeBSD, avoiding conflicts with the test
framework's static port assignments.
Co-Authored-By: Vasil Dimov <vd@FreeBSD.org>
I feel it'd be easier to debug intermittent test failures if the
error message is present in the logs instead of error code. So,
switching order of error code and message in the `try_rpc` function
to aid error debugging.
Simplify the witness malleation test helper by converting the
ValidWitnessMalleatedTx class to a standalone function
build_malleated_tx_package() and updating call sites.
Co-authored-by: rkrux <rkrux.connect@gmail.com>
9c7e4771b13d4729fd20ea08b7e2e3209b134fff test: Test listdescs with priv works even with missing priv keys (Novo)
ed945a685473712c1a822379effa42fd49223515 walletrpc: reject listdes with priv key on w-only wallets (Novo)
9e5e9824f11b1b0f9e2a4e28124edbb1616af519 descriptor: ToPrivateString() pass if at least 1 priv key exists (Novo)
5c4db25b61d417a567f152169f4ab21a491afb95 descriptor: refactor ToPrivateString for providers (Novo)
2dc74e3f4e5e6f01c8810359b91041bc6865f1c7 wallet/migration: use HavePrivateKeys in place of ToPrivateString (Novo)
e842eb90bb6db39076a43b010c0c7898d50b8d92 descriptors: add HavePrivateKeys() (Novo)
Pull request description:
_TLDR:
Currently, `listdescriptors [private=true]` will fail for a non-watch-only wallet if any descriptor has a missing private key(e.g `tr()`, `multi()`, etc.). This PR changes that while making sure `listdescriptors [private=true]` still fails if there no private keys. Closes #32078_
In non-watch-only wallets, it's possible to import descriptors as long as at least one private key is included. It's important that users can still view these descriptors when they need to create a backup—even if some private keys are missing ([#32078 (comment)](https://github.com/bitcoin/bitcoin/issues/32078#issuecomment-2781428475)). This change makes it possible to do so.
This change also helps prevent `listdescriptors true` from failing completely, because one descriptor is missing some private keys.
### Notes
- The new behaviour is applied to all descriptors including miniscript descriptors
- `listdescriptors true` still fails for watch-only wallets to preserve existing behaviour https://github.com/bitcoin/bitcoin/pull/24361#discussion_r920801352
- Wallet migration logic previously used `Descriptor::ToPrivateString()` to determine which descriptor was watchonly. This means that modifying the `ToPrivateString()` behaviour caused descriptors that were previously recognized as "watchonly" to be "non-watchonly". **In order to keep the scope of this PR limited to the RPC behaviour, this PR uses a different method to determine `watchonly` descriptors for the purpose of wallet migration.** A follow-up PR can be opened to update migration logic to exclude descriptors with some private keys from the `watchonly` migration wallet.
### Relevant PRs
https://github.com/bitcoin/bitcoin/pull/24361https://github.com/bitcoin/bitcoin/pull/32186
### Testing
Functional tests were added to test the new behaviour
EDIT
**`listdescriptors [private=true]` will still fail when there are no private keys because non-watchonly wallets must have private keys and calling `listdescriptors [private=true]` for watchonly wallet returns an error**
ACKs for top commit:
Sjors:
ACK 9c7e4771b13d4729fd20ea08b7e2e3209b134fff
achow101:
ACK 9c7e4771b13d4729fd20ea08b7e2e3209b134fff
w0xlt:
reACK 9c7e4771b1 with minor nits
rkrux:
re-ACK 9c7e4771b13d4729fd20ea08b7e2e3209b134fff
Tree-SHA512: f9b3b2c3e5425a26e158882e39e82e15b7cb13ffbfb6a5fa2868c79526e9b178fcc3cd88d3e2e286f64819d041f687353780bbcf5a355c63a136fb8179698b60
On FreeBSD, the default ephemeral port range (10000-65535) overlaps
with the test framework's static port range (11000-26000), causing
intermittent "address already in use" failures when tests use dynamic
port allocation (port=0).
Add a helper function that sets the IP_PORTRANGE/IPV6_PORTRANGE socket
option to IP_PORTRANGE_HIGH before binding, which requests ports from
the high range (49152-65535) instead. This range does not overlap with
the test framework's static ports.
Constants from FreeBSD's netinet/in.h and netinet6/in6.h:
- IP_PORTRANGE = 19 (for IPv4 sockets)
- IPV6_PORTRANGE = 14 (for IPv6 sockets)
- IP_PORTRANGE_HIGH = 1
Fixes: bitcoin/bitcoin#34331
Co-Authored-By: Vasil Dimov <vd@FreeBSD.org>
Co-Authored-By: MarcoFalke <*~=\`'#}+{/-|&$^_@721217.xyz>
a7b581423e44c51fb7d177c5a15fe2cc2ab8aa43 Fix 11-year-old mis-categorized error code in OP_IF evaluation (Calin Culianu)
Pull request description:
This was introduced by commit ab9edbd6b6eb3efbca11f16fa467c3c0ef905708.
It appears the original author may have gotten tired and pasted the wrong error code into this 1 place. Every other situation where the value stack lacks the required number of arguments for the op-code, SCRIPT_ERR_INVALID_STACK_OPERATION is reported. Not so here.
This commit fixes the situation.
EDIT: Note this turns out to be a dupe of the abandoned #30359 .
ACKs for top commit:
billymcbip:
tACK a7b581423e44c51fb7d177c5a15fe2cc2ab8aa43
achow101:
ACK a7b581423e44c51fb7d177c5a15fe2cc2ab8aa43
darosior:
utACK a7b581423e44c51fb7d177c5a15fe2cc2ab8aa43
sedited:
ACK a7b581423e44c51fb7d177c5a15fe2cc2ab8aa43
Tree-SHA512: e8c01a3e2448b5d49b76a0cab3f38a2d0249b71beeb7d9d05d5ecc3812bd91d0bd1d0f78b809b6f4ccb73186fa119cb1ed3779a73284b83a67ae219ef378fa6c
Migration still needs to be able to restore unnamed wallets, so
allow_unnamed is added to RestoreWallet to explicitly allow that
behavior for migration only.
Compute `uptime` from `SteadyClock` so it is unaffected by system time changes after startup.
Derive GUI startup time by subtracting the monotonic uptime from the wall clock time.
Add a functional test covering a large `setmocktime` jump.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
979d41bfab248990d7d520873d17fe52daa8d402 qa: Fix Windows logging bug (Hennadii Stepanov)
Pull request description:
The regex `(.*)` was capturing `\r` from subprocess output on Windows, causing the closing parenthesis in logs to wrap to the next line.
For [example](https://github.com/hebasto/bitcoin/actions/runs/20993438084/job/60350204808):
```
208/454 - feature_bip68_sequence.py passed, Duration: 10 s
209/454 - rpc_bind.py --ipv4 skipped (not on a Linux system
)
210/454 - rpc_bind.py --ipv6 skipped (not on a Linux system
)
211/454 - rpc_packages.py passed, Duration: 8 s
212/454 - rpc_bind.py --nonloopback skipped (not on a Linux system
)
213/454 - p2p_feefilter.py passed, Duration: 4 s
```
Stripping whitespace from the regex match fixes the formatting. [See](https://github.com/hebasto/bitcoin/actions/runs/20993564177/job/60350024373):
```
208/454 - feature_bip68_sequence.py passed, Duration: 9 s
209/454 - rpc_bind.py --ipv4 skipped (not on a Linux system)
210/454 - rpc_bind.py --ipv6 skipped (not on a Linux system)
211/454 - rpc_bind.py --nonloopback skipped (not on a Linux system)
212/454 - rpc_packages.py passed, Duration: 7 s
```
ACKs for top commit:
maflcko:
lgtm ACK 979d41bfab248990d7d520873d17fe52daa8d402
l0rinc:
lightly tested ACK 979d41bfab248990d7d520873d17fe52daa8d402
Tree-SHA512: bafe1937a519e45e4cab395bae622acf65220f313c773a0729ba7dccc3a0a048602f1c04b3e8cdd80d2cf68ae36cef802a819530485d5a745db8abcadf141f68
3e340672ecadb2a32b19574a99a5162806af645e test: use ephemeral ports in p2p_private_broadcast.py (w0xlt)
Pull request description:
The test `p2p_private_broadcast.py` gets some Python P2P nodes to listen and instructs the SOCKS5 proxy to redirect connections to them instead of to the requested addresses. This way the `bitcoind` which uses the proxy is tricked to think it has connected to real routable internet IP addresses or `.onion` addresses.
Picking the ports where to Python P2P nodes to listen however is tricky to be done in a non-conflicting way, given that other tests may run in parallel. https://github.com/bitcoin/bitcoin/pull/34186 made it possible to let the OS select a free port, so use that in
`p2p_private_broadcast.py`.
---
_Suggested in https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2654849875_
ACKs for top commit:
l0rinc:
code review ACK 3e340672ecadb2a32b19574a99a5162806af645e
polespinasa:
tACK 3e340672ecadb2a32b19574a99a5162806af645e
mzumsande:
utACK 3e340672ecadb2a32b19574a99a5162806af645e
Tree-SHA512: e94efd33a1845e1767aaada55f91c60bc5fc1166c281ef578a391e95e2791a922d84aa6ed1ce06e7d6ca1a65f84da52fd79d9b2f40705c1944a53c67b7392e4d
d09a19fd41cb71a5d1c10297763e72bc551a7d3a test: add coverage for issue 34206 (Greg Sanders)
4c7cfd37ad9517334a0848e6778243ddef1843f4 wallet: remove erroneous-on-reorg Assume() (Greg Sanders)
Pull request description:
Resolves https://github.com/bitcoin/bitcoin/issues/34206
I'm not certain the test is worth keeping, but included it for now to show minimal example that crashes without fix. Can be removed.
ACKs for top commit:
bensig:
ACK d09a19fd41cb71a5d1c10297763e72bc551a7d3a
dergoegge:
utACK d09a19fd41cb71a5d1c10297763e72bc551a7d3a
Tree-SHA512: 7eac19e97be6db8e38af396c406066fdcec532332e685a38bb33f0a988701c7bd5a0967f51426737fd56972847b761a3d873495928ff66efa8512fb267a9622b
The test `p2p_private_broadcast.py` gets some Python P2P nodes to listen
and instructs the SOCKS5 proxy to redirect connections to them instead
of to the requested addresses. This way the `bitcoind` which uses the
proxy is tricked to think it has connected to real routable internet
IP addresses or `.onion` addresses.
Picking the ports where to Python P2P nodes to listen however is tricky
to be done in a non-conflicting way, given that other tests may run in
parallel. https://github.com/bitcoin/bitcoin/pull/34186 made it possible
to let the OS select a free port, so use that in
`p2p_private_broadcast.py`.
ce63d37ebee8d062310a778c5efa6475814188e9 test: use dynamic port allocation to avoid test conflicts (woltx)
Pull request description:
Use `port=0` for dynamic port allocation in test framework components to avoid intermittent "address already in use" errors when running tests concurrently or when ports are stuck in TIME_WAIT state. Example: https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2634509304
Changes:
- Update `socks5.py` and `p2p.py` to support dynamic port allocation
- Convert `feature_proxy.py` and `feature_anchors.py` to use `port=0`
ACKs for top commit:
achow101:
ACK ce63d37ebee8d062310a778c5efa6475814188e9
vasild:
ACK ce63d37ebee8d062310a778c5efa6475814188e9
mzumsande:
re-ACK ce63d37ebee8d062310a778c5efa6475814188e9
Tree-SHA512: 4efcedca3bde209fbd1bdc2a4ae04b7d53515587d86e421ce61064f78c675c71b45d9782b514c5e7cfc0e92842c947d49f7a3fddb03fe619fcdec9b565f0ecbd
7b5d256af4a0f954a919604ed4346db3a814fb6d test: Add bitcoin-chainstate test for assumeutxo functionality (stringintech)
2bc32656498517fe58bd41dcbd0afd306d51d4b0 Fix `ChainstateManager::AddChainstate()` assertion crash (stringintech)
5f3d6bdb6659dba16941e6d6a05fd883d3f49a9d Add regtest support to bitcoin-chainstate tool (stringintech)
Pull request description:
This PR adds functional test coverage for the bitcoin-chainstate tool loading a datadir initialized with an assumeutxo snapshot.
The PR also includes:
- Fix for assertion crash in `ChainstateManager::AddChainstate()` when `prev_chainstate` has no initialized mempool (required for the test to pass)
- `-regtest` flag support for bitcoin-chainstate to enable the testing
This work started while experimenting with the bitcoin-chainstate tool and how the kernel API (#30595) behaved when loading a datadir containing assumeutxo data, during the time that PR was still under review. sedited suggested opening a PR to add this test coverage.
ACKs for top commit:
achow101:
ACK 7b5d256af4a0f954a919604ed4346db3a814fb6d
theStack:
Concept and code-review ACK 7b5d256af4a0f954a919604ed4346db3a814fb6d
sedited:
Re-ACK 7b5d256af4a0f954a919604ed4346db3a814fb6d
Tree-SHA512: 5d3b0050cf2d53144b5f65451c991d5e212117b4541ae1368ecf58fde5f3cca4f018aad6ae32257b9ebb1c28b926424fbcff496ba5487cdc4eb456cea6db8b24
792e2edf57ab31ae5c6f98acf33af8f67506630f p2p: first addr self-announcement in separate msg (0xb10c)
Pull request description:
This makes sure the initial address self-announcement a node sends to a peer happends in a separate P2P message. This has benefits for both inbound and outbound connections:
For inbound connections from a peer to us, previously, we might send the self-announcement along with our response to a GETADDR request. However, the self-announcement might replace an address from the GETADDR response. This isn't clean.
For outbound connections from us to a peer, previously, it could have happend that we send the self-announcement along with other addresses. Since shortly after connection open, the peer might only have one rate-limiting token for us, and the addresses are shuffeld on arrival, it's possible that the self-announcement gets rate-limited. However, note that these rate-limitings seem to be rare in practice.
This is inspired by and based on https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3462287763. The discussion there should be helpful for reviewers.
ACKs for top commit:
bensig:
ACK 792e2edf57ab31ae5c6f98acf33af8f67506630f
achow101:
ACK 792e2edf57ab31ae5c6f98acf33af8f67506630f
fjahr:
Code review ACK 792e2edf57ab31ae5c6f98acf33af8f67506630f
frankomosh:
Code Review ACK [792e2ed](792e2edf57)
Tree-SHA512: e3d39b1e3ae6208b54df4b36c624a32d70a442e01681f49e0c8a65076a818b5bf203c2e51011dc32edbbe3637b3c0b5f18de26e3461c288aa3806646a209a260
8fb5e5f41ddf550a78b1253184d79a107097815a test: check wallet rescan properly in feature_pruning (brunoerg)
9b57c8d2bd15a414e08a9e43367d8d3d82c25fe4 test: fix feature_pruning when built without wallet (brunoerg)
Pull request description:
Fixes#34175
In `feature_pruning`, the`wallet_test` doesn't require any specific wallet functionality and this test is important for one of next ones (`test_scanblocks_pruned`). The reason is that it synchronizes the node 5 and, without this sync, `test_scanblocks_pruned` will fail since we expect `scanblocks` to fail due to `Block not available (pruned data)` and it doesn't happen without this sync.
ACKs for top commit:
achow101:
ACK 8fb5e5f41ddf550a78b1253184d79a107097815a
furszy:
utACK 8fb5e5f41ddf550a78b1253184d79a107097815a
musaHaruna:
Tested ACK [8fb5e5f](8fb5e5f41d)
w0xlt:
ACK 8fb5e5f41d
Tree-SHA512: 812afbf4343a7493e2169eb6735fce25692d5cb19972abafc772b8c05a64b9c7027f6675cd084f345977e916e62a722d671f90831bbdc51683e0cd253fa482f0
The regex `(.*)` was capturing `\r` from subprocess output on Windows,
causing the closing parenthesis in logs to wrap to the next line.
Stripping whitespace from the regex match fixes the formatting.
ab41492c6ba7d3d68b53bf4299642a4f848a429f test: Prevent loop from running out of utxos in bip68 test (Fabian Jahr)
Pull request description:
This tries to fix#34205
I stared at the test code quite a bit and initially suspected some `MiniWallet` internals to be the issue but I think that was the wrong direction and there is simply a very small chance that the loop in `test_sequence_lock_confirmed_inputs` runs out of available utxos: We are starting out with 200-250 utxos and run the loop 400 times. If a transaction is accepted it could have up to 10 inputs but it always has only one output, so the pool is depleting in this case. And it's actually even worse because the output produced is not recognized as spendable by the `MiniWallet` because it is not using the correct output script. However, only a small fraction of transactions are actually accepted, which is why this issue almost never occurs. I did some extra printing and usually we end up with >100 utxos still available by the end of the test. But there is a small chance that too many transactions are accepted and then we can run out of utxos.
I considered two fixes: The first was a break at the beginning of the loop `if available_utxos == 0: break`, this would work fine but I went with the second option: Simply creating the output with the correct output script so that `MiniWallet` recognizes it as spendable. This minimal replentishment of available utxos ensures that at worst we should get a few 1 input, 1 ouput transactions by the end but we should never run out of available utxos. I didn't look back in history but I suspect that this is how it was intended before `MiniWallet` introduced.
Also moves the `random` import in the same function to the top of the file.
ACKs for top commit:
maflcko:
lgtm ACK ab41492c6ba7d3d68b53bf4299642a4f848a429f
bensig:
ACK ab41492c6ba7d3d68b53bf4299642a4f848a429f
darosior:
ACK ab41492c6ba7d3d68b53bf4299642a4f848a429f
Tree-SHA512: d3ce56b669d011257a4a6967923f56011dbd03362576f564b29464639391851a09113f84b5ca2902911be7aa0923ccc9f402d13e6d673fd089dfe2b1f113ae4d
