Page MenuHomePhabricator

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

Authored by markblundeberg on Mon, Jun 3, 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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markblundeberg created this revision.Mon, Jun 3, 14:36
Herald added a reviewer: Restricted Project. · View Herald TranscriptMon, Jun 3, 14:36

@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.Mon, Jun 3, 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.Mon, Jun 3, 20:36
deadalnix requested changes to this revision.Tue, Jun 4, 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.EditedThu, Jun 6, 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 marked 2 inline comments as done.Thu, Jun 6, 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.

markblundeberg marked an inline comment as done.Thu, Jun 6, 04:31
markblundeberg edited the summary of this revision. (Show Details)
markblundeberg updated this revision to Diff 9185.

rebased onto D3230

Fabien accepted this revision.Thu, Jun 6, 07:44
nakihito accepted this revision.Thu, Jun 6, 17:48
jasonbcox accepted this revision.Thu, Jun 6, 18:03
deadalnix accepted this revision.Wed, Jun 12, 20:53
This revision is now accepted and ready to land.Wed, Jun 12, 20:53

rebase for freshness