Age | Commit message (Collapse) | Author |
|
|
|
interface
25ef049d60535ac02508ba71ef60f17d8349f120 log: mempool: log removal reason in validation interface (James O'Beirne)
Pull request description:
Currently the exact reason a transaction is removed from the mempool isn't logged. It is sometimes detectable from context, but adding the `reason` to the validation interface logs (where it is already passed) seems like an easy way to disambiguate.
For example in the case of mempool expiry, the logs look like this:
```
[validationinterface.cpp:220] [TransactionRemovedFromMempool] [validation] Enqueuing TransactionRemovedFromMempool: txid=<txid> wtxid=<wtxid>
[txmempool.cpp:1050] [RemoveUnbroadcastTx] [mempool] Removed <txid> from set of unbroadcast txns before confirmation that txn was sent out
[validationinterface.cpp:220] [operator()] [validation] TransactionRemovedFromMempool: txid=<txid> wtxid=<wtxid>
[validation.cpp:267] [LimitMempoolSize] [mempool] Expired 1 transactions from the memory pool
```
There is no context-free way to know $txid was evicted on the basis of expiry. This change will make that case (and probably others) clear.
ACKs for top commit:
0xB10C:
ACK 25ef049d60535ac02508ba71ef60f17d8349f120
Tree-SHA512: 9890f9fa16f66c8a9296798d8c28993e1b81da17cf592946f2abc22041f0b30b0911ab86a0c48d4aa46b9a8b3f7f5de67778649ac48c97740b0a09aa6816e0af
|
|
`change_type` parameters
c3b1fe59dbc7abe45973e282cddf3677514e220f rpc: doc: add missing option "bech32m" for `change_type` parameters (Sebastian Falbesoner)
Pull request description:
Affects the help of the `fundrawtransaction`, `send` and `walletcreatefundedpsbt` RPCs.
This was found by manually inspecting the results of `$ git grep p2sh-segwit.*bech32`.
ACKs for top commit:
achow101:
ACK c3b1fe59dbc7abe45973e282cddf3677514e220f
Tree-SHA512: a3f1f8fde5905c80e1b95bd042ca0bc73d08c1c0e79c52ab0d6d12d7afdd4aa288afb41e12279fcea328a396f3d0a5564018170c0a11c5aa26dc6d44d2a62b1c
|
|
wallets that only have corresponding keys
0de30ed509a9969cb254e00097671625c9e107d2 tests: Test Taproot PSBT signing with keys in other descriptor (Andrew Chow)
6efcdf6b7f6daa83b5937aa630fce358fdaed333 tests: Use new wallets for each test in wallet_taproot.py (Andrew Chow)
8781a1b6bbd0af3cfdf1421fd18de5432494619a psbt: Include output pubkey in additional pubkeys to sign (Andrew Chow)
323890d0d7db2628f9dc6eaeba6e99ce0a12e1f5 sign: Fill in taproot pubkey info for all script path sigs (Andrew Chow)
Pull request description:
A user reported on [stackexchange](https://bitcoin.stackexchange.com/q/115742/48884) that they were unable to sign for a `multi_a` script using a wallet that only had the corresponding keys (i.e. it did not have the `multi_a()` descriptor). This PR fixes this issue.
Additionally, `wallet_taproot.py` is modified to test for this scenario by having another wallet in `do_test_psbt` which contains descriptors that only have the keys involved in the descriptor being tested. `wallet_taproot.py` was also modified to create new wallets for each test case rather than sharing wallets throughout as the sharing could result in the signing wallet having the keys in a different descriptor and accidentally result in failing to detect a test failure.
The changes to the test also revealed a similar issue with `rawtr()` descriptors, which has also been fixed by checking if a descriptor can produce a `SigningProvider` for the Taproot output pubkey.
ACKs for top commit:
instagibbs:
crACK https://github.com/bitcoin/bitcoin/pull/26418/commits/0de30ed509a9969cb254e00097671625c9e107d2
darosior:
ACK 0de30ed509a9969cb254e00097671625c9e107d2
Tree-SHA512: 12e131dd8afd93da7b1288c9054de2415a228d4477b97102da3ee4e82ce9de20b186260c3085a4b7b067bd8b74400751dcadf153f113db83abc59e7466e69f14
|
|
Currently the exact reason a transaction is removed from the mempool isn't
logged. It is sometimes detectable from context, but adding the `reason` to
the validation interface logs (where it is already passed) seems like an easy
way to disambiguate.
For example, in the case of mempool expiry, the logs look like this:
```
[validationinterface.cpp:220] [TransactionRemovedFromMempool] [validation] Enqueuing TransactionRemovedFromMempool: txid=<txid> wtxid=<wtxid>
[txmempool.cpp:1050] [RemoveUnbroadcastTx] [mempool] Removed <txid> from set of unbroadcast txns before confirmation that txn was sent out
[validationinterface.cpp:220] [operator()] [validation] TransactionRemovedFromMempool: txid=<txid> wtxid=<wtxid>
[validation.cpp:267] [LimitMempoolSize] [mempool] Expired 1 transactions from the memory pool
```
There is no context-free way to know $txid was evicted on the basis of expiry.
This change will make that case (and probably others) clear.
|
|
Affects the help of the `fundrawtransaction`, `send` and
`walletcratefundedpsbt` RPCs.
|
|
Assert()
fa3ea81c3e7d962715788ab5525958a532c51414 refactor: Add LIFETIMEBOUND / -Wdangling-gsl to Assert() (MacroFake)
Pull request description:
Currently compiles clean, but I think it may still be useful.
Can be tested by adding an `&`:
```diff
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 5766fff92d..300c1ec60f 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -125,7 +125,7 @@ BOOST_AUTO_TEST_CASE(util_check)
// Check -Wdangling-gsl does not trigger when copying the int. (It would
// trigger on "const int&")
- const int nine{*Assert(std::optional<int>{9})};
+ const int& nine{*Assert(std::optional<int>{9})};
BOOST_CHECK_EQUAL(9, nine);
}
```
Output:
```
test/util_tests.cpp:128:29: warning: object backing the pointer will be destroyed at the end of the full-expression [-Wdangling-gsl]
const int& nine{*Assert(std::optional<int>{9})};
^~~~~~~~~~~~~~~~~~~~~
./util/check.h:75:50: note: expanded from macro 'Assert'
#define Assert(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val)
^~~
1 warning generated.
ACKs for top commit:
jonatack:
ACK fa3ea81c3e7d962715788ab5525958a532c51414
theuni:
ACK fa3ea81c3e7d962715788ab5525958a532c51414
Tree-SHA512: 17dea4d75f2ee2bf6e1b6a6f6d8f439711c777df0390574e8d8edb6ac9ee807a135341e4439050bd6a15ecc4097a1ba9a7ab15d27541ebf70a4e081fa6871877
|
|
fa24239a1c2281f61ab70a62228e88f4c7e72701 net: Avoid SetTxRelay for feeler connections (MacroFake)
Pull request description:
Seems odd to reserve memory for the struct (the heaviest member being `m_tx_inventory_known_filter`) when it is never used.
This also avoids sending out `msg_sendtxrcncl` before disconnecting. This shouldn't matter, as other messages, such as `msg_wtxidrelay`, `msg_sendaddrv2`, `msg_verack` or `msg_getaddr` are still sent. Though, it allows to test the changes here as a side-effect.
ACKs for top commit:
naumenkogs:
ACK fa24239a1c2281f61ab70a62228e88f4c7e72701
vasild:
ACK fa24239a1c2281f61ab70a62228e88f4c7e72701
jonatack:
ACK fa24239a1c2281f61ab70a62228e88f4c7e72701
mzumsande:
ACK fa24239a1c2281f61ab70a62228e88f4c7e72701
Tree-SHA512: d7604c7eb4df8f2de811e600bdd312440ee03e508d3a0f09ae79f7f2d3eeec663bfd47a2d079fa50b756d61e35dfa998de068a7b9afaf35378fa0e62a538263d
|
|
handle gracefully
e049fd76f0d57c1e6400fbfbaf4cc6ebe540f16f Bugfix: Check for readlink buffer overflow and handle gracefully (Luke Dashjr)
Pull request description:
If readlink returns the size of the buffer, an overflow may have (safely) occurred.
Pass a buffer size of MAX_PATH+1 (the size of the actual buffer) to detect this scenario.
ACKs for top commit:
hebasto:
ACK e049fd76f0d57c1e6400fbfbaf4cc6ebe540f16f.
Tree-SHA512: 188bace79cbe556efe7782e46b870c02729b07b104a9316b0f7d50013504972e85baf507403d2d6060bb2bf3e13f40d735bddd18255d97a60810208c3de87691
|
|
afbcd227dd0889002eeeec6148538ed2e2c9aadb build: remove threadinterrupt from libbitcoinkernel (fanquake)
Pull request description:
Extracted from #26292.
ACKs for top commit:
hebasto:
ACK afbcd227dd0889002eeeec6148538ed2e2c9aadb, tested on Ubuntu 22.04.
ryanofsky:
Code review ACK afbcd227dd0889002eeeec6148538ed2e2c9aadb
Tree-SHA512: 9d355f0e417561be41cdd0674a8f94c9ffe3ecfb4063bb9c90f1032cb9d471be11d4fa26de40993e3a411e015272201551fbbb3d3c2b43e4c17bf49386a2741c
|
|
3a0b352c63db543833e0f919a004cf2c5093fe9c refactor: move url.h/cpp from lib util to lib common (fanquake)
058eb69ce47c17205a41fc667ce66429f09a332a build: add missing event cflags to libbitcoin_util (fanquake)
Pull request description:
Move `util/url` to `common/url`.
Also add missing `event_*` flags to `libbitcoin_util`. #26293 + the commit dropping boost cppflags from `libbitcoin_util` shows this issue. i.e:
```bash
CXX util/libbitcoin_util_a-url.o
util/url.cpp:7:10: fatal error: 'event2/http.h' file not found
#include <event2/http.h>
^~~~~~~~~~~~~~~
1 error generated.
```
ACKs for top commit:
hebasto:
ACK 3a0b352c63db543833e0f919a004cf2c5093fe9c
ryanofsky:
Code review ACK 3a0b352c63db543833e0f919a004cf2c5093fe9c
Tree-SHA512: 600a76fd334267a02d332df9b67891a38d3fd7f5baf8a82b2447879b3bc65eab2552d2c081c0a5f1ec927bf80df7fc1f0cbbdda4cb76994b46dadf260b8e1cb3
|
|
e9f1d8c27261070d209a28570ad36fe91531cdd2 Squashed 'src/minisketch/' changes from 47f0a2d26f..a571ba20f9 (Hennadii Stepanov)
Pull request description:
Contains:
- https://github.com/sipa/minisketch/pull/68
- https://github.com/sipa/minisketch/pull/69
- https://github.com/sipa/minisketch/pull/70
- https://github.com/sipa/minisketch/pull/72
Fixes bitcoin/bitcoin#26262.
Required for bitcoin/bitcoin#25797.
Guix builds on `arm64`:
```
28af3bdd17924bb5279934add1fe34f5664181a1c99f33793725adc35f90d778 guix-build-45a0f4e01448/output/aarch64-linux-gnu/SHA256SUMS.part
c9cdcb107de5b683221b0e2a3796814548414cdba27be198516a2b401ada90c3 guix-build-45a0f4e01448/output/aarch64-linux-gnu/bitcoin-45a0f4e01448-aarch64-linux-gnu-debug.tar.gz
ce1e916cc7574f555c04ed5fa9e9a47a1e4d2d9d1cb82a040269e8c32aca9194 guix-build-45a0f4e01448/output/aarch64-linux-gnu/bitcoin-45a0f4e01448-aarch64-linux-gnu.tar.gz
26404d3479108bcb8ede4ae7f3d37dbb64a335f0cd5bce7ee901d171a1ef2b0b guix-build-45a0f4e01448/output/arm-linux-gnueabihf/SHA256SUMS.part
0fe32aee5836baed86ad25cbddb43c5a9d2ecbbe5cc954f3a76670aa52ff3c6f guix-build-45a0f4e01448/output/arm-linux-gnueabihf/bitcoin-45a0f4e01448-arm-linux-gnueabihf-debug.tar.gz
41f312cbf26a0e51e6867e644beb3d085ebd4221dc83ea84a97b708e6d230b6d guix-build-45a0f4e01448/output/arm-linux-gnueabihf/bitcoin-45a0f4e01448-arm-linux-gnueabihf.tar.gz
c05ba8235e05c171ecdbacba87dfc1db34d1971286b853c26e3de52a6b0ea1cc guix-build-45a0f4e01448/output/arm64-apple-darwin/SHA256SUMS.part
9d1dae2b73573d34950684269c41f2504761f2df19f24033513f6664b70b1aad guix-build-45a0f4e01448/output/arm64-apple-darwin/bitcoin-45a0f4e01448-arm64-apple-darwin-unsigned.dmg
1ff742346bd04b7edf27e096286f4863fcfec984b99caf265a64fe72b298b36a guix-build-45a0f4e01448/output/arm64-apple-darwin/bitcoin-45a0f4e01448-arm64-apple-darwin-unsigned.tar.gz
74a001f16ba382c22acb9979ee5d905af42f4ca3613fc6e8fa66fec994fb0850 guix-build-45a0f4e01448/output/arm64-apple-darwin/bitcoin-45a0f4e01448-arm64-apple-darwin.tar.gz
ec2f70a0b8a128d3ca69307150714c400620199b506a411b0a4c0a506cc3ed66 guix-build-45a0f4e01448/output/dist-archive/bitcoin-45a0f4e01448.tar.gz
9a67ea74e1b5ef3d08b6a59217ecf8d193234c76f81157192fabe7a9b679af0c guix-build-45a0f4e01448/output/powerpc64-linux-gnu/SHA256SUMS.part
95b458427d73ed81f4a4f85be0dfe81a9a48dc254be65ef158de93d32f07a17a guix-build-45a0f4e01448/output/powerpc64-linux-gnu/bitcoin-45a0f4e01448-powerpc64-linux-gnu-debug.tar.gz
2b269c630f069cc42da5f361d13f5b684fbacb49641f44ed6687379a1e9281b0 guix-build-45a0f4e01448/output/powerpc64-linux-gnu/bitcoin-45a0f4e01448-powerpc64-linux-gnu.tar.gz
33d576b9be022108db6d76c41c1f72c865b5bb38439c6da1cbceac757a1be47f guix-build-45a0f4e01448/output/powerpc64le-linux-gnu/SHA256SUMS.part
053dc9cecc978601701459109eec82f3bcce4042d794c520fd4bd8cd4e17eff3 guix-build-45a0f4e01448/output/powerpc64le-linux-gnu/bitcoin-45a0f4e01448-powerpc64le-linux-gnu-debug.tar.gz
32946d589103c763c2fc67c0d8fa13a38b9194df76cd31755a761a7b9ad856a6 guix-build-45a0f4e01448/output/powerpc64le-linux-gnu/bitcoin-45a0f4e01448-powerpc64le-linux-gnu.tar.gz
7d1be372c84f9403333462e82d7800dfb1812b14c1ed228dacf605dff9aab75c guix-build-45a0f4e01448/output/riscv64-linux-gnu/SHA256SUMS.part
3338bcaf6b8ffd9c155c055a0a18d039e1173212e128561fdea493c263d6ecb9 guix-build-45a0f4e01448/output/riscv64-linux-gnu/bitcoin-45a0f4e01448-riscv64-linux-gnu-debug.tar.gz
7735eefa84630d16d8d37ea23c734578c6fb6540d4359b78d0d1508887e76042 guix-build-45a0f4e01448/output/riscv64-linux-gnu/bitcoin-45a0f4e01448-riscv64-linux-gnu.tar.gz
c3a96d6758631b8e8b4f5bb8035e5442e1fc5b831c7a0ddedb26473b025b2503 guix-build-45a0f4e01448/output/x86_64-apple-darwin/SHA256SUMS.part
b6c87738e6e561bfc4052464e93a6d03a87c51e8f27e4df8ae7458c9828e28cb guix-build-45a0f4e01448/output/x86_64-apple-darwin/bitcoin-45a0f4e01448-x86_64-apple-darwin-unsigned.dmg
0c16f9435c3d572970400ff4683705b297eda3dd37a682619371771da8efbb24 guix-build-45a0f4e01448/output/x86_64-apple-darwin/bitcoin-45a0f4e01448-x86_64-apple-darwin-unsigned.tar.gz
22e06590afe07b992f6f4e46d5568499403937606d16540b121f23e133270560 guix-build-45a0f4e01448/output/x86_64-apple-darwin/bitcoin-45a0f4e01448-x86_64-apple-darwin.tar.gz
56ddb44760f71fd1c1ea06c7174bf035b8b73c32569d69a1401d130551db43e5 guix-build-45a0f4e01448/output/x86_64-linux-gnu/SHA256SUMS.part
1c8a27fab5d9d0f3a5449f04ae8a7e693c5f144948f6d0c2dbf397e04ce9ebfa guix-build-45a0f4e01448/output/x86_64-linux-gnu/bitcoin-45a0f4e01448-x86_64-linux-gnu-debug.tar.gz
66aca3767407f6c2e77b8b93edc58e71c00bed518a75362e3c2f5009739c666c guix-build-45a0f4e01448/output/x86_64-linux-gnu/bitcoin-45a0f4e01448-x86_64-linux-gnu.tar.gz
7ac04c1d5f2dbb6c2c392add9c4e2c5c838c718910b920fe6691a5e9881065b9 guix-build-45a0f4e01448/output/x86_64-w64-mingw32/SHA256SUMS.part
2c51afbcf5645516d01d5f730e9883d9ed3cb90b9b9baf90792ecdb2b3eb21cf guix-build-45a0f4e01448/output/x86_64-w64-mingw32/bitcoin-45a0f4e01448-win64-debug.zip
81738a0690e954d6850f210fa3933ca5de8800078734f8b0810b5e7fe3e63e5d guix-build-45a0f4e01448/output/x86_64-w64-mingw32/bitcoin-45a0f4e01448-win64-setup-unsigned.exe
ecade9ee23ad156e057d466afe79943f008de316973738efdf5ae33d2a527631 guix-build-45a0f4e01448/output/x86_64-w64-mingw32/bitcoin-45a0f4e01448-win64-unsigned.tar.gz
3b17762d01825a3f7d5aecf34cf038938d9ee467e63ea3f34efd1dfa2af5e284 guix-build-45a0f4e01448/output/x86_64-w64-mingw32/bitcoin-45a0f4e01448-win64.zip
```
ACKs for top commit:
fanquake:
ACK 45a0f4e014484695a00c30d792a6ea514d42f04e
Tree-SHA512: e48d79aa0e3563aeae981ddf1281420fff9b739af43e3d77f13cace158724ff01a7622762aadfd093dcd18822a2c1b76d1c548ae64faf727ba93044887b560fb
|
|
784b02319128988038d4bd82f05736be22f14ee9 [net processing] Simplify use of IsContinuationOfLowWorkHeadersSync in TryLowWorkHeaderSync (dergoegge)
e891aabf5a4992a65b9c5ae8606f8dd08515b310 [net processing] Fixup TryLowWorkHeadersSync comment (dergoegge)
Pull request description:
See https://github.com/bitcoin/bitcoin/pull/26355#discussion_r1003561481 and https://github.com/bitcoin/bitcoin/pull/26355#discussion_r1004554187
ACKs for top commit:
hernanmarino:
ACK 784b02319128988038d4bd82f05736be22f14ee9
brunoerg:
crACK 784b02319128988038d4bd82f05736be22f14ee9
mzumsande:
ACK 784b02319128988038d4bd82f05736be22f14ee9
Tree-SHA512: b47ac0d78a09ca3a1806e38c5d2e2fcf1e5f0668f202450b5079c5cb168e168ac6828c0948d23f3610696375134986d75ef3c6098858173023bcb743aec8004c
|
|
180eac0f73b7d96861f9626ebb701d9a46585a5b Fix: typos (omahs)
Pull request description:
Fix: typos
ACKs for top commit:
aureleoules:
ACK 180eac0f73b7d96861f9626ebb701d9a46585a5b
Tree-SHA512: 23150f3408a2eb9cb298c6add16d1bcb149da277238786f053c2896c803f896b07555b3dc71e15cd8d390023800c8e006f931f415f887bab69ee5bd4b0420581
|
|
|
|
The fact that this is missing is currently masked by the inclusion of
BOOST_CPPFLAGS.
|
|
In addition to the pubkeys in hd_keypaths and tap_bip32_keypaths, also
see if the descriptor can produce a SigningProvider for the output
pubkey.
Also slightly refactors this area to reduce code duplication.
|
|
Taproot pubkey info was not being added for multi_a signing. The filling
of this info is moved into the common function CreateTaprootScriptSig so
that any signing of taproot scripts will include the pubkey info.
|
|
Wmissing-field-initializers in ChainstateManagerOpts
fa29ef00adac6f0842acdd38344820a1ce0e3087 refactor: Silence GCC Wmissing-field-initializers in ChainstateManagerOpts (MacroFake)
Pull request description:
The `std::optional` fields in the struct that fall back to chain param defaults if not provided should be initialized to `std::nullopt`. This already happens with the current code.
However, for consistency with `check_block_index` and to silence a GCC warning, add the "missing" `{}`.
ACKs for top commit:
achow101:
ACK fa29ef00adac6f0842acdd38344820a1ce0e3087
hebasto:
ACK fa29ef00adac6f0842acdd38344820a1ce0e3087, tested on Ubuntu 22.04 + GCC 11.3.
jonatack:
ACK fa29ef00adac6f0842acdd38344820a1ce0e3087
Tree-SHA512: bdec9c56df5d601a5616e107fed48737b13b0a7242b6526092fb682b5016544a4bc08666b60304c668d44c6f7ac69d3788093d921382c1d6c577c1f9fe31fc50
|
|
locale and platform agnostic
884304e6c6a250719a64752fe52d1283cd5e36ab test: Make `system_tests/run_command` locale agnostic (Hennadii Stepanov)
Pull request description:
Fixes bitcoin/bitcoin#26368.
ACKs for top commit:
Sjors:
tACK 884304e6c6a250719a64752fe52d1283cd5e36ab
Tree-SHA512: 76d4941e02b3b119dcf4dacbe60ef45a9dc8cf775bdb31b5291cd8147665285d41caaf1f5688abdfc9a47c393ddb535af7b11af839660d30ef30f1ca0d936133
|
|
|
|
Fix: typos
Fix: typos
Fix: typos
|
|
pre-set-inputs fetching responsibility from Coin Selection
3fcb545ab26be3e785b5e5654be0bdc77099d827 bench: benchmark transaction creation process (furszy)
a8a75346d7e7247596c8a580d65ceaad49c97b97 wallet: SelectCoins, return early if target is covered by preset-inputs (furszy)
f41712a734dc119f8a5e053a9cfa1f0411b5e8f1 wallet: simplify preset inputs selection target check (furszy)
5baedc33519661af9d19efcefd23dca8998d2547 wallet: remove fetch pre-selected-inputs responsibility from SelectCoins (furszy)
295852f61998a025b0b28a0671e6e1cf0dc08d0d wallet: encapsulate pre-selected-inputs lookup into its own function (furszy)
37e7887cb4bfd7db6eb462ed0741c45aea22a990 wallet: skip manually selected coins from 'AvailableCoins' result (furszy)
94c0766b0cd1990c1399a745c88c2ba4c685d8d1 wallet: skip available coins fetch if "other inputs" are disallowed (furszy)
Pull request description:
#### # Context (Current Flow on Master)
In the transaction creation process, in order to select which coins the new transaction will spend,
we first obtain all the available coins known by the wallet, which means walking-through the
wallet txes map, gathering the ones that fulfill certain spendability requirements in a vector.
This coins vector is then provided to the Coin Selection process, which first checks if the user
has manually selected any input (which could be internal, aka known by the wallet, or external),
and if it does, it fetches them by searching each of them inside the wallet and/or inside the
Coin Control external tx data.
Then, after finding the pre-selected-inputs and gathering them in a vector, the Coin Selection
process walks-through the entire available coins vector once more just to erase coins that are
in both vectors. So the Coin Selection process doesn’t pick them twice (duplicate inputs inside
the same transaction).
#### # Process Workflow Changes
Now, a new method, `FetchCoins` will be responsible for:
1) Lookup the user pre-selected-inputs (which can be internal or external).
2) And, fetch the available coins in the wallet (excluding the already fetched ones).
Which will occur prior to the Coin Selection process. Which allows us to never include the
pre-selected-inputs inside the available coins vector in the first place, as well as doing other
nice improvements (written below).
So, Coin Selection can perform its main responsibility without mixing it with having to fetch
internal/external coins nor any slow and unneeded duplicate coins verification.
#### # Summarizing the Improvements:
1) If any pre-selected-input lookup fail, the process will return the error right away.
(before, the wallet was fetching all the wallet available coins, walking through the
entire txes map, and then failing for an invalid pre-selected-input inside SelectCoins)
2) The pre-selected-inputs lookup failure causes are properly described on the return error.
(before, we were returning an "Insufficient Funds" error for everything, even if the failure
was due a not solvable external input)
3) **Faster Coin Selection**: no longer need to "remove the pre-set inputs from the available coins
vector so that Coin Selection doesn't pick them" (which meant to loop-over the entire
available coins vector at Coin Selection time, erasing duplicate coins that were pre-selected).
Now, the available coins vector, which is built after the pre-selected-inputs fetching,
doesn’t include the already selected inputs in the first place.
4) **Faster transaction creation** for transactions that only use manually selected inputs.
We now will return early, as soon as we finish fetching the pre-selected-inputs and
not perform the resources expensive calculation of walking-through the entire wallet
txes map to obtain the available coins (coins that we will not use).
---------------------------
Added a new bench (f6d0bb2) measuring the transaction creation process, for a wallet with ~250k UTXO, only using the pre-selected-inputs inside coin control. Setting `m_allow_other_inputs=false` to disallow the wallet to include coins automatically.
#### Result on this PR (tip f6d0bb2d):
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 1,048,675.00 | 953.58 | 0.3% | 0.06 | `WalletCreateTransaction`
vs
#### Result on master (tip 4a4289e2):
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 96,373,458.20 | 10.38 | 0.2% | 5.30 | `WalletCreateTransaction`
The benchmark took to run in master: **96.37 milliseconds**, while in this PR: **1 millisecond** 🚀 .
ACKs for top commit:
S3RK:
Code Review ACK 3fcb545ab26be3e785b5e5654be0bdc77099d827
achow101:
ACK 3fcb545ab26be3e785b5e5654be0bdc77099d827
aureleoules:
reACK 3fcb545ab26be3e785b5e5654be0bdc77099d827
Tree-SHA512: 42f833e92f40c348007ca565a4c98039e6f1ff25d8322bc2b27115824744779baf0b0a38452e4e2cdcba45076473f1028079bbd0f670020481ec5d3db42e4731
|
|
`list{transactions, sinceblock}` response
eb679a7896ce00e322972a011b023661766923b9 rpc: make `address` field optional (w0xlt)
Pull request description:
Close https://github.com/bitcoin/bitcoin/issues/26338.
This PR makes optional the `address` field in the response of `listtransactions` and `listsinceblock` RPC.
And adds two tests that fail on master, but not on this branch.
ACKs for top commit:
achow101:
ACK eb679a7896ce00e322972a011b023661766923b9
aureleoules:
ACK eb679a7896ce00e322972a011b023661766923b9
Tree-SHA512: b267439626e2ec3134ae790c849949a4c40ef0cebd20092e8187be3db0a61941b2da10bbbba92ca880b8369f46c1aaa806d057eaa5159325f65cbec7cb33c52f
|
|
|
|
b8b59ff9fea69f4f25d98005e3ac9172a7a11c12 gui: update the screen after loading wallet (w0xlt)
Pull request description:
Currently, the user loads a wallet and the screen does not switch to the selected wallet after loading (File -> Open Wallet -> wallet name).
This PR changes that by making the `OpenWalletActivity::opened` signal connection a `Qt::QueuedConnection` type.
ACKs for top commit:
jarolrod:
ACK b8b59ff9fea69f4f25d98005e3ac9172a7a11c12
hebasto:
ACK b8b59ff9fea69f4f25d98005e3ac9172a7a11c12, tested on Ubuntu 22.04.
Tree-SHA512: 43cd755638b643f481014a7933a0af25df2d109e859cb5f878bc04e562950d550716fa38465140060e28526b2441688580cbcbe4ec6819566b4f95162ca5e527
|
|
0cc23fc60374512b3c4be888d98a7dbdd3a0c931 Fix typo in comment SHA256->SHA512 (Elichai Turkel)
Pull request description:
The comment says it's the SHA-256 state, while it's actually the SHA-512 state
ACKs for top commit:
andrewtoth:
ACK 0cc23fc60374512b3c4be888d98a7dbdd3a0c931
aureleoules:
ACK 0cc23fc60374512b3c4be888d98a7dbdd3a0c931
Tree-SHA512: 4e390ceefb847d3bbe4f5caab390a4fdd14892fe443f58c32b08b3444fccd611cff22938c3dfa611dfd2497736f779fae4165497b4208e48aa8fc9d2236f943b
|
|
Goal 1:
Benchmark the transaction creation process for pre-selected-inputs only.
Setting `m_allow_other_inputs=false` to disallow the wallet to include coins automatically.
Goal 2:
Benchmark the transaction creation process for pre-selected-inputs and coin selection.
-----------------------
Benchmark Setup:
1) Generates a 5k blockchain, loading the wallet with 5k transactions with two outputs each.
2) Fetch 4 random UTXO from the wallet's available coins and pre-select them as inputs inside CoinControl.
Benchmark (Goal 1):
Call `CreateTransaction` providing the coin control, who has set `m_allow_other_inputs=false` and
the manually selected coins.
Benchmark (Goal 2):
Call `CreateTransaction` providing the coin control, who has set `m_allow_other_inputs=true` and
the manually selected coins.
|
|
|
|
we are already computing the preset inputs total amount inside `PreSelectedInputs::Insert`,
which internally decides whether to use the effective value or the raw output value based on
the 'subtract_fee_outputs' flag.
|
|
so if there is an error in any of the pre-set coins, we can fail right away
without computing the wallet available coins set (calling `AvailableCoins`)
which is a slow operation as it goes through the entire wallet's txes map.
----------------------
And to make the Coin Selection flow cleared, have decoupled SelectCoins in two functions:
1) AutomaticCoinSelection.
2) SelectCoins.
1) AutomaticCoinSelection:
Receives a set of coins and selects the best subset of them to
cover the target amount.
2) SelectCoins
In charge of select all the user manually selected coins first ("pre-set inputs"), and
if coin_control 'm_allow_other_inputs=true', call 'AutomaticCoinSelection' to select a
subset of coins owned by the wallet to cover for the target - preset_inputs.total_amount
remaining value.
|
|
First step towards decoupling the pre-selected-inputs fetching functionality
from `SelectCoins`. Which, will let us not waste resources calculating the
available coins if one of the pre-set inputs has an error.
(right now, if one of the pre-set inputs is invalid, we first walk through
the entire wallet txes map just to end up failing right after it finish)
|
|
No need to walk through the entire wallet's txes map just to get
coins that we could have gotten by just doing a simple map.find(out.hash).
(Which is what we are doing inside `SelectCoins` anyway)
|
|
no need to waste resources calculating the wallet available coins if
they are not going to be used.
The 'm_allow_other_inputs=true` default value change is to correct
an ugly misleading behavior:
The tx creation process was having a workaround patch to automatically
fall back to select coins from the wallet if `m_allow_other_inputs=false`
(previous default value) and no manual inputs were selected.
This could be seen in master in flows like `sendtoaddress`, `sendmany`
and even the GUI, where the `m_allow_other_inputs` value isn't customized
and the wallet still selects and adds coins to the tx internally.
|
|
|
|
syncing past their height
5826bf546e83478947edbdf49978414f0b69eb1a test: Add test for getblockfrompeer on syncing pruned nodes (Fabian Jahr)
7fa851fba8570ef256317f7d5759aa3de9088bf1 rpc: Pruned nodes can not fetch unsynced blocks (Fabian Jahr)
Pull request description:
This PR prevents `getblockfrompeer` from getting used on blocks that the node has not synced past yet if the node is in running in prune mode.
### Problem
While a node is still catching up to the tip that it is aware of via the headers, the user can currently use to fetch blocks close to or at the tip. These blocks are stored in the block/rev file that otherwise contains blocks the node is receiving as part of the syncing process.
This creates a problem for pruned nodes: The files containing a fetched block are not pruned during syncing because they contain a block close to the tip. This means the entire file (~130MB) will not be pruned until the tip has moved on far enough from the fetched block. In extreme cases with heavy pruning (like 550) and multiple blocks being fetched this could mean that the disc usage far exceeds what the user expects, potentially running out of space.
### Approach
There would be certainly other approaches that could fix the problem while still allowing the current behavior, but all of the ideas I came up with seemed like overkill for a niche problem on a new RPC where it's still unclear how and how much it will be used.
### Testing
So far I did not see a simple enough way to test this I am still looking into it and if it's complex will potentially add it in a follow-up. What would be needed is a way to have a node fetch headers but not sync the blocks yet, that seems like a pattern that could be generally useful.
To manually reproduce the problematic behavior:
1. Start a node with current `master` with `-prune=550` and an empty/new datadir, Testnet and Mainnet should both work.
2. While the node is syncing run `getblockfrompeer` on the current tip and a few other recent blocks.
3. Go to your datadir and observe the blocks folder: There should be a few full `blk*.dat` and `rev*.dat` files that are not being pruned. When you "pinned" a few of these files the blocks folder should be significantly above the target size of 550MB.
ACKs for top commit:
Sjors:
utACK 5826bf546e83478947edbdf49978414f0b69eb1a
achow101:
ACK 5826bf546e83478947edbdf49978414f0b69eb1a
aureleoules:
tACK 5826bf546e83478947edbdf49978414f0b69eb1a
Tree-SHA512: aa3f477ec755a9df2331c047cb10b3cd08292522bf6ad7a36a7ea36d7eba4894b84de8bd23003c9baea5ac0c53b77142c3c2819ae7528cece9d10a0d06c850d8
|
|
for descriptor wallets
0582932260e7de4e8aba01d63e7c8a9ddb9c3685 test: add test for fast rescan using block filters (top-up detection) (Sebastian Falbesoner)
ca48a4694f73e5be8f971ae482ebc2cce4caef44 rpc: doc: mention rescan speedup using `blockfilterindex=1` in affected wallet RPCs (Sebastian Falbesoner)
3449880b499d54bfbcf6caeed52851ce55259ed7 wallet: fast rescan: show log message for every non-skipped block (Sebastian Falbesoner)
935c6c4b234bbb0565cda6f58ee298048856acae wallet: take use of `FastWalletRescanFilter` (Sebastian Falbesoner)
70b35139040a2351c845a1cec1dafd2fbcd16e93 wallet: add `FastWalletRescanFilter` class for speeding up rescans (Sebastian Falbesoner)
c051026586fb269584bcba41de8a4a90280f5a7e wallet: add method for retrieving the end range for a ScriptPubKeyMan (Sebastian Falbesoner)
845279132b494f03b84d689c666fdcfad37f5a42 wallet: support fetching scriptPubKeys with minimum descriptor range index (Sebastian Falbesoner)
088e38d3bbea9694b319bc34e0d2e70d210c38b4 add chain interface methods for using BIP 157 block filters (Sebastian Falbesoner)
Pull request description:
## Description
This PR is another take of using BIP 157 block filters (enabled by `-blockfilterindex=1`) for faster wallet rescans and is a modern revival of #15845. For reviewers new to this topic I can highly recommend to read the corresponding PR review club (https://bitcoincore.reviews/15845).
The basic idea is to skip blocks for deeper inspection (i.e. looking at every single tx for matches) if our block filter doesn't match any of the block's spent or created UTXOs are relevant for our wallet. Note that there can be false-positives (see https://bitcoincore.reviews/15845#l-199 for a PR review club discussion about false-positive rates), but no false-negatives, i.e. it is safe to skip blocks if the filter doesn't match; if the filter *does* match even though there are no wallet-relevant txs in the block, no harm is done, only a little more time is spent extra.
In contrast to #15845, this solution only supports descriptor wallets, which are way more widespread now than back in the time >3 years ago. With that approach, we don't have to ever derive the relevant scriptPubKeys ourselves from keys before populating the filter, and can instead shift the full responsibility to that to the `DescriptorScriptPubKeyMan` which already takes care of that automatically. Compared to legacy wallets, the `IsMine` logic for descriptor wallets is as trivial as checking if a scriptPubKey is included in the ScriptPubKeyMan's set of scriptPubKeys (`m_map_script_pub_keys`): https://github.com/bitcoin/bitcoin/blob/e191fac4f3c37820f0618f72f0a8e8b524531ab8/src/wallet/scriptpubkeyman.cpp#L1703-L1710
One of the unaddressed issues of #15845 was that [the filter was only created once outside the loop](https://github.com/bitcoin/bitcoin/pull/15845#discussion_r343265997) and as such didn't take into account possible top-ups that have happened. This is solved here by keeping a state of ranged `DescriptorScriptPubKeyMan`'s descriptor end ranges and check at each iteration whether that range has increased since last time. If yes, we update the filter with all scriptPubKeys that have been added since the last filter update with a range index equal or higher than the last end range. Note that finding new scriptPubKeys could be made more efficient than linearly iterating through the whole `m_script_pub_keys` map (e.g. by introducing a bidirectional map), but this would mean introducing additional complexity and state and it's probably not worth it at this time, considering that the performance gain is already significant.
Output scripts from non-ranged `DescriptorScriptPubKeyMan`s (i.e. ones with a fixed set of output scripts that is never extended) are added only once when the filter is created first.
## Benchmark results
Obviously, the speed-up indirectly correlates with the wallet tx frequency in the scanned range: the more blocks contain wallet-related transactions, the less blocks can be skipped due to block filter detection.
In a [simple benchmark](https://github.com/theStack/bitcoin/blob/fast_rescan_functional_test_benchmark/test/functional/pr25957_benchmark.py), a regtest chain with 1008 blocks (corresponding to 1 week) is mined with 20000 scriptPubKeys contained (25 txs * 800 outputs) each. The blocks each have a weight of ~2500000 WUs and hence are about 62.5% full. A global constant `WALLET_TX_BLOCK_FREQUENCY` defines how often wallet-related txs are included in a block. The created descriptor wallet (default setting of `keypool=1000`, we have 8*1000 = 8000 scriptPubKeys at the start) is backuped via the `backupwallet` RPC before the mining starts and imported via `restorewallet` RPC after. The measured time for taking this import process (which involves a rescan) once with block filters (`-blockfilterindex=1`) and once without block filters (`-blockfilterindex=0`) yield the relevant result numbers for the benchmark.
The following table lists the results, sorted from worst-case (all blocks contain wallte-relevant txs, 0% can be skipped) to best-case (no blocks contain walltet-relevant txs, 100% can be skipped) where the frequencies have been picked arbitrarily:
wallet-related tx frequency; 1 tx per... | ratio of irrelevant blocks | w/o filters | with filters | speed gain
--------------------------------------------|-----------------------------|-------------|--------------|-------------
~ 10 minutes (every block) | 0% | 56.806s | 63.554s | ~0.9x
~ 20 minutes (every 2nd block) | 50% (1/2) | 58.896s | 36.076s | ~1.6x
~ 30 minutes (every 3rd block) | 66.67% (2/3) | 56.781s | 25.430s | ~2.2x
~ 1 hour (every 6th block) | 83.33% (5/6) | 58.193s | 15.786s | ~3.7x
~ 6 hours (every 36th block) | 97.22% (35/36) | 57.500s | 6.935s | ~8.3x
~ 1 day (every 144th block) | 99.31% (143/144) | 68.881s | 6.107s | ~11.3x
(no txs) | 100% | 58.529s | 5.630s | ~10.4x
Since even the (rather unrealistic) worst-case scenario of having wallet-related txs in _every_ block of the rescan range obviously doesn't take significantly longer, I'd argue it's reasonable to always take advantage of block filters if they are available and there's no need to provide an option for the user.
Feedback about the general approach (but also about details like naming, where I struggled a lot) would be greatly appreciated. Thanks fly out to furszy for discussing this subject and patiently answering basic question about descriptor wallets!
ACKs for top commit:
achow101:
ACK 0582932260e7de4e8aba01d63e7c8a9ddb9c3685
Sjors:
re-utACK 0582932260e7de4e8aba01d63e7c8a9ddb9c3685
aureleoules:
ACK 0582932260e7de4e8aba01d63e7c8a9ddb9c3685 - minor changes, documentation and updated test since last review
w0xlt:
re-ACK https://github.com/bitcoin/bitcoin/pull/25957/commits/0582932260e7de4e8aba01d63e7c8a9ddb9c3685
Tree-SHA512: 3289ba6e4572726e915d19f3e8b251d12a4cec8c96d041589956c484b5575e3708b14f6e1e121b05fe98aff1c8724de4564a5a9123f876967d33343cbef242e1
|
|
|
|
TryLowWorkHeaderSync
`m_headers_sync` is already reset in IsContinuationOfLowWorkHeadersSync
if there is a failure, so there is no need to also reset in
TryLowWorkHeaderSync.
|
|
globals
aaaa7bd0ba60aa7114810d4794940882d987c0aa iwyu: Add missing includes (MacroFake)
fa9ebec096ae185576a54aa80bd2a9e57f867ed4 Remove g_parallel_script_checks (MacroFake)
fa7c834b9f988fa7f2ace2d67b1628211b7819df Move ::fCheckBlockIndex into ChainstateManager (MacroFake)
fa43188d86288fa6666307a77c106c8f069ebdbe Move ::fCheckpointsEnabled into ChainstateManager (MacroFake)
cccca83099453bf0882bce4f897f77eee5836e8b Move ::nMinimumChainWork into ChainstateManager (MacroFake)
fa29d0b57cdeb91c8798d5c90ba9cc18085e99fb Move ::hashAssumeValid into ChainstateManager (MacroFake)
faf44876db555f7488c8df96db9fa88b793f897c Move ::nMaxTipAge into ChainstateManager (MacroFake)
Pull request description:
It seems preferable to assign globals to a class (in this case `ChainstateManager`), than to leave them dangling. This should clarify scope for code-readers, as well as clarifying unit test behaviour.
ACKs for top commit:
dergoegge:
Code review ACK aaaa7bd0ba60aa7114810d4794940882d987c0aa
ryanofsky:
Code review ACK aaaa7bd0ba60aa7114810d4794940882d987c0aa. No changes since last review, other than rebase
aureleoules:
reACK aaaa7bd0ba60aa7114810d4794940882d987c0aa
Tree-SHA512: 83ec3ba0fb4f1dad95810d4bd4e578454e0718dc1bdd3a794cc4e48aa819b6f5dad4ac4edab3719bdfd5f89cbe23c2740a50fd56c1ff81c99e521c5f6d4e898d
|
|
2147483647 (2^31-1)
9153ff3e274953ea0d92d53ddab4c72deeace1b1 rpc: add non-regression test about deriveaddresses crash when index is 2147483647 (muxator)
addf9d6502db12cebcc5976df3111cac1a369b82 rpc: fix crash in deriveaddresses when derivation index is 2147483647 (muxator)
Pull request description:
This PR is a proposal for fixing #26274 (better described there).
The problem is due to a signed int wrapping when the `index` parameter of the `deriveaddresses` RPC call has the value `2^31-1`.
```C++
for (int i = range_begin; i <= range_end; ++i) {
```
* the first commit adds a "temporary" test case (`test/functional/rpc_deriveaddresses_crash.py`) that shows the crash, and can be used to generate a core dump;
* the second commit fixes the problem giving an explicit size to the `i` variable in a for loop, from `int` to `int64_t`. The same commit also removes the ephemeral test case and adds a passing test to `test/functional/rpc_deriveaddresses.py`, in order to prevent future regressions.
This is my first submission to this project and I do not know its conventions. Please advise if something needs to be changed.
ACKs for top commit:
achow101:
ACK 9153ff3e274953ea0d92d53ddab4c72deeace1b1
Tree-SHA512: 0477b57b15dc2c682cf539d6002f100d44a8c7e668041aa3340c39dcdbd40e083c75dec6896b6c076b044a01c2e5254272ae6696d8a1467539391926f270940a
|
|
|
|
796b020c37c793674f9d614d5d70fd1ed65f0938 wallet: add taproot support to external signer (Sjors Provoost)
Pull request description:
Builds on #22558 (merged on 2022-06-28).
[HWI 2.1.0](https://github.com/bitcoin-core/HWI/releases/tag/2.1.0) or newer is required to import and use taproot descriptors. Older versions will work, but won't import a taproot descriptor.
Tested with HWI 2.1.1:
* Trezor T (firmware v2.5.1) on Signet: signs, change detection works
* Ledger Nano S (firmware 2.1.0, Bitcoin app 2.0.6): signs, change detection works
Only the most basic `tr(key)` descriptor is supported, script path spending is completely untested (if it works at all).
ACKs for top commit:
jb55:
utACK 796b020c37c793674f9d614d5d70fd1ed65f0938
achow101:
ACK 796b020c37c793674f9d614d5d70fd1ed65f0938
Tree-SHA512: 6dcb7eeb45421a3bbf2bdabeacd29979867db69077d7bf192bb77faa4bfefe446487b8df07bc40f9457009a88e598bdc09f769e6106fed2833ace7ef205a157a
|
|
wallet RPCs
|
|
For that purpose, a new logging category BCLog::SCAN is introduced.
|
|
Can be reviewed with `--ignore-all-space`.
|
|
This only supports wallet descriptors right now.
|
|
|
|
This extra method will be needed for updating the filter set for
faster wallet rescans; after an internal top-up has happened, we only
want to add the newly created scriptPubKeys.
|
|
This is useful for speeding up wallet rescans and is based on an
earlier version from PR #15845 ("wallet: Fast rescan with BIP157 block
filters"), which was never merged.
Co-authored-by: MacroFake <falke.marco@gmail.com>
|