rpc: have raw transaction decoding infer output descriptors #16795

instagibbs commented at 4:07 PM on September 3, 2019: member

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

DrahtBot added the label Tests on Sep 3, 2019

DrahtBot commented at 5:31 PM on September 3, 2019: member

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

Conflicts

No conflicts as of last run.

NicolasDorier commented at 4:49 AM on September 4, 2019: contributor

that's cool utACK

instagibbs commented at 3:55 PM on September 4, 2019: member

forgot to add description in RPC help, added.

instagibbs force-pushed on Sep 4, 2019

fanquake added the label RPC/REST/ZMQ on Sep 6, 2019

fanquake removed the label Tests on Sep 6, 2019

fanquake renamed this:
~~have raw transaction decoding infer output descriptors~~
rpc: have raw transaction decoding infer output descriptors
on Sep 6, 2019

instagibbs force-pushed on Sep 9, 2019

meshcollider commented at 11:39 PM on September 9, 2019: contributor

utACK c772728ab4a40bfb9a546eef669b0d7603bd9f89

DrahtBot added the label Needs rebase on Sep 10, 2019

instagibbs force-pushed on Sep 10, 2019

instagibbs commented at 1:22 AM on September 10, 2019: member

rebased

fanquake removed the label Needs rebase on Sep 10, 2019

meshcollider commented at 2:17 AM on September 10, 2019: contributor

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

laanwj commented at 7:21 AM on September 10, 2019: member

Travis found a real problem here (for a change !!! :tada: )

/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 force-pushed on Sep 10, 2019

instagibbs commented at 11:19 AM on September 10, 2019: member

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

in src/core_write.cpp:166 in 9018d6378a outdated

 162 | @@ -162,6 +163,7 @@ void ScriptPubKeyToUniv(const CScript& scriptPubKey,
 163 |  
 164 |      if (!ExtractDestinations(scriptPubKey, type, addresses, nRequired) || type == TX_PUBKEY) {
 165 |          out.pushKV("type", GetTxnOutputType(type));
 166 | +        out.pushKV("desc", InferDescriptor(scriptPubKey, DUMMY_SIGNING_PROVIDER)->ToString());

promag commented at 10:26 PM on September 15, 2019:

desc is always present, could just at the beginning? Like before asm.

instagibbs commented at 1:36 PM on September 16, 2019:

done

in src/core_write.cpp:178 in 9018d6378a outdated

 174 | @@ -173,6 +175,8 @@ void ScriptPubKeyToUniv(const CScript& scriptPubKey,
 175 |          a.push_back(EncodeDestination(addr));
 176 |      }
 177 |      out.pushKV("addresses", a);
 178 | +

promag commented at 10:27 PM on September 15, 2019:

nit, remove empty line?

in src/core_write.cpp:12 in 9018d6378a outdated

   8 | @@ -9,6 +9,7 @@
   9 |  #include <key_io.h>
  10 |  #include <script/script.h>
  11 |  #include <script/standard.h>
  12 | +#include <script/descriptor.h>

promag commented at 10:27 PM on September 15, 2019:

gentle nit, keep sorted.

instagibbs commented at 1:36 PM on September 16, 2019:

done

instagibbs force-pushed on Sep 16, 2019

luke-jr referenced this in commit 140244814b on Sep 21, 2019

laanwj added the label Feature on Sep 26, 2019

laanwj added the label Descriptors on Sep 26, 2019

in src/core_write.cpp:156 in 9b9459640d outdated

 157 | @@ -157,6 +158,8 @@ void ScriptPubKeyToUniv(const CScript& scriptPubKey,
 158 |      int nRequired;
 159 |  
 160 |      out.pushKV("asm", ScriptToAsmStr(scriptPubKey));
 161 | +    out.pushKV("desc", InferDescriptor(scriptPubKey, DUMMY_SIGNING_PROVIDER)->ToString());

ariard commented at 8:17 PM on November 4, 2019:

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 ?

instagibbs commented at 7:34 PM on November 20, 2019:

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

instagibbs commented at 7:36 PM on November 20, 2019:

~~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...

MarcoFalke commented at 6:26 AM on March 24, 2022:

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

ariard commented at 8:36 PM on November 4, 2019: member

ACK 9b94596 minus gettouxt/rest_getuxtos doc fixs

Tested getrawtransaction, decoderawtransaction, decodescript, work as expected.

instagibbs force-pushed on Nov 20, 2019

instagibbs commented at 2:23 PM on November 21, 2019: member

pushed update fixing @ariard nits

luke-jr referenced this in commit d5f6845de6 on Jan 5, 2020

theStack commented at 10:11 PM on February 4, 2020: member

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 commented at 10:18 PM on February 4, 2020: member

Concept ACK. We should do this for consistency, though in practice this won't add much (it'll always either be a raw() or addr() descriptor which doesn't convey anything, except for P2PK and bare multisig, which aren't used anymore).

DrahtBot added the label Needs rebase on Mar 4, 2020

instagibbs force-pushed on Aug 10, 2020

instagibbs commented at 7:18 PM on August 10, 2020: member

rebased. @theStack would have re-arranged if I didn't have to regenerate the tests again :)

DrahtBot removed the label Needs rebase on Aug 10, 2020

in src/rpc/blockchain.cpp:1041 in 06c98d68a4 outdated

1037 | @@ -1038,6 +1038,7 @@ UniValue gettxout(const JSONRPCRequest& request)
1038 |                          {RPCResult::Type::OBJ, "scriptPubKey", "",
1039 |                              {
1040 |                                  {RPCResult::Type::STR_HEX, "asm", ""},
1041 | +                                {RPCResult::Type::STR_HEX, "desc", "Inferred descriptor for the output\n"},

luke-jr commented at 7:30 PM on August 11, 2020:

Pretty sure these aren't hex?

instagibbs commented at 2:03 AM on August 13, 2020:

fixed

luke-jr changes_requested

instagibbs force-pushed on Aug 13, 2020

instagibbs commented at 2:04 AM on August 13, 2020: member

failure for createmultisig tests: https://travis-ci.org/github/bitcoin/bitcoin/jobs/716675920

presuming it's unrelated?

in src/rpc/rawtransaction.cpp:560 in ef91078d67 outdated

 556 | @@ -555,6 +557,7 @@ static UniValue decodescript(const JSONRPCRequest& request)
 557 |                              {
 558 |                                  {RPCResult::Type::STR, "address", "segwit address"},
 559 |                              }},
 560 | +                            {RPCResult::Type::STR_HEX, "desc", "Inferred descriptor for the output\n"},

MarcoFalke commented at 9:01 AM on August 30, 2020:

why hex?

instagibbs commented at 2:11 PM on August 31, 2020:

it's not, forgot to change this when fixing the other one

instagibbs force-pushed on Aug 31, 2020

in src/rpc/rawtransaction.cpp:134 in 4372d34280 outdated

 130 | @@ -131,6 +131,7 @@ static UniValue getrawtransaction(const JSONRPCRequest& request)
 131 |                                       {RPCResult::Type::OBJ, "scriptPubKey", "",
 132 |                                       {
 133 |                                           {RPCResult::Type::STR, "asm", "the asm"},
 134 | +                                         {RPCResult::Type::STR_HEX, "desc", "Inferred descriptor for the output\n"},

luke-jr commented at 2:40 PM on November 9, 2020:

Not hex.

in src/rpc/rawtransaction.cpp:486 in 4372d34280 outdated

 482 | @@ -482,6 +483,7 @@ static UniValue decoderawtransaction(const JSONRPCRequest& request)
 483 |                                  {RPCResult::Type::OBJ, "scriptPubKey", "",
 484 |                                  {
 485 |                                      {RPCResult::Type::STR, "asm", "the asm"},
 486 | +                                    {RPCResult::Type::STR_HEX, "desc", "Inferred descriptor for the output\n"},

luke-jr commented at 2:40 PM on November 9, 2020:

Not hex.

luke-jr changes_requested

in src/rpc/rawtransaction.cpp:560 in 4372d34280 outdated

 556 | @@ -555,6 +557,7 @@ static UniValue decodescript(const JSONRPCRequest& request)
 557 |                              {
 558 |                                  {RPCResult::Type::STR, "address", "segwit address"},
 559 |                              }},
 560 | +                            {RPCResult::Type::STR, "desc", "Inferred descriptor for the output\n"},

luke-jr commented at 3:35 PM on November 9, 2020:

There's no output here, just a raw script.

instagibbs commented at 6:20 AM on December 27, 2020:

can you give a suggestion?

sipa commented at 7:08 AM on December 27, 2020:

"Inferred output descriptor for the script" ?

instagibbs commented at 7:09 AM on December 27, 2020:

right I shortly realized what he meant, already pushed

instagibbs commented at 7:34 AM on December 27, 2020:

fixed

in src/rpc/blockchain.cpp:1334 in 4372d34280 outdated

1037 | @@ -1038,6 +1038,7 @@ UniValue gettxout(const JSONRPCRequest& request)
1038 |                          {RPCResult::Type::OBJ, "scriptPubKey", "",
1039 |                              {
1040 |                                  {RPCResult::Type::STR_HEX, "asm", ""},
1041 | +                                {RPCResult::Type::STR, "desc", "Inferred descriptor for the output\n"},

luke-jr commented at 3:36 PM on November 9, 2020:

There's no need for the \n in all these

sipa commented at 4:25 AM on December 27, 2020: member

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 commented at 5:52 AM on December 27, 2020: member

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

instagibbs force-pushed on Dec 27, 2020

instagibbs commented at 7:17 AM on December 27, 2020: member

@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 commented at 7:25 AM on December 27, 2020: member

@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 commented at 7:51 AM on December 27, 2020: member

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

FndNur1Labs commented at 9:09 AM on December 27, 2020: none

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

luke-jr referenced this in commit eaa9043935 on Jan 28, 2021

DrahtBot added the label Needs rebase on Mar 15, 2021

meshcollider commented at 12:26 AM on September 2, 2021: contributor

Concept ACK, sorry I missed this

instagibbs commented at 7:01 AM on September 2, 2021: member

kk will rebase

instagibbs force-pushed on Sep 2, 2021

instagibbs commented at 11:43 PM on September 2, 2021: member

@meshcollider ready for rereview

DrahtBot removed the label Needs rebase on Sep 3, 2021

DrahtBot added the label Needs rebase on Sep 28, 2021

luke-jr referenced this in commit 19a6902d14 on Oct 10, 2021

instagibbs force-pushed on Jan 25, 2022

DrahtBot removed the label Needs rebase on Jan 25, 2022

instagibbs force-pushed on Jan 26, 2022

in src/core_write.cpp:159 in c90a08886a outdated

 152 | @@ -152,8 +153,10 @@ void ScriptPubKeyToUniv(const CScript& scriptPubKey, UniValue& out, bool include
 153 |      CTxDestination address;
 154 |  
 155 |      out.pushKV("asm", ScriptToAsmStr(scriptPubKey));
 156 | +    out.pushKV("desc", InferDescriptor(scriptPubKey, DUMMY_SIGNING_PROVIDER)->ToString());
 157 |      if (include_hex) out.pushKV("hex", HexStr(scriptPubKey));
 158 |  
 159 | +

achow101 commented at 1:42 AM on January 26, 2022:

In c90a08886afe86d84bbd1d76ae3d3ecc24f24a74 "transaction decoding infer output descriptors"

Extraneous line

in src/rpc/blockchain.cpp:1334 in c90a08886a outdated

1330 | @@ -1331,6 +1331,7 @@ static RPCHelpMan gettxout()
1331 |                  {RPCResult::Type::STR_AMOUNT, "value", "The transaction value in " + CURRENCY_UNIT},
1332 |                  {RPCResult::Type::OBJ, "scriptPubKey", "", {
1333 |                      {RPCResult::Type::STR, "asm", ""},
1334 | +                                {RPCResult::Type::STR, "desc", "Inferred descriptor for the output\n"},

achow101 commented at 1:43 AM on January 26, 2022:

In c90a08886afe86d84bbd1d76ae3d3ecc24f24a74 "transaction decoding infer output descriptors"

Indentation, also an extraneous \n

in src/rpc/rawtransaction.cpp:566 in c90a08886a outdated

 562 | @@ -561,6 +563,7 @@ static RPCHelpMan decodescript()
 563 |              RPCResult::Type::OBJ, "", "",
 564 |              {
 565 |                  {RPCResult::Type::STR, "asm", "Script public key"},
 566 | +                        {RPCResult::Type::STR, "desc", "Inferred descriptor for the script"},

achow101 commented at 1:43 AM on January 26, 2022:

In c90a08886afe86d84bbd1d76ae3d3ecc24f24a74 "transaction decoding infer output descriptors"

Indentation

in src/rpc/rawtransaction.cpp:578 in c90a08886a outdated

 574 | @@ -572,6 +575,7 @@ static RPCHelpMan decodescript()
 575 |                       {RPCResult::Type::STR_HEX, "hex", "Hex string of the script public key"},
 576 |                       {RPCResult::Type::STR, "type", "The type of the script public key (e.g. witness_v0_keyhash or witness_v0_scripthash)"},
 577 |                       {RPCResult::Type::STR, "address", /*optional=*/true, "The Bitcoin address (only if a well-defined address exists)"},
 578 | +                            {RPCResult::Type::STR, "desc", "Inferred descriptor for the script"},

achow101 commented at 1:43 AM on January 26, 2022:

Indentation

transaction decoding infer output descriptors 6498ba151b

instagibbs force-pushed on Jan 26, 2022

instagibbs commented at 1:57 AM on January 26, 2022: member

rebased and formatting issues fixed

achow101 commented at 6:58 PM on January 26, 2022: member

ACK 6498ba151b35ce9621ad00730f1fdfca55538ace

meshcollider commented at 10:51 PM on January 26, 2022: contributor

utACK 6498ba151b35ce9621ad00730f1fdfca55538ace

achow101 merged this on Jan 26, 2022

achow101 closed this on Jan 26, 2022

sidhujag referenced this in commit 5642501d8c on Jan 28, 2022

MarcoFalke commented at 11:08 AM on March 22, 2022: member

I don't think we should be using output descriptors to describe redeem scripts and witness scripts. See https://github.com/bitcoin/bitcoin/pull/24636

DrahtBot locked this on Mar 24, 2023