8a5014cd8a05b3ab86ae34a47653a82ce11bdf17 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 8a5014cd8a05b3ab86ae34a47653a82ce11bdf17
Tree-SHA512: 3dff0a252fe0e93dd68cf5503135ecf6a72bcf385ba38407d6021ab77cca323f8bbe58aeca90ec124aa2a22ab9d35b706946179ac3b5d171c96a7010de51a090
Tackles two issues in the TestShell documentation:
- add missing instruction for creating a wallet prior to the
`getnewaddress` call (needed as there is no default wallet created
anymore since v0.21)
- fix `generatetoaddress` call syntax (the scripted-diff in commit
fa0b916971e5bc23ad6396831940a2899ca05402 only worked for tests using
`BitcoinTestFramework`)
7a53033303f25301675fb2d7c7b7032166807910 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 7a53033303f25301675fb2d7c7b7032166807910
Tree-SHA512: 031779567e927f05f6fae02394a8c97ba5c45ba9fffd7f1e2c006e152df5f724d92a06f18a4c2540436476eca6b40a3a5cbc4421666cd576439b823668acfcfb
2222ec71fdf573a15bb593fc0dd42d2d28ca5449 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 2222ec71fdf573a15bb593fc0dd42d2d28ca5449, only rebased and suggested changes since my recent [review](https://github.com/bitcoin/bitcoin/pull/25112#pullrequestreview-1182361605).
aureleoules:
ACK 2222ec71fdf573a15bb593fc0dd42d2d28ca5449
Tree-SHA512: 313b3c891bb000cf606df1793b068f93df99915a254fbd67a45f003d440cce7355cdcc6b196f35757cc02d3697970d30e9de0d675f2aa8eb74107c13d663927a
fa84df1f033a5d1a8342ea941eca0b5ef73d78e7 scripted-diff: wallet: rename AvailableCoinsParams members to snake_case (furszy)
61c2265629fdf11a2cc266fad54ceb0a1247bb5e wallet: group AvailableCoins filtering parameters in a single struct (furszy)
f0f6a3577bef2e9ebd084fe35850e4e9580128a9 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 fa84df1f033a5d1a8342ea941eca0b5ef73d78e7
achow101:
ACK fa84df1f033a5d1a8342ea941eca0b5ef73d78e7
aureleoules:
reACK fa84df1f033a5d1a8342ea941eca0b5ef73d78e7
kouloumos:
reACK fa84df1f033a5d1a8342ea941eca0b5ef73d78e7
theStack:
Code-review ACK fa84df1f033a5d1a8342ea941eca0b5ef73d78e7
Tree-SHA512: 0f3544cb8cfd0378a5c74594480f78e9e919c6cfb73a83e0f3112f8a0132a9147cf846f999eab522cea9ef5bd3ffd60690ea2ca367dde457b0554d7f38aec792
db929893ef0bc86ea2708cdbcf41152240cd7c73 Faster -reindex by initially deserializing only headers (Larry Ruane)
c72de9990ae8f1744006d9c852023b882d5ed80c util: add CBufferedFile::SkipTo() to move ahead in the stream (Larry Ruane)
48a68908ba3d5e077cda7bd1e908b923fbead824 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 db929893ef0bc86ea2708cdbcf41152240cd7c73
achow101:
ACK db929893ef0bc86ea2708cdbcf41152240cd7c73
aureleoules:
ACK db929893ef0bc86ea2708cdbcf41152240cd7c73 - minor changes and new benchmark since last review
theStack:
re-ACK db929893ef0bc86ea2708cdbcf41152240cd7c73
stickies-v:
re-ACK db929893e
Tree-SHA512: 5a5377192c11edb5b662e18f511c9beb8f250bc88aeadf2f404c92c3232a7617bade50477ebf16c0602b9bd3b68306d3ee7615de58acfd8cae664d28bb7b0136
fa4ec1be513c80d6db644f1e3170e980035e7306 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 fa4ec1be513c80d6db644f1e3170e980035e7306 for:
shaavan:
ACK fa4ec1be513c80d6db644f1e3170e980035e7306
aureleoules:
ACK fa4ec1be513c80d6db644f1e3170e980035e7306
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.
2dede9f67596787e18904d3d5961f48ec75f241e 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
fa095257511e53d7a593c6714724aafb484e6b6f univalue: string_view test (MacroFake)
1111c7e3f1f5c550f62016d71ccc518aafd97acf 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 fa095257511e53d7a593c6714724aafb484e6b6f
ryanofsky:
Code review ACK fa095257511e53d7a593c6714724aafb484e6b6f
Tree-SHA512: 74c441912bd0b00cdb9ea7890121f71ae5d62a7594e7d29aa402c9e3f033710c5d3afb27a37c552e6513804b249aa37e375ce013a3db853a25d1fd7b6e6cd3a8
737c285f6955253b4bb6f5e2cb024cd79aca8ee4 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 737c285f6955253b4bb6f5e2cb024cd79aca8ee4
Tree-SHA512: 1bf4186fae4390233b2f23389eb6c515c7f0209f12553592df5166e75c452ccd1fb125d9246047c08cff0b869fdda7793812d15da01441e2c4777514446f3ed6
Review note: The changes are complete, because self.options.descriptors
is set to None in parse_args (test_framework.py).
A value of None implies -disablewallet, see the previous commit.
So if a call to add_wallet_options is missing, it will lead to a test
failure when the wallet is compiled in.
The bool is only used to call a public helper, which some tests already
do. So use the public helper in all tests consistently and make the
confusingly named bool private.
887d85e43d136dbfc2428f873ced3de50076bbd0 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 887d85e43d136dbfc2428f873ced3de50076bbd0
kristapsk:
ACK 887d85e43d136dbfc2428f873ced3de50076bbd0
Tree-SHA512: 05b443ae14138769dc3c87a0178f21db2698fa5bcbeaa953c50ed0c9cf5dcd1effcf4afd09551ca9f4ce73898a7882caaf4c57078767beb6a6a65eb3a662726d
c8f91478c10affdfba8e52200c85379052f4e1b1 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 c8f91478c10affdfba8e52200c85379052f4e1b1
Tree-SHA512: 09d257f8fa6be903ec8092b2ae92887a4bec2d05085c76c110637657f4a4bfe2714bf87e2e4727719b3624c8fa4c835ce2ca259c2c9c93033837f997b2057e4f
In the wallet key-value-loading routine, most legacy type entries
require a LegacyScriptPubKeyMan instance after successful
deserialization. On a descriptor wallet, creating that (via method
`GetOrCreateLegacyScriptPubKeyMan`) fails and then leads to a
null-pointer dereference crash. Fix this by throwing an error if
if the wallet flags indicate that we have a descriptor wallet and there
is a legacy entry found.
It's nice to be able to use named options and positional arguments together.
Most shell tools accept both, and python functions combine options and
arguments allowing them to be passed with even more flexibility. This change
adds support for python's approach so as a motivating example:
bitcoin-cli -named createwallet wallet_name=mywallet load_on_startup=1
Can be shortened to:
bitcoin-cli -named createwallet mywallet load_on_startup=1
JSON-RPC standard doesn't have a convention for passing named and positional
parameters together, so this implementation makes one up and interprets any
unused "args" named parameter as a positional parameter array.
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
