rpc: Add script verification flags to getdeploymentinfo #28806

pull ajtowns wants to merge 3 commits into bitcoin:master from ajtowns:202311-depinfo-scriptflags changing 8 files +90 −46
  1. ajtowns commented at 0: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:

     0$ bitcoin-cli getdeploymentinfo | jq -j '.deployments | to_entries | .[] | (.value.height, " ", .key, "\n")'
 1227931 bip34
 2363725 bip66
 3388381 bip65
 4419328 csv
 5481824 segwit
 6709632 taproot
 7$ 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
 81: P2SH,TAPROOT,WITNESS
 9170060: 
10227931: P2SH,TAPROOT,WITNESS
11363725: DERSIG,P2SH,TAPROOT,WITNESS
12388381: CHECKLOCKTIMEVERIFY,DERSIG,P2SH,TAPROOT,WITNESS
13419328: CHECKLOCKTIMEVERIFY,CHECKSEQUENCEVERIFY,DERSIG,P2SH,TAPROOT,WITNESS
14481824: CHECKLOCKTIMEVERIFY,CHECKSEQUENCEVERIFY,DERSIG,NULLDUMMY,P2SH,TAPROOT,WITNESS
15692261: CHECKLOCKTIMEVERIFY,CHECKSEQUENCEVERIFY,DERSIG,NULLDUMMY,P2SH,WITNESS
16709632: 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).

  2. DrahtBot commented at 0: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.

  3. DrahtBot added the label RPC/REST/ZMQ on Nov 7, 2023
  4. 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.
  5. 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: 
    0#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?

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

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

    0deploymentinfo.h should add these lines:
1#include <cassert>            // for assert
2#include <cstdint>            // for uint32_t
3#include <string_view>         // for string_view
  8. maflcko commented at 9:13 am on November 7, 2023: member
    left some nits/questions. Feel free to ignore.
  9. ajtowns force-pushed on Nov 7, 2023
  10. kristapsk commented at 10:58 am on November 8, 2023: contributor
    Concept ACK
  11. 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.
  12. 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.
  13. luke-jr changes_requested
  14. luke-jr commented at 4:40 pm on November 8, 2023: member
    Getting some deja vu here - is there another PR for this already?
  15. luke-jr commented at 4:42 pm on November 8, 2023: member
    Ah, #10730
  16. 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.

  17. ajtowns force-pushed on Nov 9, 2023
  18. DrahtBot added the label CI failed on Jan 16, 2024
  19. 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
  20. validation: export GetBlockScriptFlags() aa1f379a07
  21. rpc: have getdeploymentinfo report script verify flags b2953711e4
  22. ajtowns force-pushed on Feb 6, 2024
  23. DrahtBot removed the label CI failed on Feb 6, 2024
  24. 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.

  25. achow101 closed this on Apr 9, 2024


ajtowns DrahtBot maflcko aureleoules kristapsk luke-jr achow101

Labels
RPC/REST/ZMQ

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-09-28 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me