Fix: Register all BFT sub-protocols during IBFT2→QBFT consensus migration#1
Conversation
Summary of ChangesHello @saeeddawod, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical issue that caused network stalls during IBFT2 to QBFT consensus migrations in Besu. The fix ensures that nodes correctly register and utilize all required BFT wire protocols throughout the transition, enabling seamless communication and continuous block production across the fork block. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The de-duplication logic relies on
SubProtocol.getName()uniqueness; consider documenting or enforcing the assumption that protocols with the same name (e.g. Eth) are wire-compatible so silently dropping later managers is always safe. - Since you sort by block number and keep the first occurrence when de-duplicating, it would help to clarify in a comment which consensus builder is expected to win in case of duplicate sub-protocol names (earliest vs latest transition) to avoid future confusion when adding new transitions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The de-duplication logic relies on `SubProtocol.getName()` uniqueness; consider documenting or enforcing the assumption that protocols with the same name (e.g. Eth) are wire-compatible so silently dropping later managers is always safe.
- Since you sort by block number and keep the first occurrence when de-duplicating, it would help to clarify in a comment which consensus builder is expected to win in case of duplicate sub-protocol names (earliest vs latest transition) to avoid future confusion when adding new transitions.
## Individual Comments
### Comment 1
<location> `app/src/main/java/org/hyperledger/besu/controller/ConsensusScheduleBesuControllerBuilder.java:237` </location>
<code_context>
+ // We merge all sub-protocol configurations, which registers both:
+ // - IBF/1 (IBFT2) for pre-migration communication
+ // - istanbul/100 (QBFT) for post-migration communication
+ final SubProtocolConfiguration mergedConfig = new SubProtocolConfiguration();
+ final java.util.Set<String> addedProtocolNames = new java.util.HashSet<>();
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the new sub-protocol merging logic into helper methods that encapsulate merging and de-duplication and avoid index-based parallel lists for clearer, safer code.
You can keep the new behavior but simplify the control-flow and data handling by pushing the merging/dedup logic into helpers and avoiding index-based parallel lists.
### 1. Extract the merge logic into a helper
Move the merging into a dedicated method so `createSubProtocolConfiguration` reads as “collect and merge configs” instead of doing all the work inline:
```java
@Override
protected SubProtocolConfiguration createSubProtocolConfiguration(
final EthProtocolManager ethProtocolManager,
final Optional<SnapProtocolManager> maybeSnapProtocolManager) {
final SubProtocolConfiguration mergedConfig = new SubProtocolConfiguration();
for (Map.Entry<Long, BesuControllerBuilder> entry :
besuControllerBuilderSchedule.entrySet().stream()
.sorted(Map.Entry.comparingByKey())
.toList()) {
final SubProtocolConfiguration builderConfig =
entry.getValue().createSubProtocolConfiguration(ethProtocolManager, maybeSnapProtocolManager);
mergeDistinctByName(mergedConfig, builderConfig);
}
return mergedConfig;
}
private void mergeDistinctByName(
final SubProtocolConfiguration target,
final SubProtocolConfiguration source) {
final Set<String> existingNames = new HashSet<>();
target.getSubProtocols().forEach(sp -> existingNames.add(sp.getName()));
final List<SubProtocol> subProtocols = source.getSubProtocols();
final List<ProtocolManager> protocolManagers = source.getProtocolManagers();
for (int i = 0; i < subProtocols.size(); i++) {
final SubProtocol subProtocol = subProtocols.get(i);
if (existingNames.add(subProtocol.getName())) {
target.withSubProtocol(subProtocol, protocolManagers.get(i));
}
}
}
```
This keeps all existing behavior (sorted by block, merged, deduped) but moves the complexity into a named method with a clear responsibility.
### 2. Avoid manual synchronization of parallel lists
If you can add a small helper on `SubProtocolConfiguration` (or a local helper) that returns paired entries, you can remove the index-based loop entirely:
```java
private static final class SubProtocolEntry {
final SubProtocol protocol;
final ProtocolManager manager;
SubProtocolEntry(final SubProtocol protocol, final ProtocolManager manager) {
this.protocol = protocol;
this.manager = manager;
}
}
private List<SubProtocolEntry> entriesOf(final SubProtocolConfiguration config) {
final List<SubProtocolEntry> entries = new ArrayList<>();
final List<SubProtocol> subProtocols = config.getSubProtocols();
final List<ProtocolManager> protocolManagers = config.getProtocolManagers();
for (int i = 0; i < subProtocols.size(); i++) {
entries.add(new SubProtocolEntry(subProtocols.get(i), protocolManagers.get(i)));
}
return entries;
}
private void mergeDistinctByName(
final SubProtocolConfiguration target,
final SubProtocolConfiguration source) {
final Set<String> existingNames = new HashSet<>();
target.getSubProtocols().forEach(sp -> existingNames.add(sp.getName()));
for (SubProtocolEntry entry : entriesOf(source)) {
if (existingNames.add(entry.protocol.getName())) {
target.withSubProtocol(entry.protocol, entry.manager);
}
}
}
```
This still respects the existing API but makes the relationship between `SubProtocol` and `ProtocolManager` explicit and less error-prone.
Either of these small refactors will keep the migration feature and deterministic behavior intact while making the method easier to read and maintain.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a critical bug in the consensus migration from IBFT2 to QBFT by ensuring all necessary BFT sub-protocols are registered. The approach of iterating through all scheduled consensus builders, sorting them for determinism, and merging their sub-protocol configurations is sound. My review includes a few suggestions to improve code clarity and robustness.
| entry.getValue().createSubProtocolConfiguration(ethProtocolManager, maybeSnapProtocolManager); | ||
| final List<SubProtocol> subProtocols = builderConfig.getSubProtocols(); | ||
| final List<ProtocolManager> protocolManagers = builderConfig.getProtocolManagers(); | ||
| for (int i = 0; i < subProtocols.size(); i++) { |
There was a problem hiding this comment.
The code assumes that subProtocols and protocolManagers lists have the same size. This is a common pattern with parallel collections and can be fragile. If protocolManagers were ever shorter than subProtocols, this would cause an IndexOutOfBoundsException. To make the code more robust, I recommend adding a precondition check to validate that the lists are of equal length before iterating, for example:
if (subProtocols.size() != protocolManagers.size()) {
throw new IllegalStateException("Sub-protocols and protocol managers lists must have the same size.");
}| if (!addedProtocolNames.contains(subProtocol.getName())) { | ||
| mergedConfig.withSubProtocol(subProtocol, protocolManager); | ||
| addedProtocolNames.add(subProtocol.getName()); | ||
| } |
There was a problem hiding this comment.
The logic to check for an element's presence in a Set before adding it can be made more concise. The Set.add() method returns true if the element was not already in the set, so you can perform the check and the addition in a single step within the if condition. This is a common Java idiom that improves readability.
if (addedProtocolNames.add(subProtocol.getName())) {
mergedConfig.withSubProtocol(subProtocol, protocolManager);
}2 participants
Summary
This PR fixes a critical bug that prevents successful IBFT2 → QBFT consensus migration in Besu. During migration, nodes would stall and fail to produce blocks because they could only speak one BFT wire protocol instead of both.
Problem
When running a consensus migration from IBFT2 to QBFT using a
transitionsconfiguration in genesis, the network would stall before reaching the fork block. Nodes could not exchange IBFT2 messages even though they were still running IBFT2 consensus pre-fork.Root Cause
In
ConsensusScheduleBesuControllerBuilder.createSubProtocolConfiguration(), the original code was:Issues with this approach:
besuControllerBuilderScheduleis aMap(HashMap), sokeySet().stream().skip(1)produces unpredictable resultsSolution
The fix registers all BFT sub-protocols from all scheduled consensus mechanisms:
Testing
Migration Test Results
Log Evidence
Checklist
🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com
Summary by Sourcery
Register all relevant BFT sub-protocols during consensus transitions to ensure nodes can communicate both before and after IBFT2→QBFT migration.
Bug Fixes:
Enhancements:
Summary by cubic
Fixes IBFT2→QBFT migration by registering all required BFT wire protocols so nodes can communicate before and after the fork and keep producing blocks.
Written for commit c0f000d. Summary will update automatically on new commits.