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

rpc/gui: Remove 'Unknown block versions being mined' warning #15471

Merged
merged 1 commit into from Feb 26, 2019

Conversation

Projects
None yet
@laanwj
Copy link
Member

laanwj commented Feb 25, 2019

Due to miners inserting garbage into the version numbers causing false positives, the current version signalling has become completely useless. This removes the "unknown block versions" warning which has the tendency to scare users unnecessarily (and might get them to "update" to something bad).

It preserves the warning in the logs. Whether this is desirable can be a point of discussion.

@laanwj laanwj force-pushed the laanwj:2019_02_false_positives branch from 36b75fc to 6154e50 Feb 25, 2019

@laanwj laanwj added this to the 0.18.0 milestone Feb 25, 2019

@gmaxwell

This comment has been minimized.

Copy link
Member

gmaxwell commented Feb 25, 2019

Regretful concept ACK. Logs are also problematic (people having one problem see the false 'errors' and waste their time trying to eliminate them) but less urgent and can be addresses separately.

@laanwj laanwj force-pushed the laanwj:2019_02_false_positives branch from 6154e50 to 211c5f7 Feb 25, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Feb 25, 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:

  • #14954 (build: Require python 3.5 by MarcoFalke)

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.

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Feb 25, 2019

Concept ACK and tACK 6154e50 (update: MarcoFalke found an issue). Note that there's currently no test for -alertnotify large fork notifications; feature_notifications.py sets this param on the test nodes, but doesn't do anything with it. Add a TODO?

Maybe we can put this back in if something like BIP 320 is accepted, see also #13972 by @btcdrak. @TheBlueMatt does that use the same bits you're using in BetterHash?

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Feb 25, 2019

Note that there's currently no test for -alertnotify large fork notifications; feature_notifications.py sets this param on the test nodes, but doesn't do anything with it. Add a TODO?

OK, makes sense.

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Feb 25, 2019

I haven't seen community support for this protocol change. Do we really want to set a precedent for simply giving in to miner protocol violations?

Edit: To be clear, we should do something - I just don't know what that something is, and simply giving in seems like a bad idea.

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Feb 25, 2019

It seems we have three or four options:

  1. Add a meta warning that the warning is probably nonsense, but could be serious
  2. Drop the warning (this PR currently)
  3. Implement BIP 320:
    a) can be entirely dropped in a future release (so it becomes the current PR)
    b) only if it happens to match existing behavior
    c) if it matches the BetterHash proposal
  4. Make the heuristics a bit smarter, e.g. look for a repeating signal from the same bit

I actually think option 4 might be better; just turn nUpgraded in a vector and check nUpgraded > 100/2 for each bit.

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Feb 25, 2019

This is not a "protocol change", it doesn't change the P2P code at all.

I'd say we should simply do this. An alternative or better warning mechanism can be added later if someone implements it.

If not, this is going to miss 0.18, simple as that, there's no time to do a lot here.

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Feb 25, 2019

Perhaps:

  1. Change the warning to indicate miner protocol violations as a alternative possibility to an upgrade.

?

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Feb 25, 2019

Could do that for the warning in the log. I really think we should remove the one from the GUI and RPC.
protocol violations are not a alternative possibility they're the order of the day and we know that, claiming anything else is just dishonest.

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Feb 25, 2019

ACK on removing the "50 check", but NACK on removing the versionbits softfork activation warning (or test)

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Feb 25, 2019

Good catch @MarcoFalke. It seems like option 4 should be fairly easy to implement in the for (int bit = 0; bit < VERSIONBITS_NUM_BITS; bit++) loop above.

rpc/gui: Remove 'Unknown block versions being mined' warning
Due to miners inserting garbage into the version numbers, the current
version signalling has become completely useless. This removes the
"unknown block versions" warning which has the tendency to scare
users unnecessarily (and might get them to "update" to something
bad).

It preserves the warning in the logs. Whether this is desirable can
be a point of discussion.

@laanwj laanwj force-pushed the laanwj:2019_02_false_positives branch from 374ecda to ef362f2 Feb 25, 2019

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Feb 25, 2019

Added TODO and restored feature_versionbits_warning.py apart from the part testing for the specific warning.

I actually think option 4 might be better; just turn nUpgraded in a vector and check nUpgraded > 100/2 for each bit.

I don't think this is better: when random noise is inserted in the version bits, won't the chance for individual bits still be 50% on average? Could tweak the % of course, but I'm not sure what would reduce the false positive rate enough.

We could ignore the specific bits that the miners are setting, but that would make this effectively #13972.

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Feb 25, 2019

when random noise is inserted in the version bits, won't the chance for individual bits still be 50% on average

Depends on the average percentage of bits set per block. @alecalve you don't happen to have a chart? :-) (I'll make a measurement...)

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Feb 25, 2019

Regretful Concept ACK for either this or reducing the VERSIONBITS_NUM_BITS constant. I don't have a strong feeling either way, whatever the implementer wants to do.

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Feb 25, 2019

utACK ef362f2

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Feb 25, 2019

This IS a protocol change. The version field indicate a rule change in some undefined way. Miners filling it with garbage is redefining it to a nonce instead. Removing the warning is conceding the protocol change.

It's not much different than merging code for a miner-decided and miner-enforced extension block.

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Feb 25, 2019

I wrote a script that checks all block versions form the tip down to 470,000. When it finds 50 out of a rolling 100 blocks with a version number other than 100000000000000000000000000000 it starts an alert. It stops the alert when it finds 100 blocks with version 100000000000000000000000000000. This happened 66 times. It roughly represents how many times a user could have seen a fresh alert.

When I change the heuristic to only showing an alert when the same bit is set for 50 out of 100 blocks, it only triggers for SegWit signalling on bit 1 and BIP-91 signalling on bit 2.

If I lower the threshold to 30% it starts seeing false positives (?) recently on bit 22 and 23.

@gmaxwell

This comment has been minimized.

Copy link
Member

gmaxwell commented Feb 25, 2019

Edit: To be clear, we should do something

Something simple like a removal is the only 'something' we can do for the 0.18 release without blowing up the schedule. Removing this warning for now doesn't preclude doing something else later.

Keep in mind at the end of the day this issue is just a spurious warning. We should conserve our efforts for important issues. It's hard to justify making too much of a fuss about it.

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Feb 25, 2019

Some clarification: this PR does not remove alerts for unknown BIP9 activations. Here's the code from master:

schermafbeelding 2019-02-25 om 19 54 29

Line 2244 displays a warning if an unknown rule is activated with more than the BIP9 activation threshold. This PR doesn't touch that.

The alert at 2266 is what we're removing here. It essentially warns that there is some unexpected activity in the version field, in more than 50 out of 100 blocks. That could indicate a future soft fork where we lowered the BIP9 threshold, or where made a version bit permanent (resulting in a permanent warning). I think that's still useful to know.

If we track this per bit then we'll get fewer false positives, but we can still detect a potential soft fork with lower than BIP9 threshold, as well as detect a permanent version bit.

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Feb 25, 2019

Actually implementing per bit counting seems a bit involved (expand BIP9Stats and/or ThresholdState?), so I think I agree with @gmaxwell to just remove the spurious warnings completely for now and then doing something better later.

@gmaxwell

This comment has been minimized.

Copy link
Member

gmaxwell commented Feb 25, 2019

There are lots of other options, for example we could move version signing to be in the coinbase txn. As an aside, the existing warnings are probably ruined now-- if you google them you find lots of answers where people are being told they're spurious. So in the case they're real users won't be correctly warned, and the false positives still cause damage with users trying to 'fix' an unfixable non-issue.

@jamesob
Copy link
Member

jamesob left a comment

ACK ef362f2

This log message was pretty disconcerting the first time I saw it.

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Feb 25, 2019

Per-bit counting seems like a fair bug fix for now. I don't know how complicated it would be, but as a bugfix, there's no freeze.

If we remove the warning, it may be hard to add it back later if miners are still abusing the version field.

Bad search result advice can be mitigated by changing the message language.

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Feb 25, 2019

Concept ACK

@moneyball

This comment has been minimized.

Copy link
Contributor

moneyball commented Feb 26, 2019

Concept ACK although unclear to me the best approach now and later. I too was alarmed and confused when I first saw this message, Google'd, and found others sharing their concerns on Reddit. Agree it needs to be dealt with somehow in v0.18.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Feb 26, 2019

utACK ef362f2

@MarcoFalke MarcoFalke merged commit ef362f2 into bitcoin:master Feb 26, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Feb 26, 2019

Merge #15471: rpc/gui: Remove 'Unknown block versions being mined' wa…
…rning

ef362f2 rpc/gui: Remove 'Unknown block versions being mined' warning (Wladimir J. van der Laan)

Pull request description:

  Due to miners inserting garbage into the version numbers causing false positives, the current version signalling has become completely useless. This removes the "unknown block versions" warning which has the tendency to scare users unnecessarily (and might get them to "update" to something bad).

  It preserves the warning in the logs. Whether this is desirable can be a point of discussion.

Tree-SHA512: 51407ccd24a571462465d9c7180f0f28307c50b82a03284abe783e181d8ab7e0638dbb710698d883f28de8a609db70763e39be2470d956e67c833da0768e43e9
@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Feb 26, 2019

This is a minimal patch. A more involved solution (like BIP320 #13972 or similar) with a new reworded warning can later be added, if desired

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.