Activate watchonly wallet behavior for LegacySPKM only #17677

pull instagibbs wants to merge 1 commits into bitcoin:master from instagibbs:legacy_spkm_watchonly changing 1 files +2 −2
  1. instagibbs commented at 5:08 PM on December 5, 2019: member

    Slight cleanup following #16944

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

  2. instagibbs commented at 5:09 PM on December 5, 2019: member

    cc @achow101 @Sjors

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

  3. fanquake added the label Wallet on Dec 5, 2019
  4. in src/wallet/wallet.cpp:2173 in 7f8046ea85 outdated 
    2168 | @@ -2169,8 +2169,8 @@ std::map<CTxDestination, std::vector<COutput>> CWallet::ListCoins(interfaces::Ch
2169 |      std::vector<COutPoint> lockedCoins;
2170 |      ListLockedCoins(lockedCoins);
2171 |      // Include watch-only for wallets without private keys
2172 | -    const bool include_watch_only = IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS);
2173 | -    const isminetype is_mine_filter = include_watch_only ? ISMINE_WATCH_ONLY : ISMINE_SPENDABLE;
2174 | +    const bool include_watch_only = GetLegacyScriptPubKeyMan() && IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS);
2175 | +    const isminetype is_mine_filter = include_watch_only ? ISMINE_ALL : ISMINE_SPENDABLE;
    Sjors commented at 11:21 AM on December 6, 2019:

    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

  5. Sjors approved
  6. Sjors commented at 11:21 AM on December 6, 2019: member

    Code review ACK 7f8046e

  7. achow101 commented at 5:10 PM on December 6, 2019: member

    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.

  8. instagibbs force-pushed on Dec 6, 2019
  9. instagibbs commented at 5:15 PM on December 6, 2019: member

    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.

  10. achow101 commented at 6:30 PM on December 6, 2019: member

    ACK 6beb900c0cdf86f40ca5e75f3c77b3d0461ea7a3

  11. fanquake requested review from Sjors on Dec 6, 2019
  12. DrahtBot commented at 3:09 AM on December 7, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  13. in src/qt/sendcoinsdialog.cpp:655 in 6beb900c0c outdated 
     652 | @@ -653,7 +653,7 @@ void SendCoinsDialog::useAvailableBalance(SendCoinsEntry* entry)
 653 |      }
 654 |  
 655 |      // Include watch-only for wallets without private key
    jonatack commented at 6:53 PM on December 8, 2019:

    Suggestion:

    -    // Include watch-only for wallets without private key
+    // Include watch-only for LegacyScriptPubKeyMan wallets without private keys.
  14. in src/qt/sendcoinsdialog.cpp:656 in 6beb900c0c outdated 
     652 | @@ -653,7 +653,7 @@ void SendCoinsDialog::useAvailableBalance(SendCoinsEntry* entry)
 653 |      }
 654 |  
 655 |      // Include watch-only for wallets without private key
 656 | -    coin_control.fAllowWatchOnly = model->privateKeysDisabled();
 657 | +    coin_control.fAllowWatchOnly = model->hasLegacyScriptPubKeyMan() && model->privateKeysDisabled();
    jonatack commented at 6:55 PM on December 8, 2019:

    In addition to here in SendCoinsDialog::useAvailableBalance(), should this be updated in SendCoinsDialog::updateCoinControlState() as well? https://github.com/bitcoin/bitcoin/blob/6beb900c0cdf86f40ca5e75f3c77b3d0461ea7a3/src/qt/sendcoinsdialog.cpp#L711

    instagibbs commented at 2:07 PM on December 9, 2019:

    oh, good catch. Will update.

    Do we want to also do similar gating in https://github.com/bitcoin/bitcoin/pull/16944/files#diff-76b18bd21ccf64e256f029a8198ecdd7R190 ?

    cc @achow101

    achow101 commented at 6:15 PM on December 9, 2019:

    Gating should really only apply to anything that guards ismine filters. We want to have the watch only behavior apply to any watch only wallet. The gating is just to use the correct ismine filters.

    instagibbs commented at 6:25 PM on December 9, 2019:

    to be clear, you are saying we should remove the change above at line 656, rather than add another check at 711?

    achow101 commented at 7:12 PM on December 9, 2019:

    After looking at everywhere that fAllowWatchOnly is being used, yes, I think we should remove this change. Instead we need to check everywhere that we conditionally set a filter for ISMINE_WATCHONLY and add a check for LegacySPKM at each place.

  15. in src/wallet/wallet.cpp:2171 in 6beb900c0c outdated 
    2168 | @@ -2169,7 +2169,7 @@ std::map<CTxDestination, std::vector<COutput>> CWallet::ListCoins(interfaces::Ch
2169 |      std::vector<COutPoint> lockedCoins;
2170 |      ListLockedCoins(lockedCoins);
2171 |      // Include watch-only for wallets without private keys
    jonatack commented at 6:59 PM on December 8, 2019:

    Suggestion:

    -    // Include watch-only for wallets without private keys
+    // Include watch-only for LegacyScriptPubKeyMan wallets without private keys.
  16. jonatack commented at 7:19 PM on December 8, 2019: member

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

  17. Activate no-privkey -> ISMINE_WATCH_ONLY behavior for LegacySPKM only e1e1442f3e
  18. instagibbs force-pushed on Dec 10, 2019
  19. instagibbs commented at 2:29 PM on December 10, 2019: member

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

  20. achow101 commented at 4:37 PM on December 10, 2019: member

    ACK e1e1442f3eadc1d139380e71c1b60b86d8d6bdee

  21. Sjors approved
  22. Sjors commented at 5:08 PM on December 10, 2019: member

    Code review ACK e1e1442f3eadc1d139380e71c1b60b86d8d6bdee

  23. jonatack commented at 5:14 PM on December 16, 2019: member

    This smaller diff in e1e1442f3eadc1d139380e71c1b60b86d8d6bdee 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.

  24. meshcollider commented at 10:29 PM on January 7, 2020: contributor

    Code review ACK e1e1442f3eadc1d139380e71c1b60b86d8d6bdee

  25. meshcollider referenced this in commit cab3859a35 on Jan 7, 2020
  26. meshcollider merged this on Jan 7, 2020
  27. meshcollider closed this on Jan 7, 2020
  28. sidhujag referenced this in commit 888015ee0b on Jan 8, 2020
  29. jasonbcox referenced this in commit d46fc5678d on Oct 2, 2020
  30. sidhujag referenced this in commit 6e28068d81 on Nov 10, 2020
  31. DrahtBot locked this on Feb 15, 2022
Contributors
instagibbsSjorsachow101DrahtBotjonatackmeshcollider
Labels
Wallet
Linked (view graph)
#16944 gui: create PSBT with watch-only wallet
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-21 15:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me