Fix GBT: Restore “!segwit” and “csv” to “rules” key #17946

1. 
   luke-jr commented at 5:34 am on January 17, 2020: member

   #16060 removed CSV & segwit from versionbits, breaking the “rules” key returned by GBT.

   Without this, miners don’t know they’re mining segwit blocks, and should fall back to pre-segwit block creation.

2. Bugfix: Include "csv","!segwit" in "rules"

   They have been missing since buried deployments were merged

   2abe8cc3b7
3. QA: feature_segwit: Check that template "rules" includes "!segwit" as appropriate 412d5fe879
4. fanquake added the label Mining on Jan 17, 2020
5. fanquake requested review from jnewbery on Jan 17, 2020
6. 
   ryanofsky commented at 5:08 pm on January 17, 2020: member

   Comments in IRC provide some context: without the information in rules field “miners have no way to know what rules are being used and might produce invalid blocks”
7. 
   jnewbery commented at 5:47 pm on January 17, 2020: member

   Discussion in 16060 is here: #16060 (review)
8. 
   luke-jr commented at 6:14 pm on January 17, 2020: member

   (For clarity, I didn’t mean fixing this bug in a separate PR, but rather the missing "!" and/or bad name/description of the gbt_force flag.)
9. 
   jnewbery commented at 7:00 pm on January 17, 2020: member

   @luke-jr : The rules field hasn’t been populated with “!segwit” since 0.15 (#9955 changed the behaviour so that “segwit” would be included as a rule instead of “!segwit”).

   In IRC you stated that “miners have no way to know what rules are being used and might produce invalid blocks”. Do you have any additional information about that? In what way would miners produce invalid blocks? Do you know of any miners that have had a problem because of this?

   I don’t have any objection to this change, but I’d like to better understand how miners have been impacted by this.

10. 
    luke-jr commented at 7:45 pm on January 17, 2020: member

    #9955 introduced a bug, sure, but that bug would only have impacted Segwit-unaware miners. Segwit-aware miners would have continued to work correctly because they saw that segwit was an active rule.

    Removing the rule entirely means that sigops are counted differently, and transactions may not include witness data. I don’t know if there are any real-world miners impacted by this, but it is at least theoretically possible (IIRC, at least some branches of Eloipool support a safety trim based on sigop counts).

11. 
    jnewbery commented at 8:22 pm on January 17, 2020: member

    that bug would only have impacted Segwit-unaware miners.

    I think you have this the wrong way round. Segwit-unaware miners would see “segwit” in the rules, but according to BIP 9:

    Without this ["!"] prefix, GBT clients may assume the rule will not impact usage of the template as-is;

    and so the segwit-unaware miner would use the template, but not be able to add the witness commitment to the coinbase.

    sigops are counted differently

    Since #14811, the GBT request must include “segwit” in the rule array, so sigops are always counted according to BIP 141 rules. Whether or not the return object includes “segwit” in the rules doesn’t change this.

    In any case, I think it’s fine to make this change because it doesn’t really matter. Segwit has been active for two years and all miners using 0.18 onwards have to set “segwit” in the rules. I find it very hard to believe that any are reading what’s returned in the rules field.

12. 
    luke-jr commented at 1:40 am on January 18, 2020: member

    I think you have this the wrong way round. Segwit-unaware miners would see “segwit” in the rules,

    They would fail by mining invalid blocks, instead of failing explicitly. Either way, they fail.

    Segwit-capable miners, on the other hand, shouldn’t fail, but might because they think they’re mining a non-segwit block.

    Since #14811, the GBT request must include “segwit” in the rule array, so sigops are always counted according to BIP 141 rules.

    The request rule doesn’t affect sigop counting, only indicates the client can understand a segwit template. Sigop counting is enabled only when the template has segwit in its rules list.

13. laanwj added the label Needs backport (0.19) on Jan 20, 2020
14. 
    jameshilliard commented at 1:45 am on May 18, 2020: contributor

    FYI without this mining with ckpool breaks since it will only try to produce segwit blocks when the segwit rule is seen.
15. jameshilliard approved
16. in test/functional/feature_segwit.py:210 in 412d5fe879

    206@@ -212,6 +207,7 @@ def run_test(self):
    207         assert tmpl['sigoplimit'] == 80000
    208         assert tmpl['transactions'][0]['txid'] == txid
    209         assert tmpl['transactions'][0]['sigops'] == 8
    210+        assert '!segwit' in tmpl['rules']
    

    ---

    ---

    

    jameshilliard commented at 3:58 am on May 18, 2020:

    shouldn’t we also test for 'csv' being present here?

    ---

    

    MarcoFalke commented at 12:37 pm on May 19, 2020:

    Going to merge this to not invalidate ACKs. Test improvements are welcome and can be submitted in a follow-up
17. 
    jnewbery commented at 1:19 pm on May 18, 2020: member

    FYI without this mining with ckpool breaks since it will only try to produce segwit blocks when the segwit rule is seen. @jameshilliard can you point me at the code that does that? The only getblocktemplate "rules" handling code I was able to find in ckpool was checking that the gbt response doesn’t contain an unknown rule: https://bitbucket.org/ckolivas/ckpool/src/73f560cf3144f824823077c1b9e1663d89823ed7/src/bitcoin.c#lines-127

18. 
    jameshilliard commented at 7:22 pm on May 18, 2020: contributor

    @jnewbery Well here’s the workaround ck had to apply.
19. 
    sipa commented at 7:36 pm on May 18, 2020: member

    ACK 412d5fe8791c417bf46fc55a5bb8d59be98a33db
20. 
    jnewbery commented at 8:41 pm on May 18, 2020: member

    Thanks for the references, James.

    Tested ACK 412d5fe8791c417bf46fc55a5bb8d59be98a33db.

21. MarcoFalke added the label RPC/REST/ZMQ on May 19, 2020
22. MarcoFalke merged this on May 19, 2020
23. MarcoFalke closed this on May 19, 2020

    ---

24. 
    jnewbery commented at 1:50 pm on May 19, 2020: member

    This should be backported to v0.20.
25. 
    MarcoFalke commented at 2:20 pm on May 19, 2020: member

    This this need a new rc? Maybe not for a two-line change?

    I suggest either creating a backport pr or bringing it up in the meeting, so that it is not forgotten.

26. MarcoFalke added the label Needs backport (0.20) on May 19, 2020
27. sidhujag referenced this in commit 2b8f1168f5 on May 19, 2020
28. MarcoFalke removed the label Needs backport (0.20) on May 19, 2020
29. MarcoFalke added this to the milestone 0.19.2 on May 19, 2020
30. fanquake referenced this in commit bde6a5a676 on May 20, 2020
31. fanquake referenced this in commit 0d87a5b4e2 on May 20, 2020
32. fanquake removed the label Needs backport (0.19) on May 20, 2020
33. MarcoFalke referenced this in commit e42c959c1d on Jun 2, 2020
34. MarcoFalke referenced this in commit 28a9df7d76 on Aug 11, 2020
35. MarkLTZ referenced this in commit 611bc92618 on Oct 22, 2020
36. DrahtBot locked this on Feb 15, 2022

Contributors
luke-jr ryanofsky jnewbery jameshilliard MarcoFalke sipa

Labels
RPC/REST/ZMQ Mining

Milestone
0.19.2