ffd11ea876 Fix typo and grammar (Heebs)
Pull request description:
Fix typo and grammar in the coin selection algorithm's description.
ACKs for top commit:
meshcollider:
ACK ffd11ea876
Tree-SHA512: bba07c2efd5140fb3e021618739d70aaa761bbc274afb8158809492b0606773c217e42e58e58b18a2454b9c45ebc883ebece17cdc467ac60e3d3140d7a979db7
f13a22a631 wallet: Call load handlers without cs_wallet locked (João Barbosa)
Pull request description:
Don't have `cs_wallet` locked while calling each `context.wallet_load_fns`. A load handler can always lock `cs_wallet` if needed.
The lock was added in 1c7e25db0c to satisfy TSAN. With 44c430ffac most of the code requiring the lock is in `CWallet::AttachChain`. A comment is added to warn about wallets_mutex and cs_wallet lock ordering.
ACKs for top commit:
meshcollider:
re-utACK f13a22a631
ryanofsky:
Code review ACK f13a22a631. Only change since last review is adding a lock order comment
jonatack:
ACK f13a22a631
Tree-SHA512: d51976c3aae4bebc2d1997c88edff712d21fc5523801f5614062a10f826e164579973aeb1981bb1cbc243ecff6af3250362f544c02a79e5d135cbbca1704be62
ee03c782ba wallet: Make GetOldestKeyPoolTime return nullopt for blank wallets (Hennadii Stepanov)
3e4f069d23 wallet, refactor: Make GetOldestKeyPoolTime return type std::optional (Hennadii Stepanov)
Pull request description:
The "keypoololdest" field in the `getwalletinfo` RPC response should be used for legacy wallets only.
Th current implementation (04437ee721) assumes that `CWallet::GetOldestKeyPoolTime()` always return `0` for descriptor wallets. This assumption is wrong for _blank_ descriptor wallets, when `m_spk_managers` is empty. As a result:
```
$ src/bitcoin-cli -signet -rpcwallet=211024-d-DPK getwalletinfo
{
"walletname": "211024-d-DPK",
"walletversion": 169900,
"format": "sqlite",
"balance": 0.00000000,
"unconfirmed_balance": 0.00000000,
"immature_balance": 0.00000000,
"txcount": 0,
"keypoololdest": 9223372036854775807,
"keypoolsize": 0,
"keypoolsize_hd_internal": 0,
"paytxfee": 0.00000000,
"private_keys_enabled": false,
"avoid_reuse": false,
"scanning": false,
"descriptors": true
}
```
This PR fixes this issue with direct checking of the `WALLET_FLAG_DESCRIPTORS` flag.
ACKs for top commit:
lsilva01:
re-ACK ee03c78
stratospher:
ACK ee03c78.
meshcollider:
Code review ACK ee03c782ba
Tree-SHA512: 9852f9f8ed5c08c07507274d7714f039bbfda66da6df65cf98f67bf11a600167d0f7f872680c95775399477f4df9ba9fce80ec0cbe0adb7f2bb33c3bd65b15df
80dc829be7 tests: Calculate fees more similarly to CFeeRate::GetFee (Andrew Chow)
ce2cc44afd tests: Test for assertion when feerate is rounded down (Andrew Chow)
0fbaef9676 fees: Always round up fee calculated from a feerate (Andrew Chow)
Pull request description:
When calculating the fee for a feerate, it is possible that the final calculation will have fractional satoshis. Currently those are ignored via truncation which results in the absolute fee being rounded down. Rounding down is problematic because it results in a feerate that is slightly lower than the feerate represented by the `CFeeRate` object. A slightly lower feerate particularly causes issues for coin selection as it can trigger an assertion error. To avoid potentially underpaying the feerate (and the assertion), always round up the calculated fee.
A test is added for the assertion, along with a comment explaining what happens.
It is unlikely that a user can trigger this as it requires a very specific set of rounding errors to occur as well as the transaction not needing any change and being right on the lower bound of the exact match window. However I was able to trigger the assertion while running coin selection simulations, albeit after thousands of transactions and with some weird feerates.
ACKs for top commit:
ryanofsky:
Code review ACK 80dc829be7
promag:
Tested ACK 80dc829be7.
lsilva01:
tACK 80dc829
meshcollider:
utACK 80dc829be7
Tree-SHA512: fe26684c60f236cab48ea6a4600c141ce766dbe59504ec77595dcbd7fd0b34559acc617007f4f499c9155d8fda0a336954413410ba862b19c765c0cfac79d642
b5950dd59c validation: put coins cache write log into bench debug log (Anthony Towns)
31b2b802b5 blockstorage: use debug log category (Anthony Towns)
da94ebc2fa validation: move header validation error logging to VALIDATION debug category (Anthony Towns)
1d7d835ec3 validation: include block hash when reporting prev block not found errors (Anthony Towns)
Pull request description:
Moves the following log messages into debug log categories:
* "AcceptBlockHeader: ..." to validation
* "Prune: deleted blk/rev" to new blockstorage log category
* "Leaving block file" moves from validation to blockstorage
* "write coins cache to disk" to bench
Also adds the hash of the block to the log message when AcceptBlockHeader is rejecting because of problems with the prev block.
ACKs for top commit:
practicalswift:
cr ACK b5950dd59c
Empact:
Code review ACK https://github.com/bitcoin/bitcoin/pull/23235/commits/b5950dd59ca3e144721a5f15568a65be43bd2f20
laanwj:
Code review ACK b5950dd59c
promag:
Code review ACK b5950dd59c.
meshcollider:
Code review ACK b5950dd59c
Tree-SHA512: a73fdbfe8d36da48a3e89c2d5e0b6a3c5045d280c1a57f61c38d0d21f4f198aece4bd85155be3439e179d5dabdb523bf15fa0395e0e3ceff19c878ba3112c840
fa43e7c2d9 bitcoin-tx: Avoid treating overflow as OP_0 (MarcoFalke)
fa053c0019 style: Fix whitespace in Parse* functions (MarcoFalke)
fa03dec7e9 refactor: Use C++11 range based for loop in ParseScript (MarcoFalke)
fad55e79ca doc: Fixup ToIntegral docs (MarcoFalke)
Pull request description:
Seems odd to treat integer overflow as `OP_0`, so fix that.
ACKs for top commit:
theStack:
re-ACK fa43e7c2d9
shaavan:
ACK fa43e7c2d9
Tree-SHA512: 1bbe2de62d853badc18d57d169c6e78ddcdff037e5a85357995dead11c8e67a4fe35087e08a181c60753f8ce91058b7fcc06f5b7901afedc78fbacea8bc3ef4f
928af61cdb allow send rpc take external inputs and solving data (Andrew Chow)
e39b5a5e7a Tests for funding with external inputs (Andrew Chow)
38f5642ccc allow fundtx rpcs to work with external inputs (Andrew Chow)
d5cfb864ae Allow Coin Selection be able to take external inputs (Andrew Chow)
a00eb388e8 Allow CInputCoin to also be constructed with COutPoint and CTxOut (Andrew Chow)
Pull request description:
Currently `fundrawtransaction` and `walletcreatefundedpsbt` both do not allow external inputs as the wallet does not have the information necessary to estimate their fees.
This PR adds an additional argument to both those RPCs which allows the user to specify solving data. This way, the wallet can use that solving data to estimate the size of those inputs. The solving data can be public keys, scripts, or descriptors.
ACKs for top commit:
prayank23:
reACK https://github.com/bitcoin/bitcoin/commit/928af61cdb2c4de1c3d10e6fda13bbba5ca0bba9
meshcollider:
Re-utACK 928af61cdb
instagibbs:
crACK 928af61cdb
yanmaani:
utACK 928af61.
Tree-SHA512: bc7a6ef8961a7f4971ea5985d75e2d6dc50c2a90b44c664a1c4b0f1be5c1c97823516358fdaab35771a4701dbefc0862127b1d0d4bfd02b4f20d2befa4434700
240ea294d5 doc: update doxygen documention of ComputeTimeSmart() and AddToWalletIfInvolvingMe() regarding rescanning_old_block parameter (BitcoinTsunami)
d6eb39af21 test: add functional test to check transaction time determination during block rescanning (BitcoinTsunami)
07b44f16e7 wallet: fix ComputeTimeSmart algorithm to use blocktime during old block rescanning (BitcoinTsunami)
Pull request description:
The function ComputeTimeSmart in wallet.cpp assume that transaction are discovered in the right order.
Moreover the 'smarttime' determination algorithm is coded with realtime scenario in mind and not rescanning of old block.
The functional test demonstrate that if the user import a wallet, then rescan only recent history, and then rescan the entire history, the older transaction discovered would have an incorrect time determination.
In the context of rescanning old block, the only time value that as a meaning is the blocktime.
That's why I've fixed the problem with a simple separation between rescanning of old block and realtime time determination. The fix is written to have no impact on every realtime scenario and only impact the behaviour during a rescanning process.
This PR Fixes#20181.
To be fair, I don't think that this bug could be triggered with the wallet GUI, because it always proceed with a proper rescan.
But RPC API provide the possibility to trigger it. I've discovered it, because Specter desktop v0.10.0 was impacted. (https://github.com/cryptoadvance/specter-desktop/issues/680).
ACKs for top commit:
jonatack:
ACK 240ea294d5 per `git diff b92d552 240ea29`, re-verified rebase to latest master + debug build clean + new test passes on the branch and fails on master, only change since my review a few hours ago is incorporation of latest review suggestions
meshcollider:
re-utACK 240ea294d5
Tree-SHA512: 514b02e41d011ddfa325f5e8080b93800e1ea4ed5853fa420670a6ac700e8b463000dbea65f8ced8565cfb950c7f51b69154034dcb111e67aca3b964a0061494
ea31caf6b4 update estimatesmartfee rpc to return max of estimateSmartFee, mempoolMinFee and minRelayTxFee. (pranabp-bit)
Pull request description:
This PR is in response to the issue [#19699](https://github.com/bitcoin/bitcoin/issues/19699).
Based on the discussion in the comments of PR [#22673](https://github.com/bitcoin/bitcoin/pull/22673) changes have been made in the `estimatesmartfee` itself such that it takes into account `mempoolMinFee` and `relayMinFee` . Hence it provides a fee estimate that is most likely to be paid by the user in an actual transaction, preventing issues such as [#16072](https://github.com/bitcoin/bitcoin/issues/16072).
The test file test/functional/feature_fee_estimation.py has also been updated to check this functionality.
ACKs for top commit:
meshcollider:
re-utACK ea31caf6b4
Tree-SHA512: 8f36153a07cbd552c5c13d11d9c6e987a7a555ea4cc83f2573c0c92dd97c706d90c30a7248671437c2f3a836d3272f8fad53d15a5fa6efaa0409ae8009b0a18d
43cd6b8af9 doc: add release notes for removal of the -deprecatedrpc=addresses flag (Michael Dietz)
2b1fdc2c6c refactor: minor styling, prefer snake case and same line if (Michael Dietz)
d64deac7b8 refactor: share logic between ScriptPubKeyToUniv and ScriptToUniv (Michael Dietz)
8721638daa rpc: remove deprecated addresses and reqSigs from rpc outputs (Michael Dietz)
Pull request description:
Resolves#21797 now that we've branched-off to v23 ("addresses" and "reqSigs" deprecated) "ExtractDestinations" should be removed.
`-deprecatedrpc=addresses` was initially added in this PR #20286 (which resolved the original issue #20102).
Some chunks of code and logic are no longer used/necessary with the removal of this, and therefore some minor refactoring is done in this PR as well (separated commits)
ACKs for top commit:
MarcoFalke:
re-ACK 43cd6b8af9🐉
meshcollider:
Code review ACK 43cd6b8af9
jonatack:
ACK 43cd6b8af9 per `git range-diff a9d0cec 92dc5e9 43cd6b8`, also rebased to latest master, debug built + quick re-review of each commit to bring back context, and ran tests locally at the final commit
Tree-SHA512: fba83495e396d3c06f0dcf49292f14f4aa6b68fa758f0503941fade1a6e7271cda8378e2734af1faea550d1b43c85a36c52ebcc9dec0732936f9233b4b97901c
efcaefc7b5 test: Add remaining scenarios of 0 waste (rajarshimaitra)
Pull request description:
As per the [review club](https://bitcoincore.reviews/22009) discussion on #22009 , it was observed that there were other two fee scenarios in which selection waste could be zero.
These are:
- (LTF - Fee) == Change Cost
- (LTF - Fee) == Excess
Even though these are obvious by the definition of waste metric, adding tests for them can be helpful in explaining its behavior
to new readers of the code base, along with pinning the behavior for future.
This PR adds those two cases to waste calculation unit test.
Also let me know if I am missing more scenarios.
ACKs for top commit:
jonatack:
Tested re-ACK efcaefc7b5
achow101:
ACK efcaefc7b5
meshcollider:
ACK efcaefc7b5
Tree-SHA512: 13fe3e2c0ea7bb58d34e16c32908b84705130dec16382ff941e5e60ca5b379f9c5811b33f36c4c72d7a98cfbb6af2f196d0a69e96989afa4b9e49893eaadd7cb
3633b667ff Use SelectCoinsSRD if it has less waste (Andrew Chow)
8bf789b4b4 Add SelectCoinsSRD function (Andrew Chow)
2ad3b5d2ad tests: wallet_basic lock needed unspents (Andrew Chow)
b77885f13e tests: wallet_txn explicilty specify inputs (Andrew Chow)
59ba7d2861 tests: rpc_fundrawtx better test for UTXO inclusion with include_unsafe (Andrew Chow)
a165bfbe44 tests: rpc_fundrawtx use specific inputs for unavailable change test (Andrew Chow)
df765a484d tests: rpc_fundrawtx lock to UTXO types (Andrew Chow)
Pull request description:
To ease in the use of SRD as our fallback mechanism, this PR adds it as a secondary fallback algorithm in addition to the knapsack solver. Since #22009, the solution with the least waste will be chosen. This pattern is continued with SRD simply being another solution whose waste is compared.
ACKs for top commit:
glozow:
reACK 3633b66 via `git range-diff 981b9d1...3633b66`, thanks for taking the suggestions
laanwj:
Concept and code review ACK 3633b667ff
Tree-SHA512: 895659f553fea2230990136565bdf18b1328de8b0ce47f06b64bb4d69301f6dd68cb38debe5c24fb6de1317b735fc020a987c541f00bbea65229de47e53adf92
b11a195ef4 refactor: Detach wallet transaction methods (followup for move-only) (Russell Yanofsky)
Pull request description:
This makes `CWallet` and `CWalletTx` methods in `spend.cpp` and `receive.cpp` files into standalone functions.
It's a followup to [#21207 MOVEONLY: CWallet transaction code out of wallet.cpp/.h](https://github.com/bitcoin/bitcoin/pull/21207), which moved code from `wallet.cpp` to new `spend.cpp` and `receive.cpp` files.
There are no changes in behavior. This is just making methods into functions and removing circular dependencies created by #21207. There are no comment or documentation changes, either. Removed comments from `transaction.h` are just migrated to `spend.h`, `receive.h`, and `wallet.h`.
---
This commit was split off from #21206 so there are a few earlier review comments there
ACKs for top commit:
achow101:
ACK b11a195ef4
Sjors:
utACK b11a195ef4
meshcollider:
light ACK b11a195ef4
Tree-SHA512: 75ce818d3f03b728b14b12e2d21bd20b7be73978601989cb37ff98254393300d1bb7823281449cd3d9e40756d67d42bd9a46bbdafd2e8baa95aaf2cb1c84549f
d9d3ec07cf Consolidate XOnlyPubKey lookup hack (Andrew Chow)
Pull request description:
The places where we need to lookup information for a XOnlyPubKey
currently implement a hack which makes both serializations of the full
pubkey in order to try the CKeyIDs for the lookup functions. Instead of
duplicating this everywhere it is needed, we can consolidate the CKeyID
generation into a function, and then have wrappers around GetPubKey,
GetKey, and GetKeyOrigin which takes the XOnlyPubKey, retrieves all of
the CKeyIDs (using the new GetKeyIDs() function in XOnlyPubKey), and
tries their respective underlying lookup function.
Split from #22364
ACKs for top commit:
S3RK:
Code Review reACK d9d3ec0
Zero-1729:
re-crACK d9d3ec0
theStack:
re-ACK d9d3ec07cf
meshcollider:
Code review + functional test run ACK d9d3ec07cf
Tree-SHA512: 21a7f6d37fad74483a38006f82b3558337fe9ed30e0b4392e6fff82c22251a42ac996b43f06cdaa9289ee34a768e181d87aa4208b5538e36ae4977954e1fa6a0
86beee0579 Use waste metric for deciding which selection to use (Andrew Chow)
b3df0caf7c tests: Test GetSelectionWaste (Andrew Chow)
4f5ad43b1e Add waste metric calculation function (Andrew Chow)
935b3ddf72 scripted-diff: tests: Use KnapsackSolver directly (Andrew Chow)
6a023a6f90 tests: Add KnapsackGroupOutputs helper function (Andrew Chow)
d5069fc1aa tests: Use SelectCoinsBnB directly instead of AttemptSelection (Andrew Chow)
54de7b4746 Allow the long term feerate to be configured, default of 10 sat/vb (Andrew Chow)
Pull request description:
Branch and Bound introduced a metric that we call waste. This metric is used as part of bounding the search tree, but it can be generalized to all coin selection solutions, including those with change. As such, this PR introduces the waste metric at a higher level so that we can run both of our coin selection algorithms (BnB and KnapsackSolver) and choose the one which has the least waste. In the event that both find a solution with the same change, we choose the one that spends more inputs.
Also this PR sets the long term feerate to 10 sat/vb rather than using the 1008 block estimate. This allows the long term feerate to be the feerate that we switch between consolidating and optimizing for fees. This also removes a bug where the long term feerate would incorrectly be set to the fallback fee. While this doesn't matter prior to this PR, it does have an effect following this. The long term feerate can be configured by the user through a new `-consolidatefeerate` option.
ACKs for top commit:
Xekyo:
reACK 86beee0 via git range-diff fe47558...86beee0
meshcollider:
re-utACK 86beee0579
Tree-SHA512: 54b154b346538eca68ae2a3b83a033b495c1605c14f842bfc43ded2256b110983ce674c647fe753cf0305b1b178403d8d60d6d4203c7a712bec784be52e90d42
8733a8e84c the result of CWallet::IsHDEnabled() was initialized with true. (Saibato)
Pull request description:
the result of CWallet::IsHDEnabled() was initialized with true.
But in case of no keys or a blank hd wallet the iterator would be skipped
and not set to false but true, since the loop would be not entered.
That had resulted in a wrong return and subsequent false HD and watch-only
icon display in GUi when reloading a wallet after closing.
ACKs for top commit:
achow101:
ACK 8733a8e84c
hebasto:
ACK 8733a8e84c
theStack:
utACK 8733a8e84c
meshcollider:
utACK 8733a8e84c
Tree-SHA512: 79b976594f7174d05c29fe3819037ead59aaef27498d95415ceba74d633a8e035f6b03b521000ac3370684a8cb09319d8be1a443ce2d29b3ff4089e399f6b719
5fe8100ff3 Change the wallet_backup.py test to use the restorewallet RPC command instead of restoring wallets manually. (lsilva01)
ae23faba6f Add a new RPC command: restorewallet (lsilva01)
Pull request description:
As far as I know, there is no command to restore the wallet from a backup file.
The only way to do this is to replace the `wallet.dat` of a newly created wallet with the backup file, which is hardly an intuitive way.
This PR implements the `restorewallet` RPC command which restores the wallet from the backup file.
To test:
First create a backup file:
`$ bitcoin-cli -rpcwallet="wallet-01" backupwallet /home/Backups/wallet-01.bak`
Then restore it in another wallet:
`$ bitcoin-cli restorewallet "restored-wallet-01" /home/Backups/wallet-01.bak`
ACKs for top commit:
achow101:
re-ACK 5fe8100ff3
prayank23:
tACK https://github.com/bitcoin/bitcoin/commit/5fe8100ff36fed6d50c2a25b028f57b25af3504c
meshcollider:
utACK 5fe8100ff3
Tree-SHA512: 9639df4d8ad32f255f5b868320dc69878bd9aceb3b471b49dfad500b67681e2d354292b5410982fbf18e25a44ed0c06fd4a0dd010e82807c2e00ff32e84047a1
92993aa5cf Change SignTransaction's input_errors to use bilingual_str (Andrew Chow)
171366e89b Use bilingual_str for address fetching functions (Andrew Chow)
9571c69b51 Add bilingual_str::clear() (Andrew Chow)
Pull request description:
In a couple of places in the wallet, errors are `std::string`. In order for these errors to be translated, change them to use `bilingual_str`.
ACKs for top commit:
hebasto:
re-ACK 92993aa5cf, only rebased since my [previous](https://github.com/bitcoin/bitcoin/pull/22337#pullrequestreview-694542729) review, verified with
klementtan:
Code review ACK 92993aa5cf
meshcollider:
Code review ACK 92993aa5cf
Tree-SHA512: 5400e419dd87db8c49b67ed0964de2d44b58010a566ca246f2f0760ed9ef6a9b6f6df7a6adcb211b315b74c727bfe8c7d07eb5690b5922fa5828ceef4c83461f
fa6fd3dd6a wallet: Properly set fInMempool in mempool notifications (MarcoFalke)
Pull request description:
A wallet method (like bumping the fee) might have set `fInMempool` to false because the transaction was removed from the mempool (See commit fa4e088cba).
Avoid setting it back to true (incorrectly) in the validation interface background thread.
Fixes#22357
ACKs for top commit:
ryanofsky:
Code review ACK fa6fd3dd6a. Only change since last review is extending workaround to `transactionRemovedFromMempool`. Since we know this workaround is imperfect and the goal of this PR is mainly to fix CI errors, I would probably be inclined to limit the workaround to as few places as possible where we have seen actual failures, instead of adding the workaround to as many places as possible, where there is some chance it might trigger new failures. But since this workaround is so straightforward and almost looks like a real fix, probably it doesn't matter.
meshcollider:
utACK fa6fd3dd6a
Tree-SHA512: d690136a577f1f532aa1fee80d3f6600ff7fc61286fbf564a53d7938d5ae52d33f0dbb0fef8b8c041a4970fb424f0b9f1ee7ce791e0ff8354e0000ecc9e22b84
bb822a7af8 wallet, rpc: add listdescriptors private option (S3RK)
Pull request description:
Rationale: make it possible to backup your wallet with `listdescriptors` command
* The default behaviour is still to show public version
* For private version only the root xprv is returned
Example use-case:
```
> bitcoin-cli -regtest -named createwallet wallet_name=old descriptors=true
> bitcoin-cli -regtest -rpcwallet=old listdescriptors true | jq '.descriptors' > descriptors.txt
> bitcoin-cli -regtest -named createwallet wallet_name=new descriptors=true blank=true
> bitcoin-cli -regtest -rpcwallet=new importdescriptors "$(cat descriptors.txt)"
```
In case of watch-only wallet without private keys there will be following output:
```
error code: -4
error message:
Can't get descriptor string.
```
ACKs for top commit:
achow101:
re-ACK bb822a7af8
Rspigler:
tACK bb822a7af8
jonatack:
ACK bb822a7af8 per `git diff 2854ddc bb822a7`
prayank23:
tACK https://github.com/bitcoin/bitcoin/pull/21500/commits/bb822a7af86897a9b6a5d616f193c258e8e76729
meshcollider:
Code review ACK bb822a7af8
Tree-SHA512: f6dddc72a74e5667071ccd77f8dce578382e8e29e7ed6a0834ac2e114a6d3918b59c2f194f4079b3259e13d9ba3b4f405619940c3ecb7a1a0344615aed47c43d
8465978f23 Make IsSegWitOutput return true for taproot outputs (Pieter Wuille)
Pull request description:
This fixes a bug: currently `utxoupdatepsbt` will not fill in UTXO data for PSBTs spending taproot outputs.
ACKs for top commit:
achow101:
Code Review ACK 8465978f23
jonatack:
ACK 8465978f23
meshcollider:
utACK 8465978f23
Tree-SHA512: 2f8f873450bef4b5a4ce5962a231297b386c6b1445e69ce5f36ab28eca7343be3a11bc09c38534b0f75e6f99ba15d78d3ba5d484f6c63e5a9775e1f3f55a74e0
5012a7912e Test that descriptor wallet upgrade does nothing (Andrew Chow)
48bd7d3b77 Change ScriptPubKeyMan::Upgrade to default to return true (Andrew Chow)
Pull request description:
When adding a new ScriptPubKeyMan, it's likely that there will be nothing for `Upgrade` to do. If it is called (via `upgradewallet`), then it should do nothing, successfully. This PR changes the default `ScriptPubKeyMan::Upgrade` function so that it returns a success instead of failure when doing nothing.
Fixes#22460
ACKs for top commit:
jonatack:
ACK 5012a7912e
meshcollider:
utACK 5012a7912e
Tree-SHA512: 578c6521e997f7bb5cc44be2cfe9e0a760b6bd4aa301026a6b8b3282e8757473e4cb9f68b2e79dacdc2b42dddae718450072e0a38817df205dfea177a74d7f3d
e6cf0ed92d wallet, rpc: listdescriptors does not need unlocked (Andrew Chow)
3280704886 Pass in DescriptorCache to ToNormalizedString (Andrew Chow)
7a26ff10c2 Change DescriptorImpl::ToStringHelper to use an enum (Andrew Chow)
75530c93a8 Remove priv option for ToNormalizedString (Andrew Chow)
74fede3b8b wallet: Upgrade existing descriptor caches (Andrew Chow)
432ba9e543 wallet: Store last hardened xpub cache (Andrew Chow)
d87b544b83 descriptors: Cache last hardened xpub (Andrew Chow)
cacc391098 Move DescriptorCache writing to WalletBatch (Andrew Chow)
0b4c8ef75c Refactor Cache merging and writing (Andrew Chow)
976b53b085 Revert "Cache parent xpub inside of BIP32PubkeyProvider" (Andrew Chow)
Pull request description:
Currently fetching a normalized descriptor requires the wallet to be unlocked as it needs the private keys to derive the last hardened xpub. This is not very user friendly as normalized descriptors shouldn't require and don't involve the private keys except for derivation. We solve this problem by caching the last hardened xpub (which has to be derived at some point when generating the address pool).
However the last hardened xpub was not already being cached. We only cached the immediate parent xpub and derived child keys. For example, with a descriptor derivation path of `/84'/0'/0'/0/*`, the parent xpub that is cached is `m/84'/0'/0'/0`, and the child keys of `m/84'/0'/0'/0/i` (note that child keys would not be cached in this case). This parent xpub is not suitable for the normalized descriptor form as we want the key at `m/84'/0'/0'`. So this PR adds another field to `DescriptorCache` to cache the last hardened xpub so that we can use them for normalized descriptors.
Since `DescriptorCache` is changing, existing descriptor wallets need to be upgraded to use this new cache. The upgrade will occur in the background either at loading time (if the wallet is not encrypted) or at unlocking time in the same manner that `UpgradeKeyMetadata` operates. It will use a new wallet flag `WALLET_FLAG_LAST_HARDENED_XPUB_CACHED` to indicate whether the descriptor wallet has the last hardened xpub cache.
Lastly `listdescriptors` will not require the wallet to be locked and `getaddressinfo`'s `parent_desc` will always be output (assuming the upgrade has occurred).
ACKs for top commit:
fjahr:
tACK e6cf0ed92d
S3RK:
reACK e6cf0ed
jonatack:
Semi ACK e6cf0ed92d reviewed, debug-built and ran unit tests and some of the descriptor functional tests at each commit. I'm not very familiar with this code and it could be clearer to the uninitiated IMHO, so I'm not confident enough to give a full ACK. Various minor suggestions follow, most of them for readability, feel free to pick and choose.
meshcollider:
Code review + functional test run ACK e6cf0ed92d
Tree-SHA512: ac27aade8644525cd65bfcaf27ff32afb974085b1451faf4ff68c6671a690bd6a41d4f39a33cbf461ae0fbe85995c0a4c08dbd36171da1c1d2a1d00053ad298d
d637a9b397 Taproot descriptor inference (Pieter Wuille)
c7388e5ada Report address as solvable based on inferred descriptor (Pieter Wuille)
29e5dd1a5b consensus refactor: extract ComputeTapleafHash, ComputeTaprootMerkleRoot (Pieter Wuille)
Pull request description:
Includes:
* First commit from #21365, adding TaprootSpendData in SigningProvider
* A refactor to expose ComputeTapleafHash and ComputeTaprootMerkleRoot from script/interpreter
* A tiny change to make `getaddressinfo` report tr() descriptors as solvable (so that inferred descriptors are shown), despite not having signing code for them.
* Logic to infer the script tree back from TaprootSpendData, and then use that to infer descriptors.
ACKs for top commit:
achow101:
re-ACK d637a9b397
Sjors:
re-utACK d637a9b
meshcollider:
Code review ACK d637a9b397
Tree-SHA512: 5ab9b95da662382d8549004be4a1297a577d7caca6b068f875c7c9343723931d03fa9cbf133de11f83b74e4851490ce820fb80413c77b9e8495a5f812e505d86
458a345b05 Add support for SIGHASH_DEFAULT in RPCs, and make it default (Pieter Wuille)
c0f0c8eccb tests: check spending of P2TR (Pieter Wuille)
a2380127e9 Basic Taproot signing logic in script/sign.cpp (Pieter Wuille)
49487bc3b6 Make GetInputUTXO safer: verify non-witness UTXO match (Pieter Wuille)
fd3f6890f3 Construct and use PrecomputedTransactionData in PSBT signing (Pieter Wuille)
5cb6502ac5 Construct and use PrecomputedTransactionData in SignTransaction (Pieter Wuille)
5d2e22437b Don't nuke witness data when signing fails (Pieter Wuille)
ce9353164b Permit full precomputation in PrecomputedTransactionData (Pieter Wuille)
e841fb503d Add precomputed txdata support to MutableTransactionSignatureCreator (Pieter Wuille)
a91d532338 Add CKey::SignSchnorr function for BIP 340/341 signing (Pieter Wuille)
e77a2839b5 Use HandleMissingData also in CheckSchnorrSignature (Pieter Wuille)
dbb0ce9fbf Add TaprootSpendData data structure, equivalent to script map for P2[W]SH (Pieter Wuille)
Pull request description:
Builds on top of #22051, adding signing support after derivation support.
Nothing is changed in descriptor features. Signing works for key path and script path spending, through the normal sending functions, and PSBT-based RPCs. However, PSBT usability is rather low as no extensions have been defined to convey Taproot-specific information, so all script information must be known to the signing wallet.
ACKs for top commit:
achow101:
re-ACK 458a345b05
fjahr:
Code review ACK 458a345b05
Sjors:
ACK 458a345b05
Tree-SHA512: 30ed212cf7754763a4a81624ebc084c51727b8322711ac0b390369213c1a891d367ed8b123882ac08c99595320c11ec57ee42304ff22a69afdc3d1a0d55cc711
e60cd26ad4 Do not load external signers wallets when unsupported (Andrew Chow)
Pull request description:
When external signer support is not compiled, do not load external signer wallets.
Alternative to #22168.
ACKs for top commit:
promag:
Tested ACK e60cd26ad4.
meshcollider:
Code review ACK e60cd26ad4
Tree-SHA512: aed2d0038f448c2f89c6b48f412b106e63c9ed20e748e69aae21fb58c33fc7e4fa73375a52372c73788669eb2b968a8da6b022c65658fa4484f5bbcf205b1b15
d44a261acf Fix issues when `walletdir` is root directory (unknown)
Pull request description:
+ Remove one character less from wallet path
+ After testing lot of random strings with special chars in `wallet_name`, I found that the issue was not related to special characters in the name. Reviewing PR https://github.com/bitcoin/bitcoin/pull/21907 helped me resolve the issue.
**Real issue**: If the path mentioned in `walletdir` is a root directory, first character of the wallet name or path is removed
**Solution**: `if` statement to check `walletdir` is a root directory
Fixes: https://github.com/bitcoin/bitcoin/issues/21510https://github.com/bitcoin/bitcoin/issues/21501
Related PR: https://github.com/bitcoin/bitcoin/pull/20080
Consider the wallet directories `w1` and `w2` saved in `D:\`. Run `bitcoind.exe -walletdir=D:\`, Results for `bitcoin-cli.exe listwalletdir`:
Before this PR:
```
{
"wallets": [
{
"name": "1"
},
{
"name": "2"
}
]
}
```
After this PR:
```
"wallets": [
{
"name": "w1"
},
{
"name": "w2"
}
]
}
```
ACKs for top commit:
ryanofsky:
Code review ACK d44a261acf
meshcollider:
utACK d44a261acf
Tree-SHA512: b09b00f727407e3771c8694861dae1bfd29d97a0d51ddcb5d9c0111dc618b3fff2f75829cbb4361c54457ee564e94fcefd9e2928262a1c918a2b6bbad724eb55
96c2c9520e scripted-diff: Rename SelectCoinsMinConf to AttemptSelection (Andrew Chow)
b583f73354 Move vin filling to before final fee setting (Andrew Chow)
d39cac0547 Set m_subtract_fee_outputs during recipients vector loop (Andrew Chow)
364e0698a5 Move variable initializations to where they are used (Andrew Chow)
32ab430651 Move recipients vector checks to beginning of CreateTransaction (Andrew Chow)
cd1d6d3324 Rename nSubtractFeeFromAmount in CreateTransaction (Andrew Chow)
dac21c793f Rename nValue and nValueToSelect (Andrew Chow)
d2aee3bbc7 Remove extraneous scope in CreateTransactionInternal (Andrew Chow)
b2995963b5 Move cs_wallet lock in CreateTransactionInternal to top of function (Andrew Chow)
Pull request description:
#17331 did some refactors and cleanup of `CreateTransactionInternal` to make it easier to understand, however it is still a bit convoluted even though it doesn't have to be. This PR does additional cleanup and refactoring to `CreateTransactionInternal` so that it is easier to understand. Some unnecessary code was removed, some variables moved around to where they matter, and several indents removed.
ACKs for top commit:
glozow:
reACK 96c2c95
ryanofsky:
Code review ACK 96c2c9520e also acked previously (was reverted).
meshcollider:
re-utACK 96c2c9520e
Tree-SHA512: 3dba67ed436968a07bfd82d435d566ad74e116c6e50ac9baed7144a46ad5c0f630b1ba59d91e8e8972ac2af559d7c0576f0560f09684d2ab20fad6689902866f
1c4b456e1a gui: send using external signer (Sjors Provoost)
24815c6309 gui: wallet creation detects external signer (Sjors Provoost)
3f845ea299 node: add externalSigners to interface (Sjors Provoost)
62ac119f91 gui: display address on external signer (Sjors Provoost)
450cb40a34 wallet: add displayAddress to interface (Sjors Provoost)
eef8d64529 gui: create wallet with external signer (Sjors Provoost)
6cdbc83e93 gui: add external signer path to options dialog (Sjors Provoost)
Pull request description:
Big picture overview in [this gist](https://gist.github.com/Sjors/29d06728c685e6182828c1ce9b74483d).
This PR adds GUI support for external signers, based on the since merged bitcoin/bitcoin#16546 (RPC).
The UX isn't amazing - especially the blocking calls - but it works.
First we adds a GUI setting for the signer script (e.g. path to HWI):
<img width="625" alt="Schermafbeelding 2019-08-05 om 19 32 59" src="https://user-images.githubusercontent.com/10217/62483415-e1ff1680-b7b7-11e9-97ca-8d2ce54ca1cb.png">
Then we add an external signer checkbox to the wallet creation dialog:
<img width="374" alt="Schermafbeelding 2019-11-07 om 19 17 23" src="https://user-images.githubusercontent.com/10217/68416387-b57ee000-0194-11ea-9730-127d60273008.png">
It's checked by default if HWI detects a device. It also grabs the name. It then creates a fresh wallet and imports the keys.
You can verify an address on the device (blocking...):
<img width="673" alt="Schermafbeelding 2019-08-05 om 19 29 22" src="https://user-images.githubusercontent.com/10217/62483560-43bf8080-b7b8-11e9-9902-8a036116dc4b.png">
Sending, including coin selection, Just Works(tm) as long the device is present.
~External signer support is enabled by default when the GUI is configured and Boost::Process is present.~
External signer support remains disabled by default, see https://github.com/bitcoin/bitcoin/pull/21935.
ACKs for top commit:
achow101:
Code Review ACK 1c4b456e1a
hebasto:
ACK 1c4b456e1a, tested on Linux Mint 20.1 (Qt 5.12.8) with HWW `2.0.2-rc.1`.
promag:
Tested ACK 1c4b456e1a but rebased with e033ca1379, with HWI 2.0.2, with Nano S and Nano X.
meshcollider:
re-code-review ACK 1c4b456e1a
Tree-SHA512: 3503113c5c69d40adb6ce364d8e7cae23ce82d032a00474ba9aeb6202eb70f496ef4a6bf2e623e5171e524ad31ade7941a4e0e89539c64518aaec74f4562d86b
c7bd5842e4 MOVEONLY: CWallet transaction code out of wallet.cpp/.h (Russell Yanofsky)
Pull request description:
This commit just moves function without making any changes. It can be reviewed with `git log -p -n1 --color-moved=dimmed_zebra`
Motivation for this change is to make `wallet.cpp/h` less monolithic and start to make wallet transaction state tracking comprehensible so bugs in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking can be fixed safely without introducing new problems.
This moves wallet classes and methods that deal with transactions out of `wallet.cpp/.h` into better organized files:
- `transaction.cpp/.h` - CWalletTx and CMerkleTx class definitions
- `receive.cpp/.h` - functions checking received transactions and computing balances
- `spend.cpp/.h` - functions creating transactions and finding spendable coins
After #20773, when loading is separated from syncing it will also be possible to move more `wallet.cpp/.h` functions to:
- `sync.cpp/.h` - functions handling chain notifications and rescanning
This commit arranges `receive.cpp` and `spend.cpp` functions in dependency order so it's possible to skim `receive.cpp` and get an idea of how computing balances works, and skim `spend.cpp` and get an idea of how transactions are created, without having to jump all over `wallet.cpp` where functions are not in order and there is a lot of unrelated code.
Followup commit "refactor: Detach wallet transaction methods" in https://github.com/bitcoin/bitcoin/pull/21206 follows up this PR and tweaks function names and arguments to reflect new locations. The two commits are split into separate PRs because this commit is more work to maintain and less work to review, while the other commit is less work to maintain and more work to review, so hopefully this commit can be merged earlier.
ACKs for top commit:
Sjors:
re-utACK c7bd5842e4
fjahr:
utACK c7bd5842e4
promag:
Code review ACK c7bd5842e4, verified move only claim.
meshcollider:
Dimmed-zebra-check and functional test run ACK c7bd5842e4
Tree-SHA512: 4981de6911cb1196774db375494355cc9af59b52456129c002d264a77cd9ed6175f8ecbb6b2f492a59a4d5a0def21a39d96fa79c9f4d99be0992985f553be32f
51a3ac242c Have OutputGroup determine the value to use (Andrew Chow)
6d6d278475 Change SelectCoins_test to actually test SelectCoins (Andrew Chow)
9d3bd74ab4 Remove CreateTransaction while loop and some related variables (Andrew Chow)
6f0d5189af Remove use_bnb and bnb_used (Andrew Chow)
de26eb0e1f Do both BnB and Knapsack coin selection in SelectCoinsMinConf (Andrew Chow)
01dc8ebda5 Have KnapsackSolver actually use effective values (Andrew Chow)
bf26e018de Roll static tx fees into nValueToSelect instead of having it be separate (Andrew Chow)
cc3f14b27c Move output reductions for fee to after coin selection (Andrew Chow)
d97d25d950 Make cost_of_change part of CoinSelectionParams (Andrew Chow)
af5867c896 Move some calculations to common code in SelectCoinsMinConf (Andrew Chow)
1bf4a62cb6 scripted-diff: rename some variables (Andrew Chow)
Pull request description:
Changes `KnapsackSolver` to use effective values instead of just the nominal txout value. Since fees are taken into account during the selection itself, we finally get rid of the `CreateTransaction` loop as well as a few other things that only were only necessary because of that loop.
This should not change coin selection behavior at all (except maybe remove weird edge cases that were caused by the loop). In order to keep behavior the same, `KnapsackSolver` will select outputs with a negative effective value (as it did before).
ACKs for top commit:
ryanofsky:
Code review ACK 51a3ac242c. Looks good to go!
instagibbs:
review ACK https://github.com/bitcoin/bitcoin/pull/17331/commits/51a3ac242c92e69b59df26f8f9e287b31e5c3b0f
meshcollider:
re-light-utACK 51a3ac242c
Tree-SHA512: 372c27e00edcd5dbf85177421ba88f20bfdaf1791b6e3dc022c44876ecc379403e2375ed69e71c512c49e6af87641001ff385c4b25ab93684b3a08a53bf3824e
29c9e2c2d2 wallet: Do not iterate a directory if having an error while accessing it (Hennadii Stepanov)
Pull request description:
On Windows when `ListDatabases` tries to iterate any system folder, e.g., "System Volume Information", it falls into an infinite loop.
This PR fixes this bug. Now the `debug.log` contains:
```
2021-05-12T09:07:53Z ListDatabases: Access is denied D:/System Volume Information -- skipping.
```
An easy way to reproduce the bug and test this PR is to pass the `-walletdir=D:\` command-line option, and run the `listwalletdir` RPC, or File -> Open Wallet in the GUI menu.
Fixes#20081.
Fixes#21136.
Fixes#21904.
Also https://bitcoin.stackexchange.com/questions/99243/listwalletdir-access-is-denied-d-system-volume-information
ACKs for top commit:
prayank23:
ACK https://github.com/bitcoin/bitcoin/pull/21907/commits/29c9e2c2d2015ade47ed4497926363dea3f9c59b
promag:
Code review ACK 29c9e2c2d2.
meshcollider:
Code review ACK 29c9e2c2d2
Tree-SHA512: b851c88e6d09626f4cb81acc2fa59a563b2aee64582963285715bf785c64b872e8bf738aa6b27bdbaf4c3e5c8565c2dc2c802135f9aa1f48b4b913435bc5d793
c30dd02cd8 refactor: remove redundant fOnlySafe argument (t-bast)
Pull request description:
The `fOnlySafe` argument to `AvailableCoins` is now redundant, since #21359 added a similar field inside the `CCoinControl` struct (see https://github.com/bitcoin/bitcoin/pull/21359#discussion_r591578684).
Not all code paths create a `CCoinControl` instance, but when it's missing we can default to using only safe inputs which is backwards-compatible.
ACKs for top commit:
instagibbs:
utACK c30dd02cd8
promag:
Code review ACK c30dd02cd8.
achow101:
ACK c30dd02cd8
meshcollider:
Code review + test run ACK c30dd02cd8
Tree-SHA512: af3cb598d06f233fc48a7c9c45bb14da92b5cf4168b8dbd4f134dc3e0c2b615c6590238ddb1eaf380aea5bbdd3386d2ac8ecd7d22dfc93579adc39248542839b
f9cd2bfbcc Rename CoinSelectionParams::effective_fee to m_effective_feerate (Andrew Chow)
bdd0c2934b wallet: Move discard feerate fetching to CreateTransaction (Andrew Chow)
448d04b931 wallet: Move long term feerate setting to CreateTransaction (Andrew Chow)
e2f429e6bb wallet: Replace nFeeRateNeeded with effective_fee (Andrew Chow)
1a6a0b0dfb wallet: Use existing feerate instead of getting a new one (Andrew Chow)
Pull request description:
During coin selection, there are various places where we need to have a feerate. We need the feerate for the transaction itself, the discard fee rate, and long term feerate. Fetching these each time we need them can lead to a race condition where two feerates that should be the same are actually different. One particular instance where this can happen is during the loop in `CreateTransactionInternal`. After inputs are chosen, the expected transaction fee is calculated using a newly fetched feerate. If `pick_new_inputs == false`, the loop will go again with the assumption that the fee for the transaction remains the same. However because the feerate is fetched again, it is possible that it actually isn't and this causes coin selection to fail.
Instead of fetching the feerate each time it is needed, we fetch them all at once at the top of `CreateTransactionInternal`, store them in `CoinSelectionParams`, and use them where needed.
While some of these fee rates probably don't need this caching, I've done it for consistency and the guarantee that they remain the same.
Fixes#19229
ACKs for top commit:
glozow:
reACK https://github.com/bitcoin/bitcoin/commit/f9cd2bfbccb7a2b8ff07cec5f6d2adbeca5f07c3
fjahr:
Code review re-ACK f9cd2bfbcc
Xekyo:
tACK https://github.com/bitcoin/bitcoin/pull/21083/commits/f9cd2bfbccb7a2b8ff07cec5f6d2adbeca5f07c3
meshcollider:
Code review + test run ACK f9cd2bfbcc
Tree-SHA512: be83ff64ba473c3cdd3469c812e214659b6e2a9584c22ed2b1595618fce0d4b35d0901e61068cd1069fc1a8fb911db01dd7312d05c3b8cbafbe2504ab7a3e863
48a0319bab Add a test that selects too large if BnB is used (Andrew Chow)
3e69939b78 Fail if maximum weight is too large (Andrew Chow)
51e2cd322c Have CalculateMaximumSignedTxSize also compute tx weight (Andrew Chow)
Pull request description:
Currently the `Transaction too large` is calculated on the transaction that is returned from `CreateTransaction`. This does not make sense for when `CreateTransaction` is being used for `fundrawtransaction` as no signing occurs so the final returned transaction is missing signatures. Thus users may successfully fund a transaction but fail to broadcast it after it has been fully signed.
So instead we should figure out whether the transaction we are funding will be too large after it is signed. We can do this by having `CalculateMaximumSignedTxSize` also return the transaction weight and then comparing that weight against the maximum weight.
ACKs for top commit:
instagibbs:
ACK https://github.com/bitcoin/bitcoin/pull/20536/commits/48a0319babb409cf486a9eb7c776810f70b06cb2
meshcollider:
utACK 48a0319bab
Xekyo:
utACK with nits 48a0319bab
Tree-SHA512: 1700c60b07f67e2d5c591c5ccd131ac9f1861fab3def961c3c9c4b3281ec1063fe8e4f0f7f1038cac72692340856406bcee8fb45c8104d2ad34357a0ec878ac7
6bfbc97d71 test: disallow sendtoaddress/sendmany when private keys disabled (Jon Atack)
0997019e76 Disallow sendtoaddress and sendmany when private keys disabled (Andrew Chow)
Pull request description:
Since `sendtoaddress` and `sendmany` (which use the `SendMoney` function) create and commit a transaction, they should not do anything when the wallet does not have private keys. Otherwise a valid transaction cannot be made.
Fixes#21104
ACKs for top commit:
jonatack:
ACK 6bfbc97d71
meshcollider:
utACK 6bfbc97d71
kristapsk:
ACK 6bfbc97d71. "Error: Private keys are disabled for this wallet" is definitely a better error message than "Insufficient funds" here. Hopefully change of error code from -6 to -4 doesn't break any software using Bitcoin JSON-RPC API.
Tree-SHA512: f277d6b5252e43942d568614032596f2c0827f00cd0cb71e44ffcb9822bfb15a71730a3e3688f31e59ba4eb7d275250c4e65ad4b6b3e96be6314c56a672432fb
Samuel Dobson
