Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: have raw transaction decoding infer output descriptors #16795

Merged
merged 1 commit into from
Jan 26, 2022

Conversation

instagibbs
Copy link
Member

@instagibbs instagibbs commented Sep 3, 2019

Following discussion in #16725 this is complementary data to expose. All outputs are inferred.

@DrahtBot DrahtBot added the Tests label Sep 3, 2019
@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 3, 2019

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

Conflicts

No conflicts as of last run.

@NicolasDorier
Copy link
Contributor

that's cool utACK

@instagibbs
Copy link
Member Author

forgot to add description in RPC help, added.

@fanquake fanquake changed the title have raw transaction decoding infer output descriptors rpc: have raw transaction decoding infer output descriptors Sep 6, 2019
@meshcollider
Copy link
Contributor

utACK c772728

@instagibbs
Copy link
Member Author

rebased

@meshcollider
Copy link
Contributor

meshcollider commented Sep 10, 2019

You need to update the help-text for the RPCs affected by this change (decoderawtransaction, decodescript, getrawtransaction) to include the additional field

@laanwj
Copy link
Member

laanwj commented Sep 10, 2019

Travis found a real problem here (for a change !!! 🎉 )

/usr/bin/python3.6 ../test/util/bitcoin-util-test.py

2019-09-10 01:34:27,547 - ERROR - Output data mismatch for txcreateoutpubkey1.json (format json)

2019-09-10 01:34:27,548 - ERROR - Output formatting mismatch for txcreateoutpubkey1.json:

*** txcreateoutpubkey1.json

--- returned

***************

*** 15,22 ****

              "scriptPubKey": {

                  "asm": "02a5613bd857b7048924264d1e70e08fb2a7e6527d32b7ab1bb993ac59964ff397 OP_CHECKSIG",

                  "hex": "2102a5613bd857b7048924264d1e70e08fb2a7e6527d32b7ab1bb993ac59964ff397ac",

!                 "type": "pubkey",

!                 "desc": "pk(02a5613bd857b7048924264d1e70e08fb2a7e6527d32b7ab1bb993ac59964ff397)#rk5v7uqw"

              }

          }

      ],

--- 15,21 ----

              "scriptPubKey": {

                  "asm": "02a5613bd857b7048924264d1e70e08fb2a7e6527d32b7ab1bb993ac59964ff397 OP_CHECKSIG",

                  "hex": "2102a5613bd857b7048924264d1e70e08fb2a7e6527d32b7ab1bb993ac59964ff397ac",

!                 "type": "pubkey"

              }

          }

      ],

2019-09-10 01:34:30,728 - ERROR - FAILED_TESTCASES:

['Creates a new transaction with a single pay-to-pubkey output (output as json)']

@instagibbs instagibbs force-pushed the decode_descriptor branch 2 times, most recently from 326f322 to 9018d63 Compare September 10, 2019 11:18
@instagibbs
Copy link
Member Author

fixed rebase error, now all outputs are inferred. added description to decodescript

src/core_write.cpp Outdated Show resolved Hide resolved
src/core_write.cpp Outdated Show resolved Hide resolved
src/core_write.cpp Outdated Show resolved Hide resolved
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Sep 21, 2019
Copy link
Member

@ariard ariard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 9b94596 minus gettouxt/rest_getuxtos doc fixs

Tested getrawtransaction, decoderawtransaction, decodescript, work as expected.

@@ -157,6 +158,8 @@ void ScriptPubKeyToUniv(const CScript& scriptPubKey,
int nRequired;

out.pushKV("asm", ScriptToAsmStr(scriptPubKey));
out.pushKV("desc", InferDescriptor(scriptPubKey, DUMMY_SIGNING_PROVIDER)->ToString());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to update help-debug of gettxout and also maybe REST-interface.md on rest_getutxos.

Do you want also to extend feature to decodepsbt ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want also to extend feature to decodepsbt ?

going to take a little more thinking, so for now "no" :)

fixed up other missing documentation to best of knowledge

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see **ScriptToUniv** is only used for PSBT decoding... so yeah I can do that real quick.

eh maybe not im not sure of the reasoning of addresses being included or not in decodepsbt output...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, decodepsbt is included in this pull (despite the docs not being updated)

@instagibbs
Copy link
Member Author

pushed update fixing @ariard nits

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jan 5, 2020
@theStack
Copy link
Contributor

theStack commented Feb 4, 2020

Being pedantic here, but wouldn't it be more logical to put the inferred output descriptor "desc" before "asm" within the "scriptPubKey" json object, to keep the abstraction level order strictly decreasing (address -> script -> raw bytes)? From a user perspective I'd find it nice to find the higher-level interpretation of a scriptPubKey right in the first line, and I only need to dig deeper into the next two lines if I want more low-level representation.

@sipa
Copy link
Member

sipa commented Dec 27, 2020

It would be nice to have this, as it compensates somewhat for the (weird) functionality lost in #20286. There are a bunch of unaddressed comments here though. And also, it'd be good if the decodescript output inside the "p2sh" and "segwit" blocks would report a descriptor that takes the structure into account (i.e., report "sh(...)" instead of "addr(...)" if it's a recognizable script).

@instagibbs
Copy link
Member Author

Ah, thought I closed this PR. I'll take a fresh look.

@instagibbs
Copy link
Member Author

@sipa Looks like it already does the native segwit inference in decodescript, p2sh doesn't have a block per se, just a single field with the p2sh address itself. Could add it I guess? Name suggestion for the object?

@sipa
Copy link
Member

sipa commented Dec 27, 2020

@instagibbs I was confusing decodescript's "p2sh" and "segwit" fields with the "embedded" field in getaddressinfo (thinking it'd give an Object with fields for the p2sh/segwit version of the specified script, instead of just the resulting address). Perhaps there is a use for that, but not in this PR.

@instagibbs
Copy link
Member Author

instagibbs commented Dec 27, 2020

Addressed all concerns, I think. Rebased on master since this is pretty ancient.

@SoraKohaku
Copy link

Desc for string. Update is good. Need known rebase that.

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jan 28, 2021
@meshcollider
Copy link
Contributor

Concept ACK, sorry I missed this

@instagibbs
Copy link
Member Author

kk will rebase

@instagibbs
Copy link
Member Author

@meshcollider ready for rereview

src/core_write.cpp Outdated Show resolved Hide resolved
src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved
src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved
@instagibbs
Copy link
Member Author

rebased and formatting issues fixed

@achow101
Copy link
Member

ACK 6498ba1

Copy link
Contributor

@meshcollider meshcollider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 6498ba1

@achow101 achow101 merged commit 3d22371 into bitcoin:master Jan 26, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 28, 2022
@maflcko
Copy link
Member

maflcko commented Mar 22, 2022

I don't think we should be using output descriptors to describe redeem scripts and witness scripts. See #24636

@bitcoin bitcoin locked and limited conversation to collaborators Mar 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet