-
Notifications
You must be signed in to change notification settings - Fork 5.9k
BIP 141 and 145: GBT updates for SegWit #365
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
Conversation
bip-0009.mediawiki
Outdated
|
|
||
| Each soft fork deployment is specified by the following per-chain parameters (further elaborated below): | ||
|
|
||
| # The '''name''' specifies a very brief description of the soft fork, reasonable for use as an identifier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest the for most of the time, most softforks will role out one consensus BIP so the sensible name would be bipxxxx unless you have a case like with the bip68+112+113 softfork, in which case we labelled that csv. Therefore I would suggest default of bipxxxx unless it's a bundle.
Thinking more, we probably need a table of soft forks somewhere with name, bip(s), starttime, timeout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is outside the scope of this PR, IMO.
BIP141: Sigop clarification
|
The nVersion field of a block never affects the rules applied, so a miner can always be free to set the nVersion to anything. |
|
@sipa Which part are you referring to? GBT servers may very well want to restrict the nVersion to things they can handle. |
|
I had interpreted "the server requires support for" as rules the server demanded you to enforce, rather than rules the server demands you to set bits for. It's a bit confusing that rules and rulesenforced are about consensus rules, and rulesrequires is about nVersion bits to be set. Perhaps replace rulesrequired with versionrequired, with an integer of bits the server requires you to set in nversion? |
|
I would have grouped "rules" and "rulesrequired" together, since they deal with bits available/allowed and required; vs "rulesenforced" which is the actual consensus rules being used. Maybe "vbits" and "vbitsrequired"? |
|
ACK BIP141 |
|
@luke-jr vbits would just be the same as version, I think? Isn't this sufficient:
? |
|
@sipa How will clients know what bits are available and what they mean in context? |
|
@luke-jr What do you mean by "available"? |
|
@sipa Bits which have a defined meaning. |
|
@luke-jr Why does that matter? If the bits are not active, they have no relevance for mining the block, as there are never any consensus rules in BIP9 that prevent you from setting any combination at all of version bits. The server may insist that you set some to vote for, but apart from that, that information is not relevant to the client. |
|
The client needs the information to know which bits it wants to or doesn't want to set. |
|
They can find that in the BIP.
If they need to rely on the server for that, the server can claim anything,
including claiming that there is nothing to vote for.
|
|
So put each and every BIP into the GBT client, even before it has activated? Consider that the server trusted for name:bit mappings could very well be your own local bitcoind. |
|
Also, even if each BIP were put into the GBT client, the client doesn't have the required blockchain context to determine if the BIP has activated or is still pending. |
|
If the client software doesn't know about a BIP, why does it care? How does it help that the server says "these arbitrary string names exist", |
|
@luke-jr Yes, saying with rules are active is useful. Saying which exist |
|
For example, a miner might want to use --veto-vbits=bip109 to instruct their software to automatically unset the BIP 109 bit, and reject pools that require it. Or --require-vbits=bip109 for the opposite. |
|
@luke-jr What if the server doesn't know about a particular BIP yet, but the miner does? If the miner has an opinion about a particular BIP/deployment, they also know what bits to use. |
|
If the server doesn't support it, it shouldn't be indicating it does regardless. |
|
The changes in BIP22 are just formatting? |
|
Yes, someone noticed the {{templates}} were not rendering on GitHub. |
|
ACK 141 |
|
Concept ACK on GBT changes. It's a bit hazy though as to what the server should do if a softfork rule is missing from the request, and there's the possibility of altering the block (potentially in a nasty way, like coinbase-only) to make it compatible. In segwit's case, would the lack of a "segwit" rule in the request cause an error post-activation, or would witness transactions be filtered out? |
The server has a few options:
|
|
Right, and that ambiguity is kinda nasty without a proposed implementation. I'd go as far as saying that it should never throw an error if there's the possibility of somehow creating a valid block. Maybe another input flag could be added for "fail for degraded blocks". Regardless, this needs to be laid out in the BIP. |
|
Which method a server uses in this situation is implementation-specific, not part of the standard. In any case, it is already documented clearly here: https://github.com/bitcoin/bips/pull/365/files#diff-6a9961b118279d111faf86ad6ebbfedbR204 Please feel free to proposed improved language. |
|
NACK that. Where preferences can be communicated, they should be, and they should be respected by the server. Otherwise what ends up being "part of the standard" is the default behavior of the leading implementation. |
|
I don't understand what you're NACKing. |
|
I'm NACKing the language that makes the behavior implementation-specific. I'm suggest changes to either:
Along with the addition of text along the lines of With those changes, it would be clear what the implementation for Core should look like. |
|
Any item in "rules" the client does not understand implies it must error or (if "rules/force" is allowed) "is degraded". |
|
Force (as I read it) doesn't necessarily mean degraded. It could simply mean that a serialization change would break the block, but that transactions haven't been removed. For example some future soft-fork that specifies a required order for transactions. More importantly though, the text above guarantees backwards-compatibility when possible, as opposed to letting the implementation decide. |
|
A softfork requiring ordered transactions would necessarily need to a new "rules" entry. When there are no rules that the client doesn't understand, "rules/force" is a no-op. It's not the business of the spec to force backward compatibility. Implementations that don't want to provide it should be free not to. |
bip-0009.mediawiki
Outdated
| On the other hand, when this prefix is used, it indicates a more subtle change to the block structure or generation transaction; examples of this would be BIP 34 (because it modifies coinbase construction) and 141 (since it modifies the txid hashing and adds a commitment to the generation transaction). | ||
| A client that does not understand a rule prefixed by '!' must not attempt to process the template, and must not attempt to use it for mining even unmodified. | ||
|
|
||
| Servers should only signal this mutation when they have verified a client behaving this way will not produce invalid blocks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this section still match the text before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it was left over. Removed.
| Sigops per block is currently limited to 20,000. We change this restriction as follows: | ||
|
|
||
| ''Sigop cost'' is defined. The cost of a sigop in traditional script is 4, while the cost of a sigop in witness program is 1. | ||
| Sigops in the current pubkey script, signature script, and P2SH check script are counted at 4 times their previous value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for removing the sigop cost terminology? I thought it was useful to distinguish from the original sigops definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how the original sigops definition is relevant post-segwit. ie, what's the point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoiding confusion :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally find it more confusing to have two terms for essentially the same thing of different versions... should GBT be changed to remove sigops/sigoplimit and add sigopcost/sigopcostlimit as well?
|
@theuni Have your concerns been addressed? |
e8e5d5b to
ce426ab
Compare
|
ACK |
@sipa @CodeShark @jl2012