Merge #16688: log: Add validation interface logging
ClosedPublic
Actions

Authored by jasonbcox on Jun 19 2020, 00:49.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABC04394f92dce5: Merge #16688: log: Add validation interface logging

Summary

f9abf4ab6d3d3e4d4b7e90723020b5422a141a6f Add logging for CValidationInterface events (Jeffrey Czyz)
6edebacb2191373e76d79a4972d6192300976096 Refactor FormatStateMessage for clarity (Jeffrey Czyz)
72f3227c83810936e7a334304e5fd7c6dab8e91b Format CValidationState properly in all cases (Jeffrey Czyz)
428ac70095253225f64462ee15c595644747f376 Add VALIDATION to BCLog::LogFlags (Jeffrey Czyz)

Pull request description:

Add logging of `CValidationInterface` callbacks using a new `VALIDATIONINTERFACE` log flag (see #12994). A separate flag is desirable as the logging can be noisy and thus may need to be disabled without affecting other logging.

This could help debug issues where there may be race conditions at play, such as #12978.

ACKs for top commit:

jnewbery:
  ACK f9abf4ab6d3d3e4d4b7e90723020b5422a141a6f
hebasto:
  ACK f9abf4ab6d3d3e4d4b7e90723020b5422a141a6f
ariard:
  ACK f9abf4a, only changes since 0cadb12 are replacing log indication `VALIDATIONINTERFACE` by `VALIDATION` and avoiding a forward declaration with a new include
ryanofsky:
  Code review ACK f9abf4ab6d3d3e4d4b7e90723020b5422a141a6f. Just suggested changes since last review (thanks!)

Tree-SHA512: 3e0f6e2c8951cf46fbad3ff440971d95d526df2a52a2e4d6452a82785c63d53accfdabae66b0b30e2fe0b00737f8d5cb717edbad1460b63acb11a72c8f5d4236

Partial backport of PR16688
The changes to FormatStateMessage are completely absent because I did not want to mangle them for the sake
of backporting this change, and that particular change depends on a massive stack that isn't necessary to
take advantage of this patch.

Test Plan

ninja check check-functional

Diff Detail

Repository

rABC Bitcoin ABC

Branch

pr16688

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 11583
Build 22441: Default Diff Build & Tests	Link to the build
Build 22440: Build without Wallet	Link to the build
Build 22439: Build with clang-10	Link to the build
Build 22438: arc lint + arc unit

Event Timeline

jasonbcox created this revision.Jun 19 2020, 00:49

Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 19 2020, 00:49

jasonbcox requested review of this revision.Jun 19 2020, 00:49

[Bot Message]
One or more PR numbers were detected in the summary.
Links to those PRs have been inserted into the summary for reference.

jasonbcox added a child revision: D6653: Merge #18524: refactor: drop boost::signals2 in validationinterface.Jun 19 2020, 01:04

Harbormaster completed remote builds in B11583: Diff 21559.Jun 19 2020, 01:14

I don't see what is preventing the change to FormatStateMessage to be part of the backport ? AFAIK the massive stack you mention is a class rename but all the changes from the PR can be done now without conflict.

In D6652#162338, @Fabien wrote:

I don't see what is preventing the change to FormatStateMessage to be part of the backport ? AFAIK the massive stack you mention is a class rename but all the changes from the PR can be done now without conflict.

Trying to backport BlockValidationState depends on a large stack. Trying to backport "only the necessary parts" will leave us in a state where many PRs are partially backported, which is more difficult to track and reason about.
Doing it this way leaves us with one partial backport, which I think is manageable.

Also note that trying to complete this backport with regards to FormatStateMessage changes the message format, which also requires changes to multiple tests. If I go down that route, we might as well just backport all of those
changes since they are part of that stack anyway. So rather than do everything out of order with regards to PRs, this patch is sufficient. We can take the necessary time and care to backport the large stack while gaining the
benefits of this change and the others that rely on it today.

Talked offline, the change that requires a lot of other backports is the removal of the error code from the message.
Since the message change is not related with the intent of this PR, let's move forward.

This revision is now accepted and ready to land.Jun 19 2020, 19:19

Closed by commit rABC04394f92dce5: Merge #16688: log: Add validation interface logging (authored by Wladimir J. van der Laan <laanwj@protonmail.com>, committed by abc-bot). · Explain WhyJun 19 2020, 20:43

This revision was automatically updated to reflect the committed changes.

abc-bot added a commit: rABC04394f92dce5: Merge #16688: log: Add validation interface logging.

jasonbcox mentioned this in D6862: Refactor FormatStateMessage() to better match Core.Jul 9 2020, 00:32

jasonbcox mentioned this in rABCc3bbc613c4b6: Refactor FormatStateMessage() to better match Core.Jul 14 2020, 21:06