files share the same purpose, and we shouldn't have wallet code
inside the test directory.
This later is needed to use wallet util functions in the bench
and test binaries without be forced to duplicate them.
At wallet load time, we set the crypted key "checksum_valid" variable always to false.
Which, on every wallet decryption call, forces the process to re-write the entire ckeys to db when
it's not needed.
fa68d086f3 test: Add getpeerinfo test for missing version message (MacroFake)
Pull request description:
There seems to be a lot of discussion about behaviour/code that is completely untested.
Fix this by adding a test. The test documents the current behaviour and helps to detect when the behaviour changes in the future.
ACKs for top commit:
jonatack:
ACK fa68d086f3
mzumsande:
Code Review ACK fa68d086f3
Tree-SHA512: d092b30d5bdb46712c91a7c5bd2d0c82a0da281f1460967aa4e32c648b15d8d97870ded9565a90af34874eb468aad8b99694a2485af6807994e7cfc05482aa8c
b93beef5ed doc: Mac -> macOS in release notes template (fanquake)
2747adb68a doc: Add 24.0 release notes (fanquake)
Pull request description:
Same as past releases.
ACKs for top commit:
stickies-v:
ACK b93beef5ed
Tree-SHA512: c28bc7286f330a6058ae266b238468044439457ff5b9df191232d91dc17b8092facd6ed72accec8bc9db10f055f7bb7e06700cc1ed0bd045fc15f612bc023a46
ac410e6fc0 log: improve some validation log messages to include hashPrevBlock (Skuli Dulfari)
Pull request description:
When there is an issue with a previous block the current log messages do not indicate hashPrevBlock. Adding it makes debugging easier.
ACKs for top commit:
stickies-v:
ACK ac410e6fc0
aureleoules:
reACK ac410e6fc0
theStack:
ACK ac410e6fc0
Tree-SHA512: d91481321f4474bb4fdf6ad55d1c897437b631b0a12308815c4ac5b053c8a76726e2d93f2aa0701e8cfd48fba7fad19ef5ffca3c67d3aa973dc593df806f1757
8a5014cd8a Fixes bitcoin#26490 by preventing notifications (John Moffett)
Pull request description:
This is a PR to address https://github.com/bitcoin/bitcoin/issues/26490
The menu bar currently subscribes to window focus change notifications to enable or disable certain menu options in response to the window status.
Notifications are automatically unsubscribed (disconnected in Qt parlance) if the sender is deleted -- in this case, the sender is the QTApplication object (`qApp`). However, MacOS 13 sends a window focus change notification *after* the main window has been destroyed but *before* `qApp` has been fully destroyed.
Since the menu bar is deleted in the main window's destructor, it no longer exists when it receives these notifications (in two different places via lambda expressions). The solution is to pass the main window (`this`) as context when subscribing to the notifications. In this [overloaded version](https://doc.qt.io/qt-5/qobject.html#connect-1) of `connect`, Qt automatically unsubscribes to notifications if the sender OR context (here the main window object) is destroyed. Since the spurious notifications are sent after the main window object is destroyed, this change prevents them from being sent.
Tested on Mac OS 13 and 12 only.
ACKs for top commit:
hebasto:
ACK 8a5014cd8a
Tree-SHA512: 3dff0a252fe0e93dd68cf5503135ecf6a72bcf385ba38407d6021ab77cca323f8bbe58aeca90ec124aa2a22ab9d35b706946179ac3b5d171c96a7010de51a090
7a53033303 Fix Transaction Relay tooltip text in Peers details window (Jon Atack)
Pull request description:
as a value of N/A could occur due to a lock or a disconnection race but not during connection setup -- see https://github.com/bitcoin/bitcoin/pull/26457#pullrequestreview-1181641835. Credit to Martin Zumsande for finding this.
ACKs for top commit:
jarolrod:
ACK 7a53033303
Tree-SHA512: 031779567e927f05f6fae02394a8c97ba5c45ba9fffd7f1e2c006e152df5f724d92a06f18a4c2540436476eca6b40a3a5cbc4421666cd576439b823668acfcfb
2222ec71fd util: Move error message formatting of NonFatalCheckError to cpp (MacroFake)
Pull request description:
This allows to strip down the header file.
ACKs for top commit:
hebasto:
re-ACK 2222ec71fd, only rebased and suggested changes since my recent [review](https://github.com/bitcoin/bitcoin/pull/25112#pullrequestreview-1182361605).
aureleoules:
ACK 2222ec71fd
Tree-SHA512: 313b3c891bb000cf606df1793b068f93df99915a254fbd67a45f003d440cce7355cdcc6b196f35757cc02d3697970d30e9de0d675f2aa8eb74107c13d663927a
fa84df1f03 scripted-diff: wallet: rename AvailableCoinsParams members to snake_case (furszy)
61c2265629 wallet: group AvailableCoins filtering parameters in a single struct (furszy)
f0f6a3577b RPC: listunspent, add "include immature coinbase" flag (furszy)
Pull request description:
Simple PR; adds a "include_immature_coinbase" flag to `listunspent` to include the immature coinbase UTXOs on the response. Requested by #25728.
ACKs for top commit:
danielabrozzoni:
reACK fa84df1f03
achow101:
ACK fa84df1f03
aureleoules:
reACK fa84df1f03
kouloumos:
reACK fa84df1f03
theStack:
Code-review ACK fa84df1f03
Tree-SHA512: 0f3544cb8cfd0378a5c74594480f78e9e919c6cfb73a83e0f3112f8a0132a9147cf846f999eab522cea9ef5bd3ffd60690ea2ca367dde457b0554d7f38aec792
db929893ef Faster -reindex by initially deserializing only headers (Larry Ruane)
c72de9990a util: add CBufferedFile::SkipTo() to move ahead in the stream (Larry Ruane)
48a68908ba Add LoadExternalBlockFile() benchmark (Larry Ruane)
Pull request description:
### Background
During the first part of reindexing, `LoadExternalBlockFile()` sequentially reads raw blocks from the `blocks/blk00nnn.dat` files (rather than receiving them from peers, as with initial block download) and eventually adds all of them to the block index. When an individual block is initially read, it can't be immediately added unless all its ancestors have been added, which is rare (only about 8% of the time), because the blocks are not sorted by height. When the block can't be immediately added to the block index, its disk location is saved in a map so it can be added later. When its parent is later added to the block index, `LoadExternalBlockFile()` reads and deserializes the block from disk a second time and adds it to the block index. Most blocks (92%) get deserialized twice.
### This PR
During the initial read, it's rarely useful to deserialize the entire block; only the header is needed to determine if the block can be added to the block index immediately. This change to `LoadExternalBlockFile()` initially deserializes only a block's header, then deserializes the entire block only if it can be added immediately. This reduces reindex time on mainnet by 7 hours on a Raspberry Pi, which translates to around a 25% reduction in the first part of reindexing (adding blocks to the index), and about a 6% reduction in overall reindex time.
Summary: The performance gain is the result of deserializing each block only once, except its header which is deserialized twice, but the header is only 80 bytes.
ACKs for top commit:
andrewtoth:
ACK db929893ef
achow101:
ACK db929893ef
aureleoules:
ACK db929893ef - minor changes and new benchmark since last review
theStack:
re-ACK db929893ef
stickies-v:
re-ACK db929893e
Tree-SHA512: 5a5377192c11edb5b662e18f511c9beb8f250bc88aeadf2f404c92c3232a7617bade50477ebf16c0602b9bd3b68306d3ee7615de58acfd8cae664d28bb7b0136
fa4ec1be51 test: Split overly large util_tests.cpp file (MacroFake)
Pull request description:
The file has ~3kLOC and is slow to compile.
Fix both issues by splitting it. (On my machine the compilation goes from 25 seconds previously to 17+10 seconds for the two smaller files)
To review, `--color-moved=dimmed-zebra` can be used.
ACKs for top commit:
RandyMcMillan:
ACK fa4ec1be51 for:
shaavan:
ACK fa4ec1be51
aureleoules:
ACK fa4ec1be51
Tree-SHA512: 4719439c7ee0c6c06b6f6ccf07b3a037c0cae58b1bd6e6e929ebfeab8403be3d1905581669ed733bff0cbf4e385c27ae58d519ce031e145e6889bd5bce1c1d03
MacOS 13 sends a window focus change notification after the main
window has been destroyed but before the QTApplication has been
destroyed. This results in the menu bar receiving a notification
despite it no longer existing. The solution is to pass the main
window as context when subscribing to the notifications. Qt
automatically unsubscribes to notifications if the sender OR
context is destroyed.
2dede9f675 Adjust RPCTypeCheckObj error string (Leonardo Araujo)
Pull request description:
Unifies the JSON type error strings as mentioned in #26214. Also refer to #25737.
ACKs for top commit:
furszy:
ACK 2dede9f6
Tree-SHA512: c918889e347ba32cb6d0e33c0de5956c2077dd40c996151e16741b0c4983ff098c60258206ded76ad7bbec4876c780c6abb494a97e4f1e05717d28a59b9167a6
fa09525751 univalue: string_view test (MacroFake)
1111c7e3f1 univalue: Avoid std::string copies (MacroFake)
Pull request description:
This shouldn't matter too much, unless a really large string is pushed into a json struct, but I think it also clarifies the code.
ACKs for top commit:
martinus:
Code review ACK fa09525751
aureleoules:
reACK fa09525751
ryanofsky:
Code review ACK fa09525751
Tree-SHA512: 74c441912bd0b00cdb9ea7890121f71ae5d62a7594e7d29aa402c9e3f033710c5d3afb27a37c552e6513804b249aa37e375ce013a3db853a25d1fd7b6e6cd3a8
737c285f69 test: Don't pass add_to_wallet option to walletcreatefundedpsbt (Ryan Ofsky)
Pull request description:
It's not a documented option. Noticed while working on #19762
ACKs for top commit:
achow101:
ACK 737c285f69
Tree-SHA512: 1bf4186fae4390233b2f23389eb6c515c7f0209f12553592df5166e75c452ccd1fb125d9246047c08cff0b869fdda7793812d15da01441e2c4777514446f3ed6
887d85e43d test: add missing bech32m / BIP86 test-cases to wallet_descriptor.py (Sebastian Falbesoner)
Pull request description:
This small PR adds missing "bech32m" address type / BIP86 checks w.r.t. to the `getnewaddress`/`getrawchangeaddress` RPC and descriptor export functionality to the functional test `wallet_descriptor.py`.
ACKs for top commit:
shaavan:
ACK 887d85e43d
kristapsk:
ACK 887d85e43d
Tree-SHA512: 05b443ae14138769dc3c87a0178f21db2698fa5bcbeaa953c50ed0c9cf5dcd1effcf4afd09551ca9f4ce73898a7882caaf4c57078767beb6a6a65eb3a662726d
c8f91478c1 test: Avoid collision with valid path names in `getarg_tests/logargs` (Hennadii Stepanov)
Pull request description:
This PR prevents test failure when "private" is a part of a valid path.
For example, `/private/var` is a valid path on macOS for temporary files, which in turn causes test failure on CI for tests managed by the [CTest](https://github.com/bitcoin/bitcoin/pull/25797) framework.
ACKs for top commit:
MarcoFalke:
ACK c8f91478c1
Tree-SHA512: 09d257f8fa6be903ec8092b2ae92887a4bec2d05085c76c110637657f4a4bfe2714bf87e2e4727719b3624c8fa4c835ce2ca259c2c9c93033837f997b2057e4f
25ef049d60 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 25ef049d60
Tree-SHA512: 9890f9fa16f66c8a9296798d8c28993e1b81da17cf592946f2abc22041f0b30b0911ab86a0c48d4aa46b9a8b3f7f5de67778649ac48c97740b0a09aa6816e0af
c3b1fe59db 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 c3b1fe59db
Tree-SHA512: a3f1f8fde5905c80e1b95bd042ca0bc73d08c1c0e79c52ab0d6d12d7afdd4aa288afb41e12279fcea328a396f3d0a5564018170c0a11c5aa26dc6d44d2a62b1c
0de30ed509 tests: Test Taproot PSBT signing with keys in other descriptor (Andrew Chow)
6efcdf6b7f tests: Use new wallets for each test in wallet_taproot.py (Andrew Chow)
8781a1b6bb psbt: Include output pubkey in additional pubkeys to sign (Andrew Chow)
323890d0d7 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 0de30ed509
darosior:
ACK 0de30ed509
Tree-SHA512: 12e131dd8afd93da7b1288c9054de2415a228d4477b97102da3ee4e82ce9de20b186260c3085a4b7b067bd8b74400751dcadf153f113db83abc59e7466e69f14
74d975318a test: fix intermittent failure in p2p_sendtxrcncl.py (Martin Zumsande)
Pull request description:
`p2p_sendtxrcncl.py` currently fails intermittently in the CI, see e.g. https://cirrus-ci.com/task/5511952184115200?logs=ci#L4024
I believe that this is related to the reuse of the parameter `p2p_idx=2` of `add_outbound_p2p_connection` in this test: When we call `peer_disconnect`, we don't wait until the node has completed the disconnection. So there is a race between setting up the next connection (next `addconnection` RPC), and if the old one hasn't been removed and has an identical port like the new one (because we didn't increment `p2p_idx`), `CConnman::OpenNetworkConnection` just [returns](5274f32437/src/net.cpp (L1976)) without establishing a connection, and the test fails.
Fix this by using distinct `disconnect_p2ps` instead of `peer_disconnect`, which waits for the disconnect to complete. We can then use the same value for `p2p_idx` everywhere.
ACKs for top commit:
MarcoFalke:
review ACK 74d975318a
Tree-SHA512: f99f2550b6b320c0a2416a475c1cf189c009fce3a5abf1d4462486e1bfe309e2c3fd4228a4009b0ca38cb77465ce85e3d22298719eb07302fa0a72fbab0e0668
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.
Using disconnect_p2ps instead of peer_disconnect makes
the node wait for the disconnect to complete. As a result,
we can reuse p2p_idx=0 in the add_outbound_p2p_connection calls.
29fa38a41a .python-version: bump patch version to 3.6.15 (Sjors Provoost)
Pull request description:
I'm unable to build Python 3.6.12 using PyEnv on macOS 13.0 as well as Ubuntu 22.10. Bumping the patch version fixes that issue on both systems.
A workaround is to add `.python-version` to your local git excludes and then do `pyenv local 3.6.15`, but this won't persist when you switch branches. Another workaround is to disable `pyenv`, but then you're potentially not running the test suite against the oldest supported Python version.
ACKs for top commit:
MarcoFalke:
concept ACK 29fa38a41a
Tree-SHA512: 60ecf76c957456fe4ece9e92c30866b27aba32d5bdb1696495281c2f7ef7a1fc30226e8992d55bf6f45e430b7588e05fbe49de7138aefab5597dd52bd4f021c0
fa3ea81c3e 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 fa3ea81c3e
theuni:
ACK fa3ea81c3e
Tree-SHA512: 17dea4d75f2ee2bf6e1b6a6f6d8f439711c777df0390574e8d8edb6ac9ee807a135341e4439050bd6a15ecc4097a1ba9a7ab15d27541ebf70a4e081fa6871877
201b9a02fd test: fix intermittent failure in feature_index_prune.py (Martin Zumsande)
Pull request description:
I can't reproduce the error from #26630 locally, but from analying the logs I think the problem is the following:
After calling `sync_blocks`, we didn't check that the indexes have caught up to the tip before performing the manual pruning. This could possibly lead to prune blockers with a lower height than the expected 2489, which do appear in the logs of the failed CI runs, e.g.
- `2022-10-27T21:14:17.703920Z [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\src\validation.cpp:2395] [FlushStateToDisk] [prune] coinstatsindex limited pruning to height 2488` ([Cirrus](https://cirrus-ci.com/task/5443742333665280?logs=functional_tests#L2506))
So, this should be fixed by a call to `sync_index`.
Fixes#26330
ACKs for top commit:
brunoerg:
crACK 201b9a02fd
Tree-SHA512: fb7023c9eb2ba6d0e69e059a401453cbdf63abc6804543dffcf36ba9f93c9cd13209e57aa5536d94b2e420c9d4cd0b1a7eff1adadd19aa7b3c33f592502e1bc0
fa24239a1c 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 fa24239a1c
vasild:
ACK fa24239a1c
jonatack:
ACK fa24239a1c
mzumsande:
ACK fa24239a1c
Tree-SHA512: d7604c7eb4df8f2de811e600bdd312440ee03e508d3a0f09ae79f7f2d3eeec663bfd47a2d079fa50b756d61e35dfa998de068a7b9afaf35378fa0e62a538263d
f1ee974e8e test: remove unused `CHANGE_{XPRV,XPUB}` constants (Sebastian Falbesoner)
Pull request description:
These constants exist since the introduction of the functional test wallet_taproot.py (2667366aaa), but they have never been used.
ACKs for top commit:
instagibbs:
ACK f1ee974e8e
brunoerg:
ACK f1ee974e8e
Tree-SHA512: 2c6f8cd537c35af59c4cbe6abb07386b7fb9e4662c7bed27ad8935e004f9bf452705fb1caef4f5ea1f4ad509745afba966005f95a18cf2b59e00b7739a5df7f8
e049fd76f0 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 e049fd76f0.
Tree-SHA512: 188bace79cbe556efe7782e46b870c02729b07b104a9316b0f7d50013504972e85baf507403d2d6060bb2bf3e13f40d735bddd18255d97a60810208c3de87691
98868633d1 Bugfix: configure: bitcoin-{cli,tx,util} don't need UPnP, NAT-PMP, or ZMQ (Luke Dashjr)
Pull request description:
As with #23345, these other tools likewise don't use various deps.
ACKs for top commit:
achow101:
ACK 98868633d1
Tree-SHA512: 4be056b8e0c9f69834229aa257187457de1bc34214d320b770834e21ecc1f0ca7aa7b9689fba525928947bfabbb461528795f709014fb9618b82f088fe64f271
