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

Why this line departs from the comment style around?

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

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.

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.

I am thinking about also skipping P2SH for OP_RETURN data outputs

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.

Thanks, done.

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.

Fixed

Typo: s/then/they/

Thx, removed whole sentence.

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:

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

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

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:

> src/bitcoin-cli -regtest decodescript 20393ea2d59e5565796ef02e1f91d5efe0577be63e5cf460503009c01d368c7aebba
error code: -1
error message:
rpc/util.cpp:577 (HandleRequest)
Internal bug detected: 'std::any_of(m_results.m_results.begin(), m_results.m_results.end(), [ret](const RPCResult& res) { return res.MatchesType(ret); })'
You 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.

Force pushed return r;.

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

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.

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

Thanks, fixed.

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

good first issue ;)