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

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

Details

Reviewers

jasonbcox
Fabien
schancel
deadalnix
nakihito

Group Reviewers

Restricted Project

Commits

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

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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markblundeberg created this revision.Jun 3 2019, 14:36

Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 3 2019, 14:36

Herald added a reviewer: deadalnix. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

@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.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B6117: Diff 9081.Jun 3 2019, 14:51

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.

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 marked 2 inline comments as done.Jun 6 2019, 04:29

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.