Andy19a #4
Open
Andy19a #4
Conversation
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
Building with Boost 1.73.0 is broken.
This adds a data structure representing the optimization state for the spanning-forest linearization algorithm (SFL), plus a fuzz test for its correctness. This is preparation for switching over Linearize() to use this algorithm. See https://delvingbitcoin.org/t/spanning-forest-cluster-linearization/1419 for a description of the algorithm.
Rather than using an ad-hoc no-dependency copy of the graph when a potentially non-topological linearization is needed in the clusterlin fuzz test, add this directly as a feature in ReadLinearization(). This is preparation for a later commit where another use for such a function is added.
This replaces the existing LIMO linearization algorithm (which internally uses ancestor set finding and candidate set finding) with the much more performant spanning-forest linearization algorithm. This removes the old candidate-set search algorithm, and several of its tests, benchmarks, and needed utility code. The worst case time per cost is similar to the previous algorithm, so ACCEPTABLE_ITERS is unchanged.
This introduces a queue of chunks that still need processing, in both MakeTopological() and OptimizationStep(). This is simultaneously: * A preparation for introducing randomization, by allowing permuting the queue. * An improvement to the fairness of suboptimal solutions, by distributing the work more fairly over chunks. * An optimization, by avoiding retrying chunks over and over again which are already known to be optimal.
This introduces a local RNG inside the SFL state, which is used to randomize various decisions inside the algorithm, in order to make it hard to create pathological clusters which predictably have bad performance. The decisions being randomized are: * When deciding what chunk to attempt to split, the queue order is randomized. * When deciding which dependency to split on, a uniformly random one is chosen among those with higher top feerate than bottom feerate within the chosen chunk. * When deciding which chunks to merge, a uniformly random one among those with the higher feerate difference is picked. * When merging two chunks, a uniformly random dependency between them is now activated. * When making the state topological, the queue of chunks to process is randomized.
This places equal-feerate chunks (with no dependencies between them) in random order in the linearization output, hiding information about DepGraph insertion order from the output. Likewise, it randomizes the order of transactions within chunks for the same reason.
This ended up never being used in txgraph.
With MergeLinearizations() gone and the LIMO-based Linearize() replaced by SFL, we do not need a class (LinearizationChunking) that can maintain an incrementally-improving chunk set anymore. Replace it with a function (ChunkLinearizationInfo) that just computes the chunks as SetInfos once, and returns them as a vector. This simplifies several call sites too.
Fixes #34101 by reverting `boost::multi_index::contains` calls only available in Boost 1.78.0
Replace the last few instances of `.count() != 0` and `.count() == 0` and `.count()` patterns with the more expressive C++20 `.contains()` method: * `std::set<std::string>` in `getblocktemplate` RPC; * `std::map<std::string, ...>` in `transaction_tests`; * other bare `std::unordered_set` and `std::map` count calls. With no remaining violations, enable the `readability-container-contains` clang-tidy check to prevent future regressions.
This frees up the name getCoinbaseTx() for the next commit. Changing a function name does not impact IPC clients, as they only consider the function signature and sequence number.
75bdb92 clusterlin: drop support for improvable chunking (simplification) (Pieter Wuille) 91399a7 clusterlin: remove unused MergeLinearizations (cleanup) (Pieter Wuille) 5ce2800 clusterlin: randomize equal-feerate parts of linearization (privacy) (Pieter Wuille) 13aad26 clusterlin: randomize various decisions in SFL (feature) (Pieter Wuille) ddbfa4d clusterlin: keep FIFO queue of improvable chunks (preparation) (Pieter Wuille) 3efc94d clusterlin: replace cluster linearization with SFL (feature) (Pieter Wuille) 6a8fa82 clusterlin: add support for loading existing linearization (feature) (Pieter Wuille) da48ed9 clusterlin: ReadLinearization for non-topological (tests) (Pieter Wuille) c461259 clusterlin: add class implementing SFL state (preparation) (Pieter Wuille) 95bfe7d clusterlin: replace benchmarks with SFL-hard ones (bench) (Pieter Wuille) 86dd550 clusterlin: add known-correct optimal linearization tests (tests) (Pieter Wuille) Pull request description: Part of cluster mempool: #30289. This replaces the cluster linearization algorithm introduced in #30126 and #30286 (a combination of LIMO with candidate-set search), with a completely different algorithm: [spanning-forest linearization](https://delvingbitcoin.org/t/spanning-forest-cluster-linearization/1419/1), which appears to have much better performance for hard clusters. See [this post](https://delvingbitcoin.org/t/how-to-linearize-your-cluster/303/68) for a comparison between various linearization algorithms, and [this post](https://delvingbitcoin.org/t/how-to-linearize-your-cluster/303/73) for benchmarks comparing them. Replaying historical mempool data on it shows that it can effectively linearize every observed cluster up to 64 transactions optimally within tens of microseconds, though pathological examples can be created which take longer. The algorithm is effectively a very specialized version of the [simplex algorithm](https://en.wikipedia.org/wiki/Simplex_algorithm) to the problem of finding high-feerate topological subsets of clusters, but modified to find all consecutive such subsets concurrently rather than just the first one. See the post above for how it is related. It represents the cluster as partitioned into a set of chunks, each with a spanning tree of its internal dependencies connecting the transactions. Randomized improvements are made by selecting dependencies to add and remove to these spanning trees, merging and splitting chunks, until no more improvements are possible, or a computation budget is reached. Like simplex, it does not necessarily make progress in every step, and thus has no upper bound on its runtime to find optimal, but randomization makes long runtimes very unlikely, and additionally makes it hard to adversarially construct clusters in which the algorithm reliably makes bad choices. ACKs for top commit: instagibbs: reACK 75bdb92 marcofleon: reACK 75bdb92 Tree-SHA512: 189d85b34f0eb847562af7da724c61e39f0a785e24ebe2d4c8ee44698d02bd17842d699987d282a79bd1de30f50de28ec0f11d594ebbfa499f6a9b9ce35aecd8
aeb7ccb doc: add missing copyright headers (fanquake) 68a7cb8 contrib: output copyright in generate-seeds.py (fanquake) Pull request description: Takes care of some queries from #34084. ACKs for top commit: rkrux: tACK aeb7ccb janb84: ACK aeb7ccb Tree-SHA512: 12bb8fe58e0e84d4e1dcba94f95da2ebb0518208023459cb8ca81c1f95715749a6009cda8fe140dcde809fe35bdae10ee55b5eeea2cacfa30c58b1caefb2b521
f480c1e build: Update minimum required Boost version (Hennadii Stepanov) Pull request description: Building with Boost 1.73.0 has been [broken](#34095 (review)) since #31122 was merged. This PR updates the minimum required Boost version to 1.74.0. According to https://repology.org/project/boost/versions, none of the major distros are affected by this change. ACKs for top commit: maflcko: lgtm ACK f480c1e l0rinc: untested ACK f480c1e pablomartin4btc: utACK f480c1e janb84: ut ACK f480c1e Tree-SHA512: 6af72a001a566fb5a7b60e23bdb9619e87f277a1a3928ceb304bd35b8b35f56e4d38f25983db9a8732ecdb957cec9850a0fcf6719f3a65d903872e63d80b4d7c
…itting fa33605 Move ci_exec to the Python script (MarcoFalke) fa83555 ci: Require rsync to pass (MarcoFalke) eeee02e ci: Untangle CI_EXEC bash function (MarcoFalke) fa21fd1 ci: Move macos snippet under DANGER_RUN_CI_ON_HOST (MarcoFalke) fa37559 ci: Document the retry script in PATH (MarcoFalke) 666675e ci: Move folder creation and docker kill to Python script (MarcoFalke) Pull request description: The remaining `ci/test/02_run_container.sh` is fine, but has a bunch of shellcheck SC2086 word splitting violations. This is fine currently, because the only place that needed them had additional escaping, and all other commands happened to split fine on spaces. However, this may change in the future. So fix it now, by rewriting it in Python, which is recommended in the dev notes. ACKs for top commit: frankomosh: Code Review ACK [fa33605](fa33605) m3dwards: ACK fa33605 Tree-SHA512: 472decb13edca75566dffe49b9b3f554ab977fa60ec7902d5a060fe53381aee8606a10ff0c990a62ee2454dc6d9430cc064f58320b9043070b7bf08845413bf4
…-tidy rule 1e94e56 refactor: enable `readability-container-contains` clang-tidy rule (Lőrinc) fd9f1ac Fix compilation for old Boost versions (Lőrinc) Pull request description: Replace the last few instances of `.count() != 0` and `.count() == 0` and bare `count()` patterns with the more expressive C++20 `.contains()` method: * `std::set<std::string>` in `getblocktemplate` RPC; * `std::map<std::string, ...>` in `transaction_tests`; * other bare `std::unordered_set` and `std::map` count calls. Also fixes #34101 by reverting `boost::multi_index::contains` calls not available in our minimum supported version. With no remaining violations, enable the `readability-container-contains` clang-tidy check to prevent future regressions. Follow-up to #33192 ACKs for top commit: hebasto: ACK 1e94e56. pablomartin4btc: re-ACK 1e94e56 janb84: ACK 1e94e56 rkrux: re-ACK 1e94e56 Tree-SHA512: d54a7821d319bf0d60b6c3a870917464a7d5b9279c6a86708c03a3516ec23bbf18f0e83de62b3b2b1607de96e1470f1144b4918d69a6c770e6b7e09863e7dbac
fa4cb13 test: [doc] Manually unify stale headers (MarcoFalke) fa5f297 scripted-diff: [doc] Unify stale copyright headers (MarcoFalke) Pull request description: Historically, the upper year range in file headers was bumped manually or with a script. This has many issues: * The script is causing churn. See for example commit 306ccd4, or drive-by first-time contributions bumping them one-by-one. (A few from this year: #32008, #31642, #32963, ...) * Some, or likely most, upper year values were wrong. Reasons for incorrect dates could be code moves, cherry-picks, or simply bugs in the script. * The upper range is not needed for anything. * Anyone who wants to find the initial file creation date, or file history, can use `git log` or `git blame` to get more accurate results. * Many places are already using the `-present` suffix, with the meaning that the upper range is omitted. To fix all issues, this bumps the upper range of the copyright headers to `-present`. Further notes: * Obviously, the yearly 4-line bump commit for the build system (c.f. b537a2c) is fine and will remain. * For new code, the date range can be fully omitted, as it is done already by some developers. Obviously, developers are free to pick whatever style they want. One can list the commits for each style. * For example, to list all commits that use `-present`: `git log --format='%an (%ae) [%h: %s]' -S 'present The Bitcoin'`. * Alternatively, to list all commits that use no range at all: `git log --format='%an (%ae) [%h: %s]' -S '(c) The Bitcoin'`. <!-- * The lower range can be wrong as well, so it could be omitted as well, but this is left for a follow-up. A previous attempt was in #26817. ACKs for top commit: l0rinc: ACK fa4cb13 rkrux: re-ACK fa4cb13 janb84: ACK fa4cb13 Tree-SHA512: e5132781bdc4417d1e2922809b27ef4cf0abb37ffb68c65aab8a5391d3c917b61a18928ec2ec2c75ef5184cb79a5b8c8290d63e949220dbeab3bd2c0dfbdc4c5
-BEGIN VERIFY SCRIPT-
sed --in-place --regexp-extended \
's;( 20[0-2][0-9])(-20[0-2][0-9])? The Bitcoin Core developers;\1-present The Bitcoin Core developers;g' \
$( git grep -l 'The Bitcoin Core developers' -- ':(exclude)COPYING' ':(exclude)src/ipc/libmultiprocess' ':(exclude)src/minisketch' )
-END VERIFY SCRIPT-
Specifically gets rid of batchpriority, chainparams, script/sign.h and system includes. Also take the opportunity of cleaning up the headers for the effected files and adding them to the iwyu-enforced set.
This should avoid having to include interfaces/chain.h from a kernel module. interfaces/chain.h in turn includes a bunch of non-kernel headers, that break the desired library topology and might introduce entanglement regressions.
This project uses angle brackets instead of quotes for project-specific headers. Setting MainIncludeChar enables clang-format to automatically detect the main header, so it can be kept as the top group of includes. For example, without this change, the below command would demote <signet.h> from being the main header. With this change, the order is preserved. `clang-format -i src/signet.cpp`
Migration still needs to be able to restore unnamed wallets, so allow_unnamed is added to RestoreWallet to explicitly allow that behavior for migration only.
… cached Transaction hashes are cached, it may not be intuitive that their sizes are actually recalculated every time. This is done before the other refactors to clarify why we want to avoid calling this method; Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
`PeerManagerImpl::SendBlockTransactions()` computed the total byte size of requested transactions for a debug log line by calling `ComputeTotalSize()` in a tight loop, triggering serialization even when debug logging is off. Guard the size accumulation with `LogAcceptCategory` so the serialization work only happens when the log line can be emitted. No behavior change when debug logging is enabled: the reported block hash, transaction count, and byte totals are the same. The bounds checks still run unconditionally; the debug-only loop iterates the already-validated response contents. Separating debug-only work from the critical path reduces risk and favors the performance-critical non-debug case. This also narrows the racy scope of when logging is toggled from another thread.
…is disabled `PartiallyDownloadedBlock::FillBlock()` computed the block header hash and summed missing transaction sizes for debug logging unconditionally, including when cmpctblock debug logging is disabled. Guard the debug-only hash and size computations with `LogAcceptCategory`. Since `txn_available` is invalidated after the first loop (needed for efficient moving), we compute `tx_missing_size` by iterating `vtx_missing` directly. This is safe because the later `tx_missing_offset` check guarantees `vtx_missing` was fully consumed during reconstruction. Use `block.GetHash()` instead of `header.GetHash()`, since header is cleared before logging. No behavior change when debug logging is enabled: the reported counts, hashes, and byte totals remain the same.
…luation a7b5814 Fix 11-year-old mis-categorized error code in OP_IF evaluation (Calin Culianu) Pull request description: This was introduced by commit ab9edbd. It appears the original author may have gotten tired and pasted the wrong error code into this 1 place. Every other situation where the value stack lacks the required number of arguments for the op-code, SCRIPT_ERR_INVALID_STACK_OPERATION is reported. Not so here. This commit fixes the situation. EDIT: Note this turns out to be a dupe of the abandoned #30359 . ACKs for top commit: billymcbip: tACK a7b5814 achow101: ACK a7b5814 darosior: utACK a7b5814 sedited: ACK a7b5814 Tree-SHA512: e8c01a3e2448b5d49b76a0cab3f38a2d0249b71beeb7d9d05d5ecc3812bd91d0bd1d0f78b809b6f4ccb73186fa119cb1ed3779a73284b83a67ae219ef378fa6c
…s for getreceivedbyaddress d45ec3f test: Add getreceivedbyaddress coverage to wallet_listreceivedby (b-l-u-e) Pull request description: This PR adds comprehensive functional test coverage for the `getreceivedbyaddress` RPC method. ACKs for top commit: maflcko: lgtm ACK d45ec3f fjahr: reACK d45ec3f achow101: ACK d45ec3f rkrux: lgtm ACK d45ec3f Tree-SHA512: e7f024297c18b2e11da108d9588bbf96089dce24e2fdee255dbd2f754f21ec63cb3cefa6d92b621b5ab66d18fe29523b87d14ceba38a83afa4c85eb5944a0fb3
0dafc0d clang-format: use AngleBracket for main includes (stickies-v) Pull request description: This project uses angle brackets instead of quotes for project-specific headers. Setting [`MainIncludeChar`](https://clang.llvm.org/docs/ClangFormatStyleOptions.html#mainincludechar) enables `clang-format` to automatically detect the main header, so it can be kept as the top group of includes. For example, without this change, `clang-format` would demote `<signet.h>` from being the main header in `src/signet.cpp`. With this change, the order is preserved. On 5e49f5d: ``` % clang-format src/signet.cpp | head -n 15 // Copyright (c) 2019-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include <consensus/merkle.h> #include <consensus/params.h> #include <consensus/validation.h> #include <logging.h> #include <primitives/block.h> #include <primitives/transaction.h> #include <script/interpreter.h> #include <script/script.h> #include <signet.h> #include <streams.h> #include <uint256.h> ``` With this PR: ``` % clang-format src/signet.cpp | head -n 10 // Copyright (c) 2019-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include <signet.h> #include <consensus/merkle.h> #include <consensus/params.h> #include <consensus/validation.h> #include <logging.h> ``` Note: `AngleBracket` `requires clang-format 19`, and will cause older versions (including our current minimum llvm version `17`) to fail ACKs for top commit: maflcko: review ACK 0dafc0d sedited: Nice, ACK 0dafc0d hebasto: ACK 0dafc0d, tested on Ubuntu 25.10. Tree-SHA512: c0876f505ec188f76e435af0731c411c66266b83e4c08528d0637263abcd84b3968ee6fbfa72630192f1a0cd2728af873d3d6c32f93ab8b228222fad16f232be
03f363d doc: Document IWYU workaround (Hennadii Stepanov) Pull request description: This PR addresses the following comments: - #34079 (comment): > it would be good to reduce and report this bug upstream. Otherwise, wide-spread use of iwyu in this code-base seems risky. - #34079 (comment): > Would have been good if it was documented, rather than adding undocumented workarounds for buggy tools. ACKs for top commit: maflcko: lgtm ACK 03f363d sedited: ACK 03f363d Tree-SHA512: 160a963c07f853995c8b4741a6ccca1d8431a576c760fca082116cebde4d133f7c8ec51f09e8f85f54428f86bad2635e1bd708177eecf71feb0bf1489f1e2b3e
d938947 doc: Add "Using IWYU" to Developer Notes (Hennadii Stepanov) e1a90bc iwyu: Do not export `crypto/hex_base.h` header (Hennadii Stepanov) 19a2edd iwyu: Do not export C++ headers in most cases (Hennadii Stepanov) Pull request description: First two commits address comments from discussion in #33779: - #33779 (comment) - #33779 (comment) The last commit adds a new section to the Developer Notes to document IWYU usage. ACKs for top commit: maflcko: re-ACK d938947 🚀 Tree-SHA512: 8d6c63e9d2fd190815211d80e654cb7379d16b6611b8851444f49bbbaa0fbc93557675fbcc558afd9a1cdf643570fba5eff9c1aecb5530f978797387b10b9a11
9482f00 chore: Update outdated GitHub Actions versions (Padraic Slattery) Pull request description: This PR updates outdated GitHub Action versions to ensure compatibility and improve functionality. The following changes are made to the GitHub Actions: - `actions/upload-artifact` updated from v4 to v6 - `actions/cache` updated from v4 to v5 - `actions/download-artifact` updated from v5 to v7 The updates are necessary to support newer environments and features, and ensure consistent behavior across different workflows. The changes will be tested in the CI pipeline of the pull request. ACKs for top commit: fanquake: ACK 9482f00 Tree-SHA512: 248e79162c5b2748e1a367d87a360d62eb961c24b4f8060bb932ef99a79ef10cab3e65175c092226c90140f31686fb9424911e6609729cb186b304b598a9af44
de509c6 iwyu: Add missed line to IWYU patch (Hennadii Stepanov) Pull request description: This PR makes IWYU suggest `<cassert>` over `<assert.h>`. Fixes #34237. ACKs for top commit: maflcko: lgtm ACK de509c6 Tree-SHA512: edba91eaf36992f684be2920f5da8c13a25ba6d79b879b92193e2af106cd454a64d7c4cf9dabc25675490df9edbccff1fd54c9f393e984a3a7a628b1c65f6c53
…s) fuzz targets fabf8d1 fuzz: Restore SendMessages coverage in process_message(s) fuzz targets (MarcoFalke) fac7fed refactor: Use std::reference_wrapper<AddrMan> in Connman (MarcoFalke) Pull request description: *Found and reported by Crypt-iQ (thanks!)* Currently the process_message(s) fuzz targets do not have any meaningful `SendMessages` code coverage. This is not ideal. Fix the problem by adding back the coverage, and by hardening the code here, so that the problem hopefully does not happen again in the future. ### Historic context for this regression The regression was introduced in commit fa11eea, which built a new deterministic peerman object. However, the patch was incomplete, because it was missing one hunk to replace `g_setup->m_node.peerman->SendMessages(&p2p_node);` with `peerman->SendMessages(&p2p_node);`. This means the stale and empty peerman from the node context and not the freshly created and deterministic peerman was used. A simple fix would be to just submit the missing patch hunk. However, this still leaves the risk that the issue is re-introduced at any time in the future. So instead, I think the stale and empty peerman should be de-constructed, so that any call to it will lead to a hard sanitizer error and fuzz failure. Doing that also uncovered another issue: The connman was holding on to a reference to a stale and empty addrman. So fix all issues by: * Allowing the addrman reference in connman to be re-seatable * Clearing all stale objects, before creating new objects, and then using references to the new objects in all code ACKs for top commit: Crypt-iQ: crACK fabf8d1 frankomosh: ACK fabf8d1 marcofleon: code review ACK fabf8d1 sedited: ACK fabf8d1 Tree-SHA512: 2e478102b3e928dc7505f00c08d4b9e4f8368407b100bc88f3eb3b82aa6fea5a45bae736c211f5af1551ca0de1a5ffd4a5d196d9473d4c3b87cfed57c9a0b69d
…criptors with private key information when wallet contains descriptors missing any key 9c7e477 test: Test listdescs with priv works even with missing priv keys (Novo) ed945a6 walletrpc: reject listdes with priv key on w-only wallets (Novo) 9e5e982 descriptor: ToPrivateString() pass if at least 1 priv key exists (Novo) 5c4db25 descriptor: refactor ToPrivateString for providers (Novo) 2dc74e3 wallet/migration: use HavePrivateKeys in place of ToPrivateString (Novo) e842eb9 descriptors: add HavePrivateKeys() (Novo) Pull request description: _TLDR: Currently, `listdescriptors [private=true]` will fail for a non-watch-only wallet if any descriptor has a missing private key(e.g `tr()`, `multi()`, etc.). This PR changes that while making sure `listdescriptors [private=true]` still fails if there no private keys. Closes #32078_ In non-watch-only wallets, it's possible to import descriptors as long as at least one private key is included. It's important that users can still view these descriptors when they need to create a backup—even if some private keys are missing ([#32078 (comment)](#32078 (comment))). This change makes it possible to do so. This change also helps prevent `listdescriptors true` from failing completely, because one descriptor is missing some private keys. ### Notes - The new behaviour is applied to all descriptors including miniscript descriptors - `listdescriptors true` still fails for watch-only wallets to preserve existing behaviour #24361 (comment) - Wallet migration logic previously used `Descriptor::ToPrivateString()` to determine which descriptor was watchonly. This means that modifying the `ToPrivateString()` behaviour caused descriptors that were previously recognized as "watchonly" to be "non-watchonly". **In order to keep the scope of this PR limited to the RPC behaviour, this PR uses a different method to determine `watchonly` descriptors for the purpose of wallet migration.** A follow-up PR can be opened to update migration logic to exclude descriptors with some private keys from the `watchonly` migration wallet. ### Relevant PRs #24361 #32186 ### Testing Functional tests were added to test the new behaviour EDIT **`listdescriptors [private=true]` will still fail when there are no private keys because non-watchonly wallets must have private keys and calling `listdescriptors [private=true]` for watchonly wallet returns an error** ACKs for top commit: Sjors: ACK 9c7e477 achow101: ACK 9c7e477 w0xlt: reACK 9c7e477 with minor nits rkrux: re-ACK 9c7e477 Tree-SHA512: f9b3b2c3e5425a26e158882e39e82e15b7cb13ffbfb6a5fa2868c79526e9b178fcc3cd88d3e2e286f64819d041f687353780bbcf5a355c63a136fb8179698b60
a3c71c7 [test] Add BIP 328 test vectors for Musig2 (w0xlt) Pull request description: Built on #31244 This PR adds explicit tests for Bitcoin Core's MuSig2 interface. Any issues in musig2.{cpp,h} will likely also be caught by the descriptor tests, but having more detailed tests for the MuSig2 class itself improves test reporting/coverage. It uses BIP 328 test vectors. ACKs for top commit: achow101: ACK a3c71c7 rkrux: lgtm ACK a3c71c7 Tree-SHA512: fc13beb5445c292cd7c75a47810fb1c4032ee2e3c1800dc44089b95959ccce8330291084bf788457e1d55c02d706ef04be7044badfee134149e004c44b19ec32
…act block logging is disabled 969c840 log,blocks: avoid `ComputeTotalSize` and `GetHash` work when logging is disabled (Lőrinc) babfda3 log,net: avoid `ComputeTotalSize` when logging is disabled (Lőrinc) 1658b8f refactor: rename `CTransaction::GetTotalSize` to signal that it's not cached (Lőrinc) Pull request description: ### Context The new accounting options introduced in #32582 can be quite heavy, and are not needed when debug logging is disabled. ### Problem `PartiallyDownloadedBlock::FillBlock()` and `PeerManagerImpl::SendBlockTransactions()` accumulate transaction sizes for debug logging by calling `ComputeTotalSize()` in loops, which invokes expensive `GetSerializeSize()` serializations. The block header hash is also only computed for the debug log. ### Fixes Guard the size and hash calculations with `LogAcceptCategory()` checks so the serialization and hashing work only occurs when compact block debug logging is enabled. Also modernized the surrounding code a bit since the change is quite trivial. ### Reproducer You can test the change by starting an up-to-date `bitcoind` node with `-debug=cmpctblock` and observing compact block log lines such as: > [cmpctblock] Successfully reconstructed block 00000000000000000001061eaa6c0fe79258e7f79606e67ac495765cb121a520 with 1 txn prefilled, 3122 txn from mempool (incl at least 3 from extra pool) and 641 txn (352126 bytes) requested <details> <summary>Test patch</summary> ```patch diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp index 58620c9..f16eb38fa5 100644 --- a/src/blockencodings.cpp +++ b/src/blockencodings.cpp @@ -186,6 +186,7 @@ bool PartiallyDownloadedBlock::IsTxAvailable(size_t index) const ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing, bool segwit_active) { + LogInfo("PartiallyDownloadedBlock::FillBlock called"); if (header.IsNull()) return READ_STATUS_INVALID; block = header; @@ -218,6 +219,7 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector< } if (LogAcceptCategory(BCLog::CMPCTBLOCK, BCLog::Level::Debug)) { + LogInfo("debug log enabled"); const uint256 hash{block.GetHash()}; // avoid cleared header uint32_t tx_missing_size{0}; for (const auto& tx : vtx_missing) tx_missing_size += tx->ComputeTotalSize(); // avoid cleared txn_available diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 5600c8d..c081825f77 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2470,6 +2470,7 @@ uint32_t PeerManagerImpl::GetFetchFlags(const Peer& peer) const void PeerManagerImpl::SendBlockTransactions(CNode& pfrom, Peer& peer, const CBlock& block, const BlockTransactionsRequest& req) { + LogInfo("PeerManagerImpl::SendBlockTransactions called"); BlockTransactions resp(req); for (size_t i = 0; i < req.indexes.size(); i++) { if (req.indexes[i] >= block.vtx.size()) { @@ -2480,6 +2481,7 @@ void PeerManagerImpl::SendBlockTransactions(CNode& pfrom, Peer& peer, const CBlo } if (LogAcceptCategory(BCLog::CMPCTBLOCK, BCLog::Level::Debug)) { + LogInfo("debug log enabled"); uint32_t tx_requested_size{0}; for (const auto i : req.indexes) tx_requested_size += block.vtx[i]->ComputeTotalSize(); LogDebug(BCLog::CMPCTBLOCK, "Peer %d sent us a GETBLOCKTXN for block %s, sending a BLOCKTXN with %u txns. (%u bytes)\n", pfrom.GetId(), block.GetHash().ToString(), resp.txn.size(), tx_requested_size); ``` </details> ACKs for top commit: davidgumberg: reACK 969c840 achow101: ACK 969c840 hodlinator: re-ACK 969c840 sedited: Re-ACK 969c840 danielabrozzoni: reACK 969c840 Tree-SHA512: 9780102d29778165144e3602d934ed4cb96660fd7b9ff2581b223c619e419139b8348e60f226af448702ae527736a1806d169b44342c5a82795590f664e16efe
… vtx count 3dd815f validation: pre-reserve leaves to prevent reallocs with odd vtx count (Lőrinc) 7fd47e0 bench: make `MerkleRoot` benchmark more representative (Lőrinc) f0a2183 test: adjust `ComputeMerkleRoot` tests (Lőrinc) Pull request description: #### Summary `ComputeMerkleRoot` [duplicates the last hash](https://github.com/bitcoin/bitcoin/blob/39b6c139bd6be33699af781f1d71f6fed303d468/src/consensus/merkle.cpp#L54-L56) when the input size is odd. If the caller provides a `std::vector` whose capacity equals its size, that extra `push_back` forces a reallocation, doubling its capacity (causing peak memory usage of 3x the necessary size). This affects roughly half of the created blocks (those with odd transaction counts), causing unnecessary memory fragmentation during every block validation. #### Fix * Pre-reserves vector capacity to account for the odd-count duplication using `(size + 1) & ~1ULL`. * This syntax produces [optimal assembly](#32497 (comment)) across x86/ARM and 32/64-bit platforms for GCC & Clang. * Eliminates default construction of `uint256` objects that are immediately overwritten by switching from `resize` to `reserve` + `push_back`. #### Memory Impact [Memory profiling](#32497 (comment)) shows **50% reduction in peak allocation** (576KB → 288KB) and elimination of reallocation overhead. #### Validation The benchmark was updated to use an odd leaf count to demonstrate the real-world scenario where the reallocation occurs. A full `-reindex-chainstate` up to block **896 408** ran without triggering the asserts. <details> <summary>Validation asserts</summary> Temporary asserts (not included in this PR) confirm that `push_back` never reallocates and that the coinbase witness hash remains null: ```cpp if (hashes.size() & 1) { assert(hashes.size() < hashes.capacity()); // TODO remove hashes.push_back(hashes.back()); } leaves.reserve((block.vtx.size() + 1) & ~1ULL); // capacity rounded up to even leaves.emplace_back(); assert(leaves.back().IsNull()); // TODO remove ``` </details> #### Benchmark Performance While the main purpose is to improve predictability, the reduced memory operations also improve hashing throughput slightly. ACKs for top commit: achow101: ACK 3dd815f optout21: reACK 3dd815f hodlinator: re-ACK 3dd815f vasild: ACK 3dd815f w0xlt: ACK 3dd815f with minor nits. danielabrozzoni: Code review ACK 3dd815f Tree-SHA512: e7b578f9deadc0de7d61c062c7f65c5e1d347548ead4a4bb74b056396ad7df3f1c564327edc219670e6e2b2cb51f4e1ccfd4f58dd414aeadf2008d427065c11f
I feel it'd be easier to debug intermittent test failures if the error message is present in the logs instead of error code. So, switching order of error code and message in the `try_rpc` function to aid error debugging.
faa59b3 util: Add Expected::swap() (MarcoFalke) fabb47e util: Implement Expected::operator*()&& (MarcoFalke) fab9721 util: Implement Expected::value()&& and Expected::error()&& (MarcoFalke) fac4800 util: Add Expected<void, E> specialization (MarcoFalke) fa6575d util: Make Expected::value() throw (MarcoFalke) fa1de11 util: Add Unexpected::error() (MarcoFalke) faa109f test: refactor: Use BOOST_CHECK_EQUAL over BOOST_CHECK == (MarcoFalke) fad4a9f Set bugprone-unused-return-value.AllowCastToVoid (MarcoFalke) Pull request description: Reviewers requested more member functions In #34006. They are currently unused, but bring the port closer to the original `std::expected` implementation: * Make `Expected::value()` throw when no value exists * Add `Unexpected::error()` methods * Add `Expected<void, E>` specialization * Add `Expected::value()&&` and `Expected::error()&&` methods * Add `Expected::swap()` Also, include a tiny tidy fixup: * tidy: Set `AllowCastToVoid` in the `bugprone-unused-return-value` check ACKs for top commit: stickies-v: re-ACK faa59b3 ryanofsky: Code review ACK faa59b3. Thanks for the update. The commit I objected to is fixed now and the rest of the implementation seems good enough for code that's probably temporary. hodlinator: re-ACK faa59b3 Tree-SHA512: b6ac28c1e7241837d9db83fe7534d713ca1283c20a77d2273743157d329f041ec0b503658d14b2f4425211808b61a88fed115d77149e0546825acd3bd9198edf
faa18dc refactor: Use std::bind_front over std::bind (MarcoFalke) Pull request description: `std::bind` has many issues: * It is verbosely listing all placeholders, but in a meaningless way, because it doesn't name the args or their types. * It silently ignores args passed to it, when one arg is overridden. For example [1] compiles fine on current master. * Accidentally duplicated placeholders compile fine as well. * Usually the placeholders aren't even needed. * This makes it hard to review, understand, and maintain. Fix all issues by using `std::bind_front` from C++20, which allows to drop the brittle `_1, _2, ...` placeholders. The replacement should be correct, if the trailing placeholders are ordered. Introducing the same silent bug on top of this pull request [2] will now lead to a compile failure. ---- [1] ```diff diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 694fb53..7661dd361e 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -412,3 +412,3 @@ void WalletModel::subscribeToCoreSignals() m_handler_status_changed = m_wallet->handleStatusChanged(std::bind(&NotifyKeyStoreStatusChanged, this)); - m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind(NotifyAddressBookChanged, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3, std::placeholders::_4, std::placeholders::_5)); + m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind(NotifyAddressBookChanged, this, CTxDestination{}, std::placeholders::_2, std::placeholders::_3, std::placeholders::_4, std::placeholders::_5)); m_handler_transaction_changed = m_wallet->handleTransactionChanged(std::bind(NotifyTransactionChanged, this, std::placeholders::_1, std::placeholders::_2)); ``` [2] ```diff diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 578713c..84cced741c 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -412,3 +412,3 @@ void WalletModel::subscribeToCoreSignals() m_handler_status_changed = m_wallet->handleStatusChanged(std::bind_front(&NotifyKeyStoreStatusChanged, this)); - m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind_front(NotifyAddressBookChanged, this)); + m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind_front(NotifyAddressBookChanged, this, CTxDestination{})); m_handler_transaction_changed = m_wallet->handleTransactionChanged(std::bind_front(NotifyTransactionChanged, this)); ACKs for top commit: janb84: cr ACK faa18dc fjahr: Code review ACK faa18dc hebasto: ACK faa18dc, I have reviewed the code and it looks OK. Tree-SHA512: 9dd13f49527e143a2beafbaae80b1358981f07a2ce20d25cffb1853089a32ff71639e6d718d1d193754522f9ac04e3e168ba017d5fc67a11a5918e79a92b3461
…misc.py timeout 7562e2a Squashed 'src/ipc/libmultiprocess/' changes from a4f92969649..1fc65008f7d (Ryan Ofsky) Pull request description: Includes: - bitcoin-core/libmultiprocess#229 - bitcoin-core/libmultiprocess#223 - bitcoin-core/libmultiprocess#225 - bitcoin-core/libmultiprocess#228 - bitcoin-core/libmultiprocess#230 - bitcoin-core/libmultiprocess#236 - bitcoin-core/libmultiprocess#234 - bitcoin-core/libmultiprocess#196 - bitcoin-core/libmultiprocess#237 The last change bitcoin-core/libmultiprocess#237 is expected to fix issue #34187 occasional `rpc_misc.py` test hang. The other changes are just documentation & ci updates. The changes can be verified by running `test/lint/git-subtree-check.sh src/ipc/libmultiprocess` as described in [developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#subtrees) and [lint instructions](https://github.com/bitcoin/bitcoin/tree/master/test/lint#git-subtree-checksh) ACKs for top commit: sedited: ACK 73d0fe6 Tree-SHA512: 82fb2973b8fb5e792dcff1adde0b158ac67cc6a9aeef4465e0bbdfbef88e41d44d9bad53b1d467be47bf594befa02bd8e7829c0a0bc67aaaeb4ca1e09b672a32
…ctor fab055c test: Scale NetworkThread close timeout with timeout_factor (MarcoFalke) Pull request description: Not sure if this fixes #34248, but scaling here probably makes sense, considering some CI setups run in nested VMs with a different arch system-qemu. ACKs for top commit: hebasto: ACK fab055c, the diff looks reasonable. Tree-SHA512: 98f9b0bdc3b02b692a14129f88c05f2df0d1e11e4167ff5d0cc6a3a6efd8994a743e969e83c71cb534537f134e07ba9a5cba3eb2010a6b6cf69bec959faf2c43
…d (default) wallet 75b704d wallettool: Disallow creating new unnamed wallets (Ava Chow) 5875a9c wallet: disallow unnamed wallets in createwallet and restorewallet (Ava Chow) d30ad4a wallet, rpc: Use HandleWalletError in createwallet (Ava Chow) Pull request description: We've been moving in the direction that all wallets must have a name. Therefore, we shouldn't allow creating new unnamed wallets. `createwallet`, `restorewallet`, and the wallet tool's `create` and `createfromdump` all now require the user to provide a non-empty wallet name when creating/restoring a wallet. The GUI is already enforcing this, but we were not enforcing it for RPCs or in the underlying `CreateWallet` and `RestoreWallet` functions. Wallet migration does still need to be able to restore unnamed wallets, so there is a new argument to `RestoreWallet` to explicitly allow that behavior for migration only. ACKs for top commit: rkrux: lgtm ACK 75b704d polespinasa: re ACK 75b704d Tree-SHA512: 8bde76d0b091e9276788c69412934af3426da2a7a69a00f94072d36c1a075cd41744ecdd5fef2b72870c1351b76aae061f124f716bb23f4839be20c464fc5ebd
0aba464 test: switch order of error code and message check (rkrux) Pull request description: I feel it'd be easier to debug intermittent test failures if the error message is present in the logs instead of error code. So, switching order of error code and message in the `try_rpc` function to aid error debugging. Should help in debugging #34354 IMO. It's an intermittent failure on Windows that I can't reproduce and it's more difficult to figure out what could have gone wrong only by seeing the error code like below in the CI logs. Given that the functional tests pass, I don't see a harm in checking for error message first and throwing it in case of a mismatch. ```python AssertionError: Unexpected JSONRPC error code -1 ``` <!-- *** 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 --> <!-- Please provide clear motivation for your patch and explain how it improves Bitcoin Core user experience or Bitcoin Core developer experience significantly: * Any test improvements or new tests that improve coverage are always welcome. * All other changes should have accompanying unit tests (see `src/test/`) or functional tests (see `test/`). Contributors should note which tests cover modified code. If no tests exist for a region of modified code, new tests should accompany the change. * Bug fixes are most welcome when they come with steps to reproduce or an explanation of the potential issue as well as reasoning for the way the bug was fixed. * Features are welcome, but might be rejected due to design or scope issues. If a feature is based on a lot of dependencies, contributors should first consider building the system outside of Bitcoin Core, if possible. * Refactoring changes are only accepted if they are required for a feature or bug fix or otherwise improve developer experience significantly. For example, most "code style" refactoring changes require a thorough explanation why they are useful, what downsides they have and why they *significantly* improve developer experience or avoid serious programming bugs. Note that code style is often a subjective matter. Unless they are explicitly mentioned to be preferred in the [developer notes](/doc/developer-notes.md), stylistic code changes are usually rejected. --> <!-- Bitcoin Core has a thorough review process and even the most trivial change needs to pass a lot of eyes and requires non-zero or even substantial time effort to review. There is a huge lack of active reviewers on the project, so patches often sit for a long time. --> ACKs for top commit: maflcko: lgtm ACK 0aba464 polespinasa: lgtm ACK 0aba464 fjahr: utACK 0aba464 brunoerg: code review ACK 0aba464 sedited: ACK 0aba464 Tree-SHA512: b09ba4b5d13a2c93a4a28a5c1b06af44a91295974236bb8326b74a988878c431e9ce0e19ec14bb98ac2b002da877abaa7da6a9851424453bcb494c0317b57227
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
19 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.
Avance