StdLockGuard and clang's thread safety annotations did not ensure that
the lock it was taking was not already held. Add a STDLOCK() macro which
uses an annotated StdMutex::CheckNotHeld() function to correct that.
faea12ecd9 test: Fixup docs for NodeClockContext and SteadyClockContext (MarcoFalke)
eeeeb2a0b9 fuzz: Use NodeClockContext (MarcoFalke)
fa4fae6227 test: Add NodeClockContext (MarcoFalke)
Pull request description:
Iterating over fuzz inputs will usually be done in the same process. As the mocktime is global, it can theoretically leak from one fuzz input run into the next run, making it less deterministic.
Fix this issue, by adding and using a context manager to handle the mocktime and reset it before the end.
This refactor should not change any behavior.
ACKs for top commit:
seduless:
re-ACK faea12ecd9
dergoegge:
utACK faea12ecd9
brunoerg:
code review ACK faea12ecd9
Tree-SHA512: e222c4e4217a504d058b30f1e975dfdfff019363c82385bd62f368b16fb029c46a5d1b43cd773dbdd9efcd7f968d46dbe2c75812971696b1b879b8f081fc6b1b
af0da2fce2 crypto: Use `secure_allocator` for `AES256CBC*::iv` (David Gumberg)
d53852be31 crypto: Use `secure_allocator` for `AES256_ctx` (David Gumberg)
8c6fedaa81 build: `lockedpool.cpp` kernel -> crypto (David Gumberg)
51ac1abf6f bench: Add wallet encryption benchmark (David Gumberg)
9a15872516 wallet: Make encryption derivation clock mockable (David Gumberg)
ae5485fa0d refactor: Generalize derivation target calculation (David Gumberg)
Pull request description:
Fixes#31744
Reuse `secure_allocator` for `AES256_ctx` in the aes 256 encrypters and decrypters and the `iv` of `AES256CBC` encrypters and decrypters. These classes are relevant to `CCrypter`, used for encrypting wallets, and my understanding is that if an attacker knows some or all of the contents of these data structures (`AES256_ctx` & `iv`) they might be able to decrypt a user's wallet.
Presently the `secure_allocator` tries to protect sensitive data with `mlock()` on POSIX systems and `VirtualLock()` on Windows to prevent memory being paged to disk, and by zero'ing out memory contents on deallocation with `memory_cleanse()` which is similar to `OPENSSL_cleanse()` by scaring compilers away from optimizing `memset` calls on non-Windows systems, and using `SecureZeroMemory()` on Windows.
ACKs for top commit:
achow101:
ACK af0da2fce2
furszy:
utACK af0da2fce2
theStack:
re-ACK af0da2fce2
Tree-SHA512: 49067934fd2f2b285fc7b1a7c853fd2d4475431b3a811ae511f61074dc71a99a0826c3ab40ab4a5dfc84b2b9914a90c920d2484b38ac19502e3bd6170ad27622
b19caeea09 doc: add release note for #31560 (named pipe support for `dumptxoutset` RPC) (Sebastian Falbesoner)
61a5460d0d test: add test for utxo-to-sqlite conversion using named pipe (Sebastian Falbesoner)
2e8072edbe rpc: support writing UTXO set dump (`dumptxoutset`) to a named pipe (Sebastian Falbesoner)
Pull request description:
This PR slightly modifies the `dumptxoutset` RPC to allow writing the UTXO set dump into a [named pipe](https://askubuntu.com/a/449192), so that the output data can be consumed by another process, see #31373. Taking use of this with the utxo-to-sqlite.py tool (introduced in #27432), creating an UTXO set in SQLite3 format is possible on the fly. E.g. for signet:
```
$ mkfifo /tmp/utxo_fifo && ./build/bin/bitcoin-cli -signet dumptxoutset /tmp/utxo_fifo latest &
$ ./contrib/utxo-tools/utxo_to_sqlite.py /tmp/utxo_fifo ./utxo.sqlite
UTXO Snapshot for Signet at block hash 000000012711f0a4e741be4a22792982..., contains 61848352 coins
1048576 coins converted [1.70%], 2.800s passed since start
....
....
60817408 coins converted [98.33%], 159.598s passed since start
{
"coins_written": 61848352,
"base_hash": "000000012711f0a4e741be4a22792982370f51326db20fca955c7d45da97f768",
"base_height": 294305,
"path": "/tmp/utxo_fifo",
"txoutset_hash": "34ae7fe7af33f58d4b83e00ecfc3b9605d927f154e7a94401226922f8e3f534e",
"nchaintx": 28760852
}
TOTAL: 61848352 coins written to ./utxo.sqlite, snapshot height is 294305.
```
Note that the `dumptxoutset` RPC calculates an UTXO set hash as a first step before any data is emitted, so especially on mainnet it takes quite a while until the conversion starts and something is happening visibly.
ACKs for top commit:
ajtowns:
utACK b19caeea09
sedited:
Re-ACK b19caeea09
Tree-SHA512: 7101563d0dba15439cdef8c8fb535f8593d5a779ff04208e2d72382a3f99072db8eac3651d1b3fe72c5e1f03e164efb281c3030d45d0723b943ebbbcf2a841d6
This allows external tooling (e.g. converters) to consume the output
directly, rather than having to write the dump to disk first and then
read it from there again.
Co-authored-by: Luke Dashjr <luke-jr+git@utopios.org>
79571b9181 threadpool: add ranged Submit overload (Andrew Toth)
Pull request description:
The current `ThreadPool::Submit` is not very efficient when we have a use case where we need to submit multiple tasks immediately. The `Submit` method must take the lock for each task, and notifies only a single worker thread. This will cause lock contention with the awakened worker thread trying to take the lock and the caller trying to submit the next task.
Introduce a `Submit` overload, which takes the lock once and submits a range of tasks, then notifies all worker threads after the lock is released.
This is needed for #31132 to be able to use `ThreadPool`.
ACKs for top commit:
l0rinc:
ACK 79571b9181
rkrux:
ACK 79571b9
sedited:
Re-ACK 79571b9181
willcl-ark:
ACK 79571b9181
Tree-SHA512: 1fbe0c150f01b9ea5be3459cd10b817045af52eaf6f14a1a298a68853890da4033c1b21bdc6f995bb55029fb4ab536e9dbf58d98e2e1e12b25298fa3470b4ba6
02d047fd5b refactor: add overflow-safe `CeilDiv` helper (Lőrinc)
Pull request description:
### Problem
The codebase has many open-coded ceiling-division expressions (for example `(x+y-1)/y`) scattered across files.
These are less readable, duplicate logic, and can be overflow-prone in edge cases.
### Fix
Introduce a small overflow-safe integer helper, `CeilDiv()`, and use it in existing **unsigned** callsites where the conversion is straightforward and noise-free.
### What this PR does
* Adds `CeilDiv()` to `src/util/overflow.h` for unsigned integral inputs.
* Keeps the precondition check `assert(divisor > 0)`.
* Replaces selected unsigned ceiling-division expressions with `CeilDiv(...)`.
* Adds focused unit tests in `src/test/util_tests.cpp` for the migrated patterns.
---
This is a pure refactor with no intended behavioral change.
Signed arithmetic callsites are intentionally left unchanged in this PR.
This PR changed a few more things originally but based on feedback reverted to the simplest cases only.
ACKs for top commit:
rustaceanrob:
ACK 02d047fd5b
hodlinator:
ACK 02d047fd5b
sedited:
ACK 02d047fd5b
Tree-SHA512: b09336031f487e6ce289822e0ffeb8cfc8cfe8a2f4f3f49470748dfbd0a6cbab97498674cb8686dd2bd4ab6dd0b79cfdf2da00041fee12d109892e1bc5dde0ff
faa016af54 refactor: Use aliasing shared_ptr in Sock::Wait (MarcoFalke)
Pull request description:
Currently, a no-op lambda is used as the deleter for the temporary shared pointer helper in `Sock::Wait`. This is perfectly fine, but has a few style issues:
* The lambda needs to be allocated on the heap
* It triggers a false-positive upstream GCC-16-trunk bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=123912
Fix all issues by just using an aliasing shared pointer, which points to `this`, but is otherwise empty (sits on the stack without any heap allocations).
ACKs for top commit:
hodlinator:
ACK faa016af54
sedited:
ACK faa016af54
vasild:
ACK faa016af54
Tree-SHA512: b7330862204e79fb61f30694accb16f9a24e5722bd0ceb098ca27c877cff921afa00c0cfd953d4cbb355e6433706961a25b628efefdbe0b48bdec2941eaaee7a
408d5b12e8 test: include response body in non-JSON HTTP error msg (Matthew Zipkin)
9dc653b3b4 test: threadpool, add coverage for all Submit() errors (furszy)
ce2a984ee3 test: cleanup, use HasReason in threadpool_tests.cpp (l0rinc)
d9c6769d03 test: refactor, decouple HasReason from test framework machinery (furszy)
dbbb780af0 test: move and simplify BOOST_CHECK ostream helpers (Hodlinator)
3b7cbcafcb test: ensure Stop() thread helps drain the queue (seduless)
ca101a2315 test: coverage for queued tasks completion after interrupt (furszy)
bf2c607aaa threadpool: active-wait during shutdown (furszy)
e88d274430 test: add threadpool Start-Stop race coverage (furszy)
8cd4a4363f threadpool: guard against Start-Stop race (furszy)
9ff1e82e7d test: cleanup, block threads via semaphore instead of shared_future (l0rinc)
Pull request description:
A few follow-ups to #33689, includes:
1) `ThreadPool` active-wait during shutdown:
Instead of just waiting for workers to finish processing tasks, `Stop()` now helps them actively.
This speeds up the JSON-RPC and REST server shutdown, resulting in a faster node shutdown when many requests remain unhandled. This wasn't included in the original PR due to the behavior change this introduces.
2) Decouple `HasReason` from the unit test framework machinery
This avoids providing the entire unit test framework dependency to low-level tests that only require access to the `HasReason` utility class. Examples are: `reverselock_tests.cpp`, `sync_tests.cpp`, `util_check_tests.cpp`, `util_string_tests.cpp`, `script_parse_tests.cpp` and `threadpool_tests.cpp`. These tests no longer gain access to unnecessary components like the chainstate, node context, caches, etc. It includes l0rinc's `threadpool_tests.cpp` `HasReason` changes.
3) Include response body in non-JSON HTTP error messages
Straight from pinheadmz [comment](https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2783817192), it makes debugging CI issues easier.
ACKs for top commit:
maflcko:
review ACK 408d5b12e8🕗
achow101:
ACK 408d5b12e8
hodlinator:
re-ACK 408d5b12e8
Tree-SHA512: 57aa0ef96886f32bf95a0bd7f87c878d31c9df9e34cb96de615eee703ce0824b5cfdf8f5c9cd19a3594559994295b5810c38c94f5efd6291cbbd83a95473357a
Instead of waiting for the workers to finish processing tasks, help
them actively inside Stop().
This speeds up the JSON-RPC and REST server shutdown procedure,
and results in a faster node shutdown when many requests remain unhandled
Stop() has two windows where Start() could cause troubles:
1) m_workers is temporarily empty while workers are being joined,
this creates a window where Start() could slip through and reset
m_interrupt to false, preventing the old workers from exiting and
causing a deadlock.
2) Start() could be called after workers are joined but before the
empty() sanity check on m_work_queue, causing a crash.
Fix both races by keeping m_interrupt set for the entire duration
of Stop(), so any concurrent Start() call is rejected until all
workers have exited.
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
Introduce `TrySub(T&, U)` which subtracts an unsigned integral `U` from an unsigned integral `T`, returning `false` on underflow.
Use with `Assume(TrySub(...))` at coins cache accounting decrement sites so invariant violations fail immediately rather than silently wrapping.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Co-authored-by: Pieter Wuille <pieter@wuille.net>
726b3663cc http: properly respond to HTTP request during shutdown (furszy)
59d24bd5dd threadpool: make Submit return Expected instead of throwing (furszy)
Pull request description:
Fixes#34573.
As mentioned in https://github.com/bitcoin/bitcoin/issues/34573#issuecomment-3891596958, the ThreadPool PR (#33689) revealed an existing issue.
Before that PR, we were returning an incorrect error "Request rejected because http work queue depth exceeded" during shutdown for unhandled requests (we were not differentiating between "queue depth exceeded" and "server interrupted" errors). Now, with the ThreadPool inclusion, we return the proper error but we don't handle it properly.
This PR improves exactly that. Handling the missing error and properly returning it to the user.
The race can be reproduced as follows:
1) The server receives an http request.
2) Processing of the request is delayed, and shutdown is triggered in the meantime.
3) During shutdown, the libevent callback is unregistered and the threadpool interrupted.
4) The delayed request (step 2) resumes and tries to submit a task to the now-interrupted server.
Reproduction test can be found https://github.com/bitcoin/bitcoin/pull/34577#issuecomment-3902672521.
Also, to prevent this kind of issue from happening again, this PR changes task submission
to return the error as part of the function's return value using `util::Expected` instead of
throwing the exception. Unlike exceptions, which require extra try-catch blocks and can be
ignored, returning `Expected` forces callers to explicitly handle failures, and attributes
like `[[nodiscard]]` allow us catch unhandled ones at compile time.
ACKs for top commit:
achow101:
ACK 726b3663cc
sedited:
ACK 726b3663cc
pinheadmz:
re-ACK 726b3663cc
andrewtoth:
ACK 726b3663cc
hodlinator:
re-ACK 726b3663cc
Tree-SHA512: ef026e299adde1148c9fc575e7d937e957bf0ddedfc1cf081941b568736417c2eefcd8bc8c8aea795d7347040ed05da4371bddcdbda7d385e04bf4dc8d875780
1b36bf0c5d subprocess: Fix `-Wunused-private-field` for `Child` class on Windows (Hennadii Stepanov)
9f2b338bc0 subprocess: Fix `-Wunused-private-field` for `Popen` class on Windows (Hennadii Stepanov)
Pull request description:
This PR is a prerequisite for https://github.com/bitcoin/bitcoin/pull/31507.
It resolves `-Wunused-private-field` warnings triggered in `src/util/subprocess.h` when compiling with clang-cl on Windows:
```
D:\a\bitcoin\bitcoin\src\util/subprocess.h(759,10): warning : private field 'parent_' is not used [-Wunused-private-field] [D:\a\bitcoin\bitcoin\build\src\util\bitcoin_util.vcxproj]
D:\a\bitcoin\bitcoin\src\util/subprocess.h(760,7): warning : private field 'err_wr_pipe_' is not used [-Wunused-private-field] [D:\a\bitcoin\bitcoin\build\src\util\bitcoin_util.vcxproj]
D:\a\bitcoin\bitcoin\src\util/subprocess.h(1038,7): warning : private field 'child_pid_' is not used [-Wunused-private-field] [D:\a\bitcoin\bitcoin\build\src\util\bitcoin_util.vcxproj]
```
Only the second commit has been [submitted](https://github.com/arun11299/cpp-subprocess/pull/127) upstream. The first commit is specific to this repository and not applicable upstream.
ACKs for top commit:
maflcko:
review ACK 1b36bf0c5d👋
purpleKarrot:
ACK 1b36bf0c5d
sedited:
ACK 1b36bf0c5d
Tree-SHA512: 1bc0544d769264fa74d2f39150595ee6339af4bca7b7051ecaecbe234c17b643b715e00cfb9302a16ffc4856957f4fa47c216aebf03fec0cd95c387f51bd29a6
fa48d42163 test: Stricter unit test (MarcoFalke)
fa626bd143 util: Remove brittle and confusing sp::Popen(std::string) (MarcoFalke)
Pull request description:
The subprocess Popen call that accepts a full `std::string` has many issues:
* It promotes brittle and broken code, where spaces are not properly quoted. Example: https://github.com/bitcoin/bitcoin/pull/33929#discussion_r2590523065
* The internally used `util::split` function does incorrectly split on spaces, instead of using `shlex.split`.
* It is redundant and not needed, because a vector interface already exists.
Fix all issues by removing it and just using the vector interface.
This pull request should not change any behavior: Note that the command taken from `gArgs.GetArg("-signer", "")` is still passed through the `sp::util::split` helper, just like before. Fixing that is left for a follow-up, so that this change here is basically just a refactor.
This also fixes a unit test bug as a side-effect: Fixes https://github.com/bitcoin/bitcoin/issues/32574.
ACKs for top commit:
janb84:
cr ACK fa48d42163
fjahr:
Code review ACK fa48d42163
hebasto:
re-ACK fa48d42163.
Tree-SHA512: 3d29226977c9392502f9361e2bd42b471ad03761bbf6a94ef6e545cbe4492ad5858da1ac9cc64b2791aacb9b6e6f3c3f63dbcc3a2bf45f6a13b5bc33eddf8c2b
Unlike exceptions, which can be ignored as they require extra try-catch
blocks, returning expected errors forces callers to always handle
submission failures.
Not throwing an exception also fixes an unclean shutdown bug
#34573 since we no longer throw when attempting to Submit()
from the libevent callback http_request_cb().
c1355493e2 refactor: fees: split fee rate format from fee estimate mode (ismaelsadeeq)
922ebf96ed refactor: move-only: move `FeeEstimateMode` enum to `util/fees.h` (ismaelsadeeq)
Pull request description:
### Motivation
Part of #34075
- The `FeeEstimateMode` enum was responsible for both selecting the fee estimation algorithm and specifying the fee rate' format.
#### Changes in this PR:
* The `FeeEstimateMode` enum (`UNSET`, `ECONOMICAL`, `CONSERVATIVE`) is moved to a new util/fees.h header.
* A new `FeeRateFormat `enum (`BTC_KVB`, `SAT_VB`) is introduced in `policy/feerate.h` for feerate formatting.
* The `CFeeRate::ToString()` method is updated to use `FeeRateFormat`.
* All relevant function calls have been updated to use the new `FeeRateFormat` enum for formatting and `FeeEstimateMode` for fee estimation mode.
This refactoring separates these unrelated responsibilities to improve code clarity.
ACKs for top commit:
l0rinc:
ACK c1355493e2
furszy:
utACK c1355493e2
musaHaruna:
ACK [c135549](c1355493e2) — reviewed in the context of PR [34075](https://github.com/bitcoin/bitcoin/pull/34075)
willcl-ark:
ACK c1355493e2
Tree-SHA512: 7cbe36350744313d3d688d3fd282a58c441af1818b1e8ad9cddbc911c499a5205f8d4a39c36b21fed60542db1ef763eb69752d141bcef3393bf33c0922018645
Introduce `CeilDiv()` for integral ceiling division without the typical `(dividend + divisor - 1) / divisor` overflow, asserting a non-zero divisor.
Replace existing ceiling-division expressions with `CeilDiv()` to centralize the preconditions.
Add unit tests covering return type deduction, max-value behavior, and divisor checks.
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
- 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.
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
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. 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 48161f6a05
furszy:
utACK 48161f6
Tree-SHA512: a963fac8d6714f76571df8cf9aff70601536dc6faa4326fbb5892c3f080dc393f0d7c6e2d21879c7a2c898bf0092adb154376d9b0a8929b31575ce9d1d47dec2
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
Preparation for a future commit where kernel's dependency
on logging.cpp is removed completely.
Replace usage of logging\.h with util/log\.h where it
suffices, and fix wrong includes according to iwyu.
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
With this change, callers can use util/log.h to emit log messages and do not need to
include the full logging implementation in logging.h.
There's a potential performance impact with this change from an extra
`strprintf` call in log statements where `Logger::WillLogCategoryLevel` returns
true but `Logger::Enabled` returns false. This happens when bitcoind is run
with `-noprinttoconsole -nodebuglogfile` options.
For background, log macro arguments are supposed to be evaluated when
`Logger::WillLogCategoryLevel` returns true, even if log output is not enabled.
Changing this behavior would be reasonable but needs consideration in a
separate PR since not evaluating arguments in log statements has the potential
to change non-logging behavior.
The extra `strprintf` call could have been avoided by expanding this change and
making the `ShouldLog()` function return a tri-state DO_LOG / DO_NOT_LOG /
DO_NOT_LOG_ONLY_EVALUATE_ARGS value instead of a bool, but this complexity did
not seem warranted.
Review with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Move logging macros to util/log.h so the entire codebase can use the same
macros.
Review with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
This is not strictly move-only because BCLog::Level is now defined as a type
alias for util::log::Level;
Review with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Introduce util/log.h as a lightweight header for code that only needs to emit
log messages. Move SourceLocation into this header so log-emitting code no
longer needs to include logging.h and depend on the full log management API.
This is a move-only change and the first change of several changes that
separate log generation from log handling. It also applies clang-format
suggestions to the moved code.
Review with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Currently, there are issues with headers in `batchpriority.cpp`:
1. `SCHED_BATCH` is not defined on all supported *BSD platforms.
2. `pthread.h` is necessary on other platforms.
This addresses both issues and fixes other includes.
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
ccf9172ab3 util: Remove `FilterHeaderHasher` (rustaceanrob)
Pull request description:
With respect to `std::unordered_map` documentation, the `Hash` type
defined in the template is over the `Key` and not `T`, the value. This
hasher is incorrectly named as the `FilterHeader` is the value within this map.
I consider this a bug as opposed to a refactor as the key and value
relationship is implied to be `filter header -> block hash` when it is
the opposite.
Further, the hasher for the key already exists via `BlockHasher`.
ref: https://en.cppreference.com/w/cpp/container/unordered_map.html
ACKs for top commit:
andrewtoth:
ACK ccf9172ab3
maflcko:
lgtm ACK ccf9172ab3
ismaelsadeeq:
ACK ccf9172ab3👍🏾
Tree-SHA512: 607602391bf337d4e25b04a6a643fa32c3ab4599009b181b46ecdb0705e8ff2af89a6192042453c9e8e44abcb2150589019f02c5c944ecdff41322c3e0ad45ac
40735450c0 Remove unused epochguard.h (Suhas Daftuar)
1a8494d16c Rework CTxMemPool::GetChildren() to not use epochs (Suhas Daftuar)
Pull request description:
Since #33591, the epoch-based graph traversal optimization logic is only used for `CTxMempool::GetChildren()`, a function that is only used in RPC code and tests. Rewrite it without epochs, and remove `util/epochguard.h` itself, as that was its last use.
This allows us to reduce per-transaction memory usage by 8 bytes, for no material loss. With the new TxGraph-based mempool implementation, I also don't foresee future uses for it, as TxGraph can do even better by using BitSet-based traversal tracking.
ACKs for top commit:
ajtowns:
ACK 40735450c0
instagibbs:
ACK 40735450c0
l0rinc:
code review ACK 40735450c0
Tree-SHA512: 7ce7c04835cd2425a71c4fd47f316b6fb7381caa27383de7ecc4aa81100fcf7bc5e062699b307c08e0b853b35f06710d9ac761d6e660af9f9331e708d36f2fe0
When compiling with clang-cl on Windows, `src/util/subprocess.h` emits
`-Wunused-private-field` warnings about unused private fields in the
`Child` class.
When compiling with clang-cl on Windows, `src/util/subprocess.h` emits
`-Wunused-private-field` warnings about unused private fields in the
`Popen` class.
1911db8c6d string: add LineReader (Matthew Zipkin)
ee62405cce time: implement and test RFC1123 timestamp string (Matthew Zipkin)
eea38787b9 string: add AsciiCaseInsensitive{KeyEqual, Hash} for unordered map (Matthew Zipkin)
4e300df712 string: add `base` argument for ToIntegral to operate on hexadecimal (Matthew Zipkin)
0b0d9125c1 Modernize GetBindAddress() (Matthew Zipkin)
a0ca851d26 Make GetBindAddress() callable from outside net.cpp (Matthew Zipkin)
Pull request description:
This is a component of [removing libevent as a dependency of the project](https://github.com/bitcoin/bitcoin/issues/31194). It is the first six commits of #32061 and provides a string-parsing utility (`LineReader`) that is also consumed by #34158.
These are the functions that are added / updated for HTTP and Torcontrol:
- `GetBindAddress()`: Given a socket, provides the bound address as a CService. Currently used by p2p but moved from `net` to `netbase` so other modules can call it.
- `ToIntegral()`: Already used to parse numbers from strings, added new argument `base = 10` so it can also be used to parse hexadecimal integers. HTTP chunked transfer-encoding uses hex-encoded integers to specify payload size: https://datatracker.ietf.org/doc/html/rfc7230.html#section-4.1
- `AsciiCaseInsensitive` comparators: Needed to store HTTP headers in an `unordered_map`. Headers are key-value pairs that are parsed with case-insensitive keys: https://httpwg.org/specs/rfc9110.html#rfc.section.5.1
- `FormatRFC1123DateTime()`: The required datetime format for HTTP headers (e.g. `Fri, 31 May 2024 19:18:04 GMT`)
- `LineReader`: Fields in HTTP requests are newline-terminated. This struct is given an input buffer and provides methods to read lines as strings.
ACKs for top commit:
maflcko:
review ACK 1911db8c6d👲
furszy:
utACK 1911db8c6d
sedited:
ACK 1911db8c6d
Tree-SHA512: bb8d3b7b18f158386fd391df6d377c9f5b181051dc258efbf2a896c42e20417a1b0b0d4637671ebd2829f6bc371daa15775625af989c19ef8aee76118660deff
This is a helper struct to parse HTTP messages from data in buffers
from sockets. HTTP messages begin with headers which are
CRLF-terminated lines (\n or \r\n) followed by an arbitrary amount of
body data. Whitespace is trimmed from the field lines but not the body.
https://httpwg.org/specs/rfc9110.html#rfc.section.5
