7362f8e5e2 refactor: make CoinsResult total amounts members private (furszy)
3282fad599 wallet: add assert to SelectionResult::Merge for safety (S3RK)
c4e3b7d6a1 wallet: SelectCoins, return early if wallet's UTXOs cannot cover the target (furszy)
cac2725fd0 test: bugfix, coinselector_test, use 'CoinsResult::Erase/Add' instead of direct member access (furszy)
cf79384697 test: Coin Selection, duplicated preset inputs selection (furszy)
341ba7ffd8 test: wallet, coverage for CoinsResult::Erase function (furszy)
f930aefff9 wallet: bugfix, 'CoinsResult::Erase' is erasing only one output of the set (furszy)
Pull request description:
This comes with #26559.
Solving few bugs inside the wallet's transaction creation
process and adding test coverage for them.
Plus, making use of the `CoinsResult::total_amount` cached value
inside the Coin Selection process to return early if we don't have
enough funds to cover the target amount.
### Bugs
1) The `CoinsResult::Erase` method removes only one
output from the available coins vector (there is a [loop break](c1061be14a/src/wallet/spend.cpp (L112))
that should have never been there) and not all the preset inputs.
Which on master is not a problem, because since [#25685](https://github.com/bitcoin/bitcoin/pull/25685)
we are no longer using the method. But, it's a bug on v24
(check [#26559](https://github.com/bitcoin/bitcoin/pull/26559)).
This method it's being fixed and not removed because I'm later using it to solve
another bug inside this PR.
2) As we update the total cached amount of the `CoinsResult` object inside
`AvailableCoins` and we don't use such function inside the coin selection
tests (we manually load up the `CoinsResult` object), there is a discrepancy
between the outputs that we add/erase and the total amount cached value.
### Improvements
* This makes use of the `CoinsResult` total amount field to early return
with an "Insufficient funds" error inside Coin Selection if the tx target
amount is greater than the sum of all the wallet available coins plus the
preset inputs amounts (we don't need to perform the entire coin selection
process if we already know that there aren't enough funds inside our wallet).
### Test Coverage
1) Adds test coverage for the duplicated preset input selection bug that we have in v24.
Where the wallet invalidly selects the preset inputs twice during the Coin Selection
process. Which ends up with a "good" Coin Selection result that does not cover the
total tx target amount. Which, alone, crashes the wallet due an insane fee.
But.. to make it worst, adding the subtract fee from output functionality
to this mix ends up with the wallet by-passing the "insane" fee assertion,
decreasing the output amount to fulfill the insane fee, and.. sadly,
broadcasting the tx to the network.
2) Adds test coverage for the `CoinsResult::Erase` method.
------------------------------------
TO DO:
* [ ] Update [#26559 ](https://github.com/bitcoin/bitcoin/pull/26559) description.
ACKs for top commit:
achow101:
ACK 7362f8e5e2
glozow:
ACK 7362f8e5e2, I assume there will be a followup PR to add coin selection sanity checks and we can discuss the best way to do that there.
josibake:
ACK [7362f8e](7362f8e5e2)
Tree-SHA512: 37a6828ea10d8d36c8d5873ceede7c8bef72ae4c34bef21721fa9dad83ad6dba93711c3170a26ab6e05bdbc267bb17433da08ccb83b82956d05fb16090328cba
Aside from the cleanup, this solves a bug in the following-up commit. Because, in these
tests, we are manually adding/erasing outputs from the CoinsResult object but never
updating the internal total amount field.
This exercises the bug inside CoinsResult::Erase that
ends up on (1) a wallet crash or (2) a created and
broadcasted tx that contains a reduced recipient's amount.
This is covered by making the wallet selects the preset
inputs twice during the coin selection process.
Making the wallet think that the selection process result covers
the entire tx target when it does not. It's actually creating
a tx that sends more coins than what inputs are covering for.
Which, combined with the SFFO option, makes the wallet
incorrectly reduce the recipient's amount by the difference
between the original target and the wrongly counted inputs.
Which means, a created and relayed tx sending less coins to
the destination than what the user inputted.
1b77db2653 test: add `ismine` test for descriptor scriptpubkeyman (w0xlt)
Pull request description:
Currently `src/wallet/test/ismine_tests.cpp` has tests for the legacy ScriptPubKeyMan only.
This PR adds tests for the descriptor ScriptPubKeyMan.
ACKs for top commit:
ishaanam:
ACK 1b77db2653
achow101:
ACK 1b77db2653
furszy:
ACK 1b77db26 with a non-blocking comment.
Tree-SHA512: 977b5d1e71f9468331aeb4ebaf3708dd651f9f3018d4544a395b87ca6d7fb8bfa6d20acc1a4f6e096e240e81d30fb7a6e8add190e52536e7a3cb5a80f392883f
Adds test coverage for the wallet's crypted key loading from db process.
The following scenarios are covered:
1) "All ckeys checksums valid" test:
Loads an encrypted wallet with all the crypted keys with a valid checksum and
verifies that 'CWallet::Unlock' doesn't force an entire crypted keys re-write.
(we force a complete ckeys re-write if we find any missing crypted key checksum
during the wallet loading process)
2) "Missing checksum in one ckey" test:
Verifies that loading up a wallet with, at least one, 'ckey' with no checksum
triggers a complete re-write of the crypted keys.
3) "Invalid ckey checksum error" test:
Verifies that loading up a ckey with an invalid checksum stops the wallet loading
process with a corruption error.
4) "Invalid ckey pubkey error" test:
Verifies that loading up a ckey with an invalid pubkey stops the wallet loading
process with a corruption error.
files share the same purpose, and we shouldn't have wallet code
inside the test directory.
This later is needed to use wallet util functions in the bench
and test binaries without be forced to duplicate them.
so if there is an error in any of the pre-set coins, we can fail right away
without computing the wallet available coins set (calling `AvailableCoins`)
which is a slow operation as it goes through the entire wallet's txes map.
----------------------
And to make the Coin Selection flow cleared, have decoupled SelectCoins in two functions:
1) AutomaticCoinSelection.
2) SelectCoins.
1) AutomaticCoinSelection:
Receives a set of coins and selects the best subset of them to
cover the target amount.
2) SelectCoins
In charge of select all the user manually selected coins first ("pre-set inputs"), and
if coin_control 'm_allow_other_inputs=true', call 'AutomaticCoinSelection' to select a
subset of coins owned by the wallet to cover for the target - preset_inputs.total_amount
remaining value.
d0d9cf7aea test: Check external coin effective value is used in CoinSelection (Aurèle Oulès)
76b79c1a17 wallet: Use correct effective value when checking target (Aurèle Oulès)
Pull request description:
Fixes #26185. The following assert failed because it was not checked in the parent function.
2bd9aa5a44/src/wallet/coinselection.cpp (L391)
ACKs for top commit:
glozow:
reACK d0d9cf7aea
furszy:
ACK d0d9cf7a
Tree-SHA512: e126daba1115e9d143f2a582c6953e7ea55e96853b6e819c7744fd7a23668f7d9854681d43ef55d8774655bc54e7e87c1c9fccd746d9e30fbf3caa82ef808ae9
and also fix spelling in test/lint/lint-locale-dependence.py not caught by the
spelling linter and fix up a paragraph we are touching here in test/README.md.
53e7ed075c doc: Release notes and other docs for migration (Andrew Chow)
9c44bfe244 Test migratewallet (Andrew Chow)
0b26e7cdf2 descriptors: addr() and raw() should return false for ToPrivateString (Andrew Chow)
31764c3f87 Add migratewallet RPC (Andrew Chow)
0bf7b38bff Implement MigrateLegacyToDescriptor (Andrew Chow)
e7b16f925a Implement MigrateToSQLite (Andrew Chow)
5b62f095e7 wallet: Refactor SetupDescSPKMs to take CExtKey (Andrew Chow)
22401f17e0 Implement LegacyScriptPubKeyMan::DeleteRecords (Andrew Chow)
35f428fae6 Implement LegacyScriptPubKeyMan::MigrateToDescriptor (Andrew Chow)
ea1ab390e4 scriptpubkeyman: Implement GetScriptPubKeys in Legacy (Andrew Chow)
e664af2976 Apply label to all scriptPubKeys of imported combo() (Andrew Chow)
Pull request description:
This PR adds a new `migratewallet` RPC which migrates a legacy wallet to a descriptor wallet. Migrated wallets will need a new backup. If a wallet has watchonly stuff in it, a new watchonly descriptor wallet will be created containing those watchonly things. The related transactions, labels, and descriptors for those watchonly things will be removed from the original wallet. Migrated wallets will not have any of the legacy things be available for fetching from `getnewaddress` or `getrawchangeaddress`. Wallets that have private keys enabled will have newly generated descriptors. Wallets with private keys disabled will not have any active `ScriptPubKeyMan`s.
For the basic HD wallet case of just generated keys, in addition to the standard descriptor wallet descriptors using the master key derived from the pre-existing hd seed, the migration will also create 3 descriptors for each HD chain in: a ranged combo external, a ranged combo internal, and a single key combo for the seed (the seed is a valid key that we can receive coins at!). The migrated wallet will then have newly generated descriptors as the active `ScriptPubKeyMan`s. This is equivalent to creating a new descriptor wallet and importing the 3 descriptors for each HD chain. For wallets containing non-HD keys, each key will have its own combo descriptor.
There are also tests.
ACKs for top commit:
Sjors:
tACK 53e7ed075c
w0xlt:
reACK 53e7ed075c
Tree-SHA512: c0c003694ca2e17064922d08e8464278d314e970efb7df874b4fe04ec5d124c7206409ca701c65c099d17779ab2136ae63f1da2a9dba39b45f6d62cf93b5c60a
4fef534428 wallet: use GetChange() when computing waste (S3RK)
87e0ef9031 wallet: use GetChange() in tx building (S3RK)
15e97a6886 wallet: add SelectionResult::GetChange (S3RK)
72cad28da0 wallet: calculate and store min_viable_change (S3RK)
e3210a7225 wallet: account for preselected inputs in target (S3RK)
f8e796348b wallet: add SelectionResult::Merge (S3RK)
06f558e4e2 wallet: accurate SelectionResult::m_target (S3RK)
c8cf08ea74 wallet: ensure m_min_change_target always covers change fee (S3RK)
Pull request description:
Benefits:
1. more accurate waste calculation for knapsack. Waste calculation is now consistent with tx building code. Before we always assumed change for knapsack even when the solution is changeless4.
2. simpler tx building code. Only create change output when it's needed
3. makes it easier to correctly account for fees for CPFP inputs (should be done in a follow up)
In the first three commits we fix the code to accurately track selection target in `SelectionResult::m_target`
Then we introduce new variable `min_change` that represents the minimum viable change amount
Then we introduce `SelectionResult::GetChange()` which incapsulates dropping change for fee logic and uses correct values of `SelectionResult::m_target`
Then we use `SelectionResult::GetChange()` in both tx building and waste calculation code
This PR is a refactoring and shouldn't change the behaviour.
There is only one known small change (arguably a bug fix). Before we dropped change output if it's smaller than `cost_of_change` after paying change fees. This is incorrect as `cost_of_change` already includes `change_fee`.
ACKs for top commit:
achow101:
ACK 4fef534428
Xekyo:
crACK 4fef534428
furszy:
Code review ACK 4fef5344
w0xlt:
ACK 4fef534428
Tree-SHA512: 31a7455d4129bc39a444da0f16ad478d690d4d9627b2b8fdb5605facc6488171926bf02f5d7d9a545b2b59efafcf5bb3d404005e4da15c7b44b3f7d441afb941
8cd21bb279 refactor: improve readability for AttemptSelection (josibake)
f47ff71761 test: only run test for descriptor wallets (josibake)
0760ce0b9e test: add missing BOOST_ASSERT (josibake)
db09aec937 wallet: switch to new shuffle, erase, push_back (josibake)
b6b50b0f2b scripted-diff: Uppercase function names (josibake)
3f27a2adce refactor: add new helper methods (josibake)
f5649db9d5 refactor: add UNKNOWN OutputType (josibake)
Pull request description:
This PR is to address follow-ups for #24584, specifically:
* Remove redundant, hard-to-read code by adding a new `OutputType` and adding shuffle, erase, and push_back methods for `CoinsResult`
* Add missing `BOOST_ASSERT` to unit test
* Ensure functional test only runs if using descriptor wallets
* Improve readability of `AttemptSelection` by removing triple-nested if statement
Note for reviewers: commit `refactor: add new helper methods` should throw an "unused function warning"; the function is used in the next commit. Also, commit `wallet: switch to new shuffle, erase, push_back` will fail to compile, but this is fixed in the next commit with a scripted-diff. the commits are separate like this (code change then scripted-diff) to improve legibility.
ACKs for top commit:
achow101:
ACK 8cd21bb279
aureleoules:
ACK 8cd21bb279.
LarryRuane:
Concept, code review ACK 8cd21bb279
furszy:
utACK 8cd21bb2. Left a small, non-blocking, comment.
Tree-SHA512: a1bbc5962833e3df4f01a4895d8bd748cc4c608c3f296fd94e8afd8797b8d2e94e7bd44d598bd76fa5c9f5536864f396fcd097348fa0bb190a49a86b0917d60e
This is a refactor, putting the burden to think about thread safety to
the caller. Otherwise, there is a risk that the caller will assume
thread safety where none exists, as is evident in the previous two
commits.
switch to new methods, remove old code. this also
updates the Size, All, and Clear methods to now use
the coins map.
this commit is not strictly a refactor because previously
coin selection was never run over the UNKNOWN type until the last
step when being run over all. now that we are iterating over each,
it is run over UNKNOWN but this is expected to be empty most of the time.
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
bc886fcb31 Change mapWallet to be a std::unordered_map (Andrew Chow)
272356024d Change getWalletTxs to return a set instead of a vector (Andrew Chow)
97532867cf Change mapTxSpends to be a std::unordered_multimap (Andrew Chow)
1f798fe85b wallet: Cache SigningProviders (Andrew Chow)
8a105ecd1a wallet: Use CalculateMaximumSignedInputSize to indicate solvability (Andrew Chow)
Pull request description:
While running my coin selection simulations, I noticed that towards the end of the simulation, the wallet would become slow to make new transactions. The wallet generally performs much more slowly when there are a large number of transactions and/or a large number of keys. The improvements here are focused on wallets with a large number of transactions as that is what the simulations produce.
Most of the slowdown I observed was due to `DescriptorScriptPubKeyMan::GetSigningProvider` re-deriving keys every time it is called. To avoid this, it will now cache the `SigningProvider` produced so that repeatedly fetching the `SigningProvider` for the same script will not result in the same key being derived over and over. This has a side effect of making the function non-const, which makes a lot of other functions non-const as well. This helps with wallets with lots of address reuse (as my coin selection simulations are), but not if addresses are not reused as keys will end up needing to be derived the first time `GetSigningProvider` is called for a script.
The `GetSigningProvider` problem was also exacerbated by unnecessarily fetching a `SigningProvider` for the same script multiple times. A `SigningProvider` is retrieved to be used inside of `IsSolvable`. A few lines later, we use `GetTxSpendSize` which fetches a `SigningProvider` and then calls `CalculateMaximumSignedInputSize`. We can avoid a second call to `GetSigningProvider` by using `CalculateMaximumSignedInputSize` directly with the `SigningProvider` already retrieved for `IsSolvable`.
There is an additional slowdown where `ProduceSignature` with a dummy signer is called twice for each output. The first time is `IsSolvable` checks that `ProduceSignature` succeeds, thereby informing whether we have solving data. The second is `CalculateMaximumSignedInputSize` which returns -1 if `ProduceSignature` fails, and returns the input size otherwise. We can reduce this to one call of `ProduceSignature` by using `CalculateMaximumSignedInputSize`'s result to set `solvable`.
Lastly, a lot of time is spent looking in `mapWallet` and `mapTxSpends` to determine whether an output is already spent. The performance of these lookups is slightly improved by changing those maps to use `std::unordered_map` and `std::unordered_multimap` respectively.
ACKs for top commit:
Xekyo:
ACK bc886fcb31
furszy:
diff re-reACK bc886fcb
Tree-SHA512: fd710fe1224ef67d2bb83d6ac9e7428d9f76a67f14085915f9d80e1a492d2c51cb912edfcaad1db11c2edf8d2d97eb7ddd95bfb364587fb1f143490fd72c9ec1
Rename `BResult` class to `util::Result` and update the class interface to be
more compatible with `std::optional` and with a full-featured result class
implemented in https://github.com/bitcoin/bitcoin/pull/25665. Motivation for
this change is to update existing `BResult` usages now so they don't have to
change later when more features are added in #25665.
This change makes the following improvements originally implemented in #25665:
- More explicit API. Drops potentially misleading `BResult` constructor that
treats any bilingual string argument as an error. Adds `util::Error`
constructor so it is never ambiguous when a result is being assigned an error
or non-error value.
- Better type compatibility. Supports `util::Result<bilingual_str>` return
values to hold translated messages which are not errors.
- More standard and consistent API. `util::Result` supports most of the same
operators and methods as `std::optional`. `BResult` had a less familiar
interface with `HasRes`/`GetObj`/`ReleaseObj` methods. The Result/Res/Obj
naming was also not internally consistent.
- Better code organization. Puts `src/util/` code in the `util::` namespace so
naming reflects code organization and it is obvious where the class is coming
from. Drops "B" from name because it is undocumented what it stands for
(bilingual?)
- Has unit tests.
9e04cfaa76 test: add coverage for wallet inconsistent state during sync (furszy)
77de5c693f wallet: guard and alert about a wallet invalid state during chain sync (furszy)
Pull request description:
Follow-up work to my comment in #25239.
Guarding and alerting the user about a wallet invalid state during chain synchronization.
#### Explanation
if the `AddToWallet` tx write fails, the method returns a wtx `nullptr` without removing the recently added transaction from the wallet's map.
Which makes that `AddToWalletIfInvolvingMe` return false (even when the tx is on the wallet's map already), --> which makes `SyncTransaction` skip the `MarkInputsDirty` call --> which leads to a wallet invalid state where the inputs of this new transaction are not marked dirty, while the transaction that spends them still exist on the in-memory wallet tx map.
Plus, as we only store the arriving transaction inside `AddToWalletIfInvolvingMe` when we synchronize/scan block/s from the chain and nowhere else, it makes sense to treat the transaction db write error as a runtime error to notify the user about the problem. Otherwise, the user will lose all the not stored transactions after a wallet shutdown (without be able to recover them automatically on the next startup because the chain sync would be above the block where the txs arrived).
Note:
On purpose, the first commit adds test coverage for it. Showing how the wallet can end up in an invalid state. The second commit corrects it with the proposed solution.
ACKs for top commit:
achow101:
re-ACK 9e04cfaa76
jonatack:
ACK 9e04cfaa76
Tree-SHA512: 81f765eca40547d7764833d8ccfae686b67c7728c84271bc00dc51272de643dafc270014079dcc9727b47577ba67b340aeb5f981588b54e69a06abea6958aa96
71d1d13627 test: add unit test for AvailableCoins (josibake)
da03cb41a4 test: functional test for new coin selection logic (josibake)
438e04845b wallet: run coin selection by `OutputType` (josibake)
77b0707206 refactor: use CoinsResult struct in SelectCoins (josibake)
2e67291ca3 refactor: store by OutputType in CoinsResult (josibake)
Pull request description:
# Concept
Following https://github.com/bitcoin/bitcoin/pull/23789, Bitcoin Core wallet will now generate a change address that matches the payment address type. This improves privacy by not revealing which of the outputs is the change at the time of the transaction in scenarios where the input address types differ from the payment address type. However, information about the change can be leaked in a later transaction. This proposal attempts to address that concern.
## Leaking information in a later transaction
Consider the following scenario:
1. Alice has a wallet with bech32 type UTXOs and pays Bob, who gives her a P2SH address
2. Alice's wallet generates a P2SH change output, preserving her privacy in `txid: a`
3. Alice then pays Carol, who gives her a bech32 address
4. Alice's wallet combines the P2SH UTXO with a bech32 UTXO and `txid: b` has two bech32 outputs
From a chain analysis perspective, it is reasonable to infer that the P2SH input in `txid: b` was the change from `txid: a`. To avoid leaking information in this scenario, Alice's wallet should avoid picking the P2SH output and instead fund the transaction with only bech32 Outputs. If the payment to Carol can be funded with just the P2SH output, it should be preferred over the bech32 outputs as this will convert the P2SH UTXO to bech32 UTXOs via the payment and change outputs of the new transaction.
**TLDR;** Avoid mixing output types, spend non-default `OutputTypes` when it is economical to do so.
# Approach
`AvailableCoins` now populates a struct, which makes it easier to access coins by `OutputType`. Coin selection tries to find a funding solution by each output type and chooses the most economical by waste metric. If a solution can't be found without mixing, coin selection runs over the entire wallet, allowing mixing, which is the same as the current behavior.
I've also added a functional test (`test/functional/wallet_avoid_mixing_output_types.py`) and unit test (`src/wallet/test/availablecoins_tests.cpp`.
ACKs for top commit:
achow101:
re-ACK 71d1d13627
aureleoules:
ACK 71d1d13627.
Xekyo:
reACK 71d1d13627 via `git range-diff master 6530d19 71d1d13`
LarryRuane:
ACK 71d1d13627
Tree-SHA512: 2e0716efdae5adf5479446fabc731ae81d595131d3b8bade98b64ba323d0e0c6d964a67f8c14c89c428998bda47993fa924f3cfca1529e2bd49eaa4e31b7e426
47ea70fbb8 wallet: clean AllInputsMine code, use InputIsMine internally (furszy)
bf310b0e8c wallet: clean InputIsMine code, use GetWalletTx (furszy)
0cb177263c wallet: unify CachedTxGetImmatureCredit and CachedTxGetImmatureWatchOnlyCredit (furszy)
04c6423f7b wallet: remove always true 'fUseCache' arg from CachedTxGetAvailableCredit (furszy)
4f0ca9bff6 wallet: remove always false 'recalculate' arg from GetCachableAmount (furszy)
47b1012677 wallet: remove always true 'fUseCache' from CachedTxGetImmatureWatchOnlyCredit (furszy)
da8f62de2c wallet: remove always true 'fUseCache' from CachedTxGetImmatureCredit (furszy)
Pull request description:
Another wallet's code garbage collector work. Part of the `mapWallet` encapsulation goal.
Focused on the following points:
1) Remove always true `fUseCache` argument from `CachedTxGetImmatureCredit`, `CachedTxGetImmatureWatchOnly` and `CachedTxGetAvailableCredit`.
2) Remove always false `recalculate` argument from `GetCachableAmount`.
3) Merge `CachedTxGetImmatureCredit` and `CachedTxGetImmatureWatchOnlyCredit` as they do share the exact same code.
4) Clean `InputIsMine` method; use `GetWalletTx` instead of access the wallet's map directly.
5) Clean `AllInputsMine` method; use `InputIsMine` instead of duplicate the exact same code internally.
ACKs for top commit:
aureleoules:
re-ACK 47ea70fbb8
achow101:
ACK 47ea70fbb8
theStack:
re-ACK 47ea70fbb8
Tree-SHA512: e9b64b57de7be6165c5e5552e28cd8a03d4736b0a3707d29d129e3a0a3db6a855c2abf47a24917236060835a297b564a97b66d4c8b178d6bdafb93a12a7c0b40
Pass the whole CoinsResult struct to SelectCoins instead of only a
vector. This means we now have to remove preselected coins from each
OutputType vector and shuffle each vector individually.
Pass the whole CoinsResult struct to AttemptSelection. This involves
moving the logic in AttemptSelection to a newly named function,
ChooseSelectionResult. This will allow us to run ChooseSelectionResult
over each OutputType in a later commit. This ensures the backoffs work
properly.
Update unit and bench tests to use CoinResult.
Add new interfaces::BlockInfo struct to be able to pass extra block
information (file and undo information) to indexes which they are
updated to use high level interfaces::Chain notifications.
This commit does not change behavior in any way.
1be7964189 test, wallet: Add mempool rescan test for import RPCs (Fabian Jahr)
833ce76df7 rpc, wallet: Document mempool rescan after importdescriptor, importwallet (Fabian Jahr)
0e396d1ba7 rpc, wallet: Document mempool scan after importmulti (Fabian Jahr)
e6d3ef8586 rpc, wallet: Document mempool scan after importpubkey (Fabian Jahr)
6d3db52e66 rpc, wallet: Document and test mempool scan after importprivkey (João Barbosa)
3abdbbb90a rpc, wallet: Document and test mempool scan after importaddress (João Barbosa)
236239bd40 wallet: Rescan mempool for transactions as well (Fabian Jahr)
Pull request description:
This PR picks up the work from #18964 and closes #18954.
It should incorporate all the unaddressed feedback from the PR:
- Mempool rescan now expanded to all relevant import* RPCs
- Added documentation in the help of each RPC
- More tests
ACKs for top commit:
Sjors:
re-utACK 1be7964189 (only a test change)
achow101:
ACK 1be7964189
w0xlt:
reACK 1be7964189
Tree-SHA512: b62fed5f97c6c242b2af417b41c9696a1f18878483d9e1c9429791f9c05257f57a00540a9a84df23c49faf6a61c3109c22972de81540083f38b506217804fcc5
When a transaction arrives, the wallet mark its inputs (prev-txs) as dirty.
Clearing the wallet transaction cache, triggering a balance recalculation.
If this does not happen due a db write error during `AddToWallet`, the wallet
will be in an invalid state: The transaction that spends certain wallet UTXO will
exist inside the in-memory wallet tx map, having the credit/debit calculated,
while its inputs will still have the old cached data (like if them were never
spent).
111ea3ab71 wallet: refactor GetNewDestination, use BResult (furszy)
22351725bc send: refactor CreateTransaction flow to return a BResult<CTransactionRef> (furszy)
198fcca162 wallet: refactor, include 'FeeCalculation' inside 'CreatedTransactionResult' (furszy)
7a45c33d1f Introduce generic 'Result' class (furszy)
Pull request description:
Based on a common function signature pattern that we have all around the sources:
```cpp
bool doSomething(arg1, arg2, arg3, arg4, &result_obj, &error_string) {
// do something...
if (error) {
error_string = "something bad happened";
return false;
}
result = goodResult;
return true;
}
```
Introduced a generic class `BResult` that encapsulate the function boolean result, the result object (in case of having it) and, in case of failure, the string error reason.
Obtaining in this way cleaner function signatures and removing boilerplate code:
```cpp
BResult<Obj> doSomething(arg1, arg2, arg3, arg4) {
// do something...
if (error) return "something bad happened";
return goodResult;
}
```
Same cleanup applies equally to the function callers' side as well. There is no longer need to add the error string and the result object declarations before calling the function:
Before:
```cpp
Obj result_obj;
std::string error_string;
if (!doSomething(arg1, arg2, arg3, arg4, result_obj, error_string)) {
LogPrintf("Error: %s", error_string);
}
return result_obj;
```
Now:
```cpp
BResult<Obj> op_res = doSomething(arg1, arg2, arg3, arg4);
if (!op_res) {
LogPrintf("Error: %s", op_res.GetError());
}
return op_res.GetObjResult();
```
### Initial Implementation:
Have connected this new concept to two different flows for now:
1) The `CreateTransaction` flow. --> 7ba2b87c
2) The `GetNewDestination` flow. --> bcee0912
Happy note: even when introduced a new class into the sources, the amount of lines removed is almost equal to added ones :).
Extra note: this work is an extended version (and a decoupling) of the work that is inside #24845 (which does not contain the `GetNewDestination` changes nor the inclusion of the `FeeCalculation` field inside `CreatedTransactionResult`).
ACKs for top commit:
achow101:
ACK 111ea3ab71
w0xlt:
reACK 111ea3ab71
theStack:
re-ACK 111ea3ab71
MarcoFalke:
review ACK 111ea3ab71🎏
Tree-SHA512: 6d84d901a4cb923727067f25ff64542a40edd1ea84fdeac092312ac684c34e3688a52ac5eb012717d2b73f4cb742b9d78e458eb0e9cb9d6d72a916395be91f69
230a2f4cc3 wallet test: Add unit test for wallet scan save_progress option (Ryan Ofsky)
a89ddfbe22 wallet: Save wallet scan progress (w0xlt)
Pull request description:
Currently, the wallet scan progress is not saved.
If it is interrupted, it will be necessary to start from scratch on the next load.
This PR changes this and the progress is saved right after checking a block.
Close https://github.com/bitcoin/bitcoin/issues/25010
ACKs for top commit:
furszy:
re-ACK 230a2f4
achow101:
ACK 230a2f4cc3
ryanofsky:
Code review ACK 230a2f4cc3. Only change since last review is tweaking whitespace and adding log print
Tree-SHA512: 1a9dec207ed22b3443fb06a4daf967637bc02bcaf71c070b7dc33605d0cab959551e4014c9e92293a63f54c5cbcc98bb9f8844a8c60bc32a1482b1c4130fab32
98ea43d5e9 test: add tests for negative waste during coin selection (ishaanam)
Pull request description:
#25495 mentions that waste can be negative when the current feerate is less than the long term feerate. There are currently no waste tests for negative waste, so this PR adds two of them.
ACKs for top commit:
achow101:
ACK 98ea43d5e9
glozow:
light code review ACK 98ea43d5e9, good to have tests for negative waste
Tree-SHA512: d194d370f1257975959d3c601fea9f82c30c1aabc3e8bedc997c62659283fe681cc527e59df1a0187b3c91e8067c60374dd5ce0237561bd882edafe6a575a9b9
d54c5c8b1b wallet: use CCoinControl to estimate signature size (S3RK)
a94659c84e wallet: replace GetTxSpendSize with CalculateMaximumSignedInputSize (S3RK)
Pull request description:
Currently `DummySignTx` and `DummySignInput` use different ways to determine signature size.
This PR unifies the way wallet estimates signature size for various inputs.
Instead of passing boolean flags from calling code the `use_max_sig` is now calculated at the place of signature creation using information available in `CCoinControl`
ACKs for top commit:
achow101:
ACK d54c5c8b1b
theStack:
Code-review ACK d54c5c8b1b
Tree-SHA512: e790903ad4683067070aa7dbf7434a1bd142282a5bc425112e64d88d27559f1a2cd60c68d6022feaf6b845237035cb18ece10f6243d719ba28173b69bd99110a
