rpc: Only allow specific types to be P2(W)SH wrapped in decodescript

MarcoFalke commented at 4:22 pm on November 11, 2021: member

It seems confusing to return a P2SH wrapping address that is eventually either policy- or consensus-unspendable.

in src/rpc/rawtransaction.cpp:555 in 8888154aae outdated

571+            RPCResult::Type::OBJ, "", "",
572+            {
573+                {RPCResult::Type::STR, "asm", "Script public key"},
574+                {RPCResult::Type::STR, "type", "The output type (e.g. " + GetAllOutputTypes() + ")"},
575+                {RPCResult::Type::STR, "address", /* optional */ true, "The Bitcoin address (only if a well-defined address exists)"},
576+                {RPCResult::Type::STR, "p2sh", /*optional=*/true,

katesalazar commented at 4:26 pm on November 11, 2021:

Why this line departs from the comment style around?

MarcoFalke commented at 4:28 pm on November 11, 2021:

I updated the style for the p2sh field because I changed the description for it. See #23437 (review)

MarcoFalke force-pushed on Nov 11, 2021

DrahtBot added the label RPC/REST/ZMQ on Nov 11, 2021

Sjors commented at 6:48 pm on November 11, 2021: member

Concept ACK. For slightly easier review, maybe split into a pure refactor/reformat commit and the actual behavior change.

MarcoFalke commented at 7:09 pm on November 11, 2021: member

If you pass in --ignore-all-space, it will show only the behaviour changes. I think it makes sense to adjust the whitespace in the same commit that removes the if-body.

DrahtBot commented at 3:04 am on November 12, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23545 (scripted-diff: Use clang-tidy syntax for C++ named arguments [WIP; TOO MANY CONFLICTS] by MarcoFalke)

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.

laanwj commented at 9:54 am on November 12, 2021: member

I agree with @Sjors here. I think it makes sense to split the functionality change into a separate commit. Even if it can still be reviewed by providing extra arguments to diff, it just seems cleaner, say, when browsing git history later.

Does this need a functional test?

MarcoFalke force-pushed on Nov 12, 2021

laanwj commented at 12:08 pm on November 12, 2021: member

Code review ACK fa1c1f417ef97d15331f4a13b40be9e00ecdb464

Code review re-ACK fa972d8d771c93c1d37eeb31da04e95e8e5ca0ad

MarcoFalke commented at 3:13 pm on November 12, 2021: member

Does this need a functional test?

There are steps to test in the first comment, but I am happy to add a commit with tests if someone writes them. I am also happy to review a pull if someone adds them after merge.

Sjors commented at 3:36 pm on November 12, 2021: member

tACK fa1c1f417ef97d15331f4a13b40be9e00ecdb464

in src/rpc/rawtransaction.cpp:583 in fa1c1f417e outdated

579@@ -578,9 +580,8 @@ static RPCHelpMan decodescript()
580 
581     const std::string type{find_value(r, "type").get_str()};
582 
583-    if (type == "scripthash") {
584-        // P2SH cannot be wrapped in a P2SH. If this script is already a P2SH,
585-        // don't return the address for a P2SH of the P2SH.
586+    if (type == "scripthash" || type == "witness_v1_taproot") {

achow101 commented at 8:45 pm on November 12, 2021:

In fa1c1f417ef97d15331f4a13b40be9e00ecdb464 “rpc: Avoid returning P2SH for witness_v1_taproot in decodescript”

I think it would be more future proof to disallow P2SH wrapping for witness v1+ scripts. If this did IsWitnessProgram and checked the version was not 0, then this could be futureproofed. Otherwise this list would need to be updated for every new witness version). Doing it in this way would also allow passing a higher witness version script to decodescript and not get a p2sh wrapped result.

MarcoFalke commented at 8:43 am on November 14, 2021:

Good point. It is unlikely that a future witness version will be wrappable, so the least maintenance effort would be to disallow all except v0.

MarcoFalke commented at 10:44 am on November 14, 2021:

I am thinking about also skipping P2SH for OP_RETURN data outputs

achow101 commented at 3:28 pm on November 14, 2021:

I am thinking about also skipping P2SH for OP_RETURN data outputs

I think that is reasonable as OP_RETURN in a P2SH is also unspendable.

MarcoFalke commented at 5:20 pm on November 14, 2021:

Thanks, done.

MarcoFalke force-pushed on Nov 14, 2021

MarcoFalke renamed this:
~~rpc: Avoid returning P2SH for witness_v1_taproot in decodescript~~
rpc: Only return P2SH wrapped witness programs for BIP141 types in decodescript
on Nov 14, 2021

MarcoFalke force-pushed on Nov 14, 2021

in src/rpc/rawtransaction.cpp:584 in b9cc2008ec outdated

578@@ -578,10 +579,10 @@ static RPCHelpMan decodescript()
579 
580     std::vector<std::vector<unsigned char>> solutions_data;
581     const TxoutType which_type{Solver(script, solutions_data)};
582+    const bool is_witness_v0{which_type == TxoutType::WITNESS_V0_KEYHASH || which_type == TxoutType::WITNESS_V0_SCRIPTHASH};
583 
584-    if (which_type == TxoutType::SCRIPTHASH) {
585-        // P2SH cannot be wrapped in a P2SH. If this script is already a P2SH,
586-        // don't return the address for a P2SH of the P2SH.
587+    if (which_type == TxoutType::SCRIPTHASH || !is_witness_v0) {

achow101 commented at 3:35 pm on November 14, 2021:

In b9cc2008ecfe823da840dd1c321aa298f8c8bef2 “rpc: Only return P2SH wrapped witness programs for BIP141 types in de… "

This doesn’t work as it is also catching all of the legacy things that can be wrapped such as multisig, p2pkh, p2pk, etc.

I think it would be better to just enumerate lists of P2SH wrappable, and P2WSH wrappable, and check against those. I doubt any additional types would be added to those lists in the future.

MarcoFalke commented at 5:20 pm on November 14, 2021:

Fixed

MarcoFalke renamed this:
~~rpc: Only return P2SH wrapped witness programs for BIP141 types in decodescript~~
rpc: Only allow specific types to be P2SH wrapped in decodescript
on Nov 14, 2021

MarcoFalke force-pushed on Nov 14, 2021

MarcoFalke commented at 5:21 pm on November 14, 2021: member

Reworked and updated pull request description

in src/rpc/rawtransaction.cpp:637 in fa972d8d77 outdated

657+        segwitScr = GetScriptForDestination(WitnessV0KeyHash(Hash160(solutions_data[0])));
658+    } else if (which_type == TxoutType::PUBKEYHASH) {
659+        segwitScr = GetScriptForDestination(WitnessV0KeyHash(uint160{solutions_data[0]}));
660+    } else {
661+        // Scripts that are not fit for P2WPKH are encoded as P2WSH.
662+        // Newer segwit program versions should be considered when then become available.

sipa commented at 4:49 pm on November 15, 2021:

Typo: s/then/they/

MarcoFalke commented at 7:19 pm on November 16, 2021:

Thx, removed whole sentence.

sipa commented at 4:51 pm on November 15, 2021: member

We may also need to check that no OP_CHECKSIGADD opcodes occur in a script before reporting a P2WSH for it.

achow101 commented at 5:09 pm on November 15, 2021: member

We may also need to check that no OP_CHECKSIGADD opcodes occur in a script before reporting a P2WSH for it.

And OP_SUCCESSx too

MarcoFalke renamed this:
~~rpc: Only allow specific types to be P2SH wrapped in decodescript~~
rpc: Only allow specific types to be P2(W)SH wrapped in decodescript
on Nov 16, 2021

MarcoFalke force-pushed on Nov 16, 2021

MarcoFalke commented at 7:20 pm on November 16, 2021: member

And OP_SUCCESSx too

And unparseable, and unspendable scripts.

in src/rpc/rawtransaction.cpp:604 in fa20010423 outdated

605+    }
606+    for (CScript::const_iterator it{script.begin()}; it != script.end();) {
607+        opcodetype op;
608+        CHECK_NONFATAL(script.GetOp(it, op));
609+        if (op == OP_CHECKSIGADD || IsOpSuccess(op)) {
610+            return false;

achow101 commented at 6:22 pm on November 17, 2021:

In fa200104235398a0c507122b95e0899022650be0 “rpc: Only allow specific types to be P2(W)SH wrapped in decodescript”

Returning false here is incorrect. If a script contained Taproot opcodes, we still want to decode them, we just don’t want to get it wrapped in any addresses.

This also results in the following error scripts containing OP_CHECKSIGADD and OP_SUCCESSx:

0> src/bitcoin-cli -regtest decodescript 20393ea2d59e5565796ef02e1f91d5efe0577be63e5cf460503009c01d368c7aebba
1error code: -1
2error message:
3rpc/util.cpp:577 (HandleRequest)
4Internal bug detected: 'std::any_of(m_results.m_results.begin(), m_results.m_results.end(), [ret](const RPCResult& res) { return res.MatchesType(ret); })'
5You may report this issue here: https://github.com/bitcoin/bitcoin/issues

MarcoFalke commented at 12:54 pm on November 22, 2021:

Well that is one embarrassing typo. Force pushed return r;.

in src/rpc/rawtransaction.cpp:601 in fa20010423 outdated

599+    case TxoutType::WITNESS_V1_TAPROOT:
600+        // Should not be wrapped, so return early
601         return r;
602     }
603+    if (!script.HasValidOps() || script.IsUnspendable()) {
604+        return false;

achow101 commented at 6:28 pm on November 17, 2021:

In fa200104235398a0c507122b95e0899022650be0 “rpc: Only allow specific types to be P2(W)SH wrapped in decodescript”

Returning false here is incorrect. If a script contained invalid opcodes or is otherwise unspendable, we still want to decode them, we just don’t want to get it wrapped in any addresses

Any such scripts would result in an error similar to the following:

0> src/bitcoin-cli -regtest decodescript 20393ea2d59e5565796ef02e1f91d5efe0577be63e5cf460503009c01d368c7aebba
1error code: -1
2error message:
3rpc/util.cpp:577 (HandleRequest)
4Internal bug detected: 'std::any_of(m_results.m_results.begin(), m_results.m_results.end(), [ret](const RPCResult& res) { return res.MatchesType(ret); })'
5You may report this issue here: https://github.com/bitcoin/bitcoin/issues

There appears to be a separate bug(?) where OP_SUCESSx and OP_CHECKSIGADD are being caught by this condition too.

MarcoFalke commented at 12:54 pm on November 22, 2021:

Force pushed return r;.

achow101 commented at 6:30 pm on November 17, 2021: member

Even without the two issues mentioned below, it seems that decodescript is unable to decode scripts containing OP_CHECKSIGADD and OP_SUCCESSx. But I think fixing those would be out of scope for this PR.

MarcoFalke force-pushed on Nov 22, 2021

MarcoFalke commented at 12:55 pm on November 22, 2021: member

Force pushed to fix a “typo” :confounded: and updated OP to include more tests.

MarcoFalke force-pushed on Nov 22, 2021

in src/rpc/rawtransaction.cpp:596 in fa0aec30e2 outdated

622+    case TxoutType::WITNESS_V0_SCRIPTHASH:
623+        // Can be wrapped
624+        break;
625+    case TxoutType::NULL_DATA:
626+    case TxoutType::SCRIPTHASH:
627+    case TxoutType::WITNESS_UNKNOWN:

luke-jr commented at 7:31 pm on November 30, 2021:

Isn’t it TBD if future witnesses can be wrapped? Though maybe too unlikely to matter…

MarcoFalke commented at 7:39 pm on November 30, 2021:

It is possible but unlikely. In that case you need to upgrade to a version of Bitcoin Core that “understands” the witness program to wrap it.

in src/rpc/rawtransaction.cpp:599 in fa0aec30e2 outdated

625+    case TxoutType::NULL_DATA:
626+    case TxoutType::SCRIPTHASH:
627+    case TxoutType::WITNESS_UNKNOWN:
628+    case TxoutType::WITNESS_V1_TAPROOT:
629+        // Should not be wrapped, so return early
630+        return r;

luke-jr commented at 7:32 pm on November 30, 2021:

Not sure this “return early” is a good approach. Seems like it would be better to have a conditional block.

MarcoFalke commented at 1:59 pm on December 2, 2021:

Thanks, fixed.

MarcoFalke force-pushed on Dec 1, 2021

MarcoFalke force-pushed on Dec 2, 2021

rpc: Only allow specific types to be P2(W)SH wrapped in decodescript 99993425af

MarcoFalke force-pushed on Dec 6, 2021

MarcoFalke commented at 9:33 am on December 6, 2021: member

Rebased to add tests :sparkles:

laanwj commented at 3:55 pm on December 6, 2021: member

Code review re-ACK 99993425afc2c352b26e678b7ffbc74362ac3527 It’s nice to be able to see in the .json what exactly the difference in output is.

in test/functional/data/rpc_decodescript.json:45 in 99993425af

42         }
43     ],
44     [
45         "6aee",
46         {
47             "asm": "OP_RETURN OP_UNKNOWN",

laanwj commented at 4:01 pm on December 6, 2021:

(not in this PR) it would be nice to have some test entries that do give segwit output

MarcoFalke commented at 4:04 pm on December 6, 2021:

good first issue ;)

laanwj merged this on Dec 6, 2021

laanwj closed this on Dec 6, 2021

MarcoFalke deleted the branch on Dec 7, 2021

sidhujag referenced this in commit b6843f1a12 on Dec 7, 2021

RandyMcMillan referenced this in commit 59a7815a7e on Dec 23, 2021

DrahtBot locked this on Dec 23, 2022

rpc: Only allow specific types to be P2(W)SH wrapped in decodescript #23486

Conflicts