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

Restore warning for individual unknown version bits, as well as unknown version schemas #15861

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Apr 20, 2019

Prior to #15471, we had a warning for 50% of versions being unexpected as a whole. Overt ASICBoost abuses broke that, so it was removed.

This restores the warning, but only looks for 50% of each version bit independently, or 50% with an unknown version schema. Hopefully this shouldn't get triggered by (or at least can be more practically avoided by) ASICBoost stuff.

Additionally, it changes the phrasing of the warning to reflect the uncertainty of the cause, and avoid searches from turning up "not to worry" responses (that were given to the now-removed warning).

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Apr 20, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #19898 (log: print unexpected version warning in validation log category by n-thumann)

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.

@gmaxwell
Copy link
Contributor

@gmaxwell gmaxwell commented Apr 21, 2019

I think I'd rather (or additionally) ignore some bits for warning... the issue with bit-limiting it is that it could still false trigger.

@luke-jr
Copy link
Member Author

@luke-jr luke-jr commented Apr 21, 2019

Only if a majority of miners violate the protocol. To set aside bits for ASICBoost is a protocol change, that hasn't had community support demonstrated for. Developers alone don't get to make that call.

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Apr 22, 2019

Tend to NACK. Seems a lot of code for a feature that has no use case.

@gmaxwell
Copy link
Contributor

@gmaxwell gmaxwell commented Apr 22, 2019

NAK. Doesn't actually fix the thing it intends to fix.

@luke-jr
Copy link
Member Author

@luke-jr luke-jr commented Apr 22, 2019

The use case is to provide a proper warning that the protocol rules may be changing and the user should consider upgrading (either to the new rules, or a rule-blocking fork).

NAK. Doesn't actually fix the thing it intends to fix.

If there's a bug in it, report the bug...

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Apr 22, 2019

If a future softfork is activated though versionbits on a single bit in less than 100 blocks, you might as well leave away signalling altogether. For other versionbits-activated (i.e. BIP-9 compatible) softforks we already warn appropriately before your change.

@luke-jr
Copy link
Member Author

@luke-jr luke-jr commented Apr 22, 2019

BIP 9 does not require the expected 95% for the other warning, and we currently lack any warning for different schemas.

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Apr 22, 2019

currently lack any warning for different schemas

Indeed. And your code doens't offer a warning for any kind of schema either. So I'd prefer if we just embraced that fact and avoided any softfork deployments that are incompatible with the existing BIP9 warning mechanism.

@luke-jr
Copy link
Member Author

@luke-jr luke-jr commented Apr 22, 2019

Yes, it does... And again, the other warning doesn't work for all potential BIP 9 deployments either (they can use a lower threshold). On top of that, it's generally accepted that BIP 9 was a failure, and it's pretty likely future deployments will use other mechanisms.

@Seccour
Copy link

@Seccour Seccour commented Apr 25, 2019

Concept ACK. Miners not using the version numbers as intended should not be a reason to remove the warning.

Only if a majority of miners violate the protocol. To set aside bits for ASICBoost is a protocol change, that hasn't had community support demonstrated for. Developers alone don't get to make that call.

Setting aside bits for ASICBoost should have community support. I agree with Luke on that one.

@jnewbery
Copy link
Member

@jnewbery jnewbery commented Apr 25, 2019

Concept NACK for the reasons @MarcoFalke gives

@luke-jr luke-jr force-pushed the luke-jr:restore_vbits_warning branch from 1e8361c to f7fa445 May 2, 2019
@luke-jr luke-jr force-pushed the luke-jr:restore_vbits_warning branch from f7fa445 to a0922c9 Oct 25, 2019
@luke-jr
Copy link
Member Author

@luke-jr luke-jr commented Oct 25, 2019

Rebased

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Oct 25, 2019

Rebased

Why? This has three NACKs and one Concept ACK, so at the least it seems controversial.

@luke-jr
Copy link
Member Author

@luke-jr luke-jr commented Oct 25, 2019

The NACKs are all based on the false claims, so should be ignored (or substantiated).

@luke-jr
Copy link
Member Author

@luke-jr luke-jr commented Oct 25, 2019

Per CONTRIBUTING.md:

NACK needs to include a rationale why the change is not worthwhile.
NACKs without accompanying reasoning may be disregarded.

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Oct 25, 2019

We are talking about future events, so there is no clear true or false. My point is that we should build protocols that encourage safe use with little surprises. For example, using a threshold that is not recommended in BIP 9 is surprising in my eyes and might even lead to consensus deployment issues. Any BIP or code change that explicitly makes use of that or implicitly encourages it will be NACKed by me for exactly that reasoning.

@luke-jr
Copy link
Member Author

@luke-jr luke-jr commented Oct 25, 2019

You said it "has no use case", which it clearly does. You also claimed it "doesn't offer a warning for any kind of schema either", which is also false, as it has a check for different version schemas.

@gmaxwell said it doesn't fix the issue intended (lack of a warning), which again, it clearly does.

Neither of these deal with future events, just present realities (in which the claims are false).

@rebroad
Copy link
Contributor

@rebroad rebroad commented Aug 20, 2020

@luke-jr If it "clearly" has a use case then why would someone be suggesting it does not? It seems it is clearly not clear to them. Also, "a check for different version schemas" does not seem equal to "a warning for any kind of version schema", so I am not sure you have rebutted the basis for the NACK here yet.

Seems so close to understanding being reached here.... just a little more effort from both sides..?

@luke-jr luke-jr force-pushed the luke-jr:restore_vbits_warning branch from 9de382a to dd0ff04 Aug 20, 2020
@luke-jr
Copy link
Member Author

@luke-jr luke-jr commented Aug 20, 2020

Rebased

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Sep 29, 2020

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@luke-jr luke-jr force-pushed the luke-jr:restore_vbits_warning branch from dd0ff04 to ba7a84a Dec 3, 2020
@luke-jr luke-jr force-pushed the luke-jr:restore_vbits_warning branch from ba7a84a to f016cd4 Dec 15, 2020
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

8 participants