The DataStream comment was a bit stale, because it was using
CDataStream.
Fix it by using assert_debug_log for a self-documenting and
self-checking test code.
552bc82b17 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 552bc82b17
achow101:
ACK 552bc82b17
rkrux:
lgtm tACK 552bc82
Tree-SHA512: cc99271a3955daa475242d9f4ef8f09f4c94c64e48ec4647ecfd95dceb38bb0cdd91b78ec2d5f033b449d175eaecbdda49d6c766c8a1e1a01fed93be4eb0cfc0
6f7b4323cb test: remove UNKNOWN_ERROR from script_tests (Bruno Garcia)
bd31a92d67 script: use SCRIPT_ERR_SCRIPTNUM for CScriptNum errors (Bruno Garcia)
0ca4dcd786 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 6f7b4323cb
w0xlt:
ACK 6f7b4323cb
darosior:
ACK 6f7b4323cb
Tree-SHA512: e656d9992251fbc95d33966fa18ce64bf714179d51ba6a7f429e5a55bc58e7fc08827e4ab71ace0dd385dac7e1feaea621b49503387793a30eae7a7e44aa6b0f
fafdae46ff 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 fafdae46ff
sedited:
ACK fafdae46ff
Tree-SHA512: 157f434c2faa16243890b2344c4ee36bc359e56c80ba8a04f0bba71e9760cf9106c38ed755ff57eff8d1957f35516d20b3d010e0ecb8633b845f5314cc0d050a
7d9e1a8102 test: Verify peer usage after assumeutxo validation completes (stringintech)
0067abe153 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 7d9e1a8102
achow101:
ACK 7d9e1a8102
sedited:
Re-ACK 7d9e1a8102
Tree-SHA512: 5515971da7bf7efc55eecdf03686f44c20c9e52dd168e7cfa119032d6a8ebccee69df7143075e4e9d0a01426cd9ae7202dce5c00919a82478ebf49a15dc0fe19
c6ca2b85a3 validation: do not wipe utxo cache for stats/scans/snapshots (Pieter Wuille)
7099e93d0a 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 c6ca2b85a3
cedwies:
reACK c6ca2b8 (trivial)
achow101:
ACK c6ca2b85a3
sedited:
ACK c6ca2b85a3
Tree-SHA512: f3525a85dc512db4a0a9c749ad47c0d3fa44085a121aa54cd77646260a719c71f754ec6570ae77779c0ed68a24799116f79c686e7a17ce57a26f6a598f7bf926
2845f10a2b test: extend FreeBSD ephemeral port range fix to P2P listeners (node)
34bed0ed8c 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 2845f10a2b
hebasto:
ACK 2845f10a2b, I have reviewed the code and it looks OK.
Tree-SHA512: ce501ce3e8a4023e07bad572df2b85d6829becf133813e4529aebba83e4eba59fa8b48e9d2197ebbb226adaf3054fad720775a787244d6b38c0078ee086102f4
1f60ca360e 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 1f60ca360e
fjahr:
tACK 1f60ca360e
furszy:
utACK 1f60ca360e
vasild:
ACK 1f60ca360e
Tree-SHA512: 3cc9ed547530fd53e25721177b76ab2e1eae16ce2c0e63fc01b20fdbf8bd02655dae51167ad56f9dec748d34c61ce65d38f993370820601f8257c73b876a3347
905dfdee86 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 905dfdee86
hebasto:
ACK 905dfdee86, 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 905dfdee86
Tree-SHA512: 3cedbe8fc51cc18f1c993f7747d20905f3bf94c736db99a9c4090f5823bf8c09dfbc19ef03c573d504dcdfba6ea0f7d088a7f4563b220742c9a441167c04cfd6
fa578d9434 lint: [move-only] Move python related lints to lint_py.rs (MarcoFalke)
fa392c31e7 lint: [move-only] Move repo related lints to lint_repo_hygiene.rs (MarcoFalke)
fab0cfa987 lint: [move-only] Move cpp related lints to lint_cpp.rs (MarcoFalke)
fa3e48e3fd lint: [move-only] Move docs related lints to lint_docs.rs (MarcoFalke)
fad09e77db lint: [move-only] Move text related lints to text_format.rs (MarcoFalke)
faf40c2f84 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 fa578d9434
sedited:
ACK fa578d9434
Tree-SHA512: f1e29fd3cf695fb6634d0b9f9e55508992b4b9885afee9dbe4d5d9e99cad3061e7141f39acbfe69d698422888169128cd7658a6dc991fd904b8520328b51586d
fad7bd9ba3 noui: Remove always empty caption while formatting (MarcoFalke)
fa8ebeb332 refactor: [gui] Document that the title is always empty for node message (MarcoFalke)
fafe71b743 refactor: Remove empty caption from ThreadSafeMessageBox (MarcoFalke)
fa8d0088e7 refactor: Remove empty caption from ThreadSafeQuestion (MarcoFalke)
fa0195499c refactor: [gui] Use lambdas over std::bind (MarcoFalke)
eeee1e341f 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 fad7bd9ba3, I have reviewed the code and it looks OK. Tested on Ubuntu 25.10.
sedited:
ACK fad7bd9ba3
Tree-SHA512: 58ef538b9b3e1cfdcf2955f6de9b8cee335edbf6339723cb693cb4d584817904c962dac5199ee44d7e2860a5332dec1a6abf47e621eb5cf919aa1cdae271b55f
5aeaa71c77 lint: pass args from lint.py to cargo run in container (will)
c17a2adb8d 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 5aeaa71c77🔒
davidgumberg:
code review and lightly tested reACK 5aeaa71
l0rinc:
Tested (+ lightly reviewed) ACK 5aeaa71c77
Tree-SHA512: 7c11f649b4752739d31c4f9e6306a98bd2e615b27a0819bbb5e7d9284b9e28bd9f424e145f16361f672f1a63441a1ae2f901c4f99759e997b72a4bf2d56d8d39
14f99cfe53 rpc: make `uptime` monotonic across NTP jumps (Lőrinc)
a9440b1595 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 14f99cfe53🎦
willcl-ark:
tACK 14f99cfe53
w0xlt:
ACK 14f99cfe53
sedited:
ACK 14f99cfe53
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.
3f5211cba8 test: remove child_one/child_two (w)txid variables (naiyoma)
7cfe790820 test: replace ValidWitnessMalleatedTx class with function (naiyoma)
81675a781f 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 3f5211cba8👥
rkrux:
ACK 3f5211cba8
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.
0aba464ce7 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 0aba464ce7
polespinasa:
lgtm ACK 0aba464ce7
fjahr:
utACK 0aba464ce7
brunoerg:
code review ACK 0aba464ce7
sedited:
ACK 0aba464ce7
Tree-SHA512: b09ba4b5d13a2c93a4a28a5c1b06af44a91295974236bb8326b74a988878c431e9ce0e19ec14bb98ac2b002da877abaa7da6a9851424453bcb494c0317b57227
75b704df9d wallettool: Disallow creating new unnamed wallets (Ava Chow)
5875a9c502 wallet: disallow unnamed wallets in createwallet and restorewallet (Ava Chow)
d30ad4a912 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 75b704df9d
polespinasa:
re ACK 75b704df9d
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>
9c7e4771b1 test: Test listdescs with priv works even with missing priv keys (Novo)
ed945a6854 walletrpc: reject listdes with priv key on w-only wallets (Novo)
9e5e9824f1 descriptor: ToPrivateString() pass if at least 1 priv key exists (Novo)
5c4db25b61 descriptor: refactor ToPrivateString for providers (Novo)
2dc74e3f4e wallet/migration: use HavePrivateKeys in place of ToPrivateString (Novo)
e842eb90bb 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 9c7e4771b1
achow101:
ACK 9c7e4771b1
w0xlt:
reACK 9c7e4771b1 with minor nits
rkrux:
re-ACK 9c7e4771b1
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>
a7b581423e Fix 11-year-old mis-categorized error code in OP_IF evaluation (Calin Culianu)
Pull request description:
This was introduced by commit ab9edbd6b6.
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 a7b581423e
achow101:
ACK a7b581423e
darosior:
utACK a7b581423e
sedited:
ACK a7b581423e
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>
979d41bfab 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 979d41bfab
l0rinc:
lightly tested ACK 979d41bfab
Tree-SHA512: bafe1937a519e45e4cab395bae622acf65220f313c773a0729ba7dccc3a0a048602f1c04b3e8cdd80d2cf68ae36cef802a819530485d5a745db8abcadf141f68
3e340672ec 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 3e340672ec
polespinasa:
tACK 3e340672ec
mzumsande:
utACK 3e340672ec
Tree-SHA512: e94efd33a1845e1767aaada55f91c60bc5fc1166c281ef578a391e95e2791a922d84aa6ed1ce06e7d6ca1a65f84da52fd79d9b2f40705c1944a53c67b7392e4d
d09a19fd41 test: add coverage for issue 34206 (Greg Sanders)
4c7cfd37ad 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 d09a19fd41
dergoegge:
utACK d09a19fd41
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`.
ce63d37ebe 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 ce63d37ebe
vasild:
ACK ce63d37ebe
mzumsande:
re-ACK ce63d37ebe
Tree-SHA512: 4efcedca3bde209fbd1bdc2a4ae04b7d53515587d86e421ce61064f78c675c71b45d9782b514c5e7cfc0e92842c947d49f7a3fddb03fe619fcdec9b565f0ecbd
7b5d256af4 test: Add bitcoin-chainstate test for assumeutxo functionality (stringintech)
2bc3265649 Fix `ChainstateManager::AddChainstate()` assertion crash (stringintech)
5f3d6bdb66 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 7b5d256af4
theStack:
Concept and code-review ACK 7b5d256af4
sedited:
Re-ACK 7b5d256af4
Tree-SHA512: 5d3b0050cf2d53144b5f65451c991d5e212117b4541ae1368ecf58fde5f3cca4f018aad6ae32257b9ebb1c28b926424fbcff496ba5487cdc4eb456cea6db8b24
792e2edf57 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 792e2edf57
achow101:
ACK 792e2edf57
fjahr:
Code review ACK 792e2edf57
frankomosh:
Code Review ACK [792e2ed](792e2edf57)
Tree-SHA512: e3d39b1e3ae6208b54df4b36c624a32d70a442e01681f49e0c8a65076a818b5bf203c2e51011dc32edbbe3637b3c0b5f18de26e3461c288aa3806646a209a260
8fb5e5f41d test: check wallet rescan properly in feature_pruning (brunoerg)
9b57c8d2bd 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 8fb5e5f41d
furszy:
utACK 8fb5e5f41d
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.
