I'm sorry, this should have been a few commits, but it got away from me
as the scope of this refactoring was not immediately obvious. It essentially
just moves code around so hopefully is not too hard to review. --asp
This restricts the `bumpfee` RPC, when given a feerate target, to unblinded
transactions, because this is the only way I could figure out quickly to
allow adding inputs to an already-created transaction. It appears to be the
strategy used by the `fundrawtransaction` RPC which has similar behaviour.
This should be fixed in a followup PR.
No reason for this class to exist if it doesn't have any code to run in the
destructor. e10e1e8db0 from
https://github.com/bitcoin/bitcoin/pull/16208 recently removed code destructor
code that would return an unused key if the transaction wasn't committed.
e10e1e8db0 Restrict lifetime of ReserveDestination to CWallet::CreateTransaction (Gregory Sanders)
d9ff862f2d CreateTransaction calls KeepDestination on ReserveDestination before success (Gregory Sanders)
Pull request description:
The typical usage pattern of `ReserveDestination` is to explicitly `KeepDestination`, or `ReturnDestination` when it's detected it will not be used.
Implementers such as myself may fail to complete this pattern, and could result in key re-use: https://github.com/bitcoin/bitcoin/pull/15557#discussion_r271956393
Since ReserveDestination is currently only used directly in the `CreateTransaction`/`CommitTransaction` flow(or fee bumping where it's just used in `CreateTransaction`), I instead make the assumption that if a transaction is returned by `CreateTransaction` it's highly likely that it will be accepted by the caller, and the `ReserveDestination` kept. This simplifies the API as well. There are very few cases where this would not be the case which may result in keys being burned.
Those failure cases appear to be:
`CommitTransaction` failing to get the transaction into the mempool
Belt and suspenders check in `WalletModel::prepareTransaction`
Alternative to https://github.com/bitcoin/bitcoin/pull/15796
ACKs for top commit:
achow101:
ACK e10e1e8db0 Reviewed the diff
stevenroose:
utACK e10e1e8db0
meshcollider:
utACK e10e1e8db0
Tree-SHA512: 78d047a00f39ab41cfa297052cc1e9c224d5f47d3d2299face650d71827635de077ac33fb4ab9f7dc6fc5a27f4a68415a1bc9ca33a3cb09a78f4f15b2a48411b
93ce4a0b6f Move WatchOnly stuff from SigningProvider to CWallet (Andrew Chow)
8f5b81e6ed Remove CCryptoKeyStore and move all of it's functionality into CWallet (Andrew Chow)
37a79a4fcc Move various SigningProviders to signingprovider.{cpp,h} (Andrew Chow)
16f8096e91 Move KeyOriginInfo to its own header file (Andrew Chow)
d9becff4e1 scripted-diff: rename CBasicKeyStore to FillableSigningProvider (Andrew Chow)
a913e3f2fb Move HaveKey static function from keystore to rpcwallet where it is used (Andrew Chow)
c7797ec655 Remove CKeyStore and squash into CBasicKeyStore (Andrew Chow)
1b699a5083 Add HaveKey and HaveCScript to SigningProvider (Andrew Chow)
Pull request description:
This PR compresses the `CWallet` chain of inheritance from 5 classes to 3 classes. `CBasicKeyStore` is renamed to `FillableSigningProvider` and some parts of it (the watchonly parts) are moved into `CWallet`. `CKeyStore` and `CCrypoKeyStore` are completely removed. `CKeyStore`'s `Have*` functions are moved into `SigningProvider` and the `Add*` moved into `FillableSigningProvider`, thus allowing it to go away entirely. `CCryptoKeyStore`'s functionality is moved into `CWallet`. The new inheritance chain is:
```
SigningProvider -> FillableSigningProvider -> CWallet
```
`SigningProvider` now is the class the provides keys and scripts and indicates whether keys and scripts are present. `FillableSigningProvider` allows keys and scripts to be added to the signing provider via `Add*` functions. `CWallet` handles all of the watchonly stuff (`AddWatchOnly`, `HaveWatchOnly`, `RemoveWatchOnly` which were previously in `CKeyStore`) and key encryption (previously in `CCryptoKeyStore`).
Implements the 2nd [prerequisite](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes#cwallet-subclass-stack) from the wallet restructure.
ACKs for top commit:
Sjors:
re-ACK 93ce4a0; it keeps `EncryptSecret`, `DecryptSecret` and `DecryptKey` in `wallet/crypter.cpp`, but makes them not static. It improves alphabetical includes, reorders some function definitions, fixes commit message, brings back lost code comment.
instagibbs:
utACK https://github.com/bitcoin/bitcoin/pull/16227/commits/93ce4a0b6fb54efb1f424a71dfc09cc33307e5b9
Tree-SHA512: 393dfd0623ad2dac38395eb89b862424318d6072f0b7083c92a0d207fd032c48b284f5f2cb13bc492f34557de350c5fee925da02e47daf011c5c6930a721b6d3
8e7f930828 Add GetNewChangeDestination for getting new change Destinations (Andrew Chow)
33d13edd2b Replace CReserveKey with ReserveDestinatoin (Andrew Chow)
172213be5b 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 https://github.com/bitcoin/bitcoin/pull/16237/commits/8e7f930828a9f8f9be1c90ff45e3fdfef1980eaf
sipa:
ACK 8e7f930828. Concept ACK as this gives a much cleaner abstraction to work with, and light code review ACK.
laanwj:
ACK 8e7f930828
Tree-SHA512: 5be7051409232b71e0ef2c1fd1a3e76964ed2f5b14d47d06edc2ad3b3687abd0be2803a1adc45c0433aa2c3bed172e14f8a7e9f4a23bff70f86260b5a0497500
Instead of having the same multiple lines of code everywhere
that new destinations are fetched, introduce GetNewDestination as
a member function of CWallet which does the key fetching, label
setting, script generation, and destination generation.
5c759c73b2 [wallet] Move maxTxFee to wallet (John Newbery)
Pull request description:
Closes#15355
Moves the `-maxtxfee` from the node to the wallet. See discussion in issue for details.
This is a cleanup. There is no change in behaviour.
Completes #15620
ACKs for commit 5c759c:
MarcoFalke:
utACK 5c759c73b2
ryanofsky:
utACK 5c759c73b2. Changes since last review: updated commit message and an error message and method name.
meshcollider:
utACK https://github.com/bitcoin/bitcoin/pull/15778/commits/5c759c73b2602c7fde1c50dbafe5525904c1b64c
Tree-SHA512: 2f9b2729da3940a5cda994d3f3bc11ee1a52fcc1c5e9842ea0ea63e4eb0300e8416853046776311298bc449ba07554aa46f0f245ce28598a5b0bd7347c12e752
This commit moves the maxtxfee setting to the wallet. There is only
one minor behavior change:
- an error message in feebumper now refers to -maxtxfee instead of
maxTxFee.
Moves the following wallet load functions to a new wallet/load unit in
the libbitcoin_wallet library. All other functions in wallet/init remain
in libbitcoin_server:
- `VerifyWallets`
- `LoadWallets`
- `StartWallets`
- `FlushWallets`
- `StopWallets`
- `UnloadWallets`
