diff --git a/cmd/access/node_builder/access_node_builder.go b/cmd/access/node_builder/access_node_builder.go index 81d618a6c33..7e98451f0a0 100644 --- a/cmd/access/node_builder/access_node_builder.go +++ b/cmd/access/node_builder/access_node_builder.go @@ -2236,6 +2236,8 @@ func (builder *FlowAccessNodeBuilder) Build() (cmd.Node, error) { node.EngineRegistry, node.State, node.Me, + node.StorageLockMgr, + node.ProtocolDB, node.Storage.Blocks, node.Storage.Results, node.Storage.Receipts, diff --git a/cmd/util/cmd/execution-state-extract/execution_state_extract_test.go b/cmd/util/cmd/execution-state-extract/execution_state_extract_test.go index 0131a250bc3..eeb7412ee19 100644 --- a/cmd/util/cmd/execution-state-extract/execution_state_extract_test.go +++ b/cmd/util/cmd/execution-state-extract/execution_state_extract_test.go @@ -67,7 +67,7 @@ func TestExtractExecutionState(t *testing.T) { blockID := unittest.IdentifierFixture() stateCommitment := unittest.StateCommitmentFixture() - err := unittest.WithLock(t, lockManager, storage.LockInsertOwnReceipt, func(lctx lockctx.Context) error { + err := unittest.WithLock(t, lockManager, storage.LockIndexStateCommitment, func(lctx lockctx.Context) error { return storageDB.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { // Store the state commitment for the block ID return operation.IndexStateCommitment(lctx, rw, blockID, stateCommitment) @@ -139,7 +139,7 @@ func TestExtractExecutionState(t *testing.T) { // generate random block and map it to state commitment blockID := unittest.IdentifierFixture() - err = unittest.WithLock(t, lockManager, storage.LockInsertOwnReceipt, func(lctx lockctx.Context) error { + err = unittest.WithLock(t, lockManager, storage.LockIndexStateCommitment, func(lctx lockctx.Context) error { return storageDB.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { return operation.IndexStateCommitment(lctx, rw, blockID, flow.StateCommitment(stateCommitment)) }) diff --git a/cmd/util/cmd/reindex/cmd/results.go b/cmd/util/cmd/reindex/cmd/results.go deleted file mode 100644 index 33fcd92b87c..00000000000 --- a/cmd/util/cmd/reindex/cmd/results.go +++ /dev/null @@ -1,56 +0,0 @@ -package cmd - -import ( - "fmt" - - "github.com/rs/zerolog/log" - "github.com/spf13/cobra" - - "github.com/onflow/flow-go/cmd/util/cmd/common" - "github.com/onflow/flow-go/storage" -) - -func init() { - rootCmd.AddCommand(resultsCmd) -} - -var resultsCmd = &cobra.Command{ - Use: "results", - Short: "reindex sealed result IDs by block ID", - RunE: func(cmd *cobra.Command, args []string) error { - lockManager := storage.MakeSingletonLockManager() - return common.WithStorage(flagDatadir, func(db storage.DB) error { - storages := common.InitStorages(db) - state, err := common.OpenProtocolState(lockManager, db, storages) - if err != nil { - return fmt.Errorf("could not open protocol state: %w", err) - } - - results := storages.Results - blocks := storages.Blocks - - root := state.Params().FinalizedRoot() - final, err := state.Final().Head() - if err != nil { - return fmt.Errorf("could not get final header from protocol state: %w", err) - } - - for h := root.Height + 1; h <= final.Height; h++ { - block, err := blocks.ByHeight(h) - if err != nil { - return fmt.Errorf("could not get block at height %d: %w", h, err) - } - - for _, seal := range block.Payload.Seals { - err := results.Index(seal.BlockID, seal.ResultID) - if err != nil { - return fmt.Errorf("could not index result ID at height %d: %w", h, err) - } - } - } - - log.Info().Uint64("start_height", root.Height).Uint64("end_height", final.Height).Msg("indexed execution results") - return nil - }) - }, -} diff --git a/cmd/util/cmd/reindex/cmd/root.go b/cmd/util/cmd/reindex/cmd/root.go deleted file mode 100644 index 7abc84f81f4..00000000000 --- a/cmd/util/cmd/reindex/cmd/root.go +++ /dev/null @@ -1,40 +0,0 @@ -package cmd - -import ( - "fmt" - "os" - - "github.com/spf13/cobra" - "github.com/spf13/viper" - - "github.com/onflow/flow-go/cmd/util/cmd/common" -) - -var ( - flagDatadir string -) - -var rootCmd = &cobra.Command{ - Use: "reindex", - Short: "reindex data", -} - -var RootCmd = rootCmd - -func Execute() { - if err := rootCmd.Execute(); err != nil { - fmt.Println(err) - os.Exit(1) - } -} - -func init() { - common.InitDataDirFlag(rootCmd, &flagDatadir) - _ = rootCmd.MarkPersistentFlagRequired("data-dir") - - cobra.OnInitialize(initConfig) -} - -func initConfig() { - viper.AutomaticEnv() -} diff --git a/cmd/util/cmd/reindex/main.go b/cmd/util/cmd/reindex/main.go deleted file mode 100644 index 5d082d3d072..00000000000 --- a/cmd/util/cmd/reindex/main.go +++ /dev/null @@ -1,7 +0,0 @@ -package main - -import "github.com/onflow/flow-go/cmd/util/cmd/reindex/cmd" - -func main() { - cmd.Execute() -} diff --git a/cmd/util/cmd/root.go b/cmd/util/cmd/root.go index 2fc197470c8..959985d1a56 100644 --- a/cmd/util/cmd/root.go +++ b/cmd/util/cmd/root.go @@ -39,7 +39,6 @@ import ( read_execution_state "github.com/onflow/flow-go/cmd/util/cmd/read-execution-state" read_hotstuff "github.com/onflow/flow-go/cmd/util/cmd/read-hotstuff/cmd" read_protocol_state "github.com/onflow/flow-go/cmd/util/cmd/read-protocol-state/cmd" - index_er "github.com/onflow/flow-go/cmd/util/cmd/reindex/cmd" rollback_executed_height "github.com/onflow/flow-go/cmd/util/cmd/rollback-executed-height/cmd" run_script "github.com/onflow/flow-go/cmd/util/cmd/run-script" "github.com/onflow/flow-go/cmd/util/cmd/snapshot" @@ -111,7 +110,6 @@ func addCommands() { rootCmd.AddCommand(leaders.Cmd) rootCmd.AddCommand(epochs.RootCmd) rootCmd.AddCommand(edbs.RootCmd) - rootCmd.AddCommand(index_er.RootCmd) rootCmd.AddCommand(rollback_executed_height.Cmd) rootCmd.AddCommand(read_execution_state.Cmd) rootCmd.AddCommand(snapshot.Cmd) diff --git a/engine/access/access_test.go b/engine/access/access_test.go index f5620fafe66..3056d36832d 100644 --- a/engine/access/access_test.go +++ b/engine/access/access_test.go @@ -544,6 +544,7 @@ func (suite *Suite) TestGetBlockByIDAndHeight() { func (suite *Suite) TestGetExecutionResultByBlockID() { suite.RunTest(func(handler *rpc.Handler, db storage.DB, all *store.All) { + lockManager := storage.NewTestingLockManager() // test block1 get by ID nonexistingID := unittest.IdentifierFixture() @@ -553,8 +554,16 @@ func (suite *Suite) TestGetExecutionResultByBlockID() { unittest.WithExecutionResultBlockID(blockID), unittest.WithServiceEvents(3)) - require.NoError(suite.T(), all.Results.Store(er)) - require.NoError(suite.T(), all.Results.Index(blockID, er.ID())) + require.NoError(suite.T(), storage.WithLock(lockManager, storage.LockIndexExecutionResult, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + err := all.Results.BatchStore(er, rw) + if err != nil { + return err + } + // requires LockIndexExecutionResult + return all.Results.BatchIndex(lctx, rw, blockID, er.ID()) + }) + })) assertResp := func( resp *accessproto.ExecutionResultForBlockIDResponse, @@ -626,6 +635,7 @@ func (suite *Suite) TestGetExecutionResultByBlockID() { // is reported as sealed func (suite *Suite) TestGetSealedTransaction() { unittest.RunWithPebbleDB(suite.T(), func(pdb *pebble.DB) { + lockManager := storage.NewTestingLockManager() db := pebbleimpl.ToDB(pdb) all := store.InitAll(metrics.NewNoopCollector(), db) enIdentities := unittest.IdentityListFixture(2, unittest.WithRole(flow.RoleExecution)) @@ -740,6 +750,8 @@ func (suite *Suite) TestGetSealedTransaction() { suite.net, suite.state, suite.me, + lockManager, + db, all.Blocks, all.Results, all.Receipts, @@ -816,6 +828,7 @@ func (suite *Suite) TestGetSealedTransaction() { // transaction ID, block ID, and collection ID. func (suite *Suite) TestGetTransactionResult() { unittest.RunWithPebbleDB(suite.T(), func(pdb *pebble.DB) { + lockManager := storage.NewTestingLockManager() db := pebbleimpl.ToDB(pdb) all := store.InitAll(metrics.NewNoopCollector(), db) originID := unittest.IdentifierFixture() @@ -963,6 +976,8 @@ func (suite *Suite) TestGetTransactionResult() { suite.net, suite.state, suite.me, + lockManager, + db, all.Blocks, all.Results, all.Receipts, @@ -1135,6 +1150,7 @@ func (suite *Suite) TestGetTransactionResult() { // the correct block id func (suite *Suite) TestExecuteScript() { unittest.RunWithPebbleDB(suite.T(), func(pdb *pebble.DB) { + lockManager := storage.NewTestingLockManager() db := pebbleimpl.ToDB(pdb) all := store.InitAll(metrics.NewNoopCollector(), db) identities := unittest.IdentityListFixture(2, unittest.WithRole(flow.RoleExecution)) @@ -1229,6 +1245,8 @@ func (suite *Suite) TestExecuteScript() { suite.net, suite.state, suite.me, + lockManager, + db, all.Blocks, all.Results, all.Receipts, diff --git a/engine/access/ingestion/engine.go b/engine/access/ingestion/engine.go index d544b3effd8..2e2a25b4cfc 100644 --- a/engine/access/ingestion/engine.go +++ b/engine/access/ingestion/engine.go @@ -5,6 +5,7 @@ import ( "fmt" "time" + "github.com/jordanschalm/lockctx" "github.com/rs/zerolog" "github.com/onflow/flow-go/consensus/hotstuff/model" @@ -78,6 +79,8 @@ type Engine struct { // storage // FIX: remove direct DB access by substituting indexer module + db storage.DB + lockManager storage.LockManager blocks storage.Blocks executionReceipts storage.ExecutionReceipts maxReceiptHeight uint64 @@ -101,6 +104,8 @@ func New( net network.EngineRegistry, state protocol.State, me module.Local, + lockManager storage.LockManager, + db storage.DB, blocks storage.Blocks, executionResults storage.ExecutionResults, executionReceipts storage.ExecutionReceipts, @@ -133,6 +138,8 @@ func New( log: log.With().Str("engine", "ingestion").Logger(), state: state, me: me, + lockManager: lockManager, + db: db, blocks: blocks, executionResults: executionResults, executionReceipts: executionReceipts, @@ -372,19 +379,32 @@ func (e *Engine) processFinalizedBlock(block *flow.Block) error { // TODO: substitute an indexer module as layer between engine and storage // index the block storage with each of the collection guarantee - err := e.blocks.IndexBlockContainingCollectionGuarantees(block.ID(), flow.GetIDs(block.Payload.Guarantees)) + err := storage.WithLocks(e.lockManager, []string{ + storage.LockIndexCollectionsByBlock, + storage.LockIndexExecutionResult, + }, func(lctx lockctx.Context) error { + return e.db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + // requires [storage.LockIndexCollectionsByBlock] lock + err := e.blocks.BatchIndexBlockContainingCollectionGuarantees(lctx, rw, block.ID(), flow.GetIDs(block.Payload.Guarantees)) + if err != nil { + return fmt.Errorf("could not index block for collections: %w", err) + } + + // loop through seals and index ID -> result ID + for _, seal := range block.Payload.Seals { + // requires [storage.LockIndexExecutionResult] lock + err := e.executionResults.BatchIndex(lctx, rw, seal.BlockID, seal.ResultID) + if err != nil { + return fmt.Errorf("could not index block for execution result: %w", err) + } + } + return nil + }) + }) if err != nil { return fmt.Errorf("could not index block for collections: %w", err) } - // loop through seals and index ID -> result ID - for _, seal := range block.Payload.Seals { - err := e.executionResults.Index(seal.BlockID, seal.ResultID) - if err != nil { - return fmt.Errorf("could not index block for execution result: %w", err) - } - } - e.collectionSyncer.RequestCollectionsForBlock(block.Height, block.Payload.Guarantees) e.collectionExecutedMetric.BlockFinalized(block) diff --git a/engine/access/ingestion/engine_test.go b/engine/access/ingestion/engine_test.go index ece7712446a..e3f6e18f944 100644 --- a/engine/access/ingestion/engine_test.go +++ b/engine/access/ingestion/engine_test.go @@ -131,6 +131,7 @@ func (s *Suite) SetupTest() { s.receipts = new(storagemock.ExecutionReceipts) s.transactions = new(storagemock.Transactions) s.results = new(storagemock.ExecutionResults) + s.results.On("BatchIndex", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() collectionsToMarkFinalized := stdmap.NewTimes(100) collectionsToMarkExecuted := stdmap.NewTimes(100) blocksToMarkExecuted := stdmap.NewTimes(100) @@ -213,6 +214,8 @@ func (s *Suite) initEngineAndSyncer(ctx irrecoverable.SignalerContext) (*Engine, s.net, s.proto.state, s.me, + s.lockManager, + s.db, s.blocks, s.results, s.receipts, @@ -294,7 +297,7 @@ func (s *Suite) TestOnFinalizedBlockSingle() { } // expect that the block storage is indexed with each of the collection guarantee - s.blocks.On("IndexBlockContainingCollectionGuarantees", block.ID(), []flow.Identifier(flow.GetIDs(block.Payload.Guarantees))).Return(nil).Once() + s.blocks.On("BatchIndexBlockContainingCollectionGuarantees", mock.Anything, mock.Anything, block.ID(), []flow.Identifier(flow.GetIDs(block.Payload.Guarantees))).Return(nil).Once() for _, seal := range block.Payload.Seals { s.results.On("Index", seal.BlockID, seal.ResultID).Return(nil).Once() } @@ -321,7 +324,7 @@ func (s *Suite) TestOnFinalizedBlockSingle() { // assert that the block was retrieved and all collections were requested s.headers.AssertExpectations(s.T()) s.request.AssertNumberOfCalls(s.T(), "EntityByID", len(block.Payload.Guarantees)) - s.results.AssertNumberOfCalls(s.T(), "Index", len(block.Payload.Seals)) + s.results.AssertNumberOfCalls(s.T(), "BatchIndex", len(block.Payload.Seals)) } // TestOnFinalizedBlockSeveralBlocksAhead checks OnFinalizedBlock with a block several blocks newer than the last block processed @@ -370,7 +373,7 @@ func (s *Suite) TestOnFinalizedBlockSeveralBlocksAhead() { // expected all new blocks after last block processed for _, block := range blocks { - s.blocks.On("IndexBlockContainingCollectionGuarantees", block.ID(), []flow.Identifier(flow.GetIDs(block.Payload.Guarantees))).Return(nil).Once() + s.blocks.On("BatchIndexBlockContainingCollectionGuarantees", mock.Anything, mock.Anything, block.ID(), []flow.Identifier(flow.GetIDs(block.Payload.Guarantees))).Return(nil).Once() for _, cg := range block.Payload.Guarantees { s.request.On("EntityByID", cg.CollectionID, mock.Anything).Return().Run(func(args mock.Arguments) { @@ -398,9 +401,9 @@ func (s *Suite) TestOnFinalizedBlockSeveralBlocksAhead() { } s.headers.AssertExpectations(s.T()) - s.blocks.AssertNumberOfCalls(s.T(), "IndexBlockContainingCollectionGuarantees", newBlocksCount) + s.blocks.AssertNumberOfCalls(s.T(), "BatchIndexBlockContainingCollectionGuarantees", newBlocksCount) s.request.AssertNumberOfCalls(s.T(), "EntityByID", expectedEntityByIDCalls) - s.results.AssertNumberOfCalls(s.T(), "Index", expectedIndexCalls) + s.results.AssertNumberOfCalls(s.T(), "BatchIndex", expectedIndexCalls) } // TestOnCollection checks that when a Collection is received, it is persisted diff --git a/engine/access/ingestion2/collection_syncer.go b/engine/access/ingestion2/collection_syncer.go index 397c135b56a..ad055b60696 100644 --- a/engine/access/ingestion2/collection_syncer.go +++ b/engine/access/ingestion2/collection_syncer.go @@ -189,12 +189,12 @@ func (s *CollectionSyncer) StartWorkerLoop(ctx irrecoverable.SignalerContext, re // Create a lock context for indexing lctx := s.lockManager.NewContext() + defer lctx.Release() err := lctx.AcquireLock(storage.LockInsertCollection) if err != nil { ctx.Throw(fmt.Errorf("could not acquire lock for collection indexing: %w", err)) return } - defer lctx.Release() err = indexer.IndexCollection(lctx, collection, s.collections, s.logger, s.collectionExecutedMetric) if err != nil { diff --git a/engine/access/ingestion2/engine_test.go b/engine/access/ingestion2/engine_test.go index c7813d7b028..20ac8196343 100644 --- a/engine/access/ingestion2/engine_test.go +++ b/engine/access/ingestion2/engine_test.go @@ -131,6 +131,7 @@ func (s *Suite) SetupTest() { s.receipts = new(storagemock.ExecutionReceipts) s.transactions = new(storagemock.Transactions) s.results = new(storagemock.ExecutionResults) + s.results.On("BatchIndex", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() collectionsToMarkFinalized := stdmap.NewTimes(100) collectionsToMarkExecuted := stdmap.NewTimes(100) blocksToMarkExecuted := stdmap.NewTimes(100) @@ -225,6 +226,8 @@ func (s *Suite) initEngineAndSyncer(ctx irrecoverable.SignalerContext) (*Engine, blockProcessor, err := NewFinalizedBlockProcessor( s.log, s.proto.state, + s.lockManager, + s.db, s.blocks, s.results, processedHeightInitializer, @@ -314,7 +317,7 @@ func (s *Suite) TestOnFinalizedBlockSingle() { } // expect that the block storage is indexed with each of the collection guarantee - s.blocks.On("IndexBlockContainingCollectionGuarantees", block.ID(), []flow.Identifier(flow.GetIDs(block.Payload.Guarantees))).Return(nil).Once() + s.blocks.On("BatchIndexBlockContainingCollectionGuarantees", mock.Anything, mock.Anything, block.ID(), []flow.Identifier(flow.GetIDs(block.Payload.Guarantees))).Return(nil).Once() for _, seal := range block.Payload.Seals { s.results.On("Index", seal.BlockID, seal.ResultID).Return(nil).Once() } @@ -338,7 +341,7 @@ func (s *Suite) TestOnFinalizedBlockSingle() { // assert that the block was retrieved and all collections were requested s.headers.AssertExpectations(s.T()) s.request.AssertNumberOfCalls(s.T(), "EntityByID", len(block.Payload.Guarantees)) - s.results.AssertNumberOfCalls(s.T(), "Index", len(block.Payload.Seals)) + s.results.AssertNumberOfCalls(s.T(), "BatchIndex", len(block.Payload.Seals)) } // TestOnFinalizedBlockSeveralBlocksAhead checks OnFinalizedBlock with a block several blocks newer than the last block processed @@ -387,7 +390,7 @@ func (s *Suite) TestOnFinalizedBlockSeveralBlocksAhead() { // expected all new blocks after last block processed for _, block := range blocks { - s.blocks.On("IndexBlockContainingCollectionGuarantees", block.ID(), []flow.Identifier(flow.GetIDs(block.Payload.Guarantees))).Return(nil).Once() + s.blocks.On("BatchIndexBlockContainingCollectionGuarantees", mock.Anything, mock.Anything, block.ID(), []flow.Identifier(flow.GetIDs(block.Payload.Guarantees))).Return(nil).Once() for _, cg := range block.Payload.Guarantees { s.request.On("EntityByID", cg.CollectionID, mock.Anything).Return().Run(func(args mock.Arguments) { @@ -412,9 +415,9 @@ func (s *Suite) TestOnFinalizedBlockSeveralBlocksAhead() { } s.headers.AssertExpectations(s.T()) - s.blocks.AssertNumberOfCalls(s.T(), "IndexBlockContainingCollectionGuarantees", newBlocksCount) + s.blocks.AssertNumberOfCalls(s.T(), "BatchIndexBlockContainingCollectionGuarantees", newBlocksCount) s.request.AssertNumberOfCalls(s.T(), "EntityByID", expectedEntityByIDCalls) - s.results.AssertNumberOfCalls(s.T(), "Index", expectedIndexCalls) + s.results.AssertNumberOfCalls(s.T(), "BatchIndex", expectedIndexCalls) } // TestOnCollection checks that when a Collection is received, it is persisted diff --git a/engine/access/ingestion2/finalized_block_processor.go b/engine/access/ingestion2/finalized_block_processor.go index 78511dd9803..cb352c0d75b 100644 --- a/engine/access/ingestion2/finalized_block_processor.go +++ b/engine/access/ingestion2/finalized_block_processor.go @@ -3,6 +3,7 @@ package ingestion2 import ( "fmt" + "github.com/jordanschalm/lockctx" "github.com/rs/zerolog" "github.com/onflow/flow-go/engine" @@ -45,7 +46,10 @@ type FinalizedBlockProcessor struct { consumer *jobqueue.ComponentConsumer consumerNotifier engine.Notifier - blocks storage.Blocks + lockManager storage.LockManager + db storage.DB + + blocks storage.Blocks executionResults storage.ExecutionResults @@ -60,6 +64,8 @@ type FinalizedBlockProcessor struct { func NewFinalizedBlockProcessor( log zerolog.Logger, state protocol.State, + lockManager storage.LockManager, + db storage.DB, blocks storage.Blocks, executionResults storage.ExecutionResults, finalizedProcessedHeight storage.ConsumerProgressInitializer, @@ -75,6 +81,8 @@ func NewFinalizedBlockProcessor( consumerNotifier := engine.NewNotifier() processor := &FinalizedBlockProcessor{ log: log, + db: db, + lockManager: lockManager, blocks: blocks, executionResults: executionResults, consumerNotifier: consumerNotifier, @@ -145,17 +153,28 @@ func (p *FinalizedBlockProcessor) processFinalizedBlockJobCallback( // // No errors are expected during normal operations. func (p *FinalizedBlockProcessor) indexFinalizedBlock(block *flow.Block) error { - err := p.blocks.IndexBlockContainingCollectionGuarantees(block.ID(), flow.GetIDs(block.Payload.Guarantees)) + err := storage.WithLocks(p.lockManager, + []string{storage.LockIndexCollectionsByBlock, storage.LockIndexExecutionResult}, func(lctx lockctx.Context) error { + return p.db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + // require storage.LockIndexCollectionsByBlock + err := p.blocks.BatchIndexBlockContainingCollectionGuarantees(lctx, rw, block.ID(), flow.GetIDs(block.Payload.Guarantees)) + if err != nil { + return fmt.Errorf("could not index block for collections: %w", err) + } + + // loop through seals and index ID -> result ID + for _, seal := range block.Payload.Seals { + // require storage.LockIndexExecutionResult + err := p.executionResults.BatchIndex(lctx, rw, seal.BlockID, seal.ResultID) + if err != nil { + return fmt.Errorf("could not index block for execution result: %w", err) + } + } + return nil + }) + }) if err != nil { - return fmt.Errorf("could not index block for collections: %w", err) - } - - // loop through seals and index ID -> result ID - for _, seal := range block.Payload.Seals { - err := p.executionResults.Index(seal.BlockID, seal.ResultID) - if err != nil { - return fmt.Errorf("could not index block for execution result: %w", err) - } + return fmt.Errorf("could not index execution results: %w", err) } p.collectionSyncer.RequestCollectionsForBlock(block.Height, block.Payload.Guarantees) diff --git a/engine/execution/pruner/core_test.go b/engine/execution/pruner/core_test.go index 16d1eaa5540..36cfd6195ef 100644 --- a/engine/execution/pruner/core_test.go +++ b/engine/execution/pruner/core_test.go @@ -73,8 +73,17 @@ func TestLoopPruneExecutionDataFromRootToLatestSealed(t *testing.T) { }) }) require.NoError(t, err) - require.NoError(t, results.Store(chunk.Result)) - require.NoError(t, results.Index(chunk.Result.BlockID, chunk.Result.ID())) + err = unittest.WithLock(t, lockManager, storage.LockIndexExecutionResult, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + err := results.BatchStore(chunk.Result, rw) + require.NoError(t, err) + + err = results.BatchIndex(lctx, rw, chunk.Result.BlockID, chunk.Result.ID()) + require.NoError(t, err) + return nil + }) + }) + require.NoError(t, err) require.NoError(t, unittest.WithLock(t, lockManager, storage.LockInsertChunkDataPack, func(lctx lockctx.Context) error { return chunkDataPacks.StoreByChunkID(lctx, []*flow.ChunkDataPack{chunk.ChunkDataPack}) })) diff --git a/engine/execution/state/bootstrap/bootstrap.go b/engine/execution/state/bootstrap/bootstrap.go index 19305dc257d..b129edefaa2 100644 --- a/engine/execution/state/bootstrap/bootstrap.go +++ b/engine/execution/state/bootstrap/bootstrap.go @@ -100,7 +100,11 @@ func (b *Bootstrapper) BootstrapExecutionDatabase( lctx := manager.NewContext() defer lctx.Release() - err := lctx.AcquireLock(storage.LockInsertOwnReceipt) + err := lctx.AcquireLock(storage.LockIndexExecutionResult) + if err != nil { + return err + } + err = lctx.AcquireLock(storage.LockIndexStateCommitment) if err != nil { return err } @@ -113,7 +117,7 @@ func (b *Bootstrapper) BootstrapExecutionDatabase( return fmt.Errorf("could not index initial genesis execution block: %w", err) } - err = operation.IndexExecutionResult(w, rootSeal.BlockID, rootSeal.ResultID) + err = operation.IndexOwnOrSealedExecutionResult(lctx, rw, rootSeal.BlockID, rootSeal.ResultID) if err != nil { return fmt.Errorf("could not index result for root result: %w", err) } diff --git a/engine/execution/state/state.go b/engine/execution/state/state.go index 47a7accb8fc..967ad471511 100644 --- a/engine/execution/state/state.go +++ b/engine/execution/state/state.go @@ -356,11 +356,11 @@ func (s *state) GetExecutionResultID(ctx context.Context, blockID flow.Identifie span, _ := s.tracer.StartSpanFromContext(ctx, trace.EXEGetExecutionResultID) defer span.End() - result, err := s.results.ByBlockID(blockID) + resultID, err := s.results.IDByBlockID(blockID) if err != nil { return flow.ZeroID, err } - return result.ID(), nil + return resultID, nil } func (s *state) SaveExecutionResults( @@ -410,8 +410,17 @@ func (s *state) saveExecutionResults( return fmt.Errorf("can not retrieve chunk data packs: %w", err) } - // Acquire both locks to ensure it's concurrent safe when inserting the execution results and chunk data packs. - return storage.WithLocks(s.lockManager, []string{storage.LockInsertOwnReceipt, storage.LockInsertChunkDataPack}, func(lctx lockctx.Context) error { + locks := []string{ + storage.LockInsertChunkDataPack, + storage.LockInsertEvent, + storage.LockInsertServiceEvent, + storage.LockInsertAndIndexTxResult, + storage.LockInsertOwnReceipt, + storage.LockIndexExecutionResult, + storage.LockIndexStateCommitment, + } + // Acquire locks to ensure it's concurrent safe when inserting the execution results and chunk data packs. + return storage.WithLocks(s.lockManager, locks, func(lctx lockctx.Context) error { err := s.chunkDataPacks.StoreByChunkID(lctx, chunks) if err != nil { return fmt.Errorf("can not store multiple chunk data pack: %w", err) @@ -419,7 +428,7 @@ func (s *state) saveExecutionResults( // Save entire execution result (including all chunk data packs) within one batch to minimize // the number of database interactions. - return s.db.WithReaderBatchWriter(func(batch storage.ReaderBatchWriter) error { + return storage.SkipAlreadyExistsError(s.db.WithReaderBatchWriter(func(batch storage.ReaderBatchWriter) error { batch.AddCallback(func(err error) { // Rollback if an error occurs during batch operations // Chunk data packs are saved in a separate database, there is a chance @@ -436,32 +445,39 @@ func (s *state) saveExecutionResults( } }) - err = s.events.BatchStore(blockID, []flow.EventsList{result.AllEvents()}, batch) + // require LockInsertEvent + err = s.events.BatchStore(lctx, blockID, []flow.EventsList{result.AllEvents()}, batch) if err != nil { return fmt.Errorf("cannot store events: %w", err) } - err = s.serviceEvents.BatchStore(blockID, result.AllServiceEvents(), batch) + // require LockInsertServiceEvent + err = s.serviceEvents.BatchStore(lctx, blockID, result.AllServiceEvents(), batch) if err != nil { return fmt.Errorf("cannot store service events: %w", err) } + // require LockInsertAndIndexTxResult err = s.transactionResults.BatchStore( + lctx, + batch, blockID, result.AllTransactionResults(), - batch) + ) if err != nil { return fmt.Errorf("cannot store transaction result: %w", err) } executionResult := &result.ExecutionReceipt.ExecutionResult + // require [storage.LockInsertOwnReceipt] lock // saving my receipts will also save the execution result err = s.myReceipts.BatchStoreMyReceipt(lctx, result.ExecutionReceipt, batch) if err != nil { return fmt.Errorf("could not persist execution result: %w", err) } - err = s.results.BatchIndex(blockID, executionResult.ID(), batch) + // require [storage.LockIndexExecutionResult] lock + err = s.results.BatchIndex(lctx, batch, blockID, executionResult.ID()) if err != nil { return fmt.Errorf("cannot index execution result: %w", err) } @@ -469,13 +485,14 @@ func (s *state) saveExecutionResults( // the state commitment is the last data item to be stored, so that // IsBlockExecuted can be implemented by checking whether state commitment exists // in the database + // require [storage.LockIndexStateCommitment] lock err = s.commits.BatchStore(lctx, blockID, result.CurrentEndState(), batch) if err != nil { return fmt.Errorf("cannot store state commitment: %w", err) } return nil - }) + })) }) } diff --git a/engine/execution/state/state_storehouse_test.go b/engine/execution/state/state_storehouse_test.go index 59b618e4031..3078cd3a329 100644 --- a/engine/execution/state/state_storehouse_test.go +++ b/engine/execution/state/state_storehouse_test.go @@ -55,17 +55,17 @@ func prepareStorehouseTest(f func(t *testing.T, es state.ExecutionState, l *ledg headers := storagemock.NewHeaders(t) blocks := storagemock.NewBlocks(t) events := storagemock.NewEvents(t) - events.On("BatchStore", mock.Anything, mock.Anything, mock.Anything).Return(nil) + events.On("BatchStore", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) serviceEvents := storagemock.NewServiceEvents(t) - serviceEvents.On("BatchStore", mock.Anything, mock.Anything, mock.Anything).Return(nil) + serviceEvents.On("BatchStore", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) txResults := storagemock.NewTransactionResults(t) - txResults.On("BatchStore", mock.Anything, mock.Anything, mock.Anything).Return(nil) + txResults.On("BatchStore", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) chunkDataPacks := storagemock.NewChunkDataPacks(t) chunkDataPacks.On("StoreByChunkID", mock.Anything, mock.Anything).Return(nil) results := storagemock.NewExecutionResults(t) - results.On("BatchIndex", mock.Anything, mock.Anything, mock.Anything).Return(nil) + results.On("BatchIndex", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) myReceipts := storagemock.NewMyExecutionReceipts(t) - myReceipts.On("BatchStoreMyReceipt", mock.Anything, mock.Anything, mock.Anything).Return(nil) + myReceipts.On("BatchStoreMyReceipt", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) withRegisterStore(t, func(t *testing.T, rs *storehouse.RegisterStore, diff --git a/module/builder/consensus/builder_test.go b/module/builder/consensus/builder_test.go index 71671b4222b..3c372c7d149 100644 --- a/module/builder/consensus/builder_test.go +++ b/module/builder/consensus/builder_test.go @@ -256,7 +256,7 @@ func (bs *BuilderSuite) SetupTest() { // insert finalized height and root height db := bs.db - err := unittest.WithLocks(bs.T(), lockManager, []string{storage.LockFinalizeBlock, storage.LockBootstrapping}, func(lctx lockctx.Context) error { + err := unittest.WithLocks(bs.T(), lockManager, []string{storage.LockInsertInstanceParams, storage.LockIndexExecutionResult, storage.LockInsertBlock, storage.LockFinalizeBlock}, func(lctx lockctx.Context) error { return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { enc, err := datastore.NewVersionedInstanceParams( datastore.DefaultInstanceParamsVersion, diff --git a/module/executiondatasync/optimistic_sync/core_impl_test.go b/module/executiondatasync/optimistic_sync/core_impl_test.go index 19b87866603..008fc9aa51b 100644 --- a/module/executiondatasync/optimistic_sync/core_impl_test.go +++ b/module/executiondatasync/optimistic_sync/core_impl_test.go @@ -445,9 +445,9 @@ 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.persistentEvents.On("BatchStore", mock.Anything, 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.persistentResults.On("BatchStore", mock.Anything, 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) @@ -499,7 +499,7 @@ func (c *CoreImplSuite) TestCoreImpl_Persist() { indexerData := core.workingData.indexerData c.persistentRegisters.On("Store", flow.RegisterEntries(indexerData.Registers), tf.block.Height).Return(nil).Once() - c.persistentEvents.On("BatchStore", blockID, []flow.EventsList{indexerData.Events}, mock.Anything).Return(expectedErr).Once() + c.persistentEvents.On("BatchStore", mock.Anything, blockID, []flow.EventsList{indexerData.Events}, mock.Anything).Return(expectedErr).Once() err = core.Persist() c.ErrorIs(err, expectedErr) diff --git a/module/executiondatasync/optimistic_sync/persisters/block.go b/module/executiondatasync/optimistic_sync/persisters/block.go index 371ed98b149..97089fc22ca 100644 --- a/module/executiondatasync/optimistic_sync/persisters/block.go +++ b/module/executiondatasync/optimistic_sync/persisters/block.go @@ -61,11 +61,21 @@ func (p *BlockPersister) Persist() error { start := time.Now() lctx := p.lockManager.NewContext() + defer lctx.Release() 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 = lctx.AcquireLock(storage.LockInsertEvent) + if err != nil { + return fmt.Errorf("could not acquire lock for inserting events: %w", err) + } + + err = lctx.AcquireLock(storage.LockInsertLightTransactionResult) + if err != nil { + return fmt.Errorf("could not acquire lock for inserting light transaction results: %w", err) + } err = p.protocolDB.WithReaderBatchWriter(func(batch storage.ReaderBatchWriter) error { for _, persister := range p.persisterStores { diff --git a/module/executiondatasync/optimistic_sync/persisters/block_test.go b/module/executiondatasync/optimistic_sync/persisters/block_test.go index c09b7bbb13c..4c49f1128f2 100644 --- a/module/executiondatasync/optimistic_sync/persisters/block_test.go +++ b/module/executiondatasync/optimistic_sync/persisters/block_test.go @@ -182,7 +182,7 @@ func (p *PersisterSuite) TestPersister_ErrorHandling() { Times(len(p.indexerData.Collections)) events := storagemock.NewEvents(p.T()) - events.On("BatchStore", p.executionResult.BlockID, mock.Anything, mock.Anything).Return(expectedErr).Once() + events.On("BatchStore", mock.Anything, p.executionResult.BlockID, mock.Anything, mock.Anything).Return(expectedErr).Once() persister := NewBlockPersister( unittest.Logger(), diff --git a/module/executiondatasync/optimistic_sync/persisters/stores/events.go b/module/executiondatasync/optimistic_sync/persisters/stores/events.go index cb7224c9096..b048f7aedc6 100644 --- a/module/executiondatasync/optimistic_sync/persisters/stores/events.go +++ b/module/executiondatasync/optimistic_sync/persisters/stores/events.go @@ -33,8 +33,8 @@ func NewEventsStore( // Persist adds events to the batch. // // No error returns are expected during normal operations -func (e *EventsStore) Persist(_ lockctx.Proof, batch storage.ReaderBatchWriter) error { - if err := e.persistedEvents.BatchStore(e.blockID, []flow.EventsList{e.data}, batch); err != nil { +func (e *EventsStore) Persist(lctx lockctx.Proof, batch storage.ReaderBatchWriter) error { + if err := e.persistedEvents.BatchStore(lctx, e.blockID, []flow.EventsList{e.data}, batch); err != nil { return fmt.Errorf("could not add events to batch: %w", err) } return nil diff --git a/module/executiondatasync/optimistic_sync/persisters/stores/results.go b/module/executiondatasync/optimistic_sync/persisters/stores/results.go index 7cb9770c1a9..6b36d1c207b 100644 --- a/module/executiondatasync/optimistic_sync/persisters/stores/results.go +++ b/module/executiondatasync/optimistic_sync/persisters/stores/results.go @@ -33,8 +33,8 @@ func NewResultsStore( // Persist adds results to the batch. // // No error returns are expected during normal operations -func (r *ResultsStore) Persist(_ lockctx.Proof, batch storage.ReaderBatchWriter) error { - if err := r.persistedResults.BatchStore(r.blockID, r.data, batch); err != nil { +func (r *ResultsStore) Persist(lctx lockctx.Proof, batch storage.ReaderBatchWriter) error { + if err := r.persistedResults.BatchStore(lctx, r.blockID, r.data, batch); err != nil { return fmt.Errorf("could not add transaction results to batch: %w", err) } return nil diff --git a/module/executiondatasync/optimistic_sync/pipeline_functional_test.go b/module/executiondatasync/optimistic_sync/pipeline_functional_test.go index 84364b0e7bd..9d9fd48c315 100644 --- a/module/executiondatasync/optimistic_sync/pipeline_functional_test.go +++ b/module/executiondatasync/optimistic_sync/pipeline_functional_test.go @@ -154,10 +154,16 @@ func (p *PipelineFunctionalSuite) SetupTest() { require.NoError(t, err) // Store and index sealed block execution result - err = p.results.Store(sealedExecutionResult) - p.Require().NoError(err) + err = unittest.WithLock(t, p.lockManager, storage.LockIndexExecutionResult, func(lctx lockctx.Context) error { + return p.db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + err := p.results.BatchStore(sealedExecutionResult, rw) + p.Require().NoError(err) - err = p.results.Index(sealedBlock.ID(), sealedExecutionResult.ID()) + err = p.results.BatchIndex(lctx, rw, sealedBlock.ID(), sealedExecutionResult.ID()) + p.Require().NoError(err) + return nil + }) + }) p.Require().NoError(err) p.persistentLatestSealedResult, err = store.NewLatestPersistedSealedResult(p.consumerProgress, p.headers, p.results) @@ -231,6 +237,14 @@ func (p *PipelineFunctionalSuite) TestPipelineCompletesSuccessfully() { p.txResultErrMsgsRequester.On("Request", mock.Anything).Return(p.expectedTxResultErrMsgs, nil).Once() p.WithRunningPipeline(func(pipeline Pipeline, updateChan chan State, errChan chan error, cancel context.CancelFunc) { + // Check for errors in a separate goroutine + go func() { + err := <-errChan + if err != nil { + p.T().Errorf("Pipeline error: %v", err) + } + }() + pipeline.OnParentStateUpdated(StateComplete) waitForStateUpdates(p.T(), updateChan, errChan, StateProcessing, StateWaitingPersist) @@ -343,7 +357,7 @@ func (p *PipelineFunctionalSuite) TestPipelinePersistingError() { // Mock events storage to simulate an error on a persisting step. In normal flow and with real storages, // it is hard to make a meaningful error explicitly. mockEvents := storagemock.NewEvents(p.T()) - mockEvents.On("BatchStore", mock.Anything, mock.Anything, mock.Anything).Return(expectedError).Once() + mockEvents.On("BatchStore", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(expectedError).Once() p.persistentEvents = mockEvents p.execDataRequester.On("RequestExecutionData", mock.Anything).Return(p.expectedExecutionData, nil).Once() diff --git a/module/pruner/pruners/chunk_data_pack_test.go b/module/pruner/pruners/chunk_data_pack_test.go index f595ed30bcd..d8ab773c6bd 100644 --- a/module/pruner/pruners/chunk_data_pack_test.go +++ b/module/pruner/pruners/chunk_data_pack_test.go @@ -30,7 +30,10 @@ func TestChunkDataPackPruner(t *testing.T) { // store the chunks cdp1, result1 := unittest.ChunkDataPacksFixtureAndResult() - require.NoError(t, results.Store(result1)) + err := db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return results.BatchStore(result1, rw) + }) + require.NoError(t, err) require.NoError(t, unittest.WithLock(t, lockManager, storage.LockInsertChunkDataPack, func(lctx lockctx.Context) error { return chunks.StoreByChunkID(lctx, cdp1) })) @@ -43,7 +46,7 @@ func TestChunkDataPackPruner(t *testing.T) { })) // verify they are pruned - _, err := chunks.ByChunkID(cdp1[0].ChunkID) + _, err = chunks.ByChunkID(cdp1[0].ChunkID) require.True(t, errors.Is(err, storage.ErrNotFound), fmt.Errorf("expected ErrNotFound but got %v", err)) // prune again should not return error diff --git a/module/state_synchronization/indexer/indexer_core.go b/module/state_synchronization/indexer/indexer_core.go index ef768840a7a..1e77d70756c 100644 --- a/module/state_synchronization/indexer/indexer_core.go +++ b/module/state_synchronization/indexer/indexer_core.go @@ -148,18 +148,24 @@ 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 = 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 - }) + err := storage.WithLocks(c.lockManager, + []string{storage.LockInsertEvent, storage.LockInsertLightTransactionResult}, + func(lctx lockctx.Context) error { + return c.protocolDB.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + // Needs storage.LockInsertEvent + err := c.events.BatchStore(lctx, data.BlockID, []flow.EventsList{events}, rw) + if err != nil { + return fmt.Errorf("could not index events at height %d: %w", header.Height, err) + } + + // Needs storage.LockInsertLightTransactionResult + err = c.results.BatchStore(lctx, data.BlockID, results, rw) + if err != nil { + return fmt.Errorf("could not index transaction results at height %d: %w", header.Height, err) + } + return nil + }) + }) if err != nil { return fmt.Errorf("could not commit block data: %w", err) diff --git a/module/state_synchronization/indexer/indexer_core_test.go b/module/state_synchronization/indexer/indexer_core_test.go index a1ccd0c3639..6b9685024c1 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" @@ -131,8 +132,8 @@ func (i *indexCoreTest) setStoreRegisters(f func(t *testing.T, entries flow.Regi func (i *indexCoreTest) setStoreEvents(f func(*testing.T, flow.Identifier, []flow.EventsList) error) *indexCoreTest { i.events. - On("BatchStore", mock.AnythingOfType("flow.Identifier"), mock.AnythingOfType("[]flow.EventsList"), mock.Anything). - Return(func(blockID flow.Identifier, events []flow.EventsList, batch storage.ReaderBatchWriter) error { + On("BatchStore", mock.AnythingOfType("*lockctx.context"), mock.AnythingOfType("flow.Identifier"), mock.AnythingOfType("[]flow.EventsList"), mock.Anything). + Return(func(lctx lockctx.Proof, blockID flow.Identifier, events []flow.EventsList, batch storage.ReaderBatchWriter) error { require.NotNil(i.t, batch) return f(i.t, blockID, events) }) @@ -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.AnythingOfType("*lockctx.context"), mock.AnythingOfType("flow.Identifier"), mock.AnythingOfType("[]flow.LightTransactionResult"), mock.Anything). + Return(func(lctx lockctx.Proof, blockID flow.Identifier, results []flow.LightTransactionResult, batch storage.ReaderBatchWriter) error { require.NotNil(i.t, batch) return f(i.t, blockID, results) }) @@ -168,14 +169,14 @@ func (i *indexCoreTest) useDefaultStorageMocks() *indexCoreTest { func (i *indexCoreTest) useDefaultEvents() *indexCoreTest { i.events. - On("BatchStore", mock.AnythingOfType("flow.Identifier"), mock.AnythingOfType("[]flow.EventsList"), mock.Anything). + On("BatchStore", mock.AnythingOfType("*lockctx.context"), mock.AnythingOfType("flow.Identifier"), mock.AnythingOfType("[]flow.EventsList"), mock.Anything). Return(nil) return i } func (i *indexCoreTest) useDefaultTransactionResults() *indexCoreTest { i.results. - On("BatchStore", mock.AnythingOfType("flow.Identifier"), mock.AnythingOfType("[]flow.LightTransactionResult"), mock.Anything). + On("BatchStore", mock.AnythingOfType("*lockctx.context"), mock.AnythingOfType("flow.Identifier"), mock.AnythingOfType("[]flow.LightTransactionResult"), mock.Anything). Return(nil) return i } diff --git a/state/cluster/badger/mutator.go b/state/cluster/badger/mutator.go index 13f27738f9f..6fa82116b85 100644 --- a/state/cluster/badger/mutator.go +++ b/state/cluster/badger/mutator.go @@ -129,11 +129,11 @@ func (m *MutableState) Extend(proposal *cluster.Proposal) error { } lctx := m.lockManager.NewContext() + defer lctx.Release() err = lctx.AcquireLock(storage.LockInsertOrFinalizeClusterBlock) if err != nil { return fmt.Errorf("could not acquire lock for inserting cluster block: %w", err) } - defer lctx.Release() span, _ = m.tracer.StartSpanFromContext(ctx, trace.COLClusterStateMutatorExtendCheckTransactionsValid) err = m.checkPayloadTransactions(lctx, extendCtx) diff --git a/state/protocol/badger/state.go b/state/protocol/badger/state.go index 249b0e917d8..877cde310cd 100644 --- a/state/protocol/badger/state.go +++ b/state/protocol/badger/state.go @@ -112,19 +112,22 @@ func Bootstrap( // trusted root snapshot are presumed to be finalized) lctx := lockManager.NewContext() defer lctx.Release() - err := lctx.AcquireLock(storage.LockInsertBlock) + err := lctx.AcquireLock(storage.LockInsertInstanceParams) if err != nil { return nil, err } - err = lctx.AcquireLock(storage.LockFinalizeBlock) + err = lctx.AcquireLock(storage.LockIndexExecutionResult) if err != nil { return nil, err } - err = lctx.AcquireLock(storage.LockBootstrapping) + err = lctx.AcquireLock(storage.LockInsertBlock) + if err != nil { + return nil, err + } + err = lctx.AcquireLock(storage.LockFinalizeBlock) if err != nil { return nil, err } - config := defaultBootstrapConfig() for _, opt := range options { opt(config) @@ -285,6 +288,8 @@ func bootstrapProtocolState( // segment, as it may or may not be included in SealingSegment.Blocks depending on how much // history is covered. The spork root block is persisted as a root proposal without proposer // signature (by convention). +// +// It requires [storage.LockIndexExecutionResult] lock func bootstrapSealingSegment( lctx lockctx.Proof, db storage.DB, @@ -298,11 +303,11 @@ func bootstrapSealingSegment( err := db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { w := rw.Writer() for _, result := range segment.ExecutionResults { - err := operation.InsertExecutionResult(w, result) + err := operation.InsertExecutionResult(w, result.ID(), result) if err != nil { return fmt.Errorf("could not insert execution result: %w", err) } - err = operation.IndexExecutionResult(w, result.BlockID, result.ID()) + err = operation.IndexOwnOrSealedExecutionResult(lctx, rw, result.BlockID, result.ID()) if err != nil { return fmt.Errorf("could not index execution result: %w", err) } @@ -461,7 +466,7 @@ func bootstrapSealingSegment( // If the sealed root block is different from the finalized root block, then it means the node dynamically // bootstrapped. In that case, we index the result of the latest sealed result, so that the EN is able // to confirm that it is loading the correct state to execute the next block. - err = operation.IndexExecutionResult(rw.Writer(), rootSeal.BlockID, rootSeal.ResultID) + err = operation.IndexOwnOrSealedExecutionResult(lctx, rw, rootSeal.BlockID, rootSeal.ResultID) if err != nil { return fmt.Errorf("could not index root result: %w", err) } diff --git a/storage/blocks.go b/storage/blocks.go index ad66f7f8b3b..714a481ddc3 100644 --- a/storage/blocks.go +++ b/storage/blocks.go @@ -87,8 +87,9 @@ type Blocks interface { // to decode an existing database value ByCollectionID(collID flow.Identifier) (*flow.Block, error) - // IndexBlockContainingCollectionGuarantees populates an index `guaranteeID->blockID` for each guarantee - // which appears in the block. + // BatchIndexBlockContainingCollectionGuarantees produces mappings from the IDs of [flow.CollectionGuarantee]s to the block ID containing these guarantees. + // The caller must acquire a storage.LockIndexCollectionsByBlock lock. + // // CAUTION: a collection can be included in multiple *unfinalized* blocks. However, the implementation // assumes a one-to-one map from collection ID to a *single* block ID. This holds for FINALIZED BLOCKS ONLY // *and* only in the absence of byzantine collector clusters (which the mature protocol must tolerate). @@ -96,6 +97,7 @@ type Blocks interface { // (one-to-many mapping) for soft finality and the mature protocol. // // Error returns: + // - storage.ErrAlreadyExists if any collection guarantee is already indexed // - generic error in case of unexpected failure from the database layer or encoding failure. - IndexBlockContainingCollectionGuarantees(blockID flow.Identifier, collIDs []flow.Identifier) error + BatchIndexBlockContainingCollectionGuarantees(lctx lockctx.Proof, rw ReaderBatchWriter, blockID flow.Identifier, collIDs []flow.Identifier) error } 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 +} diff --git a/storage/events.go b/storage/events.go index 4062acea82e..041dcf3c64a 100644 --- a/storage/events.go +++ b/storage/events.go @@ -1,6 +1,8 @@ package storage import ( + "github.com/jordanschalm/lockctx" + "github.com/onflow/flow-go/model/flow" ) @@ -22,11 +24,10 @@ type EventsReader interface { type Events interface { EventsReader - // Store will store events for the given block ID - Store(blockID flow.Identifier, blockEvents []flow.EventsList) error - // BatchStore will store events for the given block ID in a given batch - BatchStore(blockID flow.Identifier, events []flow.EventsList, batch ReaderBatchWriter) error + // it requires the caller to hold [storage.LockInsertEvent] + // It returns [storage.ErrAlreadyExists] if events for the block already exist. + BatchStore(lctx lockctx.Proof, blockID flow.Identifier, events []flow.EventsList, batch ReaderBatchWriter) error // BatchRemoveByBlockID removes events keyed by a blockID in provided batch // No errors are expected during normal operation, even if no entries are matched. @@ -37,8 +38,9 @@ type Events interface { type ServiceEvents interface { // BatchStore stores service events keyed by a blockID in provided batch // No errors are expected during normal operation, even if no entries are matched. - // If database unexpectedly fails to process the request, the error is wrapped in a generic error and returned. - BatchStore(blockID flow.Identifier, events []flow.Event, batch ReaderBatchWriter) error + // it requires the caller to hold [storage.LockInsertServiceEvent] + // It returns [storage.ErrAlreadyExists] if events for the block already exist. + BatchStore(lctx lockctx.Proof, blockID flow.Identifier, events []flow.Event, batch ReaderBatchWriter) error // ByBlockID returns the events for the given block ID ByBlockID(blockID flow.Identifier) ([]flow.Event, error) diff --git a/storage/light_transaction_results.go b/storage/light_transaction_results.go index e2109d8e450..3ec2be945f5 100644 --- a/storage/light_transaction_results.go +++ b/storage/light_transaction_results.go @@ -1,6 +1,10 @@ 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,8 +32,6 @@ type LightTransactionResults interface { LightTransactionResultsReader // BatchStore inserts a batch of transaction result into a batch - BatchStore(blockID flow.Identifier, transactionResults []flow.LightTransactionResult, rw ReaderBatchWriter) 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 + // it requires the caller to hold [storage.LockInsertLightTransactionResult] + BatchStore(lctx lockctx.Proof, blockID flow.Identifier, transactionResults []flow.LightTransactionResult, rw ReaderBatchWriter) error } diff --git a/storage/locks.go b/storage/locks.go index 5f7f968acaf..3fb808ade98 100644 --- a/storage/locks.go +++ b/storage/locks.go @@ -20,15 +20,26 @@ const ( // The reason they are combined is because insertion process reads some data updated by finalization process, // in order to prevent dirty reads, we need to acquire the lock for both operations. LockInsertOrFinalizeClusterBlock = "lock_insert_or_finalize_cluster_block" + // LockInsertEvent protects the insertion of events. + // This lock is reused by both EN storing its own receipt and AN indexing execution data + LockInsertEvent = "lock_insert_event" + // LockInsertServiceEvent protects the insertion of service events. + LockInsertServiceEvent = "lock_insert_service_event" + // LockInsertLightTransactionResult protects the insertion of light transaction results. + LockInsertLightTransactionResult = "lock_insert_light_transaction_result" // LockInsertOwnReceipt is intended for Execution Nodes to ensure that they never publish different receipts for the same block. // Specifically, with this lock we prevent accidental overwrites of the index `executed block ID` ➜ `Receipt ID`. - LockInsertOwnReceipt = "lock_insert_own_receipt" + LockInsertOwnReceipt = "lock_insert_own_receipt" + LockIndexExecutionResult = "lock_index_execution_result" + LockIndexStateCommitment = "lock_index_state_commitment" + LockInsertAndIndexTxResult = "lock_insert_and_index_tx_result" // LockInsertCollection protects the insertion of collections. LockInsertCollection = "lock_insert_collection" - // LockBootstrapping protects data that is *exclusively* written during bootstrapping. - LockBootstrapping = "lock_bootstrapping" + // LockInsertInstanceParams protects data that is *exclusively* written during bootstrapping. + LockInsertInstanceParams = "lock_insert_instance_params" // LockInsertChunkDataPack protects the insertion of chunk data packs (not yet used anywhere - LockInsertChunkDataPack = "lock_insert_chunk_data_pack" + LockInsertChunkDataPack = "lock_insert_chunk_data_pack" + LockIndexCollectionsByBlock = "lock_index_collections_by_block" ) // Locks returns a list of all named locks used by the storage layer. @@ -38,10 +49,17 @@ func Locks() []string { LockFinalizeBlock, LockIndexResultApproval, LockInsertOrFinalizeClusterBlock, + LockInsertEvent, + LockInsertServiceEvent, LockInsertOwnReceipt, + LockIndexExecutionResult, + LockIndexStateCommitment, + LockInsertAndIndexTxResult, LockInsertCollection, - LockBootstrapping, + LockInsertLightTransactionResult, + LockInsertInstanceParams, LockInsertChunkDataPack, + LockIndexCollectionsByBlock, } } @@ -63,9 +81,33 @@ type LockManager = lockctx.Manager // This function will panic if a policy is created which does not prevent deadlocks. func makeLockPolicy() lockctx.Policy { return lockctx.NewDAGPolicyBuilder(). + // for protocol to Bootstrap, during bootstrapping, + // we need to insert and finalize + // state/protocol/badger/state.go#Bootstrap + Add(LockInsertInstanceParams, LockIndexExecutionResult). + + // EN to bootstrap + // engine/execution/state/bootstrap/bootstrap.go#Bootstrapper.BootstrapExecutionDatabase + Add(LockIndexExecutionResult, LockInsertBlock). Add(LockInsertBlock, LockFinalizeBlock). - Add(LockFinalizeBlock, LockBootstrapping). - Add(LockInsertOwnReceipt, LockInsertChunkDataPack). + + // EN to save execution result + // engine/execution/state/state.go#state.saveExecutionResults + Add(LockInsertChunkDataPack, LockInsertEvent). + Add(LockInsertEvent, LockInsertServiceEvent). + Add(LockInsertServiceEvent, LockInsertAndIndexTxResult). + Add(LockInsertAndIndexTxResult, LockInsertOwnReceipt). + Add(LockInsertOwnReceipt, LockIndexExecutionResult). + Add(LockIndexExecutionResult, LockIndexStateCommitment). + + // AN ingestion engine processing finalized block + // engine/access/ingestion/engine.go#Engine.processFinalizedBlock + Add(LockIndexCollectionsByBlock, LockIndexExecutionResult). + + // AN optimistic syncing + // module/executiondatasync/optimistic_sync/persisters/block.go#BlockPersister.Persist + Add(LockInsertCollection, LockInsertEvent). + Add(LockInsertEvent, LockInsertLightTransactionResult). Build() } diff --git a/storage/locks_test.go b/storage/locks_test.go index ee1907a61ea..f919e9667dc 100644 --- a/storage/locks_test.go +++ b/storage/locks_test.go @@ -62,7 +62,7 @@ func TestHeldOneLock(t *testing.T) { t.Run("holds different lock", func(t *testing.T) { lctx := lockManager.NewContext() defer lctx.Release() - err := lctx.AcquireLock(LockBootstrapping) + err := lctx.AcquireLock(LockInsertInstanceParams) require.NoError(t, err) held, msg := HeldOneLock(lctx, LockInsertBlock, LockFinalizeBlock) diff --git a/storage/mock/blocks.go b/storage/mock/blocks.go index 15e799ccff2..b6807aaa983 100644 --- a/storage/mock/blocks.go +++ b/storage/mock/blocks.go @@ -16,6 +16,24 @@ type Blocks struct { mock.Mock } +// BatchIndexBlockContainingCollectionGuarantees provides a mock function with given fields: lctx, rw, blockID, collIDs +func (_m *Blocks) BatchIndexBlockContainingCollectionGuarantees(lctx lockctx.Proof, rw storage.ReaderBatchWriter, blockID flow.Identifier, collIDs []flow.Identifier) error { + ret := _m.Called(lctx, rw, blockID, collIDs) + + if len(ret) == 0 { + panic("no return value specified for BatchIndexBlockContainingCollectionGuarantees") + } + + var r0 error + if rf, ok := ret.Get(0).(func(lockctx.Proof, storage.ReaderBatchWriter, flow.Identifier, []flow.Identifier) error); ok { + r0 = rf(lctx, rw, blockID, collIDs) + } else { + r0 = ret.Error(0) + } + + return r0 +} + // BatchStore provides a mock function with given fields: lctx, rw, proposal func (_m *Blocks) BatchStore(lctx lockctx.Proof, rw storage.ReaderBatchWriter, proposal *flow.Proposal) error { ret := _m.Called(lctx, rw, proposal) @@ -154,24 +172,6 @@ func (_m *Blocks) ByView(view uint64) (*flow.Block, error) { return r0, r1 } -// IndexBlockContainingCollectionGuarantees provides a mock function with given fields: blockID, collIDs -func (_m *Blocks) IndexBlockContainingCollectionGuarantees(blockID flow.Identifier, collIDs []flow.Identifier) error { - ret := _m.Called(blockID, collIDs) - - if len(ret) == 0 { - panic("no return value specified for IndexBlockContainingCollectionGuarantees") - } - - var r0 error - if rf, ok := ret.Get(0).(func(flow.Identifier, []flow.Identifier) error); ok { - r0 = rf(blockID, collIDs) - } else { - r0 = ret.Error(0) - } - - return r0 -} - // ProposalByHeight provides a mock function with given fields: height func (_m *Blocks) ProposalByHeight(height uint64) (*flow.Proposal, error) { ret := _m.Called(height) diff --git a/storage/mock/events.go b/storage/mock/events.go index a23564f1e08..ea90103c5d3 100644 --- a/storage/mock/events.go +++ b/storage/mock/events.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" @@ -32,17 +34,17 @@ func (_m *Events) BatchRemoveByBlockID(blockID flow.Identifier, batch storage.Re return r0 } -// BatchStore provides a mock function with given fields: blockID, events, batch -func (_m *Events) BatchStore(blockID flow.Identifier, events []flow.EventsList, batch storage.ReaderBatchWriter) error { - ret := _m.Called(blockID, events, batch) +// BatchStore provides a mock function with given fields: lctx, blockID, events, batch +func (_m *Events) BatchStore(lctx lockctx.Proof, blockID flow.Identifier, events []flow.EventsList, batch storage.ReaderBatchWriter) error { + ret := _m.Called(lctx, blockID, events, batch) if len(ret) == 0 { panic("no return value specified for BatchStore") } var r0 error - if rf, ok := ret.Get(0).(func(flow.Identifier, []flow.EventsList, storage.ReaderBatchWriter) error); ok { - r0 = rf(blockID, events, batch) + if rf, ok := ret.Get(0).(func(lockctx.Proof, flow.Identifier, []flow.EventsList, storage.ReaderBatchWriter) error); ok { + r0 = rf(lctx, blockID, events, batch) } else { r0 = ret.Error(0) } @@ -170,24 +172,6 @@ func (_m *Events) ByBlockIDTransactionIndex(blockID flow.Identifier, txIndex uin return r0, r1 } -// Store provides a mock function with given fields: blockID, blockEvents -func (_m *Events) Store(blockID flow.Identifier, blockEvents []flow.EventsList) error { - ret := _m.Called(blockID, blockEvents) - - if len(ret) == 0 { - panic("no return value specified for Store") - } - - var r0 error - if rf, ok := ret.Get(0).(func(flow.Identifier, []flow.EventsList) error); ok { - r0 = rf(blockID, blockEvents) - } else { - r0 = ret.Error(0) - } - - return r0 -} - // NewEvents creates a new instance of Events. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. // The first argument is typically a *testing.T value. func NewEvents(t interface { diff --git a/storage/mock/execution_results.go b/storage/mock/execution_results.go index 07702c1db3c..868351f817f 100644 --- a/storage/mock/execution_results.go +++ b/storage/mock/execution_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 ExecutionResults struct { mock.Mock } -// BatchIndex provides a mock function with given fields: blockID, resultID, batch -func (_m *ExecutionResults) BatchIndex(blockID flow.Identifier, resultID flow.Identifier, batch storage.ReaderBatchWriter) error { - ret := _m.Called(blockID, resultID, batch) +// BatchIndex provides a mock function with given fields: lctx, rw, blockID, resultID +func (_m *ExecutionResults) BatchIndex(lctx lockctx.Proof, rw storage.ReaderBatchWriter, blockID flow.Identifier, resultID flow.Identifier) error { + ret := _m.Called(lctx, rw, blockID, resultID) if len(ret) == 0 { panic("no return value specified for BatchIndex") } var r0 error - if rf, ok := ret.Get(0).(func(flow.Identifier, flow.Identifier, storage.ReaderBatchWriter) error); ok { - r0 = rf(blockID, resultID, batch) + if rf, ok := ret.Get(0).(func(lockctx.Proof, storage.ReaderBatchWriter, flow.Identifier, flow.Identifier) error); ok { + r0 = rf(lctx, rw, blockID, resultID) } else { r0 = ret.Error(0) } @@ -128,58 +130,34 @@ func (_m *ExecutionResults) ByID(resultID flow.Identifier) (*flow.ExecutionResul return r0, r1 } -// ForceIndex provides a mock function with given fields: blockID, resultID -func (_m *ExecutionResults) ForceIndex(blockID flow.Identifier, resultID flow.Identifier) error { - ret := _m.Called(blockID, resultID) +// IDByBlockID provides a mock function with given fields: blockID +func (_m *ExecutionResults) IDByBlockID(blockID flow.Identifier) (flow.Identifier, error) { + ret := _m.Called(blockID) if len(ret) == 0 { - panic("no return value specified for ForceIndex") + panic("no return value specified for IDByBlockID") } - var r0 error - if rf, ok := ret.Get(0).(func(flow.Identifier, flow.Identifier) error); ok { - r0 = rf(blockID, resultID) - } else { - r0 = ret.Error(0) - } - - return r0 -} - -// Index provides a mock function with given fields: blockID, resultID -func (_m *ExecutionResults) Index(blockID flow.Identifier, resultID flow.Identifier) error { - ret := _m.Called(blockID, resultID) - - if len(ret) == 0 { - panic("no return value specified for Index") + var r0 flow.Identifier + var r1 error + if rf, ok := ret.Get(0).(func(flow.Identifier) (flow.Identifier, error)); ok { + return rf(blockID) } - - var r0 error - if rf, ok := ret.Get(0).(func(flow.Identifier, flow.Identifier) error); ok { - r0 = rf(blockID, resultID) + if rf, ok := ret.Get(0).(func(flow.Identifier) flow.Identifier); ok { + r0 = rf(blockID) } else { - r0 = ret.Error(0) - } - - return r0 -} - -// Store provides a mock function with given fields: result -func (_m *ExecutionResults) Store(result *flow.ExecutionResult) error { - ret := _m.Called(result) - - if len(ret) == 0 { - panic("no return value specified for Store") + if ret.Get(0) != nil { + r0 = ret.Get(0).(flow.Identifier) + } } - var r0 error - if rf, ok := ret.Get(0).(func(*flow.ExecutionResult) error); ok { - r0 = rf(result) + if rf, ok := ret.Get(1).(func(flow.Identifier) error); ok { + r1 = rf(blockID) } else { - r0 = ret.Error(0) + r1 = ret.Error(1) } - return r0 + return r0, r1 } // NewExecutionResults creates a new instance of ExecutionResults. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. diff --git a/storage/mock/execution_results_reader.go b/storage/mock/execution_results_reader.go index b812a794d1d..244624758e9 100644 --- a/storage/mock/execution_results_reader.go +++ b/storage/mock/execution_results_reader.go @@ -72,6 +72,36 @@ func (_m *ExecutionResultsReader) ByID(resultID flow.Identifier) (*flow.Executio return r0, r1 } +// IDByBlockID provides a mock function with given fields: blockID +func (_m *ExecutionResultsReader) IDByBlockID(blockID flow.Identifier) (flow.Identifier, error) { + ret := _m.Called(blockID) + + if len(ret) == 0 { + panic("no return value specified for IDByBlockID") + } + + var r0 flow.Identifier + var r1 error + if rf, ok := ret.Get(0).(func(flow.Identifier) (flow.Identifier, error)); ok { + return rf(blockID) + } + if rf, ok := ret.Get(0).(func(flow.Identifier) flow.Identifier); ok { + r0 = rf(blockID) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(flow.Identifier) + } + } + + if rf, ok := ret.Get(1).(func(flow.Identifier) error); ok { + r1 = rf(blockID) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // NewExecutionResultsReader creates a new instance of ExecutionResultsReader. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. // The first argument is typically a *testing.T value. func NewExecutionResultsReader(t interface { diff --git a/storage/mock/light_transaction_results.go b/storage/mock/light_transaction_results.go index 6e6b277acb8..6f82df4ffe2 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,35 +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, blockID, transactionResults, rw +func (_m *LightTransactionResults) BatchStore(lctx lockctx.Proof, blockID flow.Identifier, transactionResults []flow.LightTransactionResult, rw storage.ReaderBatchWriter) error { + ret := _m.Called(lctx, blockID, transactionResults, rw) 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) - } else { - r0 = ret.Error(0) - } - - 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) + if rf, ok := ret.Get(0).(func(lockctx.Proof, flow.Identifier, []flow.LightTransactionResult, storage.ReaderBatchWriter) error); ok { + r0 = rf(lctx, blockID, transactionResults, rw) } else { r0 = ret.Error(0) } diff --git a/storage/mock/service_events.go b/storage/mock/service_events.go index fc2fc46db5c..a7c47994f77 100644 --- a/storage/mock/service_events.go +++ b/storage/mock/service_events.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" @@ -32,17 +34,17 @@ func (_m *ServiceEvents) BatchRemoveByBlockID(blockID flow.Identifier, batch sto return r0 } -// BatchStore provides a mock function with given fields: blockID, events, batch -func (_m *ServiceEvents) BatchStore(blockID flow.Identifier, events []flow.Event, batch storage.ReaderBatchWriter) error { - ret := _m.Called(blockID, events, batch) +// BatchStore provides a mock function with given fields: lctx, blockID, events, batch +func (_m *ServiceEvents) BatchStore(lctx lockctx.Proof, blockID flow.Identifier, events []flow.Event, batch storage.ReaderBatchWriter) error { + ret := _m.Called(lctx, blockID, events, batch) if len(ret) == 0 { panic("no return value specified for BatchStore") } var r0 error - if rf, ok := ret.Get(0).(func(flow.Identifier, []flow.Event, storage.ReaderBatchWriter) error); ok { - r0 = rf(blockID, events, batch) + if rf, ok := ret.Get(0).(func(lockctx.Proof, flow.Identifier, []flow.Event, storage.ReaderBatchWriter) error); ok { + r0 = rf(lctx, blockID, events, batch) } else { r0 = ret.Error(0) } diff --git a/storage/mock/transaction_results.go b/storage/mock/transaction_results.go index d94b4881ce7..c35cc476460 100644 --- a/storage/mock/transaction_results.go +++ b/storage/mock/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" @@ -32,17 +34,17 @@ func (_m *TransactionResults) BatchRemoveByBlockID(id flow.Identifier, batch sto return r0 } -// BatchStore provides a mock function with given fields: blockID, transactionResults, batch -func (_m *TransactionResults) BatchStore(blockID flow.Identifier, transactionResults []flow.TransactionResult, batch storage.ReaderBatchWriter) error { - ret := _m.Called(blockID, transactionResults, batch) +// BatchStore provides a mock function with given fields: lctx, rw, blockID, transactionResults +func (_m *TransactionResults) BatchStore(lctx lockctx.Proof, rw storage.ReaderBatchWriter, blockID flow.Identifier, transactionResults []flow.TransactionResult) 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.TransactionResult, storage.ReaderBatchWriter) error); ok { - r0 = rf(blockID, transactionResults, batch) + if rf, ok := ret.Get(0).(func(lockctx.Proof, storage.ReaderBatchWriter, flow.Identifier, []flow.TransactionResult) error); ok { + r0 = rf(lctx, rw, blockID, transactionResults) } else { r0 = ret.Error(0) } diff --git a/storage/operation/commits.go b/storage/operation/commits.go index 448a8543b6f..06b8cb92855 100644 --- a/storage/operation/commits.go +++ b/storage/operation/commits.go @@ -17,13 +17,13 @@ import ( // // CAUTION: // - Confirming that no value is already stored and the subsequent write must be atomic to prevent data corruption. -// The caller must acquire the [storage.LockInsertOwnReceipt] and hold it until the database write has been committed. +// The caller must acquire the [storage.LockIndexStateCommitment] and hold it until the database write has been committed. // // Expected error returns during normal operations: // - [storage.ErrDataMismatch] if a *different* state commitment is already indexed for the same block ID func IndexStateCommitment(lctx lockctx.Proof, rw storage.ReaderBatchWriter, blockID flow.Identifier, commit flow.StateCommitment) error { - if !lctx.HoldsLock(storage.LockInsertOwnReceipt) { - return fmt.Errorf("cannot index state commitment without holding lock %s", storage.LockInsertOwnReceipt) + if !lctx.HoldsLock(storage.LockIndexStateCommitment) { + return fmt.Errorf("cannot index state commitment without holding lock %s", storage.LockIndexStateCommitment) } var existingCommit flow.StateCommitment diff --git a/storage/operation/commits_test.go b/storage/operation/commits_test.go index 90545dde3ca..bb13a110813 100644 --- a/storage/operation/commits_test.go +++ b/storage/operation/commits_test.go @@ -19,7 +19,7 @@ func TestStateCommitments(t *testing.T) { expected := unittest.StateCommitmentFixture() id := unittest.IdentifierFixture() - err := unittest.WithLock(t, lockManager, storage.LockInsertOwnReceipt, func(lctx lockctx.Context) error { + err := unittest.WithLock(t, lockManager, storage.LockIndexStateCommitment, func(lctx lockctx.Context) error { return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { return operation.IndexStateCommitment(lctx, rw, id, expected) }) diff --git a/storage/operation/events.go b/storage/operation/events.go index f07467fe6db..ffb6b9fd1c9 100644 --- a/storage/operation/events.go +++ b/storage/operation/events.go @@ -1,6 +1,10 @@ package operation import ( + "fmt" + + "github.com/jordanschalm/lockctx" + "github.com/onflow/flow-go/model/flow" "github.com/onflow/flow-go/storage" ) @@ -9,11 +13,88 @@ func eventPrefix(prefix byte, blockID flow.Identifier, event flow.Event) []byte return MakePrefix(prefix, blockID, event.TransactionID, event.TransactionIndex, event.EventIndex) } -func InsertEvent(w storage.Writer, blockID flow.Identifier, event flow.Event) error { +// InsertBlockEvents stores all events for a given block in the database. +// Requires LockInsertEvent to be held for thread safety. +// This function iterates through all events in the provided EventsList and stores each event individually. +// It returns [storage.ErrAlreadyExists] if events for the block already exist. +func InsertBlockEvents(lctx lockctx.Proof, rw storage.ReaderBatchWriter, blockID flow.Identifier, events []flow.EventsList) error { + if !lctx.HoldsLock(storage.LockInsertEvent) { + return fmt.Errorf("InsertBlockEvents requires LockInsertEvent to be held") + } + + // Check if events for the block already exist + // We can exit early if we found one exist + // Because regardless the list is empty, or has one, or more, + // we don't want to overwrite existing events indexed by the block + prefix := MakePrefix(codeEvent, blockID) + checkExists := func(key []byte) error { + return fmt.Errorf("event with key %x already exists under prefix %x: %w", key, prefix, storage.ErrAlreadyExists) + } + + err := IterateKeysByPrefixRange(rw.GlobalReader(), prefix, prefix, checkExists) + if err != nil { + return err + } + + writer := rw.Writer() + + for _, eventsList := range events { + for _, event := range eventsList { + err := insertEvent(writer, blockID, event) + if err != nil { + return fmt.Errorf("cannot batch insert event: %w", err) + } + } + } + + return nil +} + +// insertEvent stores a regular event in the database. +// The event is stored with a key that includes the block ID, transaction ID, transaction index, and event index. +func insertEvent(w storage.Writer, blockID flow.Identifier, event flow.Event) error { return UpsertByKey(w, eventPrefix(codeEvent, blockID, event), event) } -func InsertServiceEvent(w storage.Writer, blockID flow.Identifier, event flow.Event) error { +// InsertBlockServiceEvents stores all service events for a given block in the database. +// Requires LockInsertServiceEvent to be held for thread safety. +// This function iterates through all service events in the provided list and stores each event individually. +// Service events are special events generated by the system (e.g., account creation, contract deployment). +// It returns [storage.ErrAlreadyExists] if events for the block already exist. +func InsertBlockServiceEvents(lctx lockctx.Proof, rw storage.ReaderBatchWriter, blockID flow.Identifier, events []flow.Event) error { + if !lctx.HoldsLock(storage.LockInsertServiceEvent) { + return fmt.Errorf("InsertBlockServiceEvents requires LockInsertServiceEvent to be held") + } + + // Check if events for the block already exist + // We can exit early if we found one exist + // Because regardless the list is empty, or has one, or more, + // we don't want to overwrite existing events indexed by the block + prefix := MakePrefix(codeEvent, blockID) + checkExists := func(key []byte) error { + return fmt.Errorf("event with key %x already exists under prefix %x: %w", key, prefix, storage.ErrAlreadyExists) + } + + err := IterateKeysByPrefixRange(rw.GlobalReader(), prefix, prefix, checkExists) + if err != nil { + return err + } + + writer := rw.Writer() + + for _, event := range events { + err := insertServiceEvent(writer, blockID, event) + if err != nil { + return fmt.Errorf("cannot batch insert service event: %w", err) + } + } + + return nil +} + +// insertServiceEvent stores a service event in the database. +// The event is stored with a key that includes the block ID, transaction ID, transaction index, and event index. +func insertServiceEvent(w storage.Writer, blockID flow.Identifier, event flow.Event) error { return UpsertByKey(w, eventPrefix(codeServiceEvent, blockID, event), event) } @@ -37,10 +118,22 @@ func LookupEventsByBlockIDEventType(r storage.Reader, blockID flow.Identifier, e return TraverseByPrefix(r, MakePrefix(codeEvent, blockID), iterationFunc, storage.DefaultIteratorOptions()) } +// RemoveServiceEventsByBlockID removes all service events associated with the given block ID. +// This operation is typically used during block rollback or cleanup scenarios. +// It removes all service events that were generated for the specified block. +// +// Error returns: +// - generic error in case of unexpected database error func RemoveServiceEventsByBlockID(r storage.Reader, w storage.Writer, blockID flow.Identifier) error { return RemoveByKeyPrefix(r, w, MakePrefix(codeServiceEvent, blockID)) } +// RemoveEventsByBlockID removes all regular events associated with the given block ID. +// This operation is typically used during block rollback or cleanup scenarios. +// It removes all regular events that were generated for the specified block. +// +// Error returns: +// - generic error in case of unexpected database error func RemoveEventsByBlockID(r storage.Reader, w storage.Writer, blockID flow.Identifier) error { return RemoveByKeyPrefix(r, w, MakePrefix(codeEvent, blockID)) } diff --git a/storage/operation/events_test.go b/storage/operation/events_test.go index 272ccea1410..b8f35b39eb7 100644 --- a/storage/operation/events_test.go +++ b/storage/operation/events_test.go @@ -6,6 +6,7 @@ import ( "golang.org/x/exp/slices" + "github.com/jordanschalm/lockctx" "github.com/stretchr/testify/require" "github.com/onflow/flow-go/model/flow" @@ -18,6 +19,7 @@ import ( // TestRetrieveEventByBlockIDTxID tests event insertion, event retrieval by block id, block id and transaction id, // and block id and event type func TestRetrieveEventByBlockIDTxID(t *testing.T) { + lockManager := storage.NewTestingLockManager() dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { // create block ids, transaction ids and event types slices @@ -34,6 +36,7 @@ func TestRetrieveEventByBlockIDTxID(t *testing.T) { for _, b := range blockIDs { bEvents := make([]flow.Event, 0) + allEventsForBlock := make([]flow.Event, 0) // all blocks share the same transactions for i, tx := range txIDs { @@ -52,13 +55,8 @@ func TestRetrieveEventByBlockIDTxID(t *testing.T) { unittest.Event.WithTransactionID(tx), ) - // insert event into the db - err := db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { - return operation.InsertEvent(rw.Writer(), b, event) - }) - require.Nil(t, err) - - // update event arrays in the maps + // collect events for batch insertion + allEventsForBlock = append(allEventsForBlock, event) bEvents = append(bEvents, event) tEvents = append(tEvents, event) eEvents = append(eEvents, event) @@ -72,6 +70,15 @@ func TestRetrieveEventByBlockIDTxID(t *testing.T) { } txMap[b.String()+"_"+tx.String()] = tEvents } + + // insert all events for this block in one batch + err := unittest.WithLock(t, lockManager, storage.LockInsertEvent, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return operation.InsertBlockEvents(lctx, rw, b, []flow.EventsList{allEventsForBlock}) + }) + }) + require.NoError(t, err) + blockMap[b.String()] = bEvents } diff --git a/storage/operation/headers.go b/storage/operation/headers.go index 5036c08aa8f..d2b8fffe7f0 100644 --- a/storage/operation/headers.go +++ b/storage/operation/headers.go @@ -124,21 +124,44 @@ func BlockExists(r storage.Reader, blockID flow.Identifier) (bool, error) { return KeyExists(r, MakePrefix(codeHeader, blockID)) } -// IndexBlockContainingCollectionGuarantee produces a mapping from the ID of a [flow.CollectionGuarantee] to the block ID containing this guarantee. +// BatchIndexBlockContainingCollectionGuarantees produces mappings from the IDs of [flow.CollectionGuarantee]s to the block ID containing these guarantees. +// The caller must acquire a storage.LockIndexCollectionsByBlock lock. // -// CAUTION: -// - The caller must acquire the lock ??? and hold it until the database write has been committed. -// TODO: USE LOCK, we want to protect this mapping from accidental overwrites (because the key is not derived from the value via a collision-resistant hash) -// - A collection can be included in multiple *unfinalized* blocks. However, the implementation -// assumes a one-to-one map from collection ID to a *single* block ID. This holds for FINALIZED BLOCKS ONLY -// *and* only in the ABSENCE of BYZANTINE collector CLUSTERS (which the mature protocol must tolerate). -// Hence, this function should be treated as a temporary solution, which requires generalization -// (one-to-many mapping) for soft finality and the mature protocol. +// CAUTION: a collection can be included in multiple *unfinalized* blocks. However, the implementation +// assumes a one-to-one map from collection ID to a *single* block ID. This holds for FINALIZED BLOCKS ONLY +// *and* only in the absence of byzantine collector clusters (which the mature protocol must tolerate). +// Hence, this function should be treated as a temporary solution, which requires generalization +// (one-to-many mapping) for soft finality and the mature protocol. // // Expected errors during normal operations: -// TODO: return [storage.ErrAlreadyExists] or [storage.ErrDataMismatch] -func IndexBlockContainingCollectionGuarantee(w storage.Writer, collID flow.Identifier, blockID flow.Identifier) error { - return UpsertByKey(w, MakePrefix(codeCollectionBlock, collID), blockID) +// - [storage.ErrAlreadyExists] if any collection guarantee is already indexed +func BatchIndexBlockContainingCollectionGuarantees(lctx lockctx.Proof, rw storage.ReaderBatchWriter, blockID flow.Identifier, collIDs []flow.Identifier) error { + if !lctx.HoldsLock(storage.LockIndexCollectionsByBlock) { + return fmt.Errorf("BatchIndexBlockContainingCollectionGuarantees requires %v", storage.LockIndexCollectionsByBlock) + } + + // Check if any keys already exist + for _, collID := range collIDs { + key := MakePrefix(codeCollectionBlock, collID) + exists, err := KeyExists(rw.GlobalReader(), key) + if err != nil { + return fmt.Errorf("could not check if collection guarantee is already indexed: %w", err) + } + if exists { + return fmt.Errorf("collection guarantee (%x) is already indexed: %w", collID, storage.ErrAlreadyExists) + } + } + + // Index all collection guarantees + for _, collID := range collIDs { + key := MakePrefix(codeCollectionBlock, collID) + err := UpsertByKey(rw.Writer(), key, blockID) + if err != nil { + return fmt.Errorf("could not index collection guarantee (%x): %w", collID, err) + } + } + + return nil } // LookupBlockContainingCollectionGuarantee retrieves the block containing the [flow.CollectionGuarantee] with the given ID. diff --git a/storage/operation/headers_test.go b/storage/operation/headers_test.go index 9ec7b8b8644..61e9dad90b4 100644 --- a/storage/operation/headers_test.go +++ b/storage/operation/headers_test.go @@ -53,8 +53,12 @@ func TestHeaderIDIndexByCollectionID(t *testing.T) { headerID := unittest.IdentifierFixture() collectionGuaranteeID := unittest.IdentifierFixture() - err := db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { - return operation.IndexBlockContainingCollectionGuarantee(rw.Writer(), collectionGuaranteeID, headerID) + lockManager := storage.NewTestingLockManager() + + err := unittest.WithLock(t, lockManager, storage.LockIndexCollectionsByBlock, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return operation.BatchIndexBlockContainingCollectionGuarantees(lctx, rw, headerID, []flow.Identifier{collectionGuaranteeID}) + }) }) require.NoError(t, err) diff --git a/storage/operation/instance_params.go b/storage/operation/instance_params.go index 9fb67289d79..591faddc199 100644 --- a/storage/operation/instance_params.go +++ b/storage/operation/instance_params.go @@ -15,13 +15,13 @@ import ( // - This function is intended to be called exactly once during bootstrapping. // Overwrites are prevented by an explicit existence check; if data is already present, error is returned. // - To guarantee atomicity of existence-check plus database write, we require the caller to acquire -// the [storage.LockBootstrapping] lock and hold it until the database write has been committed. +// the [storage.LockInsertInstanceParams] lock and hold it until the database write has been committed. // // Expected errors during normal operations: // - [storage.ErrAlreadyExists] if instance params have already been stored. func InsertInstanceParams(lctx lockctx.Proof, rw storage.ReaderBatchWriter, params flow.VersionedInstanceParams) error { - if !lctx.HoldsLock(storage.LockBootstrapping) { - return fmt.Errorf("missing required lock: %s", storage.LockBootstrapping) + if !lctx.HoldsLock(storage.LockInsertInstanceParams) { + return fmt.Errorf("missing required lock: %s", storage.LockInsertInstanceParams) } key := MakePrefix(codeInstanceParams) exist, err := KeyExists(rw.GlobalReader(), key) diff --git a/storage/operation/instance_params_test.go b/storage/operation/instance_params_test.go index 26b0c88fdd2..76be031b2ad 100644 --- a/storage/operation/instance_params_test.go +++ b/storage/operation/instance_params_test.go @@ -20,7 +20,7 @@ import ( // 1. InstanceParams can be inserted and retrieved successfully. // 2. Overwrite attempts return storage.ErrAlreadyExists and do not change the // persisted value. -// 3. Writes without holding LockBootstrapping are denied. +// 3. Writes without holding LockInsertInstanceParams are denied. func TestInstanceParams_InsertRetrieve(t *testing.T) { lockManager := storage.NewTestingLockManager() enc, err := datastore.NewVersionedInstanceParams( @@ -34,7 +34,7 @@ func TestInstanceParams_InsertRetrieve(t *testing.T) { t.Run("happy path: insert and retrieve", func(t *testing.T) { dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { lctx := lockManager.NewContext() - require.NoError(t, lctx.AcquireLock(storage.LockBootstrapping)) + require.NoError(t, lctx.AcquireLock(storage.LockInsertInstanceParams)) defer lctx.Release() err = db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { @@ -52,7 +52,7 @@ func TestInstanceParams_InsertRetrieve(t *testing.T) { t.Run("overwrite returns ErrAlreadyExists", func(t *testing.T) { dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { lctx := lockManager.NewContext() - require.NoError(t, lctx.AcquireLock(storage.LockBootstrapping)) + require.NoError(t, lctx.AcquireLock(storage.LockInsertInstanceParams)) err = db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { return operation.InsertInstanceParams(lctx, rw, *enc) @@ -70,7 +70,7 @@ func TestInstanceParams_InsertRetrieve(t *testing.T) { require.NoError(t, err) lctx2 := lockManager.NewContext() - require.NoError(t, lctx2.AcquireLock(storage.LockBootstrapping)) + require.NoError(t, lctx2.AcquireLock(storage.LockInsertInstanceParams)) defer lctx2.Release() err = db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { @@ -94,7 +94,7 @@ func TestInstanceParams_InsertRetrieve(t *testing.T) { err = db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { return operation.InsertInstanceParams(lctx, rw, *enc) }) - require.ErrorContains(t, err, storage.LockBootstrapping) + require.ErrorContains(t, err, storage.LockInsertInstanceParams) }) }) } diff --git a/storage/operation/receipts.go b/storage/operation/receipts.go index 832a86544b4..a5b31a4f8a0 100644 --- a/storage/operation/receipts.go +++ b/storage/operation/receipts.go @@ -1,6 +1,11 @@ package operation import ( + "errors" + "fmt" + + "github.com/jordanschalm/lockctx" + "github.com/onflow/flow-go/model/flow" "github.com/onflow/flow-go/storage" ) @@ -24,17 +29,29 @@ func RetrieveExecutionReceiptStub(r storage.Reader, receiptID flow.Identifier, m // IndexOwnExecutionReceipt indexes the Execution Node's OWN execution receipt by the executed block ID. // -// CAUTION: -// - OVERWRITES existing data (potential for data corruption): -// This method silently overrides existing data without any sanity checks whether data for the same key already exits. -// Note that the Flow protocol mandates that for a previously persisted key, the data is never changed to a different -// value. Changing data could cause the node to publish inconsistent data and to be slashed, or the protocol to be -// compromised as a whole. This method does not contain any safeguards to prevent such data corruption. The caller -// is responsible to ensure that the DEDUPLICATION CHECK is done elsewhere ATOMICALLY with this write operation. -// -// No errors are expected during normal operation. -func IndexOwnExecutionReceipt(w storage.Writer, blockID flow.Identifier, receiptID flow.Identifier) error { - return UpsertByKey(w, MakePrefix(codeOwnBlockReceipt, blockID), receiptID) +// Error returns: +// - [storage.ErrDataMismatch] if a *different* receipt has already been indexed for the same block +func IndexOwnExecutionReceipt(lctx lockctx.Proof, rw storage.ReaderBatchWriter, blockID flow.Identifier, receiptID flow.Identifier) error { + if !lctx.HoldsLock(storage.LockInsertOwnReceipt) { + return fmt.Errorf("cannot index own execution receipt without holding lock %s", storage.LockInsertOwnReceipt) + } + + key := MakePrefix(codeOwnBlockReceipt, blockID) + + var existing flow.Identifier + err := RetrieveByKey(rw.GlobalReader(), key, &existing) + if err == nil { + if existing != receiptID { + return fmt.Errorf("own execution receipt for block %v already exists with different value, (existing: %v, new: %v), %w", blockID, existing, receiptID, storage.ErrDataMismatch) + } + return nil // The receipt already exists, no need to index again + } + + if !errors.Is(err, storage.ErrNotFound) { + return fmt.Errorf("could not check existing own execution receipt: %w", err) + } + + return UpsertByKey(rw.Writer(), key, receiptID) } // LookupOwnExecutionReceipt retrieves the Execution Node's OWN execution receipt ID for the specified block. diff --git a/storage/operation/receipts_test.go b/storage/operation/receipts_test.go index 0bdf92d663f..6b094961c4f 100644 --- a/storage/operation/receipts_test.go +++ b/storage/operation/receipts_test.go @@ -3,6 +3,7 @@ package operation_test import ( "testing" + "github.com/jordanschalm/lockctx" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -33,12 +34,15 @@ func TestReceipts_InsertRetrieve(t *testing.T) { func TestReceipts_Index(t *testing.T) { dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { + lockManager := storage.NewTestingLockManager() receipt := unittest.ExecutionReceiptFixture() expected := receipt.ID() blockID := receipt.ExecutionResult.BlockID - err := db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { - return operation.IndexOwnExecutionReceipt(rw.Writer(), blockID, expected) + err := storage.WithLock(lockManager, storage.LockInsertOwnReceipt, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return operation.IndexOwnExecutionReceipt(lctx, rw, blockID, expected) + }) }) require.Nil(t, err) diff --git a/storage/operation/results.go b/storage/operation/results.go index 29937653968..cc3d49af025 100644 --- a/storage/operation/results.go +++ b/storage/operation/results.go @@ -1,6 +1,10 @@ package operation import ( + "fmt" + + "github.com/jordanschalm/lockctx" + "github.com/onflow/flow-go/model/flow" "github.com/onflow/flow-go/storage" ) @@ -12,8 +16,8 @@ import ( // of data corruption, because for the same key, we expect the same value. // // No errors are expected during normal operation. -func InsertExecutionResult(w storage.Writer, result *flow.ExecutionResult) error { - return UpsertByKey(w, MakePrefix(codeExecutionResult, result.ID()), result) +func InsertExecutionResult(w storage.Writer, resultID flow.Identifier, result *flow.ExecutionResult) error { + return UpsertByKey(w, MakePrefix(codeExecutionResult, resultID), result) } // RetrieveExecutionResult retrieves an Execution Result by its ID. @@ -23,20 +27,39 @@ func RetrieveExecutionResult(r storage.Reader, resultID flow.Identifier, result return RetrieveByKey(r, MakePrefix(codeExecutionResult, resultID), result) } -// IndexExecutionResult indexes the Execution Node's OWN Execution Result by the executed block's ID. -// -// CAUTION: -// - OVERWRITES existing data (potential for data corruption): -// This method silently overrides existing data without any sanity checks whether data for the same key already exits. -// Note that the Flow protocol mandates that for a previously persisted key, the data is never changed to a different -// value. Changing data could cause the node to publish inconsistent data and to be slashed, or the protocol to be -// compromised as a whole. This method does not contain any safeguards to prevent such data corruption. +// IndexOwnOrSealedExecutionResult indexes the result of the given block. +// It is used by the following scenarios: +// 1. Execution Node indexes its own executed block's result when finish executing a block +// 2. Execution Node indexes the sealed root block's result during bootstrapping +// 3. Access Node indexes the sealed result during syncing from EN. +// The caller must acquire [storage.LockIndexExecutionResult] // -// TODO: USE LOCK, we want to protect this mapping from accidental overwrites (because the key is not derived from the value via a collision-resistant hash) -// -// No errors are expected during normal operation. -func IndexExecutionResult(w storage.Writer, blockID flow.Identifier, resultID flow.Identifier) error { - return UpsertByKey(w, MakePrefix(codeIndexExecutionResultByBlock, blockID), resultID) +// It returns [storage.ErrDataMismatch] if there is already an indexed result for the given blockID, +// but it is different from the given resultID. +func IndexOwnOrSealedExecutionResult(lctx lockctx.Proof, rw storage.ReaderBatchWriter, blockID flow.Identifier, resultID flow.Identifier) error { + // during bootstrapping, we index the sealed root block or the spork root block, which is not + // produced by the node itself, but we still need to index its execution result to be able to + // execute next block + if !lctx.HoldsLock(storage.LockIndexExecutionResult) { + return fmt.Errorf("missing require locks: %s", storage.LockIndexExecutionResult) + } + + key := MakePrefix(codeIndexExecutionResultByBlock, blockID) + var existing flow.Identifier + err := RetrieveByKey(rw.GlobalReader(), key, &existing) + if err == nil { + if existing != resultID { + return fmt.Errorf("storing result that is different from the already stored one for block: %v, storing result: %v, stored result: %v. %w", + blockID, resultID, existing, storage.ErrDataMismatch) + } + // if the result is the same, we don't need to index it again + return nil + } else if err != storage.ErrNotFound { + return fmt.Errorf("could not check if execution result exists: %w", err) + } + + // if the result is not indexed, we can index it + return UpsertByKey(rw.Writer(), key, resultID) } // LookupExecutionResult retrieves the Execution Node's OWN Execution Result ID for the specified block. @@ -48,12 +71,6 @@ func LookupExecutionResult(r storage.Reader, blockID flow.Identifier, resultID * return RetrieveByKey(r, MakePrefix(codeIndexExecutionResultByBlock, blockID), resultID) } -// ExistExecutionResult checks if the execution node has its OWN Execution Result for the specified block. -// No errors are expected during normal operation. -func ExistExecutionResult(r storage.Reader, blockID flow.Identifier) (bool, error) { - return KeyExists(r, MakePrefix(codeIndexExecutionResultByBlock, blockID)) -} - // RemoveExecutionResultIndex removes Execution Node's OWN Execution Result for the given blockID. // CAUTION: this is for recovery purposes only, and should not be used during normal operations // It returns nil if the collection does not exist. diff --git a/storage/operation/results_test.go b/storage/operation/results_test.go index a2f59425b1c..9b5dd6f7695 100644 --- a/storage/operation/results_test.go +++ b/storage/operation/results_test.go @@ -18,7 +18,7 @@ func TestResults_InsertRetrieve(t *testing.T) { expected := unittest.ExecutionResultFixture() err := db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { - return operation.InsertExecutionResult(rw.Writer(), expected) + return operation.InsertExecutionResult(rw.Writer(), expected.ID(), expected) }) require.Nil(t, err) diff --git a/storage/operation/stats_test.go b/storage/operation/stats_test.go index ff05671b1c3..2306813fdad 100644 --- a/storage/operation/stats_test.go +++ b/storage/operation/stats_test.go @@ -6,6 +6,7 @@ import ( "github.com/jordanschalm/lockctx" "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" @@ -13,43 +14,49 @@ import ( ) func TestSummarizeKeysByFirstByteConcurrent(t *testing.T) { + lockManager := storage.NewTestingLockManager() dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { - lockManager := storage.NewTestingLockManager() - err := db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { - // insert random events - b := unittest.IdentifierFixture() - events := unittest.EventsFixture(30) - for _, evt := range events { - err := operation.InsertEvent(rw.Writer(), b, evt) + err := unittest.WithLock(t, lockManager, storage.LockInsertEvent, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + // insert random events + b := unittest.IdentifierFixture() + events := unittest.EventsFixture(30) + err := operation.InsertBlockEvents(lctx, rw, b, []flow.EventsList{events}) if err != nil { return err } - } - // insert 100 chunk data packs - for i := 0; i < 100; i++ { - collectionID := unittest.IdentifierFixture() - cdp := &storage.StoredChunkDataPack{ - ChunkID: unittest.IdentifierFixture(), - StartState: unittest.StateCommitmentFixture(), - Proof: []byte{'p'}, - CollectionID: collectionID, - } - require.NoError(t, unittest.WithLock(t, lockManager, storage.LockInsertChunkDataPack, func(lctx lockctx.Context) error { - return operation.InsertChunkDataPack(lctx, rw, cdp) - })) - } + // insert 100 chunk data packs + return unittest.WithLock(t, lockManager, storage.LockInsertChunkDataPack, func(lctx2 lockctx.Context) error { + for i := 0; i < 100; i++ { + collectionID := unittest.IdentifierFixture() + cdp := &storage.StoredChunkDataPack{ + ChunkID: unittest.IdentifierFixture(), + StartState: unittest.StateCommitmentFixture(), + Proof: []byte{'p'}, + CollectionID: collectionID, + } + err := operation.InsertChunkDataPack(lctx2, rw, cdp) + if err != nil { + return err + } + } + return nil + }) + }) + }) + require.NoError(t, err) - // insert 20 results + // insert 20 results + err = db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { for i := 0; i < 20; i++ { result := unittest.ExecutionResultFixture() - err := operation.InsertExecutionResult(rw.Writer(), result) + err := operation.InsertExecutionResult(rw.Writer(), result.ID(), result) if err != nil { return err } } - return nil }) require.NoError(t, err) diff --git a/storage/operation/transaction_results.go b/storage/operation/transaction_results.go index a97197a5cde..debdd432ae2 100644 --- a/storage/operation/transaction_results.go +++ b/storage/operation/transaction_results.go @@ -3,15 +3,55 @@ package operation import ( "fmt" + "github.com/jordanschalm/lockctx" + "github.com/onflow/flow-go/model/flow" "github.com/onflow/flow-go/storage" ) -func InsertTransactionResult(w storage.Writer, blockID flow.Identifier, transactionResult *flow.TransactionResult) error { +// InsertAndIndexTransactionResults inserts and indexes multiple transaction results in a single batch write. +// The caller must hold the [storage.LockInsertAndIndexTxResult] lock. +// It returns [storage.ErrAlreadyExists] if transaction results for the block already exist. +func InsertAndIndexTransactionResults(lctx lockctx.Proof, rw storage.ReaderBatchWriter, blockID flow.Identifier, transactionResults []flow.TransactionResult) error { + if !lctx.HoldsLock(storage.LockInsertAndIndexTxResult) { + return fmt.Errorf("InsertTransactionResult requires LockInsertAndIndexTxResult to be held") + } + + // Check if transaction results for the block already exist + // We can exit early if we found one result exist + // Because regardless transactionResults is empty, or has one, or more results, + // we don't want to overwrite existing results + prefix := MakePrefix(codeTransactionResult, blockID) + checkExists := func(key []byte) error { + return fmt.Errorf("transaction results for block %v already exist: %w", blockID, storage.ErrAlreadyExists) + } + err := IterateKeysByPrefixRange(rw.GlobalReader(), prefix, prefix, checkExists) + if err != nil { + return err + } + + // there is no existing transaction result for the block, we can proceed to insert + w := rw.Writer() + for i, result := range transactionResults { + err := insertTransactionResult(w, blockID, &result) + if err != nil { + return fmt.Errorf("cannot batch insert tx result: %w", err) + } + + err = indexTransactionResult(w, blockID, uint32(i), &result) + if err != nil { + return fmt.Errorf("cannot batch index tx result: %w", err) + } + } + + return nil +} + +func insertTransactionResult(w storage.Writer, blockID flow.Identifier, transactionResult *flow.TransactionResult) error { return UpsertByKey(w, MakePrefix(codeTransactionResult, blockID, transactionResult.TransactionID), transactionResult) } -func IndexTransactionResult(w storage.Writer, blockID flow.Identifier, txIndex uint32, transactionResult *flow.TransactionResult) error { +func indexTransactionResult(w storage.Writer, blockID flow.Identifier, txIndex uint32, transactionResult *flow.TransactionResult) error { return UpsertByKey(w, MakePrefix(codeTransactionResultIndex, blockID, txIndex), transactionResult) } @@ -64,11 +104,6 @@ func BatchRemoveTransactionResultsByBlockID(blockID flow.Identifier, batch stora return nil } -// deprecated -func InsertLightTransactionResult(w storage.Writer, blockID flow.Identifier, transactionResult *flow.LightTransactionResult) error { - return UpsertByKey(w, MakePrefix(codeLightTransactionResult, blockID, transactionResult.TransactionID), transactionResult) -} - func BatchInsertLightTransactionResult(w storage.Writer, blockID flow.Identifier, transactionResult *flow.LightTransactionResult) error { return UpsertByKey(w, MakePrefix(codeLightTransactionResult, blockID, transactionResult.TransactionID), transactionResult) } diff --git a/storage/operation/transaction_results_test.go b/storage/operation/transaction_results_test.go new file mode 100644 index 00000000000..3e292757c7f --- /dev/null +++ b/storage/operation/transaction_results_test.go @@ -0,0 +1,198 @@ +package operation_test + +import ( + "testing" + + "github.com/jordanschalm/lockctx" + "github.com/stretchr/testify/assert" + "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" +) + +func TestInsertAndIndexTransactionResults(t *testing.T) { + dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { + lockManager := storage.NewTestingLockManager() + blockID := unittest.IdentifierFixture() + + // Create test transaction results + transactionResults := unittest.TransactionResultsFixture(3) + + // Test successful insertion and indexing + err := unittest.WithLock(t, lockManager, storage.LockInsertAndIndexTxResult, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return operation.InsertAndIndexTransactionResults(lctx, rw, blockID, transactionResults) + }) + }) + require.NoError(t, err) + + // Verify that transaction results can be retrieved by transaction ID + for i, expected := range transactionResults { + var actual flow.TransactionResult + err = operation.RetrieveTransactionResult(db.Reader(), blockID, expected.TransactionID, &actual) + require.NoError(t, err) + assert.Equal(t, expected, actual) + + // Verify that transaction results can be retrieved by index + var actualByIndex flow.TransactionResult + err = operation.RetrieveTransactionResultByIndex(db.Reader(), blockID, uint32(i), &actualByIndex) + require.NoError(t, err) + assert.Equal(t, expected, actualByIndex) + } + + // Verify that all transaction results can be retrieved using the index + var retrievedResults []flow.TransactionResult + err = operation.LookupTransactionResultsByBlockIDUsingIndex(db.Reader(), blockID, &retrievedResults) + require.NoError(t, err) + assert.Len(t, retrievedResults, len(transactionResults)) + + // Verify the order matches the original order + for i, expected := range transactionResults { + assert.Equal(t, expected, retrievedResults[i]) + } + }) +} + +func TestInsertAndIndexTransactionResults_AlreadyExists(t *testing.T) { + dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { + lockManager := storage.NewTestingLockManager() + blockID := unittest.IdentifierFixture() + + // Create initial transaction results + initialResults := unittest.TransactionResultsFixture(1) + + // Insert initial results + err := unittest.WithLock(t, lockManager, storage.LockInsertAndIndexTxResult, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return operation.InsertAndIndexTransactionResults(lctx, rw, blockID, initialResults) + }) + }) + require.NoError(t, err) + + // Try to insert results for the same block again - should fail + duplicateResults := unittest.TransactionResultsFixture(1) + + err = unittest.WithLock(t, lockManager, storage.LockInsertAndIndexTxResult, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return operation.InsertAndIndexTransactionResults(lctx, rw, blockID, duplicateResults) + }) + }) + require.Error(t, err) + require.ErrorIs(t, err, storage.ErrAlreadyExists) + }) +} + +func TestInsertAndIndexTransactionResults_NoLock(t *testing.T) { + dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { + blockID := unittest.IdentifierFixture() + transactionResults := unittest.TransactionResultsFixture(1) + + // Try to insert without holding the required lock - should fail + err := db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + // Create a context without the required lock + lockManager := storage.NewTestingLockManager() + lctx := lockManager.NewContext() + defer lctx.Release() + + return operation.InsertAndIndexTransactionResults(lctx, rw, blockID, transactionResults) + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "InsertTransactionResult requires LockInsertAndIndexTxResult to be held") + }) +} + +func TestInsertAndIndexTransactionResults_EmptyResults(t *testing.T) { + dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { + lockManager := storage.NewTestingLockManager() + blockID := unittest.IdentifierFixture() + + // Test with empty transaction results + emptyResults := []flow.TransactionResult{} + + err := unittest.WithLock(t, lockManager, storage.LockInsertAndIndexTxResult, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return operation.InsertAndIndexTransactionResults(lctx, rw, blockID, emptyResults) + }) + }) + require.NoError(t, err) + + // Verify that no results are stored + var retrievedResults []flow.TransactionResult + err = operation.LookupTransactionResultsByBlockIDUsingIndex(db.Reader(), blockID, &retrievedResults) + require.NoError(t, err) + assert.Len(t, retrievedResults, 0) + }) +} + +func TestInsertAndIndexTransactionResults_SingleResult(t *testing.T) { + dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { + lockManager := storage.NewTestingLockManager() + blockID := unittest.IdentifierFixture() + + // Test with single transaction result + singleResult := unittest.TransactionResultsFixture(1) + + err := unittest.WithLock(t, lockManager, storage.LockInsertAndIndexTxResult, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return operation.InsertAndIndexTransactionResults(lctx, rw, blockID, singleResult) + }) + }) + require.NoError(t, err) + + // Verify the single result can be retrieved + var retrievedResults []flow.TransactionResult + err = operation.LookupTransactionResultsByBlockIDUsingIndex(db.Reader(), blockID, &retrievedResults) + require.NoError(t, err) + assert.Len(t, retrievedResults, 1) + assert.Equal(t, singleResult[0], retrievedResults[0]) + }) +} + +func TestInsertAndIndexTransactionResults_MultipleResultsWithSameTransactionID(t *testing.T) { + dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { + lockManager := storage.NewTestingLockManager() + blockID := unittest.IdentifierFixture() + + // Create transaction results with the same transaction ID (duplicate transactions in block) + transactionID := unittest.IdentifierFixture() + results := []flow.TransactionResult{ + { + TransactionID: transactionID, + ErrorMessage: "error1", + ComputationUsed: 100, + MemoryUsed: 200, + }, + { + TransactionID: transactionID, + ErrorMessage: "error2", + ComputationUsed: 150, + MemoryUsed: 250, + }, + } + + err := unittest.WithLock(t, lockManager, storage.LockInsertAndIndexTxResult, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return operation.InsertAndIndexTransactionResults(lctx, rw, blockID, results) + }) + }) + require.NoError(t, err) + + // Verify both results are stored and can be retrieved by index + for i, expected := range results { + var actual flow.TransactionResult + err = operation.RetrieveTransactionResultByIndex(db.Reader(), blockID, uint32(i), &actual) + require.NoError(t, err) + assert.Equal(t, expected, actual) + } + + // Verify that lookup by transaction ID returns the last stored result (due to overwrite) + var actual flow.TransactionResult + err = operation.RetrieveTransactionResult(db.Reader(), blockID, transactionID, &actual) + require.NoError(t, err) + assert.Equal(t, results[1], actual) // Should be the last one stored + }) +} diff --git a/storage/results.go b/storage/results.go index a943866370e..fb00c7db3d6 100644 --- a/storage/results.go +++ b/storage/results.go @@ -1,6 +1,8 @@ package storage import ( + "github.com/jordanschalm/lockctx" + "github.com/onflow/flow-go/model/flow" ) @@ -10,28 +12,25 @@ type ExecutionResultsReader interface { // ByBlockID retrieves an execution result by block ID. ByBlockID(blockID flow.Identifier) (*flow.ExecutionResult, error) + + // IDByBlockID retrieves an execution result ID by block ID. + IDByBlockID(blockID flow.Identifier) (flow.Identifier, error) } type ExecutionResults interface { ExecutionResultsReader - // Store stores an execution result. - Store(result *flow.ExecutionResult) error - // BatchStore stores an execution result in a given batch + // No error is expected during normal operation. BatchStore(result *flow.ExecutionResult, batch ReaderBatchWriter) error - // Index indexes an execution result by block ID. - Index(blockID flow.Identifier, resultID flow.Identifier) error - - // ForceIndex indexes an execution result by block ID overwriting existing database entry - ForceIndex(blockID flow.Identifier, resultID flow.Identifier) error - // BatchIndex indexes an execution result by block ID in a given batch - BatchIndex(blockID flow.Identifier, resultID flow.Identifier, batch ReaderBatchWriter) error + // The caller must acquire [storage.LockIndexExecutionResult] + // It returns [storage.ErrDataMismatch] if there is already an indexed result for the given blockID, + // but it is different from the given resultID. + BatchIndex(lctx lockctx.Proof, rw ReaderBatchWriter, blockID flow.Identifier, resultID flow.Identifier) error // BatchRemoveIndexByBlockID removes blockID-to-executionResultID index entries keyed by blockID in a provided batch. // No errors are expected during normal operation, even if no entries are matched. - // If Badger unexpectedly fails to process the request, the error is wrapped in a generic error and returned. BatchRemoveIndexByBlockID(blockID flow.Identifier, batch ReaderBatchWriter) error } diff --git a/storage/store/blocks.go b/storage/store/blocks.go index c34a0cc53ea..f501380c199 100644 --- a/storage/store/blocks.go +++ b/storage/store/blocks.go @@ -215,24 +215,10 @@ func (b *Blocks) ByCollectionID(collID flow.Identifier) (*flow.Block, error) { return b.ByID(blockID) } -// IndexBlockContainingCollectionGuarantees populates an index `guaranteeID->blockID` for each guarantee -// which appears in the block. -// CAUTION: a collection can be included in multiple *unfinalized* blocks. However, the implementation -// assumes a one-to-one map from collection ID to a *single* block ID. This holds for FINALIZED BLOCKS ONLY -// *and* only in the absence of byzantine collector clusters (which the mature protocol must tolerate). -// Hence, this function should be treated as a temporary solution, which requires generalization -// (one-to-many mapping) for soft finality and the mature protocol. -// +// BatchIndexBlockContainingCollectionGuarantees produces mappings from the IDs of [flow.CollectionGuarantee]s to the block ID containing these guarantees. +// The caller must acquire a storage.LockIndexCollectionByBlock lock. // Error returns: -// - generic error in case of unexpected failure from the database layer or encoding failure. -func (b *Blocks) IndexBlockContainingCollectionGuarantees(blockID flow.Identifier, guaranteeIDs []flow.Identifier) error { - return b.db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { - for _, guaranteeID := range guaranteeIDs { - err := operation.IndexBlockContainingCollectionGuarantee(rw.Writer(), guaranteeID, blockID) - if err != nil { - return fmt.Errorf("could not index collection block (%x): %w", guaranteeID, err) - } - } - return nil - }) +// - storage.ErrAlreadyExists if any collection ID has already been indexed +func (b *Blocks) BatchIndexBlockContainingCollectionGuarantees(lctx lockctx.Proof, rw storage.ReaderBatchWriter, blockID flow.Identifier, collIDs []flow.Identifier) error { + return operation.BatchIndexBlockContainingCollectionGuarantees(lctx, rw, blockID, collIDs) } diff --git a/storage/store/chunk_data_packs.go b/storage/store/chunk_data_packs.go index 0568dd8f5c9..81aff93e46f 100644 --- a/storage/store/chunk_data_packs.go +++ b/storage/store/chunk_data_packs.go @@ -79,7 +79,6 @@ func (ch *ChunkDataPacks) StoreByChunkID(lctx lockctx.Proof, cs []*flow.ChunkDat // BatchRemove removes ChunkDataPack c keyed by its ChunkID in provided batch // No errors are expected during normal operation, even if no entries are matched. -// If Badger unexpectedly fails to process the request, the error is wrapped in a generic error and returned. func (ch *ChunkDataPacks) BatchRemove(chunkID flow.Identifier, rw storage.ReaderBatchWriter) error { storage.OnCommitSucceed(rw, func() { ch.byChunkIDCache.Remove(chunkID) diff --git a/storage/store/commits_test.go b/storage/store/commits_test.go index 4fcc00f37e5..259824582b2 100644 --- a/storage/store/commits_test.go +++ b/storage/store/commits_test.go @@ -30,7 +30,7 @@ func TestCommitsStoreAndRetrieve(t *testing.T) { // store a commit in db blockID := unittest.IdentifierFixture() expected := unittest.StateCommitmentFixture() - err = unittest.WithLock(t, lockManager, storage.LockInsertOwnReceipt, func(lctx lockctx.Context) error { + err = unittest.WithLock(t, lockManager, storage.LockIndexStateCommitment, func(lctx lockctx.Context) error { return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { return store1.BatchStore(lctx, blockID, expected, rw) }) @@ -43,7 +43,7 @@ func TestCommitsStoreAndRetrieve(t *testing.T) { assert.Equal(t, expected, actual) // re-insert the commit - should be idempotent - err = unittest.WithLock(t, lockManager, storage.LockInsertOwnReceipt, func(lctx lockctx.Context) error { + err = unittest.WithLock(t, lockManager, storage.LockIndexStateCommitment, func(lctx lockctx.Context) error { return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { return store1.BatchStore(lctx, blockID, expected, rw) }) @@ -61,7 +61,7 @@ func TestCommitStoreAndRemove(t *testing.T) { // Create and store a commit blockID := unittest.IdentifierFixture() expected := unittest.StateCommitmentFixture() - err := unittest.WithLock(t, lockManager, storage.LockInsertOwnReceipt, func(lctx lockctx.Context) error { + err := unittest.WithLock(t, lockManager, storage.LockIndexStateCommitment, func(lctx lockctx.Context) error { return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { return store.BatchStore(lctx, blockID, expected, rw) }) diff --git a/storage/store/events.go b/storage/store/events.go index dc6283e8d02..8cc9ccb207b 100644 --- a/storage/store/events.go +++ b/storage/store/events.go @@ -3,6 +3,8 @@ 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" @@ -37,11 +39,14 @@ func NewEvents(collector module.CacheMetrics, db storage.DB) *Events { )} } -// BatchStore stores events keyed by a blockID in provided batch -// No errors are expected during normal operation, but it may return generic error -// if badger fails to process request -func (e *Events) BatchStore(blockID flow.Identifier, blockEvents []flow.EventsList, batch storage.ReaderBatchWriter) error { - writer := batch.Writer() +// BatchStore will store events for the given block ID in a given batch +// it requires the caller to hold [storage.LockInsertEvent] +func (e *Events) BatchStore(lctx lockctx.Proof, blockID flow.Identifier, blockEvents []flow.EventsList, batch storage.ReaderBatchWriter) error { + // Use the new InsertBlockEvents operation to store all events + err := operation.InsertBlockEvents(lctx, batch, blockID, blockEvents) + if err != nil { + return fmt.Errorf("cannot batch insert events: %w", err) + } // pre-allocating and indexing slice is faster than appending sliceSize := 0 @@ -50,32 +55,19 @@ func (e *Events) BatchStore(blockID flow.Identifier, blockEvents []flow.EventsLi } combinedEvents := make([]flow.Event, sliceSize) - eventIndex := 0 for _, events := range blockEvents { for _, event := range events { - err := operation.InsertEvent(writer, blockID, event) - if err != nil { - return fmt.Errorf("cannot batch insert event: %w", err) - } combinedEvents[eventIndex] = event eventIndex++ } } - callback := func() { + storage.OnCommitSucceed(batch, func() { e.cache.Insert(blockID, combinedEvents) - } - storage.OnCommitSucceed(batch, callback) - return nil -} - -// Store will store events for the given block ID -func (e *Events) Store(blockID flow.Identifier, blockEvents []flow.EventsList) error { - return e.db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { - return e.BatchStore(blockID, blockEvents, rw) }) + return nil } // ByBlockID returns the events for the given block ID @@ -148,7 +140,6 @@ func (e *Events) RemoveByBlockID(blockID flow.Identifier) error { // BatchRemoveByBlockID removes events keyed by a blockID in provided batch // No errors are expected during normal operation, even if no entries are matched. -// If Badger unexpectedly fails to process the request, the error is wrapped in a generic error and returned. func (e *Events) BatchRemoveByBlockID(blockID flow.Identifier, rw storage.ReaderBatchWriter) error { return e.cache.RemoveTx(rw, blockID) } @@ -180,14 +171,11 @@ func NewServiceEvents(collector module.CacheMetrics, db storage.DB) *ServiceEven // BatchStore stores service events keyed by a blockID in provided batch // No errors are expected during normal operation, even if no entries are matched. -// If Badger unexpectedly fails to process the request, the error is wrapped in a generic error and returned. -func (e *ServiceEvents) BatchStore(blockID flow.Identifier, events []flow.Event, rw storage.ReaderBatchWriter) error { - writer := rw.Writer() - for _, event := range events { - err := operation.InsertServiceEvent(writer, blockID, event) - if err != nil { - return fmt.Errorf("cannot batch insert service event: %w", err) - } +func (e *ServiceEvents) BatchStore(lctx lockctx.Proof, blockID flow.Identifier, events []flow.Event, rw storage.ReaderBatchWriter) error { + // Use the new InsertBlockServiceEvents operation to store all service events + err := operation.InsertBlockServiceEvents(lctx, rw, blockID, events) + if err != nil { + return fmt.Errorf("cannot batch insert service events: %w", err) } callback := func() { @@ -215,7 +203,6 @@ func (e *ServiceEvents) RemoveByBlockID(blockID flow.Identifier) error { // BatchRemoveByBlockID removes service events keyed by a blockID in provided batch // No errors are expected during normal operation, even if no entries are matched. -// If Badger unexpectedly fails to process the request, the error is wrapped in a generic error and returned. func (e *ServiceEvents) BatchRemoveByBlockID(blockID flow.Identifier, rw storage.ReaderBatchWriter) error { return e.cache.RemoveTx(rw, blockID) } diff --git a/storage/store/events_test.go b/storage/store/events_test.go index 3db86249e8d..4a70a895000 100644 --- a/storage/store/events_test.go +++ b/storage/store/events_test.go @@ -4,6 +4,7 @@ import ( "math/rand" "testing" + "github.com/jordanschalm/lockctx" "github.com/stretchr/testify/require" "github.com/onflow/flow-go/fvm/systemcontracts" @@ -16,6 +17,7 @@ import ( ) func TestEventStoreRetrieve(t *testing.T) { + lockManager := storage.NewTestingLockManager() dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { metrics := metrics.NewNoopCollector() events := store.NewEvents(metrics, db) @@ -48,10 +50,13 @@ func TestEventStoreRetrieve(t *testing.T) { {evt2_1}, } - require.NoError(t, db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { - // store event - return events.BatchStore(blockID, expected, rw) - })) + err := unittest.WithLock(t, lockManager, storage.LockInsertEvent, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + // store event + return events.BatchStore(lctx, blockID, expected, rw) + }) + }) + require.NoError(t, err) // retrieve by blockID actual, err := events.ByBlockID(blockID) @@ -136,6 +141,7 @@ func TestEventRetrieveWithoutStore(t *testing.T) { } func TestEventStoreAndRemove(t *testing.T) { + lockManager := storage.NewTestingLockManager() dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { metrics := metrics.NewNoopCollector() store := store.NewEvents(metrics, db) @@ -169,7 +175,11 @@ func TestEventStoreAndRemove(t *testing.T) { {evt2_1}, } - err := store.Store(blockID, expected) + err := unittest.WithLock(t, lockManager, storage.LockInsertEvent, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return store.BatchStore(lctx, blockID, expected, rw) + }) + }) require.NoError(t, err) // Ensure it exists diff --git a/storage/store/light_transaction_results.go b/storage/store/light_transaction_results.go index dd8cab8e29a..7cb85644064 100644 --- a/storage/store/light_transaction_results.go +++ b/storage/store/light_transaction_results.go @@ -3,6 +3,8 @@ 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" @@ -71,7 +73,10 @@ func NewLightTransactionResults(collector module.CacheMetrics, db storage.DB, tr } } -func (tr *LightTransactionResults) BatchStore(blockID flow.Identifier, transactionResults []flow.LightTransactionResult, rw storage.ReaderBatchWriter) error { +func (tr *LightTransactionResults) BatchStore(lctx lockctx.Proof, blockID flow.Identifier, transactionResults []flow.LightTransactionResult, rw storage.ReaderBatchWriter) error { + if !lctx.HoldsLock(storage.LockInsertLightTransactionResult) { + return fmt.Errorf("BatchStore LightTransactionResults requires %v", storage.LockInsertLightTransactionResult) + } w := rw.Writer() for i, result := range transactionResults { @@ -103,10 +108,6 @@ func (tr *LightTransactionResults) BatchStore(blockID flow.Identifier, transacti return nil } -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 func (tr *LightTransactionResults) ByBlockIDTransactionID(blockID flow.Identifier, txID flow.Identifier) (*flow.LightTransactionResult, error) { key := KeyFromBlockIDTransactionID(blockID, txID) diff --git a/storage/store/light_transaction_results_test.go b/storage/store/light_transaction_results_test.go index c3ea965ab72..76431d82746 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" @@ -16,6 +17,7 @@ import ( ) func TestBatchStoringLightTransactionResults(t *testing.T) { + lockManager := storage.NewTestingLockManager() dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { metrics := metrics.NewNoopCollector() store1 := store.NewLightTransactionResults(metrics, db, 1000) @@ -24,14 +26,20 @@ 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) - })) + err := unittest.WithLock(t, lockManager, storage.LockInsertLightTransactionResult, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return store1.BatchStore(lctx, blockID, txResults, rw) + }) + }) + require.NoError(t, err) // 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) - })) + err = unittest.WithLock(t, lockManager, storage.LockInsertLightTransactionResult, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return store1.BatchStore(lctx, unittest.IdentifierFixture(), getLightTransactionResultsFixture(2), rw) + }) + }) + require.NoError(t, err) }) diff --git a/storage/store/my_receipts.go b/storage/store/my_receipts.go index 02c79cbbb56..352bf7877a2 100644 --- a/storage/store/my_receipts.go +++ b/storage/store/my_receipts.go @@ -1,14 +1,12 @@ package store import ( - "errors" "fmt" "github.com/jordanschalm/lockctx" "github.com/onflow/flow-go/model/flow" "github.com/onflow/flow-go/module" - "github.com/onflow/flow-go/module/irrecoverable" "github.com/onflow/flow-go/module/metrics" "github.com/onflow/flow-go/storage" "github.com/onflow/flow-go/storage/operation" @@ -64,6 +62,7 @@ func (m *MyExecutionReceipts) myReceipt(blockID flow.Identifier) (*flow.Executio // // If entity fails marshalling, the error is wrapped in a generic error and returned. // If database unexpectedly fails to process the request, the error is wrapped in a generic error and returned. +// It requires [storage.LockInsertOwnReceipt] to be held. // // Expected error returns during *normal* operations: // - `storage.ErrDataMismatch` if a *different* receipt has already been indexed for the same block @@ -71,29 +70,14 @@ func (m *MyExecutionReceipts) BatchStoreMyReceipt(lctx lockctx.Proof, receipt *f receiptID := receipt.ID() blockID := receipt.ExecutionResult.BlockID - if lctx == nil || !lctx.HoldsLock(storage.LockInsertOwnReceipt) { - return fmt.Errorf("cannot store my receipt, missing lock %v", storage.LockInsertOwnReceipt) - } - // add DB operation to batch for storing receipt (execution deferred until batch is committed) err := m.genericReceipts.BatchStore(receipt, rw) if err != nil { return err } - // dd DB operation to batch for indexing receipt as one of my own (execution deferred until batch is committed) - var savedReceiptID flow.Identifier - err = operation.LookupOwnExecutionReceipt(rw.GlobalReader(), blockID, &savedReceiptID) - if err == nil { - if savedReceiptID == receiptID { - return nil // no-op we are storing *same* receipt - } - return fmt.Errorf("indexing my receipt %v failed: different receipt %v for the same block %v is already indexed: %w", receiptID, savedReceiptID, blockID, storage.ErrDataMismatch) - } - if !errors.Is(err, storage.ErrNotFound) { // `storage.ErrNotFound` is expected, as this indicates that no receipt is indexed yet; anything else is an exception - return irrecoverable.NewException(err) - } - err = operation.IndexOwnExecutionReceipt(rw.Writer(), blockID, receiptID) + // require [storage.LockInsertOwnReceipt] to be held + err = operation.IndexOwnExecutionReceipt(lctx, rw, blockID, receiptID) if err != nil { return err } @@ -111,15 +95,8 @@ func (m *MyExecutionReceipts) MyReceipt(blockID flow.Identifier) (*flow.Executio return m.myReceipt(blockID) } -func (m *MyExecutionReceipts) RemoveIndexByBlockID(blockID flow.Identifier) error { - return m.db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { - return m.BatchRemoveIndexByBlockID(blockID, rw) - }) -} - // BatchRemoveIndexByBlockID removes blockID-to-my-execution-receipt index entry keyed by a blockID in a provided batch // No errors are expected during normal operation, even if no entries are matched. -// If Badger unexpectedly fails to process the request, the error is wrapped in a generic error and returned. func (m *MyExecutionReceipts) BatchRemoveIndexByBlockID(blockID flow.Identifier, rw storage.ReaderBatchWriter) error { return m.cache.RemoveTx(rw, blockID) } diff --git a/storage/store/my_receipts_test.go b/storage/store/my_receipts_test.go index 62d1e6e0286..9760440b6c6 100644 --- a/storage/store/my_receipts_test.go +++ b/storage/store/my_receipts_test.go @@ -154,7 +154,7 @@ func TestMyExecutionReceiptsStorage(t *testing.T) { for err := range errChan { if err != nil { errCount++ - require.Contains(t, err.Error(), "different receipt") + require.Contains(t, err.Error(), "data for key is different") } } require.Equal(t, 1, errCount, "Exactly one of the operations should fail") diff --git a/storage/store/results.go b/storage/store/results.go index 45c269f5a7f..c9966765224 100644 --- a/storage/store/results.go +++ b/storage/store/results.go @@ -3,6 +3,8 @@ 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" @@ -12,16 +14,17 @@ import ( // ExecutionResults implements persistent storage for execution results. type ExecutionResults struct { - db storage.DB - cache *Cache[flow.Identifier, *flow.ExecutionResult] + db storage.DB + cache *Cache[flow.Identifier, *flow.ExecutionResult] + indexCache *Cache[flow.Identifier, flow.Identifier] } var _ storage.ExecutionResults = (*ExecutionResults)(nil) func NewExecutionResults(collector module.CacheMetrics, db storage.DB) *ExecutionResults { - store := func(rw storage.ReaderBatchWriter, _ flow.Identifier, result *flow.ExecutionResult) error { - return operation.InsertExecutionResult(rw.Writer(), result) + store := func(rw storage.ReaderBatchWriter, resultID flow.Identifier, result *flow.ExecutionResult) error { + return operation.InsertExecutionResult(rw.Writer(), resultID, result) } retrieve := func(r storage.Reader, resultID flow.Identifier) (*flow.ExecutionResult, error) { @@ -30,12 +33,26 @@ func NewExecutionResults(collector module.CacheMetrics, db storage.DB) *Executio return &result, err } + retrieveByBlockID := func(r storage.Reader, blockID flow.Identifier) (flow.Identifier, error) { + var resultID flow.Identifier + err := operation.LookupExecutionResult(r, blockID, &resultID) + return resultID, err + } + res := &ExecutionResults{ db: db, cache: newCache(collector, metrics.ResourceResult, withLimit[flow.Identifier, *flow.ExecutionResult](flow.DefaultTransactionExpiry+100), withStore(store), withRetrieve(retrieve)), + + indexCache: newCache(collector, metrics.ResourceResult, + // this API is only used to fetch result for last executed block, so in happy case, it only need to be 1, + // we use 100 here to be more resilient to forks + withLimit[flow.Identifier, flow.Identifier](100), + withStoreWithLock(operation.IndexOwnOrSealedExecutionResult), + withRetrieve(retrieveByBlockID), + ), } return res @@ -54,107 +71,46 @@ func (r *ExecutionResults) byID(resultID flow.Identifier) (*flow.ExecutionResult } func (r *ExecutionResults) byBlockID(blockID flow.Identifier) (*flow.ExecutionResult, error) { - var resultID flow.Identifier - err := operation.LookupExecutionResult(r.db.Reader(), blockID, &resultID) + resultID, err := r.IDByBlockID(blockID) if err != nil { return nil, fmt.Errorf("could not lookup execution result ID: %w", err) } return r.byID(resultID) } -func (r *ExecutionResults) index(w storage.Writer, blockID, resultID flow.Identifier, force bool) error { - if !force { - // when not forcing the index, check if the result is already indexed - exist, err := operation.ExistExecutionResult(r.db.Reader(), blockID) - if err != nil { - return fmt.Errorf("could not check if execution result exists: %w", err) - } - - // if the result is already indexed, check if the stored result is the same - if exist { - var storedResultID flow.Identifier - err = operation.LookupExecutionResult(r.db.Reader(), blockID, &storedResultID) - if err != nil { - return fmt.Errorf("could not lookup execution result ID: %w", err) - } - - if storedResultID != resultID { - return fmt.Errorf("storing result that is different from the already stored one for block: %v, storing result: %v, stored result: %v. %w", - blockID, resultID, storedResultID, storage.ErrDataMismatch) - } - - // if the result is the same, we don't need to index it again - return nil - } - - // if the result is not indexed, we can index it - } - - err := operation.IndexExecutionResult(w, blockID, resultID) - if err == nil { - return nil - } - - return nil -} - -func (r *ExecutionResults) Store(result *flow.ExecutionResult) error { - return r.db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { - return r.store(rw, result) - }) +// BatchIndex indexes an execution result by block ID in a given batch +// The caller must acquire [storage.LockIndexExecutionResult] +// It returns [storage.ErrDataMismatch] if there is already an indexed result for the given blockID, +// but it is different from the given resultID. +func (r *ExecutionResults) BatchIndex(lctx lockctx.Proof, rw storage.ReaderBatchWriter, blockID flow.Identifier, resultID flow.Identifier) error { + return r.indexCache.PutWithLockTx(lctx, rw, blockID, resultID) } +// BatchStore stores an execution result in a given batch +// No error is expected during normal operation. func (r *ExecutionResults) BatchStore(result *flow.ExecutionResult, batch storage.ReaderBatchWriter) error { return r.store(batch, result) } -func (r *ExecutionResults) BatchIndex(blockID flow.Identifier, resultID flow.Identifier, batch storage.ReaderBatchWriter) error { - return operation.IndexExecutionResult(batch.Writer(), blockID, resultID) -} - +// ByID retrieves an execution result by its ID. Returns `ErrNotFound` if `resultID` is unknown. func (r *ExecutionResults) ByID(resultID flow.Identifier) (*flow.ExecutionResult, error) { return r.byID(resultID) } -// Index indexes an execution result by block ID. -// Note: this method call is not concurrent safe, because it checks if the different result is already indexed -// by the same blockID, and if it is, it returns an error. -// The caller needs to ensure that there is no concurrent call to this method with the same blockID. -func (r *ExecutionResults) Index(blockID flow.Identifier, resultID flow.Identifier) error { - err := r.db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { - return r.index(rw.Writer(), blockID, resultID, false) - }) - - if err != nil { - return fmt.Errorf("could not index execution result: %w", err) - } - return nil -} - -func (r *ExecutionResults) ForceIndex(blockID flow.Identifier, resultID flow.Identifier) error { - err := r.db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { - return r.index(rw.Writer(), blockID, resultID, true) - }) - - if err != nil { - return fmt.Errorf("could not index execution result: %w", err) - } - return nil -} - +// ByBlockID retrieves an execution result by block ID. +// It returns [storage.ErrNotFound] if `blockID` does not refer to a block executed by this node func (r *ExecutionResults) ByBlockID(blockID flow.Identifier) (*flow.ExecutionResult, error) { return r.byBlockID(blockID) } -func (r *ExecutionResults) RemoveIndexByBlockID(blockID flow.Identifier) error { - return r.db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { - return operation.RemoveExecutionResultIndex(rw.Writer(), blockID) - }) +// IDByBlockID retrieves an execution result ID by block ID. +// It returns [storage.ErrNotFound] if `blockID` does not refer to a block executed by this node +func (r *ExecutionResults) IDByBlockID(blockID flow.Identifier) (flow.Identifier, error) { + return r.indexCache.Get(r.db.Reader(), blockID) } // BatchRemoveIndexByBlockID removes blockID-to-executionResultID index entries keyed by blockID in a provided batch. // No errors are expected during normal operation, even if no entries are matched. -// If Badger unexpectedly fails to process the request, the error is wrapped in a generic error and returned. func (r *ExecutionResults) BatchRemoveIndexByBlockID(blockID flow.Identifier, batch storage.ReaderBatchWriter) error { return operation.RemoveExecutionResultIndex(batch.Writer(), blockID) } diff --git a/storage/store/results_test.go b/storage/store/results_test.go index 34f63e1f885..3afef928316 100644 --- a/storage/store/results_test.go +++ b/storage/store/results_test.go @@ -4,6 +4,7 @@ import ( "errors" "testing" + "github.com/jordanschalm/lockctx" "github.com/stretchr/testify/require" "github.com/onflow/flow-go/module/metrics" @@ -14,16 +15,24 @@ import ( ) func TestResultStoreAndRetrieve(t *testing.T) { + lockManager := storage.NewTestingLockManager() dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { metrics := metrics.NewNoopCollector() store1 := store.NewExecutionResults(metrics, db) result := unittest.ExecutionResultFixture() blockID := unittest.IdentifierFixture() - err := store1.Store(result) - require.NoError(t, err) - err = store1.Index(blockID, result.ID()) + err := unittest.WithLock(t, lockManager, storage.LockIndexExecutionResult, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + err := store1.BatchStore(result, rw) + require.NoError(t, err) + + err = store1.BatchIndex(lctx, rw, blockID, result.ID()) + require.NoError(t, err) + return nil + }) + }) require.NoError(t, err) actual, err := store1.ByBlockID(blockID) @@ -34,27 +43,42 @@ func TestResultStoreAndRetrieve(t *testing.T) { } func TestResultStoreTwice(t *testing.T) { + lockManager := storage.NewTestingLockManager() dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { metrics := metrics.NewNoopCollector() store1 := store.NewExecutionResults(metrics, db) result := unittest.ExecutionResultFixture() blockID := unittest.IdentifierFixture() - err := store1.Store(result) - require.NoError(t, err) - err = store1.Index(blockID, result.ID()) - require.NoError(t, err) + err := unittest.WithLock(t, lockManager, storage.LockIndexExecutionResult, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + err := store1.BatchStore(result, rw) + require.NoError(t, err) - err = store1.Store(result) + err = store1.BatchIndex(lctx, rw, blockID, result.ID()) + require.NoError(t, err) + return nil + }) + }) require.NoError(t, err) - err = store1.Index(blockID, result.ID()) + err = unittest.WithLock(t, lockManager, storage.LockIndexExecutionResult, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + err := store1.BatchStore(result, rw) + require.NoError(t, err) + + err = store1.BatchIndex(lctx, rw, blockID, result.ID()) + require.NoError(t, err) + return nil + }) + }) require.NoError(t, err) }) } func TestResultBatchStoreTwice(t *testing.T) { + lockManager := storage.NewTestingLockManager() dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { metrics := metrics.NewNoopCollector() store1 := store.NewExecutionResults(metrics, db) @@ -62,28 +86,35 @@ func TestResultBatchStoreTwice(t *testing.T) { result := unittest.ExecutionResultFixture() blockID := unittest.IdentifierFixture() - require.NoError(t, db.WithReaderBatchWriter(func(batch storage.ReaderBatchWriter) error { - err := store1.BatchStore(result, batch) - require.NoError(t, err) + err := unittest.WithLock(t, lockManager, storage.LockIndexExecutionResult, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(batch storage.ReaderBatchWriter) error { + err := store1.BatchStore(result, batch) + require.NoError(t, err) - err = store1.BatchIndex(blockID, result.ID(), batch) - require.NoError(t, err) - return nil - })) + err = store1.BatchIndex(lctx, batch, blockID, result.ID()) + require.NoError(t, err) + return nil + }) + }) + require.NoError(t, err) - require.NoError(t, db.WithReaderBatchWriter(func(batch storage.ReaderBatchWriter) error { - err := store1.BatchStore(result, batch) - require.NoError(t, err) + err = unittest.WithLock(t, lockManager, storage.LockIndexExecutionResult, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(batch storage.ReaderBatchWriter) error { + err := store1.BatchStore(result, batch) + require.NoError(t, err) - err = store1.BatchIndex(blockID, result.ID(), batch) - require.NoError(t, err) + err = store1.BatchIndex(lctx, batch, blockID, result.ID()) + require.NoError(t, err) - return nil - })) + return nil + }) + }) + require.NoError(t, err) }) } func TestResultStoreTwoDifferentResultsShouldFail(t *testing.T) { + lockManager := storage.NewTestingLockManager() dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { metrics := metrics.NewNoopCollector() store1 := store.NewExecutionResults(metrics, db) @@ -91,48 +122,40 @@ func TestResultStoreTwoDifferentResultsShouldFail(t *testing.T) { result1 := unittest.ExecutionResultFixture() result2 := unittest.ExecutionResultFixture() blockID := unittest.IdentifierFixture() - err := store1.Store(result1) - require.NoError(t, err) - err = store1.Index(blockID, result1.ID()) + err := unittest.WithLock(t, lockManager, storage.LockIndexExecutionResult, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + err := store1.BatchStore(result1, rw) + require.NoError(t, err) + + err = store1.BatchIndex(lctx, rw, blockID, result1.ID()) + require.NoError(t, err) + return nil + }) + }) require.NoError(t, err) - // we can store1 a different result, but we can't index + // we can store a different result, but we can't index // a different result for that block, because it will mean // one block has two different results. - err = store1.Store(result2) + err = unittest.WithLock(t, lockManager, storage.LockIndexExecutionResult, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + err := store1.BatchStore(result2, rw) + require.NoError(t, err) + return nil + }) + }) require.NoError(t, err) - err = store1.Index(blockID, result2.ID()) - require.Error(t, err) - require.True(t, errors.Is(err, storage.ErrDataMismatch)) - }) -} - -func TestResultStoreForceIndexOverridesMapping(t *testing.T) { - dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { - metrics := metrics.NewNoopCollector() - store1 := store.NewExecutionResults(metrics, db) - - result1 := unittest.ExecutionResultFixture() - result2 := unittest.ExecutionResultFixture() - blockID := unittest.IdentifierFixture() - err := store1.Store(result1) - require.NoError(t, err) - err = store1.Index(blockID, result1.ID()) - require.NoError(t, err) - - err = store1.Store(result2) - require.NoError(t, err) - - // force index - err = store1.ForceIndex(blockID, result2.ID()) - require.NoError(t, err) - - // retrieve index to make sure it points to second ER now - byBlockID, err := store1.ByBlockID(blockID) - - require.Equal(t, result2, byBlockID) + var indexErr error + err = unittest.WithLock(t, lockManager, storage.LockIndexExecutionResult, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + indexErr = store1.BatchIndex(lctx, rw, blockID, result2.ID()) + return nil + }) + }) require.NoError(t, err) + require.Error(t, indexErr) + require.True(t, errors.Is(indexErr, storage.ErrDataMismatch)) }) } diff --git a/storage/store/seals_test.go b/storage/store/seals_test.go index 02f79fdece2..852a245b3b0 100644 --- a/storage/store/seals_test.go +++ b/storage/store/seals_test.go @@ -47,9 +47,6 @@ func TestSealStoreRetrieve(t *testing.T) { // TestSealIndexAndRetrieve verifies that: // - for a block, we can s (aka index) the latest sealed block along this fork. -// -// Note: indexing the seal for a block is currently implemented only through a direct -// Badger operation. The Seals mempool only supports retrieving the latest sealed block. func TestSealIndexAndRetrieve(t *testing.T) { dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { lockManager := storage.NewTestingLockManager() diff --git a/storage/store/transaction_results.go b/storage/store/transaction_results.go index a870fe65ba8..78754793503 100644 --- a/storage/store/transaction_results.go +++ b/storage/store/transaction_results.go @@ -4,6 +4,8 @@ import ( "encoding/binary" "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" @@ -128,22 +130,15 @@ func NewTransactionResults(collector module.CacheMetrics, db storage.DB, transac } // BatchStore will store the transaction results for the given block ID in a batch -func (tr *TransactionResults) BatchStore(blockID flow.Identifier, transactionResults []flow.TransactionResult, batch storage.ReaderBatchWriter) error { - w := batch.Writer() - - for i, result := range transactionResults { - err := operation.InsertTransactionResult(w, blockID, &result) - if err != nil { - return fmt.Errorf("cannot batch insert tx result: %w", err) - } - - err = operation.IndexTransactionResult(w, blockID, uint32(i), &result) - if err != nil { - return fmt.Errorf("cannot batch index tx result: %w", err) - } +// It returns [ErrAlreadyExists] if transaction results for the block already exist. +// It requires the caller to hold [storage.LockInsertAndIndexTxResult] +func (tr *TransactionResults) BatchStore(lctx lockctx.Proof, rw storage.ReaderBatchWriter, blockID flow.Identifier, transactionResults []flow.TransactionResult) error { + err := operation.InsertAndIndexTransactionResults(lctx, rw, blockID, transactionResults) + if err != nil { + return fmt.Errorf("cannot batch insert and index tx results: %w", err) } - storage.OnCommitSucceed(batch, func() { + storage.OnCommitSucceed(rw, func() { for i, result := range transactionResults { key := KeyFromBlockIDTransactionID(blockID, result.TransactionID) // cache for each transaction, so that it's faster to retrieve diff --git a/storage/store/transaction_results_test.go b/storage/store/transaction_results_test.go index aa082c7b2b7..5d707e964d3 100644 --- a/storage/store/transaction_results_test.go +++ b/storage/store/transaction_results_test.go @@ -7,6 +7,7 @@ import ( "slices" "testing" + "github.com/jordanschalm/lockctx" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/exp/rand" @@ -21,6 +22,7 @@ import ( ) func TestBatchStoringTransactionResults(t *testing.T) { + lockManager := storage.NewTestingLockManager() dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { metrics := metrics.NewNoopCollector() st, err := store.NewTransactionResults(metrics, db, 1000) @@ -36,13 +38,11 @@ func TestBatchStoringTransactionResults(t *testing.T) { } txResults = append(txResults, expected) } - writeBatch := db.NewBatch() - defer writeBatch.Close() - - err = st.BatchStore(blockID, txResults, writeBatch) - require.NoError(t, err) - - err = writeBatch.Commit() + err = unittest.WithLock(t, lockManager, storage.LockInsertAndIndexTxResult, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return st.BatchStore(lctx, rw, blockID, txResults) + }) + }) require.NoError(t, err) for _, txResult := range txResults { @@ -75,6 +75,7 @@ func TestBatchStoringTransactionResults(t *testing.T) { func TestBatchStoreAndBatchRemoveTransactionResults(t *testing.T) { dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { + lockManager := storage.NewTestingLockManager() const blockCount = 10 const txCountPerBlock = 10 @@ -99,14 +100,16 @@ func TestBatchStoreAndBatchRemoveTransactionResults(t *testing.T) { } // Store transaction results of multiple blocks - err = db.WithReaderBatchWriter(func(rbw storage.ReaderBatchWriter) error { - for _, blockID := range blockIDs { - err := st.BatchStore(blockID, txResults[blockID], rbw) - if err != nil { - return err + err = storage.WithLock(lockManager, storage.LockInsertAndIndexTxResult, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + for _, blockID := range blockIDs { + err := st.BatchStore(lctx, rw, blockID, txResults[blockID]) + if err != nil { + return err + } } - } - return nil + return nil + }) }) require.NoError(t, err) @@ -186,6 +189,127 @@ func TestIndexKeyConversion(t *testing.T) { require.Equal(t, txIndex, tID) } +func TestBatchStoreTransactionResultsErrAlreadyExists(t *testing.T) { + lockManager := storage.NewTestingLockManager() + dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { + metrics := metrics.NewNoopCollector() + st, err := store.NewTransactionResults(metrics, db, 1000) + require.NoError(t, err) + + blockID := unittest.IdentifierFixture() + txResults := make([]flow.TransactionResult, 0) + for i := 0; i < 3; i++ { + txID := unittest.IdentifierFixture() + expected := flow.TransactionResult{ + TransactionID: txID, + ErrorMessage: fmt.Sprintf("a runtime error %d", i), + } + txResults = append(txResults, expected) + } + + // First batch store should succeed + err = unittest.WithLock(t, lockManager, storage.LockInsertAndIndexTxResult, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return st.BatchStore(lctx, rw, blockID, txResults) + }) + }) + require.NoError(t, err) + + // Second batch store with the same blockID should fail with ErrAlreadyExists + duplicateTxResults := make([]flow.TransactionResult, 0) + for i := 0; i < 2; i++ { + txID := unittest.IdentifierFixture() + expected := flow.TransactionResult{ + TransactionID: txID, + ErrorMessage: fmt.Sprintf("duplicate error %d", i), + } + duplicateTxResults = append(duplicateTxResults, expected) + } + + err = unittest.WithLock(t, lockManager, storage.LockInsertAndIndexTxResult, func(lctx lockctx.Context) error { + return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return st.BatchStore(lctx, rw, blockID, duplicateTxResults) + }) + }) + require.Error(t, err) + require.ErrorIs(t, err, storage.ErrAlreadyExists) + + // Verify that the original transaction results are still there and unchanged + for _, txResult := range txResults { + actual, err := st.ByBlockIDTransactionID(blockID, txResult.TransactionID) + require.NoError(t, err) + assert.Equal(t, txResult, *actual) + } + + // Verify that the duplicate transaction results were not stored + for _, txResult := range duplicateTxResults { + _, err := st.ByBlockIDTransactionID(blockID, txResult.TransactionID) + require.Error(t, err) + require.ErrorIs(t, err, storage.ErrNotFound) + } + }) +} + +func TestBatchStoreTransactionResultsMissingLock(t *testing.T) { + dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { + lockManager := storage.NewTestingLockManager() + metrics := metrics.NewNoopCollector() + st, err := store.NewTransactionResults(metrics, db, 1000) + require.NoError(t, err) + + blockID := unittest.IdentifierFixture() + txResults := make([]flow.TransactionResult, 0) + for i := 0; i < 3; i++ { + txID := unittest.IdentifierFixture() + expected := flow.TransactionResult{ + TransactionID: txID, + ErrorMessage: fmt.Sprintf("a runtime error %d", i), + } + txResults = append(txResults, expected) + } + + // Create a context without any locks + lctx := lockManager.NewContext() + defer lctx.Release() + + // Attempt to batch store without holding the required lock + err = db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error { + return st.BatchStore(lctx, rw, blockID, txResults) + }) + require.Error(t, err) + require.Contains(t, err.Error(), "LockInsertAndIndexTxResult") + }) +} + +func TestBatchStoreTransactionResultsWrongLock(t *testing.T) { + dbtest.RunWithDB(t, func(t *testing.T, db storage.DB) { + lockManager := storage.NewTestingLockManager() + metrics := metrics.NewNoopCollector() + st, err := store.NewTransactionResults(metrics, db, 1000) + require.NoError(t, err) + + blockID := unittest.IdentifierFixture() + txResults := make([]flow.TransactionResult, 0) + for i := 0; i < 3; i++ { + txID := unittest.IdentifierFixture() + expected := flow.TransactionResult{ + TransactionID: txID, + ErrorMessage: fmt.Sprintf("a runtime error %d", i), + } + txResults = append(txResults, expected) + } + + // Attempt to batch store with wrong lock (LockInsertBlock instead of LockInsertAndIndexTxResult) + 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, txResults) + }) + }) + require.Error(t, err) + require.Contains(t, err.Error(), "LockInsertAndIndexTxResult") + }) +} + func BenchmarkTransactionResultCacheKey(b *testing.B) { b.Run("new: create cache key", func(b *testing.B) { blockID := unittest.IdentifierFixture() diff --git a/storage/transaction_results.go b/storage/transaction_results.go index 9c7ecdbe1db..b3814a0aac5 100644 --- a/storage/transaction_results.go +++ b/storage/transaction_results.go @@ -1,6 +1,10 @@ package storage -import "github.com/onflow/flow-go/model/flow" +import ( + "github.com/jordanschalm/lockctx" + + "github.com/onflow/flow-go/model/flow" +) type TransactionResultsReader interface { // ByBlockIDTransactionID returns the transaction result for the given block ID and transaction ID @@ -18,7 +22,9 @@ type TransactionResults interface { TransactionResultsReader // BatchStore inserts a batch of transaction result into a batch - BatchStore(blockID flow.Identifier, transactionResults []flow.TransactionResult, batch ReaderBatchWriter) error + // It returns [ErrAlreadyExists] if transaction results for the block already exist. + // It requires the caller to hold [storage.LockInsertAndIndexTxResult] + BatchStore(lctx lockctx.Proof, rw ReaderBatchWriter, blockID flow.Identifier, transactionResults []flow.TransactionResult) error // RemoveByBlockID removes all transaction results for a block BatchRemoveByBlockID(id flow.Identifier, batch ReaderBatchWriter) error