Page MenuHomePhabricator

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

Authored by markblundeberg on Jun 3 2019, 14:36.

Details

Summary

PR15471 backport https://github.com/bitcoin/bitcoin/pull/15471/files
ef362f2773 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.

Depends on D3230

Test Plan

make check
test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
arcpatch-D3181
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 6179
Build 10406: Bitcoin ABC Buildbot (legacy)
Build 10405: arc lint + arc unit

Event Timeline

@schancel this fixes the functional test failure you had in D2788. Would still be worth removing the "%d of last 100 blocks have unexpected version" debug.log noise as well, IMO.

jasonbcox requested changes to this revision.Jun 3 2019, 20:36
jasonbcox added inline comments.
test/functional/feature_notifications.py
75 ↗(On Diff #9081)

Can you double check this? feature_notifications.py should be testing -alertnotify, so this comment is likely out of date (or just out of orderly backported).

This revision now requires changes to proceed.Jun 3 2019, 20:36
deadalnix requested changes to this revision.Jun 4 2019, 13:57
deadalnix added inline comments.
src/validation.cpp
2176 ↗(On Diff #9081)

This code doesn't match what is in the PR. This usually indicate that some previous backport is missing. If that's the case, this previous backport will become harder to backport. Unless there is a good reason to not do it, backporting this previous PR.

nakihito requested changes to this revision.EditedJun 6 2019, 00:28
nakihito added a subscriber: nakihito.

I went digging through the blame for Core's validation.cpp and found this: https://github.com/bitcoin/bitcoin/pull/10464 which seems to fix at least the unused fWarned variable and address part of @deadalnix's point, though I don't think its the only intermediary PR.

Edit: Please add dependency to summary https://reviews.bitcoinabc.org/D3230

src/validation.cpp
2144 ↗(On Diff #9081)

Although it was already mentioned by @deadalnix about the below block of code, if there's a left over unused variable after a backport, it probably means there was at least one missing backport between us and this PR that would fix the issue.

markblundeberg added inline comments.
test/functional/feature_notifications.py
75 ↗(On Diff #9081)

Yes that's right, at least for ABC -- we added a test of invalid large-work fork in D1951 (the code that comes after this). Though we don't have tests for valid large-work fork notification.

Core still doesn't have such any such test for either purpose.

This revision is now accepted and ready to land.Jun 12 2019, 20:53