Added log for validator selection mode transition #2909

lucassaldanha · 2021-10-15T01:53:42Z

PR description

Add a log message when validator selection mode changes between blocks
Notification logic implemented on QbftTransitionNotifier
Check happens upon creation of a new QbftBlockHeighManager in the corresponding factory. That means that we notify before the transition actually happens, to help diagnose any issues with the transition that is about to happen (e.g. wrong address for the validator contract).

Example messages:

Transitioning validator selection mode from blockheader to contract (address: 0x0000000000000000000000000000000000008888)

Transitioning validator selection mode from contract (address: 0x0000000000000000000000000000000000008888) to blockheader

Transitioning validator selection mode from contract (address: 0x0000000000000000000000000000000000008888) to contract (address: 0x0000000000000000000000000000000000007777)

Fixed Issue(s)

fixes #2784

Changelog

I thought about the changelog and included a changelog update if required.

...qbft/src/main/java/org/hyperledger/besu/consensus/qbft/validator/QbftTransitionNotifier.java

jframe · 2021-10-15T05:24:29Z

...qbft/src/main/java/org/hyperledger/besu/consensus/qbft/validator/QbftTransitionNotifier.java

+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+public class QbftTransitionNotifier {


This is more specific than ordinary transitions, perhaps QbftValidatorSelectionTransitionLogger? Though is rather a long name

Since it's already in the qbft package, can we drop the Qbft?

Another suggestion: ValidatorModeTransitionLogger...'mode' has been used elsewhere

I went with ValidatorModeTransitionLogger

jframe · 2021-10-15T05:28:11Z

...qbft/src/main/java/org/hyperledger/besu/consensus/qbft/validator/QbftTransitionNotifier.java

+    this.msgConsumer = msgConsumer;
+  }
+
+  public void checkTransitionChange(final BlockHeader parentHeader) {


This is not really checking anything it doesn't return a value. Maybe just logTransitionChange?

jframe · 2021-10-15T05:31:03Z

...qbft/src/main/java/org/hyperledger/besu/consensus/qbft/validator/QbftTransitionNotifier.java

+
+  private String parseConfigToLog(final QbftConfigOptions configOptions) {
+    if (configOptions.getValidatorContractAddress().isPresent()) {
+      return String.format("ADDRESS(%s)", configOptions.getValidatorContractAddress().get());


contract mode (%s) instead of ADDRESS?

should we make this clear it's the validator contract address? In case someone who doesn't know the detail of how this feature works may read the log.

I guess we can expect that operators (people running Besu nodes) have a fair bit of knowledge. If they are using a feature that requires smart contracts etc. they need to know what they are doing.

jframe · 2021-10-15T05:31:31Z

...qbft/src/main/java/org/hyperledger/besu/consensus/qbft/validator/QbftTransitionNotifier.java

+    if (configOptions.getValidatorContractAddress().isPresent()) {
+      return String.format("ADDRESS(%s)", configOptions.getValidatorContractAddress().get());
+    } else {
+      return "BLOCKHEADER";


blockheader mode instead of BLOCKHEADER in caps?

btw we already have the word mode in the phrase.

usmansaleem · 2021-10-15T05:34:39Z

...qbft/src/main/java/org/hyperledger/besu/consensus/qbft/validator/QbftTransitionNotifier.java

+
+    return !currentConfig
+        .getValidatorContractAddress()
+        .equals(nextConfig.getValidatorContractAddress());


nit: equalsignorecase??

Does it make more sense to normalize this inside QbftConfigOptions?

That would be better actually

siladu · 2021-10-15T07:42:19Z

...qbft/src/main/java/org/hyperledger/besu/consensus/qbft/validator/QbftTransitionNotifier.java

+  }
+
+  @VisibleForTesting
+  public QbftTransitionNotifier(


nit: could be package-private

siladu · 2021-10-15T07:44:20Z

...qbft/src/main/java/org/hyperledger/besu/consensus/qbft/validator/QbftTransitionNotifier.java

+  private static final Logger LOG = LogManager.getLogger();
+
+  private final BftForksSchedule<QbftConfigOptions> bftForksSchedule;
+  private final Consumer<String> msgConsumer;


I like this pattern as a way of testing logging 👍

Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>

jframe · 2021-10-17T23:37:44Z

...ain/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerFactory.java

    this.roundFactory = roundFactory;
    this.finalState = finalState;
    this.messageValidatorFactory = messageValidatorFactory;
    this.messageFactory = messageFactory;
+    this.qbftTransitionNotifier = validatorModeTransitionLogger;


can the field name be updated to match the constructor arg

jframe · 2021-10-17T23:39:33Z

...st/java/org/hyperledger/besu/consensus/qbft/validator/ValidatorModeTransitionLoggerTest.java

+    BftForkSpec<BftConfigOptions> forkSpecA =
+        new BftForkSpec<>(0, createQbftConfigOptionsForBlockHeader());
+    BftForkSpec<BftConfigOptions> forkSpecB =


these can be made final in all the tests

Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>

* Added log for validator selection mode transition Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>

jframe reviewed Oct 15, 2021

View reviewed changes

usmansaleem reviewed Oct 15, 2021

View reviewed changes

siladu reviewed Oct 15, 2021

View reviewed changes

lucassaldanha added 2 commits October 18, 2021 09:13

Added log for validator selection mode transition

e5803a8

Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>

PR Comments

dd4299a

Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>

lucassaldanha force-pushed the besu-2794 branch from a6ae00b to dd4299a Compare October 17, 2021 20:43

jframe reviewed Oct 17, 2021

View reviewed changes

More PR comments

f174d7c

Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>

lucassaldanha enabled auto-merge (squash) October 18, 2021 02:48

jframe approved these changes Oct 18, 2021

View reviewed changes

lucassaldanha merged commit 6be1ed5 into hyperledger:main Oct 18, 2021

lucassaldanha deleted the besu-2794 branch October 18, 2021 03:23

garyschulte pushed a commit to garyschulte/besu that referenced this pull request Oct 22, 2021

Added log for validator selection mode transition (hyperledger#2909)

9f83517

* Added log for validator selection mode transition Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>

eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023

Added log for validator selection mode transition (hyperledger#2909)

e424642

* Added log for validator selection mode transition Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added log for validator selection mode transition #2909

Added log for validator selection mode transition #2909

lucassaldanha commented Oct 15, 2021 •

edited

Loading

jframe Oct 15, 2021

siladu Oct 15, 2021 •

edited

Loading

lucassaldanha Oct 17, 2021

jframe Oct 15, 2021

lucassaldanha Oct 17, 2021

jframe Oct 15, 2021

siladu Oct 15, 2021

lucassaldanha Oct 17, 2021

jframe Oct 15, 2021

lucassaldanha Oct 17, 2021

lucassaldanha Oct 17, 2021

usmansaleem Oct 15, 2021

lucassaldanha Oct 17, 2021

jframe Oct 17, 2021

lucassaldanha Oct 18, 2021

siladu Oct 15, 2021

lucassaldanha Oct 17, 2021

siladu Oct 15, 2021

jframe Oct 17, 2021

lucassaldanha Oct 17, 2021

jframe Oct 17, 2021

lucassaldanha Oct 17, 2021

Added log for validator selection mode transition #2909

Added log for validator selection mode transition #2909

Conversation

lucassaldanha commented Oct 15, 2021 • edited Loading

PR description

Fixed Issue(s)

Changelog

Choose a reason for hiding this comment

siladu Oct 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucassaldanha commented Oct 15, 2021 •

edited

Loading

siladu Oct 15, 2021 •

edited

Loading