rpc: Only allow specific types to be P2(W)SH wrapped in decodescript by maflcko · Pull Request #23486 · bitcoin/bitcoin

maflcko · 2021-11-11T16:22:40Z

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

src/rpc/rawtransaction.cpp

Sjors · 2021-11-11T18:48:00Z

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

maflcko · 2021-11-11T19:09:06Z

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 · 2021-11-12T03:04:38Z

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 · 2021-11-12T09:54:28Z

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?

laanwj · 2021-11-12T12:08:28Z

~~Code review ACK fa1c1f417ef97d15331f4a13b40be9e00ecdb464~~

Code review re-ACK fa972d8d771c93c1d37eeb31da04e95e8e5ca0ad

maflcko · 2021-11-12T15:13:07Z

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 · 2021-11-12T15:36:28Z

tACK fa1c1f417ef97d15331f4a13b40be9e00ecdb464

src/rpc/rawtransaction.cpp

maflcko · 2021-11-14T17:21:47Z

Reworked and updated pull request description

src/rpc/rawtransaction.cpp

sipa · 2021-11-15T16:51:02Z

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

achow101 · 2021-11-15T17:09:06Z

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

maflcko · 2021-11-16T19:20:03Z

And OP_SUCCESSx too

And unparseable, and unspendable scripts.

achow101

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.

src/rpc/rawtransaction.cpp

maflcko · 2021-11-22T12:55:39Z

Force pushed to fix a "typo" 😖 and updated OP to include more tests.

luke-jr · 2021-11-30T19:31:23Z

src/rpc/rawtransaction.cpp

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.

src/rpc/rawtransaction.cpp

maflcko · 2021-12-06T09:33:40Z

Rebased to add tests ✨

laanwj · 2021-12-06T15:55:37Z

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

laanwj · 2021-12-06T16:01:18Z

test/functional/data/rpc_decodescript.json

    [
        "6aee",
        {
            "asm": "OP_RETURN OP_UNKNOWN",


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

good first issue ;)

…pped in…

…W)SH wrapped in decodescript 086d811 rpc: Only allow specific types to be P2(W)SH wrapped in decodescript (MarcoFalke) Pull request description: It seems confusing to return a P2SH wrapping address that is eventually either policy- or consensus-unspendable. ACKs for top commit: laanwj: Code review re-ACK 086d811 Tree-SHA512: 3cd530442acee7c295d244995f0f17b2cae7212f1e0970bb5807621f8ff8e4308a3236b385d77087cd493d32ee524813d8edd15e91d937ef9a800094b7bc4946

katesalazar reviewed Nov 11, 2021

View reviewed changes

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

maflcko force-pushed the 2111-rpcScript branch from 8888154 to fa95a43 Compare November 11, 2021 17:11

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

DrahtBot mentioned this pull request Nov 12, 2021

refactor: cleanup/followup of Remove -deprecatedrpc=addresses flag #22924

Closed

maflcko force-pushed the 2111-rpcScript branch from fa95a43 to fa1c1f4 Compare November 12, 2021 11:02

achow101 reviewed Nov 12, 2021

View reviewed changes

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Nov 14, 2021

Refactor: Improve API design of ScriptToUniv, TxToUniv etc to return the UniValue instead of mutating a parameter/reference #23507

Closed

maflcko force-pushed the 2111-rpcScript branch from fa1c1f4 to faa8ea8 Compare November 14, 2021 10:43

maflcko changed the title ~~rpc: Avoid returning P2SH for witness_v1_taproot in decodescript~~ rpc: Only return P2SH wrapped witness programs for BIP141 types in decodescript Nov 14, 2021

maflcko force-pushed the 2111-rpcScript branch from faa8ea8 to b9cc200 Compare November 14, 2021 12:30

achow101 reviewed Nov 14, 2021

View reviewed changes

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

maflcko changed the title ~~rpc: Only return P2SH wrapped witness programs for BIP141 types in decodescript~~ rpc: Only allow specific types to be P2SH wrapped in decodescript Nov 14, 2021

maflcko force-pushed the 2111-rpcScript branch 2 times, most recently from fa827d2 to fa972d8 Compare November 14, 2021 17:21

sipa reviewed Nov 15, 2021

View reviewed changes

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

maflcko changed the title ~~rpc: Only allow specific types to be P2SH wrapped in decodescript~~ rpc: Only allow specific types to be P2(W)SH wrapped in decodescript Nov 16, 2021

maflcko force-pushed the 2111-rpcScript branch from fa972d8 to fa20010 Compare November 16, 2021 19:15

achow101 reviewed Nov 17, 2021

View reviewed changes

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Nov 18, 2021

scripted-diff: Use clang-tidy syntax for C++ named arguments #23545

Closed

maflcko force-pushed the 2111-rpcScript branch 2 times, most recently from 2e26400 to faa1b5e Compare November 22, 2021 11:19

maflcko force-pushed the 2111-rpcScript branch from faa1b5e to fa0aec3 Compare November 22, 2021 13:23

luke-jr reviewed Nov 30, 2021

View reviewed changes

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

maflcko force-pushed the 2111-rpcScript branch from fa0aec3 to fabf3d0 Compare December 1, 2021 09:09

maflcko mentioned this pull request Dec 1, 2021

Write more decodescript tests #23641

Closed

maflcko force-pushed the 2111-rpcScript branch 3 times, most recently from fadc110 to fad77aa Compare December 2, 2021 14:42

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

9999342

maflcko force-pushed the 2111-rpcScript branch from fad77aa to 9999342 Compare December 6, 2021 09:33

laanwj reviewed Dec 6, 2021

View reviewed changes

laanwj merged commit 695ba2f into bitcoin:master Dec 6, 2021

maflcko deleted the 2111-rpcScript branch December 7, 2021 08:07

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 7, 2021

Merge bitcoin#23486: rpc: Only allow specific types to be P2(W)SH wra…

b6843f1

…pped in…

maflcko mentioned this pull request Jan 21, 2022

policy: treat P2TR outputs with invalid x-only pubkey as non-standard #24106

Closed

bitcoin locked and limited conversation to collaborators Dec 23, 2022

Conversation

maflcko commented Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Sjors commented Nov 11, 2021

Uh oh!

maflcko commented Nov 11, 2021

Uh oh!

DrahtBot commented Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

laanwj commented Nov 12, 2021

Uh oh!

laanwj commented Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Nov 12, 2021

Uh oh!

Sjors commented Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Nov 14, 2021

Uh oh!

Uh oh!

sipa commented Nov 15, 2021

Uh oh!

achow101 commented Nov 15, 2021

Uh oh!

maflcko commented Nov 16, 2021

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko commented Nov 22, 2021

Uh oh!

luke-jr Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Dec 6, 2021

Uh oh!

laanwj commented Dec 6, 2021

Uh oh!

laanwj Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

maflcko commented Nov 11, 2021 •

edited

Loading

DrahtBot commented Nov 12, 2021 •

edited

Loading

laanwj commented Nov 12, 2021 •

edited

Loading

Sjors commented Nov 12, 2021 •

edited

Loading