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

pull luke-jr wants to merge 2 commits into bitcoin:master from luke-jr:fix_gbt_buried changing 2 files +4 −6

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.

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

They have been missing since buried deployments were merged

2abe8cc3b7

QA: feature_segwit: Check that template "rules" includes "!segwit" as appropriate 412d5fe879
fanquake added the label Mining on Jan 17, 2020
fanquake requested review from jnewbery on Jan 17, 2020
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"
jnewbery commented at 5:47 PM on January 17, 2020: member

Discussion in 16060 is here: #16060 (review)
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.)
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.
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).
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.
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.
laanwj added the label Needs backport (0.19) on Jan 20, 2020
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.
jameshilliard approved
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
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
jameshilliard commented at 7:22 PM on May 18, 2020: contributor

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

ACK 412d5fe8791c417bf46fc55a5bb8d59be98a33db
jnewbery commented at 8:41 PM on May 18, 2020: member

Thanks for the references, James.

Tested ACK 412d5fe8791c417bf46fc55a5bb8d59be98a33db.
MarcoFalke added the label RPC/REST/ZMQ on May 19, 2020
MarcoFalke merged this on May 19, 2020
MarcoFalke closed this on May 19, 2020
jnewbery commented at 1:50 PM on May 19, 2020: member

This should be backported to v0.20.
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.
MarcoFalke added the label Needs backport (0.20) on May 19, 2020
sidhujag referenced this in commit 2b8f1168f5 on May 19, 2020
MarcoFalke removed the label Needs backport (0.20) on May 19, 2020
MarcoFalke added this to the milestone 0.19.2 on May 19, 2020
fanquake referenced this in commit bde6a5a676 on May 20, 2020
fanquake referenced this in commit 0d87a5b4e2 on May 20, 2020
fanquake removed the label Needs backport (0.19) on May 20, 2020
MarcoFalke referenced this in commit e42c959c1d on Jun 2, 2020
MarcoFalke referenced this in commit 28a9df7d76 on Aug 11, 2020
MarkLTZ referenced this in commit 611bc92618 on Oct 22, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Milestone
0.19.2

Linked (view graph)

#16060 Bury bip9 deployments #19019 [0.20] Fix GBT: Restore "!segwit" and "csv" to "rules" key #19025 [0.19] Backports