rpc: Add script verification flags to getdeploymentinfo #28806

ajtowns commented at 12:59 AM on November 7, 2023: contributor

Adds a script_flags field to the output of the getdeploymentinfo RPC that lists the flags selected by GetBlockScriptFlags(). This can be useful for seeing the detailed consequences of soft-fork activation behaviour, eg:

$ bitcoin-cli getdeploymentinfo | jq -j '.deployments | to_entries | .[] | (.value.height, " ", .key, "\n")'
227931 bip34
363725 bip66
388381 bip65
419328 csv
481824 segwit
709632 taproot
$ for a in 1 170060 227931 363725 388381 419328 481824 692261 709632; do echo -n "$a: "; bitcoin-cli getdeploymentinfo $(bitcoin-cli getblockhash $a) | jq -r .script_flags; done
1: P2SH,TAPROOT,WITNESS
170060: 
227931: P2SH,TAPROOT,WITNESS
363725: DERSIG,P2SH,TAPROOT,WITNESS
388381: CHECKLOCKTIMEVERIFY,DERSIG,P2SH,TAPROOT,WITNESS
419328: CHECKLOCKTIMEVERIFY,CHECKSEQUENCEVERIFY,DERSIG,P2SH,TAPROOT,WITNESS
481824: CHECKLOCKTIMEVERIFY,CHECKSEQUENCEVERIFY,DERSIG,NULLDUMMY,P2SH,TAPROOT,WITNESS
692261: CHECKLOCKTIMEVERIFY,CHECKSEQUENCEVERIFY,DERSIG,NULLDUMMY,P2SH,WITNESS
709632: CHECKLOCKTIMEVERIFY,CHECKSEQUENCEVERIFY,DERSIG,NULLDUMMY,P2SH,TAPROOT,WITNESS

This allows you to see that P2SH, WITNESS and TAPROOT are enforced since genesis; that the p2sh exception block (170060) has none of these extra rules enforced, that the taproot exception block (692261) is exempted from taproot enforcement, and exactly which rules were activated by each soft fork (eg, NULLDUMMY as part of the segwit soft fork).

DrahtBot commented at 12:59 AM on November 7, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	kristapsk

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29745 (bench: Adds a benchmark for CheckInputScripts by kevkevinpal)
#29280 (Implement OP_CHECKTEMPLATEVERIFY by reardencode)
#29270 (Implement OP_CHECKSIGFROMSTACK(VERIFY) by reardencode)
#29269 (Add OP_INTERNALKEY for Tapscript by reardencode)
#29039 (versionbits refactoring by ajtowns)

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.

DrahtBot added the label RPC/REST/ZMQ on Nov 7, 2023

ajtowns commented at 1:02 AM on November 7, 2023: contributor

This patch has been included in bitcoin-inquisition for a few releases (https://github.com/bitcoin-inquisition/bitcoin/pull/5, https://github.com/bitcoin-inquisition/bitcoin/pull/15 and https://github.com/bitcoin-inquisition/bitcoin/pull/31). I used the same technique when in #23536#pullrequestreview-872777718 and found it helpful.

in src/deploymentinfo.cpp:58 in c8ea84cbf6 outdated

  53 | @@ -52,3 +54,49 @@ std::optional<Consensus::BuriedDeployment> GetBuriedDeployment(const std::string
  54 |      }
  55 |      return std::nullopt;
  56 |  }
  57 | +
  58 | +#define FLAG_NAME(flag) {std::string(#flag), (uint32_t)SCRIPT_VERIFY_##flag},

maflcko commented at 9:03 AM on November 7, 2023:

#define FLAG_NAME(flag) {std::string{#flag}, uint32_t{SCRIPT_VERIFY_##flag}}

No need to cast, they are already unsigned, see fa621ededdf. Also, could omit the comma for clarity and to allow the use of clang-format on your code?

in src/deploymentinfo.cpp:86 in c8ea84cbf6 outdated

  81 | +};
  82 | +#undef FLAG_NAME
  83 | +
  84 | +std::string FormatScriptFlags(uint32_t flags)
  85 | +{
  86 | +    if (flags == 0) return "";

maflcko commented at 9:05 AM on November 7, 2023:

Is this ever hit? If not, maybe it can be removed, as the remaining code should reach the same conclusion anyway in the rare case that this is hit?

maflcko commented at 9:12 AM on November 7, 2023:

cc @aureleoules for the coverage report

No coverage data available for this pull request and commit.

aureleoules commented at 12:59 PM on November 7, 2023:

Thanks for the ping, I re-ran the job. AWS preempted the job and I haven't figured out how to rerun them automatically yet. Anyone in this org can re-run corecheck jobs by signing in as well!

ajtowns commented at 8:36 PM on November 7, 2023:

Is this ever hit? If not, maybe it can be removed, as the remaining code should reach the same conclusion anyway in the rare case that this is hit?

It's hit for block 170060. I think it's worth having it be explicit, as it would also be reasonable to return "NONE" (as in SCRIPT_VERIFY_NONE) for that case.

in src/deploymentinfo.h:14 in c8ea84cbf6 outdated

   7 | @@ -8,6 +8,7 @@
   8 |  #include <consensus/params.h>
   9 |  
  10 |  #include <optional>
  11 | +#include <map>
  12 |  #include <string>

maflcko commented at 9:12 AM on November 7, 2023:

unrelated nit:

deploymentinfo.h should add these lines:
#include <cassert>            // for assert
#include <cstdint>            // for uint32_t
#include <string_view>         // for string_view

maflcko commented at 9:13 AM on November 7, 2023: member

left some nits/questions. Feel free to ignore.

ajtowns force-pushed on Nov 7, 2023

kristapsk commented at 10:58 AM on November 8, 2023: contributor

Concept ACK

in src/deploymentinfo.cpp:60 in 02d8461d65 outdated

  53 | @@ -52,3 +54,49 @@ std::optional<Consensus::BuriedDeployment> GetBuriedDeployment(const std::string
  54 |      }
  55 |      return std::nullopt;
  56 |  }
  57 | +
  58 | +#define FLAG_NAME(flag) {std::string(#flag), SCRIPT_VERIFY_##flag}
  59 | +const std::map<std::string, uint32_t> g_verify_flag_names{
  60 | +    FLAG_NAME(P2SH),

luke-jr commented at 4:38 PM on November 8, 2023:

If we're exposing this, probably better to call it "BIP16" since p2wsh and p2tr(sometimes) are also p2sh.

ajtowns commented at 5:35 AM on November 9, 2023:

"Pay to script hash" is the title of BIP16, and we use both "pay-to-script-hash" and "P2SH" as descriptions of the sh() descriptor in doc/descriptors.md, so I don't think this is confusing or needs any changes.

in src/rpc/blockchain.cpp:1363 in 02d8461d65 outdated

1359 | @@ -1359,6 +1360,7 @@ RPCHelpMan getdeploymentinfo()
1360 |              UniValue deploymentinfo(UniValue::VOBJ);
1361 |              deploymentinfo.pushKV("hash", blockindex->GetBlockHash().ToString());
1362 |              deploymentinfo.pushKV("height", blockindex->nHeight);
1363 | +            deploymentinfo.pushKV("script_flags", FormatScriptFlags(GetBlockScriptFlags(*blockindex, chainman)));

luke-jr commented at 4:40 PM on November 8, 2023:

This should not be a comma-deliminated string, but an array of strings.

luke-jr commented at 4:40 PM on November 8, 2023:

Maybe number form should also be available for users of libbitcoinconsensus?

ajtowns commented at 7:23 AM on November 9, 2023:

Huh, had thought about that previously and didn't like it. Reconsidering it, now I do. :shrug:

ajtowns commented at 7:26 AM on November 9, 2023:

Numbers aren't a standard (they'll readily change between releases; also true of the names) but they also aren't particularly useful as documentation, so don't think it's sensible to export them here.

luke-jr changes_requested

luke-jr commented at 4:40 PM on November 8, 2023: member

Getting some deja vu here - is there another PR for this already?

luke-jr commented at 4:42 PM on November 8, 2023: member

Ah, #10730

ajtowns commented at 11:22 PM on November 8, 2023: contributor

Ah, #10730

"When we last discussed making scripts debuggable" -- this and #28802 are prereqs for "bitcoin-util evalscript" which I'm poking at on https://github.com/ajtowns/bitcoin/commits/202309-evalscript . Real debugging seems better done with smarter external tools like https://github.com/dgpv/bsst though.

ajtowns force-pushed on Nov 9, 2023

DrahtBot added the label CI failed on Jan 16, 2024

Move mapFlagNames and FormatScriptFlags logic to deploymentinfo.h

Moves FormatScriptFlags logic into GetScriptFlagNames which returns a
vector of strings. For completeness, also has GetScriptFlagNames report
on any bits that do not match a known script flag.

4cd4f10bdb

validation: export GetBlockScriptFlags() aa1f379a07

rpc: have getdeploymentinfo report script verify flags b2953711e4

ajtowns force-pushed on Feb 6, 2024

DrahtBot removed the label CI failed on Feb 6, 2024

achow101 commented at 3:45 PM on April 9, 2024: member

The PR didn't seem to attract much attention in the past. Also, the issue seems not important enough right now to keep it sitting around idle in the list of open PRs.

Closing due to lack of interest.

achow101 closed this on Apr 9, 2024

bitcoin locked this on Apr 9, 2025