Use IsMine to validate custom change address #11184

pull dooglus wants to merge 1 commits into bitcoin:0.15 from dooglus:change_ismine_0.15 changing 3 files +7 −9
  1. dooglus commented at 11:07 PM on August 28, 2017: contributor

    ... so it recogizes SegWit change addresses.

    Fixes #11137.

  2. in src/qt/sendcoinsdialog.cpp:792 in 89daeb8491 outdated 
     788 | @@ -789,9 +789,7 @@ void SendCoinsDialog::coinControlChangeEdited(const QString& text)
 789 |          }
 790 |          else // Valid address
 791 |          {
 792 | -            CKeyID keyid;
 793 | -            addr.GetKeyID(keyid);
 794 | -            if (!model->havePrivKey(keyid)) // Unknown change address
 795 | +            if (model->isMine(addr.Get()) != ISMINE_SPENDABLE) // Unknown change address
    promag commented at 11:28 PM on August 28, 2017:

    IMO remove the comment, the code is more expressive.

    promag commented at 11:33 PM on August 28, 2017:

    Move { to this line.

    MarcoFalke commented at 11:40 PM on August 28, 2017:

    if you feel like it, you might add a const CTxDestination dest = addr.Get(); before that line and then use if (!bool(model->IsMine(dest) & ISMINE_SPENDABLE)).

    Though, it shouldn't make a difference.

  3. in src/qt/walletmodel.cpp:564 in 89daeb8491 outdated 
     560 | @@ -561,9 +561,9 @@ bool WalletModel::getPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) const
 561 |      return wallet->GetPubKey(address, vchPubKeyOut);
 562 |  }
 563 |  
 564 | -bool WalletModel::havePrivKey(const CKeyID &address) const
 565 | +isminetype WalletModel::isMine(const CTxDestination &dest) const
    promag commented at 11:29 PM on August 28, 2017:

    const CTxDestination& dest.

    MarcoFalke commented at 11:35 PM on August 28, 2017:

    function names are PascalCase.

    dooglus commented at 11:39 PM on August 28, 2017:

    Where can I find guidelines about where to put the whitespace? I went with the existing layout. See how the line I replaced had (const CKeyID &address)?

    MarcoFalke commented at 11:41 PM on August 28, 2017:

    This is mentioned in the developer notes. Though, you could always just run clang format (also mentioned in the developer notes)

    dooglus commented at 12:20 AM on August 29, 2017:

    Check src/qt/walletmodel.h - they're all lowerThenUpper

    promag commented at 12:25 AM on August 29, 2017:

    Yes but it's outdated.

    dooglus commented at 12:40 AM on August 29, 2017:

    So what's the plan? Shouldn't we go through the code fixing all the outdated style at once rather than fixing one at a time?

    dooglus commented at 12:41 AM on August 29, 2017:

    I mean, I can change just the one I touched if that's preferable.

  4. in src/qt/walletmodel.h:9 in 89daeb8491 outdated 
       6 | @@ -7,6 +7,7 @@
   7 |  
   8 |  #include "paymentrequestplus.h"
   9 |  #include "walletmodeltransaction.h"
  10 | +#include "script/ismine.h"
    promag commented at 11:30 PM on August 28, 2017:

    Sort include.

  5. in src/qt/walletmodel.h:194 in 89daeb8491 outdated 
     190 | @@ -190,7 +191,7 @@ class WalletModel : public QObject
 191 |      UnlockContext requestUnlock();
 192 |  
 193 |      bool getPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) const;
 194 | -    bool havePrivKey(const CKeyID &address) const;
 195 | +    isminetype isMine(const CTxDestination &dest) const;
    promag commented at 11:30 PM on August 28, 2017:

    const CTxDestination& dest.

    promag commented at 11:35 PM on August 28, 2017:

    For the record, another option, that doesn't require to include in this header script/ismine.h, is bool isSpendable(const CKeyID& address).

    dooglus commented at 11:57 PM on August 28, 2017:

    I don't see bool isSpendable(const CKeyID& address) anywhere. And the point is that we want to be able to check the spendability of general addresses, not only CKeyIDs.

  6. MarcoFalke commented at 11:40 PM on August 28, 2017: member

    utACK 89daeb84915d7fd21230771ed266dde1f7d7afda

  7. promag commented at 12:13 AM on August 29, 2017: member

    BTW, squash and reword, commit message is too long.

  8. dooglus force-pushed on Aug 29, 2017
  9. dooglus force-pushed on Aug 29, 2017
  10. dooglus commented at 12:18 AM on August 29, 2017: contributor

    Addressed @MarcoFalke's nit re. masking out ISMINE_SPENDABLE.

    Squashed.

    Reworded commit message.

  11. promag commented at 12:21 AM on August 29, 2017: member

    Thanks @dooglus but could you remove period. Suggestion Use IsMine to validate custom change address. Update PR title accordingly.

  12. in src/qt/sendcoinsdialog.cpp:793 in b49064c9b5 outdated 
     788 | @@ -789,10 +789,8 @@ void SendCoinsDialog::coinControlChangeEdited(const QString& text)
 789 |          }
 790 |          else // Valid address
 791 |          {
 792 | -            CKeyID keyid;
 793 | -            addr.GetKeyID(keyid);
 794 | -            if (!model->havePrivKey(keyid)) // Unknown change address
 795 | -            {
 796 | +            const CTxDestination dest = addr.Get();
 797 | +            if (!bool(model->isMine(dest) & ISMINE_SPENDABLE)) {
    sipa commented at 12:34 AM on August 29, 2017:

    The bool(..) is unnecessary.

  13. dooglus force-pushed on Aug 29, 2017
  14. dooglus commented at 12:38 AM on August 29, 2017: contributor

    Removed period from commit message

    Removed bool

    Squashed again

  15. dooglus force-pushed on Aug 29, 2017
  16. dooglus commented at 12:39 AM on August 29, 2017: contributor

    Replaced commit message with @promag's suggested one

  17. dooglus force-pushed on Aug 29, 2017
  18. dooglus commented at 12:49 AM on August 29, 2017: contributor

    It appears that calling the function IsMine instead of isMine caused an error:

      CXX      qt/qt_libbitcoinqt_a-walletmodeltransaction.o
qt/walletmodel.cpp:566:12: error: too many arguments to function call, expected 1, have 2; did you mean '::IsMine'?
    return IsMine(*wallet, dest);
           ^~~~~~
           ::IsMine
./script/ismine.h:39:12: note: '::IsMine' declared here
isminetype IsMine(const CKeyStore& keystore, const CTxDestination& dest, SigVersion = SIGVERSION_BASE);
           ^

    I'll call it 'IsSpendable()` instead.

  19. dooglus force-pushed on Aug 29, 2017
  20. fanquake added the label Wallet on Aug 29, 2017
  21. MarcoFalke commented at 9:07 AM on August 29, 2017: member

    utACK 848ef973d7ecb054e0d6d6952fd69a0816f3b7cd

  22. promag commented at 9:13 AM on August 29, 2017: member

    @dooglus i believe you only had to call like ::IsMine(). Either way please update PR title too, and elaborate more about the motivation in the description. Ty.

  23. meshcollider commented at 11:37 AM on August 29, 2017: contributor

    @promag title is still correct I believe, because it still uses IsMine() to check

  24. promag commented at 4:48 PM on August 29, 2017: member

    @MeshCollider it's too long 🙄.

  25. dooglus renamed this:
    Use IsMine() to check whether we own the custom change address...
    Use IsMine to validate custom change address
    on Aug 29, 2017
  26. dooglus commented at 6:47 PM on August 29, 2017: contributor

    I finally understood what you meant about IsSpendable(), so did it that way. IsSpendable() calls IsMine(), so the commit log is correct. I've used the same string for the PR title.

  27. sipa commented at 7:43 PM on August 29, 2017: member

    Should change to a watch-only address be permitted?

  28. dooglus commented at 8:29 PM on August 29, 2017: contributor

    Change to any address is permitted. The warning is for if you do not have control of the address.

    I'd like to see a warning if I accidentally try to send change to a watchonly address. Because maybe I accidentally copied the wrong address and am about to lose my coins.

  29. promag commented at 8:42 PM on August 29, 2017: member

    should change to a watch-only address be permitted? @sipa follow up discussion? For now ISMINE_SPENDABLE is clear and does prevents possible problems that ISMINE_WATCH_ONLY could have.

  30. in src/qt/sendcoinsdialog.cpp:793 in 848ef973d7 outdated 
     788 | @@ -789,10 +789,8 @@ void SendCoinsDialog::coinControlChangeEdited(const QString& text)
 789 |          }
 790 |          else // Valid address
 791 |          {
 792 | -            CKeyID keyid;
 793 | -            addr.GetKeyID(keyid);
 794 | -            if (!model->havePrivKey(keyid)) // Unknown change address
 795 | -            {
 796 | +            const CTxDestination dest = addr.Get();
 797 | +            if (!model->IsSpendable(dest)) {
    jonasschnelli commented at 9:15 PM on August 29, 2017:

    Wouldn't if (!model->IsSpendable(addr.Get())) { be simpler?

    dooglus commented at 9:45 PM on August 29, 2017:

    Yes it would, and that's how I had it until @MarcoFalke made this comment.

    Is there a style guide that tells me how such things should be? Lots of people seem to have ideas about what's right, but I'd like to see something in writing, as it were.

    MarcoFalke commented at 6:54 AM on August 30, 2017:

    dest can be used in a few places below, but as I mentioned above this is just personal preference for one style over another. Just use what you feel makes more sense, no need to discuss.

    dooglus commented at 7:44 AM on August 30, 2017:

    Good point. I use it in the few places below now too.

  31. jonasschnelli commented at 9:15 PM on August 29, 2017: contributor

    utACK 848ef973d7ecb054e0d6d6952fd69a0816f3b7cd

  32. Use IsMine to validate custom change address c41224dfd5
  33. dooglus force-pushed on Aug 30, 2017
  35. promag commented at 2:51 PM on September 4, 2017: member

    utACK c41224d.

  36. practicalswift commented at 9:51 PM on September 4, 2017: contributor

    Concept ACK! Solves also the issue discussed in my PR #10972 which I've now closed.

  37. laanwj added this to the milestone 0.15.1 on Sep 5, 2017
  38. laanwj added the label Needs backport on Sep 5, 2017
  39. laanwj merged this on Sep 5, 2017
  40. laanwj closed this on Sep 5, 2017
  41. laanwj referenced this in commit a6c439f694 on Sep 5, 2017
  42. MarcoFalke removed the label Needs backport on Sep 5, 2017
  43. MarcoFalke removed this from the milestone 0.15.1 on Sep 5, 2017
  44. laanwj commented at 10:48 PM on September 5, 2017: member

    Ouch, did no one notice that this was filed against 0.15? Had to revert it, because otherwise it would mess up the release process for 0.15.0. We need a new PR for master.

  45. MarcoFalke referenced this in commit a31e9ad4f0 on Sep 5, 2017
  46. PastaPastaPasta referenced this in commit baecc42630 on Sep 20, 2019
  47. PastaPastaPasta referenced this in commit 175a137809 on Sep 20, 2019
  48. PastaPastaPasta referenced this in commit 28b75689d9 on Sep 23, 2019
  49. PastaPastaPasta referenced this in commit b2c4b988a6 on Sep 23, 2019
  50. PastaPastaPasta referenced this in commit 7168a69cbc on Sep 24, 2019
  51. codablock referenced this in commit a88e000fe9 on Sep 25, 2019
  52. barrystyle referenced this in commit bbb6ea6af3 on Jan 22, 2020
  53. DrahtBot locked this on Sep 8, 2021
Contributors
doogluspromagMarcoFalkesipameshcolliderjonasschnellipracticalswiftlaanwj
Labels
Wallet
Linked (view graph)
#11137 Coin Control doesn't recognize my segwit address as a valid change address#11247 qt: Use IsMine to validate custom change address
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-14 18:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me