wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition

ryanofsky commented at 10:44 pm on February 4, 2020: member

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

fanquake added the label Wallet on Feb 4, 2020

fanquake requested review from achow101 on Feb 4, 2020

achow101 commented at 5:23 pm on February 10, 2020: member

ACK 891b0ee0167dbc7c3d149da92bf987e18d383fc2

Confirms this fixes #18075

achow101 approved

fanquake requested review from meshcollider on Feb 11, 2020

Sjors commented at 8:26 pm on February 11, 2020: member

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 4a7e43e8460127a40a7895519587399feff3b682 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?

MarcoFalke commented at 2:21 pm on February 12, 2020: member

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

wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition

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
https://github.com/bitcoin/bitcoin/pull/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

005f8a92cc

[test] check for addmultisigaddress regression eb7d8a5b07

Revert "Store p2sh scripts in AddAndGetDestinationForScript"

This reverts commit 4a7e43e8460127a40a7895519587399feff3b682.

a304a3632f

ryanofsky force-pushed on Feb 12, 2020

ryanofsky commented at 6:39 pm on February 12, 2020: member

Rebased 891b0ee0167dbc7c3d149da92bf987e18d383fc2 -> a304a3632f0437f4d0f04589a2200e2da91624a7 (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 4a7e43e8460127a40a7895519587399feff3b682 was able to mask the problem is because of this condition in LegacyScriptPubKeyMan::CanProvide:

https://github.com/bitcoin/bitcoin/blob/2bdc476d4d23256d8396bb9051a511f540d87392/src/wallet/scriptpubkeyman.cpp#L482-L484

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 commented at 7:54 pm on February 12, 2020: member

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

achow101 commented at 8:23 pm on February 12, 2020: member

re-ACK a304a3632f0437f4d0f04589a2200e2da91624a7

meshcollider commented at 8:54 am on February 18, 2020: contributor

utACK a304a3632f0437f4d0f04589a2200e2da91624a7

meshcollider referenced this in commit 68e841e0af on Feb 19, 2020

meshcollider merged this on Feb 19, 2020

meshcollider closed this on Feb 19, 2020

sidhujag referenced this in commit 3a89bee259 on Feb 19, 2020

sidhujag referenced this in commit 93fde6adc9 on Nov 10, 2020

Fabcien referenced this in commit 20fe6bd3a7 on Feb 2, 2021

DrahtBot locked this on Feb 15, 2022

wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition #18067