c413cf12c5 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 c413cf12c5
Tree-SHA512: e28a43b1aa17ce7f0a19d16b98efed0372004d83e4d7e92a126f642599d7e1a94684032a48a3b380b3a7c970c313c92bfe30146b977e33f81b45fe70b49755e3
2cb7e99dee test: also reset CConnman::m_private_broadcast in tests (Vasil Dimov)
91b7c874e2 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 2cb7e99dee🚙
Crypt-iQ:
tACK 2cb7e99dee
frankomosh:
Code Review ACK 2cb7e99dee
brunoerg:
code review ACK 2cb7e99dee
Tree-SHA512: 0f4b114542da8dc611689457ce67034c15cbfe409b006b2db72bc74078ee9513f5ce3d0e6e67d37c127cfa0a5170fe72fe3ea45ce2a61d45a358dd11bd1881f8
eafd530d20 kernel: avoid potential duplicate object in shared library/binary (Cory Fields)
24c3b47010 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 eafd530d20
hebasto:
ACK eafd530d20.
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.
faba426b3b lint: Flatten lint image entry points (MarcoFalke)
1111fff91c 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 faba426b3b
Tree-SHA512: 9afda16723c215602c6c42fa3a286d1828c887c8f6ff9512c8ec162ec8997789695f0c464d389cae94e67acf8b5e0f1a55e2ee0d60131a2eee091cf281f91514
b623fab1ba mining: enforce minimum reserved weight for IPC (Sjors Provoost)
d3e49528d4 mining: fix -blockreservedweight shadows IPC option (Sjors Provoost)
418b7995dd 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 b623fab1ba
ryanofsky:
Code review ACK b623fab1ba. Was rebased and test split up and comment updated since last review.
ismaelsadeeq:
ACK b623fab1ba
Tree-SHA512: 9e651a520d8e4aeadb330da86788744b6ecad8060fa21d50dc8e6012a60083e7b262aaa08a64676b9ef18ba65b651bc1272d8383d184030342e4c0f2c6a9866d
65134c7e5f depends: Prefix include path for headers-only `systemtap` package (Hennadii Stepanov)
94a692b6aa cmake: Add missed `USDT::headers` (Hennadii Stepanov)
b5375c44ed depends: Prefix include path for headers-only `boost` package (Hennadii Stepanov)
d73378ffcc 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 65134c7e5f
fanquake:
ACK 65134c7e5f
Tree-SHA512: 41667b46c3bd2f872951a5651b30f7d1468f49f1265196b7868233ed44b2eb0e33f1f69a1af348b55f07a8d1f594e276eb49b724e80b8eae85aed1c9bacae197
6f113cb184 txgraph: use fallback order to sort chunks (feature) (Pieter Wuille)
0a3351947e txgraph: use fallback order when linearizing (feature) (Pieter Wuille)
fba004a3df txgraph: pass fallback_order to TxGraph (preparation) (Pieter Wuille)
941c432a46 txgraph test: subclass TxGraph::Ref like mempool does (preparation) (Pieter Wuille)
39d0052cbf clusterlin: make optimal linearizations deterministic (feature) (Pieter Wuille)
8bfbba3207 txgraph: sort distinct-cluster chunks by equal-feerate-prefix size (feature) (Pieter Wuille)
e0bc73ba92 clusterlin: sort tx in chunk by feerate and size (feature) (Pieter Wuille)
6c1bcb2c7c txgraph: clear cluster's chunk index in ~Ref (preparation) (Pieter Wuille)
7427c7d098 txgraph: update chunk index on Compact (preparation) (Pieter Wuille)
3ddafceb9a 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 6f113cb184 it was good before and now it's better
instagibbs:
ACK 6f113cb184
marcofleon:
light crACK 6f113cb184
Tree-SHA512: 16dc43c62b7e83c81db1ee14c01e068ae2f06c1ffaa0898837d87271fa7179dd98baeb74abc9fe79220e01fdba6876defe60022c2b72badc21d770644a0fe0ac
38fd85c676 http: replace WorkQueue and threads handling for ThreadPool (furszy)
c323f882ed fuzz: add test case for threadpool (TheCharlatan)
c528dd5f8c util: introduce general purpose thread pool (furszy)
6354b4fd7f tests: log node JSON-RPC errors during test setup (furszy)
45930a7941 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 38fd85c676
pinheadmz:
ACK 38fd85c676
Tree-SHA512: a0330e54ed504330ca874c42d4e318a909f548b2fb9ac46db8badf5935b9eec47dc4ed503d1b6f98574418e3473420ea45f60498be05545c4325cfa89dcca689
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.
2ccfdb582b 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 2ccfdb582b, 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 2ccfdb582b
Tree-SHA512: 664ea7a6f811c2743ad1b4d8913c61aab9b358931ee77895d35cdf8a5607fbb08facda085877c53d731afbf42a7220dcc752fc365a7625ee679c1547e1c674d0
fa8c89511d 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 fa8c89511d
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.
4c0d4f6f93 refactor: interfaces, make 'createTransaction' less error-prone (furszy)
e2c3ec9bf4 refactor: move CreatedTransactionResult to types.h (furszy)
45372175c3 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 4c0d4f6f93.
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.
fa90277d22 ci: Use ubuntu-slim for [meta] runners (MarcoFalke)
fa9627af9f ci: Rely on cmake --preset toolchain file (MarcoFalke)
fa3f89acaa ci: Add check_manifests to ci-windows.py (MarcoFalke)
1111079a16 ci: Add run_tests step to ci-windows.py (MarcoFalke)
fa561682ce ci: [refactor] Add .github/ci-windows.py prepare_tests step (MarcoFalke)
fa3e607c6d ci: Print verbose Windows CI build failure (MarcoFalke)
4444808dd3 ci: [refactor] Add .github/ci-windows.py build step (MarcoFalke)
fabdd4e823 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 fa90277d22
hebasto:
ACK fa90277d22.
willcl-ark:
utACK fa90277d22
Tree-SHA512: 00443289e3d8b6d60d1347934d9d4b16098e8c36b6325467e5804b1869714201c4f7e932da3be44392c73e4713a1f52cd8af481894d36c6a281ba7238d43434e
fe0b1513a7 test: add a test for txgraph staging (Hao Xu)
ef253a9d3d test: add block builder tests for txgraph (Hao Xu)
4a1ac31e97 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 fe0b1513a7
sipa:
reACK fe0b1513a7
Tree-SHA512: 01010a3b4e4163849df2912d1393be74d26eb199d0544cfbef58a498aca5153463a118f55a2f1cad2995552b74210031e659de8df6b88cbcffdffd2a1b464990
fa88ac3f4f 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 fa88ac3f4f
hebasto:
ACK fa88ac3f4f.
sedited:
ACK fa88ac3f4f
Tree-SHA512: d1bda846a10190a2936084a06bd87418c6a3e4ababc298e4beb9bc9e1190bff430cbe973475d634eda5ef7863571c89bfa4b78ff63fcbd9ac10c42fd9d5fa23a
fa6801366d 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 94ddc2dced.
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 fa6801366d
sedited:
ACK fa6801366d
Tree-SHA512: 77f03f496ea2a42987720cb4a36eb4e7a0d5b512ed7f029e41dd54c04fb4c1680f7cc13514acbc9f1bae991be4de3cf31c5a0d27c016a030f5749d132603f71e
fa16b275fa test: Check that interrupt results in EXIT_SUCCESS (MarcoFalke)
fab7c7f56c 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 fa16b275fa
sedited:
ACK fa16b275fa
Tree-SHA512: bb6894316fd4f78b3c0cb299cc93abf7f3f794e0507bf7deda7d86f8f45d60299ec0f027c41a6f909c197a1e5fba44fe269ce4165450b89738b82d8ddf196b80
76dae5d691 cmake: Replace recursive globbing with explicit globbing in folders (Hennadii Stepanov)
88d9092571 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 76dae5d691
sedited:
ACK 76dae5d691
janb84:
re ACK 76dae5d691
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.
7378f27b4f test: run utxo-to-sqlite script test with spk/txid format option combinations (Sebastian Falbesoner)
b30fca7498 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 7378f27b4f
w0xlt:
reACK 7378f27b4f
sedited:
ACK 7378f27b4f
Tree-SHA512: 265991a1f00e3d69e06dd9adc34684720affd416042789db2d76226e4b31cf20adc433a74d14140f17739707dee57e6703f72c20bd0f8dd08b6d383d3f28b450
37cc2a2d95 logging: use util/log.h where possible (stickies-v)
bb8e9e7c4c logging: Move message formatting to util/log.h (stickies-v)
001f0a428e move-only: Move logging macros to util/log.h (stickies-v)
94c0adf4e8 move-onlyish: Move logging levels to util/log.h (stickies-v)
56d113cab0 move-only: move logging categories to logging/categories.h (stickies-v)
f5233f7e98 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 37cc2a2d95
stickies-v:
re-ACK 37cc2a2d95
optout21:
crACK 37cc2a2d95
sedited:
ACK 37cc2a2d95
Tree-SHA512: c7a761323ae63f07ad290d4e3766ba1348a132c8cc68a9895fa9ae5c89206599c00646c42ef77223ac757b9d8bfe6c181bead15e7058e4d8966b3bac88a8f950
b73a62f667 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 b73a62f667
sedited:
ACK b73a62f667
Tree-SHA512: 96409ed55f686961c47949d31569d6be8e424d952883c92a9135a4e8a795047d4e2a86c9d27ef5653d21780717275434b0bca1586059cfd5088cd2c867c7baea
576f892027 qt: Update the `src/qt/locale/bitcoin_en.xlf` translation source file (Hennadii Stepanov)
4b9f5beafe 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 576f892027
Tree-SHA512: 9875831b8ea6ace5b6e47fe10351bc7cdd26b9c27fe275f678c7f33f6df115ce3e956f9d522905b2fda76d82fc0a4135482cfb665c4428ab2bc1164c49cf82f4
