mining: rename gbt_force and gbt_force_name #32386

pull Sjors wants to merge 2 commits into bitcoin:master from Sjors:2025/04/gbt changing 5 files +14 −13
  1. Sjors commented at 12:01 pm on April 30, 2025: member

    The term “force” is ambiguous and not used in BIP9 where there ! rule prefix is introduced.

    E.g. this code is hard to read:

    0if (!gbt_force) {
    1   s.insert(s.begin(), '!');
    

    Additionally, #29039 renamed gbt_vb_name to gbt_force_name which, at least for me, further increased the confusion.

    This is a pure (variable rename) refactor (plus documentation) and does not change behavior.

    Reminder of how to verify a scripted diff:

    0test/lint/commit-script-check.sh origin/master..HEAD
    
  2. DrahtBot commented at 12:01 pm on April 30, 2025: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32386.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK achow101, janb84

    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:

    • #26201 (Remove Taproot activation height by Sjors)

    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 Mining on Apr 30, 2025
  4. TheCharlatan commented at 12:35 pm on April 30, 2025: contributor
    Can this be a scripted diff?
  5. Sjors force-pushed on Apr 30, 2025
  6. Sjors commented at 12:49 pm on April 30, 2025: member
    @TheCharlatan converted to a scripted diff plus a documentation commit.
  7. scripted-diff: rename gbt_force and gbt_force_name
    The term "force" is ambiguous and not used in BIP9 where the ! rule
    prefix is introduced.
    
    Additionally, #29039 renamed gbt_vb_name to gbt_force_name which
    might increase the confusion.
    
    -BEGIN VERIFY SCRIPT-
    sed -i s/gbt_force_name/gbt_rule_value/g ./src/rpc/mining.cpp
    sed -i s/gbt_force/gbt_optional_rule/g $(git grep -l gbt_force)
    -END VERIFY SCRIPT-
    5e87c3ec09
  8. mining: document gbt_rule_value helper 0750249289
  9. Sjors force-pushed on Apr 30, 2025
  10. ajtowns commented at 1:44 pm on April 30, 2025: contributor

    Note that there this field combines three effects:

    1. per bip-9, it means that the template incorporates features from that soft fork and miners not aware of what the soft fork means may produce invalid blocks by mining in the regular way
    2. similar to bip-145’s implication that clients must specify “segwit” in the rules field of its request in order for witness txs to be included, it becomes an error if a client doesn’t explicitly indicate support for a feature that’s activated when requesting a template
    3. when a “!” soft fork is in the signalling phase, if the getblocktemplate request didn’t indicate support, then the template won’t signal for the feature by default

    I don’t think the third effect has ever actually been used: since bip9, segwit was the only gbt_force=false rule, but that was set to gbt_force=true prior to activation with #9955. See also #16060 and #17946. In particular https://github.com/bitcoin/bitcoin/pull/16060/files#r309323261

  11. Sjors commented at 2:31 pm on April 30, 2025: member

    I also documented some of the ! behavior in #26201. It might be worth including that here, along with @ajtowns’s explanation.

    Behavior (3) is implemented here (code from master, so no rename):

    https://github.com/bitcoin/bitcoin/blob/14b8dfb2bd5e2ca2b7c0c9a7f7d50e1e60adf75c/src/rpc/mining.cpp#L965-L970

    This seems potentially dangerous when combined with forced signalling, which IIRC has occasionally been proposed after Bitcoin Core already shipped activation code. Once something is locked in, there’s less utility in (correctly) signalling, so it could make sense to “lie”, especially since behavior (2) inevitably kicks at activation time:

    https://github.com/bitcoin/bitcoin/blob/14b8dfb2bd5e2ca2b7c0c9a7f7d50e1e60adf75c/src/rpc/mining.cpp#L974-L978

    On the other hand, perhaps a helpful community member notices the lack of signalling at this stage and alerts the miner of the impending doom. And SPV clients could use it to anticipate a bumpy period of small reorgs.

  12. achow101 commented at 8:12 pm on April 30, 2025: member

    ACK 0750249289c092fc8e2e29669fec73a58b873767

    This was a little bit confusing to me when reviewing #29039

  13. janb84 commented at 12:03 pm on May 1, 2025: contributor

    ACK 0750249

    The rename of the variables results in less confusion of the intention of the code. And the extra comment explaining the workings of the ! prefix helps to clarify intentions/code even further.


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: 2025-05-05 12:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me