Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: master
from

Conversation

@luke-jr
Copy link
Member

luke-jr commented Jan 17, 2020

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

luke-jr added 2 commits Jan 17, 2020
They have been missing since buried deployments were merged
@fanquake fanquake added the Mining label Jan 17, 2020
@fanquake fanquake requested a review from jnewbery Jan 17, 2020
@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Jan 17, 2020

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

This comment has been minimized.

Copy link
Member

jnewbery commented Jan 17, 2020

Discussion in 16060 is here: #16060 (comment)

@luke-jr

This comment has been minimized.

Copy link
Member Author

luke-jr commented Jan 17, 2020

(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.)

Copy link
Member

jnewbery left a comment

@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

This comment has been minimized.

Copy link
Member Author

luke-jr commented Jan 17, 2020

#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

This comment has been minimized.

Copy link
Member

jnewbery commented Jan 17, 2020

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

This comment has been minimized.

Copy link
Member Author

luke-jr commented Jan 18, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.