-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: optimistic execution consensus v2 #22560
base: main
Are you sure you want to change the base?
Changes from all commits
0b80553
4807332
907bcd1
a58e20d
082ddec
266d53f
857f14c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -2,6 +2,7 @@ package cometbft | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"context" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"cosmossdk.io/server/v2/cometbft/oe" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"crypto/sha256" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"errors" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"fmt" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -12,7 +13,7 @@ import ( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
abci "github.com/cometbft/cometbft/abci/types" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
abciproto "github.com/cometbft/cometbft/api/cometbft/abci/v1" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
gogoproto "github.com/cosmos/gogoproto/proto" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
protoreflect "google.golang.org/protobuf/reflect/protoreflect" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"google.golang.org/protobuf/reflect/protoreflect" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"google.golang.org/protobuf/reflect/protoregistry" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"cosmossdk.io/collections" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -67,6 +68,11 @@ type Consensus[T transaction.Tx] struct { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
extendVote handlers.ExtendVoteHandler | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
checkTxHandler handlers.CheckTxHandler[T] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// optimisticExec contains the context required for Optimistic Execution, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// including the goroutine handling.This is experimental and must be enabled | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// by developers. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
optimisticExec *oe.OptimisticExecution | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+71
to
+74
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider initializing The |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
addrPeerFilter types.PeerFilter // filter peers by address and port | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
idPeerFilter types.PeerFilter // filter peers by node ID | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -117,6 +123,10 @@ func (c *Consensus[T]) SetStreamingManager(sm streaming.Manager) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
c.streaming = sm | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
func (c *Consensus[T]) SetOptimisticExecution(oe *oe.OptimisticExecution) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
c.optimisticExec = oe | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// RegisterSnapshotExtensions registers the given extensions with the consensus module's snapshot manager. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// It allows additional snapshotter implementations to be used for creating and restoring snapshots. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
func (c *Consensus[T]) RegisterSnapshotExtensions(extensions ...snapshots.ExtensionSnapshotter) error { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -385,6 +395,14 @@ func (c *Consensus[T]) PrepareProposal( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return nil, errors.New("no prepare proposal function was set") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// Abort any running OE so it cannot overlap with `PrepareProposal`. This could happen if optimistic | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// `internalFinalizeBlock` from previous round takes a long time, but consensus has moved on to next round. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// Overlap is undesirable, since `internalFinalizeBlock` and `PrepareProoposal` could share access to | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// in-memory structs depending on application implementation. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// No-op if OE is not enabled. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// Similar call to Abort() is done in `ProcessProposal`. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
c.optimisticExec.Abort() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+398
to
+405
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential nil pointer dereference in In Apply this diff to add a nil check before calling // Similar call to Abort() is done in `ProcessProposal`.
- c.optimisticExec.Abort()
+ if c.optimisticExec != nil {
+ c.optimisticExec.Abort()
+ } 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ciCtx := contextWithCometInfo(ctx, comet.Info{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Evidence: toCoreEvidence(req.Misbehavior), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ValidatorsHash: req.NextValidatorsHash, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -421,6 +439,17 @@ func (c *Consensus[T]) ProcessProposal( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return nil, errors.New("no process proposal function was set") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// Since the application can get access to FinalizeBlock state and write to it, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// we must be sure to reset it in case ProcessProposal timeouts and is called | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// again in a subsequent round. However, we only want to do this after we've | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// processed the first block, as we want to avoid overwriting the finalizeState | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// after state changes during InitChain. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if req.Height > int64(c.initialHeight) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// abort any running OE | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
c.optimisticExec.Abort() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
//c.setState(execModeFinalize, header) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+442
to
+452
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential nil pointer dereference in In Apply this diff to add a nil check before calling if req.Height > int64(c.initialHeight) {
// abort any running OE
- c.optimisticExec.Abort()
+ if c.optimisticExec != nil {
+ c.optimisticExec.Abort()
+ }
//c.setState(execModeFinalize, header)
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ciCtx := contextWithCometInfo(ctx, comet.Info{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Evidence: toCoreEvidence(req.Misbehavior), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ValidatorsHash: req.NextValidatorsHash, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -436,6 +465,17 @@ func (c *Consensus[T]) ProcessProposal( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// Only execute optimistic execution if the proposal is accepted, OE is | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// enabled and the block height is greater than the initial height. During | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// the first block we'll be carrying state from InitChain, so it would be | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// impossible for us to easily revert. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// After the first block has been processed, the next blocks will get executed | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// optimistically, so that when the ABCI client calls `FinalizeBlock` the app | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// can have a response ready. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if c.optimisticExec.Enabled() && req.Height > int64(c.initialHeight) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
c.optimisticExec.Execute(req) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+468
to
+478
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential nil pointer dereference when checking In Apply this diff to add a nil check: if c.optimisticExec != nil && c.optimisticExec.Enabled() && req.Height > int64(c.initialHeight) {
c.optimisticExec.Execute(req)
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return &abciproto.ProcessProposalResponse{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Status: abciproto.PROCESS_PROPOSAL_STATUS_ACCEPT, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -446,6 +486,29 @@ func (c *Consensus[T]) ProcessProposal( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
func (c *Consensus[T]) FinalizeBlock( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ctx context.Context, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
req *abciproto.FinalizeBlockRequest, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) (*abciproto.FinalizeBlockResponse, error) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if c.optimisticExec.Initialized() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// check if the hash we got is the same as the one we are executing | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
aborted := c.optimisticExec.AbortIfNeeded(req.Hash) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// Wait for the OE to finish, regardless of whether it was aborted or not | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
res, err := c.optimisticExec.WaitResult() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// only return if we are not aborting | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if !aborted { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return res, err | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// if it was aborted, we need to reset the state | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
c.optimisticExec.Reset() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+490
to
+505
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential nil pointer dereference in In Apply this diff to add a nil check before calling if c.optimisticExec != nil && c.optimisticExec.Initialized() {
// Existing code...
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return c.internalFinalizeBlock(ctx, req) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't comment on the right line below, but |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
func (c *Consensus[T]) internalFinalizeBlock( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ctx context.Context, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
req *abciproto.FinalizeBlockRequest, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) (*abciproto.FinalizeBlockResponse, error) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if err := c.validateFinalizeBlockHeight(req); err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return nil, err | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,160 @@ | ||||||||||||||||||||||||||||
package oe | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
import ( | ||||||||||||||||||||||||||||
"bytes" | ||||||||||||||||||||||||||||
"context" | ||||||||||||||||||||||||||||
"encoding/hex" | ||||||||||||||||||||||||||||
"math/rand" | ||||||||||||||||||||||||||||
"sync" | ||||||||||||||||||||||||||||
"time" | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
abci "github.com/cometbft/cometbft/api/cometbft/abci/v1" | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
"cosmossdk.io/log" | ||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// FinalizeBlockFunc is the function that is called by the OE to finalize the | ||||||||||||||||||||||||||||
// block. It is the same as the one in the ABCI app. | ||||||||||||||||||||||||||||
type FinalizeBlockFunc func(context.Context, *abci.FinalizeBlockRequest) (*abci.FinalizeBlockResponse, error) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// OptimisticExecution is a struct that contains the OE context. It is used to | ||||||||||||||||||||||||||||
// run the FinalizeBlock function in a goroutine, and to abort it if needed. | ||||||||||||||||||||||||||||
type OptimisticExecution struct { | ||||||||||||||||||||||||||||
finalizeBlockFunc FinalizeBlockFunc // ABCI FinalizeBlock function with a context | ||||||||||||||||||||||||||||
logger log.Logger | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
mtx sync.Mutex | ||||||||||||||||||||||||||||
stopCh chan struct{} | ||||||||||||||||||||||||||||
request *abci.FinalizeBlockRequest | ||||||||||||||||||||||||||||
response *abci.FinalizeBlockResponse | ||||||||||||||||||||||||||||
err error | ||||||||||||||||||||||||||||
cancelFunc func() // cancel function for the context | ||||||||||||||||||||||||||||
initialized bool // A boolean value indicating whether the struct has been initialized | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// debugging/testing options | ||||||||||||||||||||||||||||
abortRate int // number from 0 to 100 that determines the percentage of OE that should be aborted | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// NewOptimisticExecution initializes the Optimistic Execution context but does not start it. | ||||||||||||||||||||||||||||
func NewOptimisticExecution(logger log.Logger, fn FinalizeBlockFunc, opts ...func(*OptimisticExecution)) *OptimisticExecution { | ||||||||||||||||||||||||||||
logger = logger.With(log.ModuleKey, "oe") | ||||||||||||||||||||||||||||
oe := &OptimisticExecution{logger: logger, finalizeBlockFunc: fn} | ||||||||||||||||||||||||||||
for _, opt := range opts { | ||||||||||||||||||||||||||||
opt(oe) | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
return oe | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// WithAbortRate sets the abort rate for the OE. The abort rate is a number from | ||||||||||||||||||||||||||||
// 0 to 100 that determines the percentage of OE that should be aborted. | ||||||||||||||||||||||||||||
// This is for testing purposes only and must not be used in production. | ||||||||||||||||||||||||||||
func WithAbortRate(rate int) func(*OptimisticExecution) { | ||||||||||||||||||||||||||||
return func(oe *OptimisticExecution) { | ||||||||||||||||||||||||||||
oe.abortRate = rate | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// Reset resets the OE context. Must be called whenever we want to invalidate | ||||||||||||||||||||||||||||
// the current OE. | ||||||||||||||||||||||||||||
func (oe *OptimisticExecution) Reset() { | ||||||||||||||||||||||||||||
oe.mtx.Lock() | ||||||||||||||||||||||||||||
defer oe.mtx.Unlock() | ||||||||||||||||||||||||||||
oe.request = nil | ||||||||||||||||||||||||||||
oe.response = nil | ||||||||||||||||||||||||||||
oe.err = nil | ||||||||||||||||||||||||||||
oe.initialized = false | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
func (oe *OptimisticExecution) Enabled() bool { | ||||||||||||||||||||||||||||
return oe != nil | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// Initialized returns true if the OE was initialized, meaning that it contains | ||||||||||||||||||||||||||||
// a request and it was run or it is running. | ||||||||||||||||||||||||||||
func (oe *OptimisticExecution) Initialized() bool { | ||||||||||||||||||||||||||||
if oe == nil { | ||||||||||||||||||||||||||||
return false | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
oe.mtx.Lock() | ||||||||||||||||||||||||||||
defer oe.mtx.Unlock() | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
return oe.initialized | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// Execute initializes the OE and starts it in a goroutine. | ||||||||||||||||||||||||||||
func (oe *OptimisticExecution) Execute(req *abci.ProcessProposalRequest) { | ||||||||||||||||||||||||||||
oe.mtx.Lock() | ||||||||||||||||||||||||||||
defer oe.mtx.Unlock() | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
oe.stopCh = make(chan struct{}) | ||||||||||||||||||||||||||||
oe.request = &abci.FinalizeBlockRequest{ | ||||||||||||||||||||||||||||
Txs: req.Txs, | ||||||||||||||||||||||||||||
DecidedLastCommit: req.ProposedLastCommit, | ||||||||||||||||||||||||||||
Misbehavior: req.Misbehavior, | ||||||||||||||||||||||||||||
Hash: req.Hash, | ||||||||||||||||||||||||||||
Height: req.Height, | ||||||||||||||||||||||||||||
Time: req.Time, | ||||||||||||||||||||||||||||
NextValidatorsHash: req.NextValidatorsHash, | ||||||||||||||||||||||||||||
ProposerAddress: req.ProposerAddress, | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
oe.logger.Debug("OE started", "height", req.Height, "hash", hex.EncodeToString(req.Hash), "time", req.Time.String()) | ||||||||||||||||||||||||||||
ctx, cancel := context.WithCancel(context.Background()) | ||||||||||||||||||||||||||||
oe.cancelFunc = cancel | ||||||||||||||||||||||||||||
oe.initialized = true | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
go func() { | ||||||||||||||||||||||||||||
start := time.Now() | ||||||||||||||||||||||||||||
Check warning Code scanning / CodeQL Calling the system time Warning
Calling the system time may be a possible source of non-determinism
|
||||||||||||||||||||||||||||
resp, err := oe.finalizeBlockFunc(ctx, oe.request) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
oe.mtx.Lock() | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
executionTime := time.Since(start) | ||||||||||||||||||||||||||||
oe.logger.Debug("OE finished", "duration", executionTime.String(), "height", oe.request.Height, "hash", hex.EncodeToString(oe.request.Hash)) | ||||||||||||||||||||||||||||
oe.response, oe.err = resp, err | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
close(oe.stopCh) | ||||||||||||||||||||||||||||
oe.mtx.Unlock() | ||||||||||||||||||||||||||||
}() | ||||||||||||||||||||||||||||
Comment on lines
+106
to
+118
Check notice Code scanning / CodeQL Spawning a Go routine Note
Spawning a Go routine may be a possible source of non-determinism
|
||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// AbortIfNeeded aborts the OE if the request hash is not the same as the one in | ||||||||||||||||||||||||||||
// the running OE. Returns true if the OE was aborted. | ||||||||||||||||||||||||||||
func (oe *OptimisticExecution) AbortIfNeeded(reqHash []byte) bool { | ||||||||||||||||||||||||||||
if oe == nil { | ||||||||||||||||||||||||||||
return false | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
oe.mtx.Lock() | ||||||||||||||||||||||||||||
defer oe.mtx.Unlock() | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
if !bytes.Equal(oe.request.Hash, reqHash) { | ||||||||||||||||||||||||||||
oe.logger.Error("OE aborted due to hash mismatch", "oe_hash", hex.EncodeToString(oe.request.Hash), "req_hash", hex.EncodeToString(reqHash), "oe_height", oe.request.Height, "req_height", oe.request.Height) | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct the logging of In the error log within Adjust the log statement to use the correct height for oe.logger.Error("OE aborted due to hash mismatch",
"oe_hash", hex.EncodeToString(oe.request.Hash),
"req_hash", hex.EncodeToString(reqHash),
"oe_height", oe.request.Height,
- "req_height", oe.request.Height)
+ "req_height", currentRequestHeight) If
|
||||||||||||||||||||||||||||
oe.cancelFunc() | ||||||||||||||||||||||||||||
return true | ||||||||||||||||||||||||||||
Comment on lines
+131
to
+134
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prevent potential nil pointer dereference when accessing In the Consider adding a nil check for + if oe.request == nil {
+ oe.logger.Error("OE aborted due to missing request")
+ oe.cancelFunc()
+ return true
+ }
if !bytes.Equal(oe.request.Hash, reqHash) {
oe.logger.Error("OE aborted due to hash mismatch", "oe_hash", hex.EncodeToString(oe.request.Hash), "req_hash", hex.EncodeToString(reqHash), "oe_height", oe.request.Height, "req_height", oe.request.Height)
oe.cancelFunc()
return true
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||
} else if oe.abortRate > 0 && rand.Intn(100) < oe.abortRate { | ||||||||||||||||||||||||||||
// this is for test purposes only, we can emulate a certain percentage of | ||||||||||||||||||||||||||||
// OE needed to be aborted. | ||||||||||||||||||||||||||||
oe.cancelFunc() | ||||||||||||||||||||||||||||
oe.logger.Error("OE aborted due to test abort rate") | ||||||||||||||||||||||||||||
return true | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
return false | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// Abort aborts the OE unconditionally and waits for it to finish. | ||||||||||||||||||||||||||||
func (oe *OptimisticExecution) Abort() { | ||||||||||||||||||||||||||||
if oe == nil || oe.cancelFunc == nil { | ||||||||||||||||||||||||||||
return | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
oe.cancelFunc() | ||||||||||||||||||||||||||||
<-oe.stopCh | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// WaitResult waits for the OE to finish and returns the result. | ||||||||||||||||||||||||||||
func (oe *OptimisticExecution) WaitResult() (*abci.FinalizeBlockResponse, error) { | ||||||||||||||||||||||||||||
<-oe.stopCh | ||||||||||||||||||||||||||||
return oe.response, oe.err | ||||||||||||||||||||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's always enable it, and possibly remove the way to disable it (cc @tac0turtle)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good to me