From af1c3e6fed1105e3fac363319bcb6dd747c3cc8b Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Tue, 7 Oct 2025 17:12:53 -0700 Subject: [PATCH 01/51] refactor index tx error message --- storage/locks.go | 3 +- storage/operation/transaction_results.go | 46 +++++++++++++++++-- .../transaction_result_error_messages.go | 23 ++++------ storage/transaction_result_error_messages.go | 9 ++-- 4 files changed, 59 insertions(+), 22 deletions(-) diff --git a/storage/locks.go b/storage/locks.go index 5f7f968acaf..caaa80995fa 100644 --- a/storage/locks.go +++ b/storage/locks.go @@ -28,7 +28,8 @@ const ( // LockBootstrapping protects data that is *exclusively* written during bootstrapping. LockBootstrapping = "lock_bootstrapping" // LockInsertChunkDataPack protects the insertion of chunk data packs (not yet used anywhere - LockInsertChunkDataPack = "lock_insert_chunk_data_pack" + LockInsertChunkDataPack = "lock_insert_chunk_data_pack" + LockInsertTransactionResultErrMessage = "lock_insert_transaction_result_message" ) // Locks returns a list of all named locks used by the storage layer. diff --git a/storage/operation/transaction_results.go b/storage/operation/transaction_results.go index a97197a5cde..3f6285efbc1 100644 --- a/storage/operation/transaction_results.go +++ b/storage/operation/transaction_results.go @@ -3,6 +3,7 @@ package operation import ( "fmt" + "github.com/jordanschalm/lockctx" "github.com/onflow/flow-go/model/flow" "github.com/onflow/flow-go/storage" ) @@ -102,15 +103,52 @@ func LookupLightTransactionResultsByBlockIDUsingIndex(r storage.Reader, blockID return TraverseByPrefix(r, MakePrefix(codeLightTransactionResultIndex, blockID), txErrIterFunc, storage.DefaultIteratorOptions()) } -// BatchInsertTransactionResultErrorMessage inserts a transaction result error message by block ID and transaction ID +// BatchInsertAndIndexTransactionResultErrorMessages inserts and indexes a batch of transaction result error messages +// the caller must hold [storage.LockInsertTransactionResultErrMessage] lock +// It returns storage.ErrAlreadyExists if tx result error messages for the block already exist +func BatchInsertAndIndexTransactionResultErrorMessages( + lctx lockctx.Proof, rw storage.ReaderBatchWriter, + blockID flow.Identifier, + transactionResultErrorMessages []flow.TransactionResultErrorMessage, +) error { + if !lctx.HoldsLock(storage.LockInsertTransactionResultErrMessage) { + return fmt.Errorf("lock %s is not held", storage.LockInsertTransactionResultErrMessage) + } + + // ensure we don't overwrite existing tx result error messages for this block + prefix := MakePrefix(codeTransactionResultErrorMessage, blockID) + checkExists := func(key []byte) error { + return fmt.Errorf("transaction result error messages for block %s already exist: %w", blockID, storage.ErrAlreadyExists) + } + err := IterateKeysByPrefixRange(rw.GlobalReader(), prefix, prefix, checkExists) + if err != nil { + return err + } + + w := rw.Writer() + for _, result := range transactionResultErrorMessages { + err := batchInsertTransactionResultErrorMessage(w, blockID, &result) + if err != nil { + return fmt.Errorf("cannot batch insert tx result error message: %w", err) + } + + err = batchIndexTransactionResultErrorMessage(w, blockID, &result) + if err != nil { + return fmt.Errorf("cannot batch index tx result error message: %w", err) + } + } + return nil +} + +// batchInsertTransactionResultErrorMessage inserts a transaction result error message by block ID and transaction ID // into the database using a batch write. -func BatchInsertTransactionResultErrorMessage(w storage.Writer, blockID flow.Identifier, transactionResultErrorMessage *flow.TransactionResultErrorMessage) error { +func batchInsertTransactionResultErrorMessage(w storage.Writer, blockID flow.Identifier, transactionResultErrorMessage *flow.TransactionResultErrorMessage) error { return UpsertByKey(w, MakePrefix(codeTransactionResultErrorMessage, blockID, transactionResultErrorMessage.TransactionID), transactionResultErrorMessage) } -// BatchIndexTransactionResultErrorMessage indexes a transaction result error message by index within the block using a +// batchIndexTransactionResultErrorMessage indexes a transaction result error message by index within the block using a // batch write. -func BatchIndexTransactionResultErrorMessage(w storage.Writer, blockID flow.Identifier, transactionResultErrorMessage *flow.TransactionResultErrorMessage) error { +func batchIndexTransactionResultErrorMessage(w storage.Writer, blockID flow.Identifier, transactionResultErrorMessage *flow.TransactionResultErrorMessage) error { return UpsertByKey(w, MakePrefix(codeTransactionResultErrorMessageIndex, blockID, transactionResultErrorMessage.Index), transactionResultErrorMessage) } diff --git a/storage/store/transaction_result_error_messages.go b/storage/store/transaction_result_error_messages.go index 6f315216aea..cba382be141 100644 --- a/storage/store/transaction_result_error_messages.go +++ b/storage/store/transaction_result_error_messages.go @@ -3,6 +3,7 @@ package store import ( "fmt" + "github.com/jordanschalm/lockctx" "github.com/onflow/flow-go/model/flow" "github.com/onflow/flow-go/module" "github.com/onflow/flow-go/module/metrics" @@ -99,27 +100,21 @@ func (t *TransactionResultErrorMessages) Exists(blockID flow.Identifier) (bool, } // BatchStore inserts a batch of transaction result error messages into a batch -// +// the caller must hold [storage.LockInsertTransactionResultErrMessage] lock // No errors are expected during normal operation. func (t *TransactionResultErrorMessages) BatchStore( + lctx lockctx.Proof, + rw storage.ReaderBatchWriter, blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage, - batch storage.ReaderBatchWriter, ) error { - writer := batch.Writer() - for _, result := range transactionResultErrorMessages { - err := operation.BatchInsertTransactionResultErrorMessage(writer, blockID, &result) - if err != nil { - return fmt.Errorf("cannot batch insert tx result error message: %w", err) - } - - err = operation.BatchIndexTransactionResultErrorMessage(writer, blockID, &result) - if err != nil { - return fmt.Errorf("cannot batch index tx result error message: %w", err) - } + // requires [storage.LockInsertTransactionResultErrMessage] + err := operation.BatchInsertAndIndexTransactionResultErrorMessages(lctx, rw, blockID, transactionResultErrorMessages) + if err != nil { + return err } - storage.OnCommitSucceed(batch, func() { + storage.OnCommitSucceed(rw, func() { for _, result := range transactionResultErrorMessages { key := KeyFromBlockIDTransactionID(blockID, result.TransactionID) // cache for each transaction, so that it's faster to retrieve diff --git a/storage/transaction_result_error_messages.go b/storage/transaction_result_error_messages.go index a573bbae8a2..b2fed2ba534 100644 --- a/storage/transaction_result_error_messages.go +++ b/storage/transaction_result_error_messages.go @@ -1,6 +1,9 @@ package storage -import "github.com/onflow/flow-go/model/flow" +import ( + "github.com/jordanschalm/lockctx" + "github.com/onflow/flow-go/model/flow" +) // TransactionResultErrorMessagesReader represents persistent storage read operations for transaction result error messages type TransactionResultErrorMessagesReader interface { @@ -38,7 +41,7 @@ type TransactionResultErrorMessages interface { Store(blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage) error // BatchStore inserts a batch of transaction result error messages into a batch - // + // caller must hold [storage.LockInsertTransactionResultErrMessage] lock // No errors are expected during normal operation. - BatchStore(blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage, batch ReaderBatchWriter) error + BatchStore(lctx lockctx.Proof, blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage, batch ReaderBatchWriter) error } From d86569fdd70c96d1c409efaaba9c59721c834fea Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Tue, 7 Oct 2025 17:21:08 -0700 Subject: [PATCH 02/51] refactor index tx error message --- storage/light_transaction_results.go | 9 +++- storage/locks.go | 4 ++ storage/operation/transaction_results.go | 45 ++++++++++++++++++- storage/store/light_transaction_results.go | 20 +++------ .../store/light_transaction_results_test.go | 14 ++++-- .../transaction_result_error_messages.go | 5 ++- storage/transaction_result_error_messages.go | 8 ++-- 7 files changed, 77 insertions(+), 28 deletions(-) diff --git a/storage/light_transaction_results.go b/storage/light_transaction_results.go index e2109d8e450..a34940eeda8 100644 --- a/storage/light_transaction_results.go +++ b/storage/light_transaction_results.go @@ -1,6 +1,9 @@ package storage -import "github.com/onflow/flow-go/model/flow" +import ( + "github.com/jordanschalm/lockctx" + "github.com/onflow/flow-go/model/flow" +) // LightTransactionResultsReader represents persistent storage read operations for light transaction result type LightTransactionResultsReader interface { @@ -28,7 +31,9 @@ type LightTransactionResults interface { LightTransactionResultsReader // BatchStore inserts a batch of transaction result into a batch - BatchStore(blockID flow.Identifier, transactionResults []flow.LightTransactionResult, rw ReaderBatchWriter) error + // It returns [ErrAlreadyExists] if light transaction results for the block already exist. + // It requires the caller to hold [storage.LockInsertLightTransactionResult] + BatchStore(lctx lockctx.Proof, rw ReaderBatchWriter, blockID flow.Identifier, transactionResults []flow.LightTransactionResult) error // Deprecated: deprecated as a part of transition from Badger to Pebble. use BatchStore instead BatchStoreBadger(blockID flow.Identifier, transactionResults []flow.LightTransactionResult, batch BatchStorage) error diff --git a/storage/locks.go b/storage/locks.go index caaa80995fa..84dbd558c00 100644 --- a/storage/locks.go +++ b/storage/locks.go @@ -30,6 +30,8 @@ const ( // LockInsertChunkDataPack protects the insertion of chunk data packs (not yet used anywhere LockInsertChunkDataPack = "lock_insert_chunk_data_pack" LockInsertTransactionResultErrMessage = "lock_insert_transaction_result_message" + // LockInsertLightTransactionResult protects the insertion of light transaction results + LockInsertLightTransactionResult = "lock_insert_light_transaction_result" ) // Locks returns a list of all named locks used by the storage layer. @@ -43,6 +45,8 @@ func Locks() []string { LockInsertCollection, LockBootstrapping, LockInsertChunkDataPack, + LockInsertTransactionResultErrMessage, + LockInsertLightTransactionResult, } } diff --git a/storage/operation/transaction_results.go b/storage/operation/transaction_results.go index 3f6285efbc1..cc3c46f9dd7 100644 --- a/storage/operation/transaction_results.go +++ b/storage/operation/transaction_results.go @@ -70,11 +70,52 @@ func InsertLightTransactionResult(w storage.Writer, blockID flow.Identifier, tra return UpsertByKey(w, MakePrefix(codeLightTransactionResult, blockID, transactionResult.TransactionID), transactionResult) } -func BatchInsertLightTransactionResult(w storage.Writer, blockID flow.Identifier, transactionResult *flow.LightTransactionResult) error { +// BatchInsertAndIndexLightTransactionResults inserts and indexes a batch of light transaction results +// the caller must hold [storage.LockInsertLightTransactionResult] lock +// It returns storage.ErrAlreadyExists if light transaction results for the block already exist +func BatchInsertAndIndexLightTransactionResults( + lctx lockctx.Proof, rw storage.ReaderBatchWriter, + blockID flow.Identifier, + transactionResults []flow.LightTransactionResult, +) error { + if !lctx.HoldsLock(storage.LockInsertLightTransactionResult) { + return fmt.Errorf("lock %s is not held", storage.LockInsertLightTransactionResult) + } + + // ensure we don't overwrite existing light transaction results for this block + prefix := MakePrefix(codeLightTransactionResult, blockID) + checkExists := func(key []byte) error { + return fmt.Errorf("light transaction results for block %s already exist: %w", blockID, storage.ErrAlreadyExists) + } + err := IterateKeysByPrefixRange(rw.GlobalReader(), prefix, prefix, checkExists) + if err != nil { + return err + } + + w := rw.Writer() + for i, result := range transactionResults { + err := batchInsertLightTransactionResult(w, blockID, &result) + if err != nil { + return fmt.Errorf("cannot batch insert light tx result: %w", err) + } + + err = batchIndexLightTransactionResult(w, blockID, uint32(i), &result) + if err != nil { + return fmt.Errorf("cannot batch index light tx result: %w", err) + } + } + return nil +} + +// batchInsertLightTransactionResult inserts a light transaction result by block ID and transaction ID +// into the database using a batch write. +func batchInsertLightTransactionResult(w storage.Writer, blockID flow.Identifier, transactionResult *flow.LightTransactionResult) error { return UpsertByKey(w, MakePrefix(codeLightTransactionResult, blockID, transactionResult.TransactionID), transactionResult) } -func BatchIndexLightTransactionResult(w storage.Writer, blockID flow.Identifier, txIndex uint32, transactionResult *flow.LightTransactionResult) error { +// batchIndexLightTransactionResult indexes a light transaction result by index within the block using a +// batch write. +func batchIndexLightTransactionResult(w storage.Writer, blockID flow.Identifier, txIndex uint32, transactionResult *flow.LightTransactionResult) error { return UpsertByKey(w, MakePrefix(codeLightTransactionResultIndex, blockID, txIndex), transactionResult) } diff --git a/storage/store/light_transaction_results.go b/storage/store/light_transaction_results.go index dd8cab8e29a..fc708bf1f1f 100644 --- a/storage/store/light_transaction_results.go +++ b/storage/store/light_transaction_results.go @@ -1,7 +1,7 @@ package store import ( - "fmt" + "github.com/jordanschalm/lockctx" "github.com/onflow/flow-go/model/flow" "github.com/onflow/flow-go/module" @@ -71,19 +71,11 @@ func NewLightTransactionResults(collector module.CacheMetrics, db storage.DB, tr } } -func (tr *LightTransactionResults) BatchStore(blockID flow.Identifier, transactionResults []flow.LightTransactionResult, rw storage.ReaderBatchWriter) error { - w := rw.Writer() - - for i, result := range transactionResults { - err := operation.BatchInsertLightTransactionResult(w, blockID, &result) - if err != nil { - return fmt.Errorf("cannot batch insert tx result: %w", err) - } - - err = operation.BatchIndexLightTransactionResult(w, blockID, uint32(i), &result) - if err != nil { - return fmt.Errorf("cannot batch index tx result: %w", err) - } +func (tr *LightTransactionResults) BatchStore(lctx lockctx.Proof, rw storage.ReaderBatchWriter, blockID flow.Identifier, transactionResults []flow.LightTransactionResult) error { + // requires [storage.LockInsertLightTransactionResult] + err := operation.BatchInsertAndIndexLightTransactionResults(lctx, rw, blockID, transactionResults) + if err != nil { + return err } storage.OnCommitSucceed(rw, func() { diff --git a/storage/store/light_transaction_results_test.go b/storage/store/light_transaction_results_test.go index c3ea965ab72..bdc6bfbe573 100644 --- a/storage/store/light_transaction_results_test.go +++ b/storage/store/light_transaction_results_test.go @@ -3,6 +3,7 @@ package store_test import ( "testing" + "github.com/jordanschalm/lockctx" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/exp/rand" @@ -24,13 +25,18 @@ func TestBatchStoringLightTransactionResults(t *testing.T) { txResults := getLightTransactionResultsFixture(10) t.Run("batch store1 results", func(t *testing.T) { - require.NoError(t, db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { - return store1.BatchStore(blockID, txResults, rw) + lockManager := storage.NewTestingLockManager() + require.NoError(t, unittest.WithLock(t, lockManager, storage.LockInsertLightTransactionResult, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return store1.BatchStore(lctx, rw, blockID, txResults) + }) })) // add a results to a new block to validate they are not included in lookups - require.NoError(t, db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { - return store1.BatchStore(unittest.IdentifierFixture(), getLightTransactionResultsFixture(2), rw) + require.NoError(t, unittest.WithLock(t, lockManager, storage.LockInsertLightTransactionResult, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return store1.BatchStore(lctx, rw, unittest.IdentifierFixture(), getLightTransactionResultsFixture(2)) + }) })) }) diff --git a/storage/store/transaction_result_error_messages.go b/storage/store/transaction_result_error_messages.go index cba382be141..15989918f1b 100644 --- a/storage/store/transaction_result_error_messages.go +++ b/storage/store/transaction_result_error_messages.go @@ -74,10 +74,11 @@ func NewTransactionResultErrorMessages(collector module.CacheMetrics, db storage // Store will store transaction result error messages for the given block ID. // +// the caller must hold [storage.LockInsertTransactionResultErrMessage] lock // No errors are expected during normal operation. -func (t *TransactionResultErrorMessages) Store(blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage) error { +func (t *TransactionResultErrorMessages) Store(lctx lockctx.Proof, blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage) error { return t.db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { - return t.BatchStore(blockID, transactionResultErrorMessages, rw) + return t.BatchStore(lctx, rw, blockID, transactionResultErrorMessages) }) } diff --git a/storage/transaction_result_error_messages.go b/storage/transaction_result_error_messages.go index b2fed2ba534..95e010bde6d 100644 --- a/storage/transaction_result_error_messages.go +++ b/storage/transaction_result_error_messages.go @@ -38,10 +38,10 @@ type TransactionResultErrorMessages interface { // Store will store transaction result error messages for the given block ID. // // No errors are expected during normal operation. - Store(blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage) error + Store(lctx lockctx.Proof, blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage) error // BatchStore inserts a batch of transaction result error messages into a batch - // caller must hold [storage.LockInsertTransactionResultErrMessage] lock - // No errors are expected during normal operation. - BatchStore(lctx lockctx.Proof, blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage, batch ReaderBatchWriter) error + // It returns [ErrAlreadyExists] if transaction result error messages for the block already exist. + // It requires the caller to hold [storage.LockInsertTransactionResultErrMessage] + BatchStore(lctx lockctx.Proof, rw ReaderBatchWriter, blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage) error } From af54bfe054f32865677d9a8c6beb5e4f54e729e2 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Tue, 7 Oct 2025 17:22:36 -0700 Subject: [PATCH 03/51] fix store tests --- .../inmemory/unsynchronized/light_transaction_results.go | 4 +++- .../unsynchronized/transaction_result_error_messages.go | 5 ++++- storage/store/light_transaction_results_test.go | 2 +- storage/store/transaction_result_error_messages_test.go | 6 +++++- 4 files changed, 13 insertions(+), 4 deletions(-) diff --git a/storage/store/inmemory/unsynchronized/light_transaction_results.go b/storage/store/inmemory/unsynchronized/light_transaction_results.go index 56b4901abcb..00d9c28e0b5 100644 --- a/storage/store/inmemory/unsynchronized/light_transaction_results.go +++ b/storage/store/inmemory/unsynchronized/light_transaction_results.go @@ -4,6 +4,8 @@ import ( "fmt" "sync" + "github.com/jordanschalm/lockctx" + "github.com/onflow/flow-go/model/flow" "github.com/onflow/flow-go/storage" "github.com/onflow/flow-go/storage/store" @@ -110,7 +112,7 @@ func (l *LightTransactionResults) Data() []flow.LightTransactionResult { // BatchStore inserts a batch of transaction result into a batch. // This method is not implemented and will always return an error. -func (l *LightTransactionResults) BatchStore(flow.Identifier, []flow.LightTransactionResult, storage.ReaderBatchWriter) error { +func (l *LightTransactionResults) BatchStore(lockctx.Proof, storage.ReaderBatchWriter, flow.Identifier, []flow.LightTransactionResult) error { return fmt.Errorf("not implemented") } diff --git a/storage/store/inmemory/unsynchronized/transaction_result_error_messages.go b/storage/store/inmemory/unsynchronized/transaction_result_error_messages.go index a3fae2f7b3d..2348742a88b 100644 --- a/storage/store/inmemory/unsynchronized/transaction_result_error_messages.go +++ b/storage/store/inmemory/unsynchronized/transaction_result_error_messages.go @@ -5,6 +5,8 @@ import ( "fmt" "sync" + "github.com/jordanschalm/lockctx" + "github.com/onflow/flow-go/model/flow" "github.com/onflow/flow-go/storage" "github.com/onflow/flow-go/storage/store" @@ -107,6 +109,7 @@ func (t *TransactionResultErrorMessages) ByBlockID(id flow.Identifier) ([]flow.T // // No errors are expected during normal operation. func (t *TransactionResultErrorMessages) Store( + lctx lockctx.Proof, blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage, ) error { @@ -139,6 +142,6 @@ func (t *TransactionResultErrorMessages) Data() []flow.TransactionResultErrorMes // BatchStore inserts a batch of transaction result error messages into a batch // This method is not implemented and will always return an error. -func (t *TransactionResultErrorMessages) BatchStore(blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage, batch storage.ReaderBatchWriter) error { +func (t *TransactionResultErrorMessages) BatchStore(lctx lockctx.Proof, rw storage.ReaderBatchWriter, blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage) error { return fmt.Errorf("not implemented") } diff --git a/storage/store/light_transaction_results_test.go b/storage/store/light_transaction_results_test.go index bdc6bfbe573..fe48dfdbc60 100644 --- a/storage/store/light_transaction_results_test.go +++ b/storage/store/light_transaction_results_test.go @@ -18,6 +18,7 @@ import ( func TestBatchStoringLightTransactionResults(t *testing.T) { dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { + lockManager := storage.NewTestingLockManager() metrics := metrics.NewNoopCollector() store1 := store.NewLightTransactionResults(metrics, db, 1000) @@ -25,7 +26,6 @@ func TestBatchStoringLightTransactionResults(t *testing.T) { txResults := getLightTransactionResultsFixture(10) t.Run("batch store1 results", func(t *testing.T) { - lockManager := storage.NewTestingLockManager() require.NoError(t, unittest.WithLock(t, lockManager, storage.LockInsertLightTransactionResult, func(lctx lockctx.Context) error { return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { return store1.BatchStore(lctx, rw, blockID, txResults) diff --git a/storage/store/transaction_result_error_messages_test.go b/storage/store/transaction_result_error_messages_test.go index 02238f0138c..2bc5eb7b844 100644 --- a/storage/store/transaction_result_error_messages_test.go +++ b/storage/store/transaction_result_error_messages_test.go @@ -4,6 +4,7 @@ import ( "fmt" "testing" + "github.com/jordanschalm/lockctx" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/exp/rand" @@ -19,6 +20,7 @@ import ( func TestStoringTransactionResultErrorMessages(t *testing.T) { dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { + lockManager := storage.NewTestingLockManager() metrics := metrics.NewNoopCollector() store1 := store.NewTransactionResultErrorMessages(metrics, db, 1000) @@ -44,7 +46,9 @@ func TestStoringTransactionResultErrorMessages(t *testing.T) { } txErrorMessages = append(txErrorMessages, expected) } - err = store1.Store(blockID, txErrorMessages) + err = unittest.WithLock(t, lockManager, storage.LockInsertTransactionResultErrMessage, func(lctx lockctx.Context) error { + return store1.Store(lctx, blockID, txErrorMessages) + }) require.NoError(t, err) // test db Exists by block id From b41320319b716b459945f995706a5560174ea6c7 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Tue, 7 Oct 2025 18:30:09 -0700 Subject: [PATCH 04/51] refactor tx error message --- .../backfill_tx_error_messages_test.go | 2 ++ .../node_builder/access_node_builder.go | 1 + .../tx_error_messages_core.go | 9 +++++++- .../tx_error_messages_core_test.go | 20 ++++++++++------ .../tx_error_messages_engine_test.go | 5 ++++ .../indexer/in_memory_indexer.go | 6 ++++- .../indexer/indexer_core.go | 23 +++++++++++-------- storage/mock/light_transaction_results.go | 12 ++++++---- .../mock/transaction_result_error_messages.go | 22 ++++++++++-------- storage/transaction_result_error_messages.go | 6 ++--- 10 files changed, 69 insertions(+), 37 deletions(-) diff --git a/admin/commands/storage/backfill_tx_error_messages_test.go b/admin/commands/storage/backfill_tx_error_messages_test.go index 4fa11fecb1e..bf06ed29940 100644 --- a/admin/commands/storage/backfill_tx_error_messages_test.go +++ b/admin/commands/storage/backfill_tx_error_messages_test.go @@ -77,6 +77,7 @@ func TestBackfillTxErrorMessages(t *testing.T) { func (suite *BackfillTxErrorMessagesSuite) SetupTest() { suite.log = zerolog.New(os.Stderr) + lockManager := storage.NewTestingLockManager() suite.state = new(protocolmock.State) suite.headers = new(storagemock.Headers) suite.receipts = new(storagemock.ExecutionReceipts) @@ -160,6 +161,7 @@ func (suite *BackfillTxErrorMessagesSuite) SetupTest() { errorMessageProvider, suite.txErrorMessages, executionNodeIdentitiesProvider, + lockManager, ) suite.command = NewBackfillTxErrorMessagesCommand( diff --git a/cmd/access/node_builder/access_node_builder.go b/cmd/access/node_builder/access_node_builder.go index 81d618a6c33..a50bc9e7a7b 100644 --- a/cmd/access/node_builder/access_node_builder.go +++ b/cmd/access/node_builder/access_node_builder.go @@ -2215,6 +2215,7 @@ func (builder *FlowAccessNodeBuilder) Build() (cmd.Node, error) { notNil(builder.txResultErrorMessageProvider), builder.transactionResultErrorMessages, notNil(builder.ExecNodeIdentitiesProvider), + node.StorageLockMgr, ) } diff --git a/engine/access/ingestion/tx_error_messages/tx_error_messages_core.go b/engine/access/ingestion/tx_error_messages/tx_error_messages_core.go index 1ce681e051c..6b563cdfa96 100644 --- a/engine/access/ingestion/tx_error_messages/tx_error_messages_core.go +++ b/engine/access/ingestion/tx_error_messages/tx_error_messages_core.go @@ -4,6 +4,7 @@ import ( "context" "fmt" + "github.com/jordanschalm/lockctx" "github.com/rs/zerolog" "github.com/onflow/flow-go/engine/access/rpc/backend/transactions/error_messages" @@ -24,6 +25,7 @@ type TxErrorMessagesCore struct { txErrorMessageProvider error_messages.Provider transactionResultErrorMessages storage.TransactionResultErrorMessages execNodeIdentitiesProvider *commonrpc.ExecutionNodeIdentitiesProvider + lockManager storage.LockManager } // NewTxErrorMessagesCore creates a new instance of TxErrorMessagesCore. @@ -32,12 +34,14 @@ func NewTxErrorMessagesCore( txErrorMessageProvider error_messages.Provider, transactionResultErrorMessages storage.TransactionResultErrorMessages, execNodeIdentitiesProvider *commonrpc.ExecutionNodeIdentitiesProvider, + lockManager storage.LockManager, ) *TxErrorMessagesCore { return &TxErrorMessagesCore{ log: log.With().Str("module", "tx_error_messages_core").Logger(), txErrorMessageProvider: txErrorMessageProvider, transactionResultErrorMessages: transactionResultErrorMessages, execNodeIdentitiesProvider: execNodeIdentitiesProvider, + lockManager: lockManager, } } @@ -124,7 +128,10 @@ func (c *TxErrorMessagesCore) storeTransactionResultErrorMessages( errorMessages = append(errorMessages, errorMessage) } - err := c.transactionResultErrorMessages.Store(blockID, errorMessages) + err := storage.WithLock(c.lockManager, storage.LockInsertTransactionResultErrMessage, func(lctx lockctx.Context) error { + // requires the [storage.LockInsertTransactionResultErrMessage] lock + return c.transactionResultErrorMessages.Store(lctx, blockID, errorMessages) + }) if err != nil { return fmt.Errorf("failed to store transaction error messages: %w", err) } diff --git a/engine/access/ingestion/tx_error_messages/tx_error_messages_core_test.go b/engine/access/ingestion/tx_error_messages/tx_error_messages_core_test.go index 04e0e8ac426..8b5d504fdfc 100644 --- a/engine/access/ingestion/tx_error_messages/tx_error_messages_core_test.go +++ b/engine/access/ingestion/tx_error_messages/tx_error_messages_core_test.go @@ -21,7 +21,8 @@ import ( "github.com/onflow/flow-go/module/irrecoverable" syncmock "github.com/onflow/flow-go/module/state_synchronization/mock" protocol "github.com/onflow/flow-go/state/protocol/mock" - storage "github.com/onflow/flow-go/storage/mock" + "github.com/onflow/flow-go/storage" + storagemock "github.com/onflow/flow-go/storage/mock" "github.com/onflow/flow-go/utils/unittest" ) @@ -37,13 +38,14 @@ type TxErrorMessagesCoreSuite struct { params *protocol.Params } - receipts *storage.ExecutionReceipts - txErrorMessages *storage.TransactionResultErrorMessages - lightTxResults *storage.LightTransactionResults + receipts *storagemock.ExecutionReceipts + txErrorMessages *storagemock.TransactionResultErrorMessages + lightTxResults *storagemock.LightTransactionResults reporter *syncmock.IndexReporter indexReporter *index.Reporter txResultsIndex *index.TransactionResultsIndex + lockManager storage.LockManager enNodeIDs flow.IdentityList execClient *accessmock.ExecutionAPIClient @@ -79,17 +81,20 @@ func (s *TxErrorMessagesCoreSuite) SetupTest() { s.proto.params = protocol.NewParams(s.T()) s.execClient = accessmock.NewExecutionAPIClient(s.T()) s.connFactory = connectionmock.NewConnectionFactory(s.T()) - s.receipts = storage.NewExecutionReceipts(s.T()) - s.txErrorMessages = storage.NewTransactionResultErrorMessages(s.T()) + s.receipts = storagemock.NewExecutionReceipts(s.T()) + s.txErrorMessages = storagemock.NewTransactionResultErrorMessages(s.T()) s.rootBlock = unittest.Block.Genesis(flow.Emulator) s.finalizedBlock = unittest.BlockWithParentFixture(s.rootBlock.ToHeader()).ToHeader() - s.lightTxResults = storage.NewLightTransactionResults(s.T()) + s.lightTxResults = storagemock.NewLightTransactionResults(s.T()) s.reporter = syncmock.NewIndexReporter(s.T()) s.indexReporter = index.NewReporter() err := s.indexReporter.Initialize(s.reporter) s.Require().NoError(err) s.txResultsIndex = index.NewTransactionResultsIndex(s.indexReporter, s.lightTxResults) + + // Initialize lock manager for tests + s.lockManager = storage.NewTestingLockManager() s.proto.state.On("Params").Return(s.proto.params) @@ -268,6 +273,7 @@ func (s *TxErrorMessagesCoreSuite) initCore() *TxErrorMessagesCore { errorMessageProvider, s.txErrorMessages, execNodeIdentitiesProvider, + s.lockManager, ) return core } diff --git a/engine/access/ingestion/tx_error_messages/tx_error_messages_engine_test.go b/engine/access/ingestion/tx_error_messages/tx_error_messages_engine_test.go index 7acc1f4ad01..11dbac37123 100644 --- a/engine/access/ingestion/tx_error_messages/tx_error_messages_engine_test.go +++ b/engine/access/ingestion/tx_error_messages/tx_error_messages_engine_test.go @@ -55,6 +55,7 @@ type TxErrorMessagesEngineSuite struct { reporter *syncmock.IndexReporter indexReporter *index.Reporter txResultsIndex *index.TransactionResultsIndex + lockManager storage.LockManager enNodeIDs flow.IdentityList execClient *accessmock.ExecutionAPIClient @@ -105,6 +106,9 @@ func (s *TxErrorMessagesEngineSuite) SetupTest() { err := s.indexReporter.Initialize(s.reporter) s.Require().NoError(err) s.txResultsIndex = index.NewTransactionResultsIndex(s.indexReporter, s.lightTxResults) + + // Initialize lock manager for tests + s.lockManager = storage.NewTestingLockManager() blockCount := 5 s.blockMap = make(map[uint64]*flow.Block, blockCount) @@ -177,6 +181,7 @@ func (s *TxErrorMessagesEngineSuite) initEngine(ctx irrecoverable.SignalerContex errorMessageProvider, s.txErrorMessages, execNodeIdentitiesProvider, + s.lockManager, ) eng, err := New( diff --git a/module/state_synchronization/indexer/in_memory_indexer.go b/module/state_synchronization/indexer/in_memory_indexer.go index 7af0586cfdc..b31607f3d24 100644 --- a/module/state_synchronization/indexer/in_memory_indexer.go +++ b/module/state_synchronization/indexer/in_memory_indexer.go @@ -68,7 +68,11 @@ func NewInMemoryIndexer( // IndexTxResultErrorMessagesData index transaction result error messages // No errors are expected during normal operation. func (i *InMemoryIndexer) IndexTxResultErrorMessagesData(txResultErrMsgs []flow.TransactionResultErrorMessage) error { - if err := i.txResultErrMsgs.Store(i.executionResult.BlockID, txResultErrMsgs); err != nil { + err := storage.WithLock(i.lockManager, storage.LockInsertTransactionResultErrMessage, func(lctx lockctx.Context) error { + // requires the [storage.LockInsertTransactionResultErrMessage] lock to be held + return i.txResultErrMsgs.Store(lctx, i.executionResult.BlockID, txResultErrMsgs) + }) + if err != nil { return fmt.Errorf("could not index transaction result error messages: %w", err) } return nil diff --git a/module/state_synchronization/indexer/indexer_core.go b/module/state_synchronization/indexer/indexer_core.go index ef768840a7a..04c8119d49f 100644 --- a/module/state_synchronization/indexer/indexer_core.go +++ b/module/state_synchronization/indexer/indexer_core.go @@ -148,17 +148,20 @@ func (c *IndexerCore) IndexBlockData(data *execution_data.BlockExecutionDataEnti results = append(results, chunk.TransactionResults...) } - err := c.protocolDB.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { - err := c.events.BatchStore(data.BlockID, []flow.EventsList{events}, rw) - if err != nil { - return fmt.Errorf("could not index events at height %d: %w", header.Height, err) - } + err = storage.WithLock(c.lockManager, storage.LockInsertLightTransactionResult, func(lctx lockctx.Context) error { + return c.protocolDB.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + err := c.events.BatchStore(data.BlockID, []flow.EventsList{events}, rw) + if err != nil { + return fmt.Errorf("could not index events at height %d: %w", header.Height, err) + } - err = c.results.BatchStore(data.BlockID, results, rw) - if err != nil { - return fmt.Errorf("could not index transaction results at height %d: %w", header.Height, err) - } - return nil + // requires the [storage.LockInsertLightTransactionResult] lock + err = c.results.BatchStore(lctx, rw, data.BlockID, results) + if err != nil { + return fmt.Errorf("could not index transaction results at height %d: %w", header.Height, err) + } + return nil + }) }) if err != nil { diff --git a/storage/mock/light_transaction_results.go b/storage/mock/light_transaction_results.go index 6e6b277acb8..ddc9c9dfcf3 100644 --- a/storage/mock/light_transaction_results.go +++ b/storage/mock/light_transaction_results.go @@ -3,7 +3,9 @@ package mock import ( + lockctx "github.com/jordanschalm/lockctx" flow "github.com/onflow/flow-go/model/flow" + mock "github.com/stretchr/testify/mock" storage "github.com/onflow/flow-go/storage" @@ -14,17 +16,17 @@ type LightTransactionResults struct { mock.Mock } -// BatchStore provides a mock function with given fields: blockID, transactionResults, rw -func (_m *LightTransactionResults) BatchStore(blockID flow.Identifier, transactionResults []flow.LightTransactionResult, rw storage.ReaderBatchWriter) error { - ret := _m.Called(blockID, transactionResults, rw) +// BatchStore provides a mock function with given fields: lctx, rw, blockID, transactionResults +func (_m *LightTransactionResults) BatchStore(lctx lockctx.Proof, rw storage.ReaderBatchWriter, blockID flow.Identifier, transactionResults []flow.LightTransactionResult) error { + ret := _m.Called(lctx, rw, blockID, transactionResults) if len(ret) == 0 { panic("no return value specified for BatchStore") } var r0 error - if rf, ok := ret.Get(0).(func(flow.Identifier, []flow.LightTransactionResult, storage.ReaderBatchWriter) error); ok { - r0 = rf(blockID, transactionResults, rw) + if rf, ok := ret.Get(0).(func(lockctx.Proof, storage.ReaderBatchWriter, flow.Identifier, []flow.LightTransactionResult) error); ok { + r0 = rf(lctx, rw, blockID, transactionResults) } else { r0 = ret.Error(0) } diff --git a/storage/mock/transaction_result_error_messages.go b/storage/mock/transaction_result_error_messages.go index 3c1075fe3ac..16a940510ef 100644 --- a/storage/mock/transaction_result_error_messages.go +++ b/storage/mock/transaction_result_error_messages.go @@ -3,7 +3,9 @@ package mock import ( + lockctx "github.com/jordanschalm/lockctx" flow "github.com/onflow/flow-go/model/flow" + mock "github.com/stretchr/testify/mock" storage "github.com/onflow/flow-go/storage" @@ -14,17 +16,17 @@ type TransactionResultErrorMessages struct { mock.Mock } -// BatchStore provides a mock function with given fields: blockID, transactionResultErrorMessages, batch -func (_m *TransactionResultErrorMessages) BatchStore(blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage, batch storage.ReaderBatchWriter) error { - ret := _m.Called(blockID, transactionResultErrorMessages, batch) +// BatchStore provides a mock function with given fields: lctx, rw, blockID, transactionResultErrorMessages +func (_m *TransactionResultErrorMessages) BatchStore(lctx lockctx.Proof, rw storage.ReaderBatchWriter, blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage) error { + ret := _m.Called(lctx, rw, blockID, transactionResultErrorMessages) if len(ret) == 0 { panic("no return value specified for BatchStore") } var r0 error - if rf, ok := ret.Get(0).(func(flow.Identifier, []flow.TransactionResultErrorMessage, storage.ReaderBatchWriter) error); ok { - r0 = rf(blockID, transactionResultErrorMessages, batch) + if rf, ok := ret.Get(0).(func(lockctx.Proof, storage.ReaderBatchWriter, flow.Identifier, []flow.TransactionResultErrorMessage) error); ok { + r0 = rf(lctx, rw, blockID, transactionResultErrorMessages) } else { r0 = ret.Error(0) } @@ -150,17 +152,17 @@ func (_m *TransactionResultErrorMessages) Exists(blockID flow.Identifier) (bool, return r0, r1 } -// Store provides a mock function with given fields: blockID, transactionResultErrorMessages -func (_m *TransactionResultErrorMessages) Store(blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage) error { - ret := _m.Called(blockID, transactionResultErrorMessages) +// Store provides a mock function with given fields: lctx, blockID, transactionResultErrorMessages +func (_m *TransactionResultErrorMessages) Store(lctx lockctx.Proof, blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage) error { + ret := _m.Called(lctx, blockID, transactionResultErrorMessages) if len(ret) == 0 { panic("no return value specified for Store") } var r0 error - if rf, ok := ret.Get(0).(func(flow.Identifier, []flow.TransactionResultErrorMessage) error); ok { - r0 = rf(blockID, transactionResultErrorMessages) + if rf, ok := ret.Get(0).(func(lockctx.Proof, flow.Identifier, []flow.TransactionResultErrorMessage) error); ok { + r0 = rf(lctx, blockID, transactionResultErrorMessages) } else { r0 = ret.Error(0) } diff --git a/storage/transaction_result_error_messages.go b/storage/transaction_result_error_messages.go index 95e010bde6d..768ebd8d89e 100644 --- a/storage/transaction_result_error_messages.go +++ b/storage/transaction_result_error_messages.go @@ -36,12 +36,12 @@ type TransactionResultErrorMessages interface { TransactionResultErrorMessagesReader // Store will store transaction result error messages for the given block ID. - // - // No errors are expected during normal operation. + // It requires the caller to hold [storage.LockInsertTransactionResultErrMessage] + // It returns [ErrAlreadyExists] if transaction result error messages for the block already exist. Store(lctx lockctx.Proof, blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage) error // BatchStore inserts a batch of transaction result error messages into a batch - // It returns [ErrAlreadyExists] if transaction result error messages for the block already exist. // It requires the caller to hold [storage.LockInsertTransactionResultErrMessage] + // It returns [ErrAlreadyExists] if transaction result error messages for the block already exist. BatchStore(lctx lockctx.Proof, rw ReaderBatchWriter, blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage) error } From a1a371d3be57979abb8c007a633a795bf5303b7d Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Tue, 7 Oct 2025 18:32:02 -0700 Subject: [PATCH 05/51] add test case --- .../transaction_result_error_messages_test.go | 121 ++++++++++++++++++ 1 file changed, 121 insertions(+) diff --git a/storage/store/transaction_result_error_messages_test.go b/storage/store/transaction_result_error_messages_test.go index 2bc5eb7b844..9f9b6b44a2c 100644 --- a/storage/store/transaction_result_error_messages_test.go +++ b/storage/store/transaction_result_error_messages_test.go @@ -112,3 +112,124 @@ func TestReadingNotStoreTransactionResultErrorMessage(t *testing.T) { assert.ErrorIs(t, err, storage.ErrNotFound) }) } + +func TestBatchStoreTransactionResultErrorMessagesErrAlreadyExists(t *testing.T) { + lockManager := storage.NewTestingLockManager() + dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { + metrics := metrics.NewNoopCollector() + st := store.NewTransactionResultErrorMessages(metrics, db, 1000) + + blockID := unittest.IdentifierFixture() + txResultErrMsgs := make([]flow.TransactionResultErrorMessage, 0) + for i := 0; i < 3; i++ { + expected := flow.TransactionResultErrorMessage{ + TransactionID: unittest.IdentifierFixture(), + ErrorMessage: fmt.Sprintf("a runtime error %d", i), + Index: uint32(i), + ExecutorID: unittest.IdentifierFixture(), + } + txResultErrMsgs = append(txResultErrMsgs, expected) + } + + // First batch store should succeed + err := unittest.WithLock(t, lockManager, storage.LockInsertTransactionResultErrMessage, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return st.BatchStore(lctx, rw, blockID, txResultErrMsgs) + }) + }) + require.NoError(t, err) + + // Second batch store with the same blockID should fail with ErrAlreadyExists + duplicateTxResultErrMsgs := make([]flow.TransactionResultErrorMessage, 0) + for i := 0; i < 2; i++ { + expected := flow.TransactionResultErrorMessage{ + TransactionID: unittest.IdentifierFixture(), + ErrorMessage: fmt.Sprintf("duplicate error %d", i), + Index: uint32(i), + ExecutorID: unittest.IdentifierFixture(), + } + duplicateTxResultErrMsgs = append(duplicateTxResultErrMsgs, expected) + } + + err = unittest.WithLock(t, lockManager, storage.LockInsertTransactionResultErrMessage, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return st.BatchStore(lctx, rw, blockID, duplicateTxResultErrMsgs) + }) + }) + require.Error(t, err) + require.ErrorIs(t, err, storage.ErrAlreadyExists) + + // Verify that the original transaction result error messages are still there and unchanged + for _, txResultErrMsg := range txResultErrMsgs { + actual, err := st.ByBlockIDTransactionID(blockID, txResultErrMsg.TransactionID) + require.NoError(t, err) + assert.Equal(t, txResultErrMsg, *actual) + } + + // Verify that the duplicate transaction result error messages were not stored + for _, txResultErrMsg := range duplicateTxResultErrMsgs { + _, err := st.ByBlockIDTransactionID(blockID, txResultErrMsg.TransactionID) + require.Error(t, err) + require.ErrorIs(t, err, storage.ErrNotFound) + } + }) +} + +func TestBatchStoreTransactionResultErrorMessagesMissingLock(t *testing.T) { + dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { + metrics := metrics.NewNoopCollector() + st := store.NewTransactionResultErrorMessages(metrics, db, 1000) + + blockID := unittest.IdentifierFixture() + txResultErrMsgs := make([]flow.TransactionResultErrorMessage, 0) + for i := 0; i < 3; i++ { + expected := flow.TransactionResultErrorMessage{ + TransactionID: unittest.IdentifierFixture(), + ErrorMessage: fmt.Sprintf("a runtime error %d", i), + Index: uint32(i), + ExecutorID: unittest.IdentifierFixture(), + } + txResultErrMsgs = append(txResultErrMsgs, expected) + } + + // Create a context without the required lock + lockManager := storage.NewTestingLockManager() + lctx := lockManager.NewContext() + defer lctx.Release() + + err := db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return st.BatchStore(lctx, rw, blockID, txResultErrMsgs) + }) + require.Error(t, err) + require.Contains(t, err.Error(), "lock_insert_transaction_result_message") + }) +} + +func TestBatchStoreTransactionResultErrorMessagesWrongLock(t *testing.T) { + lockManager := storage.NewTestingLockManager() + dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { + metrics := metrics.NewNoopCollector() + st := store.NewTransactionResultErrorMessages(metrics, db, 1000) + + blockID := unittest.IdentifierFixture() + txResultErrMsgs := make([]flow.TransactionResultErrorMessage, 0) + for i := 0; i < 3; i++ { + expected := flow.TransactionResultErrorMessage{ + TransactionID: unittest.IdentifierFixture(), + ErrorMessage: fmt.Sprintf("a runtime error %d", i), + Index: uint32(i), + ExecutorID: unittest.IdentifierFixture(), + } + txResultErrMsgs = append(txResultErrMsgs, expected) + } + + // Try to use the wrong lock + err := unittest.WithLock(t, lockManager, storage.LockInsertBlock, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return st.BatchStore(lctx, rw, blockID, txResultErrMsgs) + }) + }) + require.Error(t, err) + require.Contains(t, err.Error(), "lock_insert_transaction_result_message") + }) +} From 157d4ec1bd8241c34c81b63357ca74a9409fb7cc Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Tue, 7 Oct 2025 18:44:21 -0700 Subject: [PATCH 06/51] fix tests --- .../optimistic_sync/persisters/block_test.go | 20 ++++++++++++------- .../persisters/stores/results.go | 9 ++++++++- .../transaction_result_error_messages.go | 9 ++++++++- .../transaction_result_error_messages_test.go | 17 ++++++++++------ 4 files changed, 40 insertions(+), 15 deletions(-) diff --git a/module/executiondatasync/optimistic_sync/persisters/block_test.go b/module/executiondatasync/optimistic_sync/persisters/block_test.go index 41ec8762fa3..bf6ab0b6054 100644 --- a/module/executiondatasync/optimistic_sync/persisters/block_test.go +++ b/module/executiondatasync/optimistic_sync/persisters/block_test.go @@ -3,6 +3,7 @@ package persisters import ( "testing" + "github.com/jordanschalm/lockctx" "github.com/rs/zerolog" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -25,6 +26,7 @@ type PersisterSuite struct { inMemoryTransactions *unsynchronized.Transactions inMemoryResults *unsynchronized.LightTransactionResults inMemoryTxResultErrMsg *unsynchronized.TransactionResultErrorMessages + lockManager storage.LockManager registers *storagemock.RegisterIndex events *storagemock.Events collections *storagemock.Collections @@ -43,7 +45,7 @@ func TestPersisterSuite(t *testing.T) { } func (p *PersisterSuite) SetupTest() { - lockManager := storage.NewTestingLockManager() + p.lockManager = storage.NewTestingLockManager() t := p.T() block := unittest.BlockFixture() @@ -75,15 +77,15 @@ func (p *PersisterSuite) SetupTest() { p.persister = NewBlockPersister( zerolog.Nop(), p.database, - lockManager, + p.lockManager, p.executionResult, p.header, []stores.PersisterStore{ stores.NewEventsStore(p.inMemoryEvents, p.events, p.executionResult.BlockID), - stores.NewResultsStore(p.inMemoryResults, p.results, p.executionResult.BlockID), - stores.NewCollectionsStore(p.inMemoryCollections, p.collections, lockManager), + stores.NewResultsStore(p.inMemoryResults, p.results, p.executionResult.BlockID, p.lockManager), + stores.NewCollectionsStore(p.inMemoryCollections, p.collections, p.lockManager), stores.NewTransactionsStore(p.inMemoryTransactions, p.transactions), - stores.NewTxResultErrMsgStore(p.inMemoryTxResultErrMsg, p.txResultErrMsg, p.executionResult.BlockID), + stores.NewTxResultErrMsgStore(p.inMemoryTxResultErrMsg, p.txResultErrMsg, p.executionResult.BlockID, p.lockManager), stores.NewLatestSealedResultStore(p.latestPersistedSealedResult, p.executionResult.ID(), p.header.Height), }, ) @@ -126,7 +128,9 @@ func (p *PersisterSuite) populateInMemoryStorages() { ExecutorID: executorID, } } - err = p.inMemoryTxResultErrMsg.Store(p.executionResult.BlockID, txResultErrMsgs) + err = storage.WithLock(p.lockManager, storage.LockInsertTransactionResultErrMessage, func(lctx lockctx.Context) error { + return p.inMemoryTxResultErrMsg.Store(lctx, p.executionResult.BlockID, txResultErrMsgs) + }) p.Require().NoError(err) } @@ -139,7 +143,9 @@ func (p *PersisterSuite) TestPersister_PersistWithEmptyData() { err = p.inMemoryResults.Store(p.executionResult.BlockID, []flow.LightTransactionResult{}) p.Require().NoError(err) - err = p.inMemoryTxResultErrMsg.Store(p.executionResult.BlockID, []flow.TransactionResultErrorMessage{}) + err = storage.WithLock(p.lockManager, storage.LockInsertTransactionResultErrMessage, func(lctx lockctx.Context) error { + return p.inMemoryTxResultErrMsg.Store(lctx, p.executionResult.BlockID, []flow.TransactionResultErrorMessage{}) + }) p.Require().NoError(err) p.latestPersistedSealedResult.On("BatchSet", p.executionResult.ID(), p.header.Height, mock.Anything).Return(nil).Once() diff --git a/module/executiondatasync/optimistic_sync/persisters/stores/results.go b/module/executiondatasync/optimistic_sync/persisters/stores/results.go index bcc1116462f..2fd73f226ef 100644 --- a/module/executiondatasync/optimistic_sync/persisters/stores/results.go +++ b/module/executiondatasync/optimistic_sync/persisters/stores/results.go @@ -17,17 +17,20 @@ type ResultsStore struct { inMemoryResults *unsynchronized.LightTransactionResults persistedResults storage.LightTransactionResults blockID flow.Identifier + lockManager storage.LockManager } func NewResultsStore( inMemoryResults *unsynchronized.LightTransactionResults, persistedResults storage.LightTransactionResults, blockID flow.Identifier, + lockManager storage.LockManager, ) *ResultsStore { return &ResultsStore{ inMemoryResults: inMemoryResults, persistedResults: persistedResults, blockID: blockID, + lockManager: lockManager, } } @@ -40,7 +43,11 @@ func (r *ResultsStore) Persist(lctx lockctx.Proof, batch storage.ReaderBatchWrit } if len(results) > 0 { - if err := r.persistedResults.BatchStore(r.blockID, results, batch); err != nil { + // Use storage.WithLock to acquire the necessary lock and store the results + err := storage.WithLock(r.lockManager, storage.LockInsertLightTransactionResult, func(lctx lockctx.Context) error { + return r.persistedResults.BatchStore(lctx, batch, r.blockID, results) + }) + if err != nil { return fmt.Errorf("could not add transaction results to batch: %w", err) } } diff --git a/module/executiondatasync/optimistic_sync/persisters/stores/transaction_result_error_messages.go b/module/executiondatasync/optimistic_sync/persisters/stores/transaction_result_error_messages.go index a589c772592..e8280c33e4c 100644 --- a/module/executiondatasync/optimistic_sync/persisters/stores/transaction_result_error_messages.go +++ b/module/executiondatasync/optimistic_sync/persisters/stores/transaction_result_error_messages.go @@ -17,17 +17,20 @@ type TxResultErrMsgStore struct { inMemoryTxResultErrMsg *unsynchronized.TransactionResultErrorMessages persistedTxResultErrMsg storage.TransactionResultErrorMessages blockID flow.Identifier + lockManager storage.LockManager } func NewTxResultErrMsgStore( inMemoryTxResultErrMsg *unsynchronized.TransactionResultErrorMessages, persistedTxResultErrMsg storage.TransactionResultErrorMessages, blockID flow.Identifier, + lockManager storage.LockManager, ) *TxResultErrMsgStore { return &TxResultErrMsgStore{ inMemoryTxResultErrMsg: inMemoryTxResultErrMsg, persistedTxResultErrMsg: persistedTxResultErrMsg, blockID: blockID, + lockManager: lockManager, } } @@ -40,7 +43,11 @@ func (t *TxResultErrMsgStore) Persist(lctx lockctx.Proof, batch storage.ReaderBa } if len(txResultErrMsgs) > 0 { - if err := t.persistedTxResultErrMsg.BatchStore(t.blockID, txResultErrMsgs, batch); err != nil { + // Use storage.WithLock to acquire the necessary lock and store the error messages + err := storage.WithLock(t.lockManager, storage.LockInsertTransactionResultErrMessage, func(lctx lockctx.Context) error { + return t.persistedTxResultErrMsg.BatchStore(lctx, batch, t.blockID, txResultErrMsgs) + }) + if err != nil { return fmt.Errorf("could not add transaction result error messages to batch: %w", err) } } diff --git a/storage/store/inmemory/unsynchronized/transaction_result_error_messages_test.go b/storage/store/inmemory/unsynchronized/transaction_result_error_messages_test.go index b06b8f5051c..58ea11c3993 100644 --- a/storage/store/inmemory/unsynchronized/transaction_result_error_messages_test.go +++ b/storage/store/inmemory/unsynchronized/transaction_result_error_messages_test.go @@ -7,11 +7,12 @@ import ( "github.com/stretchr/testify/require" "github.com/onflow/flow-go/model/flow" + "github.com/onflow/flow-go/storage" "github.com/onflow/flow-go/utils/unittest" ) func TestLightTransactionResultErrorMessages_HappyPath(t *testing.T) { - storage := NewTransactionResultErrorMessages() + store := NewTransactionResultErrorMessages() // Define block ID and error messages block := unittest.BlockFixture() @@ -32,27 +33,31 @@ func TestLightTransactionResultErrorMessages_HappyPath(t *testing.T) { } // Store error messages - err := storage.Store(block.ID(), errorMessages) + lockManager := storage.NewTestingLockManager() + lctx := lockManager.NewContext() + defer lctx.Release() + + err := store.Store(lctx, block.ID(), errorMessages) require.NoError(t, err) // Retrieve by BlockID and TransactionID - retrievedErrorMessage, err := storage.ByBlockIDTransactionID(block.ID(), errorMessages[0].TransactionID) + retrievedErrorMessage, err := store.ByBlockIDTransactionID(block.ID(), errorMessages[0].TransactionID) require.NoError(t, err) assert.Equal(t, &errorMessages[0], retrievedErrorMessage) // Retrieve by BlockID and Index - retrievedErrorMessageByIndex, err := storage.ByBlockIDTransactionIndex(block.ID(), 0) + retrievedErrorMessageByIndex, err := store.ByBlockIDTransactionIndex(block.ID(), 0) require.NoError(t, err) assert.Equal(t, &errorMessages[0], retrievedErrorMessageByIndex) // Retrieve by BlockID - retrievedErrorMessages, err := storage.ByBlockID(block.ID()) + retrievedErrorMessages, err := store.ByBlockID(block.ID()) require.NoError(t, err) assert.Len(t, retrievedErrorMessages, len(errorMessages)) assert.Equal(t, errorMessages, retrievedErrorMessages) // Extract structured data - messages := storage.Data() + messages := store.Data() require.Len(t, messages, len(errorMessages)) require.ElementsMatch(t, messages, errorMessages) } From 43ef330070b2009848a63affe7c59904d5bc1c63 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Tue, 7 Oct 2025 20:25:08 -0700 Subject: [PATCH 07/51] fix tests --- .../transaction_result_error_messages_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/storage/store/inmemory/unsynchronized/transaction_result_error_messages_test.go b/storage/store/inmemory/unsynchronized/transaction_result_error_messages_test.go index 58ea11c3993..f765ba2c37e 100644 --- a/storage/store/inmemory/unsynchronized/transaction_result_error_messages_test.go +++ b/storage/store/inmemory/unsynchronized/transaction_result_error_messages_test.go @@ -3,6 +3,7 @@ package unsynchronized import ( "testing" + "github.com/jordanschalm/lockctx" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -34,10 +35,9 @@ func TestLightTransactionResultErrorMessages_HappyPath(t *testing.T) { // Store error messages lockManager := storage.NewTestingLockManager() - lctx := lockManager.NewContext() - defer lctx.Release() - - err := store.Store(lctx, block.ID(), errorMessages) + err := storage.WithLock(lockManager, storage.LockInsertTransactionResultErrMessage, func(lctx lockctx.Context) error { + return store.Store(lctx, block.ID(), errorMessages) + }) require.NoError(t, err) // Retrieve by BlockID and TransactionID From 155220d2e21b1d81aa7decbc2f34aeaf4c80da47 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Tue, 7 Oct 2025 21:15:45 -0700 Subject: [PATCH 08/51] fix tests --- .../tx_error_messages/tx_error_messages_core_test.go | 10 +++++----- .../tx_error_messages/tx_error_messages_engine_test.go | 4 ++-- module/executiondatasync/optimistic_sync/core.go | 4 ++-- .../optimistic_sync/core_impl_test.go | 2 +- .../optimistic_sync/persisters/block_test.go | 8 ++++---- 5 files changed, 14 insertions(+), 14 deletions(-) diff --git a/engine/access/ingestion/tx_error_messages/tx_error_messages_core_test.go b/engine/access/ingestion/tx_error_messages/tx_error_messages_core_test.go index 8b5d504fdfc..e89d7ace78a 100644 --- a/engine/access/ingestion/tx_error_messages/tx_error_messages_core_test.go +++ b/engine/access/ingestion/tx_error_messages/tx_error_messages_core_test.go @@ -92,7 +92,7 @@ func (s *TxErrorMessagesCoreSuite) SetupTest() { err := s.indexReporter.Initialize(s.reporter) s.Require().NoError(err) s.txResultsIndex = index.NewTransactionResultsIndex(s.indexReporter, s.lightTxResults) - + // Initialize lock manager for tests s.lockManager = storage.NewTestingLockManager() @@ -148,7 +148,7 @@ func (s *TxErrorMessagesCoreSuite) TestHandleTransactionResultErrorMessages() { expectedStoreTxErrorMessages := createExpectedTxErrorMessages(resultsByBlockID, s.enNodeIDs.NodeIDs()[0]) // Mock the storage of the fetched error messages into the protocol database. - s.txErrorMessages.On("Store", blockId, expectedStoreTxErrorMessages). + s.txErrorMessages.On("Store", mock.Anything, blockId, expectedStoreTxErrorMessages). Return(nil).Once() core := s.initCore() @@ -233,7 +233,7 @@ func (s *TxErrorMessagesCoreSuite) TestHandleTransactionResultErrorMessages_Erro // Simulate an error when attempting to store the fetched transaction error messages in storage. expectedStoreTxErrorMessages := createExpectedTxErrorMessages(resultsByBlockID, s.enNodeIDs.NodeIDs()[0]) - s.txErrorMessages.On("Store", blockId, expectedStoreTxErrorMessages). + s.txErrorMessages.On("Store", mock.Anything, blockId, expectedStoreTxErrorMessages). Return(fmt.Errorf("storage error")).Once() core := s.initCore() @@ -317,7 +317,7 @@ func mockTransactionResultsByBlock(count int) []flow.LightTransactionResult { // setupReceiptsForBlock sets up mock execution receipts for a block and returns the receipts along // with the identities of the execution nodes that processed them. -func setupReceiptsForBlock(receipts *storage.ExecutionReceipts, block *flow.Block, eNodeID flow.Identifier) { +func setupReceiptsForBlock(receipts *storagemock.ExecutionReceipts, block *flow.Block, eNodeID flow.Identifier) { receipt1 := unittest.ReceiptForBlockFixture(block) receipt1.ExecutorID = eNodeID receipt2 := unittest.ReceiptForBlockFixture(block) @@ -334,7 +334,7 @@ func setupReceiptsForBlock(receipts *storage.ExecutionReceipts, block *flow.Bloc } // setupReceiptsForBlockWithResult sets up mock execution receipts for a block with a specific execution result -func setupReceiptsForBlockWithResult(receipts *storage.ExecutionReceipts, executionResult *flow.ExecutionResult, executorIDs ...flow.Identifier) { +func setupReceiptsForBlockWithResult(receipts *storagemock.ExecutionReceipts, executionResult *flow.ExecutionResult, executorIDs ...flow.Identifier) { receiptList := make(flow.ExecutionReceiptList, 0, len(executorIDs)) for _, enID := range executorIDs { receiptList = append(receiptList, unittest.ExecutionReceiptFixture( diff --git a/engine/access/ingestion/tx_error_messages/tx_error_messages_engine_test.go b/engine/access/ingestion/tx_error_messages/tx_error_messages_engine_test.go index 11dbac37123..c2c1e309429 100644 --- a/engine/access/ingestion/tx_error_messages/tx_error_messages_engine_test.go +++ b/engine/access/ingestion/tx_error_messages/tx_error_messages_engine_test.go @@ -106,7 +106,7 @@ func (s *TxErrorMessagesEngineSuite) SetupTest() { err := s.indexReporter.Initialize(s.reporter) s.Require().NoError(err) s.txResultsIndex = index.NewTransactionResultsIndex(s.indexReporter, s.lightTxResults) - + // Initialize lock manager for tests s.lockManager = storage.NewTestingLockManager() @@ -250,7 +250,7 @@ func (s *TxErrorMessagesEngineSuite) TestOnFinalizedBlockHandleTxErrorMessages() expectedStoreTxErrorMessages := createExpectedTxErrorMessages(resultsByBlockID, s.enNodeIDs.NodeIDs()[0]) // Mock the storage of the fetched error messages into the protocol database. - s.txErrorMessages.On("Store", blockID, expectedStoreTxErrorMessages).Return(nil). + s.txErrorMessages.On("Store", mock.Anything, blockID, expectedStoreTxErrorMessages).Return(nil). Run(func(args mock.Arguments) { // Ensure the test does not complete its work faster than necessary wg.Done() diff --git a/module/executiondatasync/optimistic_sync/core.go b/module/executiondatasync/optimistic_sync/core.go index fad5723f476..fcfcd2c3e87 100644 --- a/module/executiondatasync/optimistic_sync/core.go +++ b/module/executiondatasync/optimistic_sync/core.go @@ -148,9 +148,9 @@ func NewCoreImpl( persisterStores := []stores.PersisterStore{ stores.NewEventsStore(inmemEvents, persistentEvents, executionResult.BlockID), - stores.NewResultsStore(inmemResults, persistentResults, executionResult.BlockID), + stores.NewResultsStore(inmemResults, persistentResults, executionResult.BlockID, lockManager), stores.NewCollectionsStore(inmemCollections, persistentCollections, lockManager), - stores.NewTxResultErrMsgStore(inmemTxResultErrMsgs, persistentTxResultErrMsg, executionResult.BlockID), + stores.NewTxResultErrMsgStore(inmemTxResultErrMsgs, persistentTxResultErrMsg, executionResult.BlockID, lockManager), stores.NewLatestSealedResultStore(latestPersistedSealedResult, executionResult.ID(), header.Height), } diff --git a/module/executiondatasync/optimistic_sync/core_impl_test.go b/module/executiondatasync/optimistic_sync/core_impl_test.go index 95ca5e7571f..b4b7592bdaa 100644 --- a/module/executiondatasync/optimistic_sync/core_impl_test.go +++ b/module/executiondatasync/optimistic_sync/core_impl_test.go @@ -75,7 +75,7 @@ func (c *CoreImplSuite) SetupTest() { c.persistentCollections.On("BatchStoreLightAndIndexByTransaction", mock.Anything, mock.Anything).Return(nil).Maybe() c.persistentTransactions.On("BatchStore", mock.Anything, mock.Anything).Return(nil).Maybe() c.persistentResults.On("BatchStore", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() - c.persistentTxResultErrMsg.On("BatchStore", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() + c.persistentTxResultErrMsg.On("BatchStore", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() c.latestPersistedSealedResult.On("BatchSet", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() } diff --git a/module/executiondatasync/optimistic_sync/persisters/block_test.go b/module/executiondatasync/optimistic_sync/persisters/block_test.go index bf6ab0b6054..4850ba49e5b 100644 --- a/module/executiondatasync/optimistic_sync/persisters/block_test.go +++ b/module/executiondatasync/optimistic_sync/persisters/block_test.go @@ -195,8 +195,8 @@ func (p *PersisterSuite) TestPersister_PersistWithData() { storedTransactions = append(storedTransactions, *transaction) }).Return(nil).Times(len(p.inMemoryTransactions.Data())) - p.txResultErrMsg.On("BatchStore", p.executionResult.BlockID, mock.Anything, mock.Anything).Run(func(args mock.Arguments) { - terrm, ok := args.Get(1).([]flow.TransactionResultErrorMessage) + p.txResultErrMsg.On("BatchStore", mock.Anything, mock.Anything, p.executionResult.BlockID, mock.Anything).Run(func(args mock.Arguments) { + terrm, ok := args.Get(3).([]flow.TransactionResultErrorMessage) p.Require().True(ok) storedTxResultErrMsgs = terrm }).Return(nil) @@ -270,7 +270,7 @@ func (p *PersisterSuite) TestPersister_PersistErrorHandling() { p.collections.On("BatchStoreAndIndexByTransaction", mock.Anything, mock.Anything, mock.Anything).Return(&flow.LightCollection{}, nil).Times(numberOfCollections) numberOfTransactions := len(p.inMemoryTransactions.Data()) p.transactions.On("BatchStore", mock.Anything, mock.Anything).Return(nil).Times(numberOfTransactions) - p.txResultErrMsg.On("BatchStore", p.executionResult.BlockID, mock.Anything, mock.Anything).Return(assert.AnError).Once() + p.txResultErrMsg.On("BatchStore", mock.Anything, mock.Anything, p.executionResult.BlockID, mock.Anything).Return(assert.AnError).Once() }, expectedError: "could not add transaction result error messages to batch", }, @@ -283,7 +283,7 @@ func (p *PersisterSuite) TestPersister_PersistErrorHandling() { p.collections.On("BatchStoreAndIndexByTransaction", mock.Anything, mock.Anything, mock.Anything).Return(&flow.LightCollection{}, nil).Times(numberOfCollections) numberOfTransactions := len(p.inMemoryTransactions.Data()) p.transactions.On("BatchStore", mock.Anything, mock.Anything).Return(nil).Times(numberOfTransactions) - p.txResultErrMsg.On("BatchStore", p.executionResult.BlockID, mock.Anything, mock.Anything).Return(nil).Once() + p.txResultErrMsg.On("BatchStore", mock.Anything, mock.Anything, p.executionResult.BlockID, mock.Anything).Return(nil).Once() p.latestPersistedSealedResult.On("BatchSet", p.executionResult.ID(), p.header.Height, mock.Anything).Return(assert.AnError).Once() }, expectedError: "could not persist latest sealed result", From e666a2a491a41f932f2de321ea610f491942891c Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Tue, 7 Oct 2025 21:20:09 -0700 Subject: [PATCH 09/51] fix lint --- storage/light_transaction_results.go | 1 + storage/operation/transaction_results.go | 1 + storage/store/transaction_result_error_messages.go | 1 + storage/transaction_result_error_messages.go | 1 + 4 files changed, 4 insertions(+) diff --git a/storage/light_transaction_results.go b/storage/light_transaction_results.go index a34940eeda8..60e2cffc41d 100644 --- a/storage/light_transaction_results.go +++ b/storage/light_transaction_results.go @@ -2,6 +2,7 @@ package storage import ( "github.com/jordanschalm/lockctx" + "github.com/onflow/flow-go/model/flow" ) diff --git a/storage/operation/transaction_results.go b/storage/operation/transaction_results.go index cc3c46f9dd7..c4f1675840e 100644 --- a/storage/operation/transaction_results.go +++ b/storage/operation/transaction_results.go @@ -4,6 +4,7 @@ import ( "fmt" "github.com/jordanschalm/lockctx" + "github.com/onflow/flow-go/model/flow" "github.com/onflow/flow-go/storage" ) diff --git a/storage/store/transaction_result_error_messages.go b/storage/store/transaction_result_error_messages.go index 15989918f1b..26fe43e94c8 100644 --- a/storage/store/transaction_result_error_messages.go +++ b/storage/store/transaction_result_error_messages.go @@ -4,6 +4,7 @@ import ( "fmt" "github.com/jordanschalm/lockctx" + "github.com/onflow/flow-go/model/flow" "github.com/onflow/flow-go/module" "github.com/onflow/flow-go/module/metrics" diff --git a/storage/transaction_result_error_messages.go b/storage/transaction_result_error_messages.go index 768ebd8d89e..9e52b2e0f33 100644 --- a/storage/transaction_result_error_messages.go +++ b/storage/transaction_result_error_messages.go @@ -2,6 +2,7 @@ package storage import ( "github.com/jordanschalm/lockctx" + "github.com/onflow/flow-go/model/flow" ) From 5277616b84a8f9f80c9dd1fae7f1b90d911d1ba6 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Wed, 8 Oct 2025 08:48:16 -0700 Subject: [PATCH 10/51] fix admin tests --- admin/commands/storage/backfill_tx_error_messages_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/admin/commands/storage/backfill_tx_error_messages_test.go b/admin/commands/storage/backfill_tx_error_messages_test.go index bf06ed29940..ce7a11211b2 100644 --- a/admin/commands/storage/backfill_tx_error_messages_test.go +++ b/admin/commands/storage/backfill_tx_error_messages_test.go @@ -534,7 +534,7 @@ func (suite *BackfillTxErrorMessagesSuite) mockStoreTxErrorMessages( } } - suite.txErrorMessages.On("Store", blockID, txErrorMessages).Return(nil).Once() + suite.txErrorMessages.On("Store", mock.Anything, blockID, txErrorMessages).Return(nil).Once() } // assertAllExpectations asserts that all the expectations set on various mocks are met, From 28b4f19e10ef7a44bc7cf2874bf50d514cc25f44 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Wed, 8 Oct 2025 09:12:29 -0700 Subject: [PATCH 11/51] handle already exists error --- .../optimistic_sync/persisters/stores/events.go | 10 ++++++---- .../stores/transaction_result_error_messages.go | 12 +++++------- storage/errors.go | 10 ++++++++++ 3 files changed, 21 insertions(+), 11 deletions(-) diff --git a/module/executiondatasync/optimistic_sync/persisters/stores/events.go b/module/executiondatasync/optimistic_sync/persisters/stores/events.go index 629adcc2a6f..1e99767d71b 100644 --- a/module/executiondatasync/optimistic_sync/persisters/stores/events.go +++ b/module/executiondatasync/optimistic_sync/persisters/stores/events.go @@ -39,10 +39,12 @@ func (e *EventsStore) Persist(lctx lockctx.Proof, batch storage.ReaderBatchWrite return fmt.Errorf("could not get events: %w", err) } - if len(eventsList) > 0 { - if err := e.persistedEvents.BatchStore(e.blockID, []flow.EventsList{eventsList}, batch); err != nil { - return fmt.Errorf("could not add events to batch: %w", err) - } + err = storage.SkipAlreadyExistsError( // Note: if the data already exists, we will not overwrite + e.persistedEvents.BatchStore(e.blockID, []flow.EventsList{eventsList}, batch), + ) + + if err != nil { + return fmt.Errorf("could not add events to batch: %w", err) } return nil diff --git a/module/executiondatasync/optimistic_sync/persisters/stores/transaction_result_error_messages.go b/module/executiondatasync/optimistic_sync/persisters/stores/transaction_result_error_messages.go index e8280c33e4c..b7a1edaa4b7 100644 --- a/module/executiondatasync/optimistic_sync/persisters/stores/transaction_result_error_messages.go +++ b/module/executiondatasync/optimistic_sync/persisters/stores/transaction_result_error_messages.go @@ -42,14 +42,12 @@ func (t *TxResultErrMsgStore) Persist(lctx lockctx.Proof, batch storage.ReaderBa return fmt.Errorf("could not get transaction result error messages: %w", err) } - if len(txResultErrMsgs) > 0 { - // Use storage.WithLock to acquire the necessary lock and store the error messages - err := storage.WithLock(t.lockManager, storage.LockInsertTransactionResultErrMessage, func(lctx lockctx.Context) error { + err = storage.SkipAlreadyExistsError( // Note: if the data already exists, we will not overwrite + storage.WithLock(t.lockManager, storage.LockInsertTransactionResultErrMessage, func(lctx lockctx.Context) error { return t.persistedTxResultErrMsg.BatchStore(lctx, batch, t.blockID, txResultErrMsgs) - }) - if err != nil { - return fmt.Errorf("could not add transaction result error messages to batch: %w", err) - } + })) + if err != nil { + return fmt.Errorf("could not add transaction result error messages to batch: %w", err) } return nil diff --git a/storage/errors.go b/storage/errors.go index b3d81d9709c..a31d1b6f74c 100644 --- a/storage/errors.go +++ b/storage/errors.go @@ -57,3 +57,13 @@ func NewInvalidDKGStateTransitionErrorf(from, to flow.DKGState, msg string, args err: fmt.Errorf(msg, args...), } } + +// SkipAlreadyExistsError returns nil if the provided error is ErrAlreadyExists, otherwise returns the original error. +// It usually means the storage operation to insert a record was skipped because the key of the record already exists. +// CAUTION : it does NOT check the equality of the value of the record. +func SkipAlreadyExistsError(err error) error { + if errors.Is(err, ErrAlreadyExists) { + return nil + } + return err +} From bbd6ed902dd89e5f306f49ac3a92598e1e775574 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Wed, 8 Oct 2025 09:29:38 -0700 Subject: [PATCH 12/51] fix tests --- .../optimistic_sync/persisters/block_test.go | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/module/executiondatasync/optimistic_sync/persisters/block_test.go b/module/executiondatasync/optimistic_sync/persisters/block_test.go index 4850ba49e5b..0930a29cd8b 100644 --- a/module/executiondatasync/optimistic_sync/persisters/block_test.go +++ b/module/executiondatasync/optimistic_sync/persisters/block_test.go @@ -150,15 +150,20 @@ func (p *PersisterSuite) TestPersister_PersistWithEmptyData() { p.latestPersistedSealedResult.On("BatchSet", p.executionResult.ID(), p.header.Height, mock.Anything).Return(nil).Once() + // Events store always calls BatchStore regardless of data + p.events.On("BatchStore", p.executionResult.BlockID, mock.Anything, mock.Anything).Return(nil).Once() + + // Transaction result error messages store always calls BatchStore regardless of data + p.txResultErrMsg.On("BatchStore", mock.Anything, mock.Anything, p.executionResult.BlockID, mock.Anything).Return(nil).Once() + err = p.persister.Persist() p.Require().NoError(err) // Verify other storages were not called since the data is empty - p.events.AssertNotCalled(t, "BatchStore") + // Note: events and txResultErrMsg stores always call BatchStore regardless of data p.results.AssertNotCalled(t, "BatchStore") p.collections.AssertNotCalled(t, "BatchStoreAndIndexByTransaction") p.transactions.AssertNotCalled(t, "BatchStore") - p.txResultErrMsg.AssertNotCalled(t, "BatchStore") } func (p *PersisterSuite) TestPersister_PersistWithData() { @@ -176,8 +181,8 @@ func (p *PersisterSuite) TestPersister_PersistWithData() { storedEvents = se }).Return(nil) - p.results.On("BatchStore", p.executionResult.BlockID, mock.Anything, mock.Anything).Run(func(args mock.Arguments) { - sr, ok := args.Get(1).([]flow.LightTransactionResult) + p.results.On("BatchStore", mock.Anything, mock.Anything, p.executionResult.BlockID, mock.Anything).Run(func(args mock.Arguments) { + sr, ok := args.Get(3).([]flow.LightTransactionResult) p.Require().True(ok) storedResults = sr }).Return(nil) @@ -237,7 +242,7 @@ func (p *PersisterSuite) TestPersister_PersistErrorHandling() { name: "ResultsBatchStoreError", setupMocks: func() { p.events.On("BatchStore", p.executionResult.BlockID, mock.Anything, mock.Anything).Return(nil).Once() - p.results.On("BatchStore", p.executionResult.BlockID, mock.Anything, mock.Anything).Return(assert.AnError).Once() + p.results.On("BatchStore", mock.Anything, mock.Anything, p.executionResult.BlockID, mock.Anything).Return(assert.AnError).Once() }, expectedError: "could not add transaction results to batch", }, @@ -245,7 +250,7 @@ func (p *PersisterSuite) TestPersister_PersistErrorHandling() { name: "CollectionsStoreError", setupMocks: func() { p.events.On("BatchStore", p.executionResult.BlockID, mock.Anything, mock.Anything).Return(nil).Once() - p.results.On("BatchStore", p.executionResult.BlockID, mock.Anything, mock.Anything).Return(nil).Once() + p.results.On("BatchStore", mock.Anything, mock.Anything, p.executionResult.BlockID, mock.Anything).Return(nil).Once() p.collections.On("BatchStoreAndIndexByTransaction", mock.Anything, mock.Anything, mock.Anything).Return(&flow.LightCollection{}, assert.AnError).Once() }, expectedError: "could not add light collections to batch", @@ -254,7 +259,7 @@ func (p *PersisterSuite) TestPersister_PersistErrorHandling() { name: "TransactionsStoreError", setupMocks: func() { p.events.On("BatchStore", p.executionResult.BlockID, mock.Anything, mock.Anything).Return(nil).Once() - p.results.On("BatchStore", p.executionResult.BlockID, mock.Anything, mock.Anything).Return(nil).Once() + p.results.On("BatchStore", mock.Anything, mock.Anything, p.executionResult.BlockID, mock.Anything).Return(nil).Once() numberOfCollections := len(p.inMemoryCollections.Data()) p.collections.On("BatchStoreAndIndexByTransaction", mock.Anything, mock.Anything, mock.Anything).Return(&flow.LightCollection{}, nil).Times(numberOfCollections) p.transactions.On("BatchStore", mock.Anything, mock.Anything).Return(assert.AnError).Once() @@ -265,7 +270,7 @@ func (p *PersisterSuite) TestPersister_PersistErrorHandling() { name: "TxResultErrMsgStoreError", setupMocks: func() { p.events.On("BatchStore", p.executionResult.BlockID, mock.Anything, mock.Anything).Return(nil).Once() - p.results.On("BatchStore", p.executionResult.BlockID, mock.Anything, mock.Anything).Return(nil).Once() + p.results.On("BatchStore", mock.Anything, mock.Anything, p.executionResult.BlockID, mock.Anything).Return(nil).Once() numberOfCollections := len(p.inMemoryCollections.Data()) p.collections.On("BatchStoreAndIndexByTransaction", mock.Anything, mock.Anything, mock.Anything).Return(&flow.LightCollection{}, nil).Times(numberOfCollections) numberOfTransactions := len(p.inMemoryTransactions.Data()) @@ -278,7 +283,7 @@ func (p *PersisterSuite) TestPersister_PersistErrorHandling() { name: "LatestPersistedSealedResultStoreError", setupMocks: func() { p.events.On("BatchStore", p.executionResult.BlockID, mock.Anything, mock.Anything).Return(nil).Once() - p.results.On("BatchStore", p.executionResult.BlockID, mock.Anything, mock.Anything).Return(nil).Once() + p.results.On("BatchStore", mock.Anything, mock.Anything, p.executionResult.BlockID, mock.Anything).Return(nil).Once() numberOfCollections := len(p.inMemoryCollections.Data()) p.collections.On("BatchStoreAndIndexByTransaction", mock.Anything, mock.Anything, mock.Anything).Return(&flow.LightCollection{}, nil).Times(numberOfCollections) numberOfTransactions := len(p.inMemoryTransactions.Data()) From af6427a4d9e4529110dee31f7fd2bb6b76222d61 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Wed, 8 Oct 2025 09:49:22 -0700 Subject: [PATCH 13/51] handle already exists error --- .../state_synchronization/indexer/in_memory_indexer.go | 9 +++++---- .../state_synchronization/indexer/indexer_core_test.go | 7 ++++--- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/module/state_synchronization/indexer/in_memory_indexer.go b/module/state_synchronization/indexer/in_memory_indexer.go index b31607f3d24..95cd18b8c44 100644 --- a/module/state_synchronization/indexer/in_memory_indexer.go +++ b/module/state_synchronization/indexer/in_memory_indexer.go @@ -68,10 +68,11 @@ func NewInMemoryIndexer( // IndexTxResultErrorMessagesData index transaction result error messages // No errors are expected during normal operation. func (i *InMemoryIndexer) IndexTxResultErrorMessagesData(txResultErrMsgs []flow.TransactionResultErrorMessage) error { - err := storage.WithLock(i.lockManager, storage.LockInsertTransactionResultErrMessage, func(lctx lockctx.Context) error { - // requires the [storage.LockInsertTransactionResultErrMessage] lock to be held - return i.txResultErrMsgs.Store(lctx, i.executionResult.BlockID, txResultErrMsgs) - }) + err := storage.SkipAlreadyExistsError( // Note: if the data already exists, we will not overwrite + storage.WithLock(i.lockManager, storage.LockInsertTransactionResultErrMessage, func(lctx lockctx.Context) error { + // requires the [storage.LockInsertTransactionResultErrMessage] lock to be held + return i.txResultErrMsgs.Store(lctx, i.executionResult.BlockID, txResultErrMsgs) + })) if err != nil { return fmt.Errorf("could not index transaction result error messages: %w", err) } diff --git a/module/state_synchronization/indexer/indexer_core_test.go b/module/state_synchronization/indexer/indexer_core_test.go index a1ccd0c3639..fecdafd4996 100644 --- a/module/state_synchronization/indexer/indexer_core_test.go +++ b/module/state_synchronization/indexer/indexer_core_test.go @@ -6,6 +6,7 @@ import ( "os" "testing" + "github.com/jordanschalm/lockctx" "github.com/rs/zerolog" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -141,8 +142,8 @@ func (i *indexCoreTest) setStoreEvents(f func(*testing.T, flow.Identifier, []flo func (i *indexCoreTest) setStoreTransactionResults(f func(*testing.T, flow.Identifier, []flow.LightTransactionResult) error) *indexCoreTest { i.results. - On("BatchStore", mock.AnythingOfType("flow.Identifier"), mock.AnythingOfType("[]flow.LightTransactionResult"), mock.Anything). - Return(func(blockID flow.Identifier, results []flow.LightTransactionResult, batch storage.ReaderBatchWriter) error { + On("BatchStore", mock.Anything, mock.Anything, mock.AnythingOfType("flow.Identifier"), mock.AnythingOfType("[]flow.LightTransactionResult")). + Return(func(lctx lockctx.Proof, batch storage.ReaderBatchWriter, blockID flow.Identifier, results []flow.LightTransactionResult) error { require.NotNil(i.t, batch) return f(i.t, blockID, results) }) @@ -175,7 +176,7 @@ func (i *indexCoreTest) useDefaultEvents() *indexCoreTest { func (i *indexCoreTest) useDefaultTransactionResults() *indexCoreTest { i.results. - On("BatchStore", mock.AnythingOfType("flow.Identifier"), mock.AnythingOfType("[]flow.LightTransactionResult"), mock.Anything). + On("BatchStore", mock.Anything, mock.Anything, mock.AnythingOfType("flow.Identifier"), mock.AnythingOfType("[]flow.LightTransactionResult")). Return(nil) return i } From 4eb3b45fc5a14ffc9e3eddaca4e1afb5922468be Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Wed, 8 Oct 2025 09:56:00 -0700 Subject: [PATCH 14/51] fix optimisic sync core store operation --- module/executiondatasync/optimistic_sync/core.go | 2 +- .../optimistic_sync/persisters/block_test.go | 2 +- .../optimistic_sync/persisters/stores/results.go | 14 +++----------- 3 files changed, 5 insertions(+), 13 deletions(-) diff --git a/module/executiondatasync/optimistic_sync/core.go b/module/executiondatasync/optimistic_sync/core.go index fcfcd2c3e87..44a772ca6ad 100644 --- a/module/executiondatasync/optimistic_sync/core.go +++ b/module/executiondatasync/optimistic_sync/core.go @@ -148,7 +148,7 @@ func NewCoreImpl( persisterStores := []stores.PersisterStore{ stores.NewEventsStore(inmemEvents, persistentEvents, executionResult.BlockID), - stores.NewResultsStore(inmemResults, persistentResults, executionResult.BlockID, lockManager), + stores.NewResultsStore(inmemResults, persistentResults, executionResult.BlockID), stores.NewCollectionsStore(inmemCollections, persistentCollections, lockManager), stores.NewTxResultErrMsgStore(inmemTxResultErrMsgs, persistentTxResultErrMsg, executionResult.BlockID, lockManager), stores.NewLatestSealedResultStore(latestPersistedSealedResult, executionResult.ID(), header.Height), diff --git a/module/executiondatasync/optimistic_sync/persisters/block_test.go b/module/executiondatasync/optimistic_sync/persisters/block_test.go index 0930a29cd8b..ac34a02e93c 100644 --- a/module/executiondatasync/optimistic_sync/persisters/block_test.go +++ b/module/executiondatasync/optimistic_sync/persisters/block_test.go @@ -82,7 +82,7 @@ func (p *PersisterSuite) SetupTest() { p.header, []stores.PersisterStore{ stores.NewEventsStore(p.inMemoryEvents, p.events, p.executionResult.BlockID), - stores.NewResultsStore(p.inMemoryResults, p.results, p.executionResult.BlockID, p.lockManager), + stores.NewResultsStore(p.inMemoryResults, p.results, p.executionResult.BlockID), stores.NewCollectionsStore(p.inMemoryCollections, p.collections, p.lockManager), stores.NewTransactionsStore(p.inMemoryTransactions, p.transactions), stores.NewTxResultErrMsgStore(p.inMemoryTxResultErrMsg, p.txResultErrMsg, p.executionResult.BlockID, p.lockManager), diff --git a/module/executiondatasync/optimistic_sync/persisters/stores/results.go b/module/executiondatasync/optimistic_sync/persisters/stores/results.go index 2fd73f226ef..c84fa7d5b84 100644 --- a/module/executiondatasync/optimistic_sync/persisters/stores/results.go +++ b/module/executiondatasync/optimistic_sync/persisters/stores/results.go @@ -17,20 +17,17 @@ type ResultsStore struct { inMemoryResults *unsynchronized.LightTransactionResults persistedResults storage.LightTransactionResults blockID flow.Identifier - lockManager storage.LockManager } func NewResultsStore( inMemoryResults *unsynchronized.LightTransactionResults, persistedResults storage.LightTransactionResults, blockID flow.Identifier, - lockManager storage.LockManager, ) *ResultsStore { return &ResultsStore{ inMemoryResults: inMemoryResults, persistedResults: persistedResults, blockID: blockID, - lockManager: lockManager, } } @@ -42,14 +39,9 @@ func (r *ResultsStore) Persist(lctx lockctx.Proof, batch storage.ReaderBatchWrit return fmt.Errorf("could not get results: %w", err) } - if len(results) > 0 { - // Use storage.WithLock to acquire the necessary lock and store the results - err := storage.WithLock(r.lockManager, storage.LockInsertLightTransactionResult, func(lctx lockctx.Context) error { - return r.persistedResults.BatchStore(lctx, batch, r.blockID, results) - }) - if err != nil { - return fmt.Errorf("could not add transaction results to batch: %w", err) - } + err = r.persistedResults.BatchStore(lctx, batch, r.blockID, results) + if err != nil { + return fmt.Errorf("could not add transaction results to batch: %w", err) } return nil From 45524d0397c9b925d3df8daaa59ccff26652c7ca Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Wed, 8 Oct 2025 10:12:45 -0700 Subject: [PATCH 15/51] fix optimistic sync persister block --- .../optimistic_sync/core_impl_test.go | 2 +- .../optimistic_sync/persisters/block.go | 24 +++++++++---------- .../persisters/stores/results.go | 2 ++ storage/locks.go | 3 +++ 4 files changed, 17 insertions(+), 14 deletions(-) diff --git a/module/executiondatasync/optimistic_sync/core_impl_test.go b/module/executiondatasync/optimistic_sync/core_impl_test.go index b4b7592bdaa..48161d60968 100644 --- a/module/executiondatasync/optimistic_sync/core_impl_test.go +++ b/module/executiondatasync/optimistic_sync/core_impl_test.go @@ -74,7 +74,7 @@ func (c *CoreImplSuite) SetupTest() { c.persistentEvents.On("BatchStore", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() c.persistentCollections.On("BatchStoreLightAndIndexByTransaction", mock.Anything, mock.Anything).Return(nil).Maybe() c.persistentTransactions.On("BatchStore", mock.Anything, mock.Anything).Return(nil).Maybe() - c.persistentResults.On("BatchStore", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() + c.persistentResults.On("BatchStore", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() c.persistentTxResultErrMsg.On("BatchStore", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() c.latestPersistedSealedResult.On("BatchSet", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() } diff --git a/module/executiondatasync/optimistic_sync/persisters/block.go b/module/executiondatasync/optimistic_sync/persisters/block.go index cfb617e858b..34dc4768fa7 100644 --- a/module/executiondatasync/optimistic_sync/persisters/block.go +++ b/module/executiondatasync/optimistic_sync/persisters/block.go @@ -62,20 +62,18 @@ func (p *BlockPersister) Persist() error { p.log.Debug().Msg("started to persist execution data") start := time.Now() - lctx := p.lockManager.NewContext() - err := lctx.AcquireLock(storage.LockInsertCollection) - if err != nil { - return fmt.Errorf("could not acquire lock for inserting light collections: %w", err) - } - defer lctx.Release() - - err = p.protocolDB.WithReaderBatchWriter(func(batch storage.ReaderBatchWriter) error { - for _, persister := range p.persisterStores { - if err := persister.Persist(lctx, batch); err != nil { - return err + err := storage.WithLocks(p.lockManager, []string{ + storage.LockInsertCollection, + storage.LockInsertLightTransactionResult, + }, func(lctx lockctx.Context) error { + return p.protocolDB.WithReaderBatchWriter(func(batch storage.ReaderBatchWriter) error { + for _, persister := range p.persisterStores { + if err := persister.Persist(lctx, batch); err != nil { + return err + } } - } - return nil + return nil + }) }) if err != nil { diff --git a/module/executiondatasync/optimistic_sync/persisters/stores/results.go b/module/executiondatasync/optimistic_sync/persisters/stores/results.go index c84fa7d5b84..ce56a385b0b 100644 --- a/module/executiondatasync/optimistic_sync/persisters/stores/results.go +++ b/module/executiondatasync/optimistic_sync/persisters/stores/results.go @@ -32,6 +32,7 @@ func NewResultsStore( } // Persist adds results to the batch. +// requires [storage.LockInsertLightTransactionResult] to be hold // No errors are expected during normal operations func (r *ResultsStore) Persist(lctx lockctx.Proof, batch storage.ReaderBatchWriter) error { results, err := r.inMemoryResults.ByBlockID(r.blockID) @@ -39,6 +40,7 @@ func (r *ResultsStore) Persist(lctx lockctx.Proof, batch storage.ReaderBatchWrit return fmt.Errorf("could not get results: %w", err) } + // requires [storage.LockInsertLightTransactionResult] to be hold err = r.persistedResults.BatchStore(lctx, batch, r.blockID, results) if err != nil { return fmt.Errorf("could not add transaction results to batch: %w", err) diff --git a/storage/locks.go b/storage/locks.go index 84dbd558c00..0d0581b645f 100644 --- a/storage/locks.go +++ b/storage/locks.go @@ -71,6 +71,9 @@ func makeLockPolicy() lockctx.Policy { Add(LockInsertBlock, LockFinalizeBlock). Add(LockFinalizeBlock, LockBootstrapping). Add(LockInsertOwnReceipt, LockInsertChunkDataPack). + + // module/executiondatasync/optimistic_sync/persisters/block.go#Persist + Add(LockInsertCollection, LockInsertLightTransactionResult). Build() } From 1ee701b59945f879218fc426b58adfda1fece73b Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Wed, 8 Oct 2025 10:17:44 -0700 Subject: [PATCH 16/51] fix tests --- .../optimistic_sync/persisters/block_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/module/executiondatasync/optimistic_sync/persisters/block_test.go b/module/executiondatasync/optimistic_sync/persisters/block_test.go index ac34a02e93c..d416ea8e999 100644 --- a/module/executiondatasync/optimistic_sync/persisters/block_test.go +++ b/module/executiondatasync/optimistic_sync/persisters/block_test.go @@ -153,6 +153,9 @@ func (p *PersisterSuite) TestPersister_PersistWithEmptyData() { // Events store always calls BatchStore regardless of data p.events.On("BatchStore", p.executionResult.BlockID, mock.Anything, mock.Anything).Return(nil).Once() + // Results store always calls BatchStore regardless of data + p.results.On("BatchStore", mock.Anything, mock.Anything, p.executionResult.BlockID, mock.Anything).Return(nil).Once() + // Transaction result error messages store always calls BatchStore regardless of data p.txResultErrMsg.On("BatchStore", mock.Anything, mock.Anything, p.executionResult.BlockID, mock.Anything).Return(nil).Once() @@ -160,8 +163,7 @@ func (p *PersisterSuite) TestPersister_PersistWithEmptyData() { p.Require().NoError(err) // Verify other storages were not called since the data is empty - // Note: events and txResultErrMsg stores always call BatchStore regardless of data - p.results.AssertNotCalled(t, "BatchStore") + // Note: events, results, and txResultErrMsg stores always call BatchStore regardless of data p.collections.AssertNotCalled(t, "BatchStoreAndIndexByTransaction") p.transactions.AssertNotCalled(t, "BatchStore") } From e2039acc0714e33170755e38d6b3dc9d1374f905 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Wed, 8 Oct 2025 11:48:53 -0700 Subject: [PATCH 17/51] fix tests --- .../executiondatasync/optimistic_sync/persisters/block_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/module/executiondatasync/optimistic_sync/persisters/block_test.go b/module/executiondatasync/optimistic_sync/persisters/block_test.go index c09b7bbb13c..d7bb6641f7a 100644 --- a/module/executiondatasync/optimistic_sync/persisters/block_test.go +++ b/module/executiondatasync/optimistic_sync/persisters/block_test.go @@ -111,7 +111,7 @@ func (p *PersisterSuite) testWithDatabase() { stores.NewEventsStore(p.indexerData.Events, events, p.executionResult.BlockID), stores.NewResultsStore(p.indexerData.Results, results, p.executionResult.BlockID), stores.NewCollectionsStore(p.indexerData.Collections, collections), - stores.NewTxResultErrMsgStore(p.txErrMsgs, txResultErrMsg, p.executionResult.BlockID), + stores.NewTxResultErrMsgStore(p.txErrMsgs, txResultErrMsg, p.executionResult.BlockID, lockManager), stores.NewLatestSealedResultStore(latestPersistedSealedResult, p.executionResult.ID(), p.header.Height), }, ) From c9563fd4294d790e4dfb0b615b858486c09760bb Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Wed, 8 Oct 2025 12:12:22 -0700 Subject: [PATCH 18/51] fix optimistic sync core --- module/executiondatasync/optimistic_sync/core.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/module/executiondatasync/optimistic_sync/core.go b/module/executiondatasync/optimistic_sync/core.go index c222b28a8ac..044485a7ad5 100644 --- a/module/executiondatasync/optimistic_sync/core.go +++ b/module/executiondatasync/optimistic_sync/core.go @@ -335,7 +335,7 @@ func (c *CoreImpl) Persist() error { stores.NewEventsStore(indexerData.Events, c.workingData.persistentEvents, c.executionResult.BlockID), stores.NewResultsStore(indexerData.Results, c.workingData.persistentResults, c.executionResult.BlockID), stores.NewCollectionsStore(indexerData.Collections, c.workingData.persistentCollections), - stores.NewTxResultErrMsgStore(c.workingData.txResultErrMsgsData, c.workingData.persistentTxResultErrMsgs, c.executionResult.BlockID), + stores.NewTxResultErrMsgStore(c.workingData.txResultErrMsgsData, c.workingData.persistentTxResultErrMsgs, c.executionResult.BlockID, c.workingData.lockManager), stores.NewLatestSealedResultStore(c.workingData.latestPersistedSealedResult, c.executionResult.ID(), c.block.Height), } blockPersister := persisters.NewBlockPersister( From 6dddf0a04b6e3686e4f98ffd8eabdc21b98dfd55 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Wed, 8 Oct 2025 12:12:30 -0700 Subject: [PATCH 19/51] fix mocks --- .../optimistic_sync/core_impl_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/module/executiondatasync/optimistic_sync/core_impl_test.go b/module/executiondatasync/optimistic_sync/core_impl_test.go index 19b87866603..c67bcd9a072 100644 --- a/module/executiondatasync/optimistic_sync/core_impl_test.go +++ b/module/executiondatasync/optimistic_sync/core_impl_test.go @@ -445,11 +445,11 @@ func (c *CoreImplSuite) TestCoreImpl_Persist() { indexerData := core.workingData.indexerData c.persistentRegisters.On("Store", flow.RegisterEntries(indexerData.Registers), tf.block.Height).Return(nil) - c.persistentEvents.On("BatchStore", blockID, []flow.EventsList{indexerData.Events}, mock.Anything).Return(nil) - c.persistentCollections.On("BatchStoreAndIndexByTransaction", mock.Anything, mock.Anything, mock.Anything).Return(nil, nil) - c.persistentResults.On("BatchStore", blockID, indexerData.Results, mock.Anything).Return(nil) - c.persistentTxResultErrMsg.On("BatchStore", blockID, core.workingData.txResultErrMsgsData, mock.Anything).Return(nil) - c.latestPersistedSealedResult.On("BatchSet", tf.exeResult.ID(), tf.block.Height, mock.Anything).Return(nil) + c.persistentEvents.On("BatchStore", mock.Anything, blockID, []flow.EventsList{indexerData.Events}, mock.Anything).Return(nil) + c.persistentCollections.On("BatchStoreAndIndexByTransaction", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, nil) + c.persistentResults.On("BatchStore", mock.Anything, mock.Anything, blockID, indexerData.Results).Return(nil) + c.persistentTxResultErrMsg.On("BatchStore", mock.Anything, mock.Anything, blockID, core.workingData.txResultErrMsgsData).Return(nil) + c.latestPersistedSealedResult.On("BatchSet", mock.Anything, mock.Anything, tf.exeResult.ID(), tf.block.Height, mock.Anything).Return(nil) err = core.Persist() c.Require().NoError(err) From 346cdae4bc8c3f0e8a22cc5dfcd8627a19d9c44d Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Wed, 8 Oct 2025 12:14:12 -0700 Subject: [PATCH 20/51] fix mocks --- module/executiondatasync/optimistic_sync/core_impl_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/module/executiondatasync/optimistic_sync/core_impl_test.go b/module/executiondatasync/optimistic_sync/core_impl_test.go index c67bcd9a072..e2998624e20 100644 --- a/module/executiondatasync/optimistic_sync/core_impl_test.go +++ b/module/executiondatasync/optimistic_sync/core_impl_test.go @@ -445,7 +445,7 @@ func (c *CoreImplSuite) TestCoreImpl_Persist() { indexerData := core.workingData.indexerData c.persistentRegisters.On("Store", flow.RegisterEntries(indexerData.Registers), tf.block.Height).Return(nil) - c.persistentEvents.On("BatchStore", mock.Anything, blockID, []flow.EventsList{indexerData.Events}, mock.Anything).Return(nil) + c.persistentEvents.On("BatchStore", mock.Anything, mock.Anything, blockID, []flow.EventsList{indexerData.Events}).Return(nil) c.persistentCollections.On("BatchStoreAndIndexByTransaction", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, nil) c.persistentResults.On("BatchStore", mock.Anything, mock.Anything, blockID, indexerData.Results).Return(nil) c.persistentTxResultErrMsg.On("BatchStore", mock.Anything, mock.Anything, blockID, core.workingData.txResultErrMsgsData).Return(nil) From f07a652c04a3490031f67938c9a39af612cd7a72 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Wed, 8 Oct 2025 12:25:27 -0700 Subject: [PATCH 21/51] fix tests --- module/executiondatasync/optimistic_sync/core_impl_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/module/executiondatasync/optimistic_sync/core_impl_test.go b/module/executiondatasync/optimistic_sync/core_impl_test.go index e2998624e20..878d18f3487 100644 --- a/module/executiondatasync/optimistic_sync/core_impl_test.go +++ b/module/executiondatasync/optimistic_sync/core_impl_test.go @@ -445,11 +445,11 @@ func (c *CoreImplSuite) TestCoreImpl_Persist() { indexerData := core.workingData.indexerData c.persistentRegisters.On("Store", flow.RegisterEntries(indexerData.Registers), tf.block.Height).Return(nil) - c.persistentEvents.On("BatchStore", mock.Anything, mock.Anything, blockID, []flow.EventsList{indexerData.Events}).Return(nil) + c.persistentEvents.On("BatchStore", blockID, []flow.EventsList{indexerData.Events}, mock.Anything).Return(nil) c.persistentCollections.On("BatchStoreAndIndexByTransaction", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, nil) c.persistentResults.On("BatchStore", mock.Anything, mock.Anything, blockID, indexerData.Results).Return(nil) c.persistentTxResultErrMsg.On("BatchStore", mock.Anything, mock.Anything, blockID, core.workingData.txResultErrMsgsData).Return(nil) - c.latestPersistedSealedResult.On("BatchSet", mock.Anything, mock.Anything, tf.exeResult.ID(), tf.block.Height, mock.Anything).Return(nil) + c.latestPersistedSealedResult.On("BatchSet", tf.exeResult.ID(), tf.block.Height, mock.Anything).Return(nil) err = core.Persist() c.Require().NoError(err) From e8036e7812c60f93bf58c92170ee49de3f281294 Mon Sep 17 00:00:00 2001 From: Leo Zhang Date: Thu, 9 Oct 2025 20:22:34 -0700 Subject: [PATCH 22/51] Apply suggestions from code review Co-authored-by: Jordan Schalm --- .../access/ingestion/tx_error_messages/tx_error_messages_core.go | 1 - 1 file changed, 1 deletion(-) diff --git a/engine/access/ingestion/tx_error_messages/tx_error_messages_core.go b/engine/access/ingestion/tx_error_messages/tx_error_messages_core.go index 6b563cdfa96..7216fd91e4f 100644 --- a/engine/access/ingestion/tx_error_messages/tx_error_messages_core.go +++ b/engine/access/ingestion/tx_error_messages/tx_error_messages_core.go @@ -129,7 +129,6 @@ func (c *TxErrorMessagesCore) storeTransactionResultErrorMessages( } err := storage.WithLock(c.lockManager, storage.LockInsertTransactionResultErrMessage, func(lctx lockctx.Context) error { - // requires the [storage.LockInsertTransactionResultErrMessage] lock return c.transactionResultErrorMessages.Store(lctx, blockID, errorMessages) }) if err != nil { From 190c46de8fe625d26f46a18d6ca9a1c2bb02dd21 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Thu, 9 Oct 2025 20:28:01 -0700 Subject: [PATCH 23/51] rename operation functions --- storage/operation/transaction_results.go | 37 +++++++------------ storage/store/light_transaction_results.go | 2 +- .../transaction_result_error_messages.go | 2 +- 3 files changed, 15 insertions(+), 26 deletions(-) diff --git a/storage/operation/transaction_results.go b/storage/operation/transaction_results.go index c4f1675840e..5e118c8bc29 100644 --- a/storage/operation/transaction_results.go +++ b/storage/operation/transaction_results.go @@ -42,23 +42,12 @@ func LookupTransactionResultsByBlockIDUsingIndex(r storage.Reader, blockID flow. return TraverseByPrefix(r, MakePrefix(codeTransactionResultIndex, blockID), txErrIterFunc, storage.DefaultIteratorOptions()) } -// RemoveTransactionResultsByBlockID removes the transaction results for the given blockID -func RemoveTransactionResultsByBlockID(r storage.Reader, w storage.Writer, blockID flow.Identifier) error { - prefix := MakePrefix(codeTransactionResult, blockID) - err := RemoveByKeyPrefix(r, w, prefix) - if err != nil { - return fmt.Errorf("could not remove transaction results for block %v: %w", blockID, err) - } - - return nil -} - -// BatchRemoveTransactionResultsByBlockID removes transaction results for the given blockID in a provided batch. +// RemoveTransactionResultsByBlockID removes transaction results for the given blockID in a provided batch. // No errors are expected during normal operation, but it may return generic error // if badger fails to process request -func BatchRemoveTransactionResultsByBlockID(blockID flow.Identifier, batch storage.ReaderBatchWriter) error { +func RemoveTransactionResultsByBlockID(blockID flow.Identifier, rw storage.ReaderBatchWriter) error { prefix := MakePrefix(codeTransactionResult, blockID) - err := RemoveByKeyPrefix(batch.GlobalReader(), batch.Writer(), prefix) + err := RemoveByKeyPrefix(rw.GlobalReader(), rw.Writer(), prefix) if err != nil { return fmt.Errorf("could not remove transaction results for block %v: %w", blockID, err) } @@ -71,10 +60,10 @@ func InsertLightTransactionResult(w storage.Writer, blockID flow.Identifier, tra return UpsertByKey(w, MakePrefix(codeLightTransactionResult, blockID, transactionResult.TransactionID), transactionResult) } -// BatchInsertAndIndexLightTransactionResults inserts and indexes a batch of light transaction results +// InsertAndIndexLightTransactionResults inserts and indexes a batch of light transaction results // the caller must hold [storage.LockInsertLightTransactionResult] lock // It returns storage.ErrAlreadyExists if light transaction results for the block already exist -func BatchInsertAndIndexLightTransactionResults( +func InsertAndIndexLightTransactionResults( lctx lockctx.Proof, rw storage.ReaderBatchWriter, blockID flow.Identifier, transactionResults []flow.LightTransactionResult, @@ -95,12 +84,12 @@ func BatchInsertAndIndexLightTransactionResults( w := rw.Writer() for i, result := range transactionResults { - err := batchInsertLightTransactionResult(w, blockID, &result) + err := insertLightTransactionResult(w, blockID, &result) if err != nil { return fmt.Errorf("cannot batch insert light tx result: %w", err) } - err = batchIndexLightTransactionResult(w, blockID, uint32(i), &result) + err = indexLightTransactionResultByBlockIDAndTxIndex(w, blockID, uint32(i), &result) if err != nil { return fmt.Errorf("cannot batch index light tx result: %w", err) } @@ -108,15 +97,15 @@ func BatchInsertAndIndexLightTransactionResults( return nil } -// batchInsertLightTransactionResult inserts a light transaction result by block ID and transaction ID +// insertLightTransactionResult inserts a light transaction result by block ID and transaction ID // into the database using a batch write. -func batchInsertLightTransactionResult(w storage.Writer, blockID flow.Identifier, transactionResult *flow.LightTransactionResult) error { +func insertLightTransactionResult(w storage.Writer, blockID flow.Identifier, transactionResult *flow.LightTransactionResult) error { return UpsertByKey(w, MakePrefix(codeLightTransactionResult, blockID, transactionResult.TransactionID), transactionResult) } -// batchIndexLightTransactionResult indexes a light transaction result by index within the block using a +// indexLightTransactionResultByBlockIDAndTxIndex indexes a light transaction result by index within the block using a // batch write. -func batchIndexLightTransactionResult(w storage.Writer, blockID flow.Identifier, txIndex uint32, transactionResult *flow.LightTransactionResult) error { +func indexLightTransactionResultByBlockIDAndTxIndex(w storage.Writer, blockID flow.Identifier, txIndex uint32, transactionResult *flow.LightTransactionResult) error { return UpsertByKey(w, MakePrefix(codeLightTransactionResultIndex, blockID, txIndex), transactionResult) } @@ -145,10 +134,10 @@ func LookupLightTransactionResultsByBlockIDUsingIndex(r storage.Reader, blockID return TraverseByPrefix(r, MakePrefix(codeLightTransactionResultIndex, blockID), txErrIterFunc, storage.DefaultIteratorOptions()) } -// BatchInsertAndIndexTransactionResultErrorMessages inserts and indexes a batch of transaction result error messages +// InsertAndIndexTransactionResultErrorMessages inserts and indexes a batch of transaction result error messages // the caller must hold [storage.LockInsertTransactionResultErrMessage] lock // It returns storage.ErrAlreadyExists if tx result error messages for the block already exist -func BatchInsertAndIndexTransactionResultErrorMessages( +func InsertAndIndexTransactionResultErrorMessages( lctx lockctx.Proof, rw storage.ReaderBatchWriter, blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage, diff --git a/storage/store/light_transaction_results.go b/storage/store/light_transaction_results.go index fc708bf1f1f..7fcae27cbe4 100644 --- a/storage/store/light_transaction_results.go +++ b/storage/store/light_transaction_results.go @@ -73,7 +73,7 @@ func NewLightTransactionResults(collector module.CacheMetrics, db storage.DB, tr func (tr *LightTransactionResults) BatchStore(lctx lockctx.Proof, rw storage.ReaderBatchWriter, blockID flow.Identifier, transactionResults []flow.LightTransactionResult) error { // requires [storage.LockInsertLightTransactionResult] - err := operation.BatchInsertAndIndexLightTransactionResults(lctx, rw, blockID, transactionResults) + err := operation.InsertAndIndexLightTransactionResults(lctx, rw, blockID, transactionResults) if err != nil { return err } diff --git a/storage/store/transaction_result_error_messages.go b/storage/store/transaction_result_error_messages.go index 26fe43e94c8..931ff437525 100644 --- a/storage/store/transaction_result_error_messages.go +++ b/storage/store/transaction_result_error_messages.go @@ -111,7 +111,7 @@ func (t *TransactionResultErrorMessages) BatchStore( transactionResultErrorMessages []flow.TransactionResultErrorMessage, ) error { // requires [storage.LockInsertTransactionResultErrMessage] - err := operation.BatchInsertAndIndexTransactionResultErrorMessages(lctx, rw, blockID, transactionResultErrorMessages) + err := operation.InsertAndIndexTransactionResultErrorMessages(lctx, rw, blockID, transactionResultErrorMessages) if err != nil { return err } From 9bcdb069ab9e7a31bf124ea94c8cdee3ee947cf2 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Thu, 9 Oct 2025 20:33:18 -0700 Subject: [PATCH 24/51] fix Persist transaction error message --- .../optimistic_sync/persisters/stores/events.go | 10 ++++++---- .../stores/transaction_result_error_messages.go | 9 +++++---- storage/errors.go | 10 ---------- storage/store/transaction_result_error_messages.go | 2 +- 4 files changed, 12 insertions(+), 19 deletions(-) diff --git a/module/executiondatasync/optimistic_sync/persisters/stores/events.go b/module/executiondatasync/optimistic_sync/persisters/stores/events.go index d8aec9dc061..6792009534d 100644 --- a/module/executiondatasync/optimistic_sync/persisters/stores/events.go +++ b/module/executiondatasync/optimistic_sync/persisters/stores/events.go @@ -1,6 +1,7 @@ package stores import ( + "errors" "fmt" "github.com/jordanschalm/lockctx" @@ -34,11 +35,12 @@ func NewEventsStore( // // No error returns are expected during normal operations func (e *EventsStore) Persist(_ lockctx.Proof, batch storage.ReaderBatchWriter) error { - err := storage.SkipAlreadyExistsError( // Note: if the data already exists, we will not overwrite - e.persistedEvents.BatchStore(e.blockID, []flow.EventsList{e.data}, batch), - ) - + err := e.persistedEvents.BatchStore(e.blockID, []flow.EventsList{e.data}, batch) if err != nil { + if errors.Is(err, storage.ErrAlreadyExists) { + // we don't overwrite, it's ideompotent + return nil + } return fmt.Errorf("could not add events to batch: %w", err) } diff --git a/module/executiondatasync/optimistic_sync/persisters/stores/transaction_result_error_messages.go b/module/executiondatasync/optimistic_sync/persisters/stores/transaction_result_error_messages.go index 7541e436f4f..d3049a00088 100644 --- a/module/executiondatasync/optimistic_sync/persisters/stores/transaction_result_error_messages.go +++ b/module/executiondatasync/optimistic_sync/persisters/stores/transaction_result_error_messages.go @@ -1,6 +1,7 @@ package stores import ( + "errors" "fmt" "github.com/jordanschalm/lockctx" @@ -37,11 +38,11 @@ func NewTxResultErrMsgStore( // // No error returns are expected during normal operations func (t *TxResultErrMsgStore) Persist(lctx lockctx.Proof, rw storage.ReaderBatchWriter) error { - err := storage.SkipAlreadyExistsError( // Note: if the data already exists, we will not overwrite - storage.WithLock(t.lockManager, storage.LockInsertTransactionResultErrMessage, func(lctx lockctx.Context) error { - return t.persistedTxResultErrMsg.BatchStore(lctx, rw, t.blockID, t.data) - })) + err := t.persistedTxResultErrMsg.BatchStore(lctx, rw, t.blockID, t.data) if err != nil { + if errors.Is(err, storage.ErrAlreadyExists) { + return nil + } return fmt.Errorf("could not add transaction result error messages to batch: %w", err) } return nil diff --git a/storage/errors.go b/storage/errors.go index a31d1b6f74c..b3d81d9709c 100644 --- a/storage/errors.go +++ b/storage/errors.go @@ -57,13 +57,3 @@ func NewInvalidDKGStateTransitionErrorf(from, to flow.DKGState, msg string, args err: fmt.Errorf(msg, args...), } } - -// SkipAlreadyExistsError returns nil if the provided error is ErrAlreadyExists, otherwise returns the original error. -// It usually means the storage operation to insert a record was skipped because the key of the record already exists. -// CAUTION : it does NOT check the equality of the value of the record. -func SkipAlreadyExistsError(err error) error { - if errors.Is(err, ErrAlreadyExists) { - return nil - } - return err -} diff --git a/storage/store/transaction_result_error_messages.go b/storage/store/transaction_result_error_messages.go index 931ff437525..90dc0783d02 100644 --- a/storage/store/transaction_result_error_messages.go +++ b/storage/store/transaction_result_error_messages.go @@ -103,7 +103,7 @@ func (t *TransactionResultErrorMessages) Exists(blockID flow.Identifier) (bool, // BatchStore inserts a batch of transaction result error messages into a batch // the caller must hold [storage.LockInsertTransactionResultErrMessage] lock -// No errors are expected during normal operation. +// It returns [storage.ErrAlreadyExists] if tx result error messages for the block already exist func (t *TransactionResultErrorMessages) BatchStore( lctx lockctx.Proof, rw storage.ReaderBatchWriter, From 147f3cc477c1b6224116ce3521a2451df1c7c5d2 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Thu, 9 Oct 2025 20:33:59 -0700 Subject: [PATCH 25/51] Fix lint --- storage/store/transaction_results.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/storage/store/transaction_results.go b/storage/store/transaction_results.go index a870fe65ba8..6b7bea3e019 100644 --- a/storage/store/transaction_results.go +++ b/storage/store/transaction_results.go @@ -233,7 +233,7 @@ func (tr *TransactionResults) BatchRemoveByBlockID(blockID flow.Identifier, batc saveBlockIDInBatchData(batch, batchDataKey, blockID) - return operation.BatchRemoveTransactionResultsByBlockID(blockID, batch) + return operation.RemoveTransactionResultsByBlockID(blockID, batch) } func saveBlockIDInBatchData(batch storage.ReaderBatchWriter, batchDataKey string, blockID flow.Identifier) { From 9c361824fc000af2157b37cccbee6d63d4d2b140 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Thu, 9 Oct 2025 20:38:50 -0700 Subject: [PATCH 26/51] rename function names --- storage/operation/transaction_results.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/storage/operation/transaction_results.go b/storage/operation/transaction_results.go index 5e118c8bc29..a842d04d441 100644 --- a/storage/operation/transaction_results.go +++ b/storage/operation/transaction_results.go @@ -158,12 +158,12 @@ func InsertAndIndexTransactionResultErrorMessages( w := rw.Writer() for _, result := range transactionResultErrorMessages { - err := batchInsertTransactionResultErrorMessage(w, blockID, &result) + err := insertTransactionResultErrorMessageByTxID(w, blockID, &result) if err != nil { return fmt.Errorf("cannot batch insert tx result error message: %w", err) } - err = batchIndexTransactionResultErrorMessage(w, blockID, &result) + err = indexTransactionResultErrorMessageBlockIDTxIndex(w, blockID, &result) if err != nil { return fmt.Errorf("cannot batch index tx result error message: %w", err) } @@ -171,15 +171,15 @@ func InsertAndIndexTransactionResultErrorMessages( return nil } -// batchInsertTransactionResultErrorMessage inserts a transaction result error message by block ID and transaction ID +// insertTransactionResultErrorMessageByTxID inserts a transaction result error message by block ID and transaction ID // into the database using a batch write. -func batchInsertTransactionResultErrorMessage(w storage.Writer, blockID flow.Identifier, transactionResultErrorMessage *flow.TransactionResultErrorMessage) error { +func insertTransactionResultErrorMessageByTxID(w storage.Writer, blockID flow.Identifier, transactionResultErrorMessage *flow.TransactionResultErrorMessage) error { return UpsertByKey(w, MakePrefix(codeTransactionResultErrorMessage, blockID, transactionResultErrorMessage.TransactionID), transactionResultErrorMessage) } -// batchIndexTransactionResultErrorMessage indexes a transaction result error message by index within the block using a +// indexTransactionResultErrorMessageBlockIDTxIndex indexes a transaction result error message by index within the block using a // batch write. -func batchIndexTransactionResultErrorMessage(w storage.Writer, blockID flow.Identifier, transactionResultErrorMessage *flow.TransactionResultErrorMessage) error { +func indexTransactionResultErrorMessageBlockIDTxIndex(w storage.Writer, blockID flow.Identifier, transactionResultErrorMessage *flow.TransactionResultErrorMessage) error { return UpsertByKey(w, MakePrefix(codeTransactionResultErrorMessageIndex, blockID, transactionResultErrorMessage.Index), transactionResultErrorMessage) } From 3fc2a9afee747eb5d1a923e167d28ac442097ecb Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Thu, 9 Oct 2025 20:40:04 -0700 Subject: [PATCH 27/51] rename to txErrMsgStore --- .../transaction_result_error_messages_test.go | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/storage/store/transaction_result_error_messages_test.go b/storage/store/transaction_result_error_messages_test.go index 9f9b6b44a2c..81db35a3bbb 100644 --- a/storage/store/transaction_result_error_messages_test.go +++ b/storage/store/transaction_result_error_messages_test.go @@ -22,17 +22,17 @@ func TestStoringTransactionResultErrorMessages(t *testing.T) { dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { lockManager := storage.NewTestingLockManager() metrics := metrics.NewNoopCollector() - store1 := store.NewTransactionResultErrorMessages(metrics, db, 1000) + txErrMsgStore := store.NewTransactionResultErrorMessages(metrics, db, 1000) blockID := unittest.IdentifierFixture() // test db Exists by block id - exists, err := store1.Exists(blockID) + exists, err := txErrMsgStore.Exists(blockID) require.NoError(t, err) require.False(t, exists) // check retrieving by ByBlockID - messages, err := store1.ByBlockID(blockID) + messages, err := txErrMsgStore.ByBlockID(blockID) require.NoError(t, err) require.Nil(t, messages) @@ -47,31 +47,31 @@ func TestStoringTransactionResultErrorMessages(t *testing.T) { txErrorMessages = append(txErrorMessages, expected) } err = unittest.WithLock(t, lockManager, storage.LockInsertTransactionResultErrMessage, func(lctx lockctx.Context) error { - return store1.Store(lctx, blockID, txErrorMessages) + return txErrMsgStore.Store(lctx, blockID, txErrorMessages) }) require.NoError(t, err) // test db Exists by block id - exists, err = store1.Exists(blockID) + exists, err = txErrMsgStore.Exists(blockID) require.NoError(t, err) require.True(t, exists) // check retrieving by ByBlockIDTransactionID for _, txErrorMessage := range txErrorMessages { - actual, err := store1.ByBlockIDTransactionID(blockID, txErrorMessage.TransactionID) + actual, err := txErrMsgStore.ByBlockIDTransactionID(blockID, txErrorMessage.TransactionID) require.NoError(t, err) assert.Equal(t, txErrorMessage, *actual) } // check retrieving by ByBlockIDTransactionIndex for _, txErrorMessage := range txErrorMessages { - actual, err := store1.ByBlockIDTransactionIndex(blockID, txErrorMessage.Index) + actual, err := txErrMsgStore.ByBlockIDTransactionIndex(blockID, txErrorMessage.Index) require.NoError(t, err) assert.Equal(t, txErrorMessage, *actual) } // check retrieving by ByBlockID - actual, err := store1.ByBlockID(blockID) + actual, err := txErrMsgStore.ByBlockID(blockID) require.NoError(t, err) assert.Equal(t, txErrorMessages, actual) @@ -85,7 +85,7 @@ func TestStoringTransactionResultErrorMessages(t *testing.T) { // check retrieving by index from both cache and db for i, txErrorMessage := range txErrorMessages { - actual, err := store1.ByBlockIDTransactionIndex(blockID, txErrorMessage.Index) + actual, err := txErrMsgStore.ByBlockIDTransactionIndex(blockID, txErrorMessage.Index) require.NoError(t, err) assert.Equal(t, txErrorMessages[i], *actual) @@ -99,16 +99,16 @@ func TestStoringTransactionResultErrorMessages(t *testing.T) { func TestReadingNotStoreTransactionResultErrorMessage(t *testing.T) { dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { metrics := metrics.NewNoopCollector() - store1 := store.NewTransactionResultErrorMessages(metrics, db, 1000) + txErrMsgStore := store.NewTransactionResultErrorMessages(metrics, db, 1000) blockID := unittest.IdentifierFixture() txID := unittest.IdentifierFixture() txIndex := rand.Uint32() - _, err := store1.ByBlockIDTransactionID(blockID, txID) + _, err := txErrMsgStore.ByBlockIDTransactionID(blockID, txID) assert.ErrorIs(t, err, storage.ErrNotFound) - _, err = store1.ByBlockIDTransactionIndex(blockID, txIndex) + _, err = txErrMsgStore.ByBlockIDTransactionIndex(blockID, txIndex) assert.ErrorIs(t, err, storage.ErrNotFound) }) } From dd6cfa21e6114eaad98d607aae1dc0b1cedaa313 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Fri, 10 Oct 2025 09:27:42 -0700 Subject: [PATCH 28/51] update lock policy --- module/executiondatasync/optimistic_sync/persisters/block.go | 1 + storage/locks.go | 1 + 2 files changed, 2 insertions(+) diff --git a/module/executiondatasync/optimistic_sync/persisters/block.go b/module/executiondatasync/optimistic_sync/persisters/block.go index ade61ec8c4d..76262611a1f 100644 --- a/module/executiondatasync/optimistic_sync/persisters/block.go +++ b/module/executiondatasync/optimistic_sync/persisters/block.go @@ -63,6 +63,7 @@ func (p *BlockPersister) Persist() error { err := storage.WithLocks(p.lockManager, []string{ storage.LockInsertCollection, storage.LockInsertLightTransactionResult, + storage.LockInsertTransactionResultErrMessage, }, func(lctx lockctx.Context) error { return p.protocolDB.WithReaderBatchWriter(func(batch storage.ReaderBatchWriter) error { for _, persister := range p.persisterStores { diff --git a/storage/locks.go b/storage/locks.go index 9505389f790..d2d7ab036da 100644 --- a/storage/locks.go +++ b/storage/locks.go @@ -85,6 +85,7 @@ func makeLockPolicy() lockctx.Policy { // module/executiondatasync/optimistic_sync/persisters/block.go#Persist Add(LockInsertCollection, LockInsertLightTransactionResult). + Add(LockInsertLightTransactionResult, LockInsertTransactionResultErrMessage). Build() } From 780e20a5cc87d0e2dff0fbff00d3e818620a0aac Mon Sep 17 00:00:00 2001 From: Alex Hentschel Date: Fri, 10 Oct 2025 16:55:19 -0700 Subject: [PATCH 29/51] extended documentation, especially error returns --- storage/operation/transaction_results.go | 40 +++++++++++++++++------- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/storage/operation/transaction_results.go b/storage/operation/transaction_results.go index a842d04d441..bb1a9862524 100644 --- a/storage/operation/transaction_results.go +++ b/storage/operation/transaction_results.go @@ -28,7 +28,6 @@ func RetrieveTransactionResultByIndex(r storage.Reader, blockID flow.Identifier, // LookupTransactionResultsByBlockIDUsingIndex retrieves all tx results for a block, by using // tx_index index. This correctly handles cases of duplicate transactions within block. func LookupTransactionResultsByBlockIDUsingIndex(r storage.Reader, blockID flow.Identifier, txResults *[]flow.TransactionResult) error { - txErrIterFunc := func(keyCopy []byte, getValue func(destVal any) error) (bail bool, err error) { var val flow.TransactionResult err = getValue(&val) @@ -60,9 +59,10 @@ func InsertLightTransactionResult(w storage.Writer, blockID flow.Identifier, tra return UpsertByKey(w, MakePrefix(codeLightTransactionResult, blockID, transactionResult.TransactionID), transactionResult) } -// InsertAndIndexLightTransactionResults inserts and indexes a batch of light transaction results -// the caller must hold [storage.LockInsertLightTransactionResult] lock -// It returns storage.ErrAlreadyExists if light transaction results for the block already exist +// InsertAndIndexLightTransactionResults persists and indexes all transaction results (light representation) for the given blockID +// as part of the provided batch. The caller must acquire [storage.LockInsertLightTransactionResult] and hold it until the write +// batch has been committed. +// It returns [storage.ErrAlreadyExists] if light transaction results for the block already exist. func InsertAndIndexLightTransactionResults( lctx lockctx.Proof, rw storage.ReaderBatchWriter, blockID flow.Identifier, @@ -109,10 +109,18 @@ func indexLightTransactionResultByBlockIDAndTxIndex(w storage.Writer, blockID fl return UpsertByKey(w, MakePrefix(codeLightTransactionResultIndex, blockID, txIndex), transactionResult) } +// RetrieveLightTransactionResult retrieves the result (light representation) of the specified transaction +// within the specified block. +// Expected error returns during normal operations: +// - [storage.ErrNotFound] if no result of a transaction with the specified ID is known func RetrieveLightTransactionResult(r storage.Reader, blockID flow.Identifier, transactionID flow.Identifier, transactionResult *flow.LightTransactionResult) error { return RetrieveByKey(r, MakePrefix(codeLightTransactionResult, blockID, transactionID), transactionResult) } +// RetrieveLightTransactionResultByIndex retrieves the result (light representation) of the +// transaction at the given index within the specified block. +// Expected error returns during normal operations: +// - [storage.ErrNotFound] if no result of a transaction at `txIndex` in `blockID` is known func RetrieveLightTransactionResultByIndex(r storage.Reader, blockID flow.Identifier, txIndex uint32, transactionResult *flow.LightTransactionResult) error { return RetrieveByKey(r, MakePrefix(codeLightTransactionResultIndex, blockID, txIndex), transactionResult) } @@ -134,9 +142,10 @@ func LookupLightTransactionResultsByBlockIDUsingIndex(r storage.Reader, blockID return TraverseByPrefix(r, MakePrefix(codeLightTransactionResultIndex, blockID), txErrIterFunc, storage.DefaultIteratorOptions()) } -// InsertAndIndexTransactionResultErrorMessages inserts and indexes a batch of transaction result error messages -// the caller must hold [storage.LockInsertTransactionResultErrMessage] lock -// It returns storage.ErrAlreadyExists if tx result error messages for the block already exist +// InsertAndIndexTransactionResultErrorMessages persists and indexes all transaction result error messages for the given blockID +// as part of the provided batch. The caller must acquire [storage.LockInsertTransactionResultErrMessage] and hold it until the +// write batch has been committed. +// It returns [storage.ErrAlreadyExists] if tx result error messages for the block already exist func InsertAndIndexTransactionResultErrorMessages( lctx lockctx.Proof, rw storage.ReaderBatchWriter, blockID flow.Identifier, @@ -183,17 +192,24 @@ func indexTransactionResultErrorMessageBlockIDTxIndex(w storage.Writer, blockID return UpsertByKey(w, MakePrefix(codeTransactionResultErrorMessageIndex, blockID, transactionResultErrorMessage.Index), transactionResultErrorMessage) } -// RetrieveTransactionResultErrorMessage retrieves a transaction result error message by block ID and transaction ID. +// RetrieveTransactionResultErrorMessage retrieves a transaction result error message of the specified transaction +// within the specified block. +// Expected error returns during normal operations: +// - [storage.ErrNotFound] if no result of a transaction with the specified ID is known func RetrieveTransactionResultErrorMessage(r storage.Reader, blockID flow.Identifier, transactionID flow.Identifier, transactionResultErrorMessage *flow.TransactionResultErrorMessage) error { return RetrieveByKey(r, MakePrefix(codeTransactionResultErrorMessage, blockID, transactionID), transactionResultErrorMessage) } -// RetrieveTransactionResultErrorMessageByIndex retrieves a transaction result error message by block ID and index. +// RetrieveTransactionResultErrorMessageByIndex retrieves the transaction result error message of the +// transaction at the given index within the specified block. +// Expected error returns during normal operations: +// - [storage.ErrNotFound] if no result of a transaction at `txIndex` in `blockID` is known func RetrieveTransactionResultErrorMessageByIndex(r storage.Reader, blockID flow.Identifier, txIndex uint32, transactionResultErrorMessage *flow.TransactionResultErrorMessage) error { return RetrieveByKey(r, MakePrefix(codeTransactionResultErrorMessageIndex, blockID, txIndex), transactionResultErrorMessage) } // TransactionResultErrorMessagesExists checks whether tx result error messages exist in the database. +// No error returns are expected during normal operations. func TransactionResultErrorMessagesExists(r storage.Reader, blockID flow.Identifier, blockExists *bool) error { exists, err := KeyExists(r, MakePrefix(codeTransactionResultErrorMessageIndex, blockID)) if err != nil { @@ -203,8 +219,10 @@ func TransactionResultErrorMessagesExists(r storage.Reader, blockID flow.Identif return nil } -// LookupTransactionResultErrorMessagesByBlockIDUsingIndex retrieves all tx result error messages for a block, by using -// tx_index index. This correctly handles cases of duplicate transactions within block. +// LookupTransactionResultErrorMessagesByBlockIDUsingIndex retrieves the transaction result error messages of all +// failed transactions for the specified block. +// CAUTION: this function returns the empty list in case for block IDs without known results. +// No error returns are expected during normal operations. func LookupTransactionResultErrorMessagesByBlockIDUsingIndex(r storage.Reader, blockID flow.Identifier, txResultErrorMessages *[]flow.TransactionResultErrorMessage) error { txErrIterFunc := func(keyCopy []byte, getValue func(destVal any) error) (bail bool, err error) { var val flow.TransactionResultErrorMessage From 4e11ecf9f116cbc36bc9f86f07a895cdd503950e Mon Sep 17 00:00:00 2001 From: Alex Hentschel Date: Fri, 10 Oct 2025 17:14:01 -0700 Subject: [PATCH 30/51] extended documentation, especially error returns --- storage/light_transaction_results.go | 19 ++++++++++--------- storage/operation/transaction_results.go | 6 +++--- storage/store/light_transaction_results.go | 7 +++++++ 3 files changed, 20 insertions(+), 12 deletions(-) diff --git a/storage/light_transaction_results.go b/storage/light_transaction_results.go index 60e2cffc41d..69083e1b0d5 100644 --- a/storage/light_transaction_results.go +++ b/storage/light_transaction_results.go @@ -10,20 +10,20 @@ import ( type LightTransactionResultsReader interface { // ByBlockIDTransactionID returns the transaction result for the given block ID and transaction ID // - // Expected errors during normal operation: - // - `storage.ErrNotFound` if light transaction result at given blockID wasn't found. + // Expected error returns during normal operation: + // - [storage.ErrNotFound] if light transaction result at given blockID wasn't found. ByBlockIDTransactionID(blockID flow.Identifier, transactionID flow.Identifier) (*flow.LightTransactionResult, error) // ByBlockIDTransactionIndex returns the transaction result for the given blockID and transaction index // - // Expected errors during normal operation: - // - `storage.ErrNotFound` if light transaction result at given blockID and txIndex wasn't found. + // Expected error returns during normal operation: + // - [storage.ErrNotFound] if light transaction result at given blockID and txIndex wasn't found. ByBlockIDTransactionIndex(blockID flow.Identifier, txIndex uint32) (*flow.LightTransactionResult, error) // ByBlockID gets all transaction results for a block, ordered by transaction index // - // Expected errors during normal operation: - // - `storage.ErrNotFound` if light transaction results at given blockID weren't found. + // Expected error returns during normal operation: + // - [storage.ErrNotFound] if light transaction results at given blockID weren't found. ByBlockID(id flow.Identifier) ([]flow.LightTransactionResult, error) } @@ -31,9 +31,10 @@ type LightTransactionResultsReader interface { type LightTransactionResults interface { LightTransactionResultsReader - // BatchStore inserts a batch of transaction result into a batch - // It returns [ErrAlreadyExists] if light transaction results for the block already exist. - // It requires the caller to hold [storage.LockInsertLightTransactionResult] + // BatchStore persists and indexes all transaction results (light representation) for the given blockID + // as part of the provided batch. The caller must acquire [storage.LockInsertLightTransactionResult] and + // hold it until the write batch has been committed. + // It returns [storage.ErrAlreadyExists] if light transaction results for the block already exist. BatchStore(lctx lockctx.Proof, rw ReaderBatchWriter, blockID flow.Identifier, transactionResults []flow.LightTransactionResult) error // Deprecated: deprecated as a part of transition from Badger to Pebble. use BatchStore instead diff --git a/storage/operation/transaction_results.go b/storage/operation/transaction_results.go index bb1a9862524..97035a2d7e2 100644 --- a/storage/operation/transaction_results.go +++ b/storage/operation/transaction_results.go @@ -125,10 +125,10 @@ func RetrieveLightTransactionResultByIndex(r storage.Reader, blockID flow.Identi return RetrieveByKey(r, MakePrefix(codeLightTransactionResultIndex, blockID, txIndex), transactionResult) } -// LookupLightTransactionResultsByBlockIDUsingIndex retrieves all tx results for a block, but using -// tx_index index. This correctly handles cases of duplicate transactions within block. +// LookupLightTransactionResultsByBlockIDUsingIndex retrieves all tx results for the specified block. +// CAUTION: this function returns the empty list in case for block IDs without known results. +// No error returns are expected during normal operations. func LookupLightTransactionResultsByBlockIDUsingIndex(r storage.Reader, blockID flow.Identifier, txResults *[]flow.LightTransactionResult) error { - txErrIterFunc := func(keyCopy []byte, getValue func(destVal any) error) (bail bool, err error) { var val flow.LightTransactionResult err = getValue(&val) diff --git a/storage/store/light_transaction_results.go b/storage/store/light_transaction_results.go index 7fcae27cbe4..53935d56318 100644 --- a/storage/store/light_transaction_results.go +++ b/storage/store/light_transaction_results.go @@ -71,6 +71,10 @@ func NewLightTransactionResults(collector module.CacheMetrics, db storage.DB, tr } } +// BatchStore persists and indexes all transaction results (light representation) for the given blockID +// as part of the provided batch. The caller must acquire [storage.LockInsertLightTransactionResult] and +// hold it until the write batch has been committed. +// It returns [storage.ErrAlreadyExists] if light transaction results for the block already exist. func (tr *LightTransactionResults) BatchStore(lctx lockctx.Proof, rw storage.ReaderBatchWriter, blockID flow.Identifier, transactionResults []flow.LightTransactionResult) error { // requires [storage.LockInsertLightTransactionResult] err := operation.InsertAndIndexLightTransactionResults(lctx, rw, blockID, transactionResults) @@ -120,6 +124,9 @@ func (tr *LightTransactionResults) ByBlockIDTransactionIndex(blockID flow.Identi } // ByBlockID gets all transaction results for a block, ordered by transaction index +// +// Expected error returns during normal operation: +// - [storage.ErrNotFound] if light transaction results at given blockID weren't found. func (tr *LightTransactionResults) ByBlockID(blockID flow.Identifier) ([]flow.LightTransactionResult, error) { transactionResults, err := tr.blockCache.Get(tr.db.Reader(), blockID) if err != nil { From cafb925bd1d8adaa0c7c68824cbd83fb60aa847b Mon Sep 17 00:00:00 2001 From: Alex Hentschel Date: Fri, 10 Oct 2025 17:22:30 -0700 Subject: [PATCH 31/51] added test --- storage/operation/transaction_results_test.go | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 storage/operation/transaction_results_test.go diff --git a/storage/operation/transaction_results_test.go b/storage/operation/transaction_results_test.go new file mode 100644 index 00000000000..bdd075570a4 --- /dev/null +++ b/storage/operation/transaction_results_test.go @@ -0,0 +1,30 @@ +package operation_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/onflow/flow-go/model/flow" + "github.com/onflow/flow-go/storage" + "github.com/onflow/flow-go/storage/operation" + "github.com/onflow/flow-go/storage/operation/dbtest" + "github.com/onflow/flow-go/utils/unittest" +) + +// TestRetrieveAllTxResultsForBlock verifies the working of persisting, indexing and retrieving +// [flow.LightTransactionResult] by block, transaction ID, and transaction index. +func TestRetrieveAllTxResultsForBlock(t *testing.T) { + t.Run("looking up transaction results for unknown block yields empty list", func(t *testing.T) { + dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { + unknownBlockID := unittest.IdentifierFixture() + transactionResults := make([]flow.LightTransactionResult, 0) + + err := db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return operation.LookupLightTransactionResultsByBlockIDUsingIndex(rw.GlobalReader(), unknownBlockID, &transactionResults) + }) + require.NoError(t, err) + require.Empty(t, transactionResults) + }) + }) +} From ac316dd4e056fb4f864d6327b098e6c8c53af14c Mon Sep 17 00:00:00 2001 From: Alex Hentschel Date: Fri, 10 Oct 2025 17:45:24 -0700 Subject: [PATCH 32/51] more error documentation --- storage/operation/transaction_results.go | 2 +- storage/store/light_transaction_results.go | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/storage/operation/transaction_results.go b/storage/operation/transaction_results.go index 97035a2d7e2..261d5c4ea68 100644 --- a/storage/operation/transaction_results.go +++ b/storage/operation/transaction_results.go @@ -221,7 +221,7 @@ func TransactionResultErrorMessagesExists(r storage.Reader, blockID flow.Identif // LookupTransactionResultErrorMessagesByBlockIDUsingIndex retrieves the transaction result error messages of all // failed transactions for the specified block. -// CAUTION: this function returns the empty list in case for block IDs without known results. +// CAUTION: This method returns an empty slice if transaction results/errors for the block are not indexed yet OR if the block does not have any errors. // No error returns are expected during normal operations. func LookupTransactionResultErrorMessagesByBlockIDUsingIndex(r storage.Reader, blockID flow.Identifier, txResultErrorMessages *[]flow.TransactionResultErrorMessage) error { txErrIterFunc := func(keyCopy []byte, getValue func(destVal any) error) (bail bool, err error) { diff --git a/storage/store/light_transaction_results.go b/storage/store/light_transaction_results.go index 53935d56318..c0cec690b00 100644 --- a/storage/store/light_transaction_results.go +++ b/storage/store/light_transaction_results.go @@ -104,6 +104,9 @@ func (tr *LightTransactionResults) BatchStoreBadger(blockID flow.Identifier, tra } // ByBlockIDTransactionID returns the transaction result for the given block ID and transaction ID +// +// Expected error returns during normal operation: +// - [storage.ErrNotFound] if light transaction result at given blockID wasn't found. func (tr *LightTransactionResults) ByBlockIDTransactionID(blockID flow.Identifier, txID flow.Identifier) (*flow.LightTransactionResult, error) { key := KeyFromBlockIDTransactionID(blockID, txID) transactionResult, err := tr.cache.Get(tr.db.Reader(), key) @@ -114,6 +117,9 @@ func (tr *LightTransactionResults) ByBlockIDTransactionID(blockID flow.Identifie } // ByBlockIDTransactionIndex returns the transaction result for the given blockID and transaction index +// +// Expected error returns during normal operation: +// - [storage.ErrNotFound] if light transaction result at given blockID and txIndex wasn't found. func (tr *LightTransactionResults) ByBlockIDTransactionIndex(blockID flow.Identifier, txIndex uint32) (*flow.LightTransactionResult, error) { key := KeyFromBlockIDIndex(blockID, txIndex) transactionResult, err := tr.indexCache.Get(tr.db.Reader(), key) From 64dfc0917aec83f311b422cc01b7274afef4ac19 Mon Sep 17 00:00:00 2001 From: Alex Hentschel Date: Fri, 10 Oct 2025 17:48:54 -0700 Subject: [PATCH 33/51] polishing doc --- storage/operation/transaction_results.go | 2 +- storage/transaction_result_error_messages.go | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/storage/operation/transaction_results.go b/storage/operation/transaction_results.go index 261d5c4ea68..25c7bc304e1 100644 --- a/storage/operation/transaction_results.go +++ b/storage/operation/transaction_results.go @@ -145,7 +145,7 @@ func LookupLightTransactionResultsByBlockIDUsingIndex(r storage.Reader, blockID // InsertAndIndexTransactionResultErrorMessages persists and indexes all transaction result error messages for the given blockID // as part of the provided batch. The caller must acquire [storage.LockInsertTransactionResultErrMessage] and hold it until the // write batch has been committed. -// It returns [storage.ErrAlreadyExists] if tx result error messages for the block already exist +// It returns [storage.ErrAlreadyExists] if tx result error messages for the block already exist. func InsertAndIndexTransactionResultErrorMessages( lctx lockctx.Proof, rw storage.ReaderBatchWriter, blockID flow.Identifier, diff --git a/storage/transaction_result_error_messages.go b/storage/transaction_result_error_messages.go index 9e52b2e0f33..6cc59039509 100644 --- a/storage/transaction_result_error_messages.go +++ b/storage/transaction_result_error_messages.go @@ -16,13 +16,13 @@ type TransactionResultErrorMessagesReader interface { // ByBlockIDTransactionID returns the transaction result error message for the given block ID and transaction ID. // // Expected errors during normal operation: - // - `storage.ErrNotFound` if no transaction error message is known at given block and transaction id. + // - [storage.ErrNotFound] if no transaction error message is known at given block and transaction id. ByBlockIDTransactionID(blockID flow.Identifier, transactionID flow.Identifier) (*flow.TransactionResultErrorMessage, error) // ByBlockIDTransactionIndex returns the transaction result error message for the given blockID and transaction index. // // Expected errors during normal operation: - // - `storage.ErrNotFound` if no transaction error message is known at given block and transaction index. + // - [storage.ErrNotFound] if no transaction error message is known at given block and transaction index. ByBlockIDTransactionIndex(blockID flow.Identifier, txIndex uint32) (*flow.TransactionResultErrorMessage, error) // ByBlockID gets all transaction result error messages for a block, ordered by transaction index. @@ -36,13 +36,13 @@ type TransactionResultErrorMessagesReader interface { type TransactionResultErrorMessages interface { TransactionResultErrorMessagesReader - // Store will store transaction result error messages for the given block ID. - // It requires the caller to hold [storage.LockInsertTransactionResultErrMessage] + // Store will store transaction result error messages for the given block ID. The caller must acquire + // [storage.LockInsertTransactionResultErrMessage] and hold it until the write batch has been committed. // It returns [ErrAlreadyExists] if transaction result error messages for the block already exist. Store(lctx lockctx.Proof, blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage) error - // BatchStore inserts a batch of transaction result error messages into a batch - // It requires the caller to hold [storage.LockInsertTransactionResultErrMessage] + // BatchStore inserts a batch of transaction result error messages into a batch. The caller must acquire + // [storage.LockInsertTransactionResultErrMessage] and hold it until the write batch has been committed. // It returns [ErrAlreadyExists] if transaction result error messages for the block already exist. BatchStore(lctx lockctx.Proof, rw ReaderBatchWriter, blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage) error } From 4ac5c285d87c25dc0fac07b3c490eb049471491e Mon Sep 17 00:00:00 2001 From: Alex Hentschel Date: Fri, 10 Oct 2025 17:55:14 -0700 Subject: [PATCH 34/51] added minor documentation and additional error check to test `storage/store/transaction_result_error_messages_test.go` --- .../store/transaction_result_error_messages_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/storage/store/transaction_result_error_messages_test.go b/storage/store/transaction_result_error_messages_test.go index 81db35a3bbb..5a779ec6ab5 100644 --- a/storage/store/transaction_result_error_messages_test.go +++ b/storage/store/transaction_result_error_messages_test.go @@ -113,6 +113,8 @@ func TestReadingNotStoreTransactionResultErrorMessage(t *testing.T) { }) } +// Test that attempting to batch store transaction result error messages for a block ID that already exists +// results in a [storage.ErrAlreadyExists] error, and that the original messages remain unchanged. func TestBatchStoreTransactionResultErrorMessagesErrAlreadyExists(t *testing.T) { lockManager := storage.NewTestingLockManager() dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { @@ -175,6 +177,9 @@ func TestBatchStoreTransactionResultErrorMessagesErrAlreadyExists(t *testing.T) }) } +// Test that attempting to batch store transaction result error messages without holding the required lock +// results in an error indicating the missing lock. The implementation should not conflate this error +// case with data for the same key already existing, ie. it should not return [storage.ErrAlreadyExists]. func TestBatchStoreTransactionResultErrorMessagesMissingLock(t *testing.T) { dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { metrics := metrics.NewNoopCollector() @@ -201,10 +206,14 @@ func TestBatchStoreTransactionResultErrorMessagesMissingLock(t *testing.T) { return st.BatchStore(lctx, rw, blockID, txResultErrMsgs) }) require.Error(t, err) + require.NotErrorIs(t, err, storage.ErrAlreadyExists) require.Contains(t, err.Error(), "lock_insert_transaction_result_message") }) } +// Test that attempting to batch store transaction result error messages while holding the wrong lock +// results in an error indicating the incorrect lock. The implementation should not conflate this error +// case with data for the same key already existing, ie. it should not return [storage.ErrAlreadyExists]. func TestBatchStoreTransactionResultErrorMessagesWrongLock(t *testing.T) { lockManager := storage.NewTestingLockManager() dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { @@ -230,6 +239,7 @@ func TestBatchStoreTransactionResultErrorMessagesWrongLock(t *testing.T) { }) }) require.Error(t, err) + require.NotErrorIs(t, err, storage.ErrAlreadyExists) require.Contains(t, err.Error(), "lock_insert_transaction_result_message") }) } From ac296c140856251c5bd26aabc58575bd99353b31 Mon Sep 17 00:00:00 2001 From: Alex Hentschel Date: Fri, 10 Oct 2025 18:00:40 -0700 Subject: [PATCH 35/51] polishing goDoc --- .../store/transaction_result_error_messages.go | 15 +++++++-------- storage/transaction_result_error_messages.go | 13 +++++++------ 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/storage/store/transaction_result_error_messages.go b/storage/store/transaction_result_error_messages.go index 90dc0783d02..e8238a6cc44 100644 --- a/storage/store/transaction_result_error_messages.go +++ b/storage/store/transaction_result_error_messages.go @@ -73,10 +73,9 @@ func NewTransactionResultErrorMessages(collector module.CacheMetrics, db storage } } -// Store will store transaction result error messages for the given block ID. -// -// the caller must hold [storage.LockInsertTransactionResultErrMessage] lock -// No errors are expected during normal operation. +// Store persists and indexes all transaction result error messages for the given blockID. The caller must +// acquire [storage.LockInsertTransactionResultErrMessage] and hold it until the write batch has been committed. +// It returns [storage.ErrAlreadyExists] if tx result error messages for the block already exist. func (t *TransactionResultErrorMessages) Store(lctx lockctx.Proof, blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage) error { return t.db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { return t.BatchStore(lctx, rw, blockID, transactionResultErrorMessages) @@ -84,7 +83,6 @@ func (t *TransactionResultErrorMessages) Store(lctx lockctx.Proof, blockID flow. } // Exists returns true if transaction result error messages for the given ID have been stored. -// // No errors are expected during normal operation. func (t *TransactionResultErrorMessages) Exists(blockID flow.Identifier) (bool, error) { // if the block is in the cache, return true @@ -101,9 +99,10 @@ func (t *TransactionResultErrorMessages) Exists(blockID flow.Identifier) (bool, return exists, nil } -// BatchStore inserts a batch of transaction result error messages into a batch -// the caller must hold [storage.LockInsertTransactionResultErrMessage] lock -// It returns [storage.ErrAlreadyExists] if tx result error messages for the block already exist +// BatchStore persists and indexes all transaction result error messages for the given blockID as part +// of the provided batch. The caller must acquire [storage.LockInsertTransactionResultErrMessage] and +// hold it until the write batch has been committed. +// It returns [storage.ErrAlreadyExists] if tx result error messages for the block already exist. func (t *TransactionResultErrorMessages) BatchStore( lctx lockctx.Proof, rw storage.ReaderBatchWriter, diff --git a/storage/transaction_result_error_messages.go b/storage/transaction_result_error_messages.go index 6cc59039509..c3559be1790 100644 --- a/storage/transaction_result_error_messages.go +++ b/storage/transaction_result_error_messages.go @@ -36,13 +36,14 @@ type TransactionResultErrorMessagesReader interface { type TransactionResultErrorMessages interface { TransactionResultErrorMessagesReader - // Store will store transaction result error messages for the given block ID. The caller must acquire - // [storage.LockInsertTransactionResultErrMessage] and hold it until the write batch has been committed. - // It returns [ErrAlreadyExists] if transaction result error messages for the block already exist. + // Store persists and indexes all transaction result error messages for the given blockID. The caller must + // acquire [storage.LockInsertTransactionResultErrMessage] and hold it until the write batch has been committed. + // It returns [storage.ErrAlreadyExists] if tx result error messages for the block already exist. Store(lctx lockctx.Proof, blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage) error - // BatchStore inserts a batch of transaction result error messages into a batch. The caller must acquire - // [storage.LockInsertTransactionResultErrMessage] and hold it until the write batch has been committed. - // It returns [ErrAlreadyExists] if transaction result error messages for the block already exist. + // BatchStore persists and indexes all transaction result error messages for the given blockID as part + // of the provided batch. The caller must acquire [storage.LockInsertTransactionResultErrMessage] and + // hold it until the write batch has been committed. + // It returns [storage.ErrAlreadyExists] if tx result error messages for the block already exist. BatchStore(lctx lockctx.Proof, rw ReaderBatchWriter, blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage) error } From 9d0cbdb70b1a65838f179928892c48def4947394 Mon Sep 17 00:00:00 2001 From: Alex Hentschel Date: Fri, 10 Oct 2025 18:10:13 -0700 Subject: [PATCH 36/51] added deprecation notice --- storage/store/light_transaction_results.go | 1 + 1 file changed, 1 insertion(+) diff --git a/storage/store/light_transaction_results.go b/storage/store/light_transaction_results.go index c0cec690b00..789f185af5a 100644 --- a/storage/store/light_transaction_results.go +++ b/storage/store/light_transaction_results.go @@ -99,6 +99,7 @@ func (tr *LightTransactionResults) BatchStore(lctx lockctx.Proof, rw storage.Rea return nil } +// Deprecated: panics func (tr *LightTransactionResults) BatchStoreBadger(blockID flow.Identifier, transactionResults []flow.LightTransactionResult, batch storage.BatchStorage) error { panic("LightTransactionResults BatchStoreBadger not implemented") } From 51a1895c631c1407adeb665a74186947ddf3eb10 Mon Sep 17 00:00:00 2001 From: Alex Hentschel Date: Fri, 10 Oct 2025 18:43:22 -0700 Subject: [PATCH 37/51] polished goDoc --- .../persisters/stores/transaction_result_error_messages.go | 3 +++ module/state_synchronization/indexer/indexer_core.go | 4 ++-- storage/operation/transaction_results.go | 4 ++-- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/module/executiondatasync/optimistic_sync/persisters/stores/transaction_result_error_messages.go b/module/executiondatasync/optimistic_sync/persisters/stores/transaction_result_error_messages.go index d3049a00088..1c084e75f0a 100644 --- a/module/executiondatasync/optimistic_sync/persisters/stores/transaction_result_error_messages.go +++ b/module/executiondatasync/optimistic_sync/persisters/stores/transaction_result_error_messages.go @@ -38,6 +38,9 @@ func NewTxResultErrMsgStore( // // No error returns are expected during normal operations func (t *TxResultErrMsgStore) Persist(lctx lockctx.Proof, rw storage.ReaderBatchWriter) error { + // CAUTION: here we assume that if something is already stored for our blockID, then the data is identical. + // This only holds true for sealed execution results, whose consistency has previously been verified by + // comparing the data's hash to commitments in the execution result. err := t.persistedTxResultErrMsg.BatchStore(lctx, rw, t.blockID, t.data) if err != nil { if errors.Is(err, storage.ErrAlreadyExists) { diff --git a/module/state_synchronization/indexer/indexer_core.go b/module/state_synchronization/indexer/indexer_core.go index 04c8119d49f..abe31eec3be 100644 --- a/module/state_synchronization/indexer/indexer_core.go +++ b/module/state_synchronization/indexer/indexer_core.go @@ -106,8 +106,8 @@ func (c *IndexerCore) RegisterValue(ID flow.RegisterID, height uint64) (flow.Reg // IndexBlockData indexes all execution block data by height. // This method shouldn't be used concurrently. -// Expected errors: -// - storage.ErrNotFound if the block for execution data was not found +// Expected error returns during normal operations: +// - [storage.ErrNotFound] if the block for execution data was not found func (c *IndexerCore) IndexBlockData(data *execution_data.BlockExecutionDataEntity) error { header, err := c.headers.ByBlockID(data.BlockID) if err != nil { diff --git a/storage/operation/transaction_results.go b/storage/operation/transaction_results.go index 25c7bc304e1..19b0a79a298 100644 --- a/storage/operation/transaction_results.go +++ b/storage/operation/transaction_results.go @@ -112,7 +112,7 @@ func indexLightTransactionResultByBlockIDAndTxIndex(w storage.Writer, blockID fl // RetrieveLightTransactionResult retrieves the result (light representation) of the specified transaction // within the specified block. // Expected error returns during normal operations: -// - [storage.ErrNotFound] if no result of a transaction with the specified ID is known +// - [storage.ErrNotFound] if no result of a transaction with the specified ID in `blockID` is known func RetrieveLightTransactionResult(r storage.Reader, blockID flow.Identifier, transactionID flow.Identifier, transactionResult *flow.LightTransactionResult) error { return RetrieveByKey(r, MakePrefix(codeLightTransactionResult, blockID, transactionID), transactionResult) } @@ -195,7 +195,7 @@ func indexTransactionResultErrorMessageBlockIDTxIndex(w storage.Writer, blockID // RetrieveTransactionResultErrorMessage retrieves a transaction result error message of the specified transaction // within the specified block. // Expected error returns during normal operations: -// - [storage.ErrNotFound] if no result of a transaction with the specified ID is known +// - [storage.ErrNotFound] if no result error message of a transaction with the specified ID in `blockID` is known func RetrieveTransactionResultErrorMessage(r storage.Reader, blockID flow.Identifier, transactionID flow.Identifier, transactionResultErrorMessage *flow.TransactionResultErrorMessage) error { return RetrieveByKey(r, MakePrefix(codeTransactionResultErrorMessage, blockID, transactionID), transactionResultErrorMessage) } From c20915be3f286c5ebdcf5a7d2fc2e16864d8c262 Mon Sep 17 00:00:00 2001 From: Alex Hentschel Date: Fri, 10 Oct 2025 18:52:41 -0700 Subject: [PATCH 38/51] polished goDoc --- .../optimistic_sync/persisters/stores/results.go | 5 +++-- .../persisters/stores/transaction_result_error_messages.go | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/module/executiondatasync/optimistic_sync/persisters/stores/results.go b/module/executiondatasync/optimistic_sync/persisters/stores/results.go index 953b64fa7f2..328c2990487 100644 --- a/module/executiondatasync/optimistic_sync/persisters/stores/results.go +++ b/module/executiondatasync/optimistic_sync/persisters/stores/results.go @@ -30,8 +30,9 @@ func NewResultsStore( } } -// Persist adds results to the batch. -// requires [storage.LockInsertLightTransactionResult] to be hold +// Persist saves and indexes all transaction results (light representation) for our block as part of the +// provided database batch. The caller must acquire [storage.LockInsertLightTransactionResult] and hold +// it until the write batch has been committed. // No error returns are expected during normal operations func (r *ResultsStore) Persist(lctx lockctx.Proof, rw storage.ReaderBatchWriter) error { if err := r.persistedResults.BatchStore(lctx, rw, r.blockID, r.data); err != nil { diff --git a/module/executiondatasync/optimistic_sync/persisters/stores/transaction_result_error_messages.go b/module/executiondatasync/optimistic_sync/persisters/stores/transaction_result_error_messages.go index 1c084e75f0a..3d8923a4b27 100644 --- a/module/executiondatasync/optimistic_sync/persisters/stores/transaction_result_error_messages.go +++ b/module/executiondatasync/optimistic_sync/persisters/stores/transaction_result_error_messages.go @@ -34,8 +34,9 @@ func NewTxResultErrMsgStore( } } -// Persist adds transaction result error messages to the batch. -// +// Persist saves and indexes all transaction result error messages for our block as part of the +// provided database batch. The caller must acquire [storage.LockInsertTransactionResultErrMessage] +// and hold it until the write batch has been committed. // No error returns are expected during normal operations func (t *TxResultErrMsgStore) Persist(lctx lockctx.Proof, rw storage.ReaderBatchWriter) error { // CAUTION: here we assume that if something is already stored for our blockID, then the data is identical. From 9a21aea00b240ebdd4d4df9a2e9566554f048e6c Mon Sep 17 00:00:00 2001 From: Alex Hentschel Date: Fri, 10 Oct 2025 19:19:32 -0700 Subject: [PATCH 39/51] added cautionary statement regarding missing error doc --- .../optimistic_sync/persisters/stores/results.go | 10 +++++++++- storage/events.go | 2 ++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/module/executiondatasync/optimistic_sync/persisters/stores/results.go b/module/executiondatasync/optimistic_sync/persisters/stores/results.go index 328c2990487..ebcea5c8cbd 100644 --- a/module/executiondatasync/optimistic_sync/persisters/stores/results.go +++ b/module/executiondatasync/optimistic_sync/persisters/stores/results.go @@ -1,6 +1,7 @@ package stores import ( + "errors" "fmt" "github.com/jordanschalm/lockctx" @@ -35,7 +36,14 @@ func NewResultsStore( // it until the write batch has been committed. // No error returns are expected during normal operations func (r *ResultsStore) Persist(lctx lockctx.Proof, rw storage.ReaderBatchWriter) error { - if err := r.persistedResults.BatchStore(lctx, rw, r.blockID, r.data); err != nil { + // CAUTION: here we assume that if something is already stored for our blockID, then the data is identical. + // This only holds true for sealed execution results, whose consistency has previously been verified by + // comparing the data's hash to commitments in the execution result. + err := r.persistedResults.BatchStore(lctx, rw, r.blockID, r.data) + if err != nil { + if errors.Is(err, storage.ErrAlreadyExists) { + return nil + } return fmt.Errorf("could not add transaction results to batch: %w", err) } return nil diff --git a/storage/events.go b/storage/events.go index 4062acea82e..853320709ae 100644 --- a/storage/events.go +++ b/storage/events.go @@ -23,9 +23,11 @@ type Events interface { EventsReader // Store will store events for the given block ID + // TODO: error documentation Store(blockID flow.Identifier, blockEvents []flow.EventsList) error // BatchStore will store events for the given block ID in a given batch + // TODO: error documentation BatchStore(blockID flow.Identifier, events []flow.EventsList, batch ReaderBatchWriter) error // BatchRemoveByBlockID removes events keyed by a blockID in provided batch From 127db87fbc3072e4f7dc622f7278935e0999cb8c Mon Sep 17 00:00:00 2001 From: Alex Hentschel Date: Fri, 10 Oct 2025 19:45:21 -0700 Subject: [PATCH 40/51] extending documentation --- .../tx_error_messages_core.go | 17 ++++++++++------- storage/transaction_result_error_messages.go | 18 +++++++++++++++++- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/engine/access/ingestion/tx_error_messages/tx_error_messages_core.go b/engine/access/ingestion/tx_error_messages/tx_error_messages_core.go index 7216fd91e4f..a1f2ae86e4f 100644 --- a/engine/access/ingestion/tx_error_messages/tx_error_messages_core.go +++ b/engine/access/ingestion/tx_error_messages/tx_error_messages_core.go @@ -66,6 +66,10 @@ func (c *TxErrorMessagesCore) FetchErrorMessages(ctx context.Context, blockID fl return c.FetchErrorMessagesByENs(ctx, blockID, execNodes) } +// FetchErrorMessagesByENs requests the transaction result error messages for the specified block ID from +// any of the given execution nodes and persists them once retrieved. This function blocks until ingesting +// the tx error messages is completed or failed. +// It returns [storage.ErrAlreadyExists] if tx result error messages for the block already exist. func (c *TxErrorMessagesCore) FetchErrorMessagesByENs( ctx context.Context, blockID flow.Identifier, @@ -75,7 +79,6 @@ func (c *TxErrorMessagesCore) FetchErrorMessagesByENs( if err != nil { return fmt.Errorf("could not check existance of transaction result error messages: %w", err) } - if exists { return nil } @@ -104,14 +107,14 @@ func (c *TxErrorMessagesCore) FetchErrorMessagesByENs( return nil } -// storeTransactionResultErrorMessages stores the transaction result error messages for a given block ID. +// storeTransactionResultErrorMessages persists and indexes all transaction result error messages for the given blockID. +// The caller must acquire [storage.LockInsertTransactionResultErrMessage] and hold it until the write batch has been committed. // -// Parameters: -// - blockID: The identifier of the block for which the error messages are to be stored. -// - errorMessagesResponses: A slice of responses containing the error messages to be stored. -// - execNode: The execution node associated with the error messages. +// Note that transaction error messages are auxiliary data provided by the Execution Nodes on a goodwill basis and +// not protected by the protocol. Execution Error messages might be non-deterministic, i.e. potentially different +// for different execution nodes. Hence, we also persist which execution node (`execNode) provided the error message. // -// No errors are expected during normal operation. +// It returns [storage.ErrAlreadyExists] if tx result error messages for the block already exist. func (c *TxErrorMessagesCore) storeTransactionResultErrorMessages( blockID flow.Identifier, errorMessagesResponses []*execproto.GetTransactionErrorMessagesResponse_Result, diff --git a/storage/transaction_result_error_messages.go b/storage/transaction_result_error_messages.go index c3559be1790..73e71507d1e 100644 --- a/storage/transaction_result_error_messages.go +++ b/storage/transaction_result_error_messages.go @@ -10,23 +10,39 @@ import ( type TransactionResultErrorMessagesReader interface { // Exists returns true if transaction result error messages for the given ID have been stored. // + // Note that transaction error messages are auxiliary data provided by the Execution Nodes on a goodwill basis and + // not protected by the protocol. Execution Error messages might be non-deterministic, i.e. potentially different + // for different execution nodes. + // // No errors are expected during normal operation. Exists(blockID flow.Identifier) (bool, error) // ByBlockIDTransactionID returns the transaction result error message for the given block ID and transaction ID. // + // Note that transaction error messages are auxiliary data provided by the Execution Nodes on a goodwill basis and + // not protected by the protocol. Execution Error messages might be non-deterministic, i.e. potentially different + // for different execution nodes. + // // Expected errors during normal operation: // - [storage.ErrNotFound] if no transaction error message is known at given block and transaction id. ByBlockIDTransactionID(blockID flow.Identifier, transactionID flow.Identifier) (*flow.TransactionResultErrorMessage, error) // ByBlockIDTransactionIndex returns the transaction result error message for the given blockID and transaction index. // + // Note that transaction error messages are auxiliary data provided by the Execution Nodes on a goodwill basis and + // not protected by the protocol. Execution Error messages might be non-deterministic, i.e. potentially different + // for different execution nodes. + // // Expected errors during normal operation: // - [storage.ErrNotFound] if no transaction error message is known at given block and transaction index. ByBlockIDTransactionIndex(blockID flow.Identifier, txIndex uint32) (*flow.TransactionResultErrorMessage, error) // ByBlockID gets all transaction result error messages for a block, ordered by transaction index. - // Note: This method will return an empty slice both if the block is not indexed yet and if the block does not have any errors. + // CAUTION: This method will return an empty slice both if the block is not indexed yet and if the block does not have any errors. + // + // Note that transaction error messages are auxiliary data provided by the Execution Nodes on a goodwill basis and + // not protected by the protocol. Execution Error messages might be non-deterministic, i.e. potentially different + // for different execution nodes. // // No errors are expected during normal operations. ByBlockID(id flow.Identifier) ([]flow.TransactionResultErrorMessage, error) From 015edcc07f4951805762ad21af139c5fbf659c0f Mon Sep 17 00:00:00 2001 From: Alex Hentschel Date: Fri, 10 Oct 2025 19:58:40 -0700 Subject: [PATCH 41/51] moved Testing Lock manager's initialization very close to the init of the DB --- .../tx_error_messages/tx_error_messages_core.go | 9 +++++---- .../tx_error_messages_engine_test.go | 12 ++++++------ 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/engine/access/ingestion/tx_error_messages/tx_error_messages_core.go b/engine/access/ingestion/tx_error_messages/tx_error_messages_core.go index a1f2ae86e4f..b08a9ae907f 100644 --- a/engine/access/ingestion/tx_error_messages/tx_error_messages_core.go +++ b/engine/access/ingestion/tx_error_messages/tx_error_messages_core.go @@ -69,6 +69,11 @@ func (c *TxErrorMessagesCore) FetchErrorMessages(ctx context.Context, blockID fl // FetchErrorMessagesByENs requests the transaction result error messages for the specified block ID from // any of the given execution nodes and persists them once retrieved. This function blocks until ingesting // the tx error messages is completed or failed. +// +// Note that transaction error messages are auxiliary data provided by the Execution Nodes on a goodwill basis and +// not protected by the protocol. Execution Error messages might be non-deterministic, i.e. potentially different +// for different execution nodes. Hence, we also persist which execution node (`execNode) provided the error message. +// // It returns [storage.ErrAlreadyExists] if tx result error messages for the block already exist. func (c *TxErrorMessagesCore) FetchErrorMessagesByENs( ctx context.Context, @@ -110,10 +115,6 @@ func (c *TxErrorMessagesCore) FetchErrorMessagesByENs( // storeTransactionResultErrorMessages persists and indexes all transaction result error messages for the given blockID. // The caller must acquire [storage.LockInsertTransactionResultErrMessage] and hold it until the write batch has been committed. // -// Note that transaction error messages are auxiliary data provided by the Execution Nodes on a goodwill basis and -// not protected by the protocol. Execution Error messages might be non-deterministic, i.e. potentially different -// for different execution nodes. Hence, we also persist which execution node (`execNode) provided the error message. -// // It returns [storage.ErrAlreadyExists] if tx result error messages for the block already exist. func (c *TxErrorMessagesCore) storeTransactionResultErrorMessages( blockID flow.Identifier, diff --git a/engine/access/ingestion/tx_error_messages/tx_error_messages_engine_test.go b/engine/access/ingestion/tx_error_messages/tx_error_messages_engine_test.go index c2c1e309429..edaf55370f2 100644 --- a/engine/access/ingestion/tx_error_messages/tx_error_messages_engine_test.go +++ b/engine/access/ingestion/tx_error_messages/tx_error_messages_engine_test.go @@ -85,12 +85,15 @@ func (s *TxErrorMessagesEngineSuite) TearDownTest() { } func (s *TxErrorMessagesEngineSuite) SetupTest() { - s.log = unittest.Logger() - s.metrics = metrics.NewNoopCollector() - s.ctx, s.cancel = context.WithCancel(context.Background()) + // Initialize database and lock manager pdb, dbDir := unittest.TempPebbleDB(s.T()) s.db = pebbleimpl.ToDB(pdb) s.dbDir = dbDir + s.lockManager = storage.NewTestingLockManager() + + s.log = unittest.Logger() + s.metrics = metrics.NewNoopCollector() + s.ctx, s.cancel = context.WithCancel(context.Background()) // mock out protocol state s.proto.state = protocol.NewFollowerState(s.T()) s.proto.snapshot = protocol.NewSnapshot(s.T()) @@ -107,9 +110,6 @@ func (s *TxErrorMessagesEngineSuite) SetupTest() { s.Require().NoError(err) s.txResultsIndex = index.NewTransactionResultsIndex(s.indexReporter, s.lightTxResults) - // Initialize lock manager for tests - s.lockManager = storage.NewTestingLockManager() - blockCount := 5 s.blockMap = make(map[uint64]*flow.Block, blockCount) s.rootBlock = unittest.Block.Genesis(flow.Emulator) From 73c65143a50d66844347f5013f14193f421fca40 Mon Sep 17 00:00:00 2001 From: Alex Hentschel Date: Fri, 10 Oct 2025 20:01:02 -0700 Subject: [PATCH 42/51] marginal code consolidation --- .../tx_error_messages/tx_error_messages_engine_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/engine/access/ingestion/tx_error_messages/tx_error_messages_engine_test.go b/engine/access/ingestion/tx_error_messages/tx_error_messages_engine_test.go index edaf55370f2..9ae489f74a8 100644 --- a/engine/access/ingestion/tx_error_messages/tx_error_messages_engine_test.go +++ b/engine/access/ingestion/tx_error_messages/tx_error_messages_engine_test.go @@ -85,15 +85,16 @@ func (s *TxErrorMessagesEngineSuite) TearDownTest() { } func (s *TxErrorMessagesEngineSuite) SetupTest() { + s.log = unittest.Logger() + s.metrics = metrics.NewNoopCollector() + s.ctx, s.cancel = context.WithCancel(context.Background()) + // Initialize database and lock manager pdb, dbDir := unittest.TempPebbleDB(s.T()) s.db = pebbleimpl.ToDB(pdb) s.dbDir = dbDir s.lockManager = storage.NewTestingLockManager() - s.log = unittest.Logger() - s.metrics = metrics.NewNoopCollector() - s.ctx, s.cancel = context.WithCancel(context.Background()) // mock out protocol state s.proto.state = protocol.NewFollowerState(s.T()) s.proto.snapshot = protocol.NewSnapshot(s.T()) From 9d9a74702bf5ee35299bb27ff09b1e68a4697831 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Tue, 14 Oct 2025 08:53:41 -0700 Subject: [PATCH 43/51] remove review comments --- storage/operation/transaction_results.go | 45 ++++--------------- .../store/light_transaction_results_test.go | 20 ++++----- 2 files changed, 19 insertions(+), 46 deletions(-) diff --git a/storage/operation/transaction_results.go b/storage/operation/transaction_results.go index a842d04d441..d7d2c31d635 100644 --- a/storage/operation/transaction_results.go +++ b/storage/operation/transaction_results.go @@ -55,11 +55,6 @@ func RemoveTransactionResultsByBlockID(blockID flow.Identifier, rw storage.Reade return nil } -// deprecated -func InsertLightTransactionResult(w storage.Writer, blockID flow.Identifier, transactionResult *flow.LightTransactionResult) error { - return UpsertByKey(w, MakePrefix(codeLightTransactionResult, blockID, transactionResult.TransactionID), transactionResult) -} - // InsertAndIndexLightTransactionResults inserts and indexes a batch of light transaction results // the caller must hold [storage.LockInsertLightTransactionResult] lock // It returns storage.ErrAlreadyExists if light transaction results for the block already exist @@ -84,12 +79,13 @@ func InsertAndIndexLightTransactionResults( w := rw.Writer() for i, result := range transactionResults { - err := insertLightTransactionResult(w, blockID, &result) + // inserts a light transaction result by block ID and transaction ID + err := UpsertByKey(w, MakePrefix(codeLightTransactionResult, blockID, result.TransactionID), &result) if err != nil { return fmt.Errorf("cannot batch insert light tx result: %w", err) } - - err = indexLightTransactionResultByBlockIDAndTxIndex(w, blockID, uint32(i), &result) + // indexes a light transaction result by index within the block + err = UpsertByKey(w, MakePrefix(codeLightTransactionResultIndex, blockID, uint32(i)), &result) if err != nil { return fmt.Errorf("cannot batch index light tx result: %w", err) } @@ -97,18 +93,6 @@ func InsertAndIndexLightTransactionResults( return nil } -// insertLightTransactionResult inserts a light transaction result by block ID and transaction ID -// into the database using a batch write. -func insertLightTransactionResult(w storage.Writer, blockID flow.Identifier, transactionResult *flow.LightTransactionResult) error { - return UpsertByKey(w, MakePrefix(codeLightTransactionResult, blockID, transactionResult.TransactionID), transactionResult) -} - -// indexLightTransactionResultByBlockIDAndTxIndex indexes a light transaction result by index within the block using a -// batch write. -func indexLightTransactionResultByBlockIDAndTxIndex(w storage.Writer, blockID flow.Identifier, txIndex uint32, transactionResult *flow.LightTransactionResult) error { - return UpsertByKey(w, MakePrefix(codeLightTransactionResultIndex, blockID, txIndex), transactionResult) -} - func RetrieveLightTransactionResult(r storage.Reader, blockID flow.Identifier, transactionID flow.Identifier, transactionResult *flow.LightTransactionResult) error { return RetrieveByKey(r, MakePrefix(codeLightTransactionResult, blockID, transactionID), transactionResult) } @@ -157,13 +141,14 @@ func InsertAndIndexTransactionResultErrorMessages( } w := rw.Writer() - for _, result := range transactionResultErrorMessages { - err := insertTransactionResultErrorMessageByTxID(w, blockID, &result) + for _, txErrMsg := range transactionResultErrorMessages { + // insertTransactionResultErrorMessageByTxID inserts a transaction result error message by block ID and transaction ID + err := UpsertByKey(w, MakePrefix(codeTransactionResultErrorMessage, blockID, txErrMsg.TransactionID), &txErrMsg) if err != nil { return fmt.Errorf("cannot batch insert tx result error message: %w", err) } - - err = indexTransactionResultErrorMessageBlockIDTxIndex(w, blockID, &result) + // indexTransactionResultErrorMessageBlockIDTxIndex indexes a transaction result error message by index within the block + err = UpsertByKey(w, MakePrefix(codeTransactionResultErrorMessageIndex, blockID, txErrMsg.Index), &txErrMsg) if err != nil { return fmt.Errorf("cannot batch index tx result error message: %w", err) } @@ -171,18 +156,6 @@ func InsertAndIndexTransactionResultErrorMessages( return nil } -// insertTransactionResultErrorMessageByTxID inserts a transaction result error message by block ID and transaction ID -// into the database using a batch write. -func insertTransactionResultErrorMessageByTxID(w storage.Writer, blockID flow.Identifier, transactionResultErrorMessage *flow.TransactionResultErrorMessage) error { - return UpsertByKey(w, MakePrefix(codeTransactionResultErrorMessage, blockID, transactionResultErrorMessage.TransactionID), transactionResultErrorMessage) -} - -// indexTransactionResultErrorMessageBlockIDTxIndex indexes a transaction result error message by index within the block using a -// batch write. -func indexTransactionResultErrorMessageBlockIDTxIndex(w storage.Writer, blockID flow.Identifier, transactionResultErrorMessage *flow.TransactionResultErrorMessage) error { - return UpsertByKey(w, MakePrefix(codeTransactionResultErrorMessageIndex, blockID, transactionResultErrorMessage.Index), transactionResultErrorMessage) -} - // RetrieveTransactionResultErrorMessage retrieves a transaction result error message by block ID and transaction ID. func RetrieveTransactionResultErrorMessage(r storage.Reader, blockID flow.Identifier, transactionID flow.Identifier, transactionResultErrorMessage *flow.TransactionResultErrorMessage) error { return RetrieveByKey(r, MakePrefix(codeTransactionResultErrorMessage, blockID, transactionID), transactionResultErrorMessage) diff --git a/storage/store/light_transaction_results_test.go b/storage/store/light_transaction_results_test.go index fe48dfdbc60..dec975bc351 100644 --- a/storage/store/light_transaction_results_test.go +++ b/storage/store/light_transaction_results_test.go @@ -20,22 +20,22 @@ func TestBatchStoringLightTransactionResults(t *testing.T) { dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { lockManager := storage.NewTestingLockManager() metrics := metrics.NewNoopCollector() - store1 := store.NewLightTransactionResults(metrics, db, 1000) + txResultsStore := store.NewLightTransactionResults(metrics, db, 1000) blockID := unittest.IdentifierFixture() txResults := getLightTransactionResultsFixture(10) - t.Run("batch store1 results", func(t *testing.T) { + t.Run("batch txResultsStore results", func(t *testing.T) { require.NoError(t, unittest.WithLock(t, lockManager, storage.LockInsertLightTransactionResult, func(lctx lockctx.Context) error { return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { - return store1.BatchStore(lctx, rw, blockID, txResults) + return txResultsStore.BatchStore(lctx, rw, blockID, txResults) }) })) // add a results to a new block to validate they are not included in lookups require.NoError(t, unittest.WithLock(t, lockManager, storage.LockInsertLightTransactionResult, func(lctx lockctx.Context) error { return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { - return store1.BatchStore(lctx, rw, unittest.IdentifierFixture(), getLightTransactionResultsFixture(2)) + return txResultsStore.BatchStore(lctx, rw, unittest.IdentifierFixture(), getLightTransactionResultsFixture(2)) }) })) @@ -43,7 +43,7 @@ func TestBatchStoringLightTransactionResults(t *testing.T) { t.Run("read results with cache", func(t *testing.T) { for _, txResult := range txResults { - actual, err := store1.ByBlockIDTransactionID(blockID, txResult.TransactionID) + actual, err := txResultsStore.ByBlockIDTransactionID(blockID, txResult.TransactionID) require.NoError(t, err) assert.Equal(t, txResult, *actual) } @@ -63,7 +63,7 @@ func TestBatchStoringLightTransactionResults(t *testing.T) { t.Run("cached and non-cached results are equal", func(t *testing.T) { // check retrieving by index from both cache and db for i := len(txResults) - 1; i >= 0; i-- { - actual, err := store1.ByBlockIDTransactionIndex(blockID, uint32(i)) + actual, err := txResultsStore.ByBlockIDTransactionIndex(blockID, uint32(i)) require.NoError(t, err) assert.Equal(t, txResults[i], *actual) @@ -74,7 +74,7 @@ func TestBatchStoringLightTransactionResults(t *testing.T) { }) t.Run("read all results for block", func(t *testing.T) { - actuals, err := store1.ByBlockID(blockID) + actuals, err := txResultsStore.ByBlockID(blockID) require.NoError(t, err) assert.Equal(t, len(txResults), len(actuals)) @@ -88,16 +88,16 @@ func TestBatchStoringLightTransactionResults(t *testing.T) { func TestReadingNotStoredLightTransactionResults(t *testing.T) { dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { metrics := metrics.NewNoopCollector() - store1 := store.NewLightTransactionResults(metrics, db, 1000) + txResultsStore := store.NewLightTransactionResults(metrics, db, 1000) blockID := unittest.IdentifierFixture() txID := unittest.IdentifierFixture() txIndex := rand.Uint32() - _, err := store1.ByBlockIDTransactionID(blockID, txID) + _, err := txResultsStore.ByBlockIDTransactionID(blockID, txID) assert.ErrorIs(t, err, storage.ErrNotFound) - _, err = store1.ByBlockIDTransactionIndex(blockID, txIndex) + _, err = txResultsStore.ByBlockIDTransactionIndex(blockID, txIndex) assert.ErrorIs(t, err, storage.ErrNotFound) }) } From 4854931e22115ee04f65435423b114b26d7817d8 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Tue, 14 Oct 2025 09:14:55 -0700 Subject: [PATCH 44/51] remove deprecated methods --- storage/light_transaction_results.go | 8 ++------ storage/mock/light_transaction_results.go | 18 ------------------ storage/store/light_transaction_results.go | 10 ++-------- 3 files changed, 4 insertions(+), 32 deletions(-) diff --git a/storage/light_transaction_results.go b/storage/light_transaction_results.go index 69083e1b0d5..47ff7beac61 100644 --- a/storage/light_transaction_results.go +++ b/storage/light_transaction_results.go @@ -21,9 +21,8 @@ type LightTransactionResultsReader interface { ByBlockIDTransactionIndex(blockID flow.Identifier, txIndex uint32) (*flow.LightTransactionResult, error) // ByBlockID gets all transaction results for a block, ordered by transaction index - // - // Expected error returns during normal operation: - // - [storage.ErrNotFound] if light transaction results at given blockID weren't found. + // CAUTION: this function returns the empty list in case for block IDs without known results. + // No error returns are expected during normal operations. ByBlockID(id flow.Identifier) ([]flow.LightTransactionResult, error) } @@ -36,7 +35,4 @@ type LightTransactionResults interface { // hold it until the write batch has been committed. // It returns [storage.ErrAlreadyExists] if light transaction results for the block already exist. BatchStore(lctx lockctx.Proof, rw ReaderBatchWriter, blockID flow.Identifier, transactionResults []flow.LightTransactionResult) error - - // Deprecated: deprecated as a part of transition from Badger to Pebble. use BatchStore instead - BatchStoreBadger(blockID flow.Identifier, transactionResults []flow.LightTransactionResult, batch BatchStorage) error } diff --git a/storage/mock/light_transaction_results.go b/storage/mock/light_transaction_results.go index ddc9c9dfcf3..03f5f3326b3 100644 --- a/storage/mock/light_transaction_results.go +++ b/storage/mock/light_transaction_results.go @@ -34,24 +34,6 @@ func (_m *LightTransactionResults) BatchStore(lctx lockctx.Proof, rw storage.Rea return r0 } -// BatchStoreBadger provides a mock function with given fields: blockID, transactionResults, batch -func (_m *LightTransactionResults) BatchStoreBadger(blockID flow.Identifier, transactionResults []flow.LightTransactionResult, batch storage.BatchStorage) error { - ret := _m.Called(blockID, transactionResults, batch) - - if len(ret) == 0 { - panic("no return value specified for BatchStoreBadger") - } - - var r0 error - if rf, ok := ret.Get(0).(func(flow.Identifier, []flow.LightTransactionResult, storage.BatchStorage) error); ok { - r0 = rf(blockID, transactionResults, batch) - } else { - r0 = ret.Error(0) - } - - return r0 -} - // ByBlockID provides a mock function with given fields: id func (_m *LightTransactionResults) ByBlockID(id flow.Identifier) ([]flow.LightTransactionResult, error) { ret := _m.Called(id) diff --git a/storage/store/light_transaction_results.go b/storage/store/light_transaction_results.go index 789f185af5a..6292ebbfe30 100644 --- a/storage/store/light_transaction_results.go +++ b/storage/store/light_transaction_results.go @@ -99,11 +99,6 @@ func (tr *LightTransactionResults) BatchStore(lctx lockctx.Proof, rw storage.Rea return nil } -// Deprecated: panics -func (tr *LightTransactionResults) BatchStoreBadger(blockID flow.Identifier, transactionResults []flow.LightTransactionResult, batch storage.BatchStorage) error { - panic("LightTransactionResults BatchStoreBadger not implemented") -} - // ByBlockIDTransactionID returns the transaction result for the given block ID and transaction ID // // Expected error returns during normal operation: @@ -131,9 +126,8 @@ func (tr *LightTransactionResults) ByBlockIDTransactionIndex(blockID flow.Identi } // ByBlockID gets all transaction results for a block, ordered by transaction index -// -// Expected error returns during normal operation: -// - [storage.ErrNotFound] if light transaction results at given blockID weren't found. +// CAUTION: this function returns the empty list in case for block IDs without known results. +// No error returns are expected during normal operations. func (tr *LightTransactionResults) ByBlockID(blockID flow.Identifier) ([]flow.LightTransactionResult, error) { transactionResults, err := tr.blockCache.Get(tr.db.Reader(), blockID) if err != nil { From a19f958a4eb23d8490081afe0e7681f76df04ff1 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Tue, 14 Oct 2025 09:26:15 -0700 Subject: [PATCH 45/51] update comments --- .../optimistic_sync/persisters/stores/events.go | 4 +++- .../optimistic_sync/persisters/stores/results.go | 6 +++--- .../persisters/stores/transaction_result_error_messages.go | 6 +++--- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/module/executiondatasync/optimistic_sync/persisters/stores/events.go b/module/executiondatasync/optimistic_sync/persisters/stores/events.go index 6792009534d..840680c2113 100644 --- a/module/executiondatasync/optimistic_sync/persisters/stores/events.go +++ b/module/executiondatasync/optimistic_sync/persisters/stores/events.go @@ -38,7 +38,9 @@ func (e *EventsStore) Persist(_ lockctx.Proof, batch storage.ReaderBatchWriter) err := e.persistedEvents.BatchStore(e.blockID, []flow.EventsList{e.data}, batch) if err != nil { if errors.Is(err, storage.ErrAlreadyExists) { - // we don't overwrite, it's ideompotent + // CAUTION: here we assume that if something is already stored for our blockID, then the data is identical. + // This only holds true for sealed execution results, whose consistency has previously been verified by + // comparing the data's hash to commitments in the execution result. return nil } return fmt.Errorf("could not add events to batch: %w", err) diff --git a/module/executiondatasync/optimistic_sync/persisters/stores/results.go b/module/executiondatasync/optimistic_sync/persisters/stores/results.go index ebcea5c8cbd..dca1fe94d30 100644 --- a/module/executiondatasync/optimistic_sync/persisters/stores/results.go +++ b/module/executiondatasync/optimistic_sync/persisters/stores/results.go @@ -36,11 +36,11 @@ func NewResultsStore( // it until the write batch has been committed. // No error returns are expected during normal operations func (r *ResultsStore) Persist(lctx lockctx.Proof, rw storage.ReaderBatchWriter) error { - // CAUTION: here we assume that if something is already stored for our blockID, then the data is identical. - // This only holds true for sealed execution results, whose consistency has previously been verified by - // comparing the data's hash to commitments in the execution result. err := r.persistedResults.BatchStore(lctx, rw, r.blockID, r.data) if err != nil { + // CAUTION: here we assume that if something is already stored for our blockID, then the data is identical. + // This only holds true for sealed execution results, whose consistency has previously been verified by + // comparing the data's hash to commitments in the execution result. if errors.Is(err, storage.ErrAlreadyExists) { return nil } diff --git a/module/executiondatasync/optimistic_sync/persisters/stores/transaction_result_error_messages.go b/module/executiondatasync/optimistic_sync/persisters/stores/transaction_result_error_messages.go index 3d8923a4b27..b6555ef3bba 100644 --- a/module/executiondatasync/optimistic_sync/persisters/stores/transaction_result_error_messages.go +++ b/module/executiondatasync/optimistic_sync/persisters/stores/transaction_result_error_messages.go @@ -39,11 +39,11 @@ func NewTxResultErrMsgStore( // and hold it until the write batch has been committed. // No error returns are expected during normal operations func (t *TxResultErrMsgStore) Persist(lctx lockctx.Proof, rw storage.ReaderBatchWriter) error { - // CAUTION: here we assume that if something is already stored for our blockID, then the data is identical. - // This only holds true for sealed execution results, whose consistency has previously been verified by - // comparing the data's hash to commitments in the execution result. err := t.persistedTxResultErrMsg.BatchStore(lctx, rw, t.blockID, t.data) if err != nil { + // CAUTION: here we assume that if something is already stored for our blockID, then the data is identical. + // This only holds true for sealed execution results, whose consistency has previously been verified by + // comparing the data's hash to commitments in the execution result. if errors.Is(err, storage.ErrAlreadyExists) { return nil } From 348284d665b7082e7f337b27d0014a194fb58f59 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Tue, 14 Oct 2025 09:32:12 -0700 Subject: [PATCH 46/51] add tests for light transaction results store --- .../store/light_transaction_results_test.go | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/storage/store/light_transaction_results_test.go b/storage/store/light_transaction_results_test.go index dec975bc351..c773959b8b5 100644 --- a/storage/store/light_transaction_results_test.go +++ b/storage/store/light_transaction_results_test.go @@ -102,6 +102,94 @@ func TestReadingNotStoredLightTransactionResults(t *testing.T) { }) } +// Test that attempting to batch store light transaction results for a block ID that already exists +// results in a [storage.ErrAlreadyExists] error, and that the original data remains unchanged. +func TestBatchStoreLightTransactionResultsErrAlreadyExists(t *testing.T) { + dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { + lockManager := storage.NewTestingLockManager() + metrics := metrics.NewNoopCollector() + txResultsStore := store.NewLightTransactionResults(metrics, db, 1000) + + blockID := unittest.IdentifierFixture() + txResults := getLightTransactionResultsFixture(3) + + // First batch store should succeed + err := unittest.WithLock(t, lockManager, storage.LockInsertLightTransactionResult, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return txResultsStore.BatchStore(lctx, rw, blockID, txResults) + }) + }) + require.NoError(t, err) + + // Second batch store with the same blockID should fail with ErrAlreadyExists + duplicateTxResults := getLightTransactionResultsFixture(2) + err = unittest.WithLock(t, lockManager, storage.LockInsertLightTransactionResult, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return txResultsStore.BatchStore(lctx, rw, blockID, duplicateTxResults) + }) + }) + require.Error(t, err) + require.ErrorIs(t, err, storage.ErrAlreadyExists) + + // Verify that the original data is unchanged + actuals, err := txResultsStore.ByBlockID(blockID) + require.NoError(t, err) + require.Equal(t, len(txResults), len(actuals)) + for i := range txResults { + assert.Equal(t, txResults[i], actuals[i]) + } + }) +} + +// Test that attempting to batch store light transaction results without holding the required lock +// results in an error indicating the missing lock. The implementation should not conflate this error +// case with data for the same key already existing, ie. it should not return [storage.ErrAlreadyExists]. +func TestBatchStoreLightTransactionResultsMissingLock(t *testing.T) { + dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { + metrics := metrics.NewNoopCollector() + txResultsStore := store.NewLightTransactionResults(metrics, db, 1000) + + blockID := unittest.IdentifierFixture() + txResults := getLightTransactionResultsFixture(3) + + // Create a context without the required lock + lockManager := storage.NewTestingLockManager() + lctx := lockManager.NewContext() + defer lctx.Release() + + err := db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return txResultsStore.BatchStore(lctx, rw, blockID, txResults) + }) + require.Error(t, err) + require.NotErrorIs(t, err, storage.ErrAlreadyExists) + require.Contains(t, err.Error(), "lock_insert_light_transaction_result") + }) +} + +// Test that attempting to batch store light transaction results while holding the wrong lock +// results in an error indicating the incorrect lock. The implementation should not conflate this error +// case with data for the same key already existing, ie. it should not return [storage.ErrAlreadyExists]. +func TestBatchStoreLightTransactionResultsWrongLock(t *testing.T) { + dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { + lockManager := storage.NewTestingLockManager() + metrics := metrics.NewNoopCollector() + txResultsStore := store.NewLightTransactionResults(metrics, db, 1000) + + blockID := unittest.IdentifierFixture() + txResults := getLightTransactionResultsFixture(3) + + // Try to use the wrong lock + err := unittest.WithLock(t, lockManager, storage.LockInsertBlock, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return txResultsStore.BatchStore(lctx, rw, blockID, txResults) + }) + }) + require.Error(t, err) + require.NotErrorIs(t, err, storage.ErrAlreadyExists) + require.Contains(t, err.Error(), "lock_insert_light_transaction_result") + }) +} + func getLightTransactionResultsFixture(n int) []flow.LightTransactionResult { txResults := make([]flow.LightTransactionResult, 0, n) for i := 0; i < n; i++ { From 7636d0b1e3316cca8251d0a7d5f9e8fb7d557241 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Tue, 14 Oct 2025 09:36:25 -0700 Subject: [PATCH 47/51] check locks held in tests --- .../tx_error_messages/tx_error_messages_core_test.go | 11 +++++++++-- .../tx_error_messages_engine_test.go | 7 +++++-- .../indexer/indexer_core_test.go | 7 ++++++- 3 files changed, 20 insertions(+), 5 deletions(-) diff --git a/engine/access/ingestion/tx_error_messages/tx_error_messages_core_test.go b/engine/access/ingestion/tx_error_messages/tx_error_messages_core_test.go index e89d7ace78a..431aa215a69 100644 --- a/engine/access/ingestion/tx_error_messages/tx_error_messages_core_test.go +++ b/engine/access/ingestion/tx_error_messages/tx_error_messages_core_test.go @@ -5,6 +5,7 @@ import ( "fmt" "testing" + "github.com/jordanschalm/lockctx" execproto "github.com/onflow/flow/protobuf/go/flow/execution" "github.com/rs/zerolog" "github.com/stretchr/testify/mock" @@ -149,7 +150,10 @@ func (s *TxErrorMessagesCoreSuite) TestHandleTransactionResultErrorMessages() { // Mock the storage of the fetched error messages into the protocol database. s.txErrorMessages.On("Store", mock.Anything, blockId, expectedStoreTxErrorMessages). - Return(nil).Once() + Return(func(lctx lockctx.Proof, blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage) error { + require.True(s.T(), lctx.HoldsLock(storage.LockInsertTransactionResultErrMessage)) + return nil + }).Once() core := s.initCore() err := core.FetchErrorMessages(irrecoverableCtx, blockId) @@ -234,7 +238,10 @@ func (s *TxErrorMessagesCoreSuite) TestHandleTransactionResultErrorMessages_Erro // Simulate an error when attempting to store the fetched transaction error messages in storage. expectedStoreTxErrorMessages := createExpectedTxErrorMessages(resultsByBlockID, s.enNodeIDs.NodeIDs()[0]) s.txErrorMessages.On("Store", mock.Anything, blockId, expectedStoreTxErrorMessages). - Return(fmt.Errorf("storage error")).Once() + Return(func(lctx lockctx.Proof, blockID flow.Identifier, transactionResultErrorMessages []flow.TransactionResultErrorMessage) error { + require.True(s.T(), lctx.HoldsLock(storage.LockInsertTransactionResultErrMessage)) + return fmt.Errorf("storage error") + }).Once() core := s.initCore() err := core.FetchErrorMessages(irrecoverableCtx, blockId) diff --git a/engine/access/ingestion/tx_error_messages/tx_error_messages_engine_test.go b/engine/access/ingestion/tx_error_messages/tx_error_messages_engine_test.go index 9ae489f74a8..d4bb86ad984 100644 --- a/engine/access/ingestion/tx_error_messages/tx_error_messages_engine_test.go +++ b/engine/access/ingestion/tx_error_messages/tx_error_messages_engine_test.go @@ -7,6 +7,7 @@ import ( "testing" "time" + "github.com/jordanschalm/lockctx" execproto "github.com/onflow/flow/protobuf/go/flow/execution" "github.com/rs/zerolog" "github.com/stretchr/testify/mock" @@ -253,8 +254,10 @@ func (s *TxErrorMessagesEngineSuite) TestOnFinalizedBlockHandleTxErrorMessages() // Mock the storage of the fetched error messages into the protocol database. s.txErrorMessages.On("Store", mock.Anything, blockID, expectedStoreTxErrorMessages).Return(nil). Run(func(args mock.Arguments) { - // Ensure the test does not complete its work faster than necessary - wg.Done() + lctx, ok := args[0].(lockctx.Proof) + require.True(s.T(), ok, "expecting lock proof, but cast failed") + require.True(s.T(), lctx.HoldsLock(storage.LockInsertTransactionResultErrMessage)) + wg.Done() // Ensure the test does not complete its work faster than necessary }).Once() } diff --git a/module/state_synchronization/indexer/indexer_core_test.go b/module/state_synchronization/indexer/indexer_core_test.go index fecdafd4996..909c74caee9 100644 --- a/module/state_synchronization/indexer/indexer_core_test.go +++ b/module/state_synchronization/indexer/indexer_core_test.go @@ -144,6 +144,7 @@ func (i *indexCoreTest) setStoreTransactionResults(f func(*testing.T, flow.Ident i.results. On("BatchStore", mock.Anything, mock.Anything, mock.AnythingOfType("flow.Identifier"), mock.AnythingOfType("[]flow.LightTransactionResult")). Return(func(lctx lockctx.Proof, batch storage.ReaderBatchWriter, blockID flow.Identifier, results []flow.LightTransactionResult) error { + require.True(i.t, lctx.HoldsLock(storage.LockInsertLightTransactionResult)) require.NotNil(i.t, batch) return f(i.t, blockID, results) }) @@ -177,7 +178,11 @@ func (i *indexCoreTest) useDefaultEvents() *indexCoreTest { func (i *indexCoreTest) useDefaultTransactionResults() *indexCoreTest { i.results. On("BatchStore", mock.Anything, mock.Anything, mock.AnythingOfType("flow.Identifier"), mock.AnythingOfType("[]flow.LightTransactionResult")). - Return(nil) + Return(func(lctx lockctx.Proof, batch storage.ReaderBatchWriter, _ flow.Identifier, _ []flow.LightTransactionResult) error { + require.True(i.t, lctx.HoldsLock(storage.LockInsertLightTransactionResult)) + require.NotNil(i.t, batch) + return nil + }) return i } From e645c880323bfb081ffb6c2dafa6f70546fce802 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Tue, 14 Oct 2025 09:43:37 -0700 Subject: [PATCH 48/51] update optimistic sync core impl tests to verify locks held --- .../optimistic_sync/core_impl_test.go | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/module/executiondatasync/optimistic_sync/core_impl_test.go b/module/executiondatasync/optimistic_sync/core_impl_test.go index 878d18f3487..a078d885fa6 100644 --- a/module/executiondatasync/optimistic_sync/core_impl_test.go +++ b/module/executiondatasync/optimistic_sync/core_impl_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + "github.com/jordanschalm/lockctx" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/suite" @@ -446,9 +447,21 @@ func (c *CoreImplSuite) TestCoreImpl_Persist() { indexerData := core.workingData.indexerData c.persistentRegisters.On("Store", flow.RegisterEntries(indexerData.Registers), tf.block.Height).Return(nil) c.persistentEvents.On("BatchStore", blockID, []flow.EventsList{indexerData.Events}, mock.Anything).Return(nil) - c.persistentCollections.On("BatchStoreAndIndexByTransaction", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, nil) - c.persistentResults.On("BatchStore", mock.Anything, mock.Anything, blockID, indexerData.Results).Return(nil) - c.persistentTxResultErrMsg.On("BatchStore", mock.Anything, mock.Anything, blockID, core.workingData.txResultErrMsgsData).Return(nil) + c.persistentCollections.On("BatchStoreAndIndexByTransaction", + mock.MatchedBy(func(lctx lockctx.Proof) bool { + return lctx.HoldsLock(storage.LockInsertCollection) + }), + mock.Anything, mock.Anything, mock.Anything).Return(nil, nil) + c.persistentResults.On("BatchStore", + mock.MatchedBy(func(lctx lockctx.Proof) bool { + return lctx.HoldsLock(storage.LockInsertLightTransactionResult) + }), + mock.Anything, blockID, indexerData.Results).Return(nil) + c.persistentTxResultErrMsg.On("BatchStore", + mock.MatchedBy(func(lctx lockctx.Proof) bool { + return lctx.HoldsLock(storage.LockInsertTransactionResultErrMessage) + }), + mock.Anything, blockID, core.workingData.txResultErrMsgsData).Return(nil) c.latestPersistedSealedResult.On("BatchSet", tf.exeResult.ID(), tf.block.Height, mock.Anything).Return(nil) err = core.Persist() From 94a08b5ce2fe63dc73c178a80688bd98d105c0f1 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Wed, 15 Oct 2025 08:45:40 -0700 Subject: [PATCH 49/51] fix mocks --- module/state_synchronization/indexer/indexer_core_test.go | 4 ++-- storage/locks.go | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/module/state_synchronization/indexer/indexer_core_test.go b/module/state_synchronization/indexer/indexer_core_test.go index 671d4dea32b..495d84c3adf 100644 --- a/module/state_synchronization/indexer/indexer_core_test.go +++ b/module/state_synchronization/indexer/indexer_core_test.go @@ -257,8 +257,8 @@ func TestExecutionState_IndexBlockData(t *testing.T) { t.Run("Index AllTheThings", func(t *testing.T) { test := newIndexCoreTest(t, g, blocks, tf.ExecutionDataEntity()).initIndexer() - test.events.On("BatchStore", blockID, []flow.EventsList{tf.ExpectedEvents}, mock.Anything).Return(nil) - test.results.On("BatchStore", blockID, tf.ExpectedResults, mock.Anything).Return(nil) + test.events.On("BatchStore", mock.Anything, []flow.EventsList{tf.ExpectedEvents}, mock.Anything).Return(nil) + test.results.On("BatchStore", mock.Anything, mock.Anything, blockID, tf.ExpectedResults).Return(nil) test.registers. On("Store", mock.Anything, tf.Block.Height). Run(func(args mock.Arguments) { diff --git a/storage/locks.go b/storage/locks.go index f09f4c62f42..da3c2532fba 100644 --- a/storage/locks.go +++ b/storage/locks.go @@ -89,6 +89,7 @@ func makeLockPolicy() lockctx.Policy { // module/executiondatasync/optimistic_sync/persisters/block.go#Persist Add(LockInsertCollection, LockInsertLightTransactionResult). Add(LockInsertLightTransactionResult, LockInsertTransactionResultErrMessage). + Add(LockInsertLightTransactionResult, LockIndexScheduledTransaction). Build() } From ee7a62702378c53cb97545ba9927e22906c652e6 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Wed, 15 Oct 2025 08:47:57 -0700 Subject: [PATCH 50/51] update tests --- .../indexer/indexer_core_test.go | 22 ++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/module/state_synchronization/indexer/indexer_core_test.go b/module/state_synchronization/indexer/indexer_core_test.go index 495d84c3adf..7d2146ffbb4 100644 --- a/module/state_synchronization/indexer/indexer_core_test.go +++ b/module/state_synchronization/indexer/indexer_core_test.go @@ -144,6 +144,7 @@ func (i *indexCoreTest) setStoreEvents(f func(*testing.T, flow.Identifier, []flo return i } + func (i *indexCoreTest) setStoreTransactionResults(f func(*testing.T, flow.Identifier, []flow.LightTransactionResult) error) *indexCoreTest { i.results. On("BatchStore", mock.Anything, mock.Anything, mock.AnythingOfType("flow.Identifier"), mock.AnythingOfType("[]flow.LightTransactionResult")). @@ -257,8 +258,18 @@ func TestExecutionState_IndexBlockData(t *testing.T) { t.Run("Index AllTheThings", func(t *testing.T) { test := newIndexCoreTest(t, g, blocks, tf.ExecutionDataEntity()).initIndexer() - test.events.On("BatchStore", mock.Anything, []flow.EventsList{tf.ExpectedEvents}, mock.Anything).Return(nil) - test.results.On("BatchStore", mock.Anything, mock.Anything, blockID, tf.ExpectedResults).Return(nil) + test.events.On("BatchStore", mock.Anything, []flow.EventsList{tf.ExpectedEvents}, mock.Anything). + Return(func(blockID flow.Identifier, events []flow.EventsList, batch storage.ReaderBatchWriter) error { + require.NotNil(t, batch) + // Events BatchStore doesn't require specific locks, but we validate the batch is provided + return nil + }) + test.results.On("BatchStore", mock.Anything, mock.Anything, blockID, tf.ExpectedResults). + Return(func(lctx lockctx.Proof, batch storage.ReaderBatchWriter, blockID flow.Identifier, results []flow.LightTransactionResult) error { + require.True(t, lctx.HoldsLock(storage.LockInsertLightTransactionResult)) + require.NotNil(t, batch) + return nil + }) test.registers. On("Store", mock.Anything, tf.Block.Height). Run(func(args mock.Arguments) { @@ -271,7 +282,12 @@ func TestExecutionState_IndexBlockData(t *testing.T) { test.collections.On("StoreAndIndexByTransaction", mock.Anything, collection).Return(&flow.LightCollection{}, nil) } for txID, scheduledTxID := range tf.ExpectedScheduledTransactions { - test.scheduledTransactions.On("BatchIndex", mock.Anything, blockID, txID, scheduledTxID, mock.Anything).Return(nil) + test.scheduledTransactions.On("BatchIndex", mock.Anything, blockID, txID, scheduledTxID, mock.Anything). + Return(func(lctx lockctx.Proof, blockID flow.Identifier, txID flow.Identifier, scheduledTxID uint64, batch storage.ReaderBatchWriter) error { + require.True(t, lctx.HoldsLock(storage.LockIndexScheduledTransaction)) + require.NotNil(t, batch) + return nil + }) } err := test.indexer.IndexBlockData(tf.ExecutionDataEntity()) From c51b8711820a84c1615da0b8bfac1471f9cb2ce5 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Wed, 15 Oct 2025 09:01:43 -0700 Subject: [PATCH 51/51] fix lint --- module/state_synchronization/indexer/indexer_core_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/module/state_synchronization/indexer/indexer_core_test.go b/module/state_synchronization/indexer/indexer_core_test.go index 7d2146ffbb4..24ae7424c48 100644 --- a/module/state_synchronization/indexer/indexer_core_test.go +++ b/module/state_synchronization/indexer/indexer_core_test.go @@ -144,7 +144,6 @@ func (i *indexCoreTest) setStoreEvents(f func(*testing.T, flow.Identifier, []flo return i } - func (i *indexCoreTest) setStoreTransactionResults(f func(*testing.T, flow.Identifier, []flow.LightTransactionResult) error) *indexCoreTest { i.results. On("BatchStore", mock.Anything, mock.Anything, mock.AnythingOfType("flow.Identifier"), mock.AnythingOfType("[]flow.LightTransactionResult")).