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 6354b4fd7f,
because it is no longer needed.
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
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
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.
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
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
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
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 commit 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.
mining_basic.py already ensured -blockreservedweight is enforced by
mining RPC methods. This commit adds coverage for Mining interface IPC
clients. It also verifies that -blockreservedweight has no effect on
them.
Co-Authored-By: Russell Yanofsky <russ@yanofsky.org>
Refactor the mining_create_block_template and mining_wait_next_template
helpers in ipc_util.py to return None if they time out or fail. It makes
the test easier to read and provides a more clear error message in case
of a regression.
There were a few spots that didn't use mining_wait_next_template yet,
which now do.
633d183119 test: misc interface_ipc_mining.py improvements (Sjors Provoost)
52ccd9215e test: split interface_ipc_mining.py into subtests (Sjors Provoost)
4e49fa2a68 test: add interface_ipc_mining.py (Sjors Provoost)
01a1ae889e test: move IPC helpers to ipc_util.py (Sjors Provoost)
Pull request description:
This test has been growing too large, making it difficult to maintain. Especially when multiple pull requests change it.
- move helper functions to `ipc_util.py`
- move mining test to `interface_ipc_mining.py`, keeping only an interface sanity check in `interface_ipc.py`
- split the tests in `interface_ipc_mining.py`
- misc tweaks (to reduce churn in the above commits)
Review hint:
```sh
git show --color-moved=dimmed-zebra --color-moved-ws=ignore-space-change
```
ACKs for top commit:
janb84:
re ACK 633d183119
fjahr:
reACK 633d183119
l0rinc:
code review ACK 633d183119
sedited:
Re-ACK 633d183119
Tree-SHA512: 81bbbec1f03a6ff63068dbefc1bc4768fcd24313d84ba454bb2494fe4a65838dbaeb93ad7f02ed4c9932394f3d24638453bb51ce0e05f561dc613414beda37e4
fa0677d131 refactor: Use SpanReader over DataStream (MarcoFalke)
fad3eb3956 refactor: Use SpanReader over DataStream (MarcoFalke)
fa06e26764 refactor: [qt] Use SpanReader to avoid two vector copies (MarcoFalke)
fabd4d2e2e refactor: Avoid UB in SpanReader::ignore (MarcoFalke)
fa20bc2ec2 refactor: Use empty() over eof() in the streams interface (MarcoFalke)
fa879db735 test: Read debug log for self-checking comment (MarcoFalke)
Pull request description:
This changes all places, where possible, to use SpanReader over DataStream. This makes the code easier to read and reason about, because `SpanReader` can never write data. Also, the code should be minimally faster, because it avoids a full redundant copy of the whole vector of bytes.
ACKs for top commit:
stickies-v:
re-ACK fa0677d131
achow101:
ACK fa0677d131
janb84:
re ACK fa0677d131
sipa:
crACK fa0677d131
Tree-SHA512: 1d9f43fc6e71d481cf7b8f8457f479745ee331734649e9e2c2ab00ce5d317112796c77afc328612ed004e65ac5c16fc92279d760cfb012cfddce9098c4af810f
48161f6a05 wallet: introduce "tx amount exceeds balance when fees are included" error (stratospher)
b7fa609ed1 wallet: remove PreSelectedInputs (stratospher)
7819da2c16 walllet: use CoinsResult instead of PreSelectedInputs (stratospher)
e5474079f1 wallet: introduce GetAppropriateTotal() in CoinsResult (stratospher)
d8ea921d01 wallet: correctly reserve in CoinsResult::All() (stratospher)
7072d825e3 wallet: ensure COutput added in set are unique (stratospher)
fefa3be782 wallet: fix, make 'total_effective_amount' optional actually optional (stratospher)
Pull request description:
picks up https://github.com/bitcoin/bitcoin/pull/25269.
This PR re-implements the code path so that an error message is thrown when a transaction's total amount (including fees) exceeds the available balance. It also refactors the wallet's coin selection code.
1. the first 3 commits are unrelated to the code but few small bug fixes which are nice to fix. but also kind of impacts the remaining logic. (could PR separately if reviewers wish)
1. c467325aaf: make `total_effective_amount` optional actually optional
2. 2202ab5975: ensure `set<shared_ptr<COutput>>` has unique COutput
3. a5ffbbf122: Correctly reserve size when flattening `CoinsResult.coins` map to vector
3. the next 3 commits from 4745d5480c replace the `PreSelectedInputs` struct with `CoinsResult` and removes `PreSelectedInputs`.
4. the last commit (e664484a6d) deals with the error message - `AmountWithFeeExceedsBalance` error inside `WalletModel::prepareTransaction` is never thrown and remains an unused code path. This is because `createTransaction` does not retrieve the fee when the process fails. The fee return arg is set only at the end of the process, when the transaction is successfully created. Therefore, if the transaction creation fails, the fee is not available inside `WalletModel::prepareTransaction` to trigger the `AmountWithFeeExceedsBalance` error.
This PR re-implements the feature inside `CreateTransactionInternal` and adds test coverage for it.
| on master | on PR |
|-----------|-------|
| <img src="https://github.com/user-attachments/assets/a903e687-2466-42c7-b898-5dec24bfe515" width="750" alt="Insufficient funds" /> | <img src="https://github.com/user-attachments/assets/74bb3c83-6132-4c09-91f0-0a446618b3c8" width="750" alt="AmountWithFeeExceedsBalance" /> |
the unreachable code path is removed in https://github.com/bitcoin-core/gui/pull/807 which requires this PR.
ACKs for top commit:
achow101:
ACK 48161f6a05
furszy:
utACK 48161f6
Tree-SHA512: a963fac8d6714f76571df8cf9aff70601536dc6faa4326fbb5892c3f080dc393f0d7c6e2d21879c7a2c898bf0092adb154376d9b0a8929b31575ce9d1d47dec2
In feature_assumevalid.py, we check that a modified block 102 is invalid
by asserting a message in the debug.log. However, this can
intermittently fail as exiting the assert_debug_log can occur before the
block has actually been validated, thus causing the test to fail as the
validation error message is not present in the chunk of the debug.log
being examined.
We can wait for the block to make an invalid chain tip to ensure that the log
line will be present.
Split the Mining interface test into focused subtests.
Keep the initial tip-change pre-mine check in run_mining_interface_test.
As a result run_block_template_test no longer has newblockref.
Split Mining interface tests into interface_ipc_mining.py and keep
interface_ipc.py for echo + simple inspectors.
Register the new test in test_runner.py.
The setup code around "Create Mining proxy object" is duplicated
in the new test file, but the simple insector checks below it
are not moved.
e67a676df9 fix: uptime RPC returns 0 on first call (Lőrinc)
Pull request description:
### Problem
#34328 switched uptime to use monotonic time, but `g_startup_time` was a function-local static in `GetUptime()`, meaning it was initialized on first call rather than at program start.
This caused the first uptime RPC to always return 0.
### Fix
Move `g_startup_time` to namespace scope so it initializes at program start, ensuring the first `uptime()` call returns actual elapsed time.
### Reproducer
Revert the fix and run the test or alternatively:
```bash
cmake -B build && cmake --build build --target bitcoind bitcoin-cli -j$(nproc)
./build/bin/bitcoind -regtest -daemon
sleep 10
./build/bin/bitcoin-cli -regtest uptime
./build/bin/bitcoin-cli -regtest stop
```
<details>
<summary>Before (uptime is initialized on first call)</summary>
```bash
Bitcoin Core starting
0
Bitcoin Core stopping
```
</details>
<details>
<summary>After (first uptime call is in-line with sleep)</summary>
```bash
Bitcoin Core starting
10
Bitcoin Core stopping
```
</details>
----
Fixes#34423, added reporter as coauthor.
ACKs for top commit:
maflcko:
lgtm ACK e67a676df9
optout21:
crACK e67a676df9
carloantinarella:
Tested ACK e67a676df9
achow101:
ACK e67a676df9
musaHaruna:
Tested ACK [e67a676](e67a676df9)
Tree-SHA512: b156f7ed15c3fbb50e8a15f6c9a0d4e2ffb956d0c6ef949e0f8a661a564a20c0d3ed2149fae75ce7e2baa9326788d5037e726e7a7ac2c6ef4e70e4862cd5763f
db2effaca4 scripted-diff: refactor: CWallet::Create() -> CreateNew() (David Gumberg)
27e021ebc0 wallet: Correctly log stats for encrypted messages. (David Gumberg)
d8bec61be2 wallet: remove loading logic from CWallet::Create (David Gumberg)
f35acc893f refactor: wallet: Factor out `WriteVersion()` from `PopulateWalletFromDB()` (David Gumberg)
e12ff8aca0 test: wallet: Split create and load (David Gumberg)
70dbc79b09 wallet: Use CWallet::LoadExisting() for loading existing wallets. (David Gumberg)
ae66e01164 wallet: Create separate function for wallet load (David Gumberg)
bc69070416 refactor: Wallet stats logging in its own function (David Gumberg)
a9d64cd49c wallet: Remove redundant birth time update (David Gumberg)
b4a49cc727 wallet: Move argument parsing to before DB load (David Gumberg)
b15a94a618 refactor: Split out wallet argument loading (David Gumberg)
a02c4a82d8 refactor: Move -walletbroadcast setting init (David Gumberg)
411caf7281 wallet: refactor: PopulateWalletFromDB use switch statement. (David Gumberg)
a48e23f566 refactor: wallet: move error handling to PopulateWalletFromDB() (David Gumberg)
0972785fd7 wallet: Delete unnecessary PopulateWalletFromDB() calls (David Gumberg)
f0a046094e scripted-diff: refactor: CWallet::LoadWallet->PopulateWalletFromDB (David Gumberg)
Pull request description:
This PR is mostly a refactor which splits out logic used for creating wallets and for loading wallets, both of which are presently contained in `CWallet::Create()` into `CWallet::CreateNew()` and `CWallet::LoadExisting()`
The real win of this PR is that `CWallet::Create()` uses a very bad heuristic for trying to guess whether or not it is supposed to be creating a new wallet or loading an existing wallet:
370c592612/src/wallet/wallet.cpp (L2882-L2885)
This heuristic assumes that wallets with no `ScriptPubKeyMans` are being created, which sounds reasonable, but as demonstrated in #32112 and #32111, this can happen when the user tries to load a wallet file that is corrupted, both issues are fixed by this PR and any other misbehavior for wallet files which succeeded the broken heuristic's sniff test for new wallets.
It was already the case that every caller of `CWallet::Create()` knows whether it is creating a wallet or loading one, so we can avoid replacing this bad heuristic with another one, and just shift the burden to the caller.
ACKs for top commit:
achow101:
ACK db2effaca4
polespinasa:
approach ACK db2effaca4
w0xlt:
reACK db2effaca4
murchandamus:
ACK db2effaca4
rkrux:
ACK db2effaca4
Tree-SHA512: c28d60e0a3001058da3fd2bdbe0726c7ebe742a4b900a1dee2e5132eccc22e49619cb747a99b4032b000eafd4aa2fdd4ec244c32be2012aba809fdc94b5f6ecd
02b5f6078d fees: make flushes log debug only (ismaelsadeeq)
Pull request description:
This is a simple PR that updates the flushing log to use debug-level logging under the estimatefee category. It also ensures the log consistently includes only the full file path.
The motivation behind this is that the "Flushed fee estimates to fee_estimates.dat." logs can become noisy; it's done after one hour, so hiding it in the debug estimatefee category seems reasonable.
---
I left the logs when the file is not found as info because that should only occur when you start a fresh node, change datadir, or explicitly delete the file
ACKs for top commit:
achow101:
ACK 02b5f6078d
furszy:
utACK 02b5f6078d
l0rinc:
Lightly tested ACK 02b5f6078d
sipa:
utACK 02b5f6078d
Tree-SHA512: 3e4b822caa23db9b30f1bb8990b36f35dcfcd82dbfb27c319463447da1df988eded84c47d5319ad637c822bdd04f0a727a176da3632c40d786332b6a5aaa6d89
d511adb664 [miner] omit dummy extraNonce via IPC (Sjors Provoost)
bf3b5d6d06 test: clarify getCoinbaseRawTx() comparison (Sjors Provoost)
78df9003d6 [doc] Update comments on dummy extraNonces in tests (Anthony Towns)
Pull request description:
This PR changes the Mining IPC interface to stop including a dummy `extraNonce` in the coinbase `scriptSig` by default, exposing only the consensus-required BIP34 height. This simplifies downstream mining software (including Stratum v2), avoids forcing clients to strip or ignore data we generate, and reduces the risk of incompatibilities if future soft forks add required commitments to the `scriptSig`.
Existing behavior is preserved for RPCs, tests, regtest, and internal mining by explicitly opting in to the dummy `extraNonce` where needed (e.g. to satisfy `bad-cb-length` at low heights), so consensus rules and test coverage are unchanged. The remainder of the PR consists of small comment fixes, naming clarifications, and test cleanups to make the intent and behavior clearer.
ACKs for top commit:
achow101:
ACK d511adb664
ryanofsky:
Code review ACK d511adb664. Just rebased since last review and make suggested tweaks. I'd really like to see this PR merged for the cleanups and sanity it brings to this code. Needs another reviewer though.
sedited:
ACK d511adb664
Tree-SHA512: d41fa813eb6b5626f4f475d8abc506b29090f4a2d218f2d6824db58b5ebe2ed7c584a903b44de18ccec142bb79c257b0aba6d6da073f56175aec88df96aaaaba
4fec726c4d refactor: Simplify Interpret asmap function (Fabian Jahr)
79e97d45c1 doc: Add more extensive docs to asmap implementation (Fabian Jahr)
cf4943fdcd refactor: Use span instead of vector for data in util/asmap (Fabian Jahr)
385c34a052 refactor: Unify asmap version calculation and naming (Fabian Jahr)
fa41fc6a1a refactor: Operate on bytes instead of bits in Asmap code (Fabian Jahr)
Pull request description:
This is a second slice carved out of #28792. It contains the following changes that are crucial for the embedding of asmap data which is added the following PR in the series (probably this will remain in #28792).
The changes are:
- Modernizes and simplifies the asmap code by operating on `std::byte` instead of bits
- Unifies asmap version calculation and naming (previously it was called version and checksum interchangeably)
- Operate on a `span` rather than a vector in the asmap internal to prevent holding the asmap data in memory twice
- Add more extensive documentation to the asmap implementation
- Unify asmap casing in implemetation function names
The first three commits were already part of #28792, the others are new.
The documentation commit came out of feedback gathered at the latest CoreDev. The primary input for the documentation was the documentation that already existed in the Python implementation (`contrib/asmap/asmap.py`) but there are several other comments as well. Please note: I have also asked several LLMs to provide suggestions on how to explain pieces of the implementation and better demonstrate how the parts work together. I have copied bits and pieces that I liked but everything has been edited further by me and obviously all mistakes here are my own.
ACKs for top commit:
hodlinator:
re-ACK 4fec726c4d
sipa:
ACK 4fec726c4d
sedited:
Re-ACK 4fec726c4d
Tree-SHA512: 950a591c3fcc9ddb28fcfdc3164ad3fbd325fa5004533c4a8b670fbf8b956060a0daeedd1fc2fced1f761ac49cd992b79cabe12ef46bc60b2559a7a613d0e166
The DataStream comment was a bit stale, because it was using
CDataStream.
Fix it by using assert_debug_log for a self-documenting and
self-checking test code.
552bc82b17 doc: Use multipath descriptors in descriptors.md and linked test (Anurag chavan)
Pull request description:
Updates documentation and `wallet_miniscript_decaying_multisig_descriptor_psbt.py` to use single multipath descriptors with `<0;1>` syntax instead of separate external/internal descriptors.
## Changes
- **doc/descriptors.md**: Update examples (lines 70-71) to use `/<0;1>/*` multipath syntax
- **doc/descriptors.md**: Update Basic Multisig Example instructions (line 179) to use single multipath descriptor
- **test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py**: Refactor to use single multipath descriptor pattern matching `wallet_multisig_descriptor_psbt.py`
## Implementation
- `_get_xpub()` now extracts external descriptor and converts to multipath format
- `create_multisig()` imports single descriptor that expands to receive and change addresses
- Removed fake checksums from documentation examples
- Added clear comments documenting multipath convention
Fixes#34086
ACKs for top commit:
yashbhutwala:
ACK 552bc82b17
achow101:
ACK 552bc82b17
rkrux:
lgtm tACK 552bc82
Tree-SHA512: cc99271a3955daa475242d9f4ef8f09f4c94c64e48ec4647ecfd95dceb38bb0cdd91b78ec2d5f033b449d175eaecbdda49d6c766c8a1e1a01fed93be4eb0cfc0
6f7b4323cb test: remove UNKNOWN_ERROR from script_tests (Bruno Garcia)
bd31a92d67 script: use SCRIPT_ERR_SCRIPTNUM for CScriptNum errors (Bruno Garcia)
0ca4dcd786 script: add SCRIPT_ERR_SCRIPTNUM error (Bruno Garcia)
Pull request description:
When evaluating a script, the current code is bad for analyzing some errors because it returns `SCRIPT_ERR_UNKNOWN_ERROR` for errors that are clearly known.
`CScriptNum` has two well defined errors: number overflow and non-minimally encoded number. However, for both errors we return as unknown. This PR changes it by adding a new ScriptError that is used for any `CScriptNum` error.
ACKs for top commit:
achow101:
ACK 6f7b4323cb
w0xlt:
ACK 6f7b4323cb
darosior:
ACK 6f7b4323cb
Tree-SHA512: e656d9992251fbc95d33966fa18ce64bf714179d51ba6a7f429e5a55bc58e7fc08827e4ab71ace0dd385dac7e1feaea621b49503387793a30eae7a7e44aa6b0f
fafdae46ff test: Check that redundant verack message is ignored (MarcoFalke)
Pull request description:
The code exists and is uncovered (ref https://maflcko.github.io/b-c-cov/total.coverage/src/net_processing.cpp.gcov.html#L3795), so add a trivial test to cover it.
ACKs for top commit:
brunoerg:
ACK fafdae46ff
sedited:
ACK fafdae46ff
Tree-SHA512: 157f434c2faa16243890b2344c4ee36bc359e56c80ba8a04f0bba71e9760cf9106c38ed755ff57eff8d1957f35516d20b3d010e0ecb8633b845f5314cc0d050a
7d9e1a8102 test: Verify peer usage after assumeutxo validation completes (stringintech)
0067abe153 p2p: Allow block downloads from peers without snapshot block after assumeutxo validation (stringintech)
Pull request description:
Currently, after assumeutxo background validation finishes, the node continues to skip peers that don't have the snapshot block in their best chain until restart. This unnecessarily excludes peers from block downloads even though the background sync has completed and undo data is available.
The restriction persists because `m_chainman.CurrentChainstate().SnapshotBase()` continues to return the snapshot base block until restart, even after validation completes. Added `m_chainman.CurrentChainstate().m_assumeutxo == Assumeutxo::UNVALIDATED` check to only apply the peer restriction while background validation is ongoing.
Also added test coverage in `feature_assumeutxo.py` that verifies peers without the snapshot block can be used for block downloads after background validation completes. The test fails without this fix.
ACKs for top commit:
fjahr:
Re-ACK 7d9e1a8102
achow101:
ACK 7d9e1a8102
sedited:
Re-ACK 7d9e1a8102
Tree-SHA512: 5515971da7bf7efc55eecdf03686f44c20c9e52dd168e7cfa119032d6a8ebccee69df7143075e4e9d0a01426cd9ae7202dce5c00919a82478ebf49a15dc0fe19
c6ca2b85a3 validation: do not wipe utxo cache for stats/scans/snapshots (Pieter Wuille)
7099e93d0a refactor: rename `FlushStateMode::ALWAYS` to `FORCE_FLUSH` (Lőrinc)
Pull request description:
Revival of https://github.com/bitcoin/bitcoin/pull/30610#issuecomment-3432564955 with the remaining comments applied on top
> Since #28280, the cost of a non-wiping sync of the UTXO cache is only proportional to the number of dirty entries, rather than proportional to the size of the entire cache. Because of that, there is no reason to perform a wiping flush in case the contents of the cache is still useful.
>
> Split the `FlushStateMode::ALWAYS` mode into a FORCE_SYNC (non-wiping) and a FORCE_FLUSH (wiping), and then use the former in `scantxoutset`, `gettxoutsetinfo`, snapshot creation.
(slightly updated after #30214)
ACKs for top commit:
optout21:
reACK c6ca2b85a3
cedwies:
reACK c6ca2b8 (trivial)
achow101:
ACK c6ca2b85a3
sedited:
ACK c6ca2b85a3
Tree-SHA512: f3525a85dc512db4a0a9c749ad47c0d3fa44085a121aa54cd77646260a719c71f754ec6570ae77779c0ed68a24799116f79c686e7a17ce57a26f6a598f7bf926
The monotonic uptime fix (#34328) used a function-local static for `g_startup_time`, which was initialized on first `GetUptime()` call instead of app startup time.
This caused the first `uptime()` call to always return 0.
Move `g_startup_time` to namespace scope so it initializes at program start, ensuring the first call returns actual elapsed time. Note that we don't need to make it `static` anymore because it is just used in this single translation unit.
Test was updated to simulate some work before the first call.
Co-authored-by: Carlo Antinarella <carloantinarella@users.noreply.github.com>
2845f10a2b test: extend FreeBSD ephemeral port range fix to P2P listeners (node)
34bed0ed8c test: use IP_PORTRANGE_HIGH on FreeBSD for dynamic port allocation (woltx)
Pull request description:
Reopening #34336. I’ve now tested it on FreeBSD and confirmed it works.
On FreeBSD, the default ephemeral port range (10000-65535) overlaps with the test framework's static port range (11000-26000), possibly causing intermittent "address already in use" failures when tests use dynamic port allocation (`port=0`).
This PR adds a helper that sets `IP_PORTRANGE_HIGH` via `setsockopt()` before binding, requesting ports from 49152-65535 instead, which avoids the overlap, as suggested in https://github.com/bitcoin/bitcoin/issues/34331#issuecomment-3767161843 by @maflcko .
From FreeBSD's [sys/netinet/in.h](https://cgit.freebsd.org/src/tree/sys/netinet/in.h):
```c
#define IP_PORTRANGE 19
#define IP_PORTRANGE_HIGH 1
#define IPPORT_EPHEMERALFIRST 10000 /* default range start */
#define IPPORT_HIFIRSTAUTO 49152 /* high range start */
```
See also: FreeBSD https://man.freebsd.org/cgi/man.cgi?query=ip&sektion=4 man page.
Fixes#34331
ACKs for top commit:
vasild:
ACK 2845f10a2b
hebasto:
ACK 2845f10a2b, I have reviewed the code and it looks OK.
Tree-SHA512: ce501ce3e8a4023e07bad572df2b85d6829becf133813e4529aebba83e4eba59fa8b48e9d2197ebbb226adaf3054fad720775a787244d6b38c0078ee086102f4
Currently, if the node replies to any command with an error during
the test framework's setup(), we log the generic and not really useful
"Unexpected exception" from the BaseException catch, with no further
information.
This isn't helpful for diagnosing the issue. Fix it by explicitly handling
JSONRPCException and logging the response error message and http status
code.
1f60ca360e wallet: fix removeprunedfunds bug with conflicting transactions (Martin Zumsande)
Pull request description:
`removeprunedfunds` removes all entries from `mapTxSpends` for the inputs of the pruned tx. However, this is incorrect, because there could be multiple entries from conflicting transactions (that shouldn't be removed as well). This could lead to the wallet creating invalid transactions, trying to double spend utxos.
The bug persists when the conflicting tx was mined, because the wallet trusts its internal accounting instead of calling `AddToSpends` again.
The added test should fail on master.
ACKs for top commit:
achow101:
ACK 1f60ca360e
fjahr:
tACK 1f60ca360e
furszy:
utACK 1f60ca360e
vasild:
ACK 1f60ca360e
Tree-SHA512: 3cc9ed547530fd53e25721177b76ab2e1eae16ce2c0e63fc01b20fdbf8bd02655dae51167ad56f9dec748d34c61ce65d38f993370820601f8257c73b876a3347
905dfdee86 test: use ModuleNotFoundError in interface_ipc.py (fanquake)
Pull request description:
Change this so we catch the case where the capnp shared libs have been updated, and can no-longer be loaded by the Python module, resulting in a skipped test, even though pycapnp is installed. i.e:
```bash
stderr:
Traceback (most recent call last):
File "/root/ci_scratch/build/test/functional/interface_ipc.py", line 20, in <module>
import capnp # type: ignore[import] # noqa: F401
^^^^^^^^^^^^
File "/usr/local/lib64/python3.14/site-packages/capnp/__init__.py", line 36, in <module>
from .version import version as __version__
File "/usr/local/lib64/python3.14/site-packages/capnp/version.py", line 1, in <module>
from .lib.capnp import _CAPNP_VERSION_MAJOR as LIBCAPNP_VERSION_MAJOR # noqa: F401
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ImportError: libcapnpc.so.1.0.1: cannot open shared object file: No such file or directory
```
Failing in this way should make it clear that `pycapnp` needs to be reinstalled/rebuilt.
If `pycapnp` is not installed, the test still skips as expected:
```bash
Remaining jobs: [interface_ipc.py]
1/1 - interface_ipc.py skipped (capnp module not available.)
TEST | STATUS | DURATION
interface_ipc.py | ○ Skipped | 0 s
```
Fixes: #34016.
ACKs for top commit:
maflcko:
lgtm ACK 905dfdee86
hebasto:
ACK 905dfdee86, I have reviewed the code and it looks OK. However, I'm [not able](https://github.com/bitcoin/bitcoin/issues/34016#issuecomment-3799532047) to reproduce https://github.com/bitcoin/bitcoin/issues/34016.
sedited:
ACK 905dfdee86
Tree-SHA512: 3cedbe8fc51cc18f1c993f7747d20905f3bf94c736db99a9c4090f5823bf8c09dfbc19ef03c573d504dcdfba6ea0f7d088a7f4563b220742c9a441167c04cfd6
fad7bd9ba3 noui: Remove always empty caption while formatting (MarcoFalke)
fa8ebeb332 refactor: [gui] Document that the title is always empty for node message (MarcoFalke)
fafe71b743 refactor: Remove empty caption from ThreadSafeMessageBox (MarcoFalke)
fa8d0088e7 refactor: Remove empty caption from ThreadSafeQuestion (MarcoFalke)
fa0195499c refactor: [gui] Use lambdas over std::bind (MarcoFalke)
eeee1e341f refactor: Remove trailing semicolon after ADD_SIGNALS_DECL_WRAPPER (MarcoFalke)
Pull request description:
Currently, the user interface (noui, gui) has a caption for each message. However, the caption has many issues:
* It is always hard-coded to the empty string.
* This is confusing and tedious when reading or maintaining the code.
* It is redundant, because `noui` will ignore the caption and set the logging prefix (error, warning, info) based on the `style`.
* The gui does prefer to set the title based on the caption, but since it the caption is always empty, the fallback will always be used.
Fix all issues by removing it.
ACKs for top commit:
hebasto:
ACK fad7bd9ba3, I have reviewed the code and it looks OK. Tested on Ubuntu 25.10.
sedited:
ACK fad7bd9ba3
Tree-SHA512: 58ef538b9b3e1cfdcf2955f6de9b8cee335edbf6339723cb693cb4d584817904c962dac5199ee44d7e2860a5332dec1a6abf47e621eb5cf919aa1cdae271b55f
Previously the coinbase transaction generated by our miner code was
not used downstream, because the getblocktemplate RPC excludes it.
Since the Mining IPC interface was introduced in #30200 we do expose
this dummy coinbase transaction. In Stratum v2 several parts of it
are communicated downstream, including the scriptSig.
This commit removes the dummy extraNonce from the coinbase scriptSig
in block templates requested via IPC. This limits the scriptSig
to what is essential for consensus (BIP34) and removes the need for
external mining software to remove the dummy, or even ignore
the scriptSig we provide and generate it some other way. This
could cause problems if a future soft fork requires additional
data to be committed here.
A test is added to verify the new IPC behavior.
It achieves this by introducing an include_dummy_extranonce
option which defaults to false with all test code updated to
set it to true. Because this option is not exposed via IPC,
callers will no longer see it.
The caller needs to ensure that for blocks 1 through 16
they pad the scriptSig in order to avoid bad-cb-length.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
The code comment mistakingly referred to "the deprecated getCoinbaseTx()",
instead of getCoinbaseRawTx. This was missed in d59b4cdb57.
Also rename parse_and_deserialize_coinbase_tx to make it more clear
it refers to the deprecated method.
Finally, this commit drops the getCoinbaseRawTx() call when testing
template inspectors. The coinbase input check here is already covered by
build_coinbase_test.
14f99cfe53 rpc: make `uptime` monotonic across NTP jumps (Lőrinc)
a9440b1595 util: add `TicksSeconds` (Lőrinc)
Pull request description:
### Problem
`bitcoin-cli uptime` was derived from wall-clock time, so it could jump by large amounts when the system clock is corrected after `bitcoind` starts (e.g. on RTC-less systems syncing NTP).
This breaks the expectation that uptime reflects process runtime.
### Fix
Compute uptime from a [monotonic clock](https://en.cppreference.com/w/cpp/chrono/steady_clock.html) so it is immune to wall-clock jumps, and use that monotonic uptime for the RPC.
GUI startup time is derived from wall clock time minus monotonic uptime so it remains sensible after clock corrections.
### Reproducer
Revert the fix commit and run the `rpc_uptime` functional test (it should fail with `AssertionError: uptime should not jump with wall clock`):
Or alternatively:
```bash
cmake -B build && cmake --build build --target bitcoind bitcoin-cli -j$(nproc)
DATA_DIR=$(mktemp -d)
./build/bin/bitcoind -regtest -datadir="$DATA_DIR" -connect=0 -daemon
./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" -rpcwait uptime
sleep 1
./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" setmocktime $(( $(date +%s) + 20000000 ))
./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" uptime
./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" stop
```
<details>
<summary>Before (uptime jumps with wall clock)</summary>
```bash
Bitcoin Core starting
0
20000001
Bitcoin Core stopping
```
</details>
<details>
<summary>After (uptime stays monotonic)</summary>
```bash
Bitcoin Core starting
0
1
Bitcoin Core stopping
```
</details>
----------
Issue: https://github.com/bitcoin/bitcoin/issues/34326
ACKs for top commit:
maflcko:
review ACK 14f99cfe53🎦
willcl-ark:
tACK 14f99cfe53
w0xlt:
ACK 14f99cfe53
sedited:
ACK 14f99cfe53
Tree-SHA512: 3909973f58666ffa0b784a6df087031b9e34d2022d354900a4dbb6cbe1d36285cd92770ee71350ebf64d6e8ab212d8ff0cd851f7dca1ec46ee2f19b417f53984
3f5211cba8 test: remove child_one/child_two (w)txid variables (naiyoma)
7cfe790820 test: replace ValidWitnessMalleatedTx class with function (naiyoma)
81675a781f test: use pre-generated chain (naiyoma)
Pull request description:
This PR refactors ` ValidWitnessMalleatedTx` class into a `build_malleated_tx_package` function. As a result, two tests are updated: `mempool_accept_wtxid` and `p2p_p2p_private_broadcast`. Also included are a few small refactors in mempool_accept_wtxid , (switching to MiniWallet, using a pre-mined chain, using txid directly.)
Together, these changes reduce complexity and improve test runtime.
ACKs for top commit:
stratospher:
reACK 3f5211c.
cedwies:
reACK 3f5211c
maflcko:
review ACK 3f5211cba8👥
rkrux:
ACK 3f5211cba8
Tree-SHA512: 1fd02be3432fef6b68e54fbe8b15ed56d2699580bb13d0777b21f9cbe4c6d33bbb710541e3ca2fc93eab771d17bf1c427e4b08fa216d561bdb320cc6b36ac8fc
Change this so we catch the case where the capnp shared libs have been
updated, and can no-longer be loaded by the Python module, resulting in
a skipped test, even though pycapnp is installed. i.e:
```bash
stderr:
Traceback (most recent call last):
File "/root/ci_scratch/build/test/functional/interface_ipc.py", line 20, in <module>
import capnp # type: ignore[import] # noqa: F401
^^^^^^^^^^^^
File "/usr/local/lib64/python3.14/site-packages/capnp/__init__.py", line 36, in <module>
from .version import version as __version__
File "/usr/local/lib64/python3.14/site-packages/capnp/version.py", line 1, in <module>
from .lib.capnp import _CAPNP_VERSION_MAJOR as LIBCAPNP_VERSION_MAJOR # noqa: F401
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ImportError: libcapnpc.so.1.0.1: cannot open shared object file: No such file or directory
```
Failing in this way should make it clear that `pycapnp` needs to be
reinstalled/rebuilt.
If `pycapnp` is not installed, the test still skips as expected:
```bash
Remaining jobs: [interface_ipc.py]
1/1 - interface_ipc.py skipped (capnp module not available.)
TEST | STATUS | DURATION
interface_ipc.py | ○ Skipped | 0 s
```
Fixes: #34016.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
removeprunedfunds removes all entries from mapTxSpends for the
inputs of the pruned tx. However, this is incorrect, because there could be
multiple entries from conflicting transactions (that shouldn't be
removed as well). This could lead to the wallet creating invalid
transactions, trying to double spend utxos.
The bug persists when the conflicting tx was mined, because
the wallet trusts its internal accounting instead of calling
AddToSpends again.
Add test coverage to ensure peers without the snapshot block in their chain can be used for block downloads after background validation completes. The test fails without the fix in the previous commit.
