fb3e1bf9c9772631571ca46d29c50330ebf54dfd test: check LoadBlockIndex correctly recomputes invalidity flags (stratospher)
29740c06ac53f55f71acf2a1b42b193aac39f579 validation: remove BLOCK_FAILED_MASK (stratospher)
b5b2956bda32b7b4ebc25c83b4d792ecd01f02b4 validation: reset BLOCK_FAILED_CHILD to BLOCK_FAILED_VALID when loading from disk (stratospher)
37bc207852788340dc2a1b33a73748f43226978a validation: stop using BLOCK_FAILED_CHILD (stratospher)
120c631e16893821ea4c73ff70ac60e4fec0429f refactor: use clearer variables in InvalidateBlock() (stratospher)
18f11695c755c379ca67ca0bce8d17492ad9af18 validation: don't update BLOCK_FAILED_VALID to BLOCK_FAILED_CHILD in InvalidateBlock (stratospher)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/32173
even though we have a distinction between `BLOCK_FAILED_VALID` and `BLOCK_FAILED_CHILD` in the codebase,
we don't use it for anything. Whenever we check for BlockStatus, we use `BLOCK_FAILED_MASK` which encompasses both of them.
Since there is no functional difference between `BLOCK_FAILED_VALID` and `BLOCK_FAILED_CHILD` and it's added
code complexity to correctly categorise them (ex: https://github.com/bitcoin/bitcoin/pull/31405#discussion_r1914366243, https://github.com/bitcoin/bitcoin/pull/16856#issuecomment-565506585), we could just remove it.
Looking for conceptual feedback on whether it's better to improve handling of `BLOCK_FAILED_CHILD` in the codebase or remove `BLOCK_FAILED_CHILD`.
Of less relevance, but it would also fix a `reconsiderblock` crash that could happen in the situation mentioned in https://github.com/bitcoin/bitcoin/issues/32173#issuecomment-2767030982
Similar attempt in the past in https://github.com/bitcoin/bitcoin/pull/16856#issuecomment-568073859
ACKs for top commit:
stickies-v:
re-ACK fb3e1bf9c9772631571ca46d29c50330ebf54dfd
alexanderwiederin:
ACK fb3e1bf9c9772631571ca46d29c50330ebf54dfd
mzumsande:
re-ACK fb3e1bf9c9772631571ca46d29c50330ebf54dfd
Tree-SHA512: e97b739885c40a8c021966438e9767cc02bc183056236d6a8c64f6819347ae70c0fbcd71cc2528917560d9f4fd56aed45faf1b6c75d98de7b08b621693a97fbc
fa48d421636c256069010bc03c121c36ed9c0a0c test: Stricter unit test (MarcoFalke)
fa626bd143419a7141311e84490aacd8a6691c33 util: Remove brittle and confusing sp::Popen(std::string) (MarcoFalke)
Pull request description:
The subprocess Popen call that accepts a full `std::string` has many issues:
* It promotes brittle and broken code, where spaces are not properly quoted. Example: https://github.com/bitcoin/bitcoin/pull/33929#discussion_r2590523065
* The internally used `util::split` function does incorrectly split on spaces, instead of using `shlex.split`.
* It is redundant and not needed, because a vector interface already exists.
Fix all issues by removing it and just using the vector interface.
This pull request should not change any behavior: Note that the command taken from `gArgs.GetArg("-signer", "")` is still passed through the `sp::util::split` helper, just like before. Fixing that is left for a follow-up, so that this change here is basically just a refactor.
This also fixes a unit test bug as a side-effect: Fixes https://github.com/bitcoin/bitcoin/issues/32574.
ACKs for top commit:
janb84:
cr ACK fa48d421636c256069010bc03c121c36ed9c0a0c
fjahr:
Code review ACK fa48d421636c256069010bc03c121c36ed9c0a0c
hebasto:
re-ACK fa48d421636c256069010bc03c121c36ed9c0a0c.
Tree-SHA512: 3d29226977c9392502f9361e2bd42b471ad03761bbf6a94ef6e545cbe4492ad5858da1ac9cc64b2791aacb9b6e6f3c3f63dbcc3a2bf45f6a13b5bc33eddf8c2b
c2fcf250697325636218225d578c3844ab9ca633 clusterlin: inline GetReachable into Deactivate (optimization) (Pieter Wuille)
d90f98ab4aaaa6d524d8a2ab9fb3a8ba162ebb00 clusterlin: inline UpdateChunk into (De)Activate (optimization) (Pieter Wuille)
b684f954bbfcd9a18c05110b1124276554ba072e clusterlin: unidirectional MakeTopological initially (optimization) (Pieter Wuille)
1daa600c1ca82d16dd1661292949e56b4c4c6d94 clusterlin: track suboptimal chunks (optimization) (Pieter Wuille)
63b06d5523f1da37f76dc1f8b659959a127975f3 clusterlin: keep track of active children (optimization) (Pieter Wuille)
ae16485aa94dc745556615d569914b75ea855f02 clusterlin: special-case self-merges (optimization) (Pieter Wuille)
3221f1a074e7b313e97f969d11ff8a13a1dc5aa6 clusterlin: make MergeSequence take SetIdx (simplification) (Pieter Wuille)
7194de3f7c5324a6e719ed25ef292d162b4f5d88 clusterlin: precompute reachable sets (optimization) (Pieter Wuille)
6f898dbb8bfad68b3c2168471ced211222d6f7f3 clusterlin: simplify PickMergeCandidate (optimization) (Pieter Wuille)
dcf458ffb99c57c31c5c53e001c98fc1a8baadec clusterlin: split up OptimizeStep (refactor) (Pieter Wuille)
cbd684a4713dbe1adc77986e5924867f0e22fb6a clusterlin: abstract out functions from MergeStep (refactor) (Pieter Wuille)
b75574a6531ebe1f4077482705ad9e2db0ed3438 clusterlin: improve TxData::dep_top_idx type (optimization) (Pieter Wuille)
73cbd15d457221e2e896549a32df7f5a59a9df5c clusterlin: get rid of DepData (optimization) (Pieter Wuille)
7c6f63a8a9dc7e3d168dcb023f55936cd47ccf0b clusterlin: pool SetInfos (preparation) (Pieter Wuille)
20e2f3e96df31ffd32f0752e28d90de30942f5ed scripted-diff: rename _rep -> _idx in SFL (Pieter Wuille)
268fcb6a53ec034ec12a9c2cbaceca677664962b clusterlin: add more Assumes and sanity checks (tests) (Pieter Wuille)
d69c9f56ea96e333dbf2f5b4b4e63685147fbc72 clusterlin: count chunk deps without loop (optimization) (Pieter Wuille)
f66fa69ce008e512af8d7cc6b22f0b3a0a080b2c clusterlin: split tx/chunk dep counting (preparation) (Pieter Wuille)
900e45977889f9a189036f7b0e562f8e404c7190 clusterlin: avoid depgraph argument in SanityCheck (cleanup) (Pieter Wuille)
666b37970f1566b7b369c7c5f840099fa6eda800 clusterlin: fix type to count dependencies (Pieter Wuille)
Pull request description:
Follow-up to #32545, part of #30289.
This contains many of the optimizations that were originally part of #32545 itself.
Here is a list of commits and the geometric average of the benchmark timings. Note that these aren't worst cases, but because of the nature of the optimizations here, I do expect them to apply roughly equally to all kinds of clusters. In other words, the relative improvement shown by these numbers should be representative:
| commit title | ns per optimal linearization |
|:--|--:|
| clusterlin: split tx/chunk dep counting (preparation) | 24,760.30 |
| clusterlin: count chunk deps without loop (optimization) | 24,677.64 |
| scripted-diff: rename _rep -> _idx in SFL | 24,640.08 |
| clusterlin: get rid of DepData, reuse sets (optimization) | 24,389.01 |
| clusterlin: improve TxData::dep_top_idx type (optimization) | 22,578.58 |
| clusterlin: abstract out functions from MergeStep (refactor) | 22,577.15 |
| clusterlin: split up OptimizeStep (refactor) | 22,981.11 |
| clusterlin: simplify PickMergeCandidate (optimization) | 22,018.63 |
| clusterlin: precompute reachable sets (optimization) | 21,194.91 |
| clusterlin: make MergeSequence take SetIdx (simplification) | 21,135.60 |
| clusterlin: special-case self-merges (optimization) | 20,588.13 |
| clusterlin: keep track of active children (optimization) | 13,911.22 |
| clusterlin: track suboptimal chunks (optimization) | 13,629.42 |
| clusterlin: unidirectional MakeTopological initially (optimization) | 12,796.57 |
| clusterlin: inline UpdateChunk into (De)Activate (optimization) | 12,706.35 |
| clusterlin: inline GetReachable into Deactivate (optimization) | 12,525.66 |
And to show that they apply to all clusters roughly similarly:
<img width="1239" height="640" alt="output(1)" src="https://github.com/user-attachments/assets/edd73937-3f87-4582-b2b9-eaed7e6ff352" />
ACKs for top commit:
instagibbs:
reACK c2fcf250697325636218225d578c3844ab9ca633
ajtowns:
reACK c2fcf250697325636218225d578c3844ab9ca633
Tree-SHA512: e8920f7952a6681b4c1d70c864bd0e9784127ae4fd7c740be3e24a473f72e83544d2293066ed9b3e685b755febd6bbbc6c7da0c9b6ef3699b05eaa8d5bc073a0
Add a test for block index transitioning from legacy
BLOCK_FAILED_CHILD to BLOCK_FAILED_VALID behavior.
In the scenario where a valid block has a BLOCK_FAILED_CHILD
parent and a BLOCK_FAILED_VALID grandparent, ensure that all
three blocks are correctly marked as BLOCK_FAILED_VALID
after reloading the block index.
- there maybe existing block indexes stored in disk with
BLOCK_FAILED_CHILD
- since they don't exist anymore, clean up block index entries with
BLOCK_FAILED_CHILD and reset it to BLOCK_FAILED_VALID.
even though we have a distinction between BLOCK_FAILED_VALID
and BLOCK_FAILED_CHILD in the codebase, we don't use it for
anything. since there's no functional difference between them
and it's unnecessary code complexity to categorise them correctly,
just mark as BLOCK_FAILED_VALID instead.
Improve upon the variable name for `invalid_walk_tip` to make the
InvalidateBlock logic easier to read. Block tip before disconnection
is now tracked directly via `disconnected_tip`, and `new_tip`
is the tip after the disconnect.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
Avoid two full iterations over all of a chunks' transactions to
recompute the reachable sets, by inlining them into the
dependency-updating loops.
Note that there is no need to do the same for Activate, because the
reachable sets after merging can be computed directly from the input
chunks' reachable sets. Deactivate needs to recompute them, however.
The two calls to UpdateChunk, in Activate and Deactive each, are subtly
different: the top one needs to update the chunk_idx of iterated
transactions, while the bottom one leaves it unchanged. To exploit this
difference, inline the four function calls, getting rid of UpdateChunks.
This is also a preparation for a future improvement that inlines the
recomputation of reachable sets in the same loop in Deactivate.
It suffices to initially only attempt one direction of merges in
MakeTopological(), and only try both directions on chunks that are the
result of other merges.
This means we can iterate over all active dependencies in a
cluster/chunk in O(ntx) time rather than O(ndeps) (*), as the number of
active dependencies in a set of transactions of size is at most ntx-1.
(*) Asymptotically, this is not actually true, as for large transaction
counts, iterating over a BitSet still scales with ntx. In practice
however, where BitSets are represented by a constant number of integers,
it holds.
After a split, if the top part has a dependency on the bottom part, the
first MergeSequence will always perform this merge and then stop. This
is referred to as a self-merge.
We can special case these by detecting self-merges early, and avoiding
the overhead of a full MergeSequence which involves two
PickMergeCandidate calls (a succesful and an unsuccesful one).
Future changes will rely on knowing the chunk indexes of the two created
chunks after a split. It is natural to return this information from
Deactivate, which also simplifies MergeSequence.
Instead of computing the set of reachable transactions inside
PickMergeCandidate, make the information precomputed, and updated in
Activate (by merging the two chunks' reachable sets) and Deactivate (by
recomputing).
This is a small performance gain on itself, but also a preparation for
future optimizations that rely on quickly testing whether dependencies
between chunks exist.
The current process consists of iterating over the transactions of the
chunk one by one, and then for each figuring out which of its
parents/children are in unprocessed chunks.
Simplify this (and speed it up slightly) by splitting this process into
two phases: first determine the union of all parents/children, and then
find which chunks those belong to.
The combined size of TxData::dep_top_idx can be 16 KiB with 64
transactions and SetIdx = uint32_t. Use a smaller type where possible to
reduce memory footprint and improve cache locality of m_tx_data.
Also switch from an std::vector to an std::array, reducing allocation
overhead and indirections.
With the earlier change to pool SetInfo objects, there is little need
for DepData anymore. Use parent/child TxIdxs to refer to dependencies,
and find their top set by having a child TxIdx-indexed vector in each
TxData, rather than a list of dependencies. This makes code for
iterating over dependencies more natural and simpler.
This significantly changes the data structures used in SFL, based on the
observation that the DepData::top_setinfo fields are quite wasteful:
there is one per dependency (up to n^2/4), but we only ever need one per
active dependency (of which there at most n-1). In total, the number of
chunks plus the number of active dependencies is always exactly equal to
the number of transactions, so it makes sense to have a shared pool of
SetInfos, which are used for both chunks and top sets.
To that effect, introduce a separate m_set_info variable, which stores a
SetInfo per transaction. Some of these are used for chunk sets, and some
for active dependencies' top sets. Every activation transforms the
parent's chunk into the top set for the new dependency. Every
deactivation transforms the top set into the new parent chunk.
With indexes into m_set_data (SetIdx) becoming bounded by the number of
transactions, we can use a SetType to represent sets of SetIdxs.
Specifically, an m_chunk_idxs is added which contains all SetIdx
referring to chunks. This leads to a much more natural way of iterating
over chunks.
Also use this opportunity to normalize many variable names.
This is a preparation for the next commit, where chunks will no longer
be identified using a representative transaction, but using a set index.
Reduce the load of line changes by doing this rename ahead of time.
-BEGIN VERIFY SCRIPT-
sed --in-place 's/_rep/_idx/g' src/cluster_linearize.h
-END VERIFY SCRIPT-
This small optimization avoids the need to loop over the parents of each
transaction when initializing the dependency-counting structures inside
GetLinearization().
This splits the chunk_deps variable in LoadLinearization in two, one for
tracking tx dependencies and one for chunk dependencies. This is a
preparation for a later commit, where chunks won't be identified anymore
by a representative transaction in them, but by a separate index. With
that, it seems weird to keep them both in the same structure if they
will be indexed in an unrelated way.
Note that the changes in src/test/util/cluster_linearize.h to the table
of worst observed iteration counts are due to switching to a different
data set, and are unrelated to the changes in this commit.
Since the deterministic ordering change, SpanningForestState holds a
reference to the DepGraph it is linearizing. So this means we do not
need to pass it to SanityCheck() as an argument anymore.
c1355493e2c26b613109bfac3dcd898b3acca75a refactor: fees: split fee rate format from fee estimate mode (ismaelsadeeq)
922ebf96ed6674ae7acc6f0cde4d7b064f759834 refactor: move-only: move `FeeEstimateMode` enum to `util/fees.h` (ismaelsadeeq)
Pull request description:
### Motivation
Part of #34075
- The `FeeEstimateMode` enum was responsible for both selecting the fee estimation algorithm and specifying the fee rate' format.
#### Changes in this PR:
* The `FeeEstimateMode` enum (`UNSET`, `ECONOMICAL`, `CONSERVATIVE`) is moved to a new util/fees.h header.
* A new `FeeRateFormat `enum (`BTC_KVB`, `SAT_VB`) is introduced in `policy/feerate.h` for feerate formatting.
* The `CFeeRate::ToString()` method is updated to use `FeeRateFormat`.
* All relevant function calls have been updated to use the new `FeeRateFormat` enum for formatting and `FeeEstimateMode` for fee estimation mode.
This refactoring separates these unrelated responsibilities to improve code clarity.
ACKs for top commit:
l0rinc:
ACK c1355493e2c26b613109bfac3dcd898b3acca75a
furszy:
utACK c1355493e2c26b613109bfac3dcd898b3acca75a
musaHaruna:
ACK [c135549](c1355493e2) — reviewed in the context of PR [34075](https://github.com/bitcoin/bitcoin/pull/34075)
willcl-ark:
ACK c1355493e2c26b613109bfac3dcd898b3acca75a
Tree-SHA512: 7cbe36350744313d3d688d3fd282a58c441af1818b1e8ad9cddbc911c499a5205f8d4a39c36b21fed60542db1ef763eb69752d141bcef3393bf33c0922018645
eb510f8678ba2e5f2c2fad2a8b086ce93293de1a ci: fail fast in test-each-commit script (Lőrinc)
04c4d710087b6dfdfd8f941fb9c6109238f4216f ci: remove commit count limit from `test-each-commit` (Lőrinc)
Pull request description:
### Problem
`test-each-commit` currently tests only a limited number of ancestor commits in a PR, so failures introduced deeper in the commit stack might be missed.
### Fix
Remove the max-count limit so `test-each-commit` runs the full build + unit + functional test flow on every non-head PR commit, while keeping the PR tip excluded because it is already covered by the normal CI jobs.
It will also stop after the first failure to surface the root cause sooner and keep logs readable when testing ancestor commits.
### Examples
* Example failure 10 commits deep: https://github.com/l0rinc/bitcoin/actions/runs/21390976651/job/61577575033?pr=105 in https://github.com/l0rinc/bitcoin/pull/105
* Example pass with >7 dummy commits: https://github.com/l0rinc/bitcoin/actions/runs/21392557521/job/61595159841?pr=106 in https://github.com/l0rinc/bitcoin/pull/106
---------
Note: this PR has gone through a few iterations, the latest one just extends the existing job.
ACKs for top commit:
maflcko:
lgtm ACK eb510f8678ba2e5f2c2fad2a8b086ce93293de1a 🕓
hebasto:
re-ACK eb510f8678ba2e5f2c2fad2a8b086ce93293de1a.
willcl-ark:
ACK eb510f8678ba2e5f2c2fad2a8b086ce93293de1a
Tree-SHA512: 5aadafd32daad610ce882277802c390642dc34f7d5bfa71d4b503ee007942d1ebafce2a3430ea5fd2af6673c83f9aee42450043be4722d7c02407d90920f8bce
211111b8048dad959a510150d80277bd9b3d0e25 test: Avoid empty errmsg in JSONRPCException (MarcoFalke)
Pull request description:
It is unclear why the fallback should be an empty message, when it is better to include all rpc_error details that are available.
Also, include the http status.
This allows to revert commit 6354b4fd7fe819eb13274b212e426a7d10ca75d3, because it is no longer needed.
Can be tested by running this diff:
```diff
diff --git a/test/functional/wallet_disable.py b/test/functional/wallet_disable.py
index dbcccd4778..9717a2d248 100755
--- a/test/functional/wallet_disable.py
+++ b/test/functional/wallet_disable.py
@@ -18,9 +18,8 @@ class DisableWalletTest (BitcoinTestFramework):
self.extra_args = [["-disablewallet"]]
self.wallet_names = []
- def run_test (self):
- # Make sure wallet is really disabled
- assert_raises_rpc_error(-32601, 'Method not found', self.nodes[0].getwalletinfo)
+ def run_test(self):
+ self.nodes[0].getwalletinfo()
x = self.nodes[0].validateaddress('3J98t1WpEZ73CNmQviecrnyiWrnqRhWNLy')
assert x['isvalid'] == False
x = self.nodes[0].validateaddress('mneYUmWYsuk7kySiURxCi3AGxrAqZxLgPZ')
ACKs for top commit:
ismaelsadeeq:
utACK 211111b8048dad959a510150d80277bd9b3d0e25
furszy:
utACK 211111b8048dad959a510150d80277bd9b3d0e25
hodlinator:
ACK 211111b8048dad959a510150d80277bd9b3d0e25
rkrux:
tACK 211111b8048dad959a510150d80277bd9b3d0e25
Tree-SHA512: 92067aaaa61a887398e38f587004ba31070acc6a9dbd003e9e35393e8c049e786177afa43da05d2d653af340b6c69803c4323061d73d417219bcdf37729e4b66
d159b103987f672ec4fbb6ba79ba9db45c24c977 doc: update Windows MSVC build guide to utilize WinGet to install apps (janb84)
Pull request description:
This PR updates the Windows MSVC build guide to bring it more in line with the macOS and Unix build guides.
Currently the guide listed requirements but left users to manually do the download/installation. The macOS and the Unix guide utilize package managers to provide concrete installation commands that users can follow. By introducing `winget`, the Windows MSVC build guide now also provides concrete installation commands.
The changes/commands can be tested on any windows machine, provided it run a Windows 10 (version 1809 (build 17763)) or later (e.g. Windows 11 / Server 2025).
ACKs for top commit:
hodlinator:
re-ACK d159b103987f672ec4fbb6ba79ba9db45c24c977
hebasto:
re-ACK d159b103987f672ec4fbb6ba79ba9db45c24c977.
Tree-SHA512: 9eeee60ba3e50e42362f1a6d8003120868c1f49cf146cc6fe16a6deb53ce29c67e841b8ec80d516fe2e6d6ea4c48d34fb12691151e0fea5568c14377bd3da6fb
It is unclear why the fallback should be an empty message, when it is
better to include all rpc_error details that are available.
Also, include the http status.
This allows to revert commit 6354b4fd7fe819eb13274b212e426a7d10ca75d3,
because it is no longer needed.
fa13b13239e53b7198eabab2a3771277a2b433e1 ci: [refactor] Use pathlib over os.path (MarcoFalke)
fa2719ab1ba2252b53609e254413a38ac2097dc9 ci: [refactor] Move run_unit_tests to ci-windows-cross.py (MarcoFalke)
fa99ba5f14d4c7fbc48188504a1668d8e5106c77 ci: Set PREVIOUS_RELEASES_DIR env var in ci-windows-cross.py (MarcoFalke)
fa4a1cab6c179de4e48b574e0a325c74ab7a25f7 ci: Move run_functional_tests into ci-windows-cross.py (MarcoFalke)
1111108685ec0fd09b1e288b07c2b7982fc017e0 ci: [refactor] Move pyzmq install and get_previous_releases into ci-windows-cross.py (MarcoFalke)
fac9c7bd6635d59617949564b1c8075b8537a16b ci: [refactor] Move config.ini rewrite to ci-windows-cross.py (MarcoFalke)
faf738946668b6ec16de37298e5a1da23ed77222 ci: Move check_manifests step to ci-windows-cross.py (MarcoFalke)
fa674d55df57ac0b60f3aa9c9dfec0ae53e8af14 ci: [refactor] Move print_version step into ci-windows-cross.py helper (MarcoFalke)
Pull request description:
Currently the ci yaml has a mix of Bash and Pwsh snippets, which is problematic:
* The `shellcheck` tool does not review the Bash
* The ci yaml is not merged with master on re-runs, but the code is, leading to possibly confusing CI errors on re-runs
* The Pwsh isn't reviewed at all by any tool
* It is tedious to run the CI commands locally on Windows
Fix all issues by extracting them into a step-based Python script.
ACKs for top commit:
janb84:
re ACK fa13b13239e53b7198eabab2a3771277a2b433e1
hebasto:
ACK fa13b13239e53b7198eabab2a3771277a2b433e1, I have reviewed the code and it looks OK.
Tree-SHA512: 23d21d3bfb07e102fe1cc15ba5749d553d9766ae6c4a7648bd77df0705469bd138c76a9a2fdeb4d91d3f889a425b7caf25878ecb2e68b604faf9665f8df4eb6d
c413cf12c5c6e688e71be84397a6e4fc75b5eaa4 ci: Split vcpkg tools cache into restore/save (willcl-ark)
Pull request description:
The vcpkg tools cache was using the combined actions/cache action, which by default saves on every run regardless of branch. Split it into the restore/save pattern used by the other caches, so that saves only happen on default branch pushes.
This will have little impact in bitcoin/bitcoin (which uses few branches), but on forks, if you don't update master branch frequently (which saves all caches), then all cache space will eventually be taken up by multiple vckpg tools caches, resulting in bad cache hit rates in all other jobs.
ACKs for top commit:
maflcko:
lgtm ACK c413cf12c5c6e688e71be84397a6e4fc75b5eaa4
Tree-SHA512: e28a43b1aa17ce7f0a19d16b98efed0372004d83e4d7e92a126f642599d7e1a94684032a48a3b380b3a7c970c313c92bfe30146b977e33f81b45fe70b49755e3
2cb7e99deee1017a6edd94d82de556895138361d test: also reset CConnman::m_private_broadcast in tests (Vasil Dimov)
91b7c874e2b1479ed29f067cd1bef7724aabd951 test: add ConnmanTestMsg convenience method Reset() (Vasil Dimov)
Pull request description:
Member variables of `CConnman::m_private_broadcast` (introduced in
https://github.com/bitcoin/bitcoin/pull/29415) could influence the tests
which creates non-determinism if the same instance of `CConnman` is used
for repeated test iterations.
So, reset the state of `CConnman::m_private_broadcast` from
`ConnmanTestMsg::Reset()`. Currently this affects the fuzz tests
`process_message` and `process_messages`.
Reported in https://github.com/bitcoin/bitcoin/issues/34476#issuecomment-3849088794
ACKs for top commit:
maflcko:
review ACK 2cb7e99deee1017a6edd94d82de556895138361d 🚙
Crypt-iQ:
tACK 2cb7e99deee1017a6edd94d82de556895138361d
frankomosh:
Code Review ACK 2cb7e99deee1017a6edd94d82de556895138361d
brunoerg:
code review ACK 2cb7e99deee1017a6edd94d82de556895138361d
Tree-SHA512: 0f4b114542da8dc611689457ce67034c15cbfe409b006b2db72bc74078ee9513f5ce3d0e6e67d37c127cfa0a5170fe72fe3ea45ce2a61d45a358dd11bd1881f8
eafd530d20326a101be672243de68a67161ef83e kernel: avoid potential duplicate object in shared library/binary (Cory Fields)
24c3b47010036d67e6d777d9aa37ae3ef8146254 build: add kernel-specific warnings (Cory Fields)
Pull request description:
This is a revival of https://github.com/bitcoin/bitcoin/pull/31807
Introduces the [-Wunique-object-duplication](https://clang.llvm.org/docs/DiagnosticsReference.html#wunique-object-duplication) warning flag available in clang-21 for usage when building the kernel library. It warns of potential duplicate objects in shared libraries. REDUCE_EXPORTS needs to be ON to trigger it.
Though we have a C API now that manages exporting symbols, I think it is prudent to also avoid any duplicate symbols on the internal c++ side in case we ever to decide to expose some of its headers. It also not clear that all linkers would handle these cases correctly even in the current internal usage.
ACKs for top commit:
fanquake:
ACK eafd530d20326a101be672243de68a67161ef83e
hebasto:
ACK eafd530d20326a101be672243de68a67161ef83e.
Tree-SHA512: 81961b50f0268dbe076497e130857f5b4b9151c748d107ec15158d1511dd25bce745e0beeb127b9cea51cb2edd78032735600606a75f7ff8a3fd572acced42e0
Fixes warning and potential bug whereby init_flag may exist in both
libbitcoinkernel as well as a downstream user, as opposed to being shared as
intended.
src/support/lockedpool.h:224:31:
warning: 'init_flag' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage [-Wunique-object-duplication]
In some cases, we'll want to be more aggressive or care about different things
when building the kernel. In this case, a warning is added for symbols which
may be duplicated between the kernel and downstream users.
This warning was introduced in clang 21, which is not yet the minimum
supported compiler version. REDUCE_EXPORTS needs to be ON to trigger it.
faba426b3b666c0e93e4349ba88deb79517534c6 lint: Flatten lint image entry points (MarcoFalke)
1111fff91c768d6893868032a0dfba02a9709ffc 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 faba426b3b666c0e93e4349ba88deb79517534c6
Tree-SHA512: 9afda16723c215602c6c42fa3a286d1828c887c8f6ff9512c8ec162ec8997789695f0c464d389cae94e67acf8b5e0f1a55e2ee0d60131a2eee091cf281f91514
b623fab1ba87ea93dd7e302b8c927e55f2036173 mining: enforce minimum reserved weight for IPC (Sjors Provoost)
d3e49528d479613ebe50088d530a621861463fa4 mining: fix -blockreservedweight shadows IPC option (Sjors Provoost)
418b7995ddfbc88764f1f0ceabf8993808b08bd8 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 b623fab1ba87ea93dd7e302b8c927e55f2036173
ryanofsky:
Code review ACK b623fab1ba87ea93dd7e302b8c927e55f2036173. Was rebased and test split up and comment updated since last review.
ismaelsadeeq:
ACK b623fab1ba87ea93dd7e302b8c927e55f2036173
Tree-SHA512: 9e651a520d8e4aeadb330da86788744b6ecad8060fa21d50dc8e6012a60083e7b262aaa08a64676b9ef18ba65b651bc1272d8383d184030342e4c0f2c6a9866d
65134c7e5f99500baed18d575b576e33a6294ecf depends: Prefix include path for headers-only `systemtap` package (Hennadii Stepanov)
94a692b6aa09d2e5df97bbc9cc810854818f9333 cmake: Add missed `USDT::headers` (Hennadii Stepanov)
b5375c44ed16ed6aae3d46ac6316b3981330f100 depends: Prefix include path for headers-only `boost` package (Hennadii Stepanov)
d73378ffcca2de43a79c4903221e8164cf256469 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 65134c7e5f99500baed18d575b576e33a6294ecf
fanquake:
ACK 65134c7e5f99500baed18d575b576e33a6294ecf
Tree-SHA512: 41667b46c3bd2f872951a5651b30f7d1468f49f1265196b7868233ed44b2eb0e33f1f69a1af348b55f07a8d1f594e276eb49b724e80b8eae85aed1c9bacae197
6f113cb1847c6890f1fbd052ff7eb8ea41ccafc5 txgraph: use fallback order to sort chunks (feature) (Pieter Wuille)
0a3351947e736c646a6dfffef24b83d003c569e7 txgraph: use fallback order when linearizing (feature) (Pieter Wuille)
fba004a3df02d8d5d47f1ad0bb1ccbfde01bb2af txgraph: pass fallback_order to TxGraph (preparation) (Pieter Wuille)
941c432a4637efd4e5040259f47f2bfed073af7c txgraph test: subclass TxGraph::Ref like mempool does (preparation) (Pieter Wuille)
39d0052cbf478a729ae0288262003bba9c12690b clusterlin: make optimal linearizations deterministic (feature) (Pieter Wuille)
8bfbba32077cb8682208ef31748a10562be027db txgraph: sort distinct-cluster chunks by equal-feerate-prefix size (feature) (Pieter Wuille)
e0bc73ba9270b860d81e479a7bddcff8cfd8bfb6 clusterlin: sort tx in chunk by feerate and size (feature) (Pieter Wuille)
6c1bcb2c7c1a0017562e99195d74c3a05444633b txgraph: clear cluster's chunk index in ~Ref (preparation) (Pieter Wuille)
7427c7d0983050543f1fc7863121d8e2bf4b1511 txgraph: update chunk index on Compact (preparation) (Pieter Wuille)
3ddafceb9afd9d493b927bc91dae324225ed8e32 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 6f113cb1847c6890f1fbd052ff7eb8ea41ccafc5 it was good before and now it's better
instagibbs:
ACK 6f113cb1847c6890f1fbd052ff7eb8ea41ccafc5
marcofleon:
light crACK 6f113cb1847c6890f1fbd052ff7eb8ea41ccafc5
Tree-SHA512: 16dc43c62b7e83c81db1ee14c01e068ae2f06c1ffaa0898837d87271fa7179dd98baeb74abc9fe79220e01fdba6876defe60022c2b72badc21d770644a0fe0ac
38fd85c676a072ebf256e806beda9d7533790baa http: replace WorkQueue and threads handling for ThreadPool (furszy)
c323f882ed3841401edee90ab5261d68215ab316 fuzz: add test case for threadpool (TheCharlatan)
c528dd5f8ccc3955b00bdba869f0a774efa97fe1 util: introduce general purpose thread pool (furszy)
6354b4fd7fe819eb13274b212e426a7d10ca75d3 tests: log node JSON-RPC errors during test setup (furszy)
45930a79412dc45f9d391cd7689d029fa4f0189e 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 (https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2413702370) 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 38fd85c676
sedited:
Re-ACK 38fd85c676a072ebf256e806beda9d7533790baa
pinheadmz:
ACK 38fd85c676a072ebf256e806beda9d7533790baa
Tree-SHA512: a0330e54ed504330ca874c42d4e318a909f548b2fb9ac46db8badf5935b9eec47dc4ed503d1b6f98574418e3473420ea45f60498be05545c4325cfa89dcca689
- Introduce a `FeeRateFormat` enum and change `CFeeRate::ToString()`
to use it for `BTC/kvB` vs `sat/vB` output formatting.
- Handle all enum values, hence remove default case in `CFeeRate::ToString()`
and `assert(False)` when a `FeeRateFormat` value is not handled.
- Keep `FeeEstimateMode` focused on fee estimation behavior by removing fee rate format
values from `FeeEstimateMode`.
- Update all formatting call sites and tests to pass `FeeRateFormat` explicitly, separating fee rate format
from fee-estimation mode selection.
The vcpkg tools cache was using the combined actions/cache action,
which saves on every run regardless of branch. Split it into the
restore/save pattern used by the other caches, so that saves only
happen on default branch pushes.
2ccfdb582b646d9bda07f0f13b97cb8c37a452aa build: avoid exporting secp256k1 symbols (Cory Fields)
Pull request description:
Take advantage of the [new secp256k1 option to avoid visibility attributes](https://github.com/bitcoin-core/secp256k1/pull/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](https://github.com/bitcoin-core/secp256k1/pull/1696#issuecomment-3028838988), 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 2ccfdb582b646d9bda07f0f13b97cb8c37a452aa, this is implemented exactly as I [tested](https://github.com/bitcoin-core/secp256k1/pull/1696#pullrequestreview-3033584362) the upstream PR. Tested on Fedora 43.
stickies-v:
tACK 2ccfdb582b646d9bda07f0f13b97cb8c37a452aa
Tree-SHA512: 664ea7a6f811c2743ad1b4d8913c61aab9b358931ee77895d35cdf8a5607fbb08facda085877c53d731afbf42a7220dcc752fc365a7625ee679c1547e1c674d0
