Separate IsMine from solvability #13142

pull sipa wants to merge 5 commits into bitcoin:master from sipa:201804_separate_ismine_solvable changing 4 files +67 −45
  1. sipa commented at 8:31 PM on May 1, 2018: member

    Our current IsMine logic does several things with outputs:

    • Determine "spendability" (roughly corresponding to "could we sign for this")
    • Determine "watching" (is this an output directly or indirectly a watched script)
    • Determine invalidity (is this output definitely not legally spendable, detecting accidental uncompressed pubkeys in witnesses)
    • Determine "solvability" (would we be able to sign for this ignoring the fact that we may be missing some private keys).

    The last item (solvability) is mostly unrelated and only rarely needed (there is just one instance, inside the wallet's coin control logic). This PR changes that instance to use the separate IsSolvable function, and stop IsMine from distinguishing between solvable and unsolvable.

    As an extra, this also simplifies the IsMine logic and adds some extra checks (which wouldn't be hit unless someone adds already invalid scripts to their wallet).

  2. gmaxwell commented at 9:56 PM on May 1, 2018: contributor

    Concept ACK

  3. in src/script/ismine.cpp:85 in a566712129 outdated 
      75 | @@ -76,6 +76,10 @@ isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, b
  76 |          break;
  77 |      case TX_WITNESS_V0_KEYHASH:
  78 |      {
  79 | +        if (sigversion == IsMineSigVersion::WITNESS_V0) {
  80 | +            // P2WPKH inside P2WSH is invalid.
    theuni commented at 10:16 PM on May 1, 2018:

    Do these need to flip isInvalid ?

    sipa commented at 10:19 PM on May 1, 2018:

    Good idea.

  4. theuni commented at 10:35 PM on May 1, 2018: member

    Concept ACK and light review ACK, but I'm not comfortable enough with this code and its use to ACK confidently.

  5. sipa force-pushed on May 1, 2018
  6. sipa commented at 10:48 PM on May 1, 2018: member

    @theuni If it helps, all but the last commit should have no effect at all. To see why:

    • The first commit rewrites the only code that cares about the distinction between IsMine returning ISMINE_WATCH_UNSOLVABLE or ISMINE_WATCH_SOLVABLE (by calling IsSolvable instead, which has essentially the same code as what is inside IsMine for distinguishing between the two).
    • The second commit makes IsMine just stop reporting the difference (and the related constants are renamed).
    • The third commit is a simple refactor.
  7. in src/wallet/wallet.cpp:2367 in ae572b3ff7 outdated 
    2362 | @@ -2363,10 +2363,10 @@ void CWallet::AvailableCoins(std::vector<COutput> &vCoins, bool fOnlySafe, const
2363 |                  continue;
2364 |              }
2365 |  
2366 | -            bool fSpendableIn = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (coinControl && coinControl->fAllowWatchOnly && (mine & ISMINE_WATCH_SOLVABLE) != ISMINE_NO);
2367 | -            bool fSolvableIn = (mine & (ISMINE_SPENDABLE | ISMINE_WATCH_SOLVABLE)) != ISMINE_NO;
2368 | +            bool solvable = IsSolvable(*this, pcoin->tx->vout[i].scriptPubKey);
2369 | +            bool spendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (coinControl && coinControl->fAllowWatchOnly && solvable);
    theuni commented at 12:12 AM on May 2, 2018:

    Imo it'd improve readability here to add a check to the second predicate here:

    ((mine & ISMINE_WATCH_ONLY) != ISMINE_NONE) && ...

    While redundant, it's not immediately obvious that invalid watch-only's were filtered out a few lines up. It would also guard against future oopses if a new type is ever added.

    sipa commented at 5:58 PM on May 3, 2018:

    That would be incorrect. When (coinControl && coinControl->fAllowWatchOnly) we're expected to set spendable = true in COutPut for ISMINE_WATCH_ONLY outputs.

    sipa commented at 5:59 PM on May 3, 2018:

    Oh, nevermind. I misread the !=. Will do.

  8. theuni commented at 12:13 AM on May 2, 2018: member

    @sipa Thanks, that's helpful. I was mostly nervous about bitfield assumptions from the updated isminetype values, but I see now that that's not really a concern.

  9. laanwj commented at 5:02 AM on May 2, 2018: member

    Concept ACK

  10. meshcollider commented at 8:50 AM on May 2, 2018: contributor

    Concept ACK

  11. meshcollider added the label Refactoring on May 2, 2018
  12. meshcollider added the label Wallet on May 2, 2018
  13. in src/script/ismine.cpp:48 in ae572b3ff7 outdated 
      41 | @@ -42,17 +42,20 @@ bool HaveKeys(const std::vector<valtype>& pubkeys, const CKeyStore& keystore)
  42 |      return true;
  43 |  }
  44 |  
  45 | +void Update(isminetype& val, isminetype update)
  46 | +{
  47 | +    if (val == ISMINE_NO) val = update;
  48 | +    if (val == ISMINE_WATCH_ONLY && update == ISMINE_SPENDABLE) val = update;
    promag commented at 11:39 PM on May 2, 2018:

    else if?

    jimpo commented at 4:42 PM on May 3, 2018:

    commit: Simplify IsMine logic

    Even better (arguably): ||.

    if (val == ISMINE_NO ||
    (val == ISMINE_WATCH_ONLY && update == ISMINE_SPENDABLE)) {
  val = update;
}
  14. promag commented at 11:45 PM on May 2, 2018: member

    IMHO this PR could be split in 2. The change here should be just "Separate IsMine from solvability".

    Either way utACK ae572b3

  15. in src/script/ismine.cpp:52 in 9044413bd1 outdated 
      48 | @@ -49,9 +49,9 @@ isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, b
  49 |      std::vector<valtype> vSolutions;
  50 |      txnouttype whichType;
  51 |      if (!Solver(scriptPubKey, whichType, vSolutions)) {
  52 | -        if (keystore.HaveWatchOnly(scriptPubKey))
  53 | -            return ISMINE_WATCH_UNSOLVABLE;
  54 | -        return ISMINE_NO;
  55 | +        if (keystore.HaveWatchOnly(scriptPubKey)) {
    jimpo commented at 4:39 PM on May 3, 2018:

    commit: Make IsMine stop distinguishing solvable/unsolvable

    Does this even need to be here? Wouldn't it just do the watchonly check at the end of the function anyway?

    EDIT: I see it is dropped in the next commit anyway.

  16. in src/script/ismine.cpp:164 in c85f1c2e9d outdated 
     163 |  
 164 |  isminetype IsMine(const CKeyStore& keystore, const CScript& scriptPubKey, bool& isInvalid)
 165 |  {
 166 | -    return IsMineInner(keystore, scriptPubKey, isInvalid, IsMineSigVersion::TOP);
 167 | +    isminetype ret = IsMineInner(keystore, scriptPubKey, isInvalid, IsMineSigVersion::TOP);
 168 | +    if (isInvalid) {
    jimpo commented at 4:58 PM on May 3, 2018:

    commit: Simplify IsMine logic

    It feels like ISMINE_INVALID should be an IsMine type. Would require some refactoring of the callers though...

    promag commented at 5:21 PM on May 3, 2018:

    I had the same feeling but after a while I kind of disagree. I see invalid as an exception, not as a possible "is mine" state.

  17. in src/script/ismine.cpp:153 in c85f1c2e9d outdated 
     150 |          break;
 151 |      }
 152 |      }
 153 |  
 154 | -    if (keystore.HaveWatchOnly(scriptPubKey)) {
 155 | +    if (ret == ISMINE_NO && keystore.HaveWatchOnly(scriptPubKey)) {
    jimpo commented at 5:00 PM on May 3, 2018:

    commit: Simplify IsMine logic

    I get that it's an optimization, but it still strikes me as odd that everywhere else in the function uses Update to modify the ret value, while this does a check and return.

  18. jimpo commented at 5:04 PM on May 3, 2018: contributor

    Nice set of changes. Only thing is that I don't really like the Update pattern -- I find the mutable ret value to be harder to reason about in such a complex, recursive function as compared to explicit returns. Especially given that the pattern isn't applied consistently in the final WATCH_ONLY check.

  19. Make coincontrol use IsSolvable to determine solvability 6d714c3419
  20. Make IsMine stop distinguishing solvable/unsolvable 4e91820531
  21. Simplify IsMine logic b5802a9f5f
  22. Add some checks for invalid recursion in IsMine a53f0feff8
  23. sipa force-pushed on May 3, 2018
  24. sipa commented at 6:04 PM on May 3, 2018: member

    Addressed a few comments. @jimpo: I've added a commit that perhaps makes the data flow clearer, by introducing a separate internal type that does track INVALID as a separate state. For performance reasons, there are still explicit returns in some places. Let me know if you think this helps - I can revert if not.

  25. in src/script/ismine.cpp:36 in d5c79f8aea outdated 
      27 | @@ -28,6 +28,14 @@ enum class IsMineSigVersion
  28 |      WITNESS_V0 = 2  //! P2WSH witness script execution
  29 |  };
  30 |  
  31 | +enum class IsMineResult
    jimpo commented at 6:28 PM on May 3, 2018:

    commit: Make handling of invalid in IsMine more uniform

    If values were assigned to each (NO = 0, WATCH_ONLY = 1, SPENDABLE = 2, INVALID = 3), Update could be implemented as if (update > val) val = update;.

    sipa commented at 6:41 PM on May 3, 2018:

    Good idea, done.

  26. jimpo commented at 6:33 PM on May 3, 2018: contributor

    I think the new commit helps in terms of clarity. I still think, though, that isminetype should be merged with IsMineResult (ie. become an enum instead of a bitfield and have an INVALID value). That's a bigger change, so this is fine for now, but I'm curious if you think that's a good direction or whether there's a reason not to do that.

  27. sipa force-pushed on May 3, 2018
  28. sipa commented at 6:43 PM on May 3, 2018: member

    @jimpo Yes, I think we should do that, but in a later PR. I'm planning to first add tester functions (IsMineWatchOnly, IsMineAny, IsMineSpendable, ...) on the bitfield approach now, and then turn it into a real enum.

  29. jimpo commented at 8:43 PM on May 3, 2018: contributor

    utACK 6472198

  30. in src/script/ismine.cpp:55 in 6472198772 outdated 
      49 | @@ -42,17 +50,18 @@ bool HaveKeys(const std::vector<valtype>& pubkeys, const CKeyStore& keystore)
  50 |      return true;
  51 |  }
  52 |  
  53 | -isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, bool& isInvalid, IsMineSigVersion sigversion)
  54 | +void Update(IsMineResult& val, IsMineResult update)
  55 |  {
  56 | -    isInvalid = false;
  57 | +    if (update > val) val = update;
    laanwj commented at 3:47 PM on May 10, 2018:

    I think the name Update is unclear, though I have a hard time thinking of a different term. Effectively it combines two IsMineResults and chooses the value with the highest "ownership" (apart from INVALID). Don't know if this would work, or be a good fit:

    Update(ret, IsMineResult::WATCH_ONLY);

    to

    ret = std::max(ret, IsMineResult::WATCH_ONLY);
    sipa commented at 7:33 PM on May 11, 2018:

    Done. The Update function was introduced before @jimpo noticed it was really a simple maximum if INVALID is made the highest value.

  31. sipa force-pushed on May 11, 2018
  32. laanwj added this to the "Blockers" column in a project
  33. in src/script/ismine.cpp:79 in 67e88f296f outdated 
      67 | @@ -64,50 +68,58 @@ isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, b
  68 |      case TX_PUBKEY:
  69 |          keyID = CPubKey(vSolutions[0]).GetID();
  70 |          if (!PermitsUncompressed(sigversion) && vSolutions[0].size() != 33) {
  71 | -            isInvalid = true;
  72 | -            return ISMINE_NO;
  73 | +            return IsMineResult::INVALID;
  74 | +        }
  75 | +        if (keystore.HaveKey(keyID)) {
  76 | +            ret = std::max(ret, IsMineResult::SPENDABLE);
    Empact commented at 7:14 AM on May 21, 2018:

    nit: ret is always NO at this point (and in all such assignments) so the max is unnecessary. Could be left as a safety measure though if that's what you intend.

    sipa commented at 12:16 AM on May 22, 2018:

    I expect the compiler to optimize this out; with the max it's more clear what the intention is (going through various combinations and picking the "most" isminish one, unless it's invalid), and less likely to break with future change.

  34. Make handling of invalid in IsMine more uniform c004ffc9b4
  35. in src/script/ismine.cpp:36 in 67e88f296f outdated 
      27 | @@ -28,6 +28,14 @@ enum class IsMineSigVersion
  28 |      WITNESS_V0 = 2  //! P2WSH witness script execution
  29 |  };
  30 |  
  31 | +enum class IsMineResult
  32 | +{
  33 | +    NO = 0,
  34 | +    WATCH_ONLY = 1,
  35 | +    SPENDABLE = 2,
  36 | +    INVALID = 3,
    promag commented at 1:10 PM on May 24, 2018:

    nit, add comment to note that the value matters and INVALID must be the higher?

    sipa commented at 5:29 PM on May 24, 2018:

    Done, added some comments.

  36. sipa force-pushed on May 24, 2018
  37. promag commented at 2:24 PM on May 25, 2018: member

    utACK c004ffc.

  38. in src/wallet/wallet.cpp:2367 in c004ffc9b4 
    2362 | @@ -2363,10 +2363,10 @@ void CWallet::AvailableCoins(std::vector<COutput> &vCoins, bool fOnlySafe, const
2363 |                  continue;
2364 |              }
2365 |  
2366 | -            bool fSpendableIn = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (coinControl && coinControl->fAllowWatchOnly && (mine & ISMINE_WATCH_SOLVABLE) != ISMINE_NO);
2367 | -            bool fSolvableIn = (mine & (ISMINE_SPENDABLE | ISMINE_WATCH_SOLVABLE)) != ISMINE_NO;
2368 | +            bool solvable = IsSolvable(*this, pcoin->tx->vout[i].scriptPubKey);
2369 | +            bool spendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (((mine & ISMINE_WATCH_ONLY) != ISMINE_NO) && (coinControl && coinControl->fAllowWatchOnly && solvable));
    Empact commented at 6:35 PM on May 25, 2018:

    nit: this use of ISMINE_NO seems misplaced. Checking for the existence of the flag in mine, the result should not equal 0, which only incidentally is the value of NO.

    sipa commented at 8:43 PM on May 25, 2018:

    Agree, though that's an existing problem in several places. I prefer to fix it as a follow-up (see #13142 (comment)).

  39. Empact approved
  40. Empact commented at 6:39 PM on May 25, 2018: member

    utACK c004ffc

  41. laanwj commented at 1:11 PM on May 29, 2018: member

    utACK c004ffc9b42a738043e19e4c812fc7e0566119c5

  42. laanwj merged this on May 29, 2018
  43. laanwj closed this on May 29, 2018
  44. laanwj referenced this in commit 56fe3dc235 on May 29, 2018
  45. fanquake removed this from the "Blockers" column in a project
  46. laanwj referenced this in commit 61a044a86a on Jul 4, 2018
  47. furszy referenced this in commit e2cc4aa411 on Aug 21, 2020
  48. UdjinM6 referenced this in commit de84ce0031 on Jun 19, 2021
  49. UdjinM6 referenced this in commit e6bb688cd4 on Jun 20, 2021
  50. UdjinM6 referenced this in commit 4dc7ff677f on Jun 24, 2021
  51. UdjinM6 referenced this in commit da6f2dc0d5 on Jun 24, 2021
  52. UdjinM6 referenced this in commit b7cd70f751 on Jun 26, 2021
  53. UdjinM6 referenced this in commit 2eff4342c6 on Jun 26, 2021
  54. UdjinM6 referenced this in commit 8cda3f374b on Jun 26, 2021
  55. UdjinM6 referenced this in commit ffb7d2abe3 on Jun 26, 2021
  56. UdjinM6 referenced this in commit d860dcaa50 on Jun 26, 2021
  57. UdjinM6 referenced this in commit 6f694a65ea on Jun 28, 2021
  58. UdjinM6 referenced this in commit a53599df24 on Jun 28, 2021
  59. MarcoFalke locked this on Sep 8, 2021
Contributors
sipagmaxwelltheunilaanwjmeshcolliderpromagjimpoEmpact
Labels
RefactoringWallet
Linked (view graph)
#13491 Improve handling of INVALID in IsMine
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-19 09:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me