master branch:
0m36.86s real 0m03.26s user 0m01.69s system
0m35.71s real 0m03.78s user 0m01.64s system
0m45.76s real 0m03.12s user 0m01.27s system
PR branch:
0m13.04s real 0m02.66s user 0m00.93s system
0m14.08s real 0m02.81s user 0m00.82s system
0m14.05s real 0m02.50s user 0m00.93s system
Previous bilingual_str tinyformat::format accepted bilingual format strings,
but not bilingual arguments. Extend it to accept both. This is useful when
embedding one translated string inside another translated string, for example:
`strprintf(_("Error: %s"), message)` which would fail previously if `message`
was a bilingual_str.
3141eab9c6 test: add functional test for ScanAndUnlinkAlreadyPrunedFiles (Andrew Toth)
e252909e56 test: add unit test for ScanAndUnlinkAlreadyPrunedFiles (Andrew Toth)
77557dda4a prune: scan and unlink already pruned block files on startup (Andrew Toth)
Pull request description:
There are a few cases where we can mark a block and undo file as pruned in our block index, but not actually remove the files from disk.
1. If we call `FindFilesToPrune` or `FindFilesToPruneManual` and crash before `UnlinkPrunedFiles`.
2. If on Windows there is an open file handle to the file somewhere else when calling `fs::remove` in `UnlinkPrunedFiles` (https://en.cppreference.com/w/cpp/filesystem/remove, https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-deletefilew#remarks). This could be from another process, or if we are calling `ReadBlockFromDisk`/`ReadRawBlockFromDisk` without having a lock on `cs_main` (which has been allowed since ccd8ef65f9).
This PR mitigates this by scanning all pruned block files on startup after `LoadBlockIndexDB` and unlinking them again.
ACKs for top commit:
achow101:
ACK 3141eab9c6
pablomartin4btc:
re-ACK with added functional test 3141eab9c6.
furszy:
Code review ACK 3141eab9
theStack:
Code-review ACK 3141eab9c6
Tree-SHA512: 6c73bc57838ad1b7e5d441af3c4d6bf4c61c4382e2b86485e57fbb74a61240710c0ceeceb8b4834e610ecfa3175c6955c81ea4b2285fee11ca6383f472979d8d
7013da07fb Add release note for PR#25943 (David Gumberg)
04f270b435 Add test for unspendable transactions and parameter 'maxburnamount' to sendrawtransaction. (David Gumberg)
Pull request description:
This PR adds a user configurable, zero by default parameter — `maxburnamount` — to `sendrawtransaction`. This PR makes bitcoin core reject transactions that contain unspendable outputs which exceed `maxburnamount`. closes#25899.
As a result of this PR, `sendrawtransaction` will by default block 3 kinds of transactions:
1. Those that begin with `OP_RETURN` - (datacarriers)
2. Those whose lengths exceed the script limit.
3. Those that contain invalid opcodes.
The user is able to configure a `maxburnamount` that will override this check and allow a user to send a potentially unspendable output into the mempool.
I see two legitimate use cases for this override:
1. Users that deliberately use `OP_RETURN` for datacarrier transactions that embed data into the blockchain.
2. Users that refuse to update, or are unable to update their bitcoin core client would be able to make use of new opcodes that their client doesn't know about.
ACKs for top commit:
glozow:
reACK 7013da07fb
achow101:
re-ACK 7013da07fb
Tree-SHA512: f786a796fb71a587d30313c96717fdf47e1106ab4ee0c16d713695e6c31ed6f6732dff6cbc91ca9841d66232166eb058f96028028e75c1507324426309ee4525
2f84ad7b9e docs: add ramdisk guide for running tests on OSX (Matthew Zipkin)
Pull request description:
Using a ramdisk on OSX sped up the test suite by about 5x (using default `jobs=4`) on my M1 macbook pro running macOS Monterey 12.3.1. This PR adds the relevant OSX commands following the Linux directions.
Default:
```
8204 s (accumulated)
Runtime: 2104 s
```
following commands from the PR:
```
1606 s (accumulated)
Runtime: 421 s
```
ramdisk + `jobs=32`:
```
2090 s (accumulated)
Runtime: 85 s
```
ACKs for top commit:
jonatack:
ACK 2f84ad7b9e
willcl-ark:
ACK 2f84ad7b9e
brunoerg:
utACK 2f84ad7b9e
Tree-SHA512: 37a9903c8ac2cbfaa91e7e73fc96ef65042ff4b15763d452af7b8615255adf03429ad01cf85265a99dd569290c1d69c05a393d616868c05c190b60b053820786
0af16e7134 doc: add release note for #25574 (Martin Zumsande)
57ef2a4812 validation: report if pruning prevents completion of verification (Martin Zumsande)
0c7785bb25 init, validation: Improve handling if VerifyDB() fails due to insufficient dbcache (Martin Zumsande)
d6f781f1cf validation: return VerifyDBResult::INTERRUPTED if verification was interrupted (Martin Zumsande)
6360b5302d validation: Change return value of VerifyDB to enum type (Martin Zumsande)
Pull request description:
`VerifyDB()` can fail to complete due to insufficient dbcache at the level 3 checks. This PR improves the error handling in this case in the following ways:
- The rpc `-verifychain` now returns false if the check can't be completed due to insufficient cache
- During init, we only log a warning if the default values for `-checkblocks` and `-checklevel` are taken and the check doesn't complete. However, if the user actively specifies one of these args, we return with an InitError if we can't complete the check.
This PR also changes `-verifychain` RPC to return `false` if the verification didn't finish due to missing block data (pruning) or due to being interrupted by the node being shutdown.
Previously, this PR also included a fix for a possible assert during verification - this was done in #27009 (now merged).
ACKs for top commit:
achow101:
ACK 0af16e7134
ryanofsky:
Code review ACK 0af16e7134. Only small suggested changes since the last review, like renaming some of the enum values. I did leave more suggestions, but they are not very important and could be followups
john-moffett:
ACK 0af16e7134
MarcoFalke:
lgtm re-ACK 0af16e7134 🎚
Tree-SHA512: 84b4f767cf9bfbafef362312757c9bf765b41ae3977f4ece840e40c52a2266b1457832df0cdf70440be0aac2168d9b58fc817238630b0b6812f3836ca950bc0e
c3b4b5a142 test: Replace 0xC0 constant (roconnor-blockstream)
Pull request description:
Instead it should be the named constant `LEAF_VERSION_TAPSCRIPT`.
ACKs for top commit:
instagibbs:
ACK c3b4b5a142
theStack:
ACK c3b4b5a142
Tree-SHA512: c00be584ea2d0e7c01bf5620da0da1f37e5b5298ef95df48d91d137c8c542f5d91be158d45392cf2ba8874bf27bd12924e2eed395773b49d091e3028de3356a2
4bbf5ddd44 Detailed error message for passphrases with null chars (John Moffett)
b4bdabc223 doc: Release notes for 27068 (John Moffett)
4b1205ba37 Test case for passphrases with null characters (John Moffett)
00a0861181 Pass all characters to SecureString including nulls (John Moffett)
Pull request description:
`SecureString` is a `std::string` specialization with a secure allocator. However, in practice it's treated like a C- string (no explicit length and null-terminated). This can cause unexpected and potentially insecure behavior. For instance, if a user enters a passphrase with embedded null characters (which is possible through Qt and the JSON-RPC), it will ignore any characters after the first null, potentially giving the user a false sense of security.
Instead of assigning to `SecureString` via `std::string::c_str()`, assign it via a `std::string_view` of the original. This explicitly captures the size and still doesn't make any extraneous copies in memory.
Note to reviewers, the following all compile identically in recent `GCC` (x86-64 and ARM64) with `-O2` (and `-std=c++17`):
```C++
std::string orig_string;
std::cin >> orig_string;
SecureString s;
s.reserve(100);
// The following all compile identically
s = orig_string;
s = std::string_view{orig_string};
s.assign(std::string_view{orig_string});
s.assign(orig_string.data(), orig_string.size());
```
So it's largely a matter of preference. However, one thing to keep in mind is that we want to avoid making unnecessary copies of any sensitive data in memory.
Something like `SecureString s{orig_string};` is still invalid and probably unwanted in our case, since it'd get treated as a short string and optimized away from the secure allocator. I presume that's the reason for the `reserve()` calls.
Fixes#27067.
ACKs for top commit:
achow101:
re-ACK 4bbf5ddd44
stickies-v:
re-ACK [4bbf5dd](4bbf5ddd44)
furszy:
utACK 4bbf5ddd
Tree-SHA512: 47a96905a82ca674b18076a20a388123beedf70e9de73e42574ea68afbb434734e56021835dd9b148cdbf61709926b487cc95e9021d9bc534a7c93b3e143d2f7
4d84eaec82 Raise PRNG seed log to INFO. (roconnor-blockstream)
Pull request description:
Some build infrastructure, such as Nix, will delete failed builds by default, keeping only the log (stdout/stderr) of the failed build.
For flaky tests, it would be very helpful to have the PRNG seed in the default log in order to redo the failed test.
By simply raising the PRNG seed logging to INFO, we can, by default, record the seed in the log of every build.
ACKs for top commit:
MarcoFalke:
lgtm ACK 4d84eaec82
theStack:
ACK 4d84eaec82
Tree-SHA512: 3ccb4a4e7639a3babc3b2a6456a6d0bffc090da34e4545b317f7bfbed4e9950d1b38ea5b2a90c37ccb49b3454bdeff03a6aaf86770b9c4dd14b26320aba50b94
9486509be6 wallet, rpc: Update migratewallet help text for encrypted wallets (Andrew Chow)
aaf02b5721 tests: Tests for migrating wallets by name, and providing passphrase (Andrew Chow)
7fd125b27d wallet: Be able to unlock the wallet for migration (Andrew Chow)
6bdbc5ff59 rpc: Allow users to specify wallet name for migratewallet (Andrew Chow)
dbfa345403 wallet: Allow MigrateLegacyToDescriptor to take a wallet name (Andrew Chow)
Pull request description:
`migratewallet` currently operates on wallets that are already loaded, however this is not necessarily required, and in the future, not possible once the legacy wallet is removed. So we need to also be able to give the wallet name to migrate.
Additionally, the passphrase is required when migrating a wallet. Since a wallet may not be loaded when we migrate, and as we currently unload wallets when migrating, we need the passphrase to be given to `migratewallet` in order to migrate encrypted wallets.
Fixes#27048
ACKs for top commit:
john-moffett:
reACK 9486509be6
pinheadmz:
ACK 9486509be6
furszy:
ACK 9486509b
Tree-SHA512: 35e2ba69a148e129a41e20d7fb99c4cab7947b1b7e7c362f4fd06ff8ac6e79e476e07207e063ba5b80e1a33e2343f4b4f1d72d7930ce80c34571c130d2f5cff4
61bb4e783b lint: enable E722 do not use bare except (Leonardo Lazzaro)
Pull request description:
Improve test code and enable E722 lint check.
If you want to catch all exceptions that signal program errors, use except Exception: (bare except is equivalent to except BaseException:).
Reference: https://peps.python.org/pep-0008/#programming-recommendations
ACKs for top commit:
MarcoFalke:
lgtm ACK 61bb4e783b
Tree-SHA512: c7497769d5745fa02c78a20f4a0e555d8d3996d64af6faf1ce28e22ac1d8be415b98e967294679007b7bda2a9fd04031a9d140b24201e00257ceadeb5c5d7665
6a5b348f2e test: test rescanning encrypted wallets (ishaanam)
493b813e17 wallet: ensure that the passphrase is not deleted from memory when being used to rescan (ishaanam)
66a86ebabb wallet: keep track of when the passphrase is needed when rescanning (ishaanam)
Pull request description:
Wallet passphrases are needed to top up the keypool of encrypted wallets
during a rescan. The following RPCs need the passphrase when rescanning:
- `importdescriptors`
- `rescanblockchain`
The following RPCs use the information about whether or not the
passphrase is being used to ensure that full rescans are able to
take place (meaning the following RPCs should not be able to run
if a rescan requiring the wallet to be unlocked is taking place):
- `walletlock`
- `encryptwallet`
- `walletpassphrasechange`
`m_relock_mutex` is also introduced so that the passphrase is not
deleted from memory when the timeout provided in
`walletpassphrase` is up and the wallet is still rescanning.
Fixes#25702, #11249
Thanks to achow101 for coming up with the idea of using a new mutex to solve this issue and for answering related questions.
ACKs for top commit:
achow101:
ACK 6a5b348f2e
hernanmarino:
ACK 6a5b348f2e
furszy:
Tested ACK 6a5b348f
Tree-SHA512: 0b6db692714f6f94594fa47249f5ee24f85713bfa70ac295a7e84b9ca6c07dda65df7b47781a2dc73e5b603a8725343a2f864428ae20d3e126c5b4802abc4ab5
Some build infrastructure, such as Nix, will delete failed builds by default, keeping only the log of the failed build.
For flaky tests, it would be very helpful to have the PRNG seed in the default log in order to redo the failed test.
By simply raising the PRNG seed logging to INFO, we can, by default, record the seed in the log of every build.
14b4921a91 wallet: reuse change dest when recreating TX with avoidpartialspends (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/27051
When the wallet creates a transaction internally, it will also create an alternative that spends using destination groups and see if the fee difference is negligible. If it costs the user the same to send the grouped version, we send it (even if the user has `avoidpartialspends` set to `false` which is default). This patch ensures that the second transaction creation attempt re-uses the change destination selected by the first attempt. Otherwise, the first change address remains reserved, will not be used in the second attempt, and then will never be used by the wallet, leaving gaps in the BIP44 chain.
If the user had `avoidpartialspends` set to true, there is no second version of the created transaction and the change addresses are not affected.
I believe this behavior was introduced in https://github.com/bitcoin/bitcoin/pull/14582
ACKs for top commit:
achow101:
ACK 14b4921a91
Tree-SHA512: a3d56f251ff4b333fc11325f30d05513e34ab0a2eb703fadd0ad98d167ae074493df1a24068298336c6ed2da6b31aa2befa490bc790bbc260ed357c8f2397659
73ec4b2a83 tests: decodescript can infer descriptors for scripts >520 bytes (Andrew Chow)
7cc7822371 rpc: Use FlatSigningProvider in decodescript (Andrew Chow)
Pull request description:
`FillableSigningProvider` limits scripts to 520 bytes even though segwit allows scripts to be larger than that. We can avoid this limit by using a `FlatSigningProvider` so that such larger scripts can be decoded.
Fixes#27111
ACKs for top commit:
instagibbs:
ACK 73ec4b2a83
Tree-SHA512: c0e6d21025e2da864471989ac94c54e127d05459b9b048f34a0da8d76d8e372d5472a2e667ba2db74d6286e3e6faa55486ffa9232a068b519afa676394031d5a
1819564c21 test: fix intermittent issue in `p2p_disconnect_ban` (brunoerg)
Pull request description:
Fixes#26808
When `node0` calls `disconnectnode` to disconnect `node1`, we should check in `node1` if it worked, because for `node0` the informations in `getpeerinfo` may be updated before really completing the disconnection.
ACKs for top commit:
MarcoFalke:
lgtm ACK 1819564c21
Tree-SHA512: 53a386fc38e2faa6f6da3536e76857ff4b6f55e2590d73fe857b3fe5d0f3ff92c5c7e4abd50ab4be250cb2106a4d14ad95d4809ea60c6e00ed3ac0e71255b0b0
14302a4802 test: fix test abort for high timeout values (and `--timeout-factor 0`) (Sebastian Falbesoner)
Pull request description:
On master, the functional tests's option `--timeout-factor 0` (which according to the test docs and parameter description should disable the RPC timeouts) currently fails, same as high values like `--timeout-factor 999999`:
```
$ ./test/functional/wallet_basic.py --timeout-factor 0
2022-08-29T01:26:39.561000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_f24yxzp5
2022-08-29T01:26:40.262000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/honey/bitcoin/test/functional/test_framework/test_framework.py", line 549, in start_nodes
node.wait_for_rpc_connection()
File "/home/honey/bitcoin/test/functional/test_framework/test_node.py", line 234, in wait_for_rpc_connection
rpc.getblockcount()
File "/home/honey/bitcoin/test/functional/test_framework/coverage.py", line 49, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
File "/home/honey/bitcoin/test/functional/test_framework/authproxy.py", line 142, in __call__
response, status = self._request('POST', self.__url.path, postdata.encode('utf-8'))
File "/home/honey/bitcoin/test/functional/test_framework/authproxy.py", line 107, in _request
self.__conn.request(method, path, postdata, headers)
File "/usr/local/lib/python3.9/http/client.py", line 1285, in request
self._send_request(method, url, body, headers, encode_chunked)
File "/usr/local/lib/python3.9/http/client.py", line 1331, in _send_request
self.endheaders(body, encode_chunked=encode_chunked)
File "/usr/local/lib/python3.9/http/client.py", line 1280, in endheaders
self._send_output(message_body, encode_chunked=encode_chunked)
File "/usr/local/lib/python3.9/http/client.py", line 1040, in _send_output
self.send(msg)
File "/usr/local/lib/python3.9/http/client.py", line 980, in send
self.connect()
File "/usr/local/lib/python3.9/http/client.py", line 946, in connect
self.sock = self._create_connection(
File "/usr/local/lib/python3.9/socket.py", line 844, in create_connection
raise err
File "/usr/local/lib/python3.9/socket.py", line 832, in create_connection
sock.connect(sa)
OSError: [Errno 22] Invalid argument
```
This is caused by a high timeout value that Python's HTTP(S) client library can't cope with. Fix this by clamping down the connection's set timeout value in AuthProxy. The change can easily be tested by running an arbitrary test with `--timeout-factor 0` on master (should fail), on this PR (should pass) and on this PR with the clamping value increased by 1 (should fail).
// EDIT: The behaviour was observed on OpenBSD 7.1 and Python 3.9.12.
ACKs for top commit:
MarcoFalke:
lgtm ACK 14302a4802
Tree-SHA512: 6469e8ac699f1bb7dea11d5fb8b3ae54d895bb908570587c5631144cd41fe980ca0b1e6d0b7bfa07983307cba15fb26ae92e6766375672bf5be838d8e5422dbc
When `node0` calls `disconnectnode` to disconnect `node1`, we should check in `node1` if it worked, because for `node0` the informations in `getpeerinfo` may be updated before really completing the disconnection.
2555a3950f p2p: ProcessAddrFetch(-seednode) is unnecessary if -connect is specified (Dhruv Mehta)
Pull request description:
If the user runs: `bitcoind -connect=X -seednode=Y`, I _think_ it is safe to ignore `-seednode`. A more populated `addrman` (via `getaddr` calls to peers in `-seednode`) is not useful in this configuration: `addrman` entries are used to initiate new outbound connections when slots are open, or to open feeler connections and keep `addrman` from getting stale. This is all done in a part of `ThreadOpenConnections` (below [this line](https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L1803)) which is never executed when `-connect` is supplied. With `-connect`, `ThreadOpenConnections` will run [this loop](https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L1785) and exit thread execution when interrupted.
Reviewers may also find it relevant that when `-connect` is used, we [soft disable](https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L800) `-dnsseed` in init.cpp perhaps for the same reason i.e. seeding is not useful with `-connect`.
Running `ProcessAddrFetch` does not seem to have downside except developer confusion AFAICT. I was confused by this and felt it might affect other new bitcoiners too. If there is strong preference to not remove the line, I'd also be happy to just leave a comment there mentioning `ADDR_FETCH`/`-seednode` is irrelevant when used with `-connect`.
If this change is accepted, the node will still make `getaddr` calls to peers in `-connect` and expand `addrman`. However, disabling those `getaddr` calls would leak information about the node's configuration.
ACKs for top commit:
mzumsande:
Code Review ACK 2555a3950f
achow101:
ACK 2555a3950f
vasild:
ACK 2555a3950f
Tree-SHA512: 9187a0cff58db8edeca7e15379b1c121e7ebe8c38fb82f69e3dae8846ee94c92a329d79025e0f023c7579b2d86e7dbf756e4e30e90a72236bfcd2c00714180b3
30a3230e86 script: remove out-of-date snprintf TODO (Jon Atack)
0e015146bd net: remove orphaned CSubNet::SanityCheck() (Jon Atack)
Pull request description:
`CSubNet::SanityCheck()` was added in #20140, and not removed in #22570 when it became orphaned code.
Also, remove an out-of-date `snprintf` TODO that was resolved in #27036, and fix up 2 words to make the spelling linter green again.
ACKs for top commit:
fanquake:
ACK 30a3230e86
pinheadmz:
ACK 30a3230e86
brunoerg:
crACK 30a3230e86
Tree-SHA512: f91a2a5af902d3b82ab496f19deeac17d58dbf72a8016e880ea61ad858b66e7ea0ae70b964c4032018eb3252cc34ac5fea163131c6a7f1baf87fc9ec9b5833d8
4c8ecccdcd test: add tests for `outputs` argument to `bumpfee`/`psbtbumpfee` (Seibart Nedor)
c0ebb98382 wallet: add `outputs` arguments to `bumpfee` and `psbtbumpfee` (Seibart Nedor)
a804f3cfc0 wallet: extract and reuse RPC argument format definition for outputs (Seibart Nedor)
Pull request description:
This implements a modification of the proposal in #22007: instead of **adding** outputs to the set of outputs in the original transaction, the outputs given by `outputs` argument **completely replace** the outputs in the original transaction.
As noted below, this makes it easier to "cancel" a transaction or to reduce the amounts in the outputs, which is not the case with the original proposal in #22007, but it seems from the discussion in this PR that the **replace** behavior is more desirable than **add** one.
ACKs for top commit:
achow101:
ACK 4c8ecccdcd
1440000bytes:
Code Review ACK 4c8ecccdcd
ishaanam:
reACK 4c8ecccdcd
Tree-SHA512: 31361f4a9b79c162bda7929583b0a3fd200e09f4c1a5378b12007576d6b14e02e9e4f0bab8aa209f08f75ac25a1f4805ad16ebff4a0334b07ad2378cc0090103
Since migration reloads the wallet, the wallet will always be locked
unless the passphrase is given. migratewallet can now take the
passphrase in order to unlock the wallet for migration.
7a83aa0982 test: add coverage for unparsable `-maxuploadtarget` (brunoerg)
Pull request description:
This PR adds test coverage for the following error:
7386da7a0b/src/init.cpp (L1096-L1099)
Top commit has no ACKs.
Tree-SHA512: c115b2b4d2d0eb2316bf9fafd7e0046aa18c9650062779b3a82d6145d188765bff5317f4ca5f79607732fde6d83e1f67756ac20a12c98d060ee68d8acc20c76e
741908afc1 test: previous releases: add v24.0.1 (Sebastian Falbesoner)
Pull request description:
The same procedure as every release (see dba1231672 [v23.0] and d8b705f1ca [v22.0]), only a little simpler now: thanks to #25650, the previous release fetch script defaults to downloading/building the necessary tags, i.e. we don't need to extend the tag list in the CI scripts and test/README.md anymore.
ACKs for top commit:
Sjors:
tACK 741908afc1
Tree-SHA512: a5426e989bd0bba42aa13e7d4cf60f792bf36bd9a6cdb6ef5799f7574d9a8a20979244627bbd0c6219630367e7fd73bac9e677814bc50233f64592ad035e713e
6c7a17a8e0 psbt: support externally provided preimages for Miniscript satisfaction (Antoine Poinsot)
840a396029 qa: add a "smart" Miniscript fuzz target (Antoine Poinsot)
17e3547241 qa: add a fuzz target generating random nodes from a binary encoding (Antoine Poinsot)
611e12502a qa: functional test Miniscript signing with key and timelocks (Antoine Poinsot)
d57b7f2021 refactor: make descriptors in Miniscript functional test more readable (Antoine Poinsot)
0a8fc9e200 wallet: check solvability using descriptor in AvailableCoins (Antoine Poinsot)
560e62b1e2 script/sign: signing support for Miniscripts with hash preimage challenges (Antoine Poinsot)
a2f81b6a8f script/sign: signing support for Miniscript with timelocks (Antoine Poinsot)
61c6d1a844 script/sign: basic signing support for Miniscript descriptors (Antoine Poinsot)
4242c1c521 Align 'e' property of or_d and andor with website spec (Pieter Wuille)
f5deb41780 Various additional explanations of the satisfaction logic from Pieter (Pieter Wuille)
22c5b00345 miniscript: satisfaction support (Antoine Poinsot)
Pull request description:
This makes the Miniscript descriptors solvable.
Note this introduces signing support for much more complex scripts than the wallet was previously able to solve, and the whole tooling isn't provided for a complete Miniscript integration in the wallet. Particularly, the PSBT<->Miniscript integration isn't entirely covered in this PR.
ACKs for top commit:
achow101:
ACK 6c7a17a8e0
sipa:
utACK 6c7a17a8e0 (to the extent that it's not my own code).
Tree-SHA512: a71ec002aaf66bd429012caa338fc58384067bcd2f453a46e21d381ed1bacc8e57afb9db57c0fb4bf40de43b30808815e9ebc0ae1fbd9e61df0e7b91a17771cc
that was resolved in PR27036 "test: Remove last uses of snprintf and simplify"
and while here, fix up 2 words in docs to make the spelling linter green again.
dee8549be3 test: simplify and speedup mempool_updatefromblock.py by using MiniWallet (Sebastian Falbesoner)
Pull request description:
This PR simplifies the functional test mempool_updatefromblock.py by using MiniWallet in order to avoid manual low-level tx creation (signing, outputs selection, fee calculation). Most of the tedious work is done by the method `MiniWallet.send_self_transfer_multi` (calling `create_self_transfer_multi` internally) which supports spending a given set of UTXOs and creating a certain number of outputs.
As a nice side-effect, the test's performance increases significantly (~3.5x on my system):
```
master
1m56.80s real 1m50.10s user 0m06.36s system
PR
0m32.34s real 0m30.26s user 0m01.41s system
```
The arguments `start_input_txid` and `end_address` have been removed from the `transaction_graph_test` method, as they are currently unused and I don't see them being needed for future tests.
ACKs for top commit:
brunoerg:
crACK dee8549be3
MarcoFalke:
lgtm ACK dee8549be3🚏
Tree-SHA512: 9f6da634bdc8c272f9a2af1cddaa364ee371d4e95554463a066249eecebb668d8c6cb123ec8a5404c41b3291010c0c8806a8a01dd227733cec03e73aa93b0103
511aa4f1c7 Add unit test for ChaCha20's new caching (Pieter Wuille)
fb243d25f7 Improve test vectors for ChaCha20 (Pieter Wuille)
93aee8bbda Inline ChaCha20 32-byte specific constants (Pieter Wuille)
62ec713961 Only support 32-byte keys in ChaCha20{,Aligned} (Pieter Wuille)
f21994a02e Use ChaCha20Aligned in MuHash3072 code (Pieter Wuille)
5d16f75763 Use ChaCha20 caching in FastRandomContext (Pieter Wuille)
38eaece67b Add fuzz test for testing that ChaCha20 works as a stream (Pieter Wuille)
5f05b27841 Add xoroshiro128++ PRNG (Martin Leitner-Ankerl)
12ff72476a Make unrestricted ChaCha20 cipher not waste keystream bytes (Pieter Wuille)
6babf40213 Rename ChaCha20::Seek -> Seek64 to clarify multiple of 64 (Pieter Wuille)
e37bcaa0a6 Split ChaCha20 into aligned/unaligned variants (Pieter Wuille)
Pull request description:
This is an alternative to #25354 (by my benchmarking, somewhat faster), subsumes #25712, and adds additional test vectors.
It separates the multiple-of-64-bytes-only "core" logic (which becomes simpler) from a layer around which performs caching/slicing to support arbitrary byte amounts. Both have their uses (in particular, the MuHash3072 code can benefit from multiple-of-64-bytes assumptions), plus the separation results in more readable code. Also, since FastRandomContext effectively had its own (more naive) caching on top of ChaCha20, that can be dropped in favor of ChaCha20's new built-in caching.
I thought about rebasing #25712 on top of this, but the changes before are fairly extensive, so redid it instead.
ACKs for top commit:
ajtowns:
ut reACK 511aa4f1c7
dhruv:
tACK crACK 511aa4f1c7
Tree-SHA512: 3aa80971322a93e780c75a8d35bd39da3a9ea570fbae4491eaf0c45242f5f670a24a592c50ad870d5fd09b9f88ec06e274e8aa3cefd9561d623c63f7198cf2c7
772671245d test: p2p: check that headers message with invalid proof-of-work disconnects peer (Sebastian Falbesoner)
Pull request description:
One of the earliest anti-DoS checks done after receiving and deserializing a `headers` message from a peer is verifying whether the proof-of-work is valid (called in method `PeerManagerImpl::ProcessHeadersMessage`):
f227e153e8/src/net_processing.cpp (L2752-L2762)
The called method `PeerManagerImpl::CheckHeadersPoW` calls `Misbehaving` with a score of 100, i.e. leading to an immediate disconnect of the peer:
f227e153e8/src/net_processing.cpp (L2368-L2372)
This PR adds a simple test for both the misbehaving log and the resulting disconnect. For creating a block header with invalid proof-of-work, we first create one that is accepted by the node (the difficulty field `nBits` is copied from the genesis block) and based on that the nonce is modified until we have block header hash prefix that is too high to fulfill even the minimum difficulty.
ACKs for top commit:
Sjors:
ACK 772671245d
achow101:
ACK 772671245d
brunoerg:
crACK 772671245d
furszy:
Code review ACK 77267124 with a non-blocking speedup.
Tree-SHA512: 680aa7939158d1dc672b90aa6554ba2b3a92584b6d3bcb0227776035858429feb8bc66eed18b47de0fe56df7d9b3ddaee231aaeaa360136603b9ad4b19e6ac11
ab4efad51b test: fix immediate tx relay in wallet_groups.py (Sebastian Falbesoner)
Pull request description:
In the functional test wallet_groups.py we whitelist peers on all nodes (`-whitelist=noban@127.0.0.1`) to enable immediate tx relay for fast mempool synchronization. However, considering that this setting only applies to inbound peers and the default test topology looks like this:
```
node0 <--- node1 <---- node2 <--- ... <-- nodeN
```
txs propagate fast only from lower- to higher-numbered nodes (i.e. "left to right" in the above diagram) and take long from higher- to lower-numbered nodes ("right to left") since in the latter direction we only have outbound peers, where the trickle relay is still active. As a consequence, if a tx is submitted from any node other than node0, the mempool synchronization can take quite long.
This PR fixes this by simply adding another connection from node0 to the last node, leading to a ~2-3x speedup (5 runs measured via `time ./test/functional/wallet_groups.py` are shown):
```
master:
0m53.31s real 0m08.22s user 0m05.60s system
0m32.85s real 0m07.44s user 0m04.08s system
0m46.40s real 0m09.18s user 0m04.23s system
0m46.96s real 0m11.10s user 0m05.74s system
0m57.23s real 0m10.53s user 0m05.59s system
PR:
0m19.64s real 0m09.58s user 0m05.50s system
0m18.05s real 0m07.77s user 0m04.03s system
0m18.99s real 0m07.90s user 0m04.25s system
0m17.49s real 0m07.56s user 0m03.92s system
0m18.11s real 0m07.74s user 0m03.88s system
```
Note that in most tests this is not a problem since txs very often originate from node0.
ACKs for top commit:
brunoerg:
utACK ab4efad51b
Tree-SHA512: 12675357e6eb5a18383f2bfe719a184c0790863b37a98749d8e757dd5dc3a36212e16a81f0a192340c11b793eda00db359c7011f46f7c27e3a093af4f5b62147
We'll need a better integration of the hash preimages PSBT fields to
satisfy Miniscript with such challenges from the RPC.
Thanks to Greg Sanders for his examples and suggestions to improve this
test.
Try to solve a script using the Miniscript satisfier if the legacy
solver fails under P2WSH context. Only solve public key and public key
hash challenges for now.
We don't entirely replace the raw solver and especially rule out trying to
solve CHECKMULTISIG-based multisigs with the Miniscript satisfier since
some features, such as the transaction input combiner, rely on the
specific behaviour of the former.
741c215b5f test: remove unused vars in `feature_block` (brunoerg)
Pull request description:
There is no need to assign `self.next_block` to variables if we're not using its return value. Most cases touched here, we're reassigning it right after with the value from `self.update_block`.
Top commit has no ACKs.
Tree-SHA512: 25bbea2a09f38c3a3483fa363f024d2a8edd06a00cccc93cef99e489b9a3485d58bbd6a1ed2dddc00f1cebec7e63aed8ad95701a2645ce20a0db9b69573c20a7
c9ba4f9ecb test: Add test for file system permissions (Hennadii Stepanov)
581f16ef34 Apply default umask in `SetupEnvironment()` (Hennadii Stepanov)
8a6219e543 Remove `-sysperms` option (Hennadii Stepanov)
Pull request description:
On master (1e7564eca8) docs say:
```
$ ./src/bitcoind -help | grep -A 3 sysperms
-sysperms
Create new files with system default permissions, instead of umask 077
(only effective with disabled wallet functionality)
```
Basing on that, one could expect that running `bitcoind` first time will create data directory and `wallets/` subdirectory with safe 0700 permissions.
But that is not the case:
```
$ stat .bitcoin | grep id
Access: (0775/drwxrwxr-x) Uid: ( 1000/ hebasto) Gid: ( 1000/ hebasto)
$ stat .bitcoin/wallets | grep id
Access: (0775/drwxrwxr-x) Uid: ( 1000/ hebasto) Gid: ( 1000/ hebasto)
```
Both directories, in fact, are created with system default permissions.
With this PR:
```
$ stat .bitcoin/wallets | grep id
Access: (0700/drwx------) Uid: ( 1000/ hebasto) Gid: ( 1000/ hebasto)
$ stat .bitcoin/wallets | grep id
Access: (0700/drwx------) Uid: ( 1000/ hebasto) Gid: ( 1000/ hebasto)
```
---
This PR:
- is alternative to bitcoin/bitcoin#13389
- fixesbitcoin/bitcoin#15902
- fixesbitcoin/bitcoin#22595
- closesbitcoin/bitcoin#13371
- reverts bitcoin/bitcoin#4286
Changes in behavior: removed `-sysperms` command-line argument / configure option. The related discussions are here:
- https://github.com/bitcoin/bitcoin/pull/13389#issuecomment-395306690
- https://github.com/bitcoin/bitcoin/pull/13389#issuecomment-539906114
- https://github.com/bitcoin/bitcoin/pull/13389#discussion_r279160472
If users rely on non-default access permissions, they could use `chmod`.
ACKs for top commit:
john-moffett:
ACK c9ba4f9ecb
willcl-ark:
ACK c9ba4f9ecb
Tree-SHA512: 96c745339e6bd0e4d7bf65daf9a721e2e1945b2b0ab74ca0f66576d0dc358b5de8eb8cdb89fe2160f3b19c39d2798bb8b291784316085dc73a27102d3415bd57
b8032293e6 Remove use of snprintf and simplify (John Moffett)
Pull request description:
These are the only remaining uses of `snprintf` in our project, and they can cause unexpected issues -- for example, see https://github.com/bitcoin/bitcoin/issues/27014. Change them to use our `ToString` (which uses a locale-independent version of `std::to_string`) to convert an `int` to `std::string`. Also remove resulting unused parts of `StringContentsSerializer`.
Closes https://github.com/bitcoin/bitcoin/issues/27014
ACKs for top commit:
Sjors:
tACK b8032293e6, fixes#27014.
Tree-SHA512: c903977e654711929decafe8887d0de13b38a340d7082875acc5d41950d834dcfde074e9cabecaf5f9a760f62c34322297b4b156af29761650ef5803b1a54b59
One test case uses snprintf to convert an
int to a string. Change it to use ToString
(which uses a locale-independent version of
std::to_string). Also remove unnecessary
parts of StringContentsSerializer.
The descriptor inference logic would previously always use a dummy
signing provider and would never analyze the witness script of a P2WSH
scriptPubKey.
Note even a valid Miniscript might not always be decodable from Script
without more contextual information (for instance the key preimage for a
pk_h).
6d31900e52 wallet: migrate wallet, exit early if no legacy data exist (furszy)
Pull request description:
The process first creates a backup file then return an error,
without removing the recently created file, when notices that
the db is already running sqlite.
ACKs for top commit:
john-moffett:
ACK 6d31900e52
achow101:
ACK 6d31900e52
ishaanam:
crACK 6d31900e52
Tree-SHA512: 9fb52e80de96e129487ab91bef13647bc4570a782003b1e37940e2a00ca26283fd24ad39bdb63a984ae0a56140b518fd0d74aa2fc59ab04405b2c179b7d3c54a
b530d9605d test: refactor: introduce `replace_in_config` helper (Sebastian Falbesoner)
Pull request description:
Currently two functional tests (p2p_permissions.py and wallet_crosschain.py) include quite similar code for substituting strings in a TestNode's bitcoind configuration file, so refactoring that out to a dedicated helper method seems to make sense (probably other tests could need that too in the future).
ACKs for top commit:
kouloumos:
ACK b530d9605d
Tree-SHA512: 5ca65a2ef3292460e5720d5c6acf7326335001858e8f71ab054560117f9479dbadb1dacd8c9235f67f3fcfd08dbc761b62704f379cbf619bba8804f16a25bc7b
Xoroshiro128++ is a fast non-cryptographic random generator.
Reference implementation is available at https://prng.di.unimi.it/
Co-Authored-By: Pieter Wuille <pieter@wuille.net>
b0fa5989e1 test: Check that orphaned coinbase unconf spend is still abandoned (Andrew Chow)
9addbd7890 wallet: Automatically abandon orphaned coinbases and their children (Andrew Chow)
Pull request description:
When a block is reorged out of the main chain, any descendants of the coinbase will no longer be valid. Currently they are only marked as inactive, which means that our balance calculations will still include them. In order to be excluded from the balance calculation, they need to either be abandoned or conflicted. This PR goes with the abandoned method.
Note that even when they are included in balance calculations, coin selection will not select outputs belonging to these transactions because they are not in the mempool.
Fixes#14148
ACKs for top commit:
furszy:
ACK b0fa5989 with a not-blocking nit.
aureleoules:
reACK b0fa5989e1
ishaanam:
ACK b0fa5989e1
Tree-SHA512: 68f12e7aa8df392d8817dc6ac0becce8dbe83837bfa538f47027e6730e5b2e1b1a090cfcea2dc598398fdb66090e02d321d799f087020d7e1badcf96e598c3ac
39b93649c4 test: add functional test for IBD stalling logic (Martin Zumsande)
0565951f34 p2p: Make block stalling timeout adaptive (Martin Zumsande)
Pull request description:
During IBD, there is the following stalling mechanism if we can't proceed with assigning blocks from a 1024 lookahead window because all of these blocks are either already downloaded or in-flight: We'll mark the peer from which we expect the current block that would allow us to advance our tip (and thereby move the 1024 window ahead) as a possible staller. We then give this peer 2 more seconds to deliver a block (`BLOCK_STALLING_TIMEOUT`) and if it doesn't, disconnect it and assign the critical block we need to another peer.
Now the problem is that this second peer is immediately marked as a potential staller using the same mechanism and given 2 seconds as well - if our own connection is so slow that it simply takes us more than 2 seconds to download this block, that peer will also be disconnected (and so on...), leading to repeated disconnections and no progress in IBD. This has been described in #9213, and I have observed this when doing IBD on slower connections or with Tor - sometimes there would be several minutes without progress, where all we did was disconnect peers and find new ones.
The `2s` stalling timeout was introduced in #4468, when blocks weren't full and before Segwit increased the maximum possible physical size of blocks - so I think it made a lot of sense back then.
But it would be good to revisit this timeout now.
This PR makes the timout adaptive (idea by sipa):
If we disconnect a peer for stalling, we now double the timeout for the next peer (up to a maximum of 64s). If we connect a block, we half it again up to the old value of 2 seconds. That way, peers that are comparatively slower will still get disconnected, but long phases of disconnecting all peers shouldn't happen anymore.
Fixes#9213
ACKs for top commit:
achow101:
ACK 39b93649c4
RandyMcMillan:
Strong Concept ACK 39b93649c4
vasild:
ACK 39b93649c4
naumenkogs:
ACK 39b93649c4
Tree-SHA512: 85bc57093b2fb1d28d7409ed8df5a91543909405907bc129de7c6285d0810dd79bc05219e4d5aefcb55c85512b0ad5bed43a4114a17e46c35b9a3f9a983d5754
8609f24be2 test: refactor: simplify p2p_eviction.py by using MiniWallet (Sebastian Falbesoner)
7aa4b32cd4 test: refactor: simplify p2p_tx_download.py by using MiniWallet (Sebastian Falbesoner)
Pull request description:
Similar to #26892, this PR simplies the functional tests p2p_tx_download.py and p2p_eviction.py by using MiniWallet in order to avoid manual low-level tx creation. For the latter, rather than mining 100 blocks manually, the pre-mined chain of the test framework is used.
These instances were found via `$ git grep signrawtransactionwithkey ./test/functional`. AFAICT, there are no other instances where MiniWallet could replace tx creation trivially.
ACKs for top commit:
MarcoFalke:
review ACK 8609f24be2
Tree-SHA512: dfb0103fe7f0625d125e8e4408baed8bfc1ff579954af17d0ead5277e05f933b2c2d98a0093e8109e947635f1718d5c58d837ab825f26077fac0a40575bd3e6f
fafeddfe0e rpc: Throw more user friendly arg type check error (MarcoFalke)
Pull request description:
The arg type check error doesn't list which arg (position or name) failed. Fix that.
ACKs for top commit:
stickies-v:
ACK fafeddfe0e - although I think the functional test isn't in a logical place (but not blocking)
Tree-SHA512: 17425aa145aab5045940ec74fff28f0e3b2b17ae55f91c4bb5cbcdff0ef13732f8e31621d85964dc2c04333ea37dbe528296ac61be27541384b44e37957555c8
b9d5674541 init: Remove sensitive flag from rpcbind (Andrew Chow)
Pull request description:
`-rpcbind` is currently flagged as a sensitive option which means that its value will be masked when the command line args are written to the debug.log file. However this is not useful as if `rpcbind` is actually activated, the bound IP addresses will be written to the log anyways. The test `feature_config_args.py` did not catch this contradiction as the test node was not started with `rpcallowip` and so `rpcbind` was not acted upon.
This also brings `rpcbind` inline with `bind` as that is not flagged as sensitive either.
ACKs for top commit:
Sjors:
re-utACK b9d5674541
willcl-ark:
ACK b9d5674
theStack:
ACK b9d5674541
Tree-SHA512: 50ab5ad2e18ae70649deb1ac429d404b5f5c41f32a4943b2041480580152df22e72d4aae493379d0b23fcb649ab342376a82119760fbf6dfdcda659ffd3e244a
b68e5a7fef lint: specify the right commit range when running locally (James O'Beirne)
dff7ed5732 test: add an easy way to run linters locally (James O'Beirne)
Pull request description:
Adds a Dockerfile configuration ~~(originally written mostly by fanquake)~~ that allows straightforward running of linters with compatible versions locally. This removes a ton of annoyance when trying to appease CI, because many of the linter versions are quite old and difficult to maintain locally.
I realize that people may not be thrilled to add more ancillary tooling to the repo, but I think this makes a lot of sense given the linter versions listed in this container configuration are dictated by this repo (within the CI configuration), so having these things live in two separate places is a recipe for version mismatches.
Eventually we can likely just use this container on CI directly to avoid any chance of inconsistencies between local dev experience and CI.
ACKs for top commit:
aureleoules:
ACK b68e5a7fef
stickies-v:
ACK b68e5a7fe
john-moffett:
ACK b68e5a7fef
Tree-SHA512: 7ef7a5dae023d81fdb6296d5d92dfa074ee321c7993e607c9f014d0f21c91558611aa00fc3ce1edc7b5e68371aea0d27fa1931291a79bb867a6c783bb536775f
d96d97ad30 doc: Add release note for shutdownnotify. (klementtan)
0bd73e2c45 util: Add -shutdownnotify option. (klementtan)
Pull request description:
**Description**: Similar to `-startupnotify`, this PR adds a new option to allow users to specify a command to be executed when Bitcoin Core shuts down.
**Note**: The `shutdownnotify` commands will not be executed if bitcoind shut down due to *unexpected* reasons (ie `killall -9 bitcoind`).
### Testing:
**Normal shutdown commands**
```
# start bitcoind with shutdownnotify optioin
./src/bitcoind -signet -shutdownnotify="touch foo.txt"
# shutdown bitcoind
./src/bitcoin-cli -signet stop
# check that foo.txt has been created
```
**Final RPC call**
Commands:
```
$ ./src/bitcoind -signet -nolisten -noconnect -shutdownnotify="./src/bitcoin-cli -signet getblockchaininfo > tmp.txt"
$ ./src/bitcoin-cli stop
$ cat tmp.txt
```
<details>
<summary>Screen Shot</summary>
</details>
ACKs for top commit:
achow101:
ACK d96d97ad30
1440000bytes:
ACK d96d97ad30
theStack:
re-ACK d96d97ad30
Tree-SHA512: 16f7406fd232e8b97aea5e58854c84755b0c35c88cb3ef9ee123b29a1475a376122b1e100da860cc336d4d657e6046a70e915fdb9b70c9fd097c6eef1b028161
Adds a Dockerfile configuration
that allows straightforward running of linters with compatible versions
locally. This removes a ton of annoyance when trying to appease CI,
because many of the linter versions are quite old and difficult to
maintain locally.
I realize that people may not be thrilled to more ancillary tooling to
the repo, but I think this makes a lot of sense given the linter
versions listed in this container configuration are dictated by this
repo (within the CI configuration), so having these things live in
two separate places is a recipe for version mismatches.
Eventually we can likely just use this container on CI directly to avoid
any chance of inconsistencies between local dev experience and CI.
fa6b402114 test: Run mempool_packages.py with MiniWallet (MarcoFalke)
fa448c27d2 test: Return fee from MiniWallet (MarcoFalke)
faec09f240 test: Return chain of MiniWallet txs from MiniWallet chain method (MarcoFalke)
faa12d4ccd test: Refactor MiniWallet sign_tx (MarcoFalke)
fa2d82103f test: Return wtxid from create_self_transfer_multi (MarcoFalke)
Pull request description:
This allows to run the test even when no wallet is compiled in.
Also, it is a lot nicer to read now.
ACKs for top commit:
glozow:
reACK fa6b402
Tree-SHA512: de0338068fd51db01d64ce270f94fd2982a63a6de597325cd1e1f11127e9075bd4aeacace0ed76d09a2db8b962b27237cf11edb4c1fe1a01134d397f8a11bd05
603d295199 test: wallet: add coverage for `-spendzeroconfchange` setting (Sebastian Falbesoner)
50112034bc test: remove `-spendzeroconfchange` setting from mempool_limit.py (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the `-spendzeroconfchange` setting (in particular the non-default case `=0`). Note that in contrast to the name, the setting does not only apply to change outputs, but in fact to _all_ unconfirmed outputs that we sent to ourselves, i.e. we can trigger the testing path simply with a single recipient address. The first commit removes the setting from the functional test mempool_limit.py, where it doesn't have any effect, since the test was changed to use MiniWallet in commit dddca3899c.
ACKs for top commit:
brunoerg:
crACK 603d295199
Tree-SHA512: 15d9c8bd5eb37c6b228bf887eb27debee0a391c82356662785da4553ee2558e611834c3936ef7136812b46f877bab7aa5f3088bbd278b81f296bdda96cc8e1c3
fa9f6d7bcd rpc: Run type check against RPCArgs (MarcoFalke)
faf96721a6 test: Fix wrong types passed to RPCs (MarcoFalke)
Pull request description:
It seems brittle to require `RPCTypeCheck` being called inside the code logic. Without compile-time enforcement this will lead to places where it is forgotten and thus to inconsistencies and bugs. Fix this by removing the calls to `RPCTypeCheck` and doing the check internally.
The changes should be reviewed as refactoring changes. However, if they change behavior, it will be a bugfix. For example the changes here happen to also detect/fix bugs like the one fixed in commit 3b5fb6e77a.
ACKs for top commit:
ajtowns:
ACK fa9f6d7bcd
Tree-SHA512: fb2c0981fe6e24da3ca7dbc06898730779ea4e02ea485458505a281cf421015e44dad0221a04023fc547ea2c660d94657909843fc85d92b847611ec097532439
6bd098a838 test: simplify tests by using the pre-mined chain (kouloumos)
42029a7fd4 test: remove redundant blocks generation logic (kouloumos)
0377d6bb42 test: add `rescan_utxos` in MiniWallet's initialization (kouloumos)
Pull request description:
When a pre-mined blockchain is used (default behavior), it [contains coinbase outputs in blocks 76-10](07c54de550/test/functional/test_framework/test_framework.py (L809-L813)) to [the MiniWallet's default address](07c54de550/test/functional/test_framework/wallet.py (L99-L101)). That's why we always* `rescan_utxos()` after initializing the MiniWallet, in order for the MiniWallet to account for those mature UTXOs.
> The tests following this usage pattern can be seen with:
> ```git grep -n "MiniWallet(" $(git grep -le "rescan_utxos()" $(git grep -Le "self.setup_clean_chain = True"))```
**This PR adds `rescan_utxos()` inside MiniWallet's initialization to simplify usage when the MiniWallet is used with a pre-mined chain.**
### secondary changes
- *There are a few tests that use the pre-mined blockchain but do not `rescan_utxos()`, they instead generate new blocks to create mature UTXOs.
> Those were written before the `rescan_utxos()` method was introduced with https://github.com/bitcoin/bitcoin/pull/22955 (fac66d0a39) and can be seen with:
> `git grep -n "MiniWallet(" $(git grep -Le "rescan_utxos()" $(git grep -Le "self.setup_clean_chain = True"))`
>
After including `rescan_utxos()` inside MiniWallets initilization, this blocks generation logic is not needed as the MiniWallet already accounts for enough mature UTXOs to perform the tests. **Therefore the now redundant blocks generation logic is removed from those tests with the second commit.**
- The rest of the MiniWallet tests use a clean chain (`self.setup_clean_chain = True`) and can be seen with
`git grep -n "MiniWallet(" $(git grep -le "self.setup_clean_chain = True")`
From those, there are a few that start from a clean chain and then create enough mature UTXOs for the MiniWallet with this kind of logic:
07c54de550/test/functional/mempool_expiry.py (L36-L40)
**Those tests are simplified in the third commit to instead utilize the mature UTXOs of the pre-mined chain.**
ACKs for top commit:
MarcoFalke:
ACK 6bd098a838 🕷
theStack:
re-ACK 6bd098a838
Tree-SHA512: 7f9361e36910e4000c33a32efdde4449f4a8a763bb42df96de826fcde469f9362f701b8c99e2a2c482d2d5a42a83ae5ae3844fdbed187ed0ff231f386c222493
5ca7a7be76 rpc: Return accurate results for scanblocks (Aurèle Oulès)
Pull request description:
Implements #26322.
Adds a `filter_false_positives` mode to `scanblocks` to accurately verify results from blockfilters.
If the option is enabled, pre-results given by blockfilters will be filtered out again by checking vouts and vins of all transactions of the relevant blocks against the given descriptors.
### Master
```bash
./src/bitcoin-cli -testnet -named scanblocks action=start scanobjects='["addr(tb1qcxf2gv93c26s6mqz7y6etpqdf70zmn67dualgr)"]'
{
"from_height": 0,
"to_height": 2376055,
"relevant_blocks": [
"000000000001bc35077dec4104e0ab1f667ae27059bd907f9a8fac55c802ae36",
"00000000000120a9c50542d73248fb7c37640c252850f0cf273134ad9febaf61",
"0000000000000082f7af3835da8b6146b0bfb243b8842f09c495fa1e74d454ed",
"0000000000000094c32651728193bfbe91f6789683b8d6ac6ae2d22ebd3cb5d3"
]
}
```
### PR (without `filter_false_positives` mode)
Same as master
```bash
./src/bitcoin-cli -testnet -named scanblocks action=start scanobjects='["addr(tb1qcxf2gv93c26s6mqz7y6etpqdf70zmn67dualgr)"]' filter_false_positives=false
{
"from_height": 0,
"to_height": 2376055,
"relevant_blocks": [
"000000000001bc35077dec4104e0ab1f667ae27059bd907f9a8fac55c802ae36",
"00000000000120a9c50542d73248fb7c37640c252850f0cf273134ad9febaf61",
"0000000000000082f7af3835da8b6146b0bfb243b8842f09c495fa1e74d454ed",
"0000000000000094c32651728193bfbe91f6789683b8d6ac6ae2d22ebd3cb5d3"
]
}
```
### PR (with `filter_false_positives` mode)
```bash
./src/bitcoin-cli -testnet -named scanblocks action=start scanobjects='["addr(tb1qcxf2gv93c26s6mqz7y6etpqdf70zmn67dualgr)"]' filter_false_positives=true
{
"from_height": 0,
"to_height": 2376058,
"relevant_blocks": [
"0000000000000082f7af3835da8b6146b0bfb243b8842f09c495fa1e74d454ed",
"0000000000000094c32651728193bfbe91f6789683b8d6ac6ae2d22ebd3cb5d3"
]
}
```
Also adds a test to check that the blockhash of a transaction will be included in the `relevant_blocks` whether the `filter_false_positives` mode is enabled or not.
ACKs for top commit:
achow101:
ACK 5ca7a7be76
theStack:
re-ACK 5ca7a7be76
furszy:
Code review ACK 5ca7a7be
Tree-SHA512: e8f3cceddddd66f59509717b6314d89e2fef241e13cee81b18fd95e8362cbb95cc40f884342ce6cf892a86febd9e2d434afce05d51892240e67f72ae991852e7
cfe5aebc79 rpc: add minconf and maxconf options to sendall (ishaanam)
a07a413466 Wallet/RPC: Allow specifying min & max chain depth for inputs used by fund calls (Juan Pablo Civile)
Pull request description:
This PR adds a "minconf" option to `fundrawtransaction`, `walletcreatefundedpsbt`, and `sendall`.
Alternative implementation of #14641Fixes#14542
Edit: This PR now also adds this option to `send`
ACKs for top commit:
achow101:
ACK cfe5aebc79
Xekyo:
ACK cfe5aebc79
furszy:
diff ACK cfe5aebc, only a non-blocking nit.
Tree-SHA512: 836e610926eec3a62308fba88ddbd6a13d8f4dac37352d0309599f893cde9c1df5e9c298fda6e076493068e4d213e4afa7290a9e3bdb5a95a5d507da3f7b59e8
4159ccd031 test: Run feature_bip68_sequence.py with MiniWallet (Miles Liu)
fc0caaf4aa test: Add "include mempool" flag to MiniWallet rescan_utxos (Miles Liu)
d0a909ae54 test: Add "include immature coinbase" flag to MiniWallet get_utxos (Miles Liu)
e5b9127d9e test: Add signs P2TR and RAWSCRIPT to MiniWallet sign_tx (Miles Liu)
Pull request description:
This PR enables one more of the non-wallet functional tests (feature_bip68_sequence.py) to be run even when no wallet is compiled in by using the MiniWallet instead, as proposed in #20078.
ACKs for top commit:
achow101:
ACK 4159ccd031
MarcoFalke:
review ACK 4159ccd031🤸
Tree-SHA512: e891b189381e961c840b45fc30d058363707fd54c1c4bdc3d29623b03309981f1d3ebfac27e6aecf621bdbcd7e31754a3ef7c53f86346f7dd241c137e64c92bd
