38fd85c676a072ebf256e806beda9d7533790baa http: replace WorkQueue and threads handling for ThreadPool (furszy)
c323f882ed3841401edee90ab5261d68215ab316 fuzz: add test case for threadpool (TheCharlatan)
c528dd5f8ccc3955b00bdba869f0a774efa97fe1 util: introduce general purpose thread pool (furszy)
6354b4fd7fe819eb13274b212e426a7d10ca75d3 tests: log node JSON-RPC errors during test setup (furszy)
45930a79412dc45f9d391cd7689d029fa4f0189e http-server: guard against crashes from unhandled exceptions (furszy)
Pull request description:
This has been a recent discovery; the general thread pool class created for #26966, cleanly
integrates into the HTTP server. It simplifies init, shutdown and requests execution logic.
Replacing code that was never unit tested for code that is properly unit and fuzz tested.
Although our functional test framework extensively uses this RPC interface (that’s how
we’ve been ensuring its correct behavior so far - which is not the best).
This clearly separates the responsibilities:
The HTTP server now focuses solely on receiving and dispatching requests, while ThreadPool handles
concurrency, queuing, and execution.
This will also allows us to experiment with further performance improvements at the task queuing and
execution level, such as a lock-free structure or task prioritization or any other implementation detail
like coroutines in the future, without having to deal with HTTP code that lives on a different layer.
Note:
The rationale behind introducing the ThreadPool first is to be able to easily cherry-pick it across different
working paths. Some of the ones that are benefited from it are #26966 for the parallelization of the indexes
initial sync, #31132 for the parallelization of the inputs fetching procedure, #32061 for the libevent replacement,
the kernel API #30595 (https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2413702370) to avoid blocking validation among others use cases not publicly available.
Note 2:
I could have created a wrapper around the existing code and replaced the `WorkQueue` in a subsequent
commit, but it didn’t seem worth the extra commits and review effort. The `ThreadPool` implements
essentially the same functionality in a more modern and cleaner way.
ACKs for top commit:
Eunovo:
ReACK 38fd85c676
sedited:
Re-ACK 38fd85c676a072ebf256e806beda9d7533790baa
pinheadmz:
ACK 38fd85c676a072ebf256e806beda9d7533790baa
Tree-SHA512: a0330e54ed504330ca874c42d4e318a909f548b2fb9ac46db8badf5935b9eec47dc4ed503d1b6f98574418e3473420ea45f60498be05545c4325cfa89dcca689
- Introduce a `FeeRateFormat` enum and change `CFeeRate::ToString()`
to use it for `BTC/kvB` vs `sat/vB` output formatting.
- Handle all enum values, hence remove default case in `CFeeRate::ToString()`
and `assert(False)` when a `FeeRateFormat` value is not handled.
- Keep `FeeEstimateMode` focused on fee estimation behavior by removing fee rate format
values from `FeeEstimateMode`.
- Update all formatting call sites and tests to pass `FeeRateFormat` explicitly, separating fee rate format
from fee-estimation mode selection.
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.
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
- there is no functional difference between BLOCK_FAILED_VALID and BLOCK_FAILED_CHILD
and it's unnecessary code complexity to correctly categorise them.
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
Changes flush warnings to use the actual number of dirty entries being written rather than total cache size or memory usage:
* Moves warning from `FlushStateToDisk` to `CCoinsViewDB::BatchWrite` so it applies to both regular flushes and `AssumeUTXO` snapshot writes
* Changes threshold from `WARN_FLUSH_COINS_SIZE` (1 GiB) to `WARN_FLUSH_COINS_COUNT` (10M entries), approximately equivalent - this also helps with the confusion caused by UTXO size difference on-disk vs in-memory
* Moves benchmark logging to `BatchWrite` where the actual disk I/O occurs to make sure AssumeUTXO also warns
* Uses dirty count for disk space check (48 bytes per entry estimate)
* Removes redundant `changed` counter since `dirty_count` is now tracked
This ensures users are warned appropriately even when only a fraction of the cache is dirty, and provides accurate warnings during `AssumeUTXO` loads.
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Adds `m_dirty_count` member to track the running count of dirty cache entries as follows:
* Incremented when entries are marked dirty via `CCoinsCacheEntry::SetDirty`
* Decremented when dirty entries are removed or cleaned
* Passed through `CoinsViewCacheCursor` and updated during iteration
The dirty count is needed because after non-wiping flushes (introduced in #28280 and #28233), the percentage of dirty entries in the cache may be far below 100%. Using total cache size for flush warnings and disk space checks is therefore misleading.
Updates all test code to properly initialize and maintain the dirty count.
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
Co-authored-by: optout <13562139+optout21@users.noreply.github.com>
Adds test coverage by randomly calling `EmplaceCoinInternalDANGER` in `SimulationTest` to verify it remains correct as we modify it in a future commit.
Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
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
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
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>
fa0677d131191d7db9868c4c1b3d780cb6991226 refactor: Use SpanReader over DataStream (MarcoFalke)
fad3eb39564569e7b09982bec68ae41e45a04f87 refactor: Use SpanReader over DataStream (MarcoFalke)
fa06e26764bbd00fc225df5f4601dd4f687273e0 refactor: [qt] Use SpanReader to avoid two vector copies (MarcoFalke)
fabd4d2e2e3ce734730c56660a958f9cf9dc7d38 refactor: Avoid UB in SpanReader::ignore (MarcoFalke)
fa20bc2ec27522959cdf1ad35d54f080aafbfc47 refactor: Use empty() over eof() in the streams interface (MarcoFalke)
fa879db735281d2cce123dbd59d20c7339b2b4ee 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 fa0677d131191d7db9868c4c1b3d780cb6991226
achow101:
ACK fa0677d131191d7db9868c4c1b3d780cb6991226
janb84:
re ACK fa0677d131191d7db9868c4c1b3d780cb6991226
sipa:
crACK fa0677d131191d7db9868c4c1b3d780cb6991226
Tree-SHA512: 1d9f43fc6e71d481cf7b8f8457f479745ee331734649e9e2c2ab00ce5d317112796c77afc328612ed004e65ac5c16fc92279d760cfb012cfddce9098c4af810f
fa43897c1d14549e7af0d9f912e765875b634c39 doc: Fix LLM nits in net_processing.cpp (MarcoFalke)
bbbba0fd4b87a5441c90d513d2022f4c4d9678cb scripted-diff: Use references when nullptr is not possible (MarcoFalke)
fac54154660438db6a601584fa91f87bc09395b2 refactor: Separate peer/maybe_peer in ProcessMessages and SendMessages (MarcoFalke)
fac529188e0db44875f8728c9e0b6a05d2145e75 refactor: Pass Peer& to ProcessMessage (MarcoFalke)
fa376095a01c421523ec5d012c6aafb006011788 refactor: Pass CNode& to ProcessMessages and SendMessages (MarcoFalke)
fada8380148c1266f2cc1ddb0f65f42651c82a62 refactor: Make ProcessMessage private again (MarcoFalke)
fa80cd3ceed4eb58732c2f6f748277772a8a1c36 test: [refactor] Avoid calling private ProcessMessage() function (MarcoFalke)
Pull request description:
There is a single unit test, which calls the internal `ProcessMessage` function. This is problematic, because it makes future changes harder, since they will need to carry over this public internal interface each time.
Also, there is a mixed use of pointers and references in p2p code, where just based on context, a pointer may sometimes assumed to be null, or non-null. This is confusing when reading the code, or making or reading future changes.
Fix both issues in a series of commits, to:
* refactor the single unit test to call higher-level functions
* Make `ProcessMessage` private again
* Use references instead of implicit non-null pointers, mostly in a scripted-diff
ACKs for top commit:
optout21:
reACK fa43897c1d14549e7af0d9f912e765875b634c39
ajtowns:
ACK fa43897c1d14549e7af0d9f912e765875b634c39
Crypt-iQ:
crACK fa43897c1d14549e7af0d9f912e765875b634c39
achow101:
ACK fa43897c1d14549e7af0d9f912e765875b634c39
Tree-SHA512: d03d8ea35490a995f121be3d2f3e4a22d1aadfeab30bc42c4f8383dab0e6e27046260e792d9e5a94faa6777490ba036e39c71c50611a38f70b90e3a01f002c9e
Bundle all function's outputs inside the util::Result returned object.
Reasons for the refactoring:
- The 'change_pos' ref argument has been a source of bugs in the past.
- The 'fee' ref argument is currently only set when the transaction creation process succeeds.
48161f6a0503d7dde693ef544f0d3285c8b93adc wallet: introduce "tx amount exceeds balance when fees are included" error (stratospher)
b7fa609ed1759472b004ce03c217cf4a5e32262c wallet: remove PreSelectedInputs (stratospher)
7819da2c1643e9ca892f0fc97ffc2003ac265dac walllet: use CoinsResult instead of PreSelectedInputs (stratospher)
e5474079f179c5637b6c5f2077a1c5223ea357e1 wallet: introduce GetAppropriateTotal() in CoinsResult (stratospher)
d8ea921d01404cc0b63b277878d0f2f988a1daba wallet: correctly reserve in CoinsResult::All() (stratospher)
7072d825e39d200c5e49c736a281d3db180c716a wallet: ensure COutput added in set are unique (stratospher)
fefa3be782eaf3e2fbff3ed8772fb91f2134ac0d 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. c467325aaf187d7f056bb1ea1cec6b7c4250af2e: make `total_effective_amount` optional actually optional
2. 2202ab597596c84fc49f8784e823372b7a9efcbe: ensure `set<shared_ptr<COutput>>` has unique COutput
3. a5ffbbf122d66fc4ad9b2e7c6d7d1dfa1816388e: Correctly reserve size when flattening `CoinsResult.coins` map to vector
3. the next 3 commits from 4745d5480ca5c3809edd51140e4d2c0433582844 replace the `PreSelectedInputs` struct with `CoinsResult` and removes `PreSelectedInputs`.
4. the last commit (e664484a6d34c1795ebb0925ab31faea5d64ab00) 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 48161f6a0503d7dde693ef544f0d3285c8b93adc
furszy:
utACK 48161f6
Tree-SHA512: a963fac8d6714f76571df8cf9aff70601536dc6faa4326fbb5892c3f080dc393f0d7c6e2d21879c7a2c898bf0092adb154376d9b0a8929b31575ce9d1d47dec2
PreSelectedInputs is confusing to use. it's `total_amount`
might store total amount or effective amount based on SFFO.
ex: we might accidentally sum preselected inputs effective
amount (named `total_amount`) with automatically selected
inputs actual total amount.
CoinsResult has a cleaner interface with separate fields
for both these amounts.
2 behavioural changes:
1. no more default assert error if effective value is unset
- previously PreSelectedInputs::Insert() called
COutput::GetEffectiveValue() which assert failed
if the optional was unset.
- now we don't default assert anymore.
* in GUI/getAvailableBalance better not to assert.
* SelectCoins's preselected inputs always contain a
feerate, so effective amount should be set.
explicitly added an assertion to ensure this.
2. FetchSelectedInputs uses OutputType::UNKNOWN as key to
populate CoinsResult's coins map. it's discarded later.
This refactor does not change behavior. However, it avoids a vector
copy, which can lead to a minimal speed-up of 1%-5%, depending on the
call-site. This is mostly relevant for the fuzz tests and utils that
read large blobs of data (like a full block).
returns the total amount (if SFFO), otherwise the effective amount.
previously, this was the logic in calculating
PreSelectedInputs::total_amount when PreSelectedInputs::Insert()
was called.
return optional to force callers to explicitly handle the case
when effective amount optional is not set.
coins.size() would be the number of the OutputType keys in the map.
whereas Size() would return total number of COutput objects when
flattening the map.
before #25806, set<COutput> was used and would not
contain same COutputs in the set.
now we use set<shared_ptr<COutput>> and it might be
possible for 2 distinct shared_ptr (different pointer
address but same COutputs) to be added into the set.
so preserve previous behaviour by making sure values
in the set are also distinct
e67a676df9af5ece5307438ae1b4ddb0730e3482 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 e67a676df9af5ece5307438ae1b4ddb0730e3482
optout21:
crACK e67a676df9af5ece5307438ae1b4ddb0730e3482
carloantinarella:
Tested ACK e67a676df9af5ece5307438ae1b4ddb0730e3482
achow101:
ACK e67a676df9af5ece5307438ae1b4ddb0730e3482
musaHaruna:
Tested ACK [e67a676](e67a676df9)
Tree-SHA512: b156f7ed15c3fbb50e8a15f6c9a0d4e2ffb956d0c6ef949e0f8a661a564a20c0d3ed2149fae75ce7e2baa9326788d5037e726e7a7ac2c6ef4e70e4862cd5763f
54d039305823f67688ec9116757d8244f84badc6 FUZZ: Test that BnB finds best solution (Murch)
Pull request description:
BnB’s solution is the input set with the lowest waste score, excluding any supersets of other solution candidates.
This fuzz test compares a brute force search with the BnB result to ensure that BnB succeeds.
ACKs for top commit:
achow101:
ACK 54d039305823f67688ec9116757d8244f84badc6
brunoerg:
ACK 54d039305823f67688ec9116757d8244f84badc6
Tree-SHA512: 96b6a822f53311d9a76abe8c217794e0a2dd5bd713db0a15dc70e065099b8245c430e1174e24133e0a802218ff0f2943dfcc3d638c3716485d5607c452854e7d
