This increments the field number of the `Init.makeMining` method and makes the
old `makeMining` method return an error, so existing IPC mining clients not
using the latest schema file will get an error and not be able to access the
Mining interface.
Normally, there shouldn't be a need to break compatibility this way, but the
mining interface has evolved a lot since it was first introduced, with old
clients using the original methods less stable and performant than newer
clients. So now is a good time to introduce a cutoff, drop deprecated methods,
and stop supporting old clients which can't function as well.
Bumping the field number is also an opportunity to make other improvements that
would be awkward to implement compatibly, so a few of these were implemented in
commits immediately preceding this one.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Adding a context parameter ensures that these methods are run in
their own thread and don't block other calls. They were missing
for:
- createNewBlock()
- checkBlock()
The missing parameters were first pointed out by plebhash in
https://github.com/bitcoin/bitcoin/issues/33575#issuecomment-3383290115 and
adding them should prevent possible performance problems and lockups,
especially with #34184 which can make the createNewBlock method block for a
long time before returning. It would be straightforward to make this change in
a backward compatible way
(https://github.com/bitcoin/bitcoin/pull/34184#discussion_r2770232149) but nice
to not need to go through the trouble.
Warning: This is an intermediate, review-only commit. Binaries built from it
should not be distributed or used to connect to other clients or servers. It
makes incompatible changes to the `mining.capnp` schema without updating the
`Init.makeMining` version, causing binaries to advertise support for a schema
they do not actually implement. Mixed versions may therefore exchange garbage
requests/responses instead of producing clear errors. The final commit in this
series bumps the mining interface number to ensure mismatches are detected.
git-bisect-skip: yes
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
This change removes deprecated methods from the ipc mining interface.
Warning: This is an intermediate, review-only commit. Binaries built from it
should not be distributed or used to connect to other clients or servers. It
makes incompatible changes to the `mining.capnp` schema without updating the
`Init.makeMining` version, causing binaries to advertise support for a schema
they do not actually implement. Mixed versions may therefore exchange garbage
requests/responses instead of producing clear errors. The final commit in this
series bumps the mining interface number to ensure mismatches are detected.
git-bisect-skip: yes
This change copies default option values from the C++ mining interface to the
Cap'n Proto interface. Currently, no capnp default values are set, so they are
implicitly all false or 0, which is inconvenient for the rust and python
clients and inconsistent with the C++ client.
Warning: This is an intermediate, review-only commit. Binaries built from it
should not be distributed or used to connect to other clients or servers. It
makes incompatible changes to the `mining.capnp` schema without updating the
`Init.makeMining` version, causing binaries to advertise support for a schema
they do not actually implement. Mixed versions may therefore exchange garbage
requests/responses instead of producing clear errors. The final commit in this
series bumps the mining interface number to ensure mismatches are detected.
git-bisect-skip: yes
This commit only declares constants without using them. They will be applied in
seperate commit since changing struct default field values in cap'n proto is
not backwards compatible.
libmultiprocess currently handles uncaught exceptions from IPC methods badly
when an `mp.Context` parameter is passed and the IPC call executes on an a
worker thread, with the uncaught exception leading to a std::terminate call.
https://github.com/bitcoin-core/libmultiprocess/pull/218 was created to fix
this, but before that change is available, update an IPC test which can trigger
this behavior to handle it and recover when mp.Context parameters are added in
the an upcoming commit.
Having this workaround makes the test a little more complicated and less strict
but reduces dependencies between pending PRs so they don't need to be reviewed
or merged in a particular order.
Allow `expected_stderr` option passed to `wait_until_stopped` and
`is_node_stopped` helper functions to be a regex pattern instead of just a
fixed string.
Allow `expected_ret_code` be list of possible exit codes instead of a single
error code to handle the case where exit codes vary depending on OS and libc.
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
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
452c743951fa69f25f09e42239d1e70a0acf5c2b refactor: Remove workaround for resolved MSVC bug (Hennadii Stepanov)
7164a0cab650bdf01cdcbc3da690f6b674fcc7b3 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](https://github.com/microsoft/vcpkg/issues/22074) bug in the builtin vcpkg is [fixed](https://github.com/microsoft/vcpkg/issues/22074#issuecomment-3880320585).
ACKs for top commit:
maflcko:
review ACK 452c743951fa69f25f09e42239d1e70a0acf5c2b 🍳
hodlinator:
crACK 452c743951fa69f25f09e42239d1e70a0acf5c2b
janb84:
ACK 452c743951fa69f25f09e42239d1e70a0acf5c2b
Tree-SHA512: a8f859d11d4cf0440cf7ff8353fd1babe90818356ef02eae28571a2a4a7960db1f85cdbc4f88b5fb8a1f8bf44bca8c8715cdfb9ea87997c3fcd81866cd0b156d
fa8c89511d838d9adf706ec0aeac725e64427587 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 fa8c89511d838d9adf706ec0aeac725e64427587
Tree-SHA512: 5ee13b48fc4a5802f3fadb125d71118e01d2cb08ede9d310d6ed13acd8fb7b03185cad73c475c617054c4c4423156ea927a32d0e3a670c3cc13339b552dc8a5c
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.
4c0d4f6f93f371a8ad097735945d32510a7e83bb refactor: interfaces, make 'createTransaction' less error-prone (furszy)
e2c3ec9bf4126564070f4f1097bea45753e41ead refactor: move CreatedTransactionResult to types.h (furszy)
45372175c35b73bfd33a1387b2295fc61d9eaaa9 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 [bitcoin#25269](https://github.com/bitcoin/bitcoin/pull/25269) and [bitcoin#34299](https://github.com/bitcoin/bitcoin/pull/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 4c0d4f6f93f371a8ad097735945d32510a7e83bb.
Tree-SHA512: 4fc61f08ca2e66e46001defb3a2e852265713e75006c98f0c465bd48afe42e7b0d626d28d578741906fdd26e907d6919f06dc640c55c44efc3dfa766fdbf38a4
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.
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 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.
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 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 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 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)
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 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.
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.
fa90277d22e1e6662e827e662eb6bda344cdcb20 ci: Use ubuntu-slim for [meta] runners (MarcoFalke)
fa9627af9f89b2e2c04ae564d11d1af148c9682b ci: Rely on cmake --preset toolchain file (MarcoFalke)
fa3f89acaa7ae3332f4bf13aa91fcff44902990c ci: Add check_manifests to ci-windows.py (MarcoFalke)
1111079a16b92bb34ee2f8d4ae3529fbcb3180b2 ci: Add run_tests step to ci-windows.py (MarcoFalke)
fa561682ce4052bed91fac11f8fc872af5132437 ci: [refactor] Add .github/ci-windows.py prepare_tests step (MarcoFalke)
fa3e607c6dfb220684a65638b35ac5a4d3b1fcb7 ci: Print verbose Windows CI build failure (MarcoFalke)
4444808dd3a704ec0f5d97e2c736cc917656c491 ci: [refactor] Add .github/ci-windows.py build step (MarcoFalke)
fabdd4e82342b57e8e7750c5292e9fe0d598105b 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 fa90277d22e1e6662e827e662eb6bda344cdcb20
hebasto:
ACK fa90277d22e1e6662e827e662eb6bda344cdcb20.
willcl-ark:
utACK fa90277d22e1e6662e827e662eb6bda344cdcb20
Tree-SHA512: 00443289e3d8b6d60d1347934d9d4b16098e8c36b6325467e5804b1869714201c4f7e932da3be44392c73e4713a1f52cd8af481894d36c6a281ba7238d43434e
fe0b1513a7c53b8490b81165acf1c7d42297a2ed test: add a test for txgraph staging (Hao Xu)
ef253a9d3d16f62fe39fbeee336b100554ceaff7 test: add block builder tests for txgraph (Hao Xu)
4a1ac31e97c252e5977ddd85109978307d427807 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 fe0b1513a7c53b8490b81165acf1c7d42297a2ed
sipa:
reACK fe0b1513a7c53b8490b81165acf1c7d42297a2ed
Tree-SHA512: 01010a3b4e4163849df2912d1393be74d26eb199d0544cfbef58a498aca5153463a118f55a2f1cad2995552b74210031e659de8df6b88cbcffdffd2a1b464990
fa88ac3f4f9b7ed99a1b6ee045cef9a019ed314e 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 https://github.com/bitcoin/bitcoin/pull/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 fa88ac3f4f9b7ed99a1b6ee045cef9a019ed314e
hebasto:
ACK fa88ac3f4f9b7ed99a1b6ee045cef9a019ed314e.
sedited:
ACK fa88ac3f4f9b7ed99a1b6ee045cef9a019ed314e
Tree-SHA512: d1bda846a10190a2936084a06bd87418c6a3e4ababc298e4beb9bc9e1190bff430cbe973475d634eda5ef7863571c89bfa4b78ff63fcbd9ac10c42fd9d5fa23a
fa6801366d76a34f51a7d60be7d3710ed8e722db refactor: [rpc] Remove confusing and brittle integral casts (take 2) (MarcoFalke)
Pull request description:
Second take for cases which I seem to have missed in commit 94ddc2dced5736612e358a3b80f2bc718fbd8161.
When constructing an UniValue from integral values, historically (long ago), in some cases casts where needed. With the current UniValue constructor, only very few are actually needed.
Keeping the unused casts around is:
* confusing, because code readers do not understand why they are needed
* brittle, because some may copy them into new places, where they will lead to hard-to-find logic bugs, such as the ones fixed in pull https://github.com/bitcoin/bitcoin/pull/34112
So fix all issues by removing them, except for a few cases, where casting was required:
* `static_cast<bool>(fCoinBase)`, because `bool{fCoinBase}` only works on modern compilers.
This hardening refactor does not fix any bugs and does not change any behavior.
ACKs for top commit:
Sjors:
ACK fa6801366d76a34f51a7d60be7d3710ed8e722db
sedited:
ACK fa6801366d76a34f51a7d60be7d3710ed8e722db
Tree-SHA512: 77f03f496ea2a42987720cb4a36eb4e7a0d5b512ed7f029e41dd54c04fb4c1680f7cc13514acbc9f1bae991be4de3cf31c5a0d27c016a030f5749d132603f71e
fa16b275fa94f66067c94fb8d0854859df6953b8 test: Check that interrupt results in EXIT_SUCCESS (MarcoFalke)
fab7c7f56c1d72178435618c49611a3ecd80b9b8 test: Split large init_stress_test into two smaller functions (MarcoFalke)
Pull request description:
This is a test for https://github.com/bitcoin/bitcoin/pull/34224. The test can be tested by reverting that pull request and observing the test failure.
Also, includes a small test cleanup/refactor.
ACKs for top commit:
janb84:
ACK fa16b275fa94f66067c94fb8d0854859df6953b8
sedited:
ACK fa16b275fa94f66067c94fb8d0854859df6953b8
Tree-SHA512: bb6894316fd4f78b3c0cb299cc93abf7f3f794e0507bf7deda7d86f8f45d60299ec0f027c41a6f909c197a1e5fba44fe269ce4165450b89738b82d8ddf196b80
8c03318387f6b3d1520a539f426b300bae316fc3 consensus/doc: explain `GetValueOut()` precondition (Lőrinc)
82ef92c8d006b3f5c3baaf00e5f8200d289d85d2 consensus/doc: explain unreachable `bad-txns-fee-outofrange` check (Lőrinc)
232a2bce90a96720f5c8d31413f1d14b4c9d90f2 consensus/test: add out-of-range output unit tests for `CTransaction::GetValueOut` (Lőrinc)
aa87aae14f9eee79e3a0fb9c0f5ff3eaa97433e2 consensus/test: add `MoneyRange` unit tests for `CheckTxInputs` (Lőrinc)
Pull request description:
### Problem
Coverage reports indicate that a few consensus related validations aren't exercised in unit-, and some not even in the functional-tests:
Inspired by the coverage reports:
* ["bad-txns-premature-spend-of-coinbase"](https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/consensus/tx_verify.cpp.gcov.html#L180): Only covered in functional tests
* ["bad-txns-inputvalues-outofrange"](https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/consensus/tx_verify.cpp.gcov.html#L187): Unreachable in functional tests [1], uncovered in unit tests
* ["bad-txns-in-belowout"](https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/consensus/tx_verify.cpp.gcov.html#L193): Only covered in functional tests
* ["GetValueOut: value out of range"](https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/primitives/transaction.cpp.gcov.html#L103) and [total coverage report](https://maflcko.github.io/b-c-cov/total.coverage/src/primitives/transaction.cpp.gcov.html#L103)
Replacing them with explicit throws still passes all unit (and sometimes even functional) tests, confirming those branches are not being exercised, see: https://github.com/l0rinc/bitcoin/pull/112
### Fixes
Add minimal unit test coverage for `Consensus::CheckTxInputs` invalid outcomes for `bad-txns-premature-spend-of-coinbase`, `bad-txns-inputvalues-outofrange`, `bad-txns-in-belowout`.
Add a unit test covering `CTransaction::GetValueOut()` throwing for out of range values.
After the prerequisits are tested, document why `bad-txns-fee-outofrange` is unreachable - while keeping the check in place because it is consensus-critical code.
ACKs for top commit:
maflcko:
lgtm ACK 8c03318387f6b3d1520a539f426b300bae316fc3 🍴
darosior:
utACK 8c03318387f6b3d1520a539f426b300bae316fc3
sedited:
ACK 8c03318387f6b3d1520a539f426b300bae316fc3
Tree-SHA512: 91c65dda99b42d12de99c58b02df0f861203f97d268329a3ecce79bd681fcaf847f508c1d9f2256b2b92a953a94d868cbae647f378def92484681d771722ea27
76dae5d6911b600fafa3a417a740f14b299284f3 cmake: Replace recursive globbing with explicit globbing in folders (Hennadii Stepanov)
88d909257104124bee3fc810f7fa32e5802b92fe cmake: Create subdirectories in build tree in advance (Hennadii Stepanov)
Pull request description:
While reviewing https://github.com/bitcoin/bitcoin/pull/32697, I noticed that symlink creation fails when the target subdirectory does not exist. In such cases, `file(CREATE_LINK ... COPY_ON_ERROR SYMBOLIC)` falls back to copying, which implicitly creates the required path. As a result, a single file is copied instead of symlinked for subdirectories in `test/functional`.
This PR ensures that necessary subdirectories are created in advance, so that subsequent symlink creation does not fail due to missing paths.
For example:
- on the master branch:
```
$ cmake -B build
$ ls -l build/test/functional/mocks/
total 8
-rwxrwxr-x 1 hebasto hebasto 2683 Jul 3 14:11 invalid_signer.py
lrwxrwxrwx 1 hebasto hebasto 64 Jul 3 14:11 multi_signers.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/multi_signers.py
lrwxrwxrwx 1 hebasto hebasto 60 Jul 3 14:11 no_signer.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/no_signer.py
lrwxrwxrwx 1 hebasto hebasto 57 Jul 3 14:11 signer.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/signer.py
```
- with this PR:
```
$ cmake -B build
$ ls -l build/test/functional/mocks/
total 4
lrwxrwxrwx 1 hebasto hebasto 65 Jul 3 13:51 invalid_signer.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/invalid_signer.py
lrwxrwxrwx 1 hebasto hebasto 64 Jul 3 13:51 multi_signers.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/multi_signers.py
lrwxrwxrwx 1 hebasto hebasto 60 Jul 3 13:51 no_signer.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/no_signer.py
lrwxrwxrwx 1 hebasto hebasto 57 Jul 3 13:51 signer.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/signer.py
```
ACKs for top commit:
m3dwards:
ACK 76dae5d6911b600fafa3a417a740f14b299284f3
sedited:
ACK 76dae5d6911b600fafa3a417a740f14b299284f3
janb84:
re ACK 76dae5d6911b600fafa3a417a740f14b299284f3
Tree-SHA512: a720a68752c72390b9452b192b06d09e41cac1080d32cfe3c2caabb65949626771e0709e7193f69677bd24a3c747e368c2323d9c857d4aa97e1890cc463850ed
This is the standard approach and avoids relying on
VCPKG_INSTALLATION_ROOT and -DCMAKE_TOOLCHAIN_FILE= in the ci-windows.py
script.
This makes it easier to run locally.
This is mostly a refactor, except for using the runner workspace (cwd) for:
* fuzz qa-assets
* functional temp prefix
This makes it easier to run the ci-windows.py locally.
7378f27b4fb512567b6152f986f67d9263d08d7a test: run utxo-to-sqlite script test with spk/txid format option combinations (Sebastian Falbesoner)
b30fca7498c93356fbdb8c2ce881aa8e548bae17 contrib: utxo_to_sqlite.py: add options to store txid/spk as BLOBs (Sebastian Falbesoner)
Pull request description:
This PR is a late follow-up to https://github.com/bitcoin/bitcoin/pull/27432, introducing an option for the utxo-to-sqlite script to store the txid/scriptPubKey columns as bytes (= `BLOB` storage class in sqlite, see e.g. https://www.sqlite.org/datatype3.html in sqlite) rather than hex strings. This was proposed in earlier reviews (https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-1516857024, https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-1653739351) and has the obvious advantage of a significantly smaller size of the resulting database (and with that, faster conversion) and the avoidance of hex-to-bytes conversion for further processing of the data [1]. The rationale on why hex strings were chosen back then (and still stays the default, if only for compatibility reasons) is laid out in https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-1516922824 [2].
The approach taken is introducing new parameters `--spk` and `--txid` which can either have the values "hex", "raw" (for scriptpubkey) and "hex", "raw", "rawle" (for txid). Thanks to ajtowns for providing this suggestion. Happy to take further inputs on naming and thoughts on future extensibility etc.
[1] For a concrete example, I found that having these columns as bytes would be nice while working on a SwiftSync hints generator tool (https://github.com/theStack/swiftsync-hints-gen), which takes the result of the utxo-to-sqlite tool as input.
[2] note that in contrast what I wrote back then, I think there is no ambiguity on byte-string-serialization of txids; they are ultimately just hash results and hence, they should be stored as such, and adding a big/little endian knob wouldn't make much sense. The drawback of not being able to immediately show txid-strings (as one would need to do the bytes-reversal step first, which is not possible in sqlite, see e.g. https://github.com/bitcoin/bitcoin/pull/24952#issuecomment-1165499803) still remains though.
ACKs for top commit:
ajtowns:
ACK 7378f27b4fb512567b6152f986f67d9263d08d7a
w0xlt:
reACK 7378f27b4f
sedited:
ACK 7378f27b4fb512567b6152f986f67d9263d08d7a
Tree-SHA512: 265991a1f00e3d69e06dd9adc34684720affd416042789db2d76226e4b31cf20adc433a74d14140f17739707dee57e6703f72c20bd0f8dd08b6d383d3f28b450
37cc2a2d953c072a236b657bfd7de5167092a47a logging: use util/log.h where possible (stickies-v)
bb8e9e7c4c8d70914d0878a0d7c6a1371dae23c0 logging: Move message formatting to util/log.h (stickies-v)
001f0a428e3aa3f46aad373684beb3282bffb2c0 move-only: Move logging macros to util/log.h (stickies-v)
94c0adf4e857f3c58a7ab813eb33b83635101d75 move-onlyish: Move logging levels to util/log.h (stickies-v)
56d113cab0347a768daabccdfd76583e6b272dc4 move-only: move logging categories to logging/categories.h (stickies-v)
f5233f7e9827f8dd23414c7fcf49298420f9fc42 move-only: Move SourceLocation to util/log.h (stickies-v)
Pull request description:
This is a mostly move-only change. It's a small refactoring that allows logging macros to be used by including a simple `util/log.h` header instead of the full `logging.h` logging implementation. Most of the changes here were cherry-picked from #34374.
Original motivation for this change was to reduce the size and complexity of #34374 (kernel structured logging PR) and reduce the number of conflicts it causes with other PRs. But this should also make sense as a standalone change to have a clearer separation of concerns between log generation and log handling, and avoid needing to depend on the whole logging framework in call sites that only emit log messages.
Recommended to review with `--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`
ACKs for top commit:
l0rinc:
diff ACK 37cc2a2d953c072a236b657bfd7de5167092a47a
stickies-v:
re-ACK 37cc2a2d953c072a236b657bfd7de5167092a47a
optout21:
crACK 37cc2a2d953c072a236b657bfd7de5167092a47a
sedited:
ACK 37cc2a2d953c072a236b657bfd7de5167092a47a
Tree-SHA512: c7a761323ae63f07ad290d4e3766ba1348a132c8cc68a9895fa9ae5c89206599c00646c42ef77223ac757b9d8bfe6c181bead15e7058e4d8966b3bac88a8f950
b73a62f667d0220a5700a02e11736684d2214b04 test: Ensure invalid block was processed before checking debug.log (Ava Chow)
Pull request description:
Prior to merging a PR, I run 4 different build configurations and their tests in parallel, and consistently one of those will have `feature_assumevalid.py` fail. The failure is because the `assert_debug_log` context exits before the invalid block is processed, so the lines it is looking for don't appear in the part of the log that it is examining.
This PR should resolve that issue by waiting for `getchaintips` to report that the invalid chain is invalid before exiting the `assert_debug_log` context.
ACKs for top commit:
l0rinc:
tested ACK b73a62f667d0220a5700a02e11736684d2214b04
sedited:
ACK b73a62f667d0220a5700a02e11736684d2214b04
Tree-SHA512: 96409ed55f686961c47949d31569d6be8e424d952883c92a9135a4e8a795047d4e2a86c9d27ef5653d21780717275434b0bca1586059cfd5088cd2c867c7baea
576f8920279820bca0caf2c32362ab9eb6e4ac1a qt: Update the `src/qt/locale/bitcoin_en.xlf` translation source file (Hennadii Stepanov)
4b9f5beafe9ee77209014093cc5318e037809126 Update Transifex slug for 31.x (Hennadii Stepanov)
Pull request description:
This PR follows our [Release Process](46e1288df2/doc/release-process.md).
It is required to open Transifex translations for v31.0, as scheduled in https://github.com/bitcoin/bitcoin/issues/33607.
For reference, see the previous similar PR: https://github.com/bitcoin/bitcoin/pull/33152.
**Note for reviewers:**
To reproduce the diff in the last commit, run:
```
cmake --preset dev-mode
cmake --build build_dev_mode --target translate
```
ACKs for top commit:
stickies-v:
ACK 576f8920279820bca0caf2c32362ab9eb6e4ac1a
Tree-SHA512: 9875831b8ea6ace5b6e47fe10351bc7cdd26b9c27fe275f678c7f33f6df115ce3e956f9d522905b2fda76d82fc0a4135482cfb665c4428ab2bc1164c49cf82f4
