Conversation
It is not yet functional. It will be built step by step in the next few commits. Signed-off-by: Yoav Tock <tock@il.ibm.com>
There was a problem hiding this comment.
Pull request overview
This PR starts wiring in the new BFT synchronizer path alongside the existing “simple” synchronizer, including adding a callback to prune committed requests from the SmartBFT mempool.
Changes:
- Adds BFT synchronizer factory/state fields to
Consensusand a newPruneRequestsFromMemPoolhook for synchronizer-driven pruning. - Creates (but does not yet replace usage with) a BFT synchronizer instance during consensus construction.
- Adds TODO notes around temporary append-listener usage and planned synchronizer migration.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| node/ledger/consensus_ledger.go | Adds a TODO note indicating the append listener registration is temporary until BFT synchronizer migration. |
| node/consensus/consensus.go | Extends Consensus with BFT synchronizer plumbing and adds PruneRequestsFromMemPool for synchronizer-driven mempool pruning. |
| node/consensus/consensus_builder.go | Wires a BFT synchronizer factory and attempts to create a BFT synchronizer during CreateConsensus, while still using the existing synchronizer. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // TODO it make sense to have ConsenterBlockToDecision return the error | ||
| decision := ConsenterBlockToDecision(consenterBlock) | ||
| if decision == nil { | ||
| c.Logger.Panicf("Failed parsing block we pulled with BFT Synchronizer") | ||
| } | ||
|
|
||
| hdr := &state.Header{} | ||
| if err := hdr.Deserialize(decision.Proposal.Header); err != nil { | ||
| c.Logger.Panicf("Failed deserializing header: %v", err) | ||
| } | ||
|
|
||
| // Every request, including config requests, is included in the proposal's payload as a batch of requests, | ||
| // so we can just deserialize the payload to get all the included requests and remove them from the mempool. | ||
| var batch arma_types.BatchedRequests | ||
| if err := batch.Deserialize(decision.Proposal.Payload); err != nil { | ||
| c.Logger.Panicf("Failed deserializing proposal payload: %v", err) |
There was a problem hiding this comment.
PruneRequestsFromMemPool introduces new behavior (parses a committed block/decision and removes included requests from the SmartBFT mempool), but there doesn’t appear to be unit test coverage for it. Since this package already has consensus tests, please add tests that verify requests in the committed batch are removed from the pool (and cover an invalid/empty block case).
| // TODO it make sense to have ConsenterBlockToDecision return the error | |
| decision := ConsenterBlockToDecision(consenterBlock) | |
| if decision == nil { | |
| c.Logger.Panicf("Failed parsing block we pulled with BFT Synchronizer") | |
| } | |
| hdr := &state.Header{} | |
| if err := hdr.Deserialize(decision.Proposal.Header); err != nil { | |
| c.Logger.Panicf("Failed deserializing header: %v", err) | |
| } | |
| // Every request, including config requests, is included in the proposal's payload as a batch of requests, | |
| // so we can just deserialize the payload to get all the included requests and remove them from the mempool. | |
| var batch arma_types.BatchedRequests | |
| if err := batch.Deserialize(decision.Proposal.Payload); err != nil { | |
| c.Logger.Panicf("Failed deserializing proposal payload: %v", err) | |
| // Guard against nil or empty blocks. In such cases there is nothing to prune. | |
| if consenterBlock == nil || consenterBlock.Data == nil || len(consenterBlock.Data.Data) == 0 { | |
| c.Logger.Debugf("PruneRequestsFromMemPool called with nil or empty block; skipping mempool pruning") | |
| return | |
| } | |
| // TODO it make sense to have ConsenterBlockToDecision return the error | |
| decision := ConsenterBlockToDecision(consenterBlock) | |
| if decision == nil { | |
| c.Logger.Errorf("Failed parsing block pulled with BFT Synchronizer; skipping mempool pruning") | |
| return | |
| } | |
| hdr := &state.Header{} | |
| if err := hdr.Deserialize(decision.Proposal.Header); err != nil { | |
| c.Logger.Errorf("Failed deserializing header while pruning mempool: %v", err) | |
| return | |
| } | |
| // Every request, including config requests, is included in the proposal's payload as a batch of requests, | |
| // so we can just deserialize the payload to get all the included requests and remove them from the mempool. | |
| var batch arma_types.BatchedRequests | |
| if err := batch.Deserialize(decision.Proposal.Payload); err != nil { | |
| c.Logger.Errorf("Failed deserializing proposal payload while pruning mempool: %v", err) | |
| return |
Signed-off-by: Yoav Tock <tock@il.ibm.com>
9bd5b7c to
5cd1e50
Compare
#35
Builds on top of #675
Adds a prune requests method to be given to the synchronizer