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
fa15a8d doc: Explain that low-effort pull requests may be closed (MarcoFalke) Pull request description: Lately, there seems to be a rise in low-effort pull requests. For example, where a contributor does not seem to understand the changes they are submitting, or it becomes clear that they have not tested the changes at all. I don't think such pull requests are helpful, as they extract precious review time, which could be better spent on reviewing pull requests by reviewers who care about understanding the changes they are submitting, and who ensure their changes are sound and tested. So document that such low-effort pull request may be closed. ACKs for top commit: l0rinc: ACK fa15a8d willcl-ark: ACK fa15a8d dergoegge: ACK fa15a8d pinheadmz: ACK fa15a8d Tree-SHA512: ba880f61c90c95e1e9007e337bad1a612a53ca85448f0ebfe97b34139489f22e5f709b8a0e302b11f71213e3b7863ab36ebd89b5c11cd550022d96493f917dd7
ccf9172 util: Remove `FilterHeaderHasher` (rustaceanrob) Pull request description: With respect to `std::unordered_map` documentation, the `Hash` type defined in the template is over the `Key` and not `T`, the value. This hasher is incorrectly named as the `FilterHeader` is the value within this map. I consider this a bug as opposed to a refactor as the key and value relationship is implied to be `filter header -> block hash` when it is the opposite. Further, the hasher for the key already exists via `BlockHasher`. ref: https://en.cppreference.com/w/cpp/container/unordered_map.html ACKs for top commit: andrewtoth: ACK ccf9172 maflcko: lgtm ACK ccf9172 ismaelsadeeq: ACK ccf9172 👍🏾 Tree-SHA512: 607602391bf337d4e25b04a6a643fa32c3ab4599009b181b46ecdb0705e8ff2af89a6192042453c9e8e44abcb2150589019f02c5c944ecdff41322c3e0ad45ac
…meDuration() eeee375 fuzz: Return chrono point from ConsumeTime(), Add ConsumeDuration() (MarcoFalke) faa5a9e fuzz: Use min option in ConsumeTime (MarcoFalke) Pull request description: Returning a raw i64 is a bit confusing when it comes to chrono types. For example, in the addrman fuzz tests, the `time_penalty` is not a time point, but a duration. Also, all call-sites assume second resolution right now, so document that better by returning `NodeSeconds` from `ConsumeTime(...)` and `std::chrono::seconds` from `ConsumeDuration(...)`. ACKs for top commit: l0rinc: ACK eeee375 Crypt-iQ: crACK eeee375 Tree-SHA512: 25dd779a1bf79fa42c6e69db0f0593ad4daa4c0d746e8e82a26bdd65391a27c38e484431056d4e2207b542c511a71cb536c259809728a7166b8d304c0490e321
Change this so we catch the case where the capnp shared libs have been
updated, and can no-longer be loaded by the Python module, resulting in
a skipped test, even though pycapnp is installed. i.e:
```bash
stderr:
Traceback (most recent call last):
File "/root/ci_scratch/build/test/functional/interface_ipc.py", line 20, in <module>
import capnp # type: ignore[import] # noqa: F401
^^^^^^^^^^^^
File "/usr/local/lib64/python3.14/site-packages/capnp/__init__.py", line 36, in <module>
from .version import version as __version__
File "/usr/local/lib64/python3.14/site-packages/capnp/version.py", line 1, in <module>
from .lib.capnp import _CAPNP_VERSION_MAJOR as LIBCAPNP_VERSION_MAJOR # noqa: F401
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ImportError: libcapnpc.so.1.0.1: cannot open shared object file: No such file or directory
```
Failing in this way should make it clear that `pycapnp` needs to be
reinstalled/rebuilt.
If `pycapnp` is not installed, the test still skips as expected:
```bash
Remaining jobs: [interface_ipc.py]
1/1 - interface_ipc.py skipped (capnp module not available.)
TEST | STATUS | DURATION
interface_ipc.py | ○ Skipped | 0 s
```
Fixes: #34016.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
…g-prefix-map` fa37928 build: Temporarily remove confusing and brittle -fdebug-prefix-map (MarcoFalke) Pull request description: The compiler option `-fdebug-prefix-map` is unconditionally set by the build system. This is problematic for many reasons: * Users and devs have no easy way to disable it without modifying the build system source code * The mapping is broken since the cmake migration, and requires manual fixups such as #31204 or #31957 Fix all issues by temporarily removing it. Though, the option is kept for the guix build, so that no change in behavior is observed for the release binaries. Fixes #31957 Fixes #31204 The option can be added back in the future, if there is any need to. Though, adding it back should ideally work out of the box, or at least provide easy workarounds for all commonly used tooling. ACKs for top commit: pinheadmz: ACK fa37928 l0rinc: ACK fa37928 hebasto: ACK fa37928. Tree-SHA512: 5c76faab36ec516b286c2b5b2404e1488c0c4fbc678904593b0acb9c8da9b1db1b41436a22e6aa2f2671650288ccf635554773ef3144dc1df6ea838afce07ecb
The section claims to be for ccache builds, however those are already fixed after commit 1cc58d3. If there are still any build or debug problems after that commit, dedicated instructions can be added back, along with exact steps to reproduce and test.
The -ffile-prefix-map option is no longer used and it seems fine to remove the warning about it possibly breaking coverage builds. If this needs documentation, the dev notes seem like a better place, because it also affects other places, such as depends. C.f. commit 407062f
a73a3ec doc: fix invalid arg name hints for bugprone validation (Lőrinc) Pull request description: The extra leading `=` or missing trailing `=` prevented clang-tidy's `bugprone-argument-comment` check from validating the parameter name, as it only matches comments formatted strictly as `/*parameter_name=*/` (see https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html). I have considered doing a scripted diff, but the values I found aren't so numerous and can easily be reviewed manually. ACKs for top commit: b-l-u-e: ACK a73a3ec tested and saw that argument comments now use the strict "/*param=*/" format required by bugprone-argument-comment Sjors: utACK a73a3ec maflcko: review ACK a73a3ec 🍦 Tree-SHA512: 31177934d645116f381668a0f945028d7e04fab1fc6185dd0e3b7451aab71f89f1e4dd07246db667d1c4734eea3e5d73433c8b0e09181b3ece47dacc8677401e
14e56970cb Merge bitcoin-core/secp256k1#1794: ecmult: Use size_t for array indices c7a52400d6 Merge bitcoin-core/secp256k1#1809: release cleanup: bump version after 0.7.1 ae7eb729c0 release cleanup: bump version after 0.7.1 1a53f4961f Merge bitcoin-core/secp256k1#1808: Prepare for 0.7.1 20a209f11c release: prepare for 0.7.1 c4b6a81a60 changelog: update in preparation for the v0.7.1 release ebb35882da Merge bitcoin-core/secp256k1#1796: bench: fail early if user inputs invalid value for SECP256K1_BENCH_ITERS c09215f7af bench: fail early if user inputs invalid value for SECP256K1_BENCH_ITERS 471e3a130d Merge bitcoin-core/secp256k1#1800: sage: verify Eisenstein integer connection for GLV constants 29ac4d8491 sage: verify Eisenstein integer connection for GLV constants 4721e077b4 Merge bitcoin-core/secp256k1#1793: doc/bench: added help text for SECP256K1_BENCH_ITERS env var for bench_ecmult bd5ced1fe1 doc/bench: added help text for SECP256K1_BENCH_ITERS env var for bench_ecmult 47eb70959a ecmult: Use size_t for array indices in _odd_multiplies_table bb1d199de5 ecmult: Use size_t for array indices into tables 2d9137ce9d Merge bitcoin-core/secp256k1#1764: group: Avoid using infinity field directly in other modules f9a944ff2d Merge bitcoin-core/secp256k1#1790: doc: include arg -DSECP256K1_USE_EXTERNAL_DEFAULT_CALLBACKS=ON for cmake 0406cfc4d1 doc: include arg -DUSE_EXTERNAL_DEFAULT_CALLBACKS=1 for cmake 8d445730ec Merge bitcoin-core/secp256k1#1783: Add VERIFY_CHECKs and documentation that flags must be 0 or 1 aa2a39c1a7 Merge bitcoin-core/secp256k1#1778: doc/bench: Added cmake build options to bench error messages 540fec8ae9 Merge bitcoin-core/secp256k1#1788: test: split monolithic ellswift test into independent cases d822b29021 test: split monolithic ellswift test into independent cases ae00c552df Add VERIFY_CHECKs that flags are 0 or 1 5c75183344 Merge bitcoin-core/secp256k1#1784: refactor: remove ret from secp256k1_ec_pubkey_serialize be5e4f02fd Merge bitcoin-core/secp256k1#1779: Add ARG_CHECKs to ensure "array of pointers" elements are non-NULL 3daab83a60 refactor: remove ret from secp256k1_ec_pubkey_serialize 8bcda186d2 test: Add non-NULL checks for "pointer of array" API functions 5a08c1bcdc Add ARG_CHECKs to ensure "array of pointers" elements are non-NULL 3b5b03f301 doc/bench: Added cmake build options to bench error messages e7f7083b53 Merge bitcoin-core/secp256k1#1774: refactor: split up internal pubkey serialization function into compressed/uncompressed variants b6c2a3cd77 Merge bitcoin-core/secp256k1#1761: ecmult_multi: reduce strauss memory usage by 30% f5e815f430 remove secp256k1_eckey_pubkey_serialize function 0d3659c547 use new `_eckey_pubkey_serialize{33,65}` functions in modules (ellswift,musig) adb76f82ea use new `_eckey_pubkey_serialize{33,65}` functions in public API fc7458ca3e introduce `secp256k1_eckey_pubkey_serialize{33,65}` functions c8206b1ce6 Merge bitcoin-core/secp256k1#1771: ci: Use Python virtual environment in "x86_64-macos-native" job f252da7e6e ci: Use Python virtual environment in "x86_64-macos-native" job 115b135fe8 Merge bitcoin-core/secp256k1#1763: bench: Use `ALIGNMENT` macro instead of hardcoded value 2f73e5281d group: Avoid using infinity field directly in other modules 153eea20c2 bench: Use `ALIGNMENT` macro instead of hardcoded value 26166c4f5f ecmult_multi: reduce strauss memory usage by 30% 7a2fff85e8 Merge bitcoin-core/secp256k1#1758: ci: Drop workaround for Valgrind older than 3.20.0 43e7b115f7 Merge bitcoin-core/secp256k1#1759: ci: Switch to macOS 15 Sequoia Intel-based image 8bc50b72ff ci: Switch to macOS 15 Sequoia Intel-based image c09519f0e3 ci: Drop workaround for Valgrind older than 3.20.0 git-subtree-dir: src/secp256k1 git-subtree-split: 14e56970cba37ffe4ee992c1e08707a16e22e345
…function 3f5211c test: remove child_one/child_two (w)txid variables (naiyoma) 7cfe790 test: replace ValidWitnessMalleatedTx class with function (naiyoma) 81675a7 test: use pre-generated chain (naiyoma) Pull request description: This PR refactors ` ValidWitnessMalleatedTx` class into a `build_malleated_tx_package` function. As a result, two tests are updated: `mempool_accept_wtxid` and `p2p_p2p_private_broadcast`. Also included are a few small refactors in mempool_accept_wtxid , (switching to MiniWallet, using a pre-mined chain, using txid directly.) Together, these changes reduce complexity and improve test runtime. ACKs for top commit: stratospher: reACK 3f5211c. cedwies: reACK 3f5211c maflcko: review ACK 3f5211c 👥 rkrux: ACK 3f5211c Tree-SHA512: 1fd02be3432fef6b68e54fbe8b15ed56d2699580bb13d0777b21f9cbe4c6d33bbb710541e3ca2fc93eab771d17bf1c427e4b08fa216d561bdb320cc6b36ac8fc
…warn for debug logs) b39291f doc: fix `-logips` description to clarify that non-debug logs can also contain IP addresses (Lőrinc) c7028d3 init: log that additional logs may contain privacy-sensitive information (Lőrinc) 31b771a net: move `privatebroadcast` logs to debug category (Lőrinc) Pull request description: ### Motivation The recently merged [private broadcast](#29415) is a privacy feature, and users may share `debug.log` with support. Unconditional `LogInfo()` messages that mention private broadcast and/or include (w)txids can leak sensitive context (e.g. which transactions a user originated). Since it's meant to be a private broadcast, we should minimize leaks. It's a best effort, it's not invalidated by other logs possibly leaking identifiable information, those can be addressed separately. We're not promising that the logs won't ever contain data that could be used against the user, but we should still try to minimize that data, especially for a feature that's advertised as privacy-focused. Follow up to [#29415 (comment)](#29415 (comment)) ### Changes * Move private-broadcast event logs from `LogInfo()` to `LogDebug(BCLog::PRIVBROADCAST, ...)`, so they are only emitted when `-debug=privatebroadcast` was explicitly provided. * Remove hardcoded `"[privatebroadcast]"` log-string prefixes (category logging already adds the prefix). * Keep warning at the default log level for startup failures. * Add an init log (not a warning since that would require excessive test framework updates) when any `-debug` categories are enabled that additional logs may contain privacy-sensitive information and should not be shared publicly. * Update a related startup arg (`-logips`) to clarify that clarify that non-debug logs can also contain IP addresses. ### Reproducer The new warning can be checked with: ```bash ./build/bin/bitcoind -printtoconsole=1 -stopatheight=1 -listen=0 -connect=0 | grep 'Debug logging is enabled' | wc -l 0 ./build/bin/bitcoind -printtoconsole=1 -stopatheight=1 -listen=0 -connect=0 -debug | grep 'Debug logging is enabled' | wc -l 1 ``` ACKs for top commit: janb84: re ACK b39291f vasild: ACK b39291f andrewtoth: ACK b39291f frankomosh: crACK b39291f .The approach and implementation look good. Moving private broadcast logs to debug only would effectively reduce privacy leaks for users sharing logs. sedited: ACK b39291f Tree-SHA512: feca25ebe72a03948ba436e25f9a682947966c4c09627e8f20201ef3872ddbce1c636cd82f06be1afdc09cb80da305058667c0c2eaeadeb351311155325ea06f
Also, fix whitespace in this function, while touching it. Can be reviewed via the git option --ignore-all-space
Add a ci/lint.py script to run the linter both locally or inside the CI (replacing .github/ci-lint-exec.py) which supports running from a worktree. Determines whether we are in a worktree, and mounts the real `.git` directory as a read-only volume if we are.
14f99cf rpc: make `uptime` monotonic across NTP jumps (Lőrinc) a9440b1 util: add `TicksSeconds` (Lőrinc) Pull request description: ### Problem `bitcoin-cli uptime` was derived from wall-clock time, so it could jump by large amounts when the system clock is corrected after `bitcoind` starts (e.g. on RTC-less systems syncing NTP). This breaks the expectation that uptime reflects process runtime. ### Fix Compute uptime from a [monotonic clock](https://en.cppreference.com/w/cpp/chrono/steady_clock.html) so it is immune to wall-clock jumps, and use that monotonic uptime for the RPC. GUI startup time is derived from wall clock time minus monotonic uptime so it remains sensible after clock corrections. ### Reproducer Revert the fix commit and run the `rpc_uptime` functional test (it should fail with `AssertionError: uptime should not jump with wall clock`): Or alternatively: ```bash cmake -B build && cmake --build build --target bitcoind bitcoin-cli -j$(nproc) DATA_DIR=$(mktemp -d) ./build/bin/bitcoind -regtest -datadir="$DATA_DIR" -connect=0 -daemon ./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" -rpcwait uptime sleep 1 ./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" setmocktime $(( $(date +%s) + 20000000 )) ./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" uptime ./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" stop ``` <details> <summary>Before (uptime jumps with wall clock)</summary> ```bash Bitcoin Core starting 0 20000001 Bitcoin Core stopping ``` </details> <details> <summary>After (uptime stays monotonic)</summary> ```bash Bitcoin Core starting 0 1 Bitcoin Core stopping ``` </details> ---------- Issue: #34326 ACKs for top commit: maflcko: review ACK 14f99cf 🎦 willcl-ark: tACK 14f99cf w0xlt: ACK 14f99cf sedited: ACK 14f99cf Tree-SHA512: 3909973f58666ffa0b784a6df087031b9e34d2022d354900a4dbb6cbe1d36285cd92770ee71350ebf64d6e8ab212d8ff0cd851f7dca1ec46ee2f19b417f53984
Verify that addresses announcing themselves (addr == source) are exempt from time penalties, while addresses announced by others receive the expected penalty.
The code comment mistakingly referred to "the deprecated getCoinbaseTx()", instead of getCoinbaseRawTx. This was missed in d59b4cd. Also rename parse_and_deserialize_coinbase_tx to make it more clear it refers to the deprecated method. Finally, this commit drops the getCoinbaseRawTx() call when testing template inspectors. The coinbase input check here is already covered by build_coinbase_test.
Previously the coinbase transaction generated by our miner code was not used downstream, because the getblocktemplate RPC excludes it. Since the Mining IPC interface was introduced in #30200 we do expose this dummy coinbase transaction. In Stratum v2 several parts of it are communicated downstream, including the scriptSig. This commit removes the dummy extraNonce from the coinbase scriptSig in block templates requested via IPC. This limits the scriptSig to what is essential for consensus (BIP34) and removes the need for external mining software to remove the dummy, or even ignore the scriptSig we provide and generate it some other way. This could cause problems if a future soft fork requires additional data to be committed here. A test is added to verify the new IPC behavior. It achieves this by introducing an include_dummy_extranonce option which defaults to false with all test code updated to set it to true. Because this option is not exposed via IPC, callers will no longer see it. The caller needs to ensure that for blocks 1 through 16 they pad the scriptSig in order to avoid bad-cb-length. Co-authored-by: Anthony Towns <aj@erisian.com.au>
Calling this low-level function from tests is confusing, and also makes it harder to change the peer manager implementation. So juse use the pre-existing test helpers to achieve the same.
It is not used in tests anymore.
The node is never nullptr. This can be reviewed with the git option: --word-diff-regex=.
The peer is never nullptr.
Introducing two names to refer to the peer makes it possible to have one refer to a non-null reference in a later commit.
This allows to skip nullptr checks later in the code, both mentally and literally.
This can be reviewed via the git option:
--word-diff-regex=.
-BEGIN VERIFY SCRIPT-
sed --regexp-extended --in-place '
/^bool PeerManagerImpl::SendMessages\(/,/^}$/ {
s/auto& peer\{maybe_peer\}; .. alias cleaned up .*/Peer\& peer{*maybe_peer};/;
s/peer->/peer./g;
s/\*peer\>/peer/g;
/CNode\* pto\{&node\}; .. alias removed .*/d;
s/pto->/node./g;
s/\*pto\>/node/g;
}
' src/net_processing.cpp
sed --regexp-extended --in-place '
/^void PeerManagerImpl::ProcessMessage\(/,/^}$/ {
/Peer\* peer\{&peer_alias_removed_in_later_commit};/d;
s/peer_alias_removed_in_later_commit/peer/;
s/peer->/peer./g;
s/\*peer\>/peer/g;
}
' src/net_processing.cpp
sed --regexp-extended --in-place '
/^bool PeerManagerImpl::ProcessMessages\(/,/^}$/ {
s/auto& peer\{maybe_peer\}; .. alias cleaned up .*/Peer\& peer{*maybe_peer};/;
s/peer->/peer./g;
s/\*peer\>/peer/g;
/CNode\* pfrom\{&node\}; .. alias removed .*/d;
s/pfrom->/node./g;
s/\*pfrom\>/node/g;
}
' src/net_processing.cpp
-END VERIFY SCRIPT-
Fix a typo and use a named arg, where the LLM suggested it.
This is used for argument parsing in the retry script, however we don't use the script with any arguments. So remove the unused code, and the dependency on gnu-getopt. This came up in the context of adding new CI jobs, where gnu-getopt might not be available, or working properly. It seemed easier to just remove the unused code, than look for more workarounds.
…llyCopyableMove=false fa88ac3 doc: Clarify why performance-move-const-arg.CheckTriviallyCopyableMove=false (MarcoFalke) Pull request description: Without this doc, there is a risk that the setting will be turned off, see #34514. The reason to disable it is to catch logic bugs, even on trivially copyable types: ```cpp #include <utility> void Eat(int&& food) { food = 0; }; int main() { int food{2}; Eat(std::move(food)); Eat(std::move(food)); // This should err } ``` ACKs for top commit: l0rinc: ACK fa88ac3 hebasto: ACK fa88ac3. sedited: ACK fa88ac3 Tree-SHA512: d1bda846a10190a2936084a06bd87418c6a3e4ababc298e4beb9bc9e1190bff430cbe973475d634eda5ef7863571c89bfa4b78ff63fcbd9ac10c42fd9d5fa23a
fe0b151 test: add a test for txgraph staging (Hao Xu) ef253a9 test: add block builder tests for txgraph (Hao Xu) 4a1ac31 test: add a chunk test for txgraph (Hao Xu) Pull request description: Add tests for cluster chunks, including: - txgraph_chunk_chain test: test chunk implementation for a simple chain style graph . - txgraph_staging test: test the staging feature for a basic graph. ACKs for top commit: instagibbs: reACK fe0b151 sipa: reACK fe0b151 Tree-SHA512: 01010a3b4e4163849df2912d1393be74d26eb199d0544cfbef58a498aca5153463a118f55a2f1cad2995552b74210031e659de8df6b88cbcffdffd2a1b464990
fa90277 ci: Use ubuntu-slim for [meta] runners (MarcoFalke) fa9627a ci: Rely on cmake --preset toolchain file (MarcoFalke) fa3f89a ci: Add check_manifests to ci-windows.py (MarcoFalke) 1111079 ci: Add run_tests step to ci-windows.py (MarcoFalke) fa56168 ci: [refactor] Add .github/ci-windows.py prepare_tests step (MarcoFalke) fa3e607 ci: Print verbose Windows CI build failure (MarcoFalke) 4444808 ci: [refactor] Add .github/ci-windows.py build step (MarcoFalke) fabdd4e ci: Refactor Windows CI into script (MarcoFalke) Pull request description: Just like all the other CI configs, the Windows one should print a single and readable build failure at the end. Also, includes a bunch of Windows CI refactors. ACKs for top commit: m3dwards: ACK fa90277 hebasto: ACK fa90277. willcl-ark: utACK fa90277 Tree-SHA512: 00443289e3d8b6d60d1347934d9d4b16098e8c36b6325467e5804b1869714201c4f7e932da3be44392c73e4713a1f52cd8af481894d36c6a281ba7238d43434e
Instead of returning a TxGraph::Ref from TxGraph::AddTransaction(), pass in a TxGraph::Ref& which is updated to refer to the new transaction in that graph. This cleans up the usage somewhat, avoiding the need for dummy Refs in CTxMemPoolEntry constructor calls, but the motivation is that a future commit will allow a callback to passed to MakeTxGraph to define a fallback order on the transaction objects. This does not work when a Ref is created separately from the CTxMemPoolEntry it ends up living in, as passing the newly-created Ref to the callback would be UB before it's emplaced in its final CTxMemPoolEntry.
This makes TxGraphImpl::Compact() invoke Cluster::Updated() on all affected clusters, in case they have internal GraphIndex values stored that may have become outdated with the renumbering of GraphIndex values that Compact() caused. No such GraphIndex values are currently stored, but this will change in a future commit.
Whenever a TxGraph::Ref is destroyed, if it by then still appears inside main-level clusters, wipe the chunk index entries for those clusters, to prevent having lingering indexes for transactions without Ref. This is preparation for enabling a callback being passed to MakeTxGraph to define a fallback order on objects. Once the Ref for a transaction is gone, it is not possible to invoke the callback anymore. To prevent the index becoming inconsistent, we need to immediately get rid of the index entries when the Ref disappears. This is not a problem, because such destructions necessarily will trigger a relinearization of the cluster (assuming there are transactions in it left) before becoming acceptable again, and the chunk ordering is not observable (through CompareMainOrder, or through the BlockBuilder interface) until that point. However, the index itself needs to remain consistent in the mean time, even if not meaningful.
This changes the order of transactions within a chunk to be: 1. Topology (parents before children) 2. Individual transaction feerate (high to low) 3. Individual transaction weight (small to large) 4. Random tiebreak (will be changed in a future commit) To do so, use a heap of topology-ready transactions within GetLinearization(), sorted by (2), (3), and (4). This is analogous to the order of chunks within a cluster, which is unchanged: 1. Topology (chunks after chunks they depend on) 2. Chunk feerate (high to low) 3. Chunk weight (small to large) 4. Random tiebreak (will be changed in a future commit)
…eature) This makes TxGraph track the equal-feerate-prefix size of all chunks in all clusters in the main graph, and uses it to sort chunks coming from distinct clusters. The order of chunks across clusters becomes: 1. Feerate (high to low) 2. Equal-feerate-prefix (small to large) 3. Cluster sequence number (old to new); this will be changed later. The equal-feerate-prefix size of a chunk C is defined as the sum of the weights of all chunks in the same cluster as C, with the same feerate as C, up to and including C itself, in linearization order (but excluding such chunks that appear after C). This is an approximation of sorting chunks from small to large across clusters, while remaining consistent with intra-cluster linearization order.
This allows passing in a fallback order comparator to Linearize(), which is used as final tiebreak when deciding the order of chunks and transactions within a chunk, rather than a random tiebreak. The order of transactions within a chunk becomes: 1. Topology (parents before children) 2. Individual transaction feerate (high to low) 3. Weight (small to large) 4. Fallback (low to high fallback order) The order of chunks within a cluster becomes: 1. Topology (chunks after their dependencies) 2. Feerate (high to low) 3. Weight (small to large) 4. Max-fallback (chunk with lowest maximum-fallback-tx first) For now, txgraph passes a naive comparator to Linearize(), which makes the cluster order deterministic when treating the input transactions as identified by the DepGraphIndex. However, since DepGraphIndexes are the result of possibly-randomized operations inside txgraph, this doesn't actually make txgraph's per-cluster ordering deterministic. That will be changed in a later commit, by using a txid-based fallback instead.
This is a small change to the txgraph fuzz test to make it used objects derived from TxGraph::Ref (SimTxObject) rather than TxGraph::Ref directly. This matches how the mempool uses CTxMemPoolEntry, which derives from TxGraph::Ref. This is preparation for a future commit which will introduce simulated txids to the transactions in this fuzz test, to be used as fallback order.
This adds an std::function<strong_ordering(Ref&,Ref&)> argument to the MakeTxGraph function, which can be used by the caller (e.g., mempool code) to provide a fallback order to TxGraph. This is just preparation; TxGraph does not yet use this fallback order for anything.
Add glue to make TxGraph use the fallback order provided to it, in the fallback comparator it provides to the cluster linearization code. The order of chunks within a cluster becomes: 1. Topology (chunks after their dependencies) 2. Feerate (high to low) 3. Weight (small to large) 4. Max-txid (chunk with lowest maximum-txid first) The order of transactions within a chunk becomes: 1. Topology (parents before children) 2. Individual transaction feerate (high to low) 3. Weight (small to large) 4. Txid (low to high txid) This makes optimal cluster linearization, both the order of chunks within a chunk, and the order of transactions within those chunks, completely deterministic.
This makes TxGraph also use the fallback order to decide the order of chunks from distinct clusters. The order of chunks across clusters becomes: 1. Feerate (high to low) 2. Equal-feerate-chunk-prefix (small to large) 3. Max-txid (chunk with lowest maximum-txid first) This makes the full TxGraph ordering fully deterministic as long as all clusters in it are optimally linearized.
…tion' less error-prone 4c0d4f6 refactor: interfaces, make 'createTransaction' less error-prone (furszy) e2c3ec9 refactor: move CreatedTransactionResult to types.h (furszy) 4537217 gui: remove AmountWithFeeExceedsBalance error special case (furszy) Pull request description: Bundle all function's outputs inside the `util::Result` returned object. Removals: - The input-output 'change_pos' ref arg from `createTransaction`, which has been a source of bugs in the past. - The 'fee' ref arg from `createTransaction`, which is currently only set when the transaction creation process succeeds. - The no longer needed `AmountWithFeeExceedsBalance` error (more info about its re-introduction at [#25269](#25269) and [#34299](#34299). Additionally, this PR moves the `CreatedTransactionResult` struct into its own file. This change is made to avoid further expanding the GUI dependencies on `wallet.h`. Structurally, the GUI should only access the model/interfaces and never the wallet directly. ACKs for top commit: stratospher: ACK 4c0d4f6. hebasto: ACK 4c0d4f6. Tree-SHA512: 4fc61f08ca2e66e46001defb3a2e852265713e75006c98f0c465bd48afe42e7b0d626d28d578741906fdd26e907d6919f06dc640c55c44efc3dfa766fdbf38a4
This ensures clang-format can properly restore empty lines between header groups that were previously stripped by fix_includes.py.
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Take advantage of the new secp256k1 option to avoid visibility attributes on API functions. While most users of a shared libsecp always want API functions exported so that they can actually be linked against, we always build it statically. When that static lib is linked into a (static or shared) libbitcoinkernel, by default its symbols end up exported there as well. As libsecp is an implementation detail of the kernel (and any future Core lib), its symbols should never be exported.
The bug was fixed in Visual Studio 18.0.
f8d2f30 ci: Extend diff context for clang-format (Hennadii Stepanov) Pull request description: This PR ensures `clang-format` can properly restore empty lines between header groups that were previously stripped by `fix_includes.py`. Addresses [this](#34448 (comment)) comment. ACKs for top commit: maflcko: lgtm ACK f8d2f30 l0rinc: untested code review ACK f8d2f30 sedited: ACK f8d2f30 Tree-SHA512: 97e5450fc15e415134aa1e74ee415f947173978d999afbc0f204e1cdb2f68309b763c8a2ad819c4597147fc15540600aa8c99477db64e961ad428d8f45f3d384
…unnecessary sleep fa8c895 Fixup TODO comment in feature_dbcrash.py; remove unnecessary sleep (MarcoFalke) Pull request description: Fixup some stale comments: * The `60 seconds` is outdated. It should say 120 seconds. However, just clarify that there is a timeout. * The TODO seems to imply that a timeout (failure to restart) can happen. However, I don't think we've seen it happen. So there isn't anything to do right now. Just remove the `TODO`, but keep the advice. Also, remove an unnecessary `time.sleep(1)`. If there is a need for it, a comment should explain why. ACKs for top commit: l0rinc: ACK fa8c895 Tree-SHA512: 5ee13b48fc4a5802f3fadb125d71118e01d2cb08ede9d310d6ed13acd8fb7b03185cad73c475c617054c4c4423156ea927a32d0e3a670c3cc13339b552dc8a5c
452c743 refactor: Remove workaround for resolved MSVC bug (Hennadii Stepanov) 7164a0c build: Bump VS minimum supported version to 18.3 (Hennadii Stepanov) Pull request description: The new [VS 18.0](https://learn.microsoft.com/en-us/visualstudio/releases/2026/release-notes) release includes numerous bug fixes. Bumped to v18.3.0 where [this](microsoft/vcpkg#22074) bug in the builtin vcpkg is [fixed](microsoft/vcpkg#22074 (comment)). ACKs for top commit: maflcko: review ACK 452c743 🍳 hodlinator: crACK 452c743 janb84: ACK 452c743 Tree-SHA512: a8f859d11d4cf0440cf7ff8353fd1babe90818356ef02eae28571a2a4a7960db1f85cdbc4f88b5fb8a1f8bf44bca8c8715cdfb9ea87997c3fcd81866cd0b156d
2ccfdb5 build: avoid exporting secp256k1 symbols (Cory Fields) Pull request description: Take advantage of the [new secp256k1 option to avoid visibility attributes](bitcoin-core/secp256k1#1696) on API functions. While most users of a shared libsecp always want API functions exported so that they can actually be linked against, we always build it statically. When that static lib is linked into a (static or shared) libbitcoinkernel, by default its symbols end up exported there as well. As libsecp is an implementation detail of the kernel (and any future Core lib), its symbols should never be exported. [This was the intended use for the above PR](bitcoin-core/secp256k1#1696 (comment)), looks like we just forgot to follow-up and actually hook it up. This is most easily tested by building with `-DBUILD_KERNEL_LIB=ON -DBUILD_SHARED_LIBS=ON` (with or without `-DREDUCE_EXPORTS=ON`) and inspecting via: ```bash nm -CD lib/libbitcoinkernel.so | grep secp ``` Before this change, secp's symbols will show up there. After, they should be absent. This should finally solve secp symbol visibility once and for all :) ACKs for top commit: hebasto: ACK 2ccfdb5, this is implemented exactly as I [tested](bitcoin-core/secp256k1#1696 (review)) the upstream PR. Tested on Fedora 43. stickies-v: tACK 2ccfdb5 Tree-SHA512: 664ea7a6f811c2743ad1b4d8913c61aab9b358931ee77895d35cdf8a5607fbb08facda085877c53d731afbf42a7220dcc752fc365a7625ee679c1547e1c674d0
… ThreadPool 38fd85c http: replace WorkQueue and threads handling for ThreadPool (furszy) c323f88 fuzz: add test case for threadpool (TheCharlatan) c528dd5 util: introduce general purpose thread pool (furszy) 6354b4f tests: log node JSON-RPC errors during test setup (furszy) 45930a7 http-server: guard against crashes from unhandled exceptions (furszy) Pull request description: This has been a recent discovery; the general thread pool class created for #26966, cleanly integrates into the HTTP server. It simplifies init, shutdown and requests execution logic. Replacing code that was never unit tested for code that is properly unit and fuzz tested. Although our functional test framework extensively uses this RPC interface (that’s how we’ve been ensuring its correct behavior so far - which is not the best). This clearly separates the responsibilities: The HTTP server now focuses solely on receiving and dispatching requests, while ThreadPool handles concurrency, queuing, and execution. This will also allows us to experiment with further performance improvements at the task queuing and execution level, such as a lock-free structure or task prioritization or any other implementation detail like coroutines in the future, without having to deal with HTTP code that lives on a different layer. Note: The rationale behind introducing the ThreadPool first is to be able to easily cherry-pick it across different working paths. Some of the ones that are benefited from it are #26966 for the parallelization of the indexes initial sync, #31132 for the parallelization of the inputs fetching procedure, #32061 for the libevent replacement, the kernel API #30595 (#30595 (comment)) to avoid blocking validation among others use cases not publicly available. Note 2: I could have created a wrapper around the existing code and replaced the `WorkQueue` in a subsequent commit, but it didn’t seem worth the extra commits and review effort. The `ThreadPool` implements essentially the same functionality in a more modern and cleaner way. ACKs for top commit: Eunovo: ReACK 38fd85c sedited: Re-ACK 38fd85c pinheadmz: ACK 38fd85c Tree-SHA512: a0330e54ed504330ca874c42d4e318a909f548b2fb9ac46db8badf5935b9eec47dc4ed503d1b6f98574418e3473420ea45f60498be05545c4325cfa89dcca689
6f113cb txgraph: use fallback order to sort chunks (feature) (Pieter Wuille) 0a33519 txgraph: use fallback order when linearizing (feature) (Pieter Wuille) fba004a txgraph: pass fallback_order to TxGraph (preparation) (Pieter Wuille) 941c432 txgraph test: subclass TxGraph::Ref like mempool does (preparation) (Pieter Wuille) 39d0052 clusterlin: make optimal linearizations deterministic (feature) (Pieter Wuille) 8bfbba3 txgraph: sort distinct-cluster chunks by equal-feerate-prefix size (feature) (Pieter Wuille) e0bc73b clusterlin: sort tx in chunk by feerate and size (feature) (Pieter Wuille) 6c1bcb2 txgraph: clear cluster's chunk index in ~Ref (preparation) (Pieter Wuille) 7427c7d txgraph: update chunk index on Compact (preparation) (Pieter Wuille) 3ddafce txgraph: initialize Ref in AddTransaction (preparation) (Pieter Wuille) Pull request description: Part of #30289. TxGraph's fundamental responsibility is deciding the order of transactions in the mempool. It relies on the `cluster_linearize.h` code to optimize it, but there can and often will be many different orderings that are essentially equivalent from a quality perspective, so we have to pick one. At a high level, the solution will involve one or more of: * Deciding based on **internal identifiers** (`Cluster::m_sequence`, `DepGraphIndex`). This is very simple, but risks leaking information about transaction receive order. * Deciding **randomly**, which is private, but may interfere with relay expectations, block propagation, and ability to monitor network behavior. * Deciding **based on txid**, which is private and deterministic, but risks incentivizing grinding to get an edge (though we haven't really seen such behavior). * Deciding **based on size** (e.g. prefer smaller transactions), which is somewhat related to quality, but not unconditionally (depending on mempool layout, the ideal ordering might call for smaller transactions first, last, or anywhere in between). It's also not a strong ordering as there can be many identically-sized transactions. However, if it were to encourage grinding behavior, incentivizing smaller transactions is probably not a bad thing. As of #32545, the current behavior is primarily picking randomly, though inconsistently, as some code paths also use internal identifiers and size. #33335 sought to change it to use random (preferring size in a few places), with the downsides listed above. This PR is an alternative to that, which changes the order to tie-break based on size everywhere possible, and use lowest-txid-first as final fallback. This is fully deterministic: for any given set of mempool transactions, if all linearized optimally, the transaction order exposed by TxGraph is deterministic. The transactions within a chunk are sorted according to: 1. `PostLinearize` (which improves sub-chunk order), using an initial linearization created using the rules 2-5 below. 2. Topology (parents before children). 3. Individual transaction feerate (high to low) 4. Individual transaction weight (small to large) 5. Txid (low to high txid) The chunks within a cluster are sorted according to: 1. Topology (chunks after their dependencies) 2. Chunk feerate (high to low) 3. Chunk weight (small to large) 4. Max-txid (chunk with lowest maximum-txid first) The chunks across clusters are sorted according to: 1. Feerate (high to low) 2. Equal-feerate-chunk-prefix weight (small to large) 3. Max-txid (chunk with lowest maximum-txid first) The equal-feerate-chunk-prefix weight of a chunk C is defined as the sum of the weights of all chunks in the same cluster as C, with the same feerate as C, up to and including C itself, in linearization order (but excluding such chunks that appear after C). This is a well-defined approximation of sorting chunks from small to large across clusters, while remaining consistent with intra-cluster linearization order. ACKs for top commit: ajtowns: reACK 6f113cb it was good before and now it's better instagibbs: ACK 6f113cb marcofleon: light crACK 6f113cb Tree-SHA512: 16dc43c62b7e83c81db1ee14c01e068ae2f06c1ffaa0898837d87271fa7179dd98baeb74abc9fe79220e01fdba6876defe60022c2b72badc21d770644a0fe0ac
…pollution 65134c7 depends: Prefix include path for headers-only `systemtap` package (Hennadii Stepanov) 94a692b cmake: Add missed `USDT::headers` (Hennadii Stepanov) b5375c4 depends: Prefix include path for headers-only `boost` package (Hennadii Stepanov) d73378f cmake: Add missed `Boost::headers` (Hennadii Stepanov) Pull request description: Currently, header-only dependencies in the depends subsystem are installed into the standard `include/` subdirectory. This inadvertently exposes their headers to the compiler via `-I` flags brought in by other dependencies (e.g., `libevent` or `sqlite`). This "include path pollution" masks missing dependencies in the build configuration. While the build might succeed by accident due to this overlap, it creates a fragile state. If the overlapping library is removed, the build will break, or, worse, the compiler may silently fall back to the host system's default paths (e.g., `/usr/include`). This PR improves build system security and hygiene by enforcing strict, distinguished include paths for header-only dependencies. The missing dependencies revealed by this change (`Boost::headers`, `USDT::headers`) have been fixed in separate commits. ACKs for top commit: theuni: re-ACK 65134c7 fanquake: ACK 65134c7 Tree-SHA512: 41667b46c3bd2f872951a5651b30f7d1468f49f1265196b7868233ed44b2eb0e33f1f69a1af348b55f07a8d1f594e276eb49b724e80b8eae85aed1c9bacae197
b623fab mining: enforce minimum reserved weight for IPC (Sjors Provoost) d3e4952 mining: fix -blockreservedweight shadows IPC option (Sjors Provoost) 418b799 test: have mining template helpers return None (Sjors Provoost) Pull request description: Also enforce `MINIMUM_BLOCK_RESERVED_WEIGHT` for IPC clients. The `-blockreservedweight` startup option should only affect RPC code, because IPC clients (currently) do not have a way to signal their intent to use the node default (the `BlockCreateOptions` struct defaults merely document a recommendation for client software). Before this PR however, if the user set `-blockreservedweight` then `ApplyArgsManOptions` would cause the `block_reserved_weight` option passed by IPC clients to be ignored. _Users who don't set this value were not affected._ Fix this by making BlockCreateOptions::block_reserved_weight an std::optional. Internal interface users, such as the RPC call sites, don't set a value so -blockreservedweight is used. Whereas IPC clients do set a value which is no longer ignored. Test coverage is added, with a preliminary commit that refactors the `create_block_template` and `wait_next_template` helpers. `mining_basic.py` already ensured `-blockreservedweight` is enforced by mining RPC methods. The second commit adds coverage for Mining interface IPC clients. It also verifies that `-blockreservedweight` has no effect on them. The third commit enforces `MINIMUM_BLOCK_RESERVED_WEIGHT` for IPC clients. Previously lower values were quietly clamped. --- Merge order preference: #34452 should ideally go first. ACKs for top commit: sedited: Re-ACK b623fab ryanofsky: Code review ACK b623fab. Was rebased and test split up and comment updated since last review. ismaelsadeeq: ACK b623fab Tree-SHA512: 9e651a520d8e4aeadb330da86788744b6ecad8060fa21d50dc8e6012a60083e7b262aaa08a64676b9ef18ba65b651bc1272d8383d184030342e4c0f2c6a9866d
faba426 lint: Flatten lint image entry points (MarcoFalke) 1111fff lint: Add missing --platform=linux to docker build command (MarcoFalke) Pull request description: Two fixups to the lint container: * Add a missing `--platform=linux` to avoid running a non-native arch, like s390x, which happens with podman if such a container was most recently used. * Flatten the entry points to remove the bash-based one: Previously, an additional entry point into the container that spawned a bash was supported. The bash had an alias `lint` to run all lint scripts. However, such a use-case seems limited (because it only runs inside the container), inflexible (because it only allows running all lint scripts), and possibly brittle (because it can miss re-building the image when the cache is stale). So remove it and just offer the single entry point via the `./ci/lint.py` script. If there is a use-case to skip the image building, it should be trivial to add an env var setting the the lint Python script like `DANGER_SKIP_IMAGE_RE_BUILD=1` (or so) in the future. ACKs for top commit: willcl-ark: ACK faba426 Tree-SHA512: 9afda16723c215602c6c42fa3a286d1828c887c8f6ff9512c8ec162ec8997789695f0c464d389cae94e67acf8b5e0f1a55e2ee0d60131a2eee091cf281f91514
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.
Avance