Skip to content

Activate watchonly wallet behavior for LegacySPKM only#17677

Merged
meshcollider merged 1 commit intobitcoin:masterfrom
instagibbs:legacy_spkm_watchonly
Jan 7, 2020
Merged

Activate watchonly wallet behavior for LegacySPKM only#17677
meshcollider merged 1 commit intobitcoin:masterfrom
instagibbs:legacy_spkm_watchonly

Conversation

@instagibbs
Copy link
Member

@instagibbs instagibbs commented Dec 5, 2019

Slight cleanup following #16944

This should allow future scriptpubkeymans to transparently work, since the current plan is to have ismine always be spendable.

@instagibbs
Copy link
Member Author

instagibbs commented Dec 5, 2019 •
edited
Loading

cc @achow101 @Sjors

conversation ensued on IRC after reading #16373 (comment) since this pattern should be followed in each place we use this behavior for LegacyScriptPubKeyMan

@fanquake fanquake added the Wallet label Dec 5, 2019
Sjors
Sjors approved these changes Dec 6, 2019
View reviewed changes
Copy link
Member

@Sjors Sjors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 7f8046e

src/wallet/wallet.cpp Outdated
Copy link
Member

@Sjors Sjors Dec 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to other reviewers: ISMINE_ALL = ISMINE_WATCH_ONLY | ISMINE_SPENDABLE. When WALLET_FLAG_DISABLE_PRIVATE_KEYS is set, no coins can be ISMINE_SPENDABLE. Therefore ISMINE_ALL leads to the same behavior as ISMINE_WATCH_ONLY

@achow101
Copy link
Member

achow101 commented Dec 6, 2019

This isn't quite what I was thinking.

The thought was that you would do either checking for LegacyScrriptPubKeyMan or ISMINE_ALL, not both. Since ISMINE_WATCHONLY is not intended to be used outside of LegacyScriptPubKeyMan, you could just check for whether it's LegacySPKM and set ISMINE_WATCHONLY if it is. Otherwise you just use ISMINE_SPENDABLE.

Alternatively, because ISMINE_ALL is ISMINE_WATCHONLY | ISMINE_SPENDABLE, for all watch only wallets (disabled private keys), you can have the filter be ISMINE_ALL. That will be sufficient to cover LegacySPKM and future ScriptPubKeyMans which will exclusively use ISMINE SPENDABLE. Of course all of the watch only stuff will need to check for disabled private keys, but we already do that.

I don't think doing both of those things are necessary.

@instagibbs instagibbs force-pushed the legacy_spkm_watchonly branch from 7f8046e to 6beb900 Compare December 6, 2019 17:15
@instagibbs
Copy link
Member Author

instagibbs commented Dec 6, 2019

If I have to choose, I'll stick with the Legacy check + ISMINE_WATCH_ONLY since it's easier for me to understand the purpose/scope of the code. Updated.

@achow101
Copy link
Member

achow101 commented Dec 6, 2019

ACK 6beb900c0cdf86f40ca5e75f3c77b3d0461ea7a3

@fanquake fanquake requested a review from Sjors December 6, 2019 18:31
@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 7, 2019 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

jonatack
jonatack reviewed Dec 8, 2019
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 6beb900 modulo question and suggestions below. Code review/built/ran tests. Would test coverage be helpful here?

src/qt/sendcoinsdialog.cpp Outdated
Copy link
Member

@jonatack jonatack Dec 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion:

-    // Include watch-only for wallets without private key
+    // Include watch-only for LegacyScriptPubKeyMan wallets without private keys.

src/wallet/wallet.cpp Outdated
Copy link
Member

@jonatack jonatack Dec 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion:

-    // Include watch-only for wallets without private keys
+    // Include watch-only for LegacyScriptPubKeyMan wallets without private keys.

@instagibbs instagibbs force-pushed the legacy_spkm_watchonly branch from 6beb900 to e1e1442 Compare December 10, 2019 14:28
@instagibbs
Copy link
Member Author

instagibbs commented Dec 10, 2019

Reduced changes as per discussion to just affecting the ismine filter, rather than fAllowWatchOnly.

@achow101
Copy link
Member

achow101 commented Dec 10, 2019

ACK e1e1442

Sjors
Sjors approved these changes Dec 10, 2019
View reviewed changes
Copy link
Member

@Sjors Sjors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK e1e1442

@jonatack
Copy link
Member

jonatack commented Dec 16, 2019

This smaller diff in e1e1442 seems good and the build/tests pass. Started looking at a test which, per @instagibbs suggestion, would define a new spk_man, add watchonly coins, then check they aren't returned by ListCoins.

meshcollider
meshcollider reviewed Jan 7, 2020
View reviewed changes
Copy link
Contributor

@meshcollider meshcollider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK e1e1442

meshcollider added a commit that referenced this pull request Jan 7, 2020
@meshcollider
Merge #17677: Activate watchonly wallet behavior for LegacySPKM only
cab3859 
e1e1442 Activate no-privkey -> ISMINE_WATCH_ONLY behavior for LegacySPKM only (Gregory Sanders)

Pull request description:

  Slight cleanup following #16944

  This should allow future scriptpubkeymans to transparently work, since the current plan is to have ismine always be spendable.

ACKs for top commit:
  achow101:
    ACK e1e1442
  Sjors:
    Code review ACK e1e1442
  meshcollider:
    Code review ACK e1e1442

Tree-SHA512: c0a86587d33b8b1646494a5cb0bf8681ee4a88e6913918157746943a0996b501903e0e6ee954cf04154c1e0faee0cbb375c74ca789f46ba9244eb5296632b042
@meshcollider meshcollider merged commit e1e1442 into bitcoin:master Jan 7, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 8, 2020
@meshcollider
Merge bitcoin#17677: Activate watchonly wallet behavior for LegacySPK…
888015e 
…M only

e1e1442 Activate no-privkey -> ISMINE_WATCH_ONLY behavior for LegacySPKM only (Gregory Sanders)

Pull request description:

  Slight cleanup following bitcoin#16944

  This should allow future scriptpubkeymans to transparently work, since the current plan is to have ismine always be spendable.

ACKs for top commit:
  achow101:
    ACK e1e1442
  Sjors:
    Code review ACK e1e1442
  meshcollider:
    Code review ACK e1e1442

Tree-SHA512: c0a86587d33b8b1646494a5cb0bf8681ee4a88e6913918157746943a0996b501903e0e6ee954cf04154c1e0faee0cbb375c74ca789f46ba9244eb5296632b042
@MarkLTZ MarkLTZ mentioned this pull request Apr 4, 2020
Closed
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 2, 2020
@meshcollider @majcosta
[backport#17677] Activate watchonly wallet behavior for LegacySPKM only
d46fc56 
Summary:
e1e1442f3eadc1d139380e71c1b60b86d8d6bdee Activate no-privkey -> ISMINE_WATCH_ONLY behavior for LegacySPKM only (Gregory Sanders)

Pull request description:

  Slight cleanup following bitcoin/bitcoin#16944

  This should allow future scriptpubkeymans to transparently work, since the current plan is to have ismine always be spendable.

---

Backport of Core [[bitcoin/bitcoin#17677 | PR17677]]

Test Plan:
  ninja all check check-functional

Reviewers: #bitcoin_abc, jasonbcox

Reviewed By: #bitcoin_abc, jasonbcox

Differential Revision: https://reviews.bitcoinabc.org/D7728
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
@meshcollider
Merge bitcoin#17677: Activate watchonly wallet behavior for LegacySPK…
6e28068 
…M only

e1e1442 Activate no-privkey -> ISMINE_WATCH_ONLY behavior for LegacySPKM only (Gregory Sanders)

Pull request description:

  Slight cleanup following bitcoin#16944

  This should allow future scriptpubkeymans to transparently work, since the current plan is to have ismine always be spendable.

ACKs for top commit:
  achow101:
    ACK e1e1442
  Sjors:
    Code review ACK e1e1442
  meshcollider:
    Code review ACK e1e1442

Tree-SHA512: c0a86587d33b8b1646494a5cb0bf8681ee4a88e6913918157746943a0996b501903e0e6ee954cf04154c1e0faee0cbb375c74ca789f46ba9244eb5296632b042
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@achow101 achow101 achow101 left review comments

+3 more reviewers

@jonatack jonatack jonatack left review comments

@meshcollider meshcollider meshcollider left review comments

@Sjors Sjors Sjors approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Wallet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@instagibbs @achow101 @DrahtBot @jonatack @Sjors @meshcollider @fanquake