Commit graph

1313 commits

Author SHA1 Message Date
Luke Dashjr
7a2ecf16df Wallet: Change IsMine check in CWallet::DelAddressBook from assert to failure 2020-04-06 20:52:04 +00:00
Luke Dashjr
2952c46b92 Wallet: Replace CAddressBookData.name with GetLabel() method 2020-04-06 20:52:04 +00:00
MarcoFalke
c5966a87d1
Merge #18192: Bugfix: Wallet: Safely deal with change in the address book
b5795a7886 Wallet: Add warning comments and assert to CWallet::DelAddressBook (Luke Dashjr)
6d2905f57a Wallet: Avoid unnecessary/redundant m_address_book lookups (Luke Dashjr)
c751d886f4 Wallet: Avoid treating change-in-the-addressbook as non-change everywhere (Luke Dashjr)
8e64b8c84b Wallet: New FindAddressBookEntry method to filter out change entries (and skip ->second everywhere) (Luke Dashjr)
65b6bdc2b1 Wallet: Add CAddressBookData::IsChange which returns true iff label has never been set (Luke Dashjr)
144b2f85da Wallet: Require usage of new CAddressBookData::setLabel to change label (Luke Dashjr)
b86cd155f6 scripted-diff: Wallet: Rename mapAddressBook to m_address_book (Luke Dashjr)

Pull request description:

  In many places, our code assumes that presence in the address book indicates a non-change key, and absence of an entry in mapAddressBook indicates change.

  This no longer holds true after #13756 (first released in 0.19) since it added a "used" DestData populated even for change addresses. Only avoid-reuse wallets should be affected by this issue.

  Thankfully, populating DestData does not write a label to the database, so we can retroactively fix this (so long as the user didn't see the change address and manually assign it a real label).

  Fixing it is accomplished by:

  * Adding a new bool to CAddressBookData to track if the label has ever been assigned, either by loading one from the database, or by assigning one at runtime.
  * `CAddressBookData::IsChange` and `CWallet::FindAddressBookEntry` are new methods to assist in excluding change from code that doesn't expect to see them.
  * For safety in merging, `CAddressBookData::name` has been made read-only (the actual data is stored in `m_label`, a new private member, and can be changed only with `setLabel` which updates the `m_change` flag), and `mapAddressBook` has been renamed to `m_address_book` (to force old code to be rebased to compile).

  A final commit also does some minor optimisation, avoiding redundant lookups in `m_address_book` when we already have a pointer to the `CAddressBookData`.

ACKs for top commit:
  ryanofsky:
    Code review ACK b5795a7886. Pretty clever and nicely implemented fix!
  jonatack:
    ACK b5795a7886 nice improvements -- code review, built/ran tests rebased on current master ff53433fe4 and tested manually with rpc/cli
  jnewbery:
    Good fix. utACK b5795a788.

Tree-SHA512: 40525185a0bcc1723f602243c269499ec86ecb298fecb5ef24d626bbdd5e3efece86cdb1084ad7eebf7eeaf251db4a6e056bcd25bc8457b417fcbb53d032ebf0
2020-04-07 03:51:18 +08:00
Luke Dashjr
b5795a7886 Wallet: Add warning comments and assert to CWallet::DelAddressBook 2020-04-02 16:37:42 +00:00
Luke Dashjr
c751d886f4 Wallet: Avoid treating change-in-the-addressbook as non-change everywhere 2020-04-02 16:25:17 +00:00
Luke Dashjr
8e64b8c84b Wallet: New FindAddressBookEntry method to filter out change entries (and skip ->second everywhere) 2020-04-02 16:02:56 +00:00
Luke Dashjr
144b2f85da Wallet: Require usage of new CAddressBookData::setLabel to change label 2020-04-02 16:01:36 +00:00
Luke Dashjr
b86cd155f6 scripted-diff: Wallet: Rename mapAddressBook to m_address_book
Previous versions assumed absence of an entry in mapAddressBook indicated change.
This no longer holds true (due to bugs) and will shortly be made intentional.
Renaming the field helps ensure that old code using mapAddressBook directly gets checked for necessary rebasing.

-BEGIN VERIFY SCRIPT-
sed -i -e 's/mapAddressBook/m_address_book/g' $(git grep -l 'mapAddressBook' ./src)
-END VERIFY SCRIPT-
2020-04-02 16:00:28 +00:00
Russell Yanofsky
48973402d8 wallet: Avoid use of Chain::Lock in CWallet::GetKeyBirthTimes
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.

This change only affects behavior in the case where wallet last block processed
falls behind the chain tip, where it will treat the last block processed as the
current tip.
2020-03-31 08:36:02 -05:00
Russell Yanofsky
e958ff9ab5 wallet: Avoid use of Chain::Lock in CWallet::CreateTransaction
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.

This change only affects behavior in the case where wallet last block processed
falls behind the chain tip, where it may set a different lock time.
2020-03-31 08:36:02 -05:00
Russell Yanofsky
c0d07dc4cb wallet: Avoid use of Chain::Lock in CWallet::ScanForWalletTransactions
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.

This change affects behavior in a few small ways.

- If there's no max_height specified, percentage progress is measured ending at
  wallet last processed block instead of node tip

- More consistent error reporting: Early check to see if start_block is on the
  active chain is removed, so start_block is always read and the triggers an
  error if it's unavailable
2020-03-31 08:36:02 -05:00
Russell Yanofsky
1be8ff280c wallet: Avoid use of Chain::Lock in rescanblockchain
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.

This change only affects behavior in the case where wallet last block processed
falls behind the chain tip. The rescanblockchain error height error checking
will just be stricter in this case and only accept values up to the last
processed height
2020-03-31 08:36:02 -05:00
Russell Yanofsky
3cb85ac594 wallet refactor: Avoid use of Chain::Lock in CWallet::RescanFromTime
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.

This change has no effect on behavior.
2020-03-31 08:36:02 -05:00
Russell Yanofsky
bf30cd4922 refactor: Add interfaces::FoundBlock class to selectively return block data
FoundBlock class allows interfaces::Chain::findBlock to return more block
information without having lots of optional output parameters. FoundBlock class
is also used by other chain methods in upcoming commits.

There is mostly no change in behavior. Only exception is
CWallet::RescanFromTime now throwing NonFatalCheckError instead of
std::logic_error.
2020-03-31 08:36:02 -05:00
João Barbosa
ee9e88ba27 wallet: Handle duplicate fileid exception 2020-03-31 14:23:43 +01:00
Russell Yanofsky
ab31b9d6fe Fix wallet unload race condition
Currently it's possible for ReleaseWallet to delete the CWallet pointer while
it is processing BlockConnected, etc chain notifications.

To fix this, unregister from notifications earlier in UnloadWallet instead of
ReleaseWallet, and use a new RegisterSharedValidationInterface function to
prevent the CValidationInterface shared_ptr from being deleted until the last
notification is actually finished.
2020-03-27 15:17:35 +00:00
Russell Yanofsky
96dfe5ced6 refactor: Change Chain::broadcastTransaction param order
Make output argument last argument so it works more easily with IPC framework
in #10102, and for consistency with other methods
2020-03-19 15:26:04 -05:00
Russell Yanofsky
6ceb21909c refactor: Rename Chain::Notifications methods to be consistent with other interfaces methods
This also simplifies #10102 removing overrides needed to deal with inconsistent
case convention
2020-03-19 15:26:04 -05:00
Wladimir J. van der Laan
312d27b11c
Merge #17477: Remove the mempool's NotifyEntryAdded and NotifyEntryRemoved signals
e57980b473 [mempool] Remove NotifyEntryAdded and NotifyEntryRemoved callbacks (John Newbery)
2dd561f361 [validation] Remove pool member from ConnectTrace (John Newbery)
969b65f3f5 [validation] Remove NotifyEntryRemoved callback from ConnectTrace (John Newbery)
5613f9842b [validation] Remove conflictedTxs from PerBlockConnectTrace (John Newbery)
cdb893443c [validation interface] Remove vtxConflicted from BlockConnected (John Newbery)
1168394d75 [wallet] Notify conflicted transactions in TransactionRemovedFromMempool (John Newbery)

Pull request description:

  These boost signals were added in #9371, before we had a `TransactionRemovedFromMempool` method in the validation interface. The `NotifyEntryAdded` callback was used by validation to build a vector of conflicted transactions when connecting a block, which the wallet was notified of in the `BlockConnected` CValidationInterface callback.

  Now that we have a `TransactionRemovedFromMempool` callback, we can fire that signal directly from the mempool for conflicted transactions.

  Note that #9371 was implemented to ensure `-walletnotify` events were fired for these conflicted transaction. We inadvertently stopped sending these notifications in #16624 (Sep 2019 commit 7e89994). We should probably fix that, but in a different PR.

ACKs for top commit:
  jonatack:
    Re-ACK e57980b
  ryanofsky:
    Code review ACK e57980b473, no code changes since previous review, but helpful new code comments have been added and the PR description is now more clear about where the old code came from

Tree-SHA512: 3bdbaf1ef2731e788462d4756e69c42a1efdcf168691ce1bbfdaa4b7b55ac3c5b1fd4ab7b90bcdec653703600501b4224d252cfc086aef28f9ce0da3b0563a69
2020-03-19 17:26:51 +01:00
Sjors Provoost
79804fe24b
[rpc] walletcreatefundedpsbt: don't automatically append inputs
When the user doesn't specificy inputs, it makes sense to automatically select them. But when the user does specify inputs, we now fail if the amount is insufficient, unless addInputs is set to true.
2020-03-12 13:07:17 +01:00
John Newbery
cdb893443c [validation interface] Remove vtxConflicted from BlockConnected
The wallet now uses TransactionRemovedFromMempool to be notified about
conflicted wallet, and no other clients use vtxConflicted.
2020-03-11 18:38:33 -04:00
John Newbery
1168394d75 [wallet] Notify conflicted transactions in TransactionRemovedFromMempool
The only CValidationInterface client that cares about transactions that
are removed from the mempool because of CONFLICT is the wallet.

Start using the TransactionRemovedFromMempool method to notify about
conflicted transactions instead of using the vtxConflicted vector in
BlockConnected.
2020-03-11 18:38:27 -04:00
Andrew Chow
dc174881ad Replace GetSigningProvider with GetSolvingProvider
Not all ScriptPubKeyMans will be able to provide private keys,
but pubkeys and scripts should be. So only provide public-only
SigningProviders, i.e. ones that can help with Solving.
2020-03-09 11:16:20 -04:00
Andrew Chow
6a9c429084 Move direct calls to MessageSign into new SignMessage functions in CWallet and ScriptPubKeyMan
Instead of getting a SigningProvider and then going to MessageSign,
have ScriptPubKeyMan handle the message signing internally.
2020-03-09 11:16:20 -04:00
Andrew Chow
82a30fade7 Move key and script filling and signing from CWallet::FillPSBT to ScriptPubKeyMan::FillPSBT
Instead of fetching a SigningProvider from ScriptPubKeyMan in order
to fill and sign the keys and scripts for a PSBT, just pass that
PSBT to a new FillPSBT function that does all that for us.
2020-03-09 11:16:20 -04:00
Andrew Chow
3d70dd99f9 Move FillPSBT to be a member of CWallet 2020-03-09 11:16:17 -04:00
Andrew Chow
a4af324d15 Use CWallet::SignTransaction in CreateTransaction and signrawtransactionwithwallet
Instead of duplicating signing code, just use the function we already
have.
2020-03-08 12:27:05 -04:00
Andrew Chow
f37de92744 Implement CWallet::SignTransaction using ScriptPubKeyMan::SignTransaction 2020-03-08 12:26:32 -04:00
Karl-Johan Alm
57c569e4d9
wallet: make BackupWallet() const 2020-03-02 17:27:35 +09:00
Karl-Johan Alm
df3a818d2a
wallet: make getters const 2020-03-02 17:27:35 +09:00
Karl-Johan Alm
227b9dd2d6
wallet/spkm: make GetOldestKeyPoolTime() const
The method checks the oldest key time for key pools and returns the oldest. It does no modifications.
2020-03-02 17:26:31 +09:00
Karl-Johan Alm
8cd0b86340
wallet: make CanGetAddresses() const
CWallet::CanGetAddresses() is used to check whether the wallet has available or is able to produce keys for addresses. It uses the ScriptPubKeyMan::CanGetAddresses(), which in turn uses the const KeypoolCountExternalKeys() method, all which do counting and no modifications.
2020-03-02 17:26:30 +09:00
Karl-Johan Alm
037fa770eb
wallet: make KeypoolCountExternalKeys() const
This method returns the sum of the key pool sizes. It does no modification.
2020-03-02 17:26:30 +09:00
Karl-Johan Alm
dc2d0650fd
make BlockUntilSyncedToCurrentChain() const
The method checks the chain tip for the best block, and calls SyncWithValidationInterfaceQueue() (a standalone function) if necessary.
2020-03-02 17:26:30 +09:00
Jeffrey Czyz
0aed17ef28 Refactor FormatStateMessage into ValidationState 2020-02-27 17:59:07 -08:00
Luke Dashjr
bca8665d08 scripted-diff: Wallet: Rename incorrectly named *UsedDestination
These functions are used to mark/check if a key of our own has been used to spend (and only for avoid-reuse wallets), which has nothing to do with the destination/address itself.
Give them more accurate names to avoid confusion.

-BEGIN VERIFY SCRIPT-
sed -i -e 's/UsedDestination/SpentKey/g' $(git grep -l 'UsedDestination' ./src)
-END VERIFY SCRIPT-
2020-02-21 21:16:40 +00:00
Wladimir J. van der Laan
051439813e
Merge #13339: wallet: Replace %w by wallet name in -walletnotify script
4e9efac678 test: Check wallet name in -walletnotify script (João Barbosa)
9a5b5ee81f wallet: Replace %w by wallet name in -walletnotify script (João Barbosa)

Pull request description:

  Fixes #13237.

ACKs for top commit:
  laanwj:
    ACK 4e9efac678

Tree-SHA512: 189dd1c785485f2e974d7c12531851b2a977778b3b954aa95efd527322ba3345924cfd587fb9c90b0fa979202af0ab2d90e53d125fe266a36c94f757e4176203
2020-02-17 11:59:23 +01:00
Sjors Provoost
92bcd70808
[wallet] allow transaction without change if keypool is empty 2020-02-04 11:20:25 +01:00
Sjors Provoost
709f8685ac
[wallet] CreateTransaction: simplify change address check 2020-02-04 11:20:25 +01:00
Sjors Provoost
5efc25f963
[wallet] translate "Keypool ran out" message 2020-02-04 11:18:02 +01:00
Hennadii Stepanov
e9434ee03e
Remove false positive GCC warning 2020-02-01 23:07:19 +02:00
Andrew Chow
3f373659d7 Refactor: Replace SigningProvider pointers with unique_ptrs
Needed for future ScriptPubKeyMans which may need to create
SigningProviders dynamically and thus a normal pointer is not enough

This commit does not change behavior.
2020-01-23 16:35:08 -05:00
Andrew Chow
e2f02aa59e Refactor: Copy CWallet signals and print function to LegacyScriptPubKeyMan
This commit does not change behavior.
2020-01-23 16:35:08 -05:00
Andrew Chow
c729afd0a3 Box the wallet: Add multiple keyman maps and loops
Add wallet logic for dealing with multiple ScriptPubKeyMan instances. This
doesn't change current behavior because there is still only a single
LegacyScriptPubKeyMan. But in the future the new logic will be used to support
descriptor wallets.
2020-01-23 16:35:08 -05:00
Andrew Chow
4977c30d59 refactor: define a UINT256_ONE global constant
Instead of having a uint256 representations of one scattered throughout
where it is used, define it globally in uint256.h
2020-01-23 16:35:08 -05:00
Andrew Chow
01b4511206 Make UpgradeKeyMetadata work only on LegacyScriptPubKeyMan 2020-01-23 16:35:08 -05:00
Andrew Chow
eb81fc3ee5 Refactor: Allow LegacyScriptPubKeyMan to be null
In CWallet::LoadWallet, use this to detect and empty wallet with no keys

This commit does not change behavior.
2020-01-23 16:34:28 -05:00
Andrew Chow
fadc08ad94 Locking: Lock cs_KeyStore instead of cs_wallet in legacy keyman
This commit only affects locking behavior and doesn't have other changes.
2020-01-23 16:34:28 -05:00
João Barbosa
f5be479694 wallet: Improve CWallet:MarkDestinationsDirty 2020-01-23 16:34:28 -05:00
Wladimir J. van der Laan
f018d0c9cd
Merge #17924: Bug: IsUsedDestination shouldn't use key id as script id for ScriptHash
6dd59d2e49 Don't allow implementers to think ScriptHash(Witness*()) results in nesting computation (Gregory Sanders)
4b8f1e989f IsUsedDestination shouldn't use key id as script id for ScriptHash (Gregory Sanders)

Pull request description:

  Regression introduced in https://github.com/bitcoin/bitcoin/pull/17621 which causes p2sh-segwit addresses to be erroneously missed.

  Tests are only failing in 0.19 branch, likely because that release still uses p2sh-segwit addresses rather than bech32 by default.

  I'll devise a test case to catch this going forward.

ACKs for top commit:
  achow101:
    ACK 6dd59d2e49
  MarcoFalke:
    ACK 6dd59d2
  meshcollider:
    Code review ACK 6dd59d2e49

Tree-SHA512: b3e0f320c97b8c1f814cc386840240cbde2761fee9711617b713d3f75a4a5dce2dff2df573d80873df42a1f4b74e816ab8552a573fa1d62c344997fbb6af9950
2020-01-16 19:23:33 +01:00