adb1714426365fb72f73097610ba217ac94ea560 Fix comment typos in scriptpubkeyman.cpp, wallet.cpp, wallet.h (Dimitris Tsapakidis)
Pull request description:
Fixes a number of comment typos found in the code.
Top commit has no ACKs.
Tree-SHA512: c2c996b66d33ecf0ee734b76303a0f2444e184d2f3ff6931768712ca51011ad51e54336c33a2ff55133766d20ae6adcbb14ddc754dde58b1fe9167d68f54fec5
## Issue being fixed or feature implemented
Trivial bug: `keypoolrefill` internally updates the status of the
progress bar each second.
```
m_storage.UpdateProgress(strMsg, static_cast<int>(dProgress));
```
However it can happen that one second is not enough time to make
significant progress and
`static_cast<int>(dProgress) = 0`.
Calling the function with `0` as progress opens a new progress bar and
this led to problems like #5730
## What was done?
trivially make sure to update the progress only if the parameter is >0
## How Has This Been Tested?
rpc does not spam anymore
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that
apply._
- [X] I have performed a self-review of my own code
- [X] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
BACKPORT NOTICE
fixup psbt. all missing changes belongs to src/wallet/scriptpubkeyman.h/cpp ----- they are related to descriptor wallet!
-------------------
931dd4760855e036c176a23ec2de367c460e4243 Make lint-spelling.py happy (Glenn Willen)
11a0ffb29d1b4dcc55c8826873f340ab4196af21 [gui] Load PSBT from clipboard (Glenn Willen)
a6cb0b0c29d327d01aebb98b0504f317eb19c3dc [gui] PSBT Operations Dialog (sign & broadcast) (Glenn Willen)
5dd0c03ffa3aeaa69d8a3a716f902f450d5eaaec FillPSBT: report number of inputs signed (or would sign) (Glenn Willen)
9e7b23b73387600d175aff8bd5e6624dd51f86e7 Improve TransactionErrorString messages. (Glenn Willen)
Pull request description:
Add a "PSBT Operations" dialog, reached from the "Load PSBT..." menu item, giving options to sign or broadcast the loaded PSBT as appropriate, as well as copying the result to the clipboard or saving it to a file.
This is based on Sjors' #17509, and depends on that PR going in first. (It effectively replaces the small "load PSBT" dialog from that PR with a more feature-rich one.)
Some notes:
* The way I display status information is maybe unusual (a status bar, rather than messageboxes.) I think it's helpful to have the information in it be persistent rather than transitory. But if people dislike it, I would probably move the "current state of the transaction" info to the top line of the main label, and the "what action just happened, and did it succeed" info into a messagebox.
* I don't really know much about the translation/localization stuff. I put tr() in all the places it seemed like it ought to go. I did not attempt to translate the result of TransactionErrorString (which is shared by GUI and non-GUI code); I don't know if that's correct, but it matches the "error messages in logs should be googleable in English" heuristic. I don't know whether there are things I should be doing to reduce translator effort (like minimizing the total number of distinct message strings I use, or something.)
* I don't really know how (if?) automated testing is applied to GUI code. I can make a list of PSBTs exercising all the codepaths for manual testing, if that's the right approach. Input appreciated.
ACKs for top commit:
instagibbs:
tested ACK 931dd47608
Sjors:
re-tACK 931dd4760855e036c176a23ec2de367c460e4243
jb55:
ACK 931dd4760855e036c176a23ec2de367c460e4243
achow101:
ACK 931dd4760855e036c176a23ec2de367c460e4243
Tree-SHA512: ade52471a2242f839a8bd6a1fd231443cc4b43bb9c1de3fb5ace7c5eb59eca99b1f2e9f17dfdb4b08d84d91f5fd65677db1433dd03eef51c7774963ef4e2e74f
84d295e51341a126a6c3cbeea7a8caa04c7b5bc3 tests: Check that segwit inputs in psbt have both UTXO types (Andrew Chow)
46004790588c24174a0bec49b540d158ce163ffd psbt: always put a non_witness_utxo and don't remove it (Andrew Chow)
5279d8bc07d601fe6a67ad665fbc7591fe73c7de psbt: Allow both non_witness_utxo and witness_utxo (Andrew Chow)
72f6bec1da198764d4648a10a61c485e7ab65e9e rpc: show both UTXOs in decodepsbt (Andrew Chow)
Pull request description:
Due to recent changes to hardware wallets, the full previous transaction will need to be provided for segwit inputs. Since some software may be checking for the existence of a `witness_utxo` to determine whether to produce a segwit signature, we keep that field to ease the transition.
Because all of the sanity checks implemented by the `IsSane` functions were related to having mixed segwit and non-segwit data in a PSBT, those functions are removed as those checks are no longer proper.
Some tests are updated/removed to accommodate this and a simple test added to check that both UTXOs are being added to segwit inputs.
As discussed in the wallet IRC meeting, our own signer will not require `non_witness_utxo` for segwit inputs.
ACKs for top commit:
Sjors:
utACK 84d295e51341a126a6c3cbeea7a8caa04c7b5bc3 (didn't retest compared to 836d6fc, but fortunately HWI's CI tracks our master branch, with a bunch of hardware wallet simulators)
ryanofsky:
Code review re-ACK 84d295e51341a126a6c3cbeea7a8caa04c7b5bc3. No changes since last review, but now I understand the context better. I think it would good to improve the comments as suggested https://github.com/bitcoin/bitcoin/pull/19215#discussion_r447889473 and maybe refer to
meshcollider:
utACK 84d295e51341a126a6c3cbeea7a8caa04c7b5bc3
Tree-SHA512: ccc1fd3c16ac3859f5aca4fa489bd40f68be0b81bbdc4dd51188bbf28827a8642dc8b605a37318e5f16cf40f1c4910052dace2f27eca21bb58435f02a443e940
4d7369125a82214ea42b808a32b71b315a5c3c72 Disallow automatic conversion between hash types (Ben Woosley)
fa9ef2cdbed32438bdb32623af6e06f13ecd35e4 Remove an apparently unnecessary conversion (Ben Woosley)
966a22d859db37b1775e2180e5be032fc4fdf483 Explicitly support conversion between equivalent hash types (Ben Woosley)
f32c1e07fd6c174ff3f6406a619550d2f6c19360 Use explicit conversion from WitnessV0KeyHash -> CKeyID (Ben Woosley)
2c54217f913967703b404747133be67cf2f4feac Use explicit conversion from PKHash -> CKeyID (Ben Woosley)
a9e451f144480d7b170e49087df162989d31cd20 Convert CPubKey to WitnessV0KeyHash directly (Ben Woosley)
3fcc46812334074d2c77a6233e8a961cd0785872 Prefer explicit CScriptID construction (Ben Woosley)
0a5ea32ce605984094c5552877cb99bc81654f2c Prefer explicit uint160 conversion (Ben Woosley)
Pull request description:
This bases the script/standard hash types, TxDestination-related and CScriptID on a base template which does not silently convert the underlying `uintN` type.
Inspired by and built on #17924. Commits are small and focused to ease review.
Note some of these changes may be relative to existing bugs of the same sort as #17924. See particularly "Convert CPubKey to WitnessV0KeyHash directly" and "Remove an apparently unnecessary conversion".
ACKs for top commit:
achow101:
ACK 4d7369125a82214ea42b808a32b71b315a5c3c72
meshcollider:
re-utACK 4d7369125a82214ea42b808a32b71b315a5c3c72
Tree-SHA512: f1b3284ddc6fb6c6e726f2c22668b6d732d45eb5418262ed2b9c728f60be7be43dfb414b6ddd9915025c8dcd7f360dc3b46e997a945a2feb95b0e5c4f05d6b54
fa650ca7f19307a9237e64ac311488c8947fc12a Use -Wswitch for TxoutType where possible (MarcoFalke)
fa59e0b5bd2aed8380cc9b9e52791f662aecd6a6 test: Add missing script_standard_Solver_success cases (MarcoFalke)
Pull request description:
This removes unused `default:` cases for all `switch` statements on `TxoutType` and adds the cases (`MULTISIG`, `NULL_DATA`, `NONSTANDARD`) to `ExtractDestination` for clarity.
Also, the compiler is now able to use `-Wswitch`.
ACKs for top commit:
practicalswift:
cr ACK fa650ca7f19307a9237e64ac311488c8947fc12a: patch looks correct and `assert(false);` is better than UB :)
hebasto:
ACK fa650ca7f19307a9237e64ac311488c8947fc12a, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 282458b6523bd8923a0c0f5c423d1db2dce2a2d1b1d1dae455415c6fc995bb41ce82c1f9b0a1c0dcc6d874d171e04c30eca585f147582f52c7048c140358630a
92bcd70808b9cac56b184903aa6d37baf9641b37 [wallet] allow transaction without change if keypool is empty (Sjors Provoost)
709f8685ac37510aa145ac259753583c82280038 [wallet] CreateTransaction: simplify change address check (Sjors Provoost)
5efc25f9638866941028454cfa9bae27f1519cb4 [wallet] translate "Keypool ran out" message (Sjors Provoost)
Pull request description:
Extracted from #16944
First this PR simplifies the check when generating a change address, by dropping `CanGetAddresses` and just letting `reservedest.GetReservedDestination` do this check.
Second, when the keypool is empty, instead of immediately giving up, we create a dummy change address and pass that to coin selection. If we didn't need the change address (e.g. when spending the entire balance), then it's all good. If we did need a change address, we throw the original error.
ACKs for top commit:
fjahr:
Code review ACK 92bcd70808b9cac56b184903aa6d37baf9641b37
jonasschnelli:
utACK 92bcd70808b9cac56b184903aa6d37baf9641b37
achow101:
ACK 92bcd70808b9cac56b184903aa6d37baf9641b37
meshcollider:
Code review ACK 92bcd70808b9cac56b184903aa6d37baf9641b37
Tree-SHA512: 07b8c8251f57061c58a85ebf0359be63583c23bac7a2c4cefdc14820c0cdebcc90a2bb218e5ede0db11d1e204cda149e056dfd18614642070b3d56efe2735006
fa4632c41714dfaa699bacc6a947d72668a4deef test: Move boost/stdlib includes last (MarcoFalke)
fa488f131fd4f5bab0d01376c5a5013306f1abcd scripted-diff: Bump copyright headers (MarcoFalke)
fac5c373006a9e4bcbb56843bb85f1aca4d87599 scripted-diff: Sort test includes (MarcoFalke)
Pull request description:
When writing tests, often includes need to be added or removed. Currently the list of includes is not sorted, so developers that write tests and have `clang-format` installed will either have an unrelated change (sorting) included in their commit or they will have to manually undo the sort.
This pull preempts both issues by just sorting all includes in one commit.
Please be aware that this is **NOT** a change to policy to enforce clang-format or any other developer guideline or process. Developers are free to use whatever tool they want, see also #18651.
Edit: Also includes a commit to bump the copyright headers, so that the touched files don't need to be touched again for that.
ACKs for top commit:
practicalswift:
ACK fa4632c41714dfaa699bacc6a947d72668a4deef
jonatack:
ACK fa4632c41714dfaa, light review and sanity checks with gcc build and clang fuzz build
Tree-SHA512: 130a8d073a379ba556b1e64104d37c46b671425c0aef0ed725fd60156a95e8dc83fb6f0b5330b2f8152cf5daaf3983b4aca5e75812598f2626c39fd12b88b180
d67055e00dd90f504384e5c3f229fc95306d5aac Upgrade or rewrite encrypted key checksums (Andrew Chow)
c9a9ddb4142af0af5f7b1a5ccd13f8e585007089 Set fDecryptionThoroughlyChecked based on whether crypted key checksums are valid (Andrew Chow)
a8334f7ac39532528c5f8bd3b0eea05aa63e8794 Read and write a checksum for encrypted keys (Andrew Chow)
Pull request description:
Adds a checksum to the encrypted key record in the wallet database so that encrypted keys can be checked for corruption on wallet loading, in the same way that unencrypted keys are. This allows for us to skip the full decryption of keys upon the first unlocking of the wallet in that session as any key corruption will have already been detected. The checksum is just the double SHA256 of the encrypted key and it is appended to the record after the encrypted key itself.
This is backwards compatible as old wallets will be able to read the encrypted key and ignore that there is more data in the stream. Additionally, old wallets will be upgraded upon their first unlocking (so that key decryption is checked before we commit to a checksum of the encrypted key) and a wallet flag set indicating that. The presence of the wallet flag lets us skip the full decryption as if `fDecryptionThoroughlyChecked` were true.
This does mean that the first time an old wallet is unlocked in a new version will take much longer, but subsequent unlocks will be instantaneous. Furthermore, corruption will be detected upon loading rather than on trying to send so wallet corruption will be detected sooner.
Fixes#12423
ACKs for top commit:
laanwj:
code review ACK d67055e00dd90f504384e5c3f229fc95306d5aac
jonatack:
Code review ACK d67055e00dd90f504384e5c3f229fc95306d5aac
meshcollider:
Code review ACK d67055e00dd90f504384e5c3f229fc95306d5aac
Tree-SHA512: d5c1c10cfcb5db9e10dcf2326423565a9f499290b81f3155ec72254ed5bd7491e2ff5c50e98590eb07842c20d7797b4efa1c3475bae64971d500aad3b4e711d4
## Issue being fixed or feature implemented
- make progress calculations sane
- show progress in GUI but only when you need 100+ new keys
- make it stop on shutdown request
- spam less in debug.log
## What was done?
## How Has This Been Tested?
run tests, run `keypoolrefill` with `1100` (add 100 keys, no gui popup)
and `10000` (100+ keys, progress bar) on testnet wallet, check logs,
verify it can be interrupted on shutdown
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
d2774c09cfcc6c5c967d40bb094eabc8c0bdb6bf Clear any input_errors for an input after it is signed (Andrew Chow)
dc174881ad8498a6905ba282a48077bc5c8037a7 Replace GetSigningProvider with GetSolvingProvider (Andrew Chow)
6a9c429084b40356aa36aa67992da35f61c2f6a2 Move direct calls to MessageSign into new SignMessage functions in CWallet and ScriptPubKeyMan (Andrew Chow)
82a30fade70a2a95c2bbeac4aa06dafda600479d Move key and script filling and signing from CWallet::FillPSBT to ScriptPubKeyMan::FillPSBT (Andrew Chow)
3d70dd99f9f74eef70b19ff6f6f850adc0d5ef8f Move FillPSBT to be a member of CWallet (Andrew Chow)
a4af324d15c1ee43c2abd11a304ae18c7ee82eb0 Use CWallet::SignTransaction in CreateTransaction and signrawtransactionwithwallet (Andrew Chow)
f37de927442d3f024926a66c436d59e391c8696a Implement CWallet::SignTransaction using ScriptPubKeyMan::SignTransaction (Andrew Chow)
d999dd588cab0ff479bc7bee8c9fc33880265ec6 Add SignTransaction function to ScriptPubKeyMan and LegacyScriptPubKeyMan (Andrew Chow)
2c52b59d0a44a86d94fee4e437978d822862c542 Refactor rawtransaction's SignTransaction into generic SignTransaction function (Andrew Chow)
Pull request description:
Following #17261, the way to sign transactions, PSBTs, and messages was to use `GetSigningProvider()` and get a `SigningProvider` containing the private keys. However this may not be feasible for future `ScriptPubKeyMan`s, such as for hardware wallets. Instead of exporting a `SigningProvider` containing private keys, we need to pass these things into the `ScriptPubKeyMan` (via `CWallet`) so that they can do whatever is needed internally to sign them. This is largely a refactor as the logic of processing transactions, PSBTs, and messages for is moved into `LegacyScriptPubKeyMan` and `CWallet` instead of being handled by the caller (e.g. `signrawtransaction`).
To help with this, I've refactored the 3(!) implementations of a `SignTransaction()` function into one generic one. This function will be called by `signrawtransactionwithkey` and `LegacyScriptPubKeyMan::SignTransaction()`. `CWallet::CreateTransaction()` is changed to call `CWallet::SignTransaction()` which in turn, calls `LegacyScriptPubKeyMan::SignTransaction()`. Other `ScriptPubKeyMan`s may implement `SignTransaction()` differently.
`FillPSBT()` is moved to be a member function of `CWallet` and the `psbtwallet.cpp/h` files removed. It is further split so that `CWallet` handles filling the UTXOs while the `ScriptPubKeyMan` handles adding keys, derivation paths, scripts, and signatures. In the end `LegacyScriptPubKeyMan::FillPSBT` still calls `SignPSBTInput`, but the `SigningProvider` is internal to `LegacyScriptPubKeyMan`. Other `ScriptPubKeyMan`s may do something different.
A new `SignMessage()` function is added to both `CWallet` and `ScriptPubKeyMan`. Instead of having the caller (i.e. `signmessage` or the sign message dialog) get the private key, hash the message, and sign, `ScriptPubKeyMan` will now handle that (`CWallet` passes through to the `ScriptPubKeyMan`s as it does for many functions). This signing code is thus consolidated into `LegacyScriptPubKeyMan::SignMessage()`, though other `ScriptPubKeyMan`s may implement it differently. Additionally, a `SigningError` enum is introduced for the different errors that we expect to see from `SignMessage()`.
Lastly, `GetSigningProvider()` is renamed to `GetPublicSigningProvider()`. It will now only provide pubkeys, key origins, and scripts. `LegacySigningProvider` has it's `GetKey` and `HaveKey` functions changed to only return false. Future implementations should return `HidingSigningProvider`s where private keys are hidden.
Other things like `dumpprivkey` and `dumpwallet` are not changed because they directly need and access the `LegacyScriptPubKeyMan` so are not relevant to future changes.
ACKs for top commit:
instagibbs:
reACK d2774c09cf
Sjors:
re-utACK d2774c09cfcc6c5c967d40bb094eabc8c0bdb6bf
meshcollider:
re-utACK d2774c09cfcc6c5c967d40bb094eabc8c0bdb6bf
Tree-SHA512: 89c83e7e7e9315e283fae145a2264648a9d7f7ace8f3281cb3f44f0b013c988d67ba4fa9726e50c643c0ed921bdd269adaec984840d11acf4a681f3e8a582cc1
79facb11e92f8b61063f301027dee7c7344eb1be wallet: use constant CWallets in rpcwallet.cpp (Karl-Johan Alm)
d9b0ebc1da8758645f6de24a4a557511ef9b5e36 wallet: make ReserveDestination pwallet ivar const (Karl-Johan Alm)
57c569e4d9779e2263848770e0ba7eab3054a1bf wallet: make BackupWallet() const (Karl-Johan Alm)
df3a818d2a9fe48e656a8ad2da18fab8a1bfd6e3 wallet: make getters const (Karl-Johan Alm)
227b9dd2d6e1914edfec108af6bec5f12d9f6f39 wallet/spkm: make GetOldestKeyPoolTime() const (Karl-Johan Alm)
22d329ad0ed3ed501bd811720be6a2876d1afe4d wallet: use constant CWallets in rpcdump.cpp (Karl-Johan Alm)
7b3587b29db9eaf11718fc09d48817a45a0a429a wallet/db: make IsDummy() const (Karl-Johan Alm)
d366795d180bc52ba750f71f201a6e5e0c40f1b6 wallet/db: make Backup() const (Karl-Johan Alm)
8cd0b86340870d8f359e4ae26880e03ea36818ab wallet: make CanGetAddresses() const (Karl-Johan Alm)
037fa770eb1ed5152b3ef2c5d3fb2a812d3ef944 wallet: make KeypoolCountExternalKeys() const (Karl-Johan Alm)
ddc93557ad0cf8e433df850d38710828ccd99c16 wallet: make CanGenerateKeys() const (Karl-Johan Alm)
dc2d0650fdb69d27fe1b0092555b7841d542a635 make BlockUntilSyncedToCurrentChain() const (Karl-Johan Alm)
Pull request description:
A lot of places refer to `CWallet*`'s as `CWallet * const`, which translates to *"an immutable pointer to a mutable `CWallet` instance"*; this is
1. often not what the author meant, especially as a lot of these places do not at all modify the wallet object, and
2. confusing, as it tends to suggest that this is a proper way to refer to a constant `CWallet` instance.
This PR changes references to wallets to `const CWallet* const` whenever immutability is expected. This should result in no behavioral changes at all, and improved compile-time error checking.
Note from irc:
> <sipa> sounds good to me; this is the sort of change that as long as it compiles, the behavior shouldn't change
> <sipa> though in general it may lead to introducing automatic copying of objects sometimes (e.g. trying to std::move a const object will work, but generally result in a copy rather than an efficient move)
> <sipa> CWallet objects aren't copied or moved though
ACKs for top commit:
laanwj:
ACK 79facb11e92f8b61063f301027dee7c7344eb1be
Empact:
ACK 79facb11e9
promag:
ACK 79facb11e92f8b61063f301027dee7c7344eb1be.
fjahr:
ACK 79facb11e92f8b61063f301027dee7c7344eb1be
Tree-SHA512: 80a80c1a52f0f788d0ccb268b53bc0f46c796643a3c5a22b55bbbde4ffa6c7e347784e5e53b1e488a3b4e14399e31d5be9417ad5b6319c74a462609e9b1a98e8
a304a3632f0437f4d0f04589a2200e2da91624a7 Revert "Store p2sh scripts in AddAndGetDestinationForScript" (Russell Yanofsky)
eb7d8a5b07e89133a5fb465ad1b793362e7439f7 [test] check for addmultisigaddress regression (Sjors Provoost)
005f8a92ccb5bc10c8daa106d75e1c21390461d3 wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition (Russell Yanofsky)
Pull request description:
Make `LegacyScriptPubKeyMan::CanProvide` method able to recognize p2sh scripts when the redeem script is present in the `mapScripts` map without the p2sh script also having to be added to the `mapScripts` map. This restores behavior prior to #17261, which I think broke backwards compatibility with old wallet files by no longer treating addresses created by `addmultisigaddress` calls before #17261 as solvable.
The reason why tests didn't fail with the CanProvide implementation in #17261 is because of a workaround added in 4a7e43e8460127a40a7895519587399feff3b682 "Store p2sh scripts in AddAndGetDestinationForScript", which masked the problem for new `addmultisigaddress` RPC calls without fixing it for multisig addresses already created in old wallet files.
This change adds a lot of comments and allows reverting commit 4a7e43e8460127a40a7895519587399feff3b682 "Store p2sh scripts in AddAndGetDestinationForScript", so the `AddAndGetDestinationForScript()` function, `CanProvide()` method, and `mapScripts` map should all be more comprehensible
ACKs for top commit:
Sjors:
re-ACK a304a3632f0437f4d0f04589a2200e2da91624a7 (rebase, slight text changes and my test)
achow101:
re-ACK a304a3632f0437f4d0f04589a2200e2da91624a7
meshcollider:
utACK a304a3632f0437f4d0f04589a2200e2da91624a7
Tree-SHA512: 03b625220c49684c376a8062d7646aeba0e5bfe043f977dc7dc357a6754627d594e070e4d458d12d2291888405d94c1dbe08c7787c318374cedd5755e724fb6e
3f373659d732a5b1e5fdc692a45b2b8179f66bec Refactor: Replace SigningProvider pointers with unique_ptrs (Andrew Chow)
3afe53c4039103670cec5f9cace897ead76e20a8 Cleanup: Drop unused GUI learnRelatedScripts method (Andrew Chow)
e2f02aa59e3402048269362ff692d49a6df35cfd Refactor: Copy CWallet signals and print function to LegacyScriptPubKeyMan (Andrew Chow)
c729afd0a3b74a3943e4c359270beaf3e6ff8a7b Box the wallet: Add multiple keyman maps and loops (Andrew Chow)
4977c30d59e88a3e5ee248144bcc023debcd895b refactor: define a UINT256_ONE global constant (Andrew Chow)
415afcccd3e5583defdb76e3a280f48e98983301 HD Split: Avoid redundant upgrades (Andrew Chow)
01b4511206e399981a77976deb15785d18db46ae Make UpgradeKeyMetadata work only on LegacyScriptPubKeyMan (Andrew Chow)
4a7e43e8460127a40a7895519587399feff3b682 Store p2sh scripts in AddAndGetDestinationForScript (Andrew Chow)
501acb5538008d98abe79288b92040bc186b93f3 Always try to sign for all pubkeys in multisig (Andrew Chow)
81610eddbc57c46ae243f45d73e715d509f53a6c List output types in an array in order to be iterated over (Andrew Chow)
eb81fc3ee58d3e88af36d8091b9e4017a8603b3c Refactor: Allow LegacyScriptPubKeyMan to be null (Andrew Chow)
fadc08ad944cad42e805228cdd58e0332f4d7184 Locking: Lock cs_KeyStore instead of cs_wallet in legacy keyman (Andrew Chow)
f5be479694d4dbaf59eef562d80fbeacb3bb7dc1 wallet: Improve CWallet:MarkDestinationsDirty (João Barbosa)
Pull request description:
Continuation of wallet boxes project.
Actually makes ScriptPubKeyMan an interface which LegacyScriptPubkeyMan. Moves around functions and things from CWallet into LegacyScriptPubKeyMan so that they are actually separate things without circular dependencies.
***
Introducing the `ScriptPubKeyMan` (short for ScriptPubKeyManager) for managing scriptPubKeys and their associated scripts and keys. This functionality is moved over from `CWallet`. Instead, `CWallet` will have a pointer to a `ScriptPubKeyMan` for every possible address type, internal and external. It will fetch the correct `ScriptPubKeyMan` as necessary. When fetching new addresses, it chooses the `ScriptPubKeyMan` based on address type and whether it is change. For signing, it takes the script and asks each `ScriptPubKeyMan` for whether that `ScriptPubKeyMan` considers that script `IsMine`, whether it has that script, or whether it is able to produce a signature for it. If so, the `ScriptPubKeyMan` will provide a `SigningProvider` to the caller which will use that in order to sign.
There is currently one `ScriptPubKeyMan` - the `LegacyScriptPubKeyMan`. Each `CWallet` will have only one `LegacyScriptPubKeyMan` with the pointers for all of the address types and change pointing to this `LegacyScriptPubKeyMan`. It is created when the wallet is loaded and all keys and metadata are loaded into it instead of `CWallet`. The `LegacyScriptPubKeyMan` is primarily made up of all of the key and script management that used to be in `CWallet`. For convenience, `CWallet` has a `GetLegacyScriptPubKeyMan` which will return the `LegacyScriptPubKeyMan` or a `nullptr` if it does not have one (not yet implemented, but callers will check for the `nullptr`). For purposes of signing, `LegacyScriptPubKeyMan`'s `GetSigningProvider` will return itself rather than a separate `SigningProvider`. This will be different for future `ScriptPubKeyMan`s.
The `LegacyScriptPubKeyMan` will also handle the importing and exporting of keys and scripts instead of `CWallet`. As such, a number of RPCs have been limited to work only if a `LegacyScriptPubKeyMan` can be retrieved from the wallet. These RPCs are `sethdseed`, `addmultisigaddress`, `importaddress`, `importprivkey`, `importpubkey`, `importmulti`, `dumpprivkey`, and `dumpwallet`. Other RPCs which relied on the wallet for scripts and keys have been modified in order to take the `SigningProvider` retrieved from the `ScriptPubKeyMan` for a given script.
Overall, these changes should not effect how everything actually works and the user should experience no difference between having this change and not having it. As such, no functional tests were changed, and the only unit tests changed were those that were directly accessing `CWallet` functions that have been removed.
This PR is the last step in the [Wallet Structure Changes](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes).
ACKs for top commit:
instagibbs:
re-utACK 3f373659d7
Sjors:
re-utACK 3f373659d732a5b1e5fdc692a45b2b8179f66bec (it still compiles on macOS after https://github.com/bitcoin/bitcoin/pull/17261#discussion_r370377070)
meshcollider:
Tested re-ACK 3f373659d732a5b1e5fdc692a45b2b8179f66bec
Tree-SHA512: f8e2b8d9efa750b617691e8702d217ec4c33569ec2554a060141d9eb9b9a3a5323e4216938e2485c44625d7a6e0925d40dea1362b3af9857cf08860c2f344716
Changes in this commit are required as a preparation to bitcoin#17261
Method GenerateNewHDChainEncrypted moved back from LegacyScriptManager to CWallet
This methods should not be moved before in #17260.
Also added 2 new methods in interface WalletStorage: NewKeyPoolCallback and KeepDestinationCallback
78e407ad0c26190a22de1bc8ed900164a44a36c3 GetKeyBirthTimes should return key ids, not destinations (Gregory Sanders)
70946e7fee54323ce6a5ea8aeb377e2c7c790bc6 Replace CScriptID and CKeyID in CTxDestination with dedicated types (Gregory Sanders)
Pull request description:
The current usage seems to be an overloading of meanings. `CScriptID` is used in the wallet as a lookup key, as well as a destination, and `CKeyID` likewise. Instead, have all destinations be dedicated types.
New types:
`CScriptID`->`ScriptHash`
`CKeyID`->`PKHash`
ACKs for commit 78e407:
ryanofsky:
utACK 78e407ad0c26190a22de1bc8ed900164a44a36c3. Only changes are removing extra CScriptID()s and fixing the test case.
Sjors:
utACK 78e407a
meshcollider:
utACK 78e407ad0c
Tree-SHA512: 437f59fc3afb83a40540da3351507aef5aed44e3a7f15b01ddad6226854edeee762ff0b0ef336fe3654c4cd99a205cef175211de8b639abe1130c8a6313337b9
6e77a7b65cda1b46ce42f0c99ca91562255aeb28 keypool: Add comment about TopUp and when to use it (Andrew Chow)
ea50e34b287e0da0806c1116bb55ade730e8ff6c keypool: Move opportunistic TopUps from LegacyScriptPubKeyMan to CWallet and ReserveDestination (Andrew Chow)
bb2c8ce23c9d7ba8d0e5538243e07218443c85b4 keypool: Remove superfluous topup from CWallet::GetNewChangeDestination (Andrew Chow)
Pull request description:
* The `TopUp()` in `CWallet::GetNewChangeDestination` is unnecessary as currently m_spk_man calls TopUp further down the call stack inside LegacyScriptPubKeyMan::ReserveKeyFromKeyPool (called by LegacyScriptPubKeyMan::GetReservedDestination). This also lets us prepare for future changes with multiple ScriptPubKeyMans in the wallet.
* An opportunistic `TopUp()` is moved from `LegacyScriptPubKeyMan::GetNewDestination` to `CWallet::GetNewDestination`.
* Another opportunistic `TopUp()` is moved from `LegacyScriptPubKeyMan::ReserveKeyFromKeyPool`
Moving opportunistic TopUps ensures that ScriptPubKeyMans will always be topped up before requesting Destinations from them as we cannot always rely on future ScriptPubKeyMan implementaions topping up internally.
See also: https://github.com/bitcoin/bitcoin/pull/17373#discussion_r348598174
ACKs for top commit:
instagibbs:
utACK 6e77a7b65c only change is slight elaboration on comment
ryanofsky:
Code review ACK 6e77a7b65cda1b46ce42f0c99ca91562255aeb28. Only the comment changed since my previous review.
Tree-SHA512: bdfc8d303842c3fb7c3d40af7abfa6d9dac4ef71a24922bb92229674ee89bfe3113ebb46d3903ac48ef99f0a7d6eaac33282495844f2b31f91b8df55084c421f
7cecf10ac32af0fca206ac5f24f482bdec88cb7d Replace LegacyScriptPubKeyMan::IsCrypted with LegacyScriptPubKeyMan::HasEncryptionKeys (Andrew Chow)
bf6417142f36a2f75b3a11368bd73fe788ae1ccb Remove SetCrypted() and fUseCrypto; Change IsCrypted()'s implementation (Andrew Chow)
77a777118eaf78f10a439810d1c08d510a539aa0 Rename EncryptKeys to Encrypt and pass in the encrypted batch to use (Andrew Chow)
35f962fcf0d5107ae6a3a9348e249a9b18ff7106 Clear mapKeys before encrypting (Andrew Chow)
14b5efd66ff0afbf3bf9158a724534a9090fc7fc Move fDecryptionThoroughlyChecked from CWallet to LegacyScriptPubKeyMan (Andrew Chow)
97c0374a46943b2ed38ea24eeeff1f1568dd55b3 Move Unlock implementation to LegacyScriptPubKeyMan (Andrew Chow)
e576b135d6451101d6a8219f55d80aefa216dc38 Replace LegacyScriptPubKeyMan::vMasterKey with GetDecryptionKey() (Andrew Chow)
fd9d6eebc1eabb4675a118d19d38283da2dead39 Add GetEncryptionKey() and HasEncryptionKeys() to WalletStorage (Andrew Chow)
Pull request description:
Let wallet class handle locked/unlocked status and master key, and let keyman
handle encrypting its data and determining whether there is encrypted data.
There should be no change in behavior, but state is tracked differently. The
fUseCrypto atomic bool is eliminated and replaced with equivalent
HasEncryptionKeys checks.
Split from #17261
ACKs for top commit:
laanwj:
ACK 7cecf10
Tree-SHA512: 95a997c366ca539abba0c0a7a0015f39d27b55220683d8d86344ff2d926db4724da67700d2c8ec2d82ed75d07404318c6cb81544af8aadeefab312167257e673
886f1731bec4393dd342403ac34069a3a4f95eea Key pool: Fix omitted pre-split count in GetKeyPoolSize (Andrew Chow)
386a994b853bc5b3a2ed0d812673465b8ffa4849 Key pool: Change ReturnDestination interface to take address instead of key (Andrew Chow)
ba41aa4969169cd73d6b4f57444ed7d8d875de10 Key pool: Move LearnRelated and GetDestination calls (Andrew Chow)
65833a74076cddf986037c6eb3b29a9b9dbe31c5 Add OutputType and CPubKey parameters to KeepDestination (Andrew Chow)
9fcf8ce7ae02bf170b9bf0c2887fd709d752cbf7 Rename Keep/ReturnKey to Keep/ReturnDestination and remove the wrapper (Andrew Chow)
596f6460f9fd8273665c8754ccd673d93a4f25f0 Key pool: Move CanGetAddresses call (Andrew Chow)
Pull request description:
* The `pwallet->CanGetAddresses()` call in `ReserveDestination::GetReservedDestination` to `LegacyScriptPubKeyMan::GetReservedDestination` so that the sanity check results in a failure when a `ScriptPubKeyMan` individually cannot get a destination, not when any of the `ScriptPubKeyMan`s can't.
* `ScriptPubKeyMan::GetReservedDestination` is changed to return the destination so that future `ScriptPubKeyMan`s can return destinations constructed in other ways. This is implemented for `LegacyScriptPubKeyMan` by moving key-to-destination code from `CWallet` to `LegacyScriptPubKeyMan`
* In order for `ScriptPubKeyMan` to be generic and work with future `ScriptPubKeyMan`s, `ScriptPubKeyMan::ReturnDestination` is changed to take a `CTxDestination` instead of a `CPubKey`. Since `LegacyScriptPubKeyMan` still deals with keys internally, a new map `m_reserved_key_to_index` is added in order to track the keypool indexes that have been reserved.
* A bug is fixed in how the total keypool size is calculated as it was omitting `set_pre_split_keypool` which is a bug.
Split from #17261
ACKs for top commit:
ryanofsky:
Code review ACK 886f1731bec4393dd342403ac34069a3a4f95eea. Only change is moving earlier fix to a better commit (same end result).
promag:
Code review ACK 886f1731bec4393dd342403ac34069a3a4f95eea.
instagibbs:
code review re-ACK 886f1731be
Sjors:
Code review re-ACK 886f1731bec4393dd342403ac34069a3a4f95eea
Tree-SHA512: f4be290759f63fdc920d5c02bd0d09acc4b06a5f053787d4afcd3c921b2e35d2bd97617fadae015da853dc189f559fb8d2c6e58d53e4cabfac9af151cd97ad19
* MOVEONLY: Reorder LegacyScriptPubKeyMan methods
Can verify move-only with:
git log -p -n1 --color-moved
This commit is move-only and doesn't change code or affect behavior.
* Refactor: Declare LegacyScriptPubKeyMan methods as virtual
This commit does not change behavior.
* Refactor: Add new ScriptPubKeyMan virtual methods
This commit does not change behavior.
* Refactor: Move SetAddressBook call out of LegacyScriptPubKeyMan::GetNewDestination
This commit does not change behavior.
* Refactor: Move SetWalletFlag out of LegacyScriptPubKeyMan::UpgradeKeyMetadata
This commit does not change behavior.
* Remove SetWalletFlag from WalletStorage
SetWalletFlag is unused.
Does not change any behavior
* Refactor: Remove UnsetWalletFlag call from LegacyScriptPubKeyMan::SetHDSeed
This commit does not change behavior.
* refactor: Replace UnsetWalletFlagWithDB with UnsetBlankWalletFlag in ScriptPubKeyMan
ScriptPubKeyMan is only using UnsetWalletFlagWithDB to unset the blank
wallet flag. Just make that it's own function and not expose the flag
writing directly.
This does not change behavior.
* Refactor: Move SetAddressBookWithDB call out of LegacyScriptPubKeyMan::ImportScriptPubKeys
This commit does not change behavior.
* Refactor: Move LoadKey LegacyScriptPubKeyMan method definition
This commit does not change behavior.
* Refactor: Move GetMetadata code out of getaddressinfo
Easier to review ignoring whitespace:
git log -p -n1 -w
This commit does not change behavior.
* Refactor: Move MarkUnusedAddresses code out of CWallet::AddToWalletIfInvolvingMe
This commit does not change behavior.
* Refactor: Move HavePrivateKeys code out of CWallet::CreateWalletFromFile
This commit does not change behavior.
* Refactor: Move RewriteDB code out of CWallet
This commit does not change behavior.
* Refactor: Move GetKeypoolSize code out of CWallet
This commit does not change behavior.
* Refactor: Move nTimeFirstKey accesses out of CWallet
This commit does not change behavior.
* Re-order methods of scriptpubkeyman for easier backporting changes in future
* Fixup for missing cs_wallet lock:
```
wallet/wallet.cpp:4536:41: error: calling function 'GetTimeFirstKey' requires holding mutex 'spk_man->cs_wallet' exclusively [-Werror,-Wthread-safety-analysis]
int64_t time = spk_man->GetTimeFirstKey();
^
wallet/wallet.cpp:4570:106: error: calling function 'GetTimeFirstKey' requires holding mutex 'walletInstance->m_spk_man->cs_wallet' exclusively [-Werror,-Wthread-safety-analysis]
walletInstance->WalletLogPrintf("nTimeFirstKey = %u\n", walletInstance->m_spk_man->GetTimeFirstKey());
```
* Fix 2 locks
* more of "refactor: Replace UnsetWalletFlagWithDB with UnsetBlankWalletFlag in ScriptPubKeyMan"
* Refactoring GetOldestKeyInPool -> GetOldestKeyTimeInPool, partial bitcoin#10235
Co-authored-by: Andrew Chow <achow101-github@achow101.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
40ad2f6a58228c72c655e3061a19a63640419378 Have importwallet use ImportPrivKeys and ImportScripts (Andrew Chow)
78941da5baf6244c7c54e86cf8ce3e09ce60c239 Optionally allow ImportScripts to set script creation timestamp (Andrew Chow)
94bf156f391759420465b2ff8c44f5f150246c7f Have importaddress use ImportScripts and ImportScriptPubKeys (Andrew Chow)
a00d1e5ec5eb019f8bbeb060a2b09e341d360fe5 Have importpubkey use CWallet's ImportScriptPubKeys and ImportPubKeys functions (Andrew Chow)
c6a827424711333f6f66cf5f9d79e0e6884769de Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys (Andrew Chow)
fae7a5befd0b8746d84a6fde575e5b4ea46cb3c4 Log when an import is being skipped because we already have it (Andrew Chow)
ab28e31c9563bd2cd1e4a088ffd2479517dc83f2 Change ImportScriptPubKeys' internal to apply_label (Andrew Chow)
Pull request description:
#15741 introduced `ImportPrivKeys`, `ImportPubKeys`, `ImportScripts`, and `ImportScriptPubKeys` in `CWallet` which are used by `importmulti`. This PR changes the remaining `import*` RPCs (`importaddress`, `importprivkey`, `importpubkey`, and `importwallet`) to use these functions as well instead of directly adding the imported items to the wallet.
ACKs for top commit:
MarcoFalke:
ACK 40ad2f6a58228c72c655e3061a19a63640419378 (checked that behavior changes are mentioned in the commit body)
ryanofsky:
utACK 40ad2f6a58228c72c655e3061a19a63640419378. Only change since last review is a tweaked commit message (mentioning label update in importpubkey commit)
Sjors:
ACK 40ad2f6a5. Those extra tests also pass.
Tree-SHA512: 910e3bbe20b6f8809a47b7293775db234125615d886c7fd99c194f4cdf00c765eb1e24b1799260f1213b98c88f9bbe696796f36087c182925e567d44e9194c98
8e7f930828a9f8f9be1c90ff45e3fdfef1980eaf Add GetNewChangeDestination for getting new change Destinations (Andrew Chow)
33d13edd2bda0af90660e275ea4fa96ca9896f2a Replace CReserveKey with ReserveDestinatoin (Andrew Chow)
172213be5b174243dc501c1103ad5fe2fee67a16 Add GetNewDestination to CWallet to fetch new destinations (Andrew Chow)
Pull request description:
The wallet should give out destinations instead of keys. It should be the one that handles the conversion from key to destination and the setting of the label, not the caller. In order to do this, two new member functions are introduced `GetNewDestination()` and `GetNewChangeDestination()`. Additionally, `CReserveKey` is changed to be `ReserveDestination` and represents destinations whose keys can be returned to the keypool.
ACKs for top commit:
instagibbs:
re-utACK 8e7f930828
sipa:
ACK 8e7f930828a9f8f9be1c90ff45e3fdfef1980eaf. Concept ACK as this gives a much cleaner abstraction to work with, and light code review ACK.
laanwj:
ACK 8e7f930828a9f8f9be1c90ff45e3fdfef1980eaf
Tree-SHA512: 5be7051409232b71e0ef2c1fd1a3e76964ed2f5b14d47d06edc2ad3b3687abd0be2803a1adc45c0433aa2c3bed172e14f8a7e9f4a23bff70f86260b5a0497500
* Move wallet enums to walletutil.h
* MOVEONLY: Move key handling code out of wallet to keyman file
Start moving wallet and ismine code to scriptpubkeyman.h, scriptpubkeyman.cpp
The easiest way to review this commit is to run:
git log -p -n1 --color-moved=dimmed_zebra
And check that everything is a move (other than includes and copyrights comments).
This commit is move-only and doesn't change code or affect behavior.
* Refactor: Split up CWallet and LegacyScriptPubKeyMan and classes
This moves CWallet members and methods dealing with keys to a new
LegacyScriptPubKeyMan class, and updates calling code to reference the new
class instead of CWallet.
Most of the changes are simple text replacements and variable substitutions
easily verified with:
git log -p -n1 -U0 --word-diff-regex=.
The only nontrivial chunk of code added is the new LegacyScriptPubKeyMan class
declaration, but this code isn't new and is just selectively copied and moved
from the previous CWallet class declaration. This can be verified with:
git log -p -n1 --color-moved=dimmed_zebra src/wallet/scriptpubkeyman.h src/wallet/wallet.h
or
git diff HEAD~1:src/wallet/wallet.h HEAD:src/wallet/scriptpubkeyman.h
This commit does not change behavior.
* Renamed classes in scriptpubkeyman
* Fixes for conflicts, compilation and linkage errors due to previous commits
* Reordered methods in scriptpubkeyman to make further backports easier
* Reordered methods in scriptpubkeyman to make further backports easier (part II)
* Remove HDChain copy from SigningProvider class
* fixes/suggestions
Co-authored-by: Andrew Chow <achow101-github@achow101.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
