6ee05c4b18 test: wallet: BnB incomplete result on attempt-limit success (Bruno Garcia)
Pull request description:
BnB can return a valid selection before exhausting the search tree, then hit `TOTAL_TRIES` while continuing to look for a better one. Add a unit test for that path using a known exhaustion fixture plus an exact-match coin, and assert the result is marked incomplete via `GetAlgoCompleted() == false`.
It kills the following mutant:
```diff
diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp
index 8d69957c30..e4d07415f3 100644
--- a/src/wallet/coinselection.cpp
+++ b/src/wallet/coinselection.cpp
@@ -212,7 +212,7 @@ util::Result<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool
if (curr_try >= TOTAL_TRIES) {
// Solution is not guaranteed to be optimal if `curr_try` hit TOTAL_TRIES
- result.SetAlgoCompleted(false);
+ result.SetAlgoCompleted(true);
break;
}
```
ACKs for top commit:
yashbhutwala:
tACK 6ee05c4b18
achow101:
ACK 6ee05c4b18
murchandamus:
ACK 6ee05c4b18
Tree-SHA512: 32d0cc6da7586abdd195b87459da0a6ebbdcf8115bef41f5e625864bfa9ce8231a58f8d4b61708279ebeaef372e4f3b5fb18eab28a01dad53f043ba083330b3d
BnB can return a valid selection before exhausting
the search tree, then hit TOTAL_TRIES while continuing
to look for a better one. Add a unit test for that path
using a known exhaustion fixture plus an exact-match
coin, and assert the result is marked incomplete via
GetAlgoCompleted() == false.
Co-authored-by: Murch <murch@murch.one>
fab8eeed82 fuzz: clang-format LIMITED_WHILE (MarcoFalke)
fa0d777ce2 fuzz: Clang-format LIMITED_WHILE like while (MarcoFalke)
fa1a9bde5a fuzz: Remove unused workaround after fix in libmultiprocess byte-span serializer (MarcoFalke)
fa55385ab3 fuzz: Use LIMITED_WHILE over for-loop with consumed size integral (MarcoFalke)
6d5f753921 Squashed 'src/ipc/libmultiprocess/' changes from 28e056576a..e8de5c7b68 (MarcoFalke)
Pull request description:
Includes several changes, to first update the subtree. Then, modify the fuzz test to address review comments:
* https://github.com/bitcoin/bitcoin/pull/35118#discussion_r3506566815
* https://github.com/bitcoin/bitcoin/pull/35118#discussion_r3523175145
ACKs for top commit:
ryanofsky:
Code review ACK fab8eeed82. Just fuzz test clang-format cleanups added since last review, which seem nice
Tree-SHA512: 0836628f8ee54adf02571025456211a74f63d05058b72280b10111ecfbb93d30945f4a48f30cc790774de4e2b489907313e86b0fa6729f3480c64850c94848b4
This is a whitespace-only clang-format change.
To verify it, one can run:
```sh
(git show | git apply --reverse ) && ( git diff -U0 | ./contrib/devtools/clang-format-diff.py -p1 -i -v ) && git diff HEAD
```
A few minor, non-macro formatting adjustments were made in touched files:
* `src/wallet/test/fuzz/crypter.cpp`: Removed a redundant double semicolon
* `src/test/fuzz/txorphan.cpp`: Corrected indentation on an `else if` block.
* `src/test/fuzz/mini_miner.cpp`: Removed an unnecessary empty line.
fed3cf6f0e wallet: Replace CWalletTx's vOrderForm with specific fields (Ava Chow)
4f8823e8e1 wallet: Drop vOrderForm from CommitTransaction (Ava Chow)
a2b0bfcd85 wallet: Drop mapValue from CWalletTx (Ava Chow)
cb99864c91 wallet: Throw if unknown entry is found in mapValue (Ava Chow)
98d5cdae66 wallet: Make CWalletTx "replaces_txid" and "replaced_by_txid" member variables (Ava Chow)
7ef8a6efc2 wallet: Make CWalletTx "comment" and "to" member variables (Ava Chow)
2155e913d3 wallet: Make CWalletTx "from" and "message" member variables (Ava Chow)
c6ba98dcc8 wallet: Drop mapValue from CommitTransaction (Ava Chow)
00abb174a8 wallet: Pass comment and comment_to to CommitTransaction (Ava Chow)
1a219a37a2 wallet: Pass replaces_txid to CommitTransaction outside of mapValue (Ava Chow)
Pull request description:
`mapValue` and `vOrderForm` are opaque data structures that contain transaction metadata. It is hard to determine what actual data each field contains, and they can ostensibly be misused where metadata is added in the future without developers realizing that such metadata exists.
It's much clearer to have all of that metadata live in their own explicit member variables within `CWalletTx`. This PR implements that change.
Since the serialization format of `CWalletTx` depends on `mapValue` and `vOrderForm`, the serialization remains unchanged, so when serializing these new members, they need to be shoved/extracted from a temporary `mapValue` or `vOrderForm`.
This does end up breaking forwards compatibility as unknown fields in `mapValue` and `vOrderForm` are stripped out if the record is rewritten. However, I don't expect that we would continue to use these fields for future metadata, so I think that risk is low.
ACKs for top commit:
ajtowns:
reACK fed3cf6f0e
w0xlt:
ACK fed3cf6f0e with above nits/caveats.
Eunovo:
ACK fed3cf6f0e
Tree-SHA512: c7deab5aaeac13656012f8b13c0161fd420d2a5348eebd7649310e78ccb1216995aa6a7cbd506ac8d11d7b46b0856d6e6a897bc39965b51cfcf2268356ace261
777d23f25c test: add regression test for in-memory SQLiteDatabase reopen (Pablo Martin)
d1e7f8c986 wallet: use in-memory SQLite for temporary wallet in exportwatchonlywallet (Pablo Martin)
ee43743f12 wallet: store m_additional_flags in SQLiteDatabase to fix reopen path (Pablo Martin)
Pull request description:
Since #33032 landed (in-memory `SQLiteDatabase` via `SQLITE_OPEN_MEMORY`), the intermediate wallet built during `exportwatchonlywallet` can live entirely in memory instead of being written to the wallets directory as a temporary file.
The temp wallet is a pure build artifact: it is populated with descriptors, transactions, and address book data, then immediately discarded once `BackupWallet()` copies its contents to the destination file. Making it in-memory removes all on-disk footprint and eliminates the `cleanup_watchonly_wallet` RAII handler — along with the `wallet_path` and `cleanup_files` variables it needed — which previously ensured the temp files were deleted on both success and failure paths.
This PR introduces `InMemoryWalletDatabase` (a minimal `SQLiteDatabase` subclass) and `MakeInMemoryWalletDatabase()` factory in `sqlite.h/cpp`, following the same pattern as `MockableSQLiteDatabase` / `CreateMockableWalletDatabase()`. `MockableSQLiteDatabase` now derives from `InMemoryWalletDatabase`, removing its redundant `Files()` override.
Suggested by Sjors in #32489 ([comment](https://github.com/bitcoin/bitcoin/pull/32489#issuecomment-4874894955)).
---
Also fixes a related issue found (by Sjors) during review:
- `SQLiteDatabase::Open()` (the no-arg public override) hardcoded 0 as `additional_flags` when reopening after a failed `TxnAbort()`, which would reopen an in-memory database as on-disk. Fixed by storing `m_additional_flags` in the constructor and using it in the reopen path. For in-memory databases, both the `force_conn_refresh` path and the public `Open()` now throw instead of silently creating a fresh empty connection. A regression test for the `Open()` throw is included in a separate commit.
---
As a follow-up, `InMemoryWalletDatabase` could replace `MockableSQLiteDatabase` in `src/bench/` (5 files, 6 call sites), since benchmarks don't need mock-specific behaviour and benefit from using the same in-memory path as production code.
ACKs for top commit:
Sjors:
re-utACK 777d23f25c
achow101:
ACK 777d23f25c
janb84:
ACK 777d23f25c
Tree-SHA512: 71178ce99c7ebc0fc5ba17956c27d37f90e3d36cefbdc0d15d1424b7a70bf15f05a13cb03c268d885678aba1fd3c90567d8389d3680650c8ae46f4cb4b10b28a
InMemoryWalletDatabase::Open() now throws to prevent silently returning
a fresh empty connection after close, which would discard all data. Add a
test to pin this behaviour.
Co-authored-by: Jan B <608446+janb84@users.noreply.github.com>
The intermediate watchonly wallet created during exportwatchonlywallet is
a pure build artifact — it is always discarded once BackupWallet() copies
it to the destination. Creating it as an in-memory SQLiteDatabase
(SQLITE_OPEN_MEMORY) removes the need to write files to the wallets
directory and eliminates the cleanup handler that deleted those files on
both success and failure paths.
Introduces InMemoryWalletDatabase (a minimal SQLiteDatabase subclass) and
MakeInMemoryWalletDatabase() factory in sqlite.h/cpp, following the same
pattern as MockableSQLiteDatabase / CreateMockableWalletDatabase() in the
test utilities. MockableSQLiteDatabase now derives from InMemoryWalletDatabase,
removing its redundant Files() override.
The wallet is named after the source wallet ("<name>_watchonly_temp") so
concurrent exports of different wallets use distinct names and log lines
remain traceable to the source wallet.
b2de59d486 wallet, bdbro: Validate btree page levels (Ava Chow)
dc3a2b9c3b wallet, bdbro: Enforce overflow data lengths (Ava Chow)
Pull request description:
Alternative to #34946
BDB's overflow records include the total length of the data to be read from the overflow pages. If this length is impossible (larger than max page * page size), or if the data that we are reading exceeds the stated length, then throw an exception as this is an invalid BDB file. This prevents infinite looping if an overflow page makes a circular reference.
BDB BTrees also include the level in the tree that the page is supposed to be at. Leaf pages are always at level 1. Starting from the root page, we can validate that each child has a level one less than the parent, until we reach a leaf page with a level of 1. This also ensures that we cannot have circular internal page references.
ACKs for top commit:
sedited:
ACK b2de59d486
rkrux:
code review ACK b2de59d486
Tree-SHA512: b740c349f68c78150ee57b77e8e81605bb45a7db4fbca3c3a0d87318178dfa197f37faf0cbfd8bef791ab20f4a9afc3d2571842960055a70b6c081d605f59f94
This parameter is only used to pass in the "Messages" from the GUI.
Instead of making it opaque by putting those into vOrderForm, use a
specific dedicated parameter for providing the messages.
2818a171c0 test: add abortscan unit test (Pol Espinasa)
bc30e95163 wallet: move fAbortRescan reset into WalletRescanReserver reserve() (Pol Espinasa)
Pull request description:
Follow-up of https://github.com/bitcoin/bitcoin/pull/35179
For extra context refer to the conversations https://github.com/bitcoin/bitcoin/pull/35179#discussion_r3378136210 and comments bellow it.
Long story short: currently `ScanForWalletTransactions()` resets the value of `fAbortRescan` before starting the rescan loop. This can cause a race condition where some function (e.g. `importdescriptors`) starts a rescan and at the same time the user aborts it manually. Could happen that the `abortrescan` call returns True (success) but the rescan continues running as the value is overwritten.
This PR fixes this by resetting the value of `fAbortRescan` at the very beginning, when the wallet rescan is reserved, removing the race condition. Also adds a test for it.
ACKs for top commit:
achow101:
ACK 2818a171c0
w0xlt:
ACK 2818a171c0
nebula-21:
ACK 2818a171c0
pablomartin4btc:
ACK 2818a17
pinheadmz:
ACK 2818a171c0
Tree-SHA512: 5b64b9a16a209dd145ccf7b2d7c3a7205038b3b214b02c3d9f1cbda75a6a1c9a41ad5c42cbeda43af6141793965613c47e44300d758149be9bfdf458e85a5520
de92208c2b migrate: Handle HD chains that have identical seeds but different IDs (Ava Chow)
Pull request description:
The seed ID is calculated from a pubkey produced by treating the seed as a private key. This calculation includes a pubkey compression parameter, even thought that compression is completely irrelevant for the usage of the seed as a BIP 32 seed. Thus migration should detect if a seed has been used multiple times by checking if the computed master key was already processed.
The spkm_migration fuzzer needs to have it's added descriptors accounting updated for this fix.
It should not be possible for users to actually run into this problem as all HD chains use seeds with the pubkey compression option set.
Fixes#35434
ACKs for top commit:
kevkevinpal:
crACK [de92208](de92208c2b)
marcofleon:
crACK de92208c2b
rkrux:
code review ACK de92208
Tree-SHA512: c420a24722fd6a94bf6656f195bad3432ba54c38b3c49a02750577281d0864988fd6d44cd9594b57cfaf33061a1e250e21378e3637b4e9a45f2d7aad6045884d
The context is easier to reason about: E.g.,
* in TestBasicMining it allows to drop manual SetMockTime(0) calls,
* in connections_desirable_service_flags it allows to drop manual calls
to SetMockTime(GetTime<std::chrono::seconds>() + _n_) and replace them
by operator+=(_n_)
* in wallet_tests it clarifies that the mocktime does not persist
outside the AddTx function
35a814a045 test: Limit clocks to one active instance (MarcoFalke)
55e402ffef scripted-diff: Rename NodeClockContext to FakeNodeClock (seduless)
1e9546fcf4 test: Use NodeClockContext in more call sites (seduless)
758fea59a8 test: Drop ++ from NodeClockContext default constructor (seduless)
7c2ec3949a test: Enter mocktime before peer creation in block_relay_only_eviction (seduless)
Pull request description:
Follow-up to #34858
Updates remaining `SetMockTime` call sites that are clean, mechanical swaps fitting the spirit of the original PR (see: https://github.com/bitcoin/bitcoin/pull/34858#pullrequestreview-4031647119 and https://github.com/bitcoin/bitcoin/pull/34858#issuecomment-4221757881). Further updates to `SetMockTime` are more complex and deserve separate, isolated PRs.
The default constructor for `NodeClockContext` increments to the next tick, which is a defensive measure to prevent time going backwards on construction. This has caused some confusion (see thread: https://github.com/bitcoin/bitcoin/pull/34858#discussion_r3057648646) and can be safely removed after updating the only test where this is load-bearing (b3c9bd7f2df230525c8e339394a315a2c500055d) (see: https://github.com/bitcoin/bitcoin/pull/34858#discussion_r3091085328). The removal also tightens the `addrman_tests/addrman_evictionworks` test to sit exactly on the `ADDRMAN_REPLACEMENT` boundary (4h), catching mutations such as:
```diff
diff --git a/src/addrman.cpp b/src/addrman.cpp
index d3dae59ae7..d0929c62cb 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -920,3 +920,3 @@ void AddrManImpl::ResolveCollisions_()
// Has successfully connected in last X hours
- if (current_time - info_old.m_last_success < ADDRMAN_REPLACEMENT) {
+ if (current_time - info_old.m_last_success <= ADDRMAN_REPLACEMENT) {
erase_collision = true;
```
The last follow-up item is updating `NodeClockContext` to `FakeNodeClock` to make it clear it is intended for testing (motivated by https://github.com/bitcoin/bitcoin/pull/34858#pullrequestreview-4082110904 and supported in https://github.com/bitcoin/bitcoin/pull/34858#issuecomment-4214352770).
ACKs for top commit:
maflcko:
re-ACK 35a814a045🛒
sedited:
ACK 35a814a045
Tree-SHA512: ade776e288a4b7bbc4c8855c14d61381b5b20329fe1e72fee87f773e47a9519975d58c277fbacda37dd73c0c1d4ce358c92dcdc4ca049d58cb3453ddf751b45b
d0b76c7f3e rpc+bitcoin-tx: Specify correct type for ParseFixedPoint() (Hodlinator)
43ca54ca00 refactor(test): Make CAmount arg explicit for BuildCreditingTransaction() (Hodlinator)
b5e91e946c wallet: Remove CoinsResult::Clear() (Hodlinator)
Pull request description:
The *knapsack_solver_test* in *coinselector_tests.cpp* was accumulating satoshi amounts beyond 21M BTC. This was uncovered while experimenting with adding checks to `CAmount`. Fix that by fully resetting the `CoinsResult` object accumulating those amounts, inspired by https://github.com/bitcoin/bitcoin/issues/35449#issuecomment-4613968627.
Also, while we're at it, add 2 commits which correct some `int64_t`/`CAmount` confusion.
Fixes https://github.com/bitcoin/bitcoin/issues/35449
ACKs for top commit:
sedited:
ACK d0b76c7f3e
furszy:
utACK d0b76c7f3e
brunoerg:
code review ACK d0b76c7f3e
Tree-SHA512: 6d989ded6f6327dc657f437dc256d4adf42a34a1252621421ee38d7851c6cdc97a462f033a4728e3aa7d5514deee4db6e83646105633f9cf7ed6e7e90406b67d
The previous name did not indicate the type was intended for
testing. Renaming to FakeNodeClock makes this explicit and
allows call sites to drop the ctx suffix on the variable name.
Suggested in #34858 review feedback.
-BEGIN VERIFY SCRIPT-
s() { git grep -l "$1" -- src | xargs sed -i "s/$1/$2/g"; }
s '\<NodeClockContext\>' 'FakeNodeClock'
s '\<clock_ctx\>' 'clock'
-END VERIFY SCRIPT-
7249b376a0 opt: Skip UTXOs with worse waste, same eff_value (Murch)
5204291860 opt: Skip evaluation of equivalent input sets (Murch)
ba1807b981 coinselection: Track effective_value lookahead (Murch)
fa226ab902 coinselection: BnB skip exploring high waste (Murch)
7ecea1dc5d coinselection: Track whether BnB completed (Murch)
3ca0f36164 coinselection: rewrite BnB in CoinGrinder-style (Murch)
2e73739837 coinselection: Track BnB iteration count in result (Murch)
Pull request description:
This PR rewrites the implementation of the BnB coinselection algorithm
to skip the duplicate evaluation of previously visited input selections.
In the original implementation of BnB, the state of the search is
backtracked by explicitly walking back to the omission branch and then
testing again. This retests an equivalent candidate set as before, e.g.,
after backtracking from {ABC}, it would evaluate {AB_}, before trying
{AB_D}, but {AB_} is equivalent to {AB} which was tested before.
CoinGrinder tracks the state of the search instead by remembering which
UTXO was last added and explicitly shifting from that UTXO directly to
the next, so after {ABC}, it will immediately move on to {AB_D}. We
replicate this approach here.
As fewer nodes are visited, this approach will enumerate more possible
combinations than the original implementation given the same limit for
iterations.
ACKs for top commit:
achow101:
ACK 7249b376a0
w0xlt:
reACK 7249b376a0
Tree-SHA512: fd5851ceea3a3a4699fc062254fa5438daa4275b4d52325983e63670040cf0ba35112be9e63813d8f30b38993c031f3df343b2152eb8c068d272fbff72d1881a
The seed ID is calculated from a pubkey produced by treating the seed as
a private key. This calculation includes a pubkey compression parameter,
even thought that compression is completely irrelevant for the usage of
the seed as a BIP 32 seed. Thus migration should detect if a seed has
been used multiple times by checking if the computed master key was
already processed.
The spkm_migration fuzzer needs to have it's added descriptors
accounting to be updated for this fix.
451fdd26a4 test: wallet: Constructing a DSPKM that can't TopUp() throws. (David Gumberg)
32946e0291 wallet: Setup new autogenerated descriptors on construction (Ava Chow)
e20aaff70f wallet: Construct ExternalSignerSPKM with the new descriptor (Ava Chow)
aa4f7823aa wallet: include keys when constructing DescriptorSPKM during import (Ava Chow)
6538f69135 fuzz: Skip adding descriptor to wallet if it cannot be expanded (Ava Chow)
8be5ee554b test: wallet: Check that loading wallet with both unencrypted and encrypted keys fails. (David Gumberg)
80b0c25992 wallet: Load everything into DescSPKM on construction (Ava Chow)
f713fd1725 refactor: wallet: Don't reuse WALLET_BLANK flag for born-encrypted wallets. (David Gumberg)
cd912c4e10 wallet: Consolidate generation setup callers into one function (Ava Chow)
0301c758ea wallet migration, fuzz: Migrate hd seed once (Ava Chow)
Pull request description:
Instead of constructing ScriptPubKeyMans with no data, and then loading data as we find it, we should gather everything first and then load it all on construction. If there actually is no data and we want to setup generation, then that should also occur in a constructor rather than afterwards.
This change is only applied to DescriptorScriptPubKeyMan and ExternalSignerScriptPubKeyMan, and should be done for any ScriptPubKeyMans added in the future. I don't think it's really worth it to do this for LegacyScriptPubKeyMan since it would make loading performance worse (or cause layer violations) and it's (supposed to be) going away soon.
ACKs for top commit:
polespinasa:
ACK 451fdd26a4
davidgumberg:
re crACK 451fdd26a4
w0xlt:
ACK 451fdd26a4
Tree-SHA512: 58a889bf7c77d5da78041907a76a1958207f95a19bec8dc4d86d4e4108d256a729e0949c0973f7d447178f78a7fd4268cda71d358cae4dec5a76dc453b5283af
eff9e798b9 coinselection: Tiebreak SRD eviction by weight (Murch)
Pull request description:
yancyribbens [pointed out](https://github.com/p2pderivatives/rust-bitcoin-coin-selection/pull/108#issuecomment-3202107069) that SRD would fail to find a possible solution if there are multiple UTXOs of the same effective value with diverse weight.
This adds a tiebreaker that will make SRD succeed in such a scenario.
ACKs for top commit:
yancyribbens:
utACK eff9e798b9
Tree-SHA512: 6a26f3c06f3346cef7e06926d9f747ecea91f057435f22fa8c3333f129227cf5a361f45047003e8a762a99225e3df27e3980a8b4397b36dc9b59c44a9626a2ec
cd8d3bd937 wallet: use outpoint when estimating input size (Lőrinc)
Pull request description:
### Problem
`CalculateMaximumSignedInputSize()` is passed the outpoint being sized, but a previous refactor stopped using that context when estimating the signed input size.
This could make externally selected inputs look slightly smaller than they really are.
### Fix
Pass the outpoint through again when estimating the signed input size.
Add a regression test for the external-input case.
> [!NOTE]
> the branch name still reflects the previous state of this PR, where the unused parameter was removed instead of wired back in
ACKs for top commit:
achow101:
ACK cd8d3bd937
pablomartin4btc:
ACK cd8d3bd937
Tree-SHA512: 6089ae65ae12677c32be0556d704f8c179f1ff5a017690846ae495644890526f85d8c0d75d4ec4c3c9ac5b519251169009484623340b8bc3a87fa9a3be27fefd
a39cc16b43 doc: Release note for addhdkey (Ava Chow)
89b9a01b4e wallet, rpc: Disallow importing unused() to wallets without privkeys (Ava Chow)
35bbee6374 wallet, rpc: Disallow import of unused() if key already exists (Ava Chow)
f3f8bcbd1d wallet: Add addhdkey RPC (Ava Chow)
82bc280de4 test: Simple test for importing unused(KEY) (Ava Chow)
80c29bc6f1 descriptor: Add unused(KEY) descriptor (Ava Chow)
Pull request description:
It is sometimes useful for the wallet to have keys that it can sign with but are not (initially) involved in any scripts, e.g. for setting up a multisig. Ryanofsky [suggested](https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1867721948) A `unused(KEY)` descriptor which allows for a key to be specified, but produces no scripts. These can be imported into the wallet, and subsequently retrieved with `gethdkeys`. Additionally, `listdescriptors` will output these descriptors so that they can be easily backed up.
In order to make it easier for people to add HD keys to their wallet, and to generate a new one if they want to rotate their descriptors, an `addhdkey` RPC is also added. Without arguments, it will generate a new HD key and add it to the wallet via a `unused(KEY)` descriptor. If provided a private key, it will construct the descriptor and add it to the wallet.
See also: https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1866961865
Based on #29130 as `gethdkeys` is useful for testing this.
ACKs for top commit:
Sjors:
utACK a39cc16
rkrux:
lgtm ACK a39cc16b43
Tree-SHA512: c1288c792ab01ca2eaddd24b0e7d11c259cd59e79042465d0d1eb656fd559c1200dc19750b4d84acc762b5b599935a06df214c18226e662087842ea91ec3011b
dc84a31014 wallet: remove fUpdate argument from AddToWalletIfInvolvingMe (rkrux)
94845df073 wallet: remove update_tx argument from SyncTransaction (rkrux)
6e796e1f47 wallet: remove fUpdate argument from ScanForWalletTransactions (rkrux)
54e4c0be8f wallet: remove update argument from RescanFromTime method (rkrux)
Pull request description:
This caught my attention while going through #32993.
The corresponding boolean arguments from CWallet methods related to
updating transactions during the blockchain scanning have been removed
because effectively these arguments were always passed as true making
the need for the boolean arguments unnecessary. The only falsy call sites
were in the unit tests that don't need to test scenarios that never happen
in actuality.
ACKs for top commit:
Bicaru20:
ACK dc84a31014. I also think that makes sense to remove the update argument if it is not used.
achow101:
ACK dc84a31014
sedited:
ACK dc84a31014
l0rinc:
untested code review ACK dc84a31014
Tree-SHA512: dfee793c88cc94d4a5b554e29710133ec4ad5e49455413245c7d7004ef71b618b0b2f821fa085415c34d80aeccd182c98e55223d84afbe8b408f64acac0571bb
`CalculateMaximumSignedInputSize()` is passed the outpoint being sized, but that context was not used when estimating the signed input size.
Pass the outpoint through so externally selected inputs are not underestimated.
Co-authored-by: Antoine Poinsot <darosior@protonmail.com>
9fa4076b20 test: Test merging implicit PSBTv0 with explicit PSBTv0 (w0xlt)
1660c18232 doc: Release notes for psbtv2 (Ava Chow)
470e52a5f8 fuzz: Enforce additional version invariants in PSBT fuzzer (Antoine Poinsot)
5bd0579c09 test: Tests for PSBT AddInput and AddOutput (Ava Chow)
b8b6e7f0c2 tests: Add PSBT unit test for ComputeTimeLock (Ava Chow)
0bc1c2e508 tests: Add test vectors from BIP 370 (Ava Chow)
e0e4dbdeb5 psbt: Change default psbt version to 2 (Ava Chow)
bcc1dca77b Add psbt_version to PSBT RPCs and default to v2 (Ava Chow)
ab38c30195 Implement PSBTv2 field merging (Ava Chow)
93e339e29f Implement PSBTv2 AddInput and AddOutput (Ava Chow)
b39c86ae60 Allow specifying PSBT version in constructor (Ava Chow)
dcc9a3c8df Implement PSBTv2 in decodepsbt (Ava Chow)
5770dbd39f Add PSBT::ComputeLockTime() (Ava Chow)
863cf47b33 Update test_framework/psbt.py for PSBTv2 (Ava Chow)
925161eaf0 Implement PSBTv2 fields de/ser (Ava Chow)
d9cf658ee0 Restrict joinpsbts to PSBTv0 only (Ava Chow)
3da0e16012 Replace PSBT.tx with PSBT::GetUnsignedTx and PSBT::GetUniqueID (Ava Chow)
c568624ff2 psbt: Return std::optional from PrecomputePSBTData (Ava Chow)
092de4f1f6 Replace PSBT::GetInputUTXO with PSBTInput::GetUTXO (Ava Chow)
82c9fe3179 psbt: Use PSBTInput and PSBTOutput fields instead of accessing global tx (Ava Chow)
95897507e9 psbt: AddInput and AddOutput should take only PSBTInput and PSBTOutput (Ava Chow)
1b7d323a72 Add PSBTInput::GetOutPoint (Ava Chow)
543d3e1cdc psbt: add PSBTv2 global tx fields (Ava Chow)
c01c7f068c psbt: Remove default constructor (Ava Chow)
9671aa08c2 psbt: add tx input and output fields in PSBTInput and PSBTOutput (Ava Chow)
990b084f11 Have PSBTInput and PSBTOutput know the PSBT's version (Ava Chow)
7eacc21ff6 psbt: make PSBT structs into classes (Ava Chow)
f926c326bb gui: Store PSBT in std::optional in PSBTOperationsDialog (Ava Chow)
1e2d146b47 psbt: Refactor duplicate key lookup and size checks (Ava Chow)
88384180d3 test: PSBTs should roundtrip through RPCs that do nothing (Ava Chow)
001877500d test: construct psbt with unknown field programmatically (David Gumberg)
0cb884e6df psbt: Fill hash preimages and taproot builder from SignatureData (Ava Chow)
Pull request description:
BIP 370 PSBTv2 introduces several new fields and different invariants for PSBT. This PR implements those new fields and restructures the PSBT implementation to match PSBTv2 but still remain compatible with PSBTv0.
ACKs for top commit:
nervana21:
re-ACK 9fa4076b20
theStack:
re-ACK 9fa4076b20
w0xlt:
ACK 9fa4076b20
Tree-SHA512: ab0a5ada4fa5fca27ba9ec9c291a44b30e69d6db11971957572d86c58c71c4caa4557dc25f403e1170ba4fac751306d074cc582defefc6e2fdd37be51c3d9dd0
1d1ae6f0c4 wallet, test: Remove DuplicateMockDatabase (Ava Chow)
57820c472b bench: Utilize setup() for WalletLoading and use a real database (Ava Chow)
9a7604fd25 bench: Use setup() in WalletMigration to prepare the legacy wallet (Ava Chow)
426a94e7bd bench: Utilize setup() in WalletEncrypt to create the encryption wallet (Ava Chow)
d672455d20 bench: Utilitze setup() in WalletBalance for marking caches dirty (Ava Chow)
61412ef887 bench: Utilize setup() in WalletCreate to cleanup previous wallets (Ava Chow)
Pull request description:
Several of the wallet benchmarks have some setup or cleanup that needs to be done per run. Now that #34208 is merged, these can use `setup()`. Additionally, this allows for removing `DuplicateMockDatabase` in `WalletEncryptDescriptors`.
This PR also removes `DuplicateMockDatabase` in `WalletLoadingDescriptors`. `DuplicateMockDatabase` was added here in #24924 as part of benchmark performance improvements. However, it does not appear to make a significant difference today.
Removing `DuplicateMockDatabase` makes future database changes easier. In particular it should simplify #33032 and #33034, and any future changes that introduce sqlite features.
ACKs for top commit:
l0rinc:
code review ACK 1d1ae6f0c4
furszy:
Other than that, ACK 1d1ae6f0c4
sedited:
ACK 1d1ae6f0c4
Tree-SHA512: 41130144972b759b401f990820eaf524d1f17f47d81bf1afea4a529d15a21d253521838a9e31df8f424996582b718a92634ab255204c6fce703b7e47a1d23670
2104282ddd fuzz: Add tests for CCoinControl methods (Chandra Pratap)
43b09b993d fuzz: Improve oracle for existing CCoinControl tests (Chandra Pratap)
Pull request description:
The `ccoincontrol` fuzzer misses tests for a number of `CCoinControl` operations. Add them.
While at it, improve the oracle for the existing tests.
ACKs for top commit:
l0rinc:
Lightly tested code review ACK 2104282ddd
brunoerg:
reACK 2104282ddd
sedited:
Re-ACK 2104282ddd
Tree-SHA512: bfc8c9a51fca94437332056c476840d841a5b42dd6749cb34105b7ae78215ec9c3eb0f407e1a5f51b3ac20d7abb97cae7c21ad2146d5be9409edbc2cd2c568ee
The global unsigned tx is decomposed into separate fields inside of
PSBT, which mirrors what PSBTv2 will do. However, we still need to get
the global unsigned tx so PSBT::GetUnsignedTx is introduced to do that.
In order to also have a stable unique ID, we also introduce
PSBT::GetUniqueID to replace uses of PSBT.tx.GetHash().
DuplicateMockDatabase is no longer used. Furthermore, as SQLite gets
used more as a database and less as a key value store, this function
gets more complicated and more bug prone. As the benchmarks now run
equivalently quickly with a real database, retaining this duplication
function is no longer necessary.
PSBTInput now has the previous txid and output index, and PSBTOutput has
the amount and script. We no longer need to access the global unsigned
tx for these fields.
Additionally, we can change iterating tx.vin and tx.vout to psbtx.inputs
and psbtx.outputs.
This is in prepration for use with PSBTv2 where the global unsigned tx
will not exist.
Instead of allowing PSBTs to be default constructor, force usage of the
deserialization constructor.
CombinePSBTs, DecodeBase64PSBT, and DecodeRawPSBT are all changed to
return std::optional or util::result rather than using an output
parameter to avoid the need for a default constructor.
With the split between LoadWallet and CreateNew, it's no longer
necessary to utilize the blank flag to prevent the wallet from having
descriptors automatically being generated. Instead, CreateNew can take a
separate parameter to indicate whether the wallet is to be born
encrypted and therefore should not have any keys generated.
If a wallet has multiple HD chains that have the same seed, we should
only migrate that seed a single time.
This fixes a fuzz crash that occurs once the return value of
AddDescriptorKeyWithDB is checked during descriptor construction.
Replace the sign, finalize , bip32derivs and sighash_type arguments which
are passed to FillPSBT() and SignPSBTInput() with a PSBTFillOptions struct.
This makes it easier to add additional options later without large code
churn, such as avoid_script_path proposed in #32857. It also makes the
use of default boolean options safer compared to positional arguments
that can easily get mixed up.
Only the unit test code passes false, the actual code passes
true always. I don't see a reason why the tests need to exercise
a behaviour that never happens in production.
037ea2c714 walletdb: Remove m_mock from SQLiteDatabase (Ava Chow)
59484e2fdb wallet: Make Mockable{Database,Batch} subclasses of SQLite classes (Ava Chow)
b69f989dc5 wallet, bench: Use TestingSetup in CoinSelection benchmark (Ava Chow)
e7d67c9fd9 test: Make duplicating MockableDatabases use cursor and batch (Ava Chow)
964eafb71c bench, wallet: Make WalletMigration's setup WalletBatch scoped (Ava Chow)
Pull request description:
`MockableDatabase` was introduced for the tests to avoid tying non-database tests to a particular database type. However, since the only database type now is sqlite, and because the mockable behavior is no longer used by the tests, we can replace usage of the `MockabeDatabase` with a SQLite database that lives only in memory.
This is particularly useful for future work that has the wallet make use of SQLite's capabilities more, which are less conducive to having a separate mock database implementation.
ACKs for top commit:
brunoerg:
code review ACK 037ea2c714
sedited:
Re-ACK 037ea2c714
furszy:
Code review ACK 037ea2c714
Tree-SHA512: 0a99c27ef4e590966b3af929bf3acf99666861905aabf150fe5660ea07c881a49935a4e7dcd676dcd5e70616898d89d872b6e156ae9c600de1361c1b2469b64d
BTree pages contain the level in the BTree that the page is supposed to
be at. The root starts at some level between 1 and 255, leaves are
always level 1. Internal pages must be a level that is one less than its
parent. Validating that pages are at their expected level (except for
the root page) enforces that no cycles can occur.
When two successive UTXOs differ in waste but match in effective value,
we can skip the second if the first is not selected, because all input
sets we can generate by swapping out a less wasteful UTXOs with a more
wastefull UTXO of matching effective value would be strictly worse.
Also expand documentation of Branch and Bound.
When two successive UTXOs match in effective value and weight, we can
skip the second if the prior is not selected: adding it would create an
equivalent input set to a previously evaluated.
E.g. if we have three UTXOs with effective values {5, 3, 3} of the same
weight each, we want to evaluate
{5, _, _}, {5, 3, _}, {5, 3, 3}, {_, 3, _}, {_, 3, 3},
but skip {5, _, 3}, and {_, _, 3}, because the first 3 is not selected,
and we therefore do not need to evaluate the second 3 at the same
position in the input set.
If we reach the end of the branch, we must SHIFT the previously selected
UTXO group instead.
Introduces a dedicated data structure to track the total
effective_value available in the remaining UTXOs at each index of the
UTXO pool. In contrast to the original approach in BnB, this allows us
to immediately jump to a lower index instead of visiting every UTXO to
add back their eff_value to the lookahead.
At high feerates adding more inputs will increase the waste score. If
the current waste is already higher than the best selection’s we cannot
improve upon the best selection. All solutions that include the current
selection with more additional inputs must be worse than the best
selection so far: SHIFT
This optimization only works at high feerates, because at low feerates,
adding more inputs decreases waste, so this condition would exit
prematurely. We would never attempt input sets with higher weight than
the prior best selection, even though we would prefer those at low
feerates.
