Problem: no abort OE in PrepareProposal (#860)

* Abort OE in PrepareProposal * Log the correct FinalizeBlock request height when OE aborts with mismatch (#54) fix OE log --------- Co-authored-by: Teddy Ding <[email protected]>
crypto-org-chain · Oct 18, 2024 · d78d66e · d78d66e
1 parent 783e224
commit d78d66e
Show file tree

Hide file tree

Showing 3 changed files with 22 additions and 7 deletions.
diff --git a/baseapp/abci.go b/baseapp/abci.go
@@ -379,6 +379,14 @@ func (app *BaseApp) PrepareProposal(req *abci.RequestPrepareProposal) (resp *abc
 		return nil, errors.New("PrepareProposal handler not 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`.
+	app.optimisticExec.Abort()
+
 	// Always reset state given that PrepareProposal can timeout and be called
 	// again in a subsequent round.
 	header := cmtproto.Header{
@@ -904,7 +912,7 @@ func (app *BaseApp) FinalizeBlock(req *abci.RequestFinalizeBlock) (res *abci.Res
 
 	if app.optimisticExec.Initialized() {
 		// check if the hash we got is the same as the one we are executing
-		aborted := app.optimisticExec.AbortIfNeeded(req.Hash)
+		aborted := app.optimisticExec.AbortIfNeeded(req)
 		// Wait for the OE to finish, regardless of whether it was aborted or not
 		res, err = app.optimisticExec.WaitResult()
 

diff --git a/baseapp/oe/optimistic_execution.go b/baseapp/oe/optimistic_execution.go
@@ -120,16 +120,16 @@ func (oe *OptimisticExecution) Execute(req *abci.RequestProcessProposal) {
 
 // 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 {
+func (oe *OptimisticExecution) AbortIfNeeded(req *abci.RequestFinalizeBlock) 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)
+	if !bytes.Equal(oe.request.Hash, req.Hash) {
+		oe.logger.Error("OE aborted due to hash mismatch", "oe_hash", hex.EncodeToString(oe.request.Hash), "req_hash", hex.EncodeToString(req.Hash), "oe_height", oe.request.Height, "req_height", req.Height)
 		oe.cancelFunc()
 		return true
 	} else if oe.abortRate > 0 && rand.Intn(100) < oe.abortRate {

diff --git a/baseapp/oe/optimistic_execution_test.go b/baseapp/oe/optimistic_execution_test.go
@@ -27,8 +27,15 @@ func TestOptimisticExecution(t *testing.T) {
 	assert.Nil(t, resp)
 	assert.EqualError(t, err, "test error")
 
-	assert.False(t, oe.AbortIfNeeded([]byte("test")))
-	assert.True(t, oe.AbortIfNeeded([]byte("wrong_hash")))
-
+	assert.False(t, oe.AbortIfNeeded(
+		&abci.RequestFinalizeBlock{
+			Hash: []byte("test"),
+		},
+	))
+	assert.True(t, oe.AbortIfNeeded(
+		&abci.RequestFinalizeBlock{
+			Hash: []byte("wrong_hash"),
+		},
+	))
 	oe.Reset()
 }