ac599c4a9cb3b2d424932d3fd91f9eed17426827 test: Test MuSig2 in the wallet (Ava Chow)
68ef954c4c59802a6810a462eaa8dd61728ba820 wallet: Keep secnonces in DescriptorScriptPubKeyMan (Ava Chow)
4a273edda0ec10f0c5ae5d94b9925fa334d1c6e6 sign: Create MuSig2 signatures for known MuSig2 aggregate keys (Ava Chow)
258db938899409c8ee1cef04e16ba1795ea0038d sign: Add CreateMuSig2AggregateSig (Ava Chow)
bf69442b3f5004dc3df5a1b1d752114ba68fa5f4 sign: Add CreateMuSig2PartialSig (Ava Chow)
512b17fc56eac3a2e2b9ba489b5423d098cce0db sign: Add CreateMuSig2Nonce (Ava Chow)
82ea67c607cde6187d7082429d27b927dc21c0c6 musig: Add MuSig2AggregatePubkeys variant that validates the aggregate (Ava Chow)
d99a081679e16668458512aba2fd13a3e1bdb09f psbt: MuSig2 data in Fill/FromSignatureData (Ava Chow)
4d8b4f53363f013ed3972997f0b05b9c19e9db9d signingprovider: Add musig2 secnonces (Ava Chow)
c06a1dc86ff2347538e95041ab7b97af25342958 Add MuSig2SecNonce class for secure allocation of musig nonces (Ava Chow)
9baff05e494443cd82708490f384aa3034ad43bd sign: Include taproot output key's KeyOriginInfo in sigdata (Ava Chow)
4b24bfeab9d6732aae3e69efd33105792ef1198f pubkey: Return tweaks from BIP32 derivation (Ava Chow)
f14876213aad0e67088b75cae24323db9f2576d8 musig: Move synthetic xpub construction to its own function (Ava Chow)
fb8720f1e09f4e41802f07be53fb220d6f6c127f sign: Refactor Schnorr sighash computation out of CreateSchnorrSig (Ava Chow)
a4cfddda644f1fc9a815b2d16c997716cd63554a tests: Clarify why musig derivation adds a pubkey and xpub (Ava Chow)
39a63bf2e7e38dd3f30b5d1a8f6b2fff0e380d12 descriptors: Add a doxygen comment for has_hardened output_parameter (Ava Chow)
2320184d0ea87279558a8e6cbb3bccf5ba1bb781 descriptors: Fix meaning of any_key_parsed (Ava Chow)
Pull request description:
This PR implements MuSig2 signing so that the wallet can receive and spend from imported `musig(0` descriptors.
The libsecp musig module is enabled so that it can be used for all of the MuSig2 cryptography.
Secnonces are handled in a separate class which holds the libsecp secnonce object in a `secure_unique_ptr`. Since secnonces must not be used, this class has no serialization and will only live in memory. A restart of the software will require a restart of the MuSig2 signing process.
ACKs for top commit:
fjahr:
tACK ac599c4a9cb3b2d424932d3fd91f9eed17426827
rkrux:
lgtm tACK ac599c4a9cb3b2d424932d3fd91f9eed17426827
theStack:
Code-review ACK ac599c4a9cb3b2d424932d3fd91f9eed17426827 🗝️
Tree-SHA512: 626b9adc42ed2403e2f4405321eb9ce009a829c07d968e95ab288fe4940b195b0af35ca279a4a7fa51af76e55382bad6f63a23bca14a84140559b3c667e7041e
75e6984ec8c6fa196ad78c11f454da506d7c8ff1 test/refactor: use test deque to avoid quadratic iteration (Lőrinc)
Pull request description:
Extracted from https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2323012972.
-----
In Python, [list `pop(0)` is linear](https://docs.python.org/3/tutorial/datastructures.html#using-lists-as-queues), so consuming all items in the test results in quadratic iteration.
Switching to `collections.deque` with `popleft()` expresses FIFO intent and avoids the O(n^2) path.
Behavior is unchanged - for a few hundred items the perf impact is likely negligible.
ACKs for top commit:
maflcko:
lgtm ACK 75e6984ec8c6fa196ad78c11f454da506d7c8ff1
theStack:
re-ACK 75e6984ec8c6fa196ad78c11f454da506d7c8ff1
enirox001:
reACK 75e6984
w0xlt:
reACK 75e6984ec8
Tree-SHA512: 290f6aeeb33d8b12b7acbbfede7ce0bef1c831a7ab9efc9c3a08c049986572e289cdece0844db908cf198395f574575ce4073c268033bf6dbaadc3828c96c1d8
113a4228229baedda2a730e097f2d59ad58a4b0d wallet: Add m_cached_from_me to cache "from me" status (Ava Chow)
609d265ebc51abfe9a9ce570da647b6839dc1214 test: Add a test for anchor outputs in the wallet (Ava Chow)
c40dc822d74aea46e4a21774ca282e008f609c2a wallet: Throw an error in sendall if the tx size cannot be calculated (Ava Chow)
39a7dbdd277d1dea9a70314d8cc5ae057999ee88 wallet: Determine IsFromMe by checking for TXOs of inputs (Ava Chow)
e76c2f7a4111f87080e31539f83c21390fcd8f3b test: Test wallet 'from me' status change (Ava Chow)
Pull request description:
One of the ways that the wallet would determine if a transaction was sent from the wallet was by checking if the total amount being spent by a transaction from outputs known to the wallet was greater than 0. This has worked fine until recently since there was no reason for 0-value outputs to be created. However, with ephemeral dust and P2A, it is possible to create standard 0-value outputs, and the wallet was not correctly identifying the spends of such outputs. This PR updates `IsFromMe` to only check whether the wallet knows any of the inputs, rather than checking the debit amount of a transaction.
Additionally, a new functional test is added to test for this case, as well as a few other anchor output related scenarios. This also revealed a bug in `sendall` which would cause an assertion error when trying to spend all of the outputs in a wallet that has anchor outputs.
Fixes#33265
ACKs for top commit:
rkrux:
lgtm ACK 113a4228229baedda2a730e097f2d59ad58a4b0d
enirox001:
Tested ACK 113a422. Ran the full functional test suite including `wallet_anchor.py`; all tests passed. Fix for 0 value anchor detection and sendall size errors looks good. LGTM.
furszy:
ACK 113a4228229baedda2a730e097f2d59ad58a4b0d
Tree-SHA512: df2ce4b258d1875ad0b4f27a5b9b4437137a5889a7d5ed7fbca65f904615e9572d232a8b8d070760f75ac168c1a49b7981f6b5052308575866dc610d191ca964
Extracted from https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2323012972.
In Python, list `pop(0)` is linear, so consuming all items is quadratic.
Switched to `collections.deque` with `popleft()` to express FIFO intent and avoid the O(n^2) path.
Behavior is unchanged; for a few hundred items the perf impact is likely negligible.
Ref: https://docs.python.org/3/tutorial/datastructures.html#using-lists-as-queues
> While appends and pops from the end of list are fast, doing inserts or pops
> from the beginning of a list is slow (because all of the other elements have
> to be shifted by one).
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
fa4885ef2fde2b9fd9eb7367564a2946a45f20ac test: Remove polling loop from test_runner (MarcoFalke)
Pull request description:
(This picks up my prior attempt from https://github.com/bitcoin/bitcoin/pull/13384)
Currently, the test_runner is using a `time.sleep` before polling to check if any tests have completed. This is largely fine when running a few tests, or when the tests take a long time.
However, when running many fast tests, this can accumulate and leave the CPU idle for no reason.
A trivial improvement would be to only sleep when really needed:
```diff
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index 7c8c15f391..1d9f28cee4 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -747,7 +747,6 @@ class TestHandler:
dot_count = 0
while True:
# Return all procs that have finished, if any. Otherwise sleep until there is one.
- time.sleep(.5)
ret = []
for job in self.jobs:
(name, start_time, proc, testdir, log_out, log_err) = job
@@ -771,6 +770,7 @@ class TestHandler:
ret.append((TestResult(name, status, int(time.time() - start_time)), testdir, stdout, stderr, skip_reason))
if ret:
return ret
+ time.sleep(.5)
if self.use_term_control:
print('.', end='', flush=True)
dot_count += 1
```
However, ideally there is no sleep at all. So do that by using a `ThreadPoolExecutor`.
This can be tested via something like:
```
time ./bld-cmake/test/functional/test_runner.py $(for i in {1..200}; do echo -n "tool_rpcauth "; done) -j 200
```
The result should show:
* Current `master` is the slowest
* The "sleep patch" from above is a bit faster (1.5x improvement)
* This pull request is the fastest (2x improvement)
ACKs for top commit:
achow101:
ACK fa4885ef2fde2b9fd9eb7367564a2946a45f20ac
l0rinc:
tested ACK fa4885ef2fde2b9fd9eb7367564a2946a45f20ac
Eunovo:
ReACK fa4885ef2f
Tree-SHA512: f097636c5d9e005781012d8e20c2886cd9968544d4d555b1d2e28982d420ff63fec15cfabb6bd30e4d3c389b8b8350a1ddad721cceaf4b7760cad38b95160175
c76797481155754329ec6a6f58e8402569043944 clang-tidy: Fix critical warnings (Fabian Jahr)
54dc34ec2279378c78fa2d9155668e39e20decda index: Remove unused coinstatsindex recovery code (Fabian Jahr)
37c4fba1f4c18632bceb16f41f5a8f1a61fb9096 index: Check BIP30 blocks when rewinding Coinstatsindex (Fabian Jahr)
51df9de8e5b9c8ecd8339d95b630f312fcb9414e doc: Add release note for 30469 (Fabian Jahr)
bb8d673183294a43c716ff8738da2492f3d7a94b test: Add coinstatsindex compatibility test (Fabian Jahr)
b2e8b64ddc351124ac1390ee906a8fcd2781ca50 index, refactor: Append blocks to coinstatsindex without db read (Fabian Jahr)
431a076ae6e3cc32a8725d4a01483d27c5081a34 index: Fix coinstatsindex overflow issue (Fabian Jahr)
84e813a02bb7b3c735ae413f06c0fc156bfeb7ac index, refactor: DRY coinbase check (Fabian Jahr)
fab842b3248744fb0030486f64d3febe815f8377 index, refactor: Rename ReverseBlock to RevertBlock (Fabian Jahr)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/26362
This continues the work that was started with #26426. It fixes the overflow issue by switching the tracked values that are in danger of overflowing from `CAmount` to `arith_uint256`.
The current approach opts for a simple solution to ensure compatibility with datadirs including the previous version of the index: The new version of the index goes into a separate location in the datadir (`index/coinstatsindex/` rather than `index/coinstats/` before, the new naming is more consistent with the naming of the other indexes). There is no explicit concept of versioning of the index which earlier versions of this PR had. Having the two different versions of the index in separate places allows for downgrading of the node without having to rebuild the index. However, there will be a warning printed in the logs if the new code (v30) detects the old index still being present. A future version could delete a left-over legacy index automatically.
The PR also includes several minor improvements but most notably it lets new entries be calculated and stored without needing to read any DB records.
ACKs for top commit:
achow101:
ACK c76797481155754329ec6a6f58e8402569043944
TheCharlatan:
ACK c76797481155754329ec6a6f58e8402569043944
mzumsande:
Tested / Code Review ACK c76797481155754329ec6a6f58e8402569043944
Tree-SHA512: 3fa4a19dd1a01c1b01390247bc9daa6871eece7c1899eac976e0cc21ede09c79c65f758d14daafc46a43c4ddd7055c85fb28ff03029132d48936b248639c6ab9
fa2163159511a099ecffde2bfddf9cfe33eb9c76 test: Use self.log (MarcoFalke)
fa346f7797ae9fd3ff0e874ea96416c3fe2b4ba5 test: Move error string into exception (MarcoFalke)
fa1986181f246d75b1fcc814353377eeb2256fa0 test: Remove useless catch-throw (MarcoFalke)
fa2f1c55b7da729eb6bb9f88bf48459235cc2664 move-only util data to test/functional/data/util (MarcoFalke)
faa18bf287fc02339b7af29d54af862a289bf96d test: Turn util/test_runner into functional test (MarcoFalke)
fa955154c773c5129f9594982343b440d05957e2 test: Add missing skip_if_no_bitcoin_tx (MarcoFalke)
fac9db6eb0c64333cd202e1053a4dd5fd27344f1 test: Add missing tx util to Binaries (MarcoFalke)
fa91835ec6ad723dc4a379a900ae9331e07a0fba test: Use lowercase env var as attribute name (MarcoFalke)
fac49094cdb12bd566df5a00832462491a839f81 test: Remove duplicate ConfigParser (MarcoFalke)
Pull request description:
The `test/util/test_runner.py` has many issues:
* The boilerplate for the test runner is duplicate or inconsistent with the other (functional) tests. For example, logging options, `ConfigParser` handling, `Binaries` handling ...
* The cmake/ci behavior is brittle and can silently fail, as explained in https://github.com/bitcoin/bitcoin/issues/31476
* corecheck (and likely other places that manually run the tests) completely forget to run it
* If the test is manually called, it runs single threaded, when it could just run in parallel with the other functional tests
Fix all issues by removing the util test_runner and moving the test logic into a new functional test file.
ACKs for top commit:
janb84:
re ACK fa2163159511a099ecffde2bfddf9cfe33eb9c76
brunoerg:
re-ACK fa2163159511a099ecffde2bfddf9cfe33eb9c76
hebasto:
re-ACK fa2163159511a099ecffde2bfddf9cfe33eb9c76, additional feedback has been addressed since my previous [review](https://github.com/bitcoin/bitcoin/pull/32697#pullrequestreview-2940350432).
Tree-SHA512: 694e647887801f002843a74011035d5ed3dfed091d3f0ae18e812a16a4680e04e60e50de0a92af7e047e8ddd6ff5a7834c690f16fd42b74ebc1674bf9989406f
Additionally this commit gives each test its
own function.
The assert_submitblock helper is absorbed into
assert_template.
Review hint:
git show --color-moved=dimmed-zebra
10845cd7cc89fc83b4ee844dc577b391720bba9c qa: Add feature_framework_startup_failures.py (Hodlinator)
28e282ef9ae94ede4aace6b97ff18c66cb72a001 qa: assert_raises_message() - Stop assuming certain structure for exceptions (Hodlinator)
1f639efca5e71a0ff208415d94e408a74778d4db qa: Work around Python socket timeout issue (Hodlinator)
9b24a403fae4b896ff7705519bd48c877b4e621b qa: Only allow calling TestNode.stop() after connecting (Hodlinator)
6ad21b4c0114029d16d334bf8d437834708a295e qa: Include ignored errors in RPC connection timeout (Hodlinator)
879243e81fd55f54739fbdae4d5b8666f5acb9a9 qa refactor: wait_for_rpc_connection - Treat OSErrors the same (Hodlinator)
Pull request description:
Improves handling of startup errors in functional tests and puts tests in place to ensure knock-on errors don't creep in.
- `wait_for_rpc_connection()` now appends specific failures leading up to the `Unable to connect to bitcoind` error to that error message:
`[node 0] Unable to connect to bitcoind after 60s (ignored errors: {'missing_credentials': 1, 'OSError.ECONNREFUSED': 239}, latest error: ConnectionRefusedError(111, 'Connection refused'))`
- Fixes Windows Python issue where `socket.timeout` exceptions end up with unset `errno`-fields.
- Also adds comments, refactors code, improves logging.
The underlying purpose is to ensure developer efficiency in finding root causes of test failures.
Prior iterations of the PR partially focused on fixing the same issue as #31620.
Originally inspired by #30390.
### Testing
Can be tested by reverting either faf2f2c654d9aa18b2f49a157956f9ab0fce302a or fae3bf6b870eb0f9cddd1adac82ba72890806ae3 from #31620, or the "qa: Avoid calling stop-RPC if not connected" from this PR, and running *feature_framework_startup_failures.py*.
ACKs for top commit:
l0rinc:
ACK 10845cd7cc89fc83b4ee844dc577b391720bba9c
ryanofsky:
Code review ACK 10845cd7cc89fc83b4ee844dc577b391720bba9c. Only changes since last review were adding a new commit tweaking assert_raises_message(), extending the new test to have a self-check, and to pass through all options to child tests instead of a hardcoded list of options. I left some cleanup suggestions below but they are not important.
Tree-SHA512: f0235c5cbb6d1bb85d8dc5de492a08a34f6edc83499cbf0a5f9a3824809ff84635888c62c9c01101e3cc9ef9f1cdee2c9ab6537fea6feeb005b29f428caf8b22
Removes all legacy wallet specific functional tests.
Also removes the --descriptor and --legacy-wallet options as these are
no longer necessary with the legacy wallet removed.
bb633c9407c46eeadf6fe85e859cea1fed24473f tests: add functional test for miniscript decaying multisig (Michael Dietz)
Pull request description:
This is very closely based on [test/functional/wallet_multisig_descriptor_psbt.py](https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_multisig_descriptor_psbt.py) both in code and concept. It should serve as some integration testing for Miniscript descriptors, and also documents a simple multisig that starts as 4-of-4 and decays to 3-of-4, 2-of-4, and finally 1-of-4 at block heights (I think in the real world aligning this to halvenings would be nice).
ACKs for top commit:
achow101:
ACK bb633c9407c46eeadf6fe85e859cea1fed24473f
rkrux:
reACK bb633c9407c46eeadf6fe85e859cea1fed24473f
hodlinator:
ACK bb633c9407c46eeadf6fe85e859cea1fed24473f
Tree-SHA512: 1f8e8e50258d45d8f2b882b5f86dcd390d86c543ff4801f397733017102e0854ac387960b6e296bb164603545615d224a4b400247cbbc07bf21b2f4b718ab2ff
4080b66cbec2b6fc2fcfd7356941236f65d508e3 test: add test for utxo-to-sqlite conversion script (Sebastian Falbesoner)
ec99ed738083fc1ad4c9d85095e26e7e58372217 contrib: add tool to convert compact-serialized UTXO set to SQLite database (Sebastian Falbesoner)
Pull request description:
## Problem description
There is demand from users to get the UTXO set in form of a SQLite database (#24628). Bitcoin Core currently only supports dumping the UTXO set in a binary _compact-serialized_ format, which was crafted specifically for AssumeUTXO snapshots (see PR #16899), with the primary goal of being as compact as possible. Previous PRs tried to extend the `dumptxoutset` RPC with new formats, either in human-readable form (e.g. #18689, #24202), or most recently, directly as SQLite database (#24952). Both are not optimal: due to the huge size of the ever-growing UTXO set with already more than 80 million entries on mainnet, human-readable formats are practically useless, and very likely one of the first steps would be to put them in some form of database anyway. Directly adding SQLite3 dumping support on the other hand introduces an additional dependency to the non-wallet part of bitcoind and the risk of increased maintenance burden (see e.g. https://github.com/bitcoin/bitcoin/pull/24952#issuecomment-1163551060, https://github.com/bitcoin/bitcoin/issues/24628#issuecomment-1108469715).
## Proposed solution
This PR follows the "external tooling" route by adding a simple Python script for achieving the same goal in a two-step process (first create compact-serialized UTXO set via `dumptxoutset`, then convert it to SQLite via the new script). Executive summary:
- single file, no extra dependencies (sqlite3 is included in Python's standard library [1])
- ~150 LOC, mostly deserialization/decompression routines ported from the Core codebase and (probably the most difficult part) a little elliptic curve / finite field math to decompress pubkeys (essentialy solving the secp256k1 curve equation y^2 = x^3 + 7 for y given x, respecting the proper polarity as indicated by the compression tag)
- creates a database with only one table `utxos` with the following schema:
```(txid TEXT, vout INT, value INT, coinbase INT, height INT, scriptpubkey TEXT)```
- the resulting file has roughly 2x the size of the compact-serialized UTXO set (this is mostly due to encoding txids and scriptpubkeys as hex-strings rather than bytes)
[1] note that there are some rare cases of operating systems like FreeBSD though, where the sqlite3 module has to installed explicitly (see #26819)
A functional test is also added that creates UTXO set entries with various output script types (standard and also non-standard, for e.g. large scripts) and verifies that the UTXO sets of both formats match by comparing corresponding MuHashes. One MuHash is supplied by the bitcoind instance via `gettxoutsetinfo muhash`, the other is calculated in the test by reading back the created SQLite database entries and hashing them with the test framework's `MuHash3072` module.
## Manual test instructions
I'd suggest to do manual tests also by comparing MuHashes. For that, I've written a go tool some time ago which would calculate the MuHash of a sqlite database in the created format (I've tried to do a similar tool in Python, but it's painfully slow).
```
$ [run bitcoind instance with -coinstatsindex]
$ ./src/bitcoin-cli dumptxoutset ~/utxos.dat
$ ./src/bitcoin-cli gettxoutsetinfo muhash <block height returned in previous call>
(outputs MuHash calculated from node)
$ ./contrib/utxo-tools/utxo_to_sqlite.py ~/utxos.dat ~/utxos.sqlite
$ git clone https://github.com/theStack/utxo_dump_tools
$ cd utxo_dump_tools/calc_utxo_hash
$ go run calc_utxo_hash.go ~/utxos.sqlite
(outputs MuHash calculated from the SQLite UTXO set)
=> verify that both MuHashes are equal
```
For a demonstration what can be done with the resulting database, see https://github.com/bitcoin/bitcoin/pull/24952#pullrequestreview-956290477 for some example queries. Thanks go to LarryRuane who gave me to the idea of rewriting this script in Python and adding it to `contrib`.
ACKs for top commit:
ajtowns:
ACK 4080b66cbec2b6fc2fcfd7356941236f65d508e3 - light review
achow101:
ACK 4080b66cbec2b6fc2fcfd7356941236f65d508e3
romanz:
tACK 4080b66cbe on signet (using [calc_utxo_hash](8981aa3e85/calc_utxo_hash/calc_utxo_hash.go)):
tdb3:
ACK 4080b66cbec2b6fc2fcfd7356941236f65d508e3
Tree-SHA512: be8aa0369a28c8421a3ccdf1402e106563dd07c082269707311ca584d1c4c8c7b97d48c4fcd344696a36e7ab8cdb64a1d0ef9a192a15cff6d470baf21e46ee7b
This is similar in structure to test/functional/wallet_multisig_descriptor_psbt.py
both in code and concept. It should serve as some integration testing for
Miniscript descriptors, and also documents a simple multisig that starts as 4-of-4
and decays to 3-of-4, 2-of-4, and finally 1-of-4 at block heights (I think in the
real world aligning this to halvenings would be nice).
1dd3af8fbc350c6f1efa8ae6449e67e1b42ccff4 Add release note for #31223 (Martin Zumsande)
997757dd2b4d7b20b17299fbd21970b2efb8bbc8 test: add functional test for -port behavior (Martin Zumsande)
0e2b12b92a28a2949e75bf50f31563f52e647d6e net, init: derive default onion port if a user specified a -port (Martin Zumsande)
Pull request description:
This resolves#31133 (setups with multiple local nodes each using a different `-port` no longer working with v28.0, see the issue description for more details) by deriving the default onion listening port to be the value specified by `-port` incremented by 1 (idea by vasild / laanwj).
Note that with this fix, the chosen `-port` values of two local nodes cannot be adjacent, otherwise there will be port collisions again.
From the discussion in the linked issue, this was the most popular option, followed by doing nothing and telling affected users to change their setups to use `-bind` instead of `-port`. But more opinions are certainly welcome!
I think that if we decide to do something about the problem described in the issue, we should do so soon (in 28.1.), so I opened this PR.
Fixes#31133
ACKs for top commit:
achow101:
ACK 1dd3af8fbc350c6f1efa8ae6449e67e1b42ccff4
laanwj:
Tested ACK 1dd3af8fbc350c6f1efa8ae6449e67e1b42ccff4
tdb3:
Code review ACK 1dd3af8fbc350c6f1efa8ae6449e67e1b42ccff4
Tree-SHA512: 37fda2b23bbedcab5df3a401cf5afce66ae5318fb78f9660f83e3fd075b528e8156d7a0903f9a12ffe97ab5d83860587116b74af28670a1f4c2f0d1be4999f40
37a5c5d83664c31d83fc649d3c8c858bd5f10f21 doc: update descriptors.md for getdescriptoractivity (James O'Beirne)
ee3ce6a4f4d35afe7fcab16eff419a6788b02170 test: rpc: add no address case for getdescriptoractivity (James O'Beirne)
811f76f3a511d20750046319b390e225a1151caa rpc: add getdescriptoractivity (James O'Beirne)
25fe087de59e967ce968d35ed77138325eb9a9fa rpc: move-only: move ScriptPubKeyDoc to utils (James O'Beirne)
Pull request description:
The RPC command `scanblocks` provides a useful way to get a set of blockhashes that have activity relevant to a set of descriptors (`relevant_blocks`). However actually extracting the activity from those blocks is left as an exercise to the end user.
This process involves not only generating the (potentially ranged) set of scripts for the descriptor set on the client side (maybe via `deriveaddresses`), but then the user must retrieve each block's contents one-by-one using `getblock <hash>`, which is transmitted over a network link. And that's all before they perform the actual search over block content. There's even more work required to incorporate unconfirmed transactions.
This PR introduces an RPC `getdescriptoractivity` that [dovetails](https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2024-08-16#1046393;) with `scanblocks` output, handling the process described above. Users specify the blockhashes (perhaps from `relevant_blocks`) and a set of descriptors; they are then given all spend/receive activity in that set of blocks.
This is a very useful tool when implementing lightweight wallets that want neither to require a third-party indexer like electrs, nor the overhead of creating and managing watch-only wallets in Core. This allows Core to be more easily used in a "stateless" manner by wallets, with potentially many nodes interchangeably acting as backends.
### Example usage
```
% ./src/bitcoin-cli scanblocks start \
'["addr(bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t)"]' \
857263
{
"from_height": 857263,
"to_height": 858263,
"relevant_blocks": [
"00000000000000000002bc5cc78f5b0913a5230a8f4b0d5060bc9a60900a5a88",
"00000000000000000001c5291ed6a40c06d3db5c8fb738567654b24a14b24ecb"
],
"completed": true
}
% ./src/bitcoin-cli getdescriptoractivity \
'["00000000000000000002bc5cc78f5b0913a5230a8f4b0d5060bc9a60900a5a88", "00000000000000000001c5291ed6a40c06d3db5c8fb738567654b24a14b24ecb"]' \
'["addr(bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t)"]'
{
"activity": [
{
"type": "receive",
"amount": 0.00002900,
"blockhash": "00000000000000000002bc5cc78f5b0913a5230a8f4b0d5060bc9a60900a5a88",
"height": 857907,
"txid": "c9d34f202c1f66d80cae76f305350f5fdde910b97cf6ae6bf79f5bcf2a337d06",
"vout": 254,
"output_spk": {
"asm": "1 7e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b",
"desc": "rawtr(7e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b)#yewcd80j",
"hex": "51207e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b",
"address": "bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t",
"type": "witness_v1_taproot"
}
},
{
"type": "spend",
"amount": 0.00002900,
"blockhash": "00000000000000000001c5291ed6a40c06d3db5c8fb738567654b24a14b24ecb",
"height": 858260,
"spend_txid": "7f61d1b248d4ee46376f9c6df272f63fbb0c17039381fb23ca5d90473b823c36",
"spend_vin": 0,
"prevout_txid": "c9d34f202c1f66d80cae76f305350f5fdde910b97cf6ae6bf79f5bcf2a337d06",
"prevout_vout": 254,
"prevout_spk": {
"asm": "1 7e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b",
"desc": "rawtr(7e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b)#yewcd80j",
"hex": "51207e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b",
"address": "bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t",
"type": "witness_v1_taproot"
}
}
]
}
```
ACKs for top commit:
instagibbs:
reACK 37a5c5d83664c31d83fc649d3c8c858bd5f10f21
achow101:
ACK 37a5c5d83664c31d83fc649d3c8c858bd5f10f21
tdb3:
Code review and light retest ACK 37a5c5d83664c31d83fc649d3c8c858bd5f10f21
rkrux:
re-ACK 37a5c5d83664c31d83fc649d3c8c858bd5f10f21
Tree-SHA512: 04aa51e329c6c2ed72464b9886281d5ebd7511a8a8e184ea81249033a4dad535a12829b1010afc2da79b344ea8b5ab8ed47e426d0bf2eb78ab395d20b1da8dbb
ee1128ead846698db5e5633f193883837f2fbc64 doc: update stack-clash-protection comment re mingw-w64 (fanquake)
bf47448f152316145d9abb9b8abc3b564194fe46 test: drop check for Windows < 10 (fanquake)
35b898c47f8af6807c4a5f404af165c663c81a99 release: target Windows 10 or later (fanquake)
398754e70bc96b86ad0327fbe70fafdf27bb4e35 depends: target Windows 10 when building for mingw-w64 (fanquake)
Pull request description:
Follows up to https://github.com/bitcoin/bitcoin/pull/31048#discussion_r1803165670.
We definitely cannot claim that Bitcoin Core is "supported and extensively tested on" on Windows 7.
Note that #30997 is also increasing the minimum required Windows version (for the GUI) to 10.
ACKs for top commit:
hodlinator:
cr-ACK ee1128ead846698db5e5633f193883837f2fbc64
davidgumberg:
ACK ee1128ead8
achow101:
ACK ee1128ead846698db5e5633f193883837f2fbc64
hebasto:
re-ACK ee1128ead846698db5e5633f193883837f2fbc64, only rebased, a commit message and a comment have been amended since my recent [review](https://github.com/bitcoin/bitcoin/pull/31172#pullrequestreview-2415452160).
TheCharlatan:
ACK ee1128ead846698db5e5633f193883837f2fbc64
Tree-SHA512: 245e0bac3d63414d919a1948661fef4ff79359faaacaf19d64abd91cc62e822797fb1cf3379e340bfdf9a85c0b88fd99a90eda450dd4218b6213ab78aefb1374
fa6d14eacb2a8c1c3243e3075254dfdebaa9290e test: Treat exclude list warning as failure in CI (MarcoFalke)
Pull request description:
An outdated exclude list or otherwise an error in the exclude list handling is usually a bug.
So make it fatal in the CI, instead of silently ignoring it.
Fixes https://github.com/bitcoin/bitcoin/pull/30872/files#r1757015334
Can be tested with something like (with and without `--ci`):
```
./bld-cmake/test/functional/test_runner.py wallet_disable -x wallet_disablee
ACKs for top commit:
tdb3:
ACK fa6d14eacb2a8c1c3243e3075254dfdebaa9290e
ismaelsadeeq:
utACK fa6d14eacb2a8c1c3243e3075254dfdebaa9290e
Tree-SHA512: 03a70dff9d1272d982591d60ab764f9233d4802488bc1bad305a2755e2d7ed86e691ee94767a3bc5f68321b63214aba44e6f9edd1543dfad7a20f9397cf78734
- p2p_node_network_limited -v*transport
- feature_assume_utxo
On CI runners these tests are taking longer than their current bucket
suggests, often being among the last to finish.
Re-bucket them to improve CI efficiency.
replaced --enable-debug with -DCMAKE_BUILD_TYPE=Debug in developer-notes
replaced --enable-multiprocess with -DWITH_MULTIPROCESS=ON
replaced --disable-zmq with -DWITH_ZMQ=OFF
72b46f28bf8d37a166961b5aa2a22302b932b756 test: fix exclude parsing for functional runner (Max Edwards)
Pull request description:
This restores previous behaviour of being able to exclude a test by name without having to specify .py extension.
It was noticed in https://github.com/bitcoin/bitcoin/issues/30851 that tests were no longer being excluded.
PR https://github.com/bitcoin/bitcoin/pull/30244 introduced being able to exclude a specific tests based on args (such as `--exclude "rpc_bind.py --ipv6`) but it made the wrong assumption that test names intended to be excluded would include the .py extension.
The following https://github.com/bitcoin/bitcoin/pull/30244#issuecomment-2344009687 shows that this is not how the `--exclude` flag was used in CI.
https://github.com/bitcoin/bitcoin/pull/30244#issuecomment-2344009687 gave three examples of `--exclude` being used in CI so I compared the number of tests that the runner would run for these three examples in three situations, before #30244 was introduced, in master today and with this PR applied.
Example:
`--previous-releases --coverage --extended --exclude feature_dbcrash`
Test count:
Before #30244 introduced: 314
Master: 315
With this PR: 314
Example:
`--exclude feature_init,rpc_bind,feature_bind_extra`
Test count:
Before #30244 introduced: 306
Master 311
With this PR: 306
Example:
`--exclude rpc_bind,feature_bind_extra`
Before #30244 introduced: 307
Master 311
With this PR: 307
I've also tested that the functionality introduced with #30244 remains and we can still exclude specific tests by argument.
ACKs for top commit:
maflcko:
review ACK 72b46f28bf8d37a166961b5aa2a22302b932b756
willcl-ark:
ACK 72b46f28bf8d37a166961b5aa2a22302b932b756
Tree-SHA512: 37c0e3115f4e3efdf9705f4ff8cd86a5cc906aacc1ab26b0f767f5fb6a953034332b29b0667073f8382a48a2fe9d649b7e60493daf04061260adaa421419d8c8
2ad560139b80e149844f285cd1456bb7cc0eb1ee Remove unused src_dir param from run_tests (Lőrinc)
Pull request description:
The `src_dir` usage was removed in a8a2e364ac (diff-437d7f6e9f2229879b60aae574a8217f14c643bbf3cfa9225d8011d6d52df00cL598), making the parameter unused.
Top commit has no ACKs.
Tree-SHA512: 1fd8b93811b4ab467ba5a160a4fe204e9606e1bf237c7595ed6f8b7821cf59d2a776c0e1e154852a45b2a35e5bdbd8996314e4f63a9c750f21b9a17875cb636a
8131bf7483c0ea10d3573c9f2e977d19d8569b7f ci: parse TEST_RUNNER_EXTRA into an array (Max Edwards)
c4762b0aa06f2654d108bc7ca05887ffd88cf6f8 test: allow excluding func test by name and arg (Max Edwards)
Pull request description:
While working on CI I wanted to disable some functional tests so I used the `TEST_RUNNER_EXTRA` var. The problem I had was tests that have flags such as `rpc_bind.py --ipv6` must be passed in quotes otherwise the `--ipv6` portion will be considered an argument to `test_runner.py` rather than a test name.
This change allows proper parsing of quotes and complex values such as:
```shell
TEST_RUNNER_EXTRA='--exclude "rpc_bind.py --ipv6,feature_proxy.py"'
```
Update:
While testing this it was noticed that `test_runner.py` when given `--exclude "rpc_bind.py --ipv6"` will exclude all `rpc_bind.py` tests so this PR has been updated to include a change to the test runner to only exclude the specific test if you pass an arg or exclude all tests of that name if you do not pass an arg. `--exclude rpc_bind.py` will exclude all three variants and `--exclude rpc_bind --ipv6` will only exclude the IPV6 variant.
ACKs for top commit:
maflcko:
ACK 8131bf7483c0ea10d3573c9f2e977d19d8569b7f
achow101:
ACK 8131bf7483c0ea10d3573c9f2e977d19d8569b7f
hebasto:
ACK 8131bf7483c0ea10d3573c9f2e977d19d8569b7f, tested on Ubuntu 23.10 and Windows 11.
Tree-SHA512: 82b73f12d627f533d8e5be4a518d455ef4427a755bbe03ccd11d0bb70c7ff3cee76220b0264fcfb236661c4cf5deba034cbfc2372b96d5861f3436c21eae8264
6eeb188d40fe8f6c3b0a3be6dca519ea9a7b2358 test: adds seednode functional tests (Sergi Delgado Segura)
3270f0adad6ccbb8c004fb222f420e9b3ea32ea6 net: Favor peers from addrman over fetching seednodes (Sergi Delgado Segura)
Pull request description:
This is a follow-up of #28016 motivated by https://github.com/bitcoin/bitcoin/pull/28016#pullrequestreview-1913140932 and https://github.com/bitcoin/bitcoin/pull/28016#issuecomment-1984448937.
The current behavior of seednode fetching is pretty eager: we do it as the first step under `ThreadOpenNetworkConnections` even if some peers may be queryable from our addrman. This poses two potential issues:
- First, if permanently set (e.g. running with seednode in a config file) we'd be signaling such seed every time we restart our node
- Second, we will be giving the seed node way too much influence over our addrman, populating the latter with data from the former even when unnecessary
This changes the behavior to only add seednodes to `m_addr_fetch` if our addrman is empty, or little by little after we've spent some time trying addresses from our addrman. Also, seednodes are added to `m_addr_fetch` in random order, to avoid signaling the same node in case more than one seed is added and we happen to try them over multiple restarts
ACKs for top commit:
achow101:
ACK 6eeb188d40fe8f6c3b0a3be6dca519ea9a7b2358
cbergqvist:
ACK 6eeb188d40fe8f6c3b0a3be6dca519ea9a7b2358
itornaza:
Tested ACK 6eeb188d40fe8f6c3b0a3be6dca519ea9a7b2358
tdb3:
ACK 6eeb188d40fe8f6c3b0a3be6dca519ea9a7b2358
Tree-SHA512: b04445412f22018852d6bef4d3f1e88425ee6ddb434f61dcffa9e0c41b8e31f8c56f83858d5c7686289c86dc4c9476c437df15ea61a47082e2bb2e073cc62f15
