doc: Updated outdated help command for getblocktemplate #19646

pull jakeleventhal wants to merge 1 commits into bitcoin:master from jakeleventhal:fix-outdated-getblocktemplate-help changing 1 files +9 −10
  1. jakeleventhal commented at 7:56 PM on August 2, 2020: contributor

    Summary of Changes

    • Removed coinbasetxn from the help outputs
    • Added the missing name for transactions in the help outputs
    • Added help outputs for longpollid and default_witness_commitment
    • Added more clarity to capabilities, rules, and coinbaseaux

    Rationale The outputs from the help command for getblocktemplate are outdated and don't reflect the actual results from getblocktemplate (see #19625 for more details)

    Fixes #19625.

  2. DrahtBot commented at 8:11 PM on August 2, 2020: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  3. DrahtBot cross-referenced this on Aug 2, 2020 from issue Bugfix: RPC/Mining: Fix up GBT docs by luke-jr
  4. jakeleventhal force-pushed on Aug 2, 2020
  5. DrahtBot added the label Docs on Aug 2, 2020
  6. DrahtBot added the label Mining on Aug 2, 2020
  7. DrahtBot added the label RPC/REST/ZMQ on Aug 2, 2020
  8. jakeleventhal force-pushed on Aug 2, 2020
  9. jakeleventhal commented at 1:01 AM on August 3, 2020: contributor

    got it passing, just updated the commit message to rerun it

  10. fanquake renamed this:
    doc: Updated outdated help command for getblocktemplate (fixes #19625)
    doc: Updated outdated help command for getblocktemplate
    on Aug 3, 2020
  11. instagibbs commented at 2:51 PM on August 5, 2020: member

    concept ACK, might want to get review from someone who knows these fields better though. @luke-jr ?

  12. laanwj commented at 3:01 PM on August 5, 2020: member

    ACK c91b241b48d7f97b3e6b39d84ec780f2a3e3a0a7

    • coinbasetxn was removed (only matches for it are in the doc and a RPC test that checks it is gone)
    • naming the 'transactions' array makes sense
    • 'default_witness_commitment"' was added — it's an optional field though, you might want to add the optional flag
  13. jakeleventhal force-pushed on Aug 6, 2020
  14. jakeleventhal commented at 4:43 AM on August 6, 2020: contributor

    @laanwj Updated and added optional flag to default_witness_commitment

  15. luke-jr commented at 6:32 AM on August 6, 2020: member

    Maybe combine with #19395?

  16. jakeleventhal force-pushed on Aug 6, 2020
  17. jakeleventhal commented at 7:47 AM on August 6, 2020: contributor

    @luke-jr I just included those changes in this branch as well

  18. bitcoin deleted a comment on Aug 6, 2020
  19. laanwj commented at 11:03 AM on August 7, 2020: member

    You should credit @luke-jr in your commit message or keep his changes as a separate commit :smiley:

  20. jakeleventhal force-pushed on Aug 7, 2020
  21. jakeleventhal commented at 5:37 PM on August 7, 2020: contributor

    @laanwj I just updated the commit message to credit @luke-jr (didn't use the @ mention though, as specified in CONTRIBUTING.md)

  22. jakeleventhal force-pushed on Aug 7, 2020
  23. laanwj commented at 4:43 PM on August 9, 2020: member

    @laanwj I just updated the commit message to credit @luke-jr (didn't use the @ mention though, as specified in CONTRIBUTING.md)

    Perfect! code review ACK 7c983d337bdc210744a2d3cc10cc0d3aa2814cae

  24. in src/rpc/mining.cpp:521 in 7c983d337b outdated 
     517 | @@ -517,15 +518,15 @@ static UniValue getblocktemplate(const JSONRPCRequest& request)
 518 |                          {RPCResult::Type::NUM, "version", "The preferred block version"},
 519 |                          {RPCResult::Type::ARR, "rules", "specific block rules that are to be enforced",
 520 |                              {
 521 | -                                {RPCResult::Type::STR, "", "rulename"},
 522 | +                                {RPCResult::Type::STR, "", "name of a rule the client must understand to some extend; see BIP 9 for format"},
    luke-jr commented at 6:51 PM on August 9, 2020:

    You introduced a misspelling here. Should be "extent"

  25. in src/rpc/mining.cpp:550 in 7c983d337b outdated 
     551 |                          {RPCResult::Type::NUM, "coinbasevalue", "maximum allowable input to coinbase transaction, including the generation award and transaction fees (in satoshis)"},
 552 | -                        {RPCResult::Type::OBJ, "coinbasetxn", "information for coinbase transaction",
 553 | -                        {
 554 | -                            {RPCResult::Type::ELISION, "", ""},
 555 | -                        }},
 556 | +                        {RPCResult::Type::STR, "longpollid", "the id of the block template longpoll"},
    luke-jr commented at 6:52 PM on August 9, 2020:

    "an id to include with a request to longpoll on an update to this template" ?

  26. in src/rpc/mining.cpp:564 in 7c983d337b outdated 
     560 | @@ -563,6 +561,7 @@ static UniValue getblocktemplate(const JSONRPCRequest& request)
 561 |                          {RPCResult::Type::NUM_TIME, "curtime", "current timestamp in " + UNIX_EPOCH_TIME},
 562 |                          {RPCResult::Type::STR, "bits", "compressed target of next block"},
 563 |                          {RPCResult::Type::NUM, "height", "The height of the next block"},
 564 | +                        {RPCResult::Type::STR, "default_witness_commitment", /* optional */ true, "the default witness commitment of the block template"}
    luke-jr commented at 6:52 PM on August 9, 2020:

    "a valid witness commitment for the unmodified block template" ?

  27. jakeleventhal commented at 6:53 PM on August 9, 2020: contributor

    @laanwj the build is failing but it seems to be unrelated? How do i rerun the build

  28. luke-jr changes_requested
  29. jakeleventhal force-pushed on Aug 9, 2020
  30. jakeleventhal commented at 7:05 PM on August 9, 2020: contributor

    @luke-jr updated per your suggestions

  31. jonatack commented at 7:33 PM on August 9, 2020: contributor

    @jakeleventhal for later, you can also try this as a nice way to credit a co-author in the commit message, if you like:

    https://stackoverflow.com/questions/7442112/how-to-attribute-a-single-commit-to-multiple-developers

  32. luke-jr approved
  33. luke-jr commented at 8:40 PM on August 9, 2020: member

    utACK

  34. jakeleventhal commented at 4:09 AM on August 10, 2020: contributor

    @luke-jr it looks like one build timed out and another failed for no clear reason. how should i rerun aside from just making a new push with a slightly altered commit message (would rather not do that if there is a better way)

  35. luke-jr commented at 6:20 AM on August 10, 2020: member

    I don't see a way to restart the Cirrus, but I took care of Travis.

  36. in src/rpc/mining.cpp:503 in 1c028ff583 outdated 
     499 | @@ -500,12 +500,13 @@ static UniValue getblocktemplate(const JSONRPCRequest& request)
 500 |                              {"mode", RPCArg::Type::STR, /* treat as named arg */ RPCArg::Optional::OMITTED_NAMED_ARG, "This must be set to \"template\", \"proposal\" (see BIP 23), or omitted"},
 501 |                              {"capabilities", RPCArg::Type::ARR, /* treat as named arg */ RPCArg::Optional::OMITTED_NAMED_ARG, "A list of strings",
 502 |                                  {
 503 | -                                    {"support", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "client side supported feature, 'longpoll', 'coinbasetxn', 'coinbasevalue', 'proposal', 'serverlist', 'workid'"},
 504 | +                                    {"str", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "client side supported feature, 'longpoll', 'coinbasevalue', 'proposal', 'serverlist', 'workid'"},
    fjahr commented at 7:33 PM on August 10, 2020:

    nit: I would have named this feature to be more expressive

    jakeleventhal commented at 6:26 AM on August 11, 2020:

    fixed

    luke-jr commented at 6:47 PM on August 11, 2020:

    The rest of our RPC doc uses "str" for things like this...

    fjahr commented at 8:19 PM on August 11, 2020:

    Where can I find that? Maybe I am doing something wrong but git grep "RPCArg::Type::STR" | grep "\"str\"" gives me no results.

  37. in src/rpc/mining.cpp:509 in 1c028ff583 outdated 
     506 |                                  },
 507 |                              {"rules", RPCArg::Type::ARR, RPCArg::Optional::NO, "A list of strings",
 508 |                                  {
 509 | -                                    {"support", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "client side supported softfork deployment"},
 510 | +                                    {"segwit", RPCArg::Type::STR, RPCArg::Optional::NO, "(literal) indicates client side segwit support"},
 511 | +                                    {"str", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "other client side supported softfork deployment"},
    fjahr commented at 7:46 PM on August 10, 2020:

    nit: Same here, would have called it rule or softfork

    MarcoFalke commented at 6:14 AM on August 11, 2020:

    Agree, but to mention for completeness, json types inside json arrays aren't named, so the name only serves for internal developer documentation

    jakeleventhal commented at 6:26 AM on August 11, 2020:

    fixed

  38. fjahr commented at 7:58 PM on August 10, 2020: contributor

    utACK 1c028ff5831b5688a571bed93b008917f1c542c0

    Feel free to ignore my nits but I think you should credit @luke-jr properly as indicated by @jonatack. Here is a link to the official docs: https://docs.github.com/en/github/committing-changes-to-your-project/creating-a-commit-with-multiple-authors

  39. jakeleventhal force-pushed on Aug 11, 2020
  40. jakeleventhal commented at 6:27 AM on August 11, 2020: contributor

    @fjahr resolved nits and updated commit message

  41. fjahr commented at 8:28 AM on August 11, 2020: contributor

    utACK 50c087910df650e87e630232b07302bbf7135423

  42. luke-jr changes_requested
  43. luke-jr commented at 6:48 PM on August 11, 2020: member

    @fjahr's nits were in error IMO

  44. jakeleventhal force-pushed on Aug 11, 2020
  45. jakeleventhal commented at 11:16 PM on August 11, 2020: contributor

    @luke-jr reverted nits. I thought the same too - it seemed like "str" was the type used in other places

  46. fjahr commented at 11:38 PM on August 11, 2020: contributor

    @luke-jr reverted nits. I thought the same too - it seemed like "str" was the type used in other places

    Which other places?

    $ git grep "\"str\""
src/univalue/test/object.cpp:    std::string correctValue("str");

    I seem to be missing something fundamental and I am curious what it is...

  47. in src/rpc/mining.cpp:547 in b908b2bf14 outdated 
     541 | @@ -541,15 +542,12 @@ static UniValue getblocktemplate(const JSONRPCRequest& request)
 542 |                                          {RPCResult::Type::NUM, "weight", "total transaction weight, as counted for purposes of block limits"},
 543 |                                      }},
 544 |                              }},
 545 | -                        {RPCResult::Type::OBJ, "coinbaseaux", "data that should be included in the coinbase's scriptSig content",
 546 | +                        {RPCResult::Type::OBJ_DYN, "coinbaseaux", "data that should be included in the coinbase's scriptSig content",
 547 |                          {
 548 | -                            {RPCResult::Type::ELISION, "", ""},
 549 | +                            {RPCResult::Type::STR_HEX, "str", "values must be in the coinbase (keys may be ignored)"},
    fjahr commented at 10:34 PM on August 12, 2020:

    Looking at other RPCResult::Type::OBJ_DYN instances, the keys are usually named "key" unless there is a better name.

                                {RPCResult::Type::STR_HEX, "key", "values must be in the coinbase (keys may be ignored)"},
    jakeleventhal commented at 3:56 AM on August 14, 2020:

    @fjahr is seems there is a good mix of types that are used - but hex seems to be the one that fits most to me. thoughts?

    fjahr commented at 4:29 PM on August 14, 2020:

    Hm, I don't really understand your comment. I didn't say anything about changing the hex string type which is the type of the value. "key" or "str" is just the placeholder for the key.

    This is generated by the current code:

      "coinbaseaux" : {                        (json object) data that should be included in the coinbase's scriptSig content
    "str" : "hex",                         (string) values must be in the coinbase (keys may be ignored)
    ...
  },

    And this is with the suggested change:

      "coinbaseaux" : {                        (json object) data that should be included in the coinbase's scriptSig content
    "key" : "hex",                         (string) values must be in the coinbase (keys may be ignored)
    ...
  },
    jakeleventhal commented at 8:06 PM on August 15, 2020:

    @fjahr updated

  48. fjahr commented at 11:49 PM on August 12, 2020: contributor

    I think I have realized what you meant while reviewing another PR: "str" gets automatically inserted by RPCHelpMan in various places of the help depending on the context of a string type in args and results. For example as the value of a key-value pair with value type string, see the mode arg in this RPC as well as the rules array in the results. But "str" is never explicitly given as a name in the RPC code as you are doing here.

    Still this was a nit so feel free to ignore, I was just scratching my head where the discrepancy came from.

  49. Updated outdated help command for getblocktemplate (fixes #19625)
    * Removed coinbasetxn from the getblocktemplate help outputs
* Added the missing name for transactions in the help outputs
* Added getblocktemplate help outputs for longpollid and default_witness_commitment
* Added more clarity to capabilities, rules, and coinbaseaux for getblocktemplate help (credit to luke-jr)

Co-authored-by: Luke Dashjr <luke+github_public@dashjr.org>
    c91b241b48
  50. jakeleventhal force-pushed on Aug 15, 2020
  51. jakeleventhal commented at 3:49 AM on August 19, 2020: contributor

    bumping on this to reviewers. all comments addressed @fjahr @MarcoFalke @luke-jr

  52. fjahr commented at 1:28 PM on August 19, 2020: contributor

    utACK c91b241b48d7f97b3e6b39d84ec780f2a3e3a0a7

  53. laanwj merged this on Aug 28, 2020
  54. laanwj closed this on Aug 28, 2020
  55. sidhujag referenced this in commit 4ecbbea6ec on Aug 28, 2020
  56. unknown cross-referenced this on Oct 8, 2020 from issue Remove `reqSigs` from output of decoderawtransaction and other RPCs by sanket1729
  57. jakeleventhal deleted the branch on Nov 7, 2020
  58. jakeleventhal commented at 11:51 PM on November 7, 2020: contributor

    @laanwj @luke-jr @fjahr How does the review process work for bitcoin? What if the four of us colluded to add some backdoor? What is the "decentralized" approach to merging in code? Is this not a huge vulnerability?

  59. CalamariDude commented at 11:53 PM on November 7, 2020: none

    ^^

  60. michaelfolkson commented at 12:06 AM on November 8, 2020: contributor

    @jakeleventhal: This isn't the right location for general questions. Please try bitcoin-core-pr-reviews channel on IRC instead.

    Jon Atack discussed the Core review process in this Stephan Livera episode. But please no more discussion here. Thanks

  61. bitcoin locked this on Nov 8, 2020
Contributors
jakeleventhalDrahtBotinstagibbslaanwjluke-jrjonatackfjahrMarcoFalkeCalamariDudemichaelfolkson
Labels
DocsRPC/REST/ZMQMining
Linked (view graph)
#19625 rpc: Missing/wrong fields in getblocktemplate Result example#20102 Remove `reqSigs` from output of decoderawtransaction and other RPCs
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: 2026-05-19 06:53 UTC