wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition #18067

ryanofsky · 2020-02-04T22:44:28Z

Make LegacyScriptPubKeyMan::CanProvide method able to recognize p2sh scripts when the redeem script is present in the mapScripts map without the p2sh script also having to be added to the mapScripts map. This restores behavior prior to #17261, which I think broke backwards compatibility with old wallet files by no longer treating addresses created by addmultisigaddress calls before #17261 as solvable.

The reason why tests didn't fail with the CanProvide implementation in #17261 is because of a workaround added in 4a7e43e "Store p2sh scripts in AddAndGetDestinationForScript", which masked the problem for new addmultisigaddress RPC calls without fixing it for multisig addresses already created in old wallet files.

This change adds a lot of comments and allows reverting commit 4a7e43e "Store p2sh scripts in AddAndGetDestinationForScript", so the AddAndGetDestinationForScript() function, CanProvide() method, and mapScripts map should all be more comprehensible

achow101 · 2020-02-10T17:23:11Z

ACK 891b0ee

Confirms this fixes #18075

Sjors · 2020-02-11T20:26:48Z

Tested ACK 891b0ee. Thanks for adding all the clarifications and scriptpubkeyman_tests.

So IIUC the reason we didn't see addresses added with addmultisigaddress in older wallets, is that AddAndGetMultisigDestination calls AddAndGetDestinationForScript which in turn called AddCScript on the multisig p2sh script, but not the public keys and private keys. Without those keys it's considered ISMINE_NO. When CanProvide looks at the P2SH address, it calls IsMine which calls IsMineInner with recurse_scripthash = true. Starting at the TOP level IsMineInner it identifies the address as TX_SCRIPTHASH. It would then try to find the script and run it through IsMineInner at the IsMineSigVersion::P2SH level. There it would be recognised as a TX_MULTISIG, which requires all (private) keys, but we have none (or maybe one in a more realistic wallet).

I'm still not entirely clear what the workaround in 4a7e43e did other than "pass the tests", but it clearly doesn't solve the above, since it still doesn't add (private) keys.

In this PR CanProvide uses recurse_scripthash = false in IsMineInner, and we modified that to consider a TX_SCRIPTHASH or TX_WITNESS_V0_SCRIPTHASH to be SPENDABLE.

Meanwhile the RPC uses pwallet->IsMine(dest) which uses the original "need all keys" behaviour under recurse_scripthash = true. So we slightly expand what's covered by CanProvide without adding stuff to the wallet.

@sipa does this many any sense?

maflcko · 2020-02-12T14:21:12Z

Would be nice to rebase and cherry-pick Sjors/bitcoin@bacf559 into this

Make LegacyScriptPubKeyMan::CanProvide method able to recognize p2sh scripts when the redeem script is present in the mapScripts map without the p2sh script also having to be added to the mapScripts map. This restores behavior prior to bitcoin#17261, which I think broke backwards compatibility with old wallet files by no longer treating addresses created by `addmultisigaddress` calls before bitcoin#17261 as solvable. The reason why tests didn't fail with the CanProvide implementation in bitcoin#17261 is because of a workaround added in 4a7e43e "Store p2sh scripts in AddAndGetDestinationForScript", which masked the problem for new `addmultisigaddress` RPC calls without fixing it for multisig addresses already created in old wallet files. This change adds a lot of comments and allows reverting commit 4a7e43e "Store p2sh scripts in AddAndGetDestinationForScript", so the AddAndGetDestinationForScript() function, CanProvide() method, and mapScripts map should all be more comprehensible

This reverts commit 4a7e43e.

ryanofsky · 2020-02-12T18:39:58Z

Rebased 891b0ee -> a304a36 (pr/provide.1 -> pr/provide.2, compare) on top of #12134, also editing the mapScripts comment and cherry-picking Sjors' test commit

Thank you Sjors for the great test case and bug description in #18067 (comment)! I'm really glad to see the test framework in #12134 merged, too.

re: #18067 (comment)

I'm still not entirely clear what the workaround in 4a7e43e did other than "pass the tests", but it clearly doesn't solve the above, since it still doesn't add (private) keys.

The reason why the workaround in 4a7e43e was able to mask the problem is because of this condition in LegacyScriptPubKeyMan::CanProvide:

bitcoin/src/wallet/scriptpubkeyman.cpp

Lines 482 to 484 in 2bdc476

    
           } else if (HaveCScript(CScriptID(script))) { 
        
               // We can still provide some stuff if we have the script, but IsMine failed because we don't have keys 
        
               return true;

The first commit in this PR drops this condition so it is breaking the workaround at the same time as making the workaround unnecessary by fixing the bug.

I was a little on the fence about whether to remove this condition, but I think it's better not to have it, so CanProvide just wraps the IsMine and ProduceSignature code instead of trying to be its own third IsMine implementation.

Sjors · 2020-02-12T19:54:43Z

re-ACK a304a36 (rebase, slight text changes and my test)

achow101 · 2020-02-12T20:23:10Z

re-ACK a304a36

meshcollider

utACK a304a36

…t recognition a304a36 Revert "Store p2sh scripts in AddAndGetDestinationForScript" (Russell Yanofsky) eb7d8a5 [test] check for addmultisigaddress regression (Sjors Provoost) 005f8a9 wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition (Russell Yanofsky) Pull request description: Make `LegacyScriptPubKeyMan::CanProvide` method able to recognize p2sh scripts when the redeem script is present in the `mapScripts` map without the p2sh script also having to be added to the `mapScripts` map. This restores behavior prior to #17261, which I think broke backwards compatibility with old wallet files by no longer treating addresses created by `addmultisigaddress` calls before #17261 as solvable. The reason why tests didn't fail with the CanProvide implementation in #17261 is because of a workaround added in 4a7e43e "Store p2sh scripts in AddAndGetDestinationForScript", which masked the problem for new `addmultisigaddress` RPC calls without fixing it for multisig addresses already created in old wallet files. This change adds a lot of comments and allows reverting commit 4a7e43e "Store p2sh scripts in AddAndGetDestinationForScript", so the `AddAndGetDestinationForScript()` function, `CanProvide()` method, and `mapScripts` map should all be more comprehensible ACKs for top commit: Sjors: re-ACK a304a36 (rebase, slight text changes and my test) achow101: re-ACK a304a36 meshcollider: utACK a304a36 Tree-SHA512: 03b625220c49684c376a8062d7646aeba0e5bfe043f977dc7dc357a6754627d594e070e4d458d12d2291888405d94c1dbe08c7787c318374cedd5755e724fb6e

…e script recognition a304a36 Revert "Store p2sh scripts in AddAndGetDestinationForScript" (Russell Yanofsky) eb7d8a5 [test] check for addmultisigaddress regression (Sjors Provoost) 005f8a9 wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition (Russell Yanofsky) Pull request description: Make `LegacyScriptPubKeyMan::CanProvide` method able to recognize p2sh scripts when the redeem script is present in the `mapScripts` map without the p2sh script also having to be added to the `mapScripts` map. This restores behavior prior to bitcoin#17261, which I think broke backwards compatibility with old wallet files by no longer treating addresses created by `addmultisigaddress` calls before bitcoin#17261 as solvable. The reason why tests didn't fail with the CanProvide implementation in bitcoin#17261 is because of a workaround added in 4a7e43e "Store p2sh scripts in AddAndGetDestinationForScript", which masked the problem for new `addmultisigaddress` RPC calls without fixing it for multisig addresses already created in old wallet files. This change adds a lot of comments and allows reverting commit 4a7e43e "Store p2sh scripts in AddAndGetDestinationForScript", so the `AddAndGetDestinationForScript()` function, `CanProvide()` method, and `mapScripts` map should all be more comprehensible ACKs for top commit: Sjors: re-ACK a304a36 (rebase, slight text changes and my test) achow101: re-ACK a304a36 meshcollider: utACK a304a36 Tree-SHA512: 03b625220c49684c376a8062d7646aeba0e5bfe043f977dc7dc357a6754627d594e070e4d458d12d2291888405d94c1dbe08c7787c318374cedd5755e724fb6e

…ript recognition Summary: a304a3632f0437f4d0f04589a2200e2da91624a7 Revert "Store p2sh scripts in AddAndGetDestinationForScript" (Russell Yanofsky) eb7d8a5b07e89133a5fb465ad1b793362e7439f7 [test] check for addmultisigaddress regression (Sjors Provoost) 005f8a92ccb5bc10c8daa106d75e1c21390461d3 wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition (Russell Yanofsky) Pull request description: Make `LegacyScriptPubKeyMan::CanProvide` method able to recognize p2sh scripts when the redeem script is present in the `mapScripts` map without the p2sh script also having to be added to the `mapScripts` map. This restores behavior prior to #17261, which I think broke backwards compatibility with old wallet files by no longer treating addresses created by `addmultisigaddress` calls before #17261 as solvable. The reason why tests didn't fail with the CanProvide implementation in #17261 is because of a workaround added in 4a7e43e8460127a40a7895519587399feff3b682 "Store p2sh scripts in AddAndGetDestinationForScript", which masked the problem for new `addmultisigaddress` RPC calls without fixing it for multisig addresses already created in old wallet files. This change adds a lot of comments and allows reverting commit 4a7e43e8460127a40a7895519587399feff3b682 "Store p2sh scripts in AddAndGetDestinationForScript", so the `AddAndGetDestinationForScript()` function, `CanProvide()` method, and `mapScripts` map should all be more comprehensible --- Backport of Core [[bitcoin/bitcoin#18067 | PR18067]] Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Subscribers: PiRK Differential Revision: https://reviews.bitcoinabc.org/D9115

fanquake added the Wallet label Feb 4, 2020

fanquake requested a review from achow101 February 4, 2020 22:45

ryanofsky mentioned this pull request Feb 5, 2020

wallet: #17261 regression: addmultisigaddress addresses not treated as solvable #18075

Closed

achow101 approved these changes Feb 10, 2020

View reviewed changes

fanquake requested a review from meshcollider February 11, 2020 00:30

This was referenced Feb 11, 2020

Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) #16341

Closed

Build previous releases and run functional tests #12134

Merged

ryanofsky and others added 3 commits February 12, 2020 11:48

[test] check for addmultisigaddress regression

eb7d8a5

Revert "Store p2sh scripts in AddAndGetDestinationForScript"

a304a36

This reverts commit 4a7e43e.

ryanofsky force-pushed the pr/provide branch from 891b0ee to a304a36 Compare February 12, 2020 18:19

meshcollider reviewed Feb 18, 2020

View reviewed changes

meshcollider merged commit a304a36 into bitcoin:master Feb 19, 2020

instagibbs mentioned this pull request Mar 2, 2020

Update firmware build scripts and tests to work with new firmwares bitcoin-core/HWI#311

Merged

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition #18067

wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition #18067

Uh oh!

ryanofsky commented Feb 4, 2020 •

edited by fanquake

Loading

Uh oh!

achow101 commented Feb 10, 2020

Uh oh!

Sjors commented Feb 11, 2020 •

edited

Loading

Uh oh!

maflcko commented Feb 12, 2020

Uh oh!

ryanofsky commented Feb 12, 2020

Uh oh!

Sjors commented Feb 12, 2020

Uh oh!

achow101 commented Feb 12, 2020

Uh oh!

meshcollider left a comment

Uh oh!

Uh oh!

wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition #18067

wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition #18067

Uh oh!

Conversation

ryanofsky commented Feb 4, 2020 • edited by fanquake Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Feb 10, 2020

Uh oh!

Sjors commented Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Feb 12, 2020

Uh oh!

ryanofsky commented Feb 12, 2020

Uh oh!

Sjors commented Feb 12, 2020

Uh oh!

achow101 commented Feb 12, 2020

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky commented Feb 4, 2020 •

edited by fanquake

Loading

Sjors commented Feb 11, 2020 •

edited

Loading