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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
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?
Note that there’s currently no test for
-alertnotifylarge fork notifications;feature_notifications.pysets this param on the test nodes, but doesn’t do anything with it. Add a TODO?
OK, makes sense.
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.
It seems we have three or four options:
I actually think option 4 might be better; just turn nUpgraded in a vector and check nUpgraded > 100/2 for each bit.
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.
Perhaps:
?
20-VB_TOP_BITS = 0x20000000
21-VB_UNKNOWN_BIT = 27 # Choose a bit unassigned to any deployment
22-VB_UNKNOWN_VERSION = VB_TOP_BITS | (1 << VB_UNKNOWN_BIT)
23-
24-WARN_UNKNOWN_RULES_MINED = "Unknown block versions being mined! It's possible unknown rules are in effect"
25-WARN_UNKNOWN_RULES_ACTIVE = "unknown new rules activated (versionbit {})".format(VB_UNKNOWN_BIT)
Why is this test removed?
this warning is still in the code and is still triggered in the gui, rpc and alertnotify
for (int bit = 0; bit < VERSIONBITS_NUM_BITS; bit++) loop above.
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.
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
nUpgradedin a vector and checknUpgraded > 100/2for 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.
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.
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.
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.
Some clarification: this PR does not remove alerts for unknown BIP9 activations. Here’s the code from master:
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.
ACK https://github.com/bitcoin/bitcoin/pull/15471/commits/ef362f27733e9281062b4e65a26e5696f34692b4
This log message was pretty disconcerting the first time I saw it.
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.
Labels
GUI
RPC/REST/ZMQ
Milestone
0.18.0