Improve handling of INVALID in IsMine #13491

pull sipa wants to merge 3 commits into bitcoin:master from sipa:201806_cleanmine changing 7 files +125 −161

sipa commented at 1:00 AM on June 18, 2018: member
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.
Do not expose invalidity from IsMine e6b9730c49
Add additional unit tests for invalid IsMine combinations eaba1c111e
MarcoFalke added the label Refactoring on Jun 18, 2018
MarcoFalke added the label Wallet on Jun 18, 2018
sipa added the label Tests on Jun 18, 2018
Add P2WSH destination helper and use it instead of manual hashing bb582a59c7
DrahtBot commented at 10:39 PM on June 19, 2018: member
Note to reviewers: This pull request conflicts with the following ones:
- #13072 (Update createmultisig RPC to support segwit by ajtowns)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
promag commented at 1:35 PM on June 20, 2018: member

Concept ACK.

users of IsMine don't care about the reason for non-mine-ness, only whether it is or not

👍
promag commented at 2:10 PM on June 26, 2018: member

utACK bb582a5. Commits are clean, refactor and new tests LGTM (some nits aside).
in src/test/script_standard_tests.cpp:492 in bb582a59c7
```
 523 | +
 524 | +        CScript redeemscript_inner = GetScriptForDestination(pubkeys[0].GetID());
 525 | +        CScript redeemscript = GetScriptForDestination(CScriptID(redeemscript_inner));
 526 | +        scriptPubKey = GetScriptForDestination(CScriptID(redeemscript));
 527 | +
 528 | +        keystore.AddCScript(redeemscript);
```
promag commented at 2:38 PM on June 26, 2018:

The test passes if these keystore.Add* are removed. How could this be updated so that these are meaningful?

sipa commented at 11:50 PM on June 26, 2018:

They are meaningful. The tests verifies that even when all scripts are known the output isn't treated as ours. The positive test case is the variant without 2 nested P2SHs, where adding all scripts does result in treating the output as ours.
promag commented at 2:42 PM on June 27, 2018:

I understand. I was thinking in adding:

result = IsMine(keystore, redeemscript); BOOST_CHECK_EQUAL(result, ISMINE_SPENDABLE);

So that these keystore.Add* make sense and can't be removed. Feel free to ignore.
sipa added this to the "Blockers" column in a project

jimpo commented at 6:28 PM on July 2, 2018: contributor

utACK bb582a5.

in src/rpc/rawtransaction.cpp:639 in bb582a59c7

 636 | @@ -637,9 +637,7 @@ static UniValue decodescript(const JSONRPCRequest& request)
 637 |              } else {
 638 |                  // Scripts that are not fit for P2WPKH are encoded as P2WSH.
 639 |                  // Newer segwit program versions should be considered when then become available.

Empact commented at 8:42 PM on July 2, 2018:

nit: existing typo they

Empact commented at 8:55 PM on July 2, 2018: member

utACK bb582a5
jb55 approved
jb55 commented at 5:25 PM on July 3, 2018: member

utACK bb582a59c7532b0e4f647d9dfe50f0d816e81427 seems reasonable
laanwj commented at 9:36 AM on July 4, 2018: member

utACK bb582a59c7532b0e4f647d9dfe50f0d816e81427
laanwj merged this on Jul 4, 2018
laanwj closed this on Jul 4, 2018
laanwj referenced this in commit 61a044a86a on Jul 4, 2018
fanquake removed this from the "Blockers" column in a project

deadalnix referenced this in commit 71905e7247 on Jun 5, 2020
ftrader referenced this in commit e5d2194797 on Aug 17, 2020
UdjinM6 referenced this in commit e6bb688cd4 on Jun 20, 2021
UdjinM6 referenced this in commit da6f2dc0d5 on Jun 24, 2021
UdjinM6 referenced this in commit 2eff4342c6 on Jun 26, 2021
UdjinM6 referenced this in commit 8cda3f374b on Jun 26, 2021
UdjinM6 referenced this in commit d860dcaa50 on Jun 26, 2021
UdjinM6 referenced this in commit a53599df24 on Jun 28, 2021
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Refactoring Wallet Tests

Linked (view graph)

#13142 Separate IsMine from solvability