https://quickshare.samsungcloud.com/dwJP6p3a6K84https://quickshare.samsungcloud.com/dwJP6p3a6K84#9
Open
Lost4urluv7 wants to merge 6216 commits into
Open
https://quickshare.samsungcloud.com/dwJP6p3a6K84https://quickshare.samsungcloud.com/dwJP6p3a6K84#9Lost4urluv7 wants to merge 6216 commits into
Lost4urluv7 wants to merge 6216 commits into
Conversation
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.
fa4ec1b 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 fa4ec1b for: shaavan: ACK fa4ec1b aureleoules: ACK fa4ec1b Tree-SHA512: 4719439c7ee0c6c06b6f6ccf07b3a037c0cae58b1bd6e6e929ebfeab8403be3d1905581669ed733bff0cbf4e385c27ae58d519ce031e145e6889bd5bce1c1d03
db92989 Faster -reindex by initially deserializing only headers (Larry Ruane) c72de99 util: add CBufferedFile::SkipTo() to move ahead in the stream (Larry Ruane) 48a6890 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 db92989 achow101: ACK db92989 aureleoules: ACK db92989 - minor changes and new benchmark since last review theStack: re-ACK db92989 stickies-v: re-ACK db92989 Tree-SHA512: 5a5377192c11edb5b662e18f511c9beb8f250bc88aeadf2f404c92c3232a7617bade50477ebf16c0602b9bd3b68306d3ee7615de58acfd8cae664d28bb7b0136
…" flag fa84df1 scripted-diff: wallet: rename AvailableCoinsParams members to snake_case (furszy) 61c2265 wallet: group AvailableCoins filtering parameters in a single struct (furszy) f0f6a35 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 bitcoin#25728. ACKs for top commit: danielabrozzoni: reACK fa84df1 achow101: ACK fa84df1 aureleoules: reACK fa84df1 kouloumos: reACK fa84df1 theStack: Code-review ACK fa84df1 Tree-SHA512: 0f3544cb8cfd0378a5c74594480f78e9e919c6cfb73a83e0f3112f8a0132a9147cf846f999eab522cea9ef5bd3ffd60690ea2ca367dde457b0554d7f38aec792
This allows to strip down the header file
…heckError to cpp 2222ec7 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 2222ec7, only rebased and suggested changes since my recent [review](bitcoin#25112 (review)). aureleoules: ACK 2222ec7 Tree-SHA512: 313b3c891bb000cf606df1793b068f93df99915a254fbd67a45f003d440cce7355cdcc6b196f35757cc02d3697970d30e9de0d675f2aa8eb74107c13d663927a
This change nukes the policy/fees->mempool circular dependency. Easy to review using `diff --color-moved=dimmed-zebra`.
as a value of N/A could occur due to a lock or a disconnection race but not during connection setup.
…ad of the transaction hex for feerate calculation.
…rs details window 7a53033 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 bitcoin#26457 (review). Credit to Martin Zumsande for finding this. ACKs for top commit: jarolrod: ACK 7a53033 Tree-SHA512: 031779567e927f05f6fae02394a8c97ba5c45ba9fffd7f1e2c006e152df5f724d92a06f18a4c2540436476eca6b40a3a5cbc4421666cd576439b823668acfcfb
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 fa0b916 only worked for tests using `BitcoinTestFramework`)
…tain notifications after main window is destroyed 8a5014c Fixes bitcoin#26490 by preventing notifications (John Moffett) Pull request description: This is a PR to address bitcoin#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 8a5014c Tree-SHA512: 3dff0a252fe0e93dd68cf5503135ecf6a72bcf385ba38407d6021ab77cca323f8bbe58aeca90ec124aa2a22ab9d35b706946179ac3b5d171c96a7010de51a090
When there is an issue with a previous block the current log messages do not indicate hashPrevBlock. Adding it makes debugging easier.
…lude hashPrevBlock ac410e6 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 ac410e6 aureleoules: reACK ac410e6 theStack: ACK ac410e6 Tree-SHA512: d91481321f4474bb4fdf6ad55d1c897437b631b0a12308815c4ac5b053c8a76726e2d93f2aa0701e8cfd48fba7fad19ef5ffca3c67d3aa973dc593df806f1757
b93beef doc: Mac -> macOS in release notes template (fanquake) 2747adb doc: Add 24.0 release notes (fanquake) Pull request description: Same as past releases. ACKs for top commit: stickies-v: ACK b93beef Tree-SHA512: c28bc7286f330a6058ae266b238468044439457ff5b9df191232d91dc17b8092facd6ed72accec8bc9db10f055f7bb7e06700cc1ed0bd045fc15f612bc023a46
…essage fa68d08 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 fa68d08 mzumsande: Code Review ACK fa68d08 Tree-SHA512: d092b30d5bdb46712c91a7c5bd2d0c82a0da281f1460967aa4e32c648b15d8d97870ded9565a90af34874eb468aad8b99694a2485af6807994e7cfc05482aa8c
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.
…is less than necessary 6630a1e Add warning on first startup if free disk space is less than necessary (Ben Woosley) Pull request description: This reworks/revives bitcoin#15848 to add a check for low disk space on first startup and issue a warning if disk space is below the expected space required to accommodate the blocks. This PR was fashioned by a team of developers at the [bitcoin++](https://www.btcplusplus.dev/) conference workshop: "[Let's contribute to Bitcoin Core](https://sched.co/12P6Z)" Fixes bitcoin#15813 ACKs for top commit: achow101: ACK 6630a1e willcl-ark: tACK 6630a1e rebased on master. Warning shows on first start but not on restart after some blocks have been downloaded. aureleoules: ACK 6630a1e pablomartin4btc: re-ACK bitcoin@6630a1e hernanmarino: ReACK bitcoin@6630a1e Tree-SHA512: 0f18acabdf2b514e96e2eea8f304960b952226b83dc91334cf7d1f6355ea2f257aaec0ee38d43ac36435385ecd918333d20657c35a8a7407e7cf2680ccb643bb
…endencies c8dc0e3 refactor: Inline `CTxMemPoolEntry` class's functions (Hennadii Stepanov) 75bbe59 refactor: Move `CTxMemPoolEntry` class to its own module (Hennadii Stepanov) Pull request description: This PR: - gets rid of the `policy/fees` -> `txmempool` -> `policy/fees` circular dependency - is an alternative to bitcoin#13949, which nukes only one circular dependency ACKs for top commit: ryanofsky: Code review ACK c8dc0e3. Just include and whitespace changes since last review, and there's a moveonly commit now so it's very easy to review theStack: Code-review ACK c8dc0e3 glozow: utACK c8dc0e3, agree these changes are an improvement. Tree-SHA512: 36ece824e6ed3ab1a1e198b30a906c8ac12de24545f840eb046958a17315ac9260c7de26e11e2fbab7208adc3d74918db7a7e389444130f8810548ca2e81af41
…ndly fafcc94 Make bitcoin-util grind_task tsan friendly (MacroFake) Pull request description: While there is no issue with the current code, `libtsan-12.2.1` on my machine does not seem to like it. This is understandable, because the nonce isn't protected by a mutex that the sanitizer can see (only by an atomic, which achieves the same). Fix this by guarding the nonce by the existing atomic bool, which tsan seems to understand. ACKs for top commit: ajtowns: ACK fafcc94 hebasto: ACK fafcc94, I have reviewed the code and it looks OK, I agree it can be merged. Confirming that initial bug has been fixed. Tree-SHA512: 4e67fab5833ec7d91678b85a300368892ee9f7cd89a52cc5e15a7df65b2da813b24eaffd8362d0d8a3c8951e024041d69ebddf25101b11d0a1a62c1208ddc9a5
adb7dba clang-tidy: Fix `modernize-use-nullptr` in headers (Hennadii Stepanov) Pull request description: Split from bitcoin#26705 as was requested in bitcoin#26705 (comment). To test this PR, consider applying a diff as follows: ```diff --- a/src/.clang-tidy +++ b/src/.clang-tidy @@ -12,17 +12,9 @@ readability-redundant-declaration, readability-redundant-string-init, ' WarningsAsErrors: ' -bugprone-argument-comment, -bugprone-use-after-move, -misc-unused-using-decls, -modernize-use-default-member-init, modernize-use-nullptr, -performance-for-range-copy, -performance-move-const-arg, -performance-unnecessary-copy-initialization, -readability-redundant-declaration, -readability-redundant-string-init, ' CheckOptions: - key: performance-move-const-arg.CheckTriviallyCopyableMove value: false +HeaderFilterRegex: '.' ``` ACKs for top commit: john-moffett: ACK adb7dba Tree-SHA512: 67241fb212d837157a0a26f0d59e7f30a9d270d5b0ebfeb6ad9631e460fc7fba8c9a9dcd4c0520789353f68025a9f090f40f17176472a93cce1411e6d56f930b
…aders 48033d4 clang-tidy: Fix `performance-for-range-copy` in headers (Hennadii Stepanov) Pull request description: Split from bitcoin#26705 as was requested in bitcoin#26705 (comment). To test this PR, consider applying a diff as follows: ```diff --- a/src/.clang-tidy +++ b/src/.clang-tidy @@ -12,17 +12,9 @@ readability-redundant-declaration, readability-redundant-string-init, ' WarningsAsErrors: ' -bugprone-argument-comment, -bugprone-use-after-move, -misc-unused-using-decls, -modernize-use-default-member-init, -modernize-use-nullptr, performance-for-range-copy, -performance-move-const-arg, -performance-unnecessary-copy-initialization, -readability-redundant-declaration, -readability-redundant-string-init, ' CheckOptions: - key: performance-move-const-arg.CheckTriviallyCopyableMove value: false +HeaderFilterRegex: '.' ``` ACKs for top commit: MarcoFalke: review ACK 48033d4 Tree-SHA512: eaf7a0e9b4fdc4ce788f78e5675632f3c278fc24bee2434874cbabc3e25ad7059b0c53ab7834908e901872d5afee08acba860542b03454c09fe129be6ad03f09
istream_iterator eats whitespace charactesr which causes parsing failures for PSBTs that contain the bytes corresponding to those characters.
2b77a33 test: Improve `check-doc.py` pattern (Hennadii Stepanov) Pull request description: On master (cb32328): ``` $ ./test/lint/check-doc.py Args used : 158 Args documented : 219 Args undocumented: 0 set() Args unknown : 61 {'-stopatheight', '-maxtipage', '-maxreceivebuffer', '-txconfirmtarget', '-maxconnections', '-maxsigcachesize', '-peertimeout', '-limitancestorsize', '-output-csv', '-blockmaxweight', '-par', '-rpcclienttimeout', '-dbcrashratio', '-zmqpubsequence', '-zmqpubhashtxhwm', '-zmqpubrawblock', '-dbbatchsize', '-zmqpubrawtxhwm', '-includeconf', '-checkblocks', '-limitancestorcount', '-zmqpubrawtx', '-checklevel', '-checkmempool', '-rpcthreads', '-rpcworkqueue', '-zmqpubsequencehwm', '-zmqpubrawblockhwm', '-rpcservertimeout', '-testnet', '-zmqpubhashtx', '-signet', '-rpcwaittimeout', '-limitdescendantcount', '-output-json', '-maxmempool', '-mocktime', '-datacarriersize', '-rpcport', '-dbcache', '-zmqpubhashblockhwm', '-mempoolexpiry', '-settings', '-min-time', '-maxtimeadjustment', '-bytespersigop', '-blockversion', '-limitdescendantsize', '-maxorphantx', '-rpccookiefile', '-rpcserialversion', '-bantime', '-blockreconstructionextratxn', '-checkaddrman', '-debuglogfile', '-pid', '-dblogsize', '-timeout', '-zmqpubhashblock', '-maxsendbuffer', '-regtest'} ``` With this PR: ``` $ ./test/lint/check-doc.py Args used : 208 Args documented : 219 Args undocumented: 0 set() Args unknown : 11 {'-zmqpubrawblock', '-zmqpubhashblockhwm', '-zmqpubsequencehwm', '-zmqpubrawtx', '-zmqpubhashblock', '-zmqpubhashtx', '-includeconf', '-zmqpubhashtxhwm', '-zmqpubrawblockhwm', '-zmqpubrawtxhwm', '-zmqpubsequence'} ``` ACKs for top commit: vincenzopalazzo: ACK bitcoin@2b77a33 Tree-SHA512: 6cf4ccc4e8319aad8006ae915f0d25637ac12974fbc1f81808f26b72fbe2649e2b6ff993bc2c1894f81bd6756bff77491b3d56382c034a84fd50325a3c807d8b
…ble-debug` 1647a11 tests: Reorder longer running tests in test_runner (Andrew Chow) ff6c9fe tests: Whitelist test p2p connection in rpc_packages (Andrew Chow) 8c20796 tests: Use waitfornewblock for work queue test in interface_rpc (Andrew Chow) 6c872d5 tests: Initialize sigops draining script with bytes in feature_taproot (Andrew Chow) 544cbf7 tests: Use batched RPC in feature_fee_estimation (Andrew Chow) 4ad7272 tests: reduce number of generated blocks for wallet_import_rescan (Andrew Chow) Pull request description: When configured with `--enable-debug`, many tests become dramatically slower. These slow downs are particularly noticed in tests that generate a lot of blocks in separate calls, make a lot of RPC calls, or send a lot of data from the test framework's P2P connection. This PR aims to improve the runtime of some of the slower tests and improve the overall runtime of the test runner. This has improved the runtime of the test runner from ~400s to ~140s on my computer. The slowest test by far was `wallet_import_rescan.py`. This was taking ~320s. Most of that time was spent waiting for blocks to be mined and then synced to the other nodes. It was generating a new block for every new transaction it was creating in a setup loop. However it is not necessary to have one tx per block. By mining a block only every 10 txs, the runtime is improved to ~61s. The second slowest test was `feature_fee_estimation.py`. This test spends most of its time waiting for RPCs to respond. I was able to improve its runtime by batching RPC requests. This has improved the runtime from ~201s to ~140s. In `feature_taproot.py`, the test was constructing a Python `CScript` using a very large list of `OP_CHECKSIG`s. The constructor for the Python implementation of `CScript` was iterating this list in order to create a `bytes` from it even though a `bytes` could be created from it without iterating. By making the `bytes` before passing it into the constructor, we are able to improve this test's runtime from ~131s to ~106s. Although `interface_rpc.py` was not typically a slow test, I found that it would occasionally have a super long runtime. It typically takes ~7s, but I have observed it taking >400s to run on occasion. This longer runtime occurs more often when `--enable-debug`. This long runtime was caused by the "exceeding work queue" test which is really just trying to trigger a race condition. In this test, it would create a few threads and try an RPC in a loop in the hopes that eventually one of the RPCs would be added to the work queue while another was processing. It used `getrpcinfo` for this, but this function is fairly fast. I believe what was happening was that with `--enable-debug`, all of the code for receiving the RPC would often take longer to run than the RPC itself, so the majority of the requests would succeed, until we got lucky after 10's of thousands of requests. By changing this to use a slow RPC, the race condition can be triggered more reliably, and much sooner as well. I've used `waitfornewblock` with a 500ms timeout. This improves the runtime to ~3s consistently. The last test I've changed was `rpc_packages.py`. This test was one of the higher runtime variability tests. The main source of this variation appears to be waiting for the test node to relay a transaction to the test framework's P2P connection. By whitelisting that peer, the variability is reduced to nearly 0. Lastly, I've reordered the tests in `test_runner.py` to account for the slower runtimes when configured with `--enable-debug`. Some of the slow tests I've looked at were listed as being fast which was causing overall `test_runner.py` runtime to be extended. This change makes the test runner's runtime be bounded by the slowest test (currently `feature_fee_estimation.py` with my usual config (`-j 60`). ACKs for top commit: willcl-ark: ACK 1647a11 Tree-SHA512: 529e0da4bc51f12c78a40d6d70b3a492b97723c96a3526148c46943d923c118737b32d2aec23d246392e50ab48013891ef19fe6205bf538b61b70d4f16a203eb
36c201f remove CBlockIndex copy construction (James O'Beirne) Pull request description: Copy construction of CBlockIndex objects is a footgun because of the wide use of equality-by-pointer comparison in the code base. There are also potential lifetime confusions of using copied instances, since there are recursive pointer members (e.g. pprev). (See also bitcoin#24008 (comment)) We can't just delete the copy constructors because they are used for derived classes (CDiskBlockIndex), so we mark them protected. ACKs for top commit: ajtowns: ACK 36c201f - code review only MarcoFalke: re-ACK 36c201f 🏻 Tree-SHA512: b1cf9a1cb992464a4377dad609713eea63cc099435df374e4553bfe62d362a4eb5e3c6c6649177832f38c0905b23841caf9d62196cef8e3084bfea0bfc26374b
…ateStats 6fefd49 rpc: Require NodeStateStats object in getpeerinfo (Martin Zumsande) Pull request description: The objects `CNode`, `CNodeState` and `Peer` store different info about a peer - `InitializeNode()` and `FinalizeNode()` make sure that for the duration of a connection, we should always have one of each for a peer. Therefore, there is no situation in which, as part of getpeerinfo RPC, `GetNodeStateStats()` (which requires a `CNodeState` and a `Peer` entry for a `NodeId` to succeed) could fail for a legitimate reason while the peer is connected - this can only happen if there is a race condition between peer disconnection and the `getpeerinfo` processing (see also a more detailed description of this in bitcoin#26457 (review)). But in this case I think it's better to just not include the newly disconnected peer in the response instead of returning just parts of its data. An earlier version of this PR also made the affected `CNodeStateStats` fields non-optional (see mzumsande@5f900e2). Since this conflicts with bitcoin#25923 and should be a separate discussion, I removed that commit from this PR. ACKs for top commit: dergoegge: Approach ACK 6fefd49 MarcoFalke: review ACK 6fefd49 👒 Tree-SHA512: 89c8f7318df4634c1630415de9c8350e6dc2d14d9d07e039e5b180c51bfd3ee2ce99eeac4f9f858af7de846f7a6b48fcae96ebac08495b30e431a5d2d4660532
…d should throw an error ec63a48 test: call `keypoolrefill` with private keys disabled should throw an error (brunoerg) Pull request description: This PR adds test coverage for the following error: https://github.com/bitcoin/bitcoin/blob/cb32328d1b80d0ccd6eb9532bd8fe4e0a4de385e/src/wallet/rpc/addresses.cpp#L332-L334 ACKs for top commit: aureleoules: ACK ec63a48 Tree-SHA512: b5deda8981ff472f290e6e16c8723a58e02cbe099afd1f672c099f4add0a1d9b192b11a2c3f0e11b96794671f6b9efa75812b7a174248d7c58d7fd7d3310e7b9
Since the original fix was set to be a "reasonable" transaction to reduce allocations and the true motivation later revealed, it makes sense to relax this check to something more principled. There are more exotic transaction patterns that could take advantage of a relaxed requirement, such as 1 input, 1 output OP_RETURN to burn a utxo to fees for CPFP purposes when change isn't practical. Two changes could be accomplished: 1) Anything not 64 bytes could be allowed 2) Anything above 64 bytes could be allowed In the Great Consensus Cleanup, suggestion (2) was the route taken. It would not allow an "empty" OP_RETURN but would reduce the required padding from 22 bytes to 5. The functional test is also modified to test the actual case we care about: 64 bytes
a4defcd test, lint: add `crypted` to `ignore-words` (brunoerg) Pull request description: Fixes bitcoin#26719 "Crypted" is used in some comments at `walletload_tests` because it refers to `DBKeys::CRYPTED_KEY`, it's not necessary a mistake. Obs: I can change the approach (changing `walletload_tests` comments to use `encrypted` word instead of adding it to the `ignore_words`) if reviewers think it makes more sense. ACKs for top commit: achow101: ACK a4defcd Tree-SHA512: 49f38eed30ffb0fda4e792566591c3455629379619eb9a5c4240c5b00e14cd27ba1faa36337192233752e642f0998373b86fcb8ca586508bbf15900d68b17950
1b22849 qt: Drop no longer used `SplashScreen::finish()` slot (Hennadii Stepanov) 10811af qt: Drop no longer used `BitcoinApplication::splashFinished()` signal (Hennadii Stepanov) 5299cfe qt: Delete splash screen widget explicitly (Hennadii Stepanov) Pull request description: Fixes bitcoin-core/gui#604. Fixes bitcoin#25146. Fixes bitcoin#26340. `SplashScreen::deleteLater()` [does not guarantee](https://doc.qt.io/qt-5/qobject.html#deleteLater) deletion of the `m_splash` object prior to the wallet context deletion. If the latter happens first, the [segfault](bitcoin-core/gui#604 (comment)) follows. ACKs for top commit: dooglus: ACK bitcoin-core/gui@1b22849 furszy: ACK 1b22849 john-moffett: ACK 1b22849 Tree-SHA512: bb01d0bf2051f5b184dc415c4f5d32dfb7b8bd772feff7ec7754ded4c6482de27f004b9712df7d53c5ee82e153f48aef4372e4a49d7bcbbb137f73e9b4947962
2c07cfa gui: bumpfee signer support (Sjors Provoost) 7e02a33 rpc: bumpfee signer support (Sjors Provoost) 304ece9 rpc: document bools in FillPSBT() calls (Sjors Provoost) Pull request description: The `bumpfee` RPC call and GUI fee bump interface now work with an external signer. ACKs for top commit: achow101: ACK 2c07cfa furszy: code review ACK 2c07cfa jarolrod: tACK 2c07cfa Tree-SHA512: 0c7b931f76fac67c9e33b9b935f29af6f69ac67a5ffcc586ed2f1676feac427735b1d971723b29ef332bb6fb5762949598ebbf728587e8f0ded95a9bfbb3e7a4
17554ef test: prefer sqlite for wallet tests (S3RK) 8e0faba test: make wallet_migration.py pass with both wallet flags (S3RK) Pull request description: Fixes bitcoin#26511 ACKs for top commit: MarcoFalke: review ACK 17554ef achow101: ACK 17554ef Tree-SHA512: 97cae275998f07032feffe1b533d4747b8ff03c3c1fb830af69ee38cadb75fd58532956f66f79c0d275b00620ce53b0b5240f885e4f29b8bd4d0b6e6cbc683fa
…by 18-35% via `keypoolrefill()` 31fdc54 test: speed up wallet_fundrawtransaction.py and wallet_sendall.py (kdmukai) Pull request description: ## Problem `wallet_fundrawtransaction.py` and `wallet_sendall.py` are the two slowest functional tests *when running without a RAM disk*. ``` # M1 MacBook Pro timings wallet_fundrawtransaction.py --descriptors | ✓ Passed | 55 s wallet_fundrawtransaction.py --legacy-wallet | ✓ Passed | 381 s wallet_sendall.py --descriptors | ✓ Passed | 43 s wallet_sendall.py --legacy-wallet | ✓ Passed | 327 s ``` In each case, the majority of the time is spent iterating through 1500 to 1600 `getnewaddress()` calls. This is particularly slow in the `--legacy-wallet` runs. see: https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_fundrawtransaction.py#L986-L987 see: https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_sendall.py#L324 ## Solution Pre-fill the keypool before iterating through those `getnewaddress()` calls. With this change, the execution time drops to: ``` wallet_fundrawtransaction.py --descriptors | ✓ Passed | 52 s # -3s diff wallet_fundrawtransaction.py --legacy-wallet | ✓ Passed | 291 s # -90s diff wallet_sendall.py --descriptors | ✓ Passed | 27 s # -16s diff wallet_sendall.py --legacy-wallet | ✓ Passed | 228 s # -99s diff ``` --- Tagging @ Sjors as he had encouraged me to take a look at speeding up the tests. ACKs for top commit: achow101: ACK 31fdc54 Tree-SHA512: e8dd89323551779832a407d068977c827c09dff55c1079d3c19aab39fcce6957df22b1da797ed7aa3bc2f6dd22fdf9e6f5e1a9a0200fdb16ed6042fc5f6dd992
…er than istream_iterator bb5ea1d qt: Load PSBTs using istreambuf_iterator rather than istream_iterator (Andrew Chow) Pull request description: `istream_iterator` eats whitespace charactesr which causes parsing failures for PSBTs that contain the bytes corresponding to those characters. `istreambuf_iterator` is the correct thing to use here. This is a regression in 24.0. bitcoin#25001 accidentally changed the original `istreambuf_iterator` to `istream_iterator`. ACKs for top commit: furszy: Tested ACK bb5ea1d MarcoFalke: review ACK bb5ea1d 🍇 Tree-SHA512: 35d90eee3efdcb6a360af69ac1727f9f2837ea621297196de3136299f5de6d9975df4e425e1fc5b8813c1ddb2a4d60c3969e1d5d968953a4628ca45e37d3bf05
97115de doc: Refactor/Format getrawtransaction RPC docs and add ScriptPubKeyDoc function (Douglas Chimento) Pull request description: Added `ScriptPubKeyDoc` function ACKs for top commit: MarcoFalke: ACK 97115de kristapsk: cr utACK 97115de Tree-SHA512: 1371375986177862e8c99923eb7f1800fef8da7a7ac9f0ec9037bf5b23681c3348d5afe913aab7457f029ee1774d160ac10d7f57238500a03c6385cc0c7013fc
…tion properly d3a8434 ci: remove --prefix from msan job (fanquake) 574e50a ci: Use `CONFIG_SITE` variable and `--prefix` option properly (Hennadii Stepanov) Pull request description: When running CI scripts locally, they attempt to use a `$DEPENDS_DIR/$HOST` directory even `NO_DEPENDS=1` is provided. This PR fixes this broken behavior. Top commit has no ACKs. Tree-SHA512: 5e83b921763e6d463e520bbee2ed1599e9f4de36668d19b23dd9d2d7e4441c415e275f588c585b72cadda8bfab5a938979acc1ee4963230aa47081785c741e98
…'t run unsigned arm64 binaries; self-sign when needed dc12f2e test: improve error msg on previous release tarball extraction failure (kdmukai) 7121fd8 test: self-sign previous release binaries for arm64 macOS (kdmukai) Pull request description: ## The Problem If you run `test/get_previous_releases.py -b` on an M1 or M2 mac, you'll get an unsigned v23.0 binary in the arm64 tarball. macOS [sets stricter requirements on ARM binaries](https://news.ycombinator.com/item?id=26996578) so the unsigned arm64 binary is apparently completely unusable without being signed/notarized(?). This means that any test that depends on a previous release (e.g. `wallet_backwards_compatibility.py`) will fail because the v23.0 node cannot launch: ``` TestFramework (ERROR): Assertion failed Traceback (most recent call last): File "/Users/kdmukai/dev/bitcoin-core/test/functional/test_framework/test_framework.py", line 563, in start_nodes node.wait_for_rpc_connection() File "/Users/kdmukai/dev/bitcoin-core/test/functional/test_framework/test_node.py", line 231, in wait_for_rpc_connection raise FailedToStartError(self._node_msg( test_framework.test_node.FailedToStartError: [node 2] bitcoind exited with status -9 during initialization ``` This can also be confirmed by downloading bitcoin-23.0-arm64-apple-darwin.tar.gz (https://bitcoincore.org/bin/bitcoin-core-23.0/) and trying to run any of the binaries manually on an M1 or M2 mac. ## Solution in this PR (UPDATED) Per @ hebasto, we can self-sign the arm64 binaries. This PR checks each binary in the previous release's "bin/" and verifies if the arm64 binary is signed. If not, attempt to self-sign and confirm success. (note: an earlier version of this PR downloaded the x86_64 binary as a workaround but this approach has been discarded) ## Longer term solution If possible, produce signed arm64 binaries in a future v23.x tarball? Note that this same problem affects the new v24.0.1 arm64 tarball so perhaps a signed v24.x.x tarball would also be ideal? That being said, this PR will check all current and future arm64 binaries and self-sign as needed, so perhaps we need not worry about pre-signing the tarball binaries. And I did test a version of `get_previous_releases.py` that includes the new v24.0.1 binaries and it successfully self-signed both v23.0 and v24.0.1, as expected. ## Further info: Somewhat related to: bitcoin#15774 (comment) And @ fanquake noted on IRC that you can confirm which binaries are or are not signed via: ``` $ codesign -v -d bitcoin-qt bitcoin-qt: code object is not signed at all ``` ACKs for top commit: hebasto: ACK dc12f2e Tree-SHA512: 644895f8e97f5ffb3c4754c1db2c48abd77fa100c2058e3c896af04806596fc2b9c807a3f3a2add5be53301ad40ca2b8171585bd254e691f6eb38714d938396b
… 65 non-witness bytes b2aa9e8 Add release note for MIN_STANDARD_TX_NONWITNESS_SIZE relaxation (Greg Sanders) 8c5b364 Relax MIN_STANDARD_TX_NONWITNESS_SIZE to 65 non-witness bytes (Greg Sanders) Pull request description: Since the original fix was set to be a "reasonable" transaction to reduce allocations and the true motivation later revealed, it makes sense to relax this check to something more principled. There are more exotic transaction patterns that could take advantage of a relaxed requirement, such as 1 input, 1 output OP_RETURN to burn a utxo to fees for CPFP purposes when change isn't practical. Two changes could be accomplished: 1) Anything not 64 bytes could be allowed 2) Anything above 64 bytes could be allowed In the Great Consensus Cleanup, suggestion (2) was proposed as a consensus change, and is the simpler of the two suggestions. It would not allow an "empty" OP_RETURN but would reduce the required padding from 22 bytes to 5. The functional test is also modified to test the actual case we care about: 64 bytes Related mailing list discussions here: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-October/020995.html And a couple years earlier: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2020-May/017883.html ACKs for top commit: achow101: reACK b2aa9e8 glozow: reACK b2aa9e8 pablomartin4btc: re-ACK bitcoin@b2aa9e8 jonatack: ACK b2aa9e8 with some suggestions Tree-SHA512: c1ec1af9ddcf31b2272209a4f1ee0c5607399f8172e5a1dfd4604cf98bfb933810dd9369a5917ad122add003327c9fcf6ee26995de3aca41d5c42dba527991ad
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[<!--](`
`)
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->