extended, refined, polished documentation (no algorithmic changes)

Author: AlexHentschel

engine/access/access_test.go

@@ -560,8 +560,7 @@ func (suite *Suite) TestGetExecutionResultByBlockID() {
 				if err != nil {
 					return err
 				}
-				// requires LockIndexExecutionResult
-				return all.Results.BatchIndex(lctx, rw, blockID, er.ID())
+				return all.Results.BatchIndex(lctx, rw, blockID, er.ID()) // requires storage.LockIndexExecutionResult
 			})
 		}))
 

engine/access/ingestion2/finalized_block_processor.go

@@ -49,8 +49,7 @@ type FinalizedBlockProcessor struct {
 	lockManager      storage.LockManager
 	db               storage.DB
 
-	blocks storage.Blocks
-
+	blocks           storage.Blocks
 	executionResults storage.ExecutionResults
 
 	collectionSyncer         *CollectionSyncer

engine/execution/state/bootstrap/bootstrap.go

@@ -97,7 +97,6 @@ func (b *Bootstrapper) BootstrapExecutionDatabase(
 	db storage.DB,
 	rootSeal *flow.Seal,
 ) error {
-
 	commit := rootSeal.FinalState
 	return storage.WithLocks(manager, storage.LockGroupExecutionBootstrap, func(lctx lockctx.Context) error {
 		return db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error {

engine/execution/state/state.go

@@ -417,8 +417,9 @@ func (s *state) saveExecutionResults(
 	// dedicated database. However, we have not yet persisted that this execution node is committing to the
 	// result represented by the chunk data packs. Populating the index from chunk ID to chunk data pack ID
 	// in the protocol database (signifying the node's slashable commitment to the respective result) is
-	// done by the functor returned by `Store`. The functor's is invoked as part of the atomic batch update
-	// of the protocol database below.
+	// done by the functor returned by `Store`.
+	// The functor's is invoked as part of the atomic batch update of the protocol database below
+	// and requires the lock [storage.LockIndexChunkDataPackByChunkID].
 	storeChunkDataPacksFunc, err := s.chunkDataPacks.Store(chunks)
 	if err != nil {
 		return fmt.Errorf("can not store chunk data packs for block ID: %v: %w", blockID, err)
@@ -445,25 +446,26 @@ func (s *state) saveExecutionResults(
 		// overwriting of the previously stored mapping.
 		err := s.db.WithReaderBatchWriter(func(batch storage.ReaderBatchWriter) error {
 			// store the ChunkID -> StoredChunkDataPack.ID() mapping
-			// in s.db (protocol database along with other execution data in a single batch)
+			// in protocol database along with other execution data in a single batch
+			// requires [storage.LockIndexChunkDataPackByChunkID]
 			err := storeChunkDataPacksFunc(lctx, batch)
 			if err != nil {
 				return fmt.Errorf("cannot store chunk data packs: %w", err)
 			}
 
-			// require LockInsertEvent
+			// requires [storage.LockInsertEvent]
 			err = s.events.BatchStore(lctx, blockID, []flow.EventsList{result.AllEvents()}, batch)
 			if err != nil {
 				return fmt.Errorf("cannot store events: %w", err)
 			}
 
-			// require LockInsertServiceEvent
+			// requires [storage.LockInsertServiceEvent]
 			err = s.serviceEvents.BatchStore(lctx, blockID, result.AllServiceEvents(), batch)
 			if err != nil {
 				return fmt.Errorf("cannot store service events: %w", err)
 			}
 
-			// require LockInsertAndIndexTxResult
+			// requires [storage.LockInsertAndIndexTxResult]
 			err = s.transactionResults.BatchStore(
 				lctx,
 				batch,
@@ -475,14 +477,14 @@ func (s *state) saveExecutionResults(
 			}
 
 			executionResult := &result.ExecutionReceipt.ExecutionResult
-			// require [storage.LockInsertMyReceipt] lock
 			// saving my receipts will also save the execution result
+			// requires [storage.LockInsertMyReceipt] lock
 			err = s.myReceipts.BatchStoreMyReceipt(lctx, result.ExecutionReceipt, batch)
 			if err != nil {
 				return fmt.Errorf("could not persist execution result: %w", err)
 			}
 
-			// require [storage.LockIndexExecutionResult] lock
+			// requires [storage.LockIndexExecutionResult] lock
 			err = s.results.BatchIndex(lctx, batch, blockID, executionResult.ID())
 			if err != nil {
 				return fmt.Errorf("cannot index execution result: %w", err)
@@ -491,7 +493,7 @@ 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
+			// requires [storage.LockIndexStateCommitment] lock
 			err = s.commits.BatchStore(lctx, blockID, result.CurrentEndState(), batch)
 			if err != nil {
 				return fmt.Errorf("cannot store state commitment: %w", err)

module/builder/consensus/builder_test.go

@@ -254,7 +254,10 @@ func (bs *BuilderSuite) SetupTest() {
 
 	lockManager := storage.NewTestingLockManager()
 
-	// insert finalized height and root height
+	// Insert finalized height and root height:
+	// Currently, we need locks [storage.LockInsertInstanceParams] and [storage.LockFinalizeBlock]. However, the lock policy only permits the locking
+	// order [storage.LockInsertInstanceParams] → [storage.LockIndexExecutionResult] → [storage.LockInsertBlock] → [storage.LockFinalizeBlock]. Hence,
+	// we acquire all 4 locks in that order.
 	db := bs.db
 	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 {

module/executiondatasync/optimistic_sync/persisters/stores/events.go

@@ -32,6 +32,7 @@ func NewEventsStore(
 }
 
 // Persist adds events to the batch.
+// The caller must acquire [storage.LockInsertEvent] and hold it until the write batch is  committed.
 //
 // No error returns are expected during normal operations
 func (e *EventsStore) Persist(lctx lockctx.Proof, batch storage.ReaderBatchWriter) error {

state/cluster/badger/state.go

@@ -61,32 +61,30 @@ func Bootstrap(db storage.DB, lockManager lockctx.Manager, stateRoot *StateRoot)
 			if err != nil {
 				return fmt.Errorf("could not insert genesis block: %w", err)
 			}
-			// insert block height -> ID mapping
+			// insert block height -> ID mapping for genesis block (finalized by protocol convention)
 			err = operation.IndexClusterBlockHeight(lctx, rw, chainID, genesis.Height, genesis.ID())
 			if err != nil {
 				return fmt.Errorf("failed to map genesis block height to block: %w", err)
 			}
-			// insert boundary
+			// insert latest finalized height
 			err = operation.BootstrapClusterFinalizedHeight(lctx, rw, chainID, genesis.Height)
 			if err != nil {
 				return fmt.Errorf("could not insert genesis boundary: %w", err)
 			}
 
+			// Initialize and persist safety and liveness data for cluster consensus
 			safetyData := &hotstuff.SafetyData{
 				LockedOneChainView:      genesis.View,
 				HighestAcknowledgedView: genesis.View,
 			}
-
 			livenessData := &hotstuff.LivenessData{
 				CurrentView: genesis.View + 1, // starting view for hotstuff
 				NewestQC:    rootQC,
 			}
-			// insert safety data
 			err = operation.UpsertSafetyData(lctx, rw, chainID, safetyData)
 			if err != nil {
 				return fmt.Errorf("could not insert safety data: %w", err)
 			}
-			// insert liveness data
 			err = operation.UpsertLivenessData(lctx, rw, chainID, livenessData)
 			if err != nil {
 				return fmt.Errorf("could not insert liveness data: %w", err)

state/protocol/badger/state.go

@@ -88,7 +88,7 @@ func SkipNetworkAddressValidation(conf *BootstrapConfig) {
 	conf.SkipNetworkAddressValidation = true
 }
 
-// Bootstrap initializes a the protocol state from the provided root snapshot and persists it to the database.
+// Bootstrap initializes the protocol state from the provided root snapshot and persists it to the database.
 // No errors expected during normal operation.
 func Bootstrap(
 	metrics module.ComplianceMetrics,
@@ -143,11 +143,17 @@ func Bootstrap(
 
 	var state *State
 
-	// we acquire both [storage.LockInsertBlock] and [storage.LockFinalizeBlock] because
-	// the bootstrapping process inserts and finalizes blocks (all blocks within the
-	// trusted root snapshot are presumed to be finalized)
+	// Overview of ACQUIRED LOCKS:
+	//  * In a nutshell, the instance parameters describe how far back the node's local history reaches in comparison
+	//    to the genesis / spork root block. These values are immutable throughout the lifetime of a node. Hence, the
+	//    lock [storage.LockInsertInstanceParams] should only ever be used here during bootstrapping.
+	//  * We acquire both [storage.LockInsertBlock] and [storage.LockFinalizeBlock] because the bootstrapping process
+	//    inserts and finalizes blocks (all blocks within the trusted root snapshot are presumed to be finalized).
+	//  * Liveness and safety data for Jolteon consensus need to be initialized, requiring [storage.LockInsertSafetyData]
+	//    and [storage.LockInsertLivenessData].
+	//  * The lock [storage.LockIndexExecutionResult] is required for bootstrapping execution. When bootstrapping other
+	//    node roles, the lock is acquired (for algorithmic uniformity) but not used.
 	err = storage.WithLocks(lockManager, storage.LockGroupProtocolStateBootstrap, func(lctx lockctx.Context) error {
-
 		// bootstrap the sealing segment
 		// creating sealed root block with the rootResult
 		// creating finalized root block with lastFinalized
@@ -209,9 +215,12 @@ func Bootstrap(
 // bootstrapProtocolStates bootstraps data structures needed for Dynamic Protocol State.
 // The sealing segment may contain blocks committing to different Protocol State entries,
 // in which case each of these protocol state entries are stored in the database during
-// bootstrapping.
-// For each distinct protocol state entry, we also store the associated EpochSetup and
-// EpochCommit service events.
+// bootstrapping. For each distinct protocol state entry, we also store the associated
+// EpochSetup and EpochCommit service events.
+//
+// Caller must hold [storage.LockInsertBlock] lock.
+//
+// No error returns expected during normal operation.
 func bootstrapProtocolState(
 	lctx lockctx.Proof,
 	rw storage.ReaderBatchWriter,
@@ -280,7 +289,7 @@ func bootstrapProtocolState(
 //     history is covered. The spork root block is persisted as a root proposal without proposer
 //     signature (by convention).
 //
-// It requires [storage.LockIndexExecutionResult] lock
+// Required locks: [storage.LockIndexExecutionResult] and [storage.LockInsertBlock] and [storage.LockFinalizeBlock]
 func bootstrapSealingSegment(
 	lctx lockctx.Proof,
 	db storage.DB,
@@ -298,7 +307,7 @@ func bootstrapSealingSegment(
 			if err != nil {
 				return fmt.Errorf("could not insert execution result: %w", err)
 			}
-			err = operation.IndexTrustedExecutionResult(lctx, rw, result.BlockID, result.ID())
+			err = operation.IndexTrustedExecutionResult(lctx, rw, result.BlockID, result.ID()) // requires [storage.LockIndexExecutionResult]
 			if err != nil {
 				return fmt.Errorf("could not index execution result: %w", err)
 			}
@@ -504,13 +513,19 @@ func bootstrapSealingSegment(
 // bootstrapStatePointers instantiates central pointers used to by the protocol
 // state for keeping track of lifecycle variables:
 //   - Consensus Safety and Liveness Data (only used by consensus participants)
-//   - Root Block's Height (heighest block in sealing segment)
+//   - Root Block's Height (highest block in sealing segment)
 //   - Sealed Root Block Height (block height sealed as of the Root Block)
 //   - Latest Finalized Height (initialized to height of Root Block)
 //   - Latest Sealed Block Height (initialized to block height sealed as of the Root Block)
 //   - Spork root block ID (spork root block in sealing segment)
 //   - initial entry in map:
 //     Finalized Block ID -> ID of latest seal in fork with this block as head
+//
+// Caller must hold locks:
+// [storage.LockInsertSafetyData] and [storage.LockInsertLivenessData] and
+// [storage.LockInsertBlock] and [storage.LockFinalizeBlock]
+//
+// No error returns expected during normal operation.
 func bootstrapStatePointers(lctx lockctx.Proof, rw storage.ReaderBatchWriter, root protocol.Snapshot) error {
 	// sealing segment lists blocks in order of ascending height, so the tail
 	// is the oldest ancestor and head is the newest child in the segment
@@ -716,6 +731,9 @@ func bootstrapEpochForProtocolStateEntry(
 // indexEpochHeights populates the epoch height index from the root snapshot.
 // We index the FirstHeight for every epoch where the transition occurs within the sealing segment of the root snapshot,
 // or for the first epoch of a spork if the snapshot is a spork root snapshot (1 block sealing segment).
+//
+// Caller must hold [storage.LockFinalizeBlock] lock.
+//
 // No errors are expected during normal operation.
 func indexEpochHeights(lctx lockctx.Proof, rw storage.ReaderBatchWriter, segment *flow.SealingSegment) error {
 	// CASE 1: For spork root snapshots, there is exactly one block B and one epoch E.

storage/blocks.go

@@ -16,10 +16,15 @@ import (
 // a certified block (including a QC for the block).
 type Blocks interface {
 
-	// BatchStore stores a valid block in a batch.
-	// Error returns:
-	//   - storage.ErrAlreadyExists if the blockID already exists in the database.
-	//   - generic error in case of unexpected failure from the database layer or encoding failure.
+	// BatchStore adds the provided block to the database write batch and populates all secondary storage indices
+	// (maps from the block ID to some block-related information).
+	//
+	// CAUTION: Under the hood, `BatchStore` performs some prior database reads, which must happen atomically with
+	// the subsequent database write in order to prevent accidental state corruption. Therefore, the caller must
+	// acquire [storage.LockInsertBlock] and hold it until the database write has been committed.
+	//
+	// Expected error returns during normal operations:
+	// - [storage.ErrAlreadyExists] if some block with the same ID has already been stored
 	BatchStore(lctx lockctx.Proof, rw ReaderBatchWriter, proposal *flow.Proposal) error
 
 	// ByID returns the block with the given hash. It is available for all incorporated blocks (validated blocks
@@ -88,16 +93,15 @@ type Blocks interface {
 	ByCollectionID(collID flow.Identifier) (*flow.Block, error)
 
 	// BatchIndexBlockContainingCollectionGuarantees produces mappings from the IDs of [flow.CollectionGuarantee]s to the block ID containing these guarantees.
-	// The caller must acquire a storage.LockIndexBlockByPayloadGuarantees lock.
+	// The caller must acquire [storage.LockIndexBlockByPayloadGuarantees] and hold it until the database write has been committed.
 	//
 	// 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.
 	//
-	// 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.
+	// Expected error returns during normal operations:
+	//   - [storage.ErrAlreadyExists] if any collection guarantee is already indexed
 	BatchIndexBlockContainingCollectionGuarantees(lctx lockctx.Proof, rw ReaderBatchWriter, blockID flow.Identifier, guaranteeIDs []flow.Identifier) error
 }

storage/chunk_data_packs_stored.go

@@ -113,7 +113,7 @@ func (c StoredChunkDataPack) Equals(other StoredChunkDataPack) (equal bool, diff
 	return true, ""
 }
 
-// ID returns the identifier of the chunk data pack, which is derived from its contents.
+// ID returns the identifier of the chunk data pack, which is derived from its contents via a collision-resistant hash.
 // Note, StoredChunkDataPack.ID() is the same as ChunkDataPack.ID()
 func (c StoredChunkDataPack) ID() flow.Identifier {
 	return flow.NewChunkDataPackHeader(c.ChunkID, c.StartState, flow.MakeID(c.Proof), c.CollectionID, c.ExecutionDataRoot).ID()