Separate IsMine from solvability #13142

sipa · 2018-05-01T20:31:38Z

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).

gmaxwell · 2018-05-01T21:56:52Z

Concept ACK

theuni · 2018-05-01T22:16:49Z

src/script/ismine.cpp

Do these need to flip isInvalid ?

theuni · 2018-05-01T22:35:48Z

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

sipa · 2018-05-01T22:48:07Z

@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.

theuni · 2018-05-02T00:12:15Z

src/wallet/wallet.cpp

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.

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

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

theuni · 2018-05-02T00:13:49Z

@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.

laanwj · 2018-05-02T05:02:33Z

Concept ACK

meshcollider · 2018-05-02T08:50:52Z

Concept ACK

promag

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

Either way utACK ae572b3

promag · 2018-05-02T23:39:31Z

src/script/ismine.cpp

commit: Simplify IsMine logic

Even better (arguably): ||.

if (val == ISMINE_NO || (val == ISMINE_WATCH_ONLY && update == ISMINE_SPENDABLE)) { val = update; }

jimpo

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.

jimpo · 2018-05-03T16:39:49Z

src/script/ismine.cpp

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.

jimpo · 2018-05-03T16:42:27Z

src/script/ismine.cpp

commit: Simplify IsMine logic

Even better (arguably): ||.

if (val == ISMINE_NO || (val == ISMINE_WATCH_ONLY && update == ISMINE_SPENDABLE)) { val = update; }

jimpo · 2018-05-03T16:58:19Z

src/script/ismine.cpp

commit: Simplify IsMine logic

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

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.

jimpo · 2018-05-03T17:00:24Z

src/script/ismine.cpp

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.

sipa · 2018-05-03T18:04:37Z

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.

jimpo

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.

jimpo · 2018-05-03T18:28:19Z

src/script/ismine.cpp

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;.

Good idea, done.

sipa · 2018-05-03T18:43:24Z

@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.

jimpo · 2018-05-03T20:43:34Z

utACK 6472198

laanwj · 2018-05-10T15:47:56Z

src/script/ismine.cpp

+void Update(IsMineResult& val, IsMineResult update)
 {
-    isInvalid = false;
+    if (update > val) val = update;


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);

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

Empact · 2018-05-21T07:14:01Z

src/script/ismine.cpp

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.

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.

promag · 2018-05-24T13:10:15Z

src/script/ismine.cpp

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

Done, added some comments.

promag · 2018-05-25T14:24:57Z

utACK c004ffc.

Empact · 2018-05-25T18:35:41Z

src/wallet/wallet.cpp

-            bool fSpendableIn = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (coinControl && coinControl->fAllowWatchOnly && (mine & ISMINE_WATCH_SOLVABLE) != ISMINE_NO);
-            bool fSolvableIn = (mine & (ISMINE_SPENDABLE | ISMINE_WATCH_SOLVABLE)) != ISMINE_NO;
+            bool solvable = IsSolvable(*this, pcoin->tx->vout[i].scriptPubKey);
+            bool spendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (((mine & ISMINE_WATCH_ONLY) != ISMINE_NO) && (coinControl && coinControl->fAllowWatchOnly && solvable));


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.

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

Empact · 2018-05-25T18:39:16Z

utACK c004ffc

laanwj · 2018-05-29T13:11:46Z

utACK c004ffc

c004ffc Make handling of invalid in IsMine more uniform (Pieter Wuille) a53f0fe Add some checks for invalid recursion in IsMine (Pieter Wuille) b5802a9 Simplify IsMine logic (Pieter Wuille) 4e91820 Make IsMine stop distinguishing solvable/unsolvable (Pieter Wuille) 6d714c3 Make coincontrol use IsSolvable to determine solvability (Pieter Wuille) Pull request description: 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). Tree-SHA512: 95a6ef75fbf2eedc5ed938c48a8e5d77dcf09c933372acdd0333129fb7301994a78498f9aacce2c8db74275e19260549dd67a83738e187d40b5090cc04f33adf

bb582a5 Add P2WSH destination helper and use it instead of manual hashing (Pieter Wuille) eaba1c1 Add additional unit tests for invalid IsMine combinations (Pieter Wuille) e6b9730 Do not expose invalidity from IsMine (Pieter Wuille) Pull request description: This improves the handling of INVALID in IsMine: * Extra INVALID conditions were added to `IsMine` (following https://github.com/bitcoin/bitcoin/pull/13142/files#r185349057), but these were untested. Add unit tests for them. * In #13142 (comment) it was suggested to merge `isInvalid` into the return status. This PR takes a different approach, and removes the `isInvalid` entirely. It was only ever used inside tests, as normal users of IsMine don't care about the reason for non-mine-ness, only whether it is or not. As the unit tests are extensive enough, it seems sufficient to have a black box text (with tests for both compressed and uncompressed keys). Some addition code simplification is done as well. Tree-SHA512: 3267f8846f3fa4e994f57504b155b0e1bbdf13808c4c04dab7c6886c2c0b88716169cee9c5b350513297e0ca2a00812e3401acf30ac9cde5d892f9fb59ad7fef

…values e02edd3 Cleaning isminetype::ISMINE_SPENDABLE_STAKEABLE. (furszy) 1bf9dc4 wallet: unify GetUnspentCredit with GetCredit (furszy) 9f2ae47 wallet: remove currently unused UpdateAmount method. (furszy) bef69d3 wallet: add cachable amounts for caching credit/debit values (furszy) 8f2a156 Make IsMine stop distinguishing solvable/unsolvable (furszy) e541593 Make coincontrol use IsSolvable to determine solvability (furszy) e1a0cfb Back port of IsSolvable to check scripts solvability. (furszy) Pull request description: Built on top of #1755 and #1666 . The goal of this PR has been to improve the `CWalletTx` to be able to add shielded cached balances in a much cleaner way. Including the following changes: * Adapted IsSolvable function from 0c8ea63 (without the witness check. Only used to check regular outputs to be able to decouple the output solvability from `ismine.h` enum) * Solvability decoupled from `ismine.h`. Back ported 6d714c3 and 4e91820 from bitcoin#13142. * And lastly, adapted bitcoin#15780 beauty cleanup to our sources. ACKs for top commit: random-zebra: utACK e02edd3 Tree-SHA512: 3c54397585f2ce1a47a5a86292216b5beab6cbf1e4ad6c289a1daa958cf4c535855bac1e13a7de290bd8d374abef3da7a909b11ee0f5016af31e90f642b448d5

c004ffc Make handling of invalid in IsMine more uniform (Pieter Wuille) a53f0fe Add some checks for invalid recursion in IsMine (Pieter Wuille) b5802a9 Simplify IsMine logic (Pieter Wuille) 4e91820 Make IsMine stop distinguishing solvable/unsolvable (Pieter Wuille) 6d714c3 Make coincontrol use IsSolvable to determine solvability (Pieter Wuille) Pull request description: 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). Tree-SHA512: 95a6ef75fbf2eedc5ed938c48a8e5d77dcf09c933372acdd0333129fb7301994a78498f9aacce2c8db74275e19260549dd67a83738e187d40b5090cc04f33adf

bb582a5 Add P2WSH destination helper and use it instead of manual hashing (Pieter Wuille) eaba1c1 Add additional unit tests for invalid IsMine combinations (Pieter Wuille) e6b9730 Do not expose invalidity from IsMine (Pieter Wuille) Pull request description: This improves the handling of INVALID in IsMine: * Extra INVALID conditions were added to `IsMine` (following https://github.com/bitcoin/bitcoin/pull/13142/files#r185349057), but these were untested. Add unit tests for them. * In bitcoin#13142 (comment) it was suggested to merge `isInvalid` into the return status. This PR takes a different approach, and removes the `isInvalid` entirely. It was only ever used inside tests, as normal users of IsMine don't care about the reason for non-mine-ness, only whether it is or not. As the unit tests are extensive enough, it seems sufficient to have a black box text (with tests for both compressed and uncompressed keys). Some addition code simplification is done as well. Tree-SHA512: 3267f8846f3fa4e994f57504b155b0e1bbdf13808c4c04dab7c6886c2c0b88716169cee9c5b350513297e0ca2a00812e3401acf30ac9cde5d892f9fb59ad7fef

theuni reviewed May 1, 2018

View reviewed changes

sipa force-pushed the 201804_separate_ismine_solvable branch from a566712 to ae572b3 Compare May 1, 2018 22:44

theuni reviewed May 2, 2018

View reviewed changes

meshcollider added Refactoring Wallet labels May 2, 2018

promag reviewed May 2, 2018

View reviewed changes

jimpo reviewed May 3, 2018

View reviewed changes

sipa added 4 commits May 3, 2018 11:01

Make coincontrol use IsSolvable to determine solvability

6d714c3

Make IsMine stop distinguishing solvable/unsolvable

4e91820

Simplify IsMine logic

b5802a9

Add some checks for invalid recursion in IsMine

a53f0fe

sipa force-pushed the 201804_separate_ismine_solvable branch from 4662113 to d5c79f8 Compare May 3, 2018 18:03

jimpo reviewed May 3, 2018

View reviewed changes

sipa force-pushed the 201804_separate_ismine_solvable branch from d5c79f8 to 6472198 Compare May 3, 2018 18:40

laanwj reviewed May 10, 2018

View reviewed changes

sipa force-pushed the 201804_separate_ismine_solvable branch from 6472198 to 67e88f2 Compare May 11, 2018 19:36

Empact reviewed May 21, 2018

View reviewed changes

promag reviewed May 24, 2018

View reviewed changes

Make handling of invalid in IsMine more uniform

c004ffc

sipa force-pushed the 201804_separate_ismine_solvable branch from 67e88f2 to c004ffc Compare May 24, 2018 17:29

Empact approved these changes May 25, 2018

View reviewed changes

laanwj merged commit c004ffc into bitcoin:master May 29, 2018

Bushstar mentioned this pull request Jun 1, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#340

Merged

sipa mentioned this pull request Jun 18, 2018

Improve handling of INVALID in IsMine #13491

Merged

HashUnlimited mentioned this pull request Sep 11, 2018

Improve handling of INVALID in IsMine chaincoin/chaincoin#477

Merged

furszy mentioned this pull request Jul 21, 2020

[Wallet] add cacheable amounts for caching credit/debit values PIVX-Project/PIVX#1757

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Separate IsMine from solvability #13142

Separate IsMine from solvability #13142

Uh oh!

Conversation

sipa commented May 1, 2018

Uh oh!

gmaxwell commented May 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni commented May 1, 2018

Uh oh!

sipa commented May 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni commented May 2, 2018

Uh oh!

laanwj commented May 2, 2018

Uh oh!

meshcollider commented May 2, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimpo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented May 3, 2018

Uh oh!

jimpo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimpo commented May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Empact May 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented May 25, 2018

Uh oh!

Choose a reason for hiding this comment

sipa commented May 3, 2018 •

edited

Loading

jimpo commented May 3, 2018 •

edited

Loading

Empact May 21, 2018 •

edited

Loading