feat: support for pipelined consensus algorithm #24912
Conversation
…d consensus algorithm such as HotStuff-pipeline
📝 WalkthroughWalkthroughThe changes introduce a priority queue (min-heap) to manage multiple finalize states in the state manager, replacing the previous single-state approach. Methods for getting, setting, and clearing finalize states now interact with this heap. Block processing logic is updated to ensure finalize state selection matches block height and hash, improving accuracy during block replay or finalization. Changes
Sequence Diagram(s)sequenceDiagram
participant ConsensusEngine
participant BaseApp
participant StateManager (MinHeap)
participant Store
ConsensusEngine->>BaseApp: ProcessProposal(req)
BaseApp->>StateManager: SetState(ExecModeFinalize, ..., header{ConsensusHash: req.Hash}, ...)
StateManager->>StateManager: Push new State onto MinHeap
ConsensusEngine->>BaseApp: internalFinalizeBlock(height, hash)
loop Until matching finalize state found or heap exhausted
BaseApp->>StateManager: GetState(ExecModeFinalize)
alt Heap empty or no match
BaseApp->>StateManager: ClearState(ExecModeFinalize)
else Matching state found
BaseApp->>Store: Use state for finalization
end
end
alt No matching state found after loop
BaseApp->>StateManager: SetState(ExecModeFinalize, ..., header, ...)
end
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
baseapp/abci.go (1)
749-770: Consider improving readability of the state matching logic.The implementation correctly handles finding the appropriate finalize state from the heap. However, the condition on line 763 could be more readable:
- if (req.Height == app.initialHeight && firstState.Context().HeaderInfo().Height == app.initialHeight) || (req.Height == firstState.Context().HeaderInfo().Height && bytes.Equal(req.Hash, firstState.Context().HeaderInfo().Hash)) { + // Match state for initial height (special case) or by exact height and hash + isInitialHeightMatch := req.Height == app.initialHeight && firstState.Context().HeaderInfo().Height == app.initialHeight + isExactMatch := req.Height == firstState.Context().HeaderInfo().Height && bytes.Equal(req.Hash, firstState.Context().HeaderInfo().Hash) + + if isInitialHeightMatch || isExactMatch {baseapp/state/manager.go (1)
41-41: Simplify the Less method implementation.The MinHeap implementation correctly orders states by height and time. However, the Less method can be simplified:
func (h MinHeap) Less(i, j int) bool { - if h[i].ctx.BlockHeight() < h[j].ctx.BlockHeight() { - return true - } else if h[i].ctx.BlockHeight() == h[j].ctx.BlockHeight() && h[i].ctx.BlockTime().Before(h[j].ctx.BlockTime()) { - return true - } else { - return false - } + if h[i].ctx.BlockHeight() != h[j].ctx.BlockHeight() { + return h[i].ctx.BlockHeight() < h[j].ctx.BlockHeight() + } + return h[i].ctx.BlockTime().Before(h[j].ctx.BlockTime()) }Also applies to: 47-59
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (2)
baseapp/abci.go(3 hunks)baseapp/state/manager.go(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
baseapp/abci.go (2)
baseapp/state/state.go (1)
State(11-16)types/context.go (1)
Context(40-67)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (6)
baseapp/abci.go (1)
493-493: LGTM! Consensus hash properly propagated to state manager.The addition of
ConsensusHashto the header ensures that the block hash is available in the state for later matching during finalization.baseapp/state/manager.go (5)
74-79: LGTM! Proper heap initialization.The MinHeap is correctly initialized as an empty heap with
heap.Initcalled as required by the container/heap package.
88-91: LGTM! Safe retrieval of the minimum state.The implementation correctly returns the top element (minimum by height/time) from the heap with proper nil handling for empty heap.
119-119: LGTM! Hash field properly added to header info.The consensus hash is correctly stored in the header info, enabling state matching by hash in the finalization logic.
143-143: LGTM! States correctly pushed to heap.Using
heap.Pushensures the heap property is maintained when adding new finalize states.
164-166: LGTM! Safe removal of the minimum state.The implementation correctly removes the top element from the heap with proper empty check to prevent panic.
|
Any thoughts on this? @aljo242 |
1 participant
This PR supports multiple
finalizeStateso that the pipelined consensus algorithm (such as HotStuff) could work with cosmos-sdk.Problem
A pipelined consensus algorithm, as its name suggests, works in a pipelined fashion: voting on trailing blocks will be considered as extra rounds of voting on the parent block, and each block has 3 rounds of voting before it's committed.
So, in terms of the ABCI interface, they will be calling the application in this sequence:
PrepareProposal(N) -> ProcessProposal(N) -> PrepareProposal(N+1) -> ProcessProposal (N+1) -> PrepareProposal(N+2) -> ProcessProposal(N+2) -> FinalizeBlock(N)Which is quite different from a PBFT algorithm (such as cometBFT)
PrepareProposal(N) -> ProcessProposal(N) -> FinalizeBlock(N)In the current implementation,
finalizeStateis just one state, and it's updated inInitChainandProcessProposal, however, this is problematic for a pipelined calling sequence. In the case above, after callingProcessProposal(N+2), the finalizeState will be updated to (N+2), and when we try to finalize (N), we can't get the correct state.Description
In the commits of this PR, I kept the interface of the state manager as-is for easier integration. In the state manager,
finalizeStateis now defined as a sorted heap (by height and timestamp in headInfo). This way, ProcessProposal only pushs state into the heap, and in FinalizeBlock the state is finalized only if it matches with request by height and hash.Summary by CodeRabbit