fadf901fd4 rpc: Run type check on decodepsbt result (MarcoFalke)
fa4d5891b9 refactor: Introduce TxDocOptions (MarcoFalke)
fa8250e961 refactor: Add and use RPCResultOptions (MarcoFalke)
Pull request description:
For RPCResults, the type may be ELISION, which is confusing and brittle:
* The elision should only affect the help output, not the type.
* The type should be the real type, so that type checks can be run on it.
Fix this issue by introducing a new print_elision option and using it in `decodepsbt`.
This change will ensure that `RPCResult::MatchesType` is properly run.
Can be tested by introducing a bug:
```diff
diff --git a/src/core_io.cpp b/src/core_io.cpp
index 7492e9ca50..4927b70c8e 100644
--- a/src/core_io.cpp
+++ b/src/core_io.cpp
@@ -436,2 +436,3 @@ void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry
entry.pushKV("version", tx.version);
+ entry.pushKV("bug", "error!");
entry.pushKV("size", tx.ComputeTotalSize());
```
And then running (in a debug build) `decodepsbt cHNidP8BAAoCAAAAAAAAAAAAAA==`
Before, on master: passes
Now, on this pull: Properly detects the bug
ACKs for top commit:
nervana21:
tACK fadf901fd4
achow101:
ACK fadf901fd4
willcl-ark:
ACK fadf901fd4
satsfy:
re-ACK fadf901fd4
seduless:
re-ACK fadf901fd4
Tree-SHA512: 4fb000dba9fe39bcd2bac72e2d88553f54134a250c985b4ca7150b483d7185009047d8fe4ba75c522bfc26706de20c913b8905e7552ab0c41802ae744cb92038
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
22335474d7 net: format peer+addr logs with `LogPeer` (Lőrinc)
e55ea534f7 test: add pre-`LogPeer` net log assertion (Lőrinc)
736b17c0f0 log: fix minor formatting in debug logs (Lőrinc)
9cf82bed32 log: show placeholders for missing peer fields (Lőrinc)
Pull request description:
This is an alternative to #34293, but aims to address the remaining logging inconsistencies more broadly.
It extends the example fixed there to every instance, restores the original separator behavior, applies it consistently via a single helper, and adds tests for `logips` (covering both current and new behavior).
### Problem
After #28521 centralized peer address logging into `CNode::LogIP()`, the original comma separator before `peeraddr=` was lost, resulting in inconsistent formatting across net (and recent private broadcast) logs.
Some lines also had double spaces, empty fields, or mismatched format specifiers.
### Fix
Introduces `CNode::LogPeer(bool)` which always emits `peer=<id>` and, when `-logips=1`, appends `, peeraddr=<addr>`. This eliminates hand-rolled separators and makes peer identification predictable.
Minor issues (double spaces, empty placeholders, format specifiers) are fixed along the way in separate commits.
### Reproducer
Run with `-debug=net -logips=1` and observe peer log lines now show `peer=<id>, peeraddr=<addr>` (comma-separated). The new assertion in `feature_logging.py` automates this check.
ACKs for top commit:
naiyoma:
ACK 22335474d7
vasild:
ACK 22335474d7
sedited:
ACK 22335474d7
Tree-SHA512: 562262a58c3042f139099ff4c41e3fc6a97505fe9603c2bf700a97fd0aa052954b47c14da0e50c1fc311db1ae6c04e6a92156c9b85e25c777a637b7766c1dafe
c6a6435ced wallet: remove `DBErrors::NEED_REWRITE` enum value (rkrux)
61039d72a5 wallet: remove unimplemented `RewriteDB` calls from SPKM (rkrux)
Pull request description:
ISTM that there is no implementation left of the `RewriteDB` method in any of the SPKMs, and thus, its call sites can be removed safely.
Also remove `DBErrors::NEED_REWRITE` enum value as its usage is outdated now.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improve coverage are always welcome.
* All other changes should have accompanying unit tests (see `src/test/`) or
functional tests (see `test/`). Contributors should note which tests cover
modified code. If no tests exist for a region of modified code, new tests
should accompany the change.
* Bug fixes are most welcome when they come with steps to reproduce or an
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
* Features are welcome, but might be rejected due to design or scope issues.
If a feature is based on a lot of dependencies, contributors should first
consider building the system outside of Bitcoin Core, if possible.
* Refactoring changes are only accepted if they are required for a feature or
bug fix or otherwise improve developer experience significantly. For example,
most "code style" refactoring changes require a thorough explanation why they
are useful, what downsides they have and why they *significantly* improve
developer experience or avoid serious programming bugs. Note that code style
is often a subjective matter. Unless they are explicitly mentioned to be
preferred in the [developer notes](/doc/developer-notes.md), stylistic code
changes are usually rejected.
-->
<!--
Bitcoin Core has a thorough review process and even the most trivial change
needs to pass a lot of eyes and requires non-zero or even substantial time
effort to review. There is a huge lack of active reviewers on the project, so
patches often sit for a long time.
-->
ACKs for top commit:
achow101:
ACK c6a6435ced
furszy:
ACK c6a6435ced
sedited:
ACK c6a6435ced
Tree-SHA512: 050d3115ad436eb7728716c897c8d23c6dd0c6cd7a018dc023896d301b126bae5ba89b684811226a0ca4045b393b612296aeab3b3b5dfb7ed0bf443fc1eaef41
af041c4057 wallet: Always rewrite tx records during migration (Ava Chow)
Pull request description:
Since loading a wallet may change some parts of tx records (e.g. adding nOrderPos), we should rewrite the records instead of copying them so that the automatic upgrade does not need to be performed again when the wallet is loaded.
This is useful for future PRs I'm working on where we need to be sure about what data exists in a tx record in descriptor wallets.
ACKs for top commit:
rkrux:
lgtm ACK af041c4057
Eunovo:
ACK af041c4057
furszy:
ACK af041c4057
Tree-SHA512: 96984e46fe110a7749495965587b88a94d1297794e5d8b632b89dcdb2ebc1cd3070cd4458cf8e1b4ec0c76c4e56994f21867c44fa74f25739cbd9c0c911732a6
Adds a special case where if the elapsed time during measurement of DKF
performance is 0, the default derive iterations are used so that
behavior is stable for testing and benchmarks.
501a3dd4ad walletdb: hash pubkey/privkey in one shot to avoid leaking secret data (Sebastian Falbesoner)
Pull request description:
In several places in the wallet DB module, byte strings containing serialized public keys and secret keys are created in order to be hashed. To avoid sensitive data lingering in memory (and potentially leaking), don't store the preimage, but hash both public key and secret key in one shot, using the overloaded `Hash` function:
d198635fa2/src/hash.h (L82-L88)
See e.g. #31166 and #31774 for similarly themed PRs (Note that in #31166 we used the explicit `memory_cleanse` approach though, as changing the allocator was not possible.)
ACKs for top commit:
davidgumberg:
crACK 501a3dd4ad
furszy:
ACK 501a3dd4ad
rkrux:
ACK 501a3dd
theuni:
ACK 501a3dd4ad
Tree-SHA512: 8a71685b26bf89fca181aed6512a8db843b6d1dc740a468bb33fb2a629a23167a9676c228d1077ad8db2df9db80f47e32ec013737e93df8ee6f4ba505d3d50c9
This refactor does not change any behavior.
However, it is nice to know that no global mocktime leaks from the fuzz
init step to the first fuzz input, or from one fuzz input execution to
the next.
With the clock context, the global is re-set at the end of the context.
a067ca3410 [doc] coin selection filters by max cluster count, not descendant (glozow)
f7be5fb8fc [refactor] rename variable to clarify it is unused and cluster count (glozow)
Pull request description:
Followup to #33629.
Fix misleading docs and variable names. Namely, `getTransactionAncestry` returns the cluster count, not max descendant count of ancestor set (not worth reimplementing as it is merely a heuristic). No behavior changes - I don’t think much needs to be changed for the first release containing cluster mempool.
Current `CoinEligibilityFilter`s enforce a maximum ancestor count (summed across all outputs, potentially overestimating) and max descendant count across ancestors of the output.
Since #33629, these filters have started using cluster count instead of max desc count across ancestors. The change isn’t dangerous, as the cluster count bounds descendant count as well. Currently, the wallet is essentially enforcing a mixture of both limits - this is good while we are transitioning. Note that the cluster count enforced is 25, not 64, since it's grabbing the node's descendant count limit. While it is not an apples-to-apples comparison, a cluster count limit of 25 helps us avoid busting legacy descendant limits (which will be common on the network for a while).
Potential things for the future, out of scope for this PR:
- When we get rid of the ancestor/descendant config options, `getPackageLimits` can probably be replaced with hard-coded values.
- Change the `OutputGroup`s to track the actual cluster count that results from spending these outputs and merging their clusters.
- Loosen from 25 after that policy is no longer common.
- Clean up `getPackageLimits`.
ACKs for top commit:
achow101:
ACK a067ca3410
ismaelsadeeq:
reACK a067ca3410
rkrux:
crACK a067ca3410
Tree-SHA512: d7cacd5bf668d42e26e8b83e42a42c280929c3bfd554c3db1de605e5939f8b36c14ecfd2839abeb4eec352363df1891b3420a693c250916391ab10a5ce26396b
Avoid storing the privkey in a vector, which could linger in memory
and potentially leak sensitive data. An alternative approach is to
use `secure_allocator` for the `std::vector` instances, but this
commit has the advantage of also deduplicating code at the same shot.
Thanks to @theuni for suggesting this.
98e8af4bb9 wallet: Drain validation interface queue after notifications disconnect (Ava Chow)
52992ebe1c interfaces: Add waitForNotifications() to call SyncWithValidationInterfaceQueue() (Ava Chow)
Pull request description:
When the wallet disconnects chain notifications, it is expecting no further notifications to execute, but this is not the case. This results in test failures such as in #34354. Instead of disconnecting the notifications and continuing shutdown, we should wait for the validation interface queue to be drained before the rest of wallet shutdown. This is achieved by adding an `interfaces::Chain::waitForNotifications()` function which calls `SyncWithValidationInterfaceQueue()`.
Fixes#34354
ACKs for top commit:
stickies-v:
utACK 98e8af4bb9
furszy:
ACK 98e8af4bb9
rkrux:
crACK 98e8af4bb9
sedited:
ACK 98e8af4bb9
Tree-SHA512: 263628556f740cb633d3970c22a0dfdb52a644bd1d0cd5a69c2970524edbb0e25d592cb39fc9bf1d0c281eebce09578526e2958dffee9026fc7473db35bd0dec
When unloading a wallet, there may be unexecuted callbacks in the
validation interface queue that can still execute after we have
completed all of the other wallet shutdown tasks. Instead of letting
these run in the background, once the notifications are disconnected,
wait for the queue to drain before continuing with wallet shutdown.
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
Move the operator<< overloads used by BOOST_CHECK_* out of the
unit test machinery test/setup_common, into test/util/common.h.
And replace the individual per-type ToString() overloads with
a single concept-constrained template that covers any type
exposing a ToString() method. This is important to not add
uint256.h and transaction_identifier.h dependencies to the
shared test/util/common.h file.
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
50cf6838e6 wallet: rpc: manpage: fix example missing `fee_rate` argument (SomberNight)
Pull request description:
The function signature for the `send` RPC is:
```
send [{"address":amount,...},{"data":"hex"},...] ( conf_target "estimate_mode" fee_rate options version )
```
The last example in the manpage is missing the `fee_rate` arg, but is trying to specify the `options` arg, by index. The parser confuses the intended `options` arg as the missing `fee_rate` arg.
See:
```
$ bitcoin-cli -rpcuser=doggman -rpcpassword=donkey -rpcport=18554 -regtest send '{"bcrt1qusm48zmlzwr32csxdw4ar7atw260h22c9ten9l": 0.1}' 1 economical '{"add_to_wallet": false, "inputs": [{"txid":"0b7e1a471dc948b7a6187936b16e6d7d9833629b2f9dd8a392eb89928f63aaad", "vout":0}]}'
error code: -8
error message:
Cannot specify both conf_target and fee_rate. Please provide either a confirmation target in blocks for automatic fee estimation, or an explicit fee rate.
```
vs
```
$ bitcoin-cli -rpcuser=doggman -rpcpassword=donkey -rpcport=18554 -regtest send '{"bcrt1qusm48zmlzwr32csxdw4ar7atw260h22c9ten9l": 0.1}' 1 economical null '{"add_to_wallet": false, "inputs": [{"txid":"0b7e1a471dc948b7a6187936b16e6d7d9833629b2f9dd8a392eb89928f63aaad", "vout":0}]}'
{
"psbt": "cHNidP8BAHECAAAAAa2qY4+SieuSo9idL5tiM5h9bW6xNnkYprdIyR1HGn4LAAAAAAD9////AkR2DwQAAAAAFgAUpLDwJu+wFRHLQAgKAb0psk7UVd2AlpgAAAAAABYAFOQ3U4t/E4cVYgZrq9H7q3K0+6lYAAAAAAABAIUCAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/////wQC4wMA/////wLIF6gEAAAAABYAFLMY1zihXrefAA0DA5nld4MCPjkrAAAAAAAAAAAmaiSqIant4vYcP3HR3v0/qZnfo2lTdVxpBol5mWK0i+vYNpdOjPkAAAAAAQEfyBeoBAAAAAAWABSzGNc4oV63nwANAwOZ5XeDAj45KwEIawJHMEQCIElTV4pbUrsPR9qHWcioowVv3QVWHizxwevfD0u/I8YyAiBCY3OzF81PSLM00h4ueQkehYuxDFZu7Jk51iejphKnnwEhA0VKdYVSyBpWoxBwTDOupB58Fi3mEBs+u+OOqEYVd2sZACICA98YLWyH7dBCfXVxe7woiLSTgV1mJN8Zc8KgZ77pVSg+GNBMeT5UAACAAQAAgAAAAIABAAAAbAAAAAAA",
"txid": "625b71b314a6ac4f738634e29dc007cd5edc0427c1ae96ab706d06a62910cea2",
"hex": "02000000000101adaa638f9289eb92a3d89d2f9b6233987d6d6eb1367918a6b748c91d471a7e0b0000000000fdffffff0244760f0400000000160014a4b0f026efb01511cb40080a01bd29b24ed455dd8096980000000000160014e437538b7f13871562066babd1fbab72b4fba9580247304402204953578a5b52bb0f47da8759c8a8a3056fdd05561e2cf1c1ebdf0f4bbf23c6320220426373b317cd4f48b334d21e2e79091e858bb10c566eec9939d627a3a612a79f012103454a758552c81a56a310704c33aea41e7c162de6101b3ebbe38ea84615776b1900000000",
"complete": true
}
```
ACKs for top commit:
svanstaa:
tACK 50cf6838e6
kannapoix:
Tested ACK 50cf6838e6
achow101:
ACK 50cf6838e6
rkrux:
tACK 50cf6838e6
theStack:
Tested ACK 50cf6838e6
Tree-SHA512: 499701729038cd863b612698098a73ce995589fc5ab08a2962f8edf1ff3cb3de6f7090e04722ca13ba7707a566fa3750ae549b6ad55750a3d01127eb6b94a79f
4b53cbd692 test: Test for musig() in various miniscript expressions (Ava Chow)
ec0f47b15c miniscript: Using Func and Expr when parsing keys, hashes, and locktimes (Ava Chow)
6fd780d4fb descriptors: Increment key_exp_index in ParsePubkey(Inner) (Ava Chow)
b12281bd86 miniscript: Use a reference to key_exp_index in KeyParser (Ava Chow)
ce4c66eb7c test: Test that key expression indexes match key count (Ava Chow)
Pull request description:
The miniscript parser currently only looks for the next `)` when parsing key, hash, and locktime expressions. This fails to parse when the expressions contain a nested expression. Currently, this is only possible with `musig()` inside of key expressions. However, this pattern can be generalized to handling hashes and locktimes, so I implemented those too.
Fixes#34076
ACKs for top commit:
rkrux:
ACK 4b53cbd692
sipa:
ACK 4b53cbd692
darosior:
Other than that, Approach ACK 4b53cbd692. That makes sense to me but i have not closely reviewed the code.
Tree-SHA512: 01040c7b07a59d8e3725ff11ab9543b256aea22535fb94059f490a5bb45319e859666af04c2f0a4edcb8cf1e6dfc7bd8a8271b21ad81143bafccd4d0a39cae9c
24f93c9af7 release note (Pol Espinasa)
331a5279d2 wallet, rpc:remove settxfee and paytxfee (Pol Espinasa)
Pull request description:
**Summary**
This PR removes the settxfee RPC and paytxfee setting (Bitcoin Core 31.0).
These two features were deprecated in https://github.com/bitcoin/bitcoin/pull/31278.
ACKs for top commit:
achow101:
ACK 24f93c9af7
w0xlt:
reACK 24f93c9af7
Tree-SHA512: e090f1a72ba2cbeba7c982dd51dfdcf6baf0a164827337cf56fd85f733e143b8d6116b8cd53c59c812cacef193dfa0b101a830fc455e32bf225e8505e7b2a554
Tidy a few debug log strings to avoid double spaces, concatenated status words, and mismatched format specifiers.
Co-authored-by: naiyoma <lankas.aurelia@gmail.com>
The function signature for the `send` RPC is:
```
send [{"address":amount,...},{"data":"hex"},...] ( conf_target "estimate_mode" fee_rate options version )
```
The last example in the manpage is missing the `fee_rate` arg, but is trying to specify the `options` arg, by index.
The parser confuses the intended `options` arg as the missing `fee_rate` arg.
See:
```
$ bitcoin-cli -rpcuser=doggman -rpcpassword=donkey -rpcport=18554 -regtest send '{"bcrt1qusm48zmlzwr32csxdw4ar7atw260h22c9ten9l": 0.1}' 1 economical '{"add_to_wallet": false, "inputs": [{"txid":"0b7e1a471dc948b7a6187936b16e6d7d9833629b2f9dd8a392eb89928f63aaad", "vout":0}]}'
error code: -8
error message:
Cannot specify both conf_target and fee_rate. Please provide either a confirmation target in blocks for automatic fee estimation, or an explicit fee rate.
```
vs
```
$ bitcoin-cli -rpcuser=doggman -rpcpassword=donkey -rpcport=18554 -regtest send '{"bcrt1qusm48zmlzwr32csxdw4ar7atw260h22c9ten9l": 0.1}' 1 economical null '{"add_to_wallet": false, "inputs": [{"txid":"0b7e1a471dc948b7a6187936b16e6d7d9833629b2f9dd8a392eb89928f63aaad", "vout":0}]}'
{
"psbt": "cHNidP8BAHECAAAAAa2qY4+SieuSo9idL5tiM5h9bW6xNnkYprdIyR1HGn4LAAAAAAD9////AkR2DwQAAAAAFgAUpLDwJu+wFRHLQAgKAb0psk7UVd2AlpgAAAAAABYAFOQ3U4t/E4cVYgZrq9H7q3K0+6lYAAAAAAABAIUCAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/////wQC4wMA/////wLIF6gEAAAAABYAFLMY1zihXrefAA0DA5nld4MCPjkrAAAAAAAAAAAmaiSqIant4vYcP3HR3v0/qZnfo2lTdVxpBol5mWK0i+vYNpdOjPkAAAAAAQEfyBeoBAAAAAAWABSzGNc4oV63nwANAwOZ5XeDAj45KwEIawJHMEQCIElTV4pbUrsPR9qHWcioowVv3QVWHizxwevfD0u/I8YyAiBCY3OzF81PSLM00h4ueQkehYuxDFZu7Jk51iejphKnnwEhA0VKdYVSyBpWoxBwTDOupB58Fi3mEBs+u+OOqEYVd2sZACICA98YLWyH7dBCfXVxe7woiLSTgV1mJN8Zc8KgZ77pVSg+GNBMeT5UAACAAQAAgAAAAIABAAAAbAAAAAAA",
"txid": "625b71b314a6ac4f738634e29dc007cd5edc0427c1ae96ab706d06a62910cea2",
"hex": "02000000000101adaa638f9289eb92a3d89d2f9b6233987d6d6eb1367918a6b748c91d471a7e0b0000000000fdffffff0244760f0400000000160014a4b0f026efb01511cb40080a01bd29b24ed455dd8096980000000000160014e437538b7f13871562066babd1fbab72b4fba9580247304402204953578a5b52bb0f47da8759c8a8a3056fdd05561e2cf1c1ebdf0f4bbf23c6320220426373b317cd4f48b334d21e2e79091e858bb10c566eec9939d627a3a612a79f012103454a758552c81a56a310704c33aea41e7c162de6101b3ebbe38ea84615776b1900000000",
"complete": true
}
```
- 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.
4c0d4f6f93 refactor: interfaces, make 'createTransaction' less error-prone (furszy)
e2c3ec9bf4 refactor: move CreatedTransactionResult to types.h (furszy)
45372175c3 gui: remove AmountWithFeeExceedsBalance error special case (furszy)
Pull request description:
Bundle all function's outputs inside the `util::Result` returned object.
Removals:
- The input-output 'change_pos' ref arg from `createTransaction`, which has been a source of bugs in the past.
- The 'fee' ref arg from `createTransaction`, which is currently only set when the transaction creation process succeeds.
- The no longer needed `AmountWithFeeExceedsBalance` error (more info about its re-introduction at [bitcoin#25269](https://github.com/bitcoin/bitcoin/pull/25269) and [bitcoin#34299](https://github.com/bitcoin/bitcoin/pull/34299).
Additionally, this PR moves the `CreatedTransactionResult` struct into its own file. This change is made to avoid further expanding the GUI dependencies on `wallet.h`. Structurally, the GUI should only access the model/interfaces and never the wallet directly.
ACKs for top commit:
stratospher:
ACK 4c0d4f6.
hebasto:
ACK 4c0d4f6f93.
Tree-SHA512: 4fc61f08ca2e66e46001defb3a2e852265713e75006c98f0c465bd48afe42e7b0d626d28d578741906fdd26e907d6919f06dc640c55c44efc3dfa766fdbf38a4
fa6801366d refactor: [rpc] Remove confusing and brittle integral casts (take 2) (MarcoFalke)
Pull request description:
Second take for cases which I seem to have missed in commit 94ddc2dced.
When constructing an UniValue from integral values, historically (long ago), in some cases casts where needed. With the current UniValue constructor, only very few are actually needed.
Keeping the unused casts around is:
* confusing, because code readers do not understand why they are needed
* brittle, because some may copy them into new places, where they will lead to hard-to-find logic bugs, such as the ones fixed in pull https://github.com/bitcoin/bitcoin/pull/34112
So fix all issues by removing them, except for a few cases, where casting was required:
* `static_cast<bool>(fCoinBase)`, because `bool{fCoinBase}` only works on modern compilers.
This hardening refactor does not fix any bugs and does not change any behavior.
ACKs for top commit:
Sjors:
ACK fa6801366d
sedited:
ACK fa6801366d
Tree-SHA512: 77f03f496ea2a42987720cb4a36eb4e7a0d5b512ed7f029e41dd54c04fb4c1680f7cc13514acbc9f1bae991be4de3cf31c5a0d27c016a030f5749d132603f71e
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
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.
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
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
54d0393058 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 54d0393058
brunoerg:
ACK 54d0393058
Tree-SHA512: 96b6a822f53311d9a76abe8c217794e0a2dd5bd713db0a15dc70e065099b8245c430e1174e24133e0a802218ff0f2943dfcc3d638c3716485d5607c452854e7d
As highlighted in the PR review comment, this enum seems no longer
required as the specific issue it solves for involves BDB based wallets
that can't be loaded anymore outside the context of wallet migration,
which rewrites the database anyway.
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.
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
b189a34557 test: add case where `TOTAL_TRIES` is exceeded yet solution remains (yancy)
Pull request description:
Show that `CoinGrider` halts searching when the number of attempts exceeds `TOTAL_TRIES`. To do so, show that a solution is found, then add one more entry to the same set of inputs. Since the search orders by `effective_value`, the solution is constructed such that only values with the lowest `effective_value` have the least weight. Only the lowest weight values will not exceed the `max_selection_weight`. Therefore, `CoinGrinder` will not evaluate all lowest weight solutions together before exceeding `TOTAL_TRIES` since they are last found.
This test case was inspired by a similar test for `BnB` currently named `bnb_test`.
ACKs for top commit:
frankomosh:
Code review ACK b189a34
achow101:
ACK b189a34557
murchandamus:
ACK b189a34557
Tree-SHA512: 1df0b6e29ae219edbeed14cfa97f0ad4688d6bf97ed946719ba3c3b69e004f3dee82991578eb5aceb554914b70c5b68feff9e321283c1fc8bc0fedf08df2cb4c
