Pebble Lock Manager PoC #7364

jordanschalm · 2025-05-01T20:06:31Z

This PR extends #7262 to add a lock manager for storage operations.

So far, the lock manager has been added to the Mutator. There are still several outstanding TODOs in this PR, where other locking strategies are used, but I think the current diff is sufficient to discuss the lock manager strategy as a whole.

The lock manager is a process-wide singleton that mediates access to a set of locks used to synchronize storage operations. High-level storage methods create a lockctx.Context, then acquire one or more required locks prior to performing any reads or writes. (Acquiring a lock will return an error if the lock order policy is violated.) They then pass a lockctx.Proof to low-level functions which require synchronization which carries information about which locks have been acquired. Low-level functions check the proof, and return an error if it fails.

Benefits

whether a function requires synchronization protection is communicated and enforced in both high- and low-level storage components
- since locks are acquired at the higher level, we don't need to worry about re-entrant deadlocks
- since locks are checked at the lower level, the code is more robust against future changes (a future user or a lock-requiring low-level storage function will get an error unless they acquire the necessary lock)
lock policy prevents deadlock

zhangchiqing · 2025-05-06T16:30:43Z

storage/locks.go

+// This function will panic if a policy is created which does not prevent deadlocks.
+func makeLockPolicy() lockctx.Policy {
+	return lockctx.NewDAGPolicyBuilder().
+		Add(LockInsertBlock, LockFinalizeBlock).


Can you explain a bit more with examples of what is allowed and what not?

My understanding is:
// A process, which has acquired LockInsertBlock, can also acquire LockFinalizeBlock
// A process, which has not acquired LockInsertBlock, can not acquire LockFinalizeBlock (but why?)
// A process, which has acquired LockInsertBlock, can acquire LockInsertBlock again,
// meaning it can continue executing logic protected by the LockInsertBlock
// what else?

// A process, which has acquired LockInsertBlock, can also acquire LockFinalizeBlock

Yes

// A process, which has not acquired LockInsertBlock, can not acquire LockFinalizeBlock (but why?)

Just to be precise:

A process which has acquired some other lock besides LockInsertBlock, can not acquire LockFinalizeBlock

A process which has acquired no locks can acquire LockFinalizeBlock (or any other lock)

The policy exists to prevent deadlock, which can only occur when a process is holding a lock while attempting to acquire another lock. So if you don't hold any lock initially, then you can acquire any one lock, regardless of the DAG defining the lock order.

Now let's consider the first case, where a process has acquired some lock besides LockInsertBlock, and is disallowed from acquiring LockFinalizeBlock. The reason we do this is to prevent deadlock. The DAGPolicy defaults to disallowing all possible sequences of acquiring multiple locks. We use the DAGPolicyBuilder to specify which sequences we want to allow, then Build validates that our specification is acyclic, which guarantees deadlock-free operation.

// A process, which has acquired LockInsertBlock, can acquire LockInsertBlock again,

No, it cannot acquire LockInsertBlock again. It should acquire the lock once, then if a lower level requires validation that a lock has been acquired, it should down in a Proof.

Maybe a useful comment for context is that we only need the edge LockInsertBlock->LockFinalizeBlock because bootstrapping both inserts and finalizes blocks (and hence acquires both locks at the same time). Extend and Finalize both only acquire one lock, so they would work with an empty DAG policy.

Makes sense.

Could you add this to comments? The policy is to allow a process to acquire the insert block lock then optionally to acquire the finalize block lock. And bootstrap is such process that acquire both locks to insert and finalize root block.

zhangchiqing · 2025-05-06T16:35:50Z

storage/operation/headers.go

+// Returns [storage.ErrAlreadyExists] if an ID has already been finalized for this height.
+func IndexBlockHeight(lctx lockctx.Proof, rw storage.ReaderBatchWriter, height uint64, blockID flow.Identifier) error {
+	if !lctx.HoldsLock(storage.LockFinalizeBlock) {
+		return fmt.Errorf("missing required lock: %s", storage.LockFinalizeBlock)


Maybe the HoldsLock method could include the lock name in the error message, so that we just need to wrap data that didn't get passed to the HoldsLock method:

Suggested change

return fmt.Errorf("missing required lock: %s", storage.LockFinalizeBlock)

return fmt.Errorf("missing required lock for block %v (height: %v): %w", blockID, height, err)

👍 I like this idea. Will save a lot of boilerplate.

zhangchiqing · 2025-05-06T16:41:27Z

storage/locks.go

+
+// MakeSingletonLockManager returns the lock manager used by the storage layer.
+// This function must be used for production builds and must be called exactly once process-wide.
+// If this function is called more than once, it will panic. This is strictly enforced because


This is not the typical singleton pattern, which can be called multiple time. Why not allowing it to be called multiple times, and always return the same global singleton?

The reason I did it this way is because our existing structure uses dependency injection and avoids typical singleton getters which can be called multiple times. My goal with this function is to:

continue using the dependency injection pattern (the LockManager dependency should be constructed once then passed down to components which depend on it)

but also, fail loudly if someone misuses this function by invoking it multiple times (because creating multiple instances of this component completely breaks the behaviour)

zhangchiqing

Nice work! I really like the API design of the lock context. Clean and easy to understand and reason about 👏

zhangchiqing · 2025-05-06T20:59:59Z

state/protocol/badger/state.go

@@ -102,6 +105,16 @@ func Bootstrap(
 	root protocol.Snapshot,
 	options ...BootstrapConfigOptions,
 ) (*State, error) {
+	lctx := lockManager.NewContext()
+	defer lctx.Release()
+	err := lctx.AcquireLock(storage.LockInsertBlock)


Let's add some comments to explain why we need to lock both

jordanschalm · 2025-05-07T19:18:39Z

storage/store/index.go

+	// TODO(7355): lockctx - we should be passing a lockctx but it is challenging to implement lockctx here because
+	//             the Cache's store function doesn't allow passing in the context...
 	store := func(rw storage.ReaderBatchWriter, blockID flow.Identifier, index *flow.Index) error {
 		return procedure.InsertIndex(indexing, rw, blockID, index)
 	}


Some of our methods which need to be synchronized use the Cache. But the database methods used by cache don't allow providing the lockctx.Proof argument, making it difficult to use lockctx with the Cache. Not sure how to deal with this yet.

…nto jord/pebble/add-lockctx

AlexHentschel

first batch of comments

AlexHentschel · 2025-05-07T22:00:12Z

storage/locks.go

+}
+
+// NewTestingLockManager returns the lock manager used by the storage layer.
+// This function must be used for testing only and must not be used in production builds.


Suggested change

// This function must be used for testing only and must not be used in production builds.

// This function must be used for testing only but NOT for PRODUCTION builds.

AlexHentschel · 2025-05-08T19:04:47Z

storage/locks.go

+// This file enumerates all named locks used by the storage layer.
+
+const (
+	// LockInsertBlock protects the entire block insertion process (Extend or ExtendCertified)


I think it would be helpful to include the interfaces here (not just the method names):

Suggested change

// LockInsertBlock protects the entire block insertion process (Extend or ExtendCertified)

// LockInsertBlock protects the entire block insertion process (`ParticipantState.Extend` or `FollowerState.ExtendCertified`)

AlexHentschel · 2025-05-08T19:06:15Z

storage/locks.go

+const (
+	// LockInsertBlock protects the entire block insertion process (Extend or ExtendCertified)
+	LockInsertBlock = "lock_insert_block"
+	// LockFinalizeBlock protects the entire block finalization process (Finalize)


Suggested change

// LockFinalizeBlock protects the entire block finalization process (Finalize)

// LockFinalizeBlock protects the entire block finalization process (`FollowerState.Finalize`)

AlexHentschel · 2025-05-09T21:58:20Z

storage/locks.go

+// MakeSingletonLockManager returns the lock manager used by the storage layer.
+// This function must be used for production builds and must be called exactly once process-wide.
+// If this function is called more than once, it will panic. This is strictly enforced because
+// correctness of the lock manager depends on the same set of locks being used everywhere.


I think Leo raises a good question in his comment, which I also had when reviewing your code.

I understand and agree with your explanation - though I feel the proper explanation is not part of the code and therefore this reasoning will be unavailable to many engineers, likely causing this question to resurface repeatedly. I think we should update the documentation.

Regarding your statement:

correctness of the lock manager depends on the same set of locks being used everywhere.

I agree that we need a singleton, but I don't understand why this is a reason for panicking on repeated calls (I think that's exactly the detail where you also lost Leo). We could just as well return always the same instance on repeated calls thereby satisfying the requirement of "the same set of locks being used everywhere" - but without panicking.

Suggested change

// MakeSingletonLockManager returns the lock manager used by the storage layer.

// This function must be used for production builds and must be called exactly once process-wide.

// If this function is called more than once, it will panic. This is strictly enforced because

// correctness of the lock manager depends on the same set of locks being used everywhere.

// MakeSingletonLockManager returns the lock manager used by the storage layer.

// This function must be used for production builds and must be called exactly once process-wide.

//

// The Lock Manager is a core component enforcing atomicity of various storage operations across different

// components. Therefore, the lock manager is a singleton instance, because its correctness depends on the

// same set of locks being used everywhere.

// By convention, the lock mananger singleton is injected into the node's components during their

// initialization, following the same dependency-injection pattern as other components that are conceptually

// singletons (e.g. the storage layer abstractions). Thereby, we explicitly codify in the constructor that a

// component uses the lock mananger. We think it is helpful to emphasize that the component at times

// will acquire _exclusive access_ to all key-value pairs in the database whose keys start with some specific

// prefixes (see `storage/badger/operation/prefix.go` for an exhaustive list of prefixes).

// In comparison, the alternative pattern (which we do not use) of retrieving a singleton instance via a

// global variable would hide which components required exclusive storage access, and in addition, it would

// break with our broadly established dependency-injection pattern. To enforce best practices, this function

// will panic if it is called more than once.

//

// CAUTION:

// - The lock manager only guarantees atomicity of reads and writes for the thread holding the lock.

// Other threads can continue to read possibly stale values, while the lock is held by a different thread.

// - Furthermore, the writer must bundle all their writes into a _single_ Write Batch for atomicity. Even

// when holding the lock, reading threads can still observe the writes of one batch while not observing

// the writes of a second batch, despite the thread writing both batches while holding the lock. It was

// a deliberate choice for the sake of performance to allow reads without any locking - so instead of

// waiting for the newest value in case a write is currently ongoing, the reader will just retrieve the

// previous value. This aligns with our architecture of the node operating as an eventually-consistent

// system, which favors loose coupling and high throughput for different components within a node.

I think we should maybe move the CAUTION part into a readme or so, where we explain the lock manager's use within flow-go in more detail.

created issue #7397 for creating a Readme ... assume that there might be multiple aspects that could be combined into a readme.

AlexHentschel · 2025-05-09T22:36:55Z

storage/operation/approvals.go

 //   - In order to make sure only one approval is indexed for the chunk, _all calls_ to
-//     `UnsafeIndexResultApproval` must be synchronized by the higher-logic. Currently, we have the
-//     convention that `store.ResultApprovals` is the only place that is allowed to call this method.
-func UnsafeIndexResultApproval(w storage.Writer, resultID flow.Identifier, chunkIndex uint64, approvalID flow.Identifier) error {
+//     [IndexResultApproval] must acquire the [storage.LockIndexResultApproval] and check
+//     that no value already exists for the index prior to writing.


Suggested change

// - In order to make sure only one approval is indexed for the chunk, _all calls_ to

// `UnsafeIndexResultApproval` must be synchronized by the higher-logic. Currently, we have the

// convention that `store.ResultApprovals` is the only place that is allowed to call this method.

func UnsafeIndexResultApproval(w storage.Writer, resultID flow.Identifier, chunkIndex uint64, approvalID flow.Identifier) error {

// [IndexResultApproval] must acquire the [storage.LockIndexResultApproval] and check

// that no value already exists for the index prior to writing.

// - A verifier sending _different_ approvals for the _same chunk_ is a slashable protocol violation,

// which we want to prevent in all cases via a sanity check. In order to ensure that at most a

// single approval is indexed for the chunk, _all calls_ to [IndexResultApproval] must acquire

// [storage.LockIndexResultApproval] to atomically check that no conflicting value was already

// indexed prior to writing.

AlexHentschel · 2025-05-09T22:42:54Z

storage/locks.go

+func NewTestingLockManager() lockctx.Manager {
+	return lockctx.NewManager(Locks(), makeLockPolicy())
+}


can we move this function into the testing package, please to further indicate that this is only to be used for testing?

…lockctx

jordanschalm added 2 commits May 1, 2025 13:02

add todos by mutexes in storage/*

2da4e0d

pin lockctx library

14c4c4c

jordanschalm requested a review from a team as a code owner May 1, 2025 20:06

Merge branch 'leo/dbops-follower' into jord/pebble/add-lockctx

ae80b53

jordanschalm mentioned this pull request May 2, 2025

[Storage] Refactor storage for Follower Engine #7262

Open

jordanschalm added 9 commits May 5, 2025 07:47

Merge branch 'leo/dbops-follower' into jord/pebble/add-lockctx

23b7fe7

add lockmgr init to scaffold

b76c6fe

integrate lockctx for headers IndexHeight

1b7db28

todo

dc83b81

add lock manager to bootstrapping

e69a33d

lctx in other mutator writes

f3980be

missing dep

15c389d

differentiate between prod/testing lock manager

55cc555

rm irrelevant todos

2ccaa19

jordanschalm changed the title ~~WIP: Pebble Lock Manager PoC~~ Pebble Lock Manager PoC May 6, 2025

doc tweaks

6f71001

jordanschalm requested review from zhangchiqing and AlexHentschel May 6, 2025 16:00

zhangchiqing reviewed May 6, 2025

View reviewed changes

zhangchiqing approved these changes May 6, 2025

View reviewed changes

jordanschalm added 5 commits May 7, 2025 07:02

policy docs

92adeab

add todos for batch writer Lock uses

36eb0e2

Merge branch 'leo/dbops-follower' into jord/pebble/add-lockctx

5c2a315

document acquisition of both locks for bootstrapping

43beace

lockctx: result approval indexing

6916965

jordanschalm commented May 7, 2025

View reviewed changes

jordanschalm and others added 4 commits May 8, 2025 12:34

update IndexLatestSealAtBlock

23f064b

Merge branch 'leo/dbops-follower' into leo/add-lockctx

56df6bd

add storeWithLock method to storage cache

c432cb3

update gomod

1c4135a

zhangchiqing and others added 8 commits May 8, 2025 13:50

update storage blocks

0eaf0ca

update mocks

ec09288

fix BatchStore in tests

dca69eb

fix scaffold

f40f04d

fix linter

b6e1b69

Merge branch 'jord/pebble/add-lockctx' of github.com:onflow/flow-go i…

48d850b

…nto jord/pebble/add-lockctx

fix header test

5910c42

fix payloads test

2e821f5

AlexHentschel reviewed May 9, 2025

View reviewed changes

AlexHentschel mentioned this pull request May 9, 2025

[Storage] Readme documenting usage and subtleties of Lock Manager's usage within flow-go #7397

Open

1 task

fix deadlock in tests

44ffd0f

zhangchiqing force-pushed the jord/pebble/add-lockctx branch from e1c0831 to 44ffd0f Compare May 16, 2025 17:08

zhangchiqing added 4 commits May 22, 2025 16:50

disable migrate last executed block

da53570

disable chaining

99029e1

Merge branch 'leo/dbops-follower' into jord/pebble/add-lockctx

513b08a

Merge branch 'leo/disable-en-halfway-migration' into jord/pebble/add-…

efd6dba

…lockctx

jordanschalm mentioned this pull request May 29, 2025

[Storage] Use lockctx to store approvals #7406

Open

zhangchiqing added 2 commits May 29, 2025 09:36

Merge branch 'leo/dbops-follower' into jord/pebble/add-lockctx

c4f9871

Merge branch 'leo/dbops-follower' into jord/pebble/add-lockctx

1e4582f

	return fmt.Errorf("missing required lock: %s", storage.LockFinalizeBlock)
	return fmt.Errorf("missing required lock for block %v (height: %v): %w", blockID, height, err)

	// This function must be used for testing only and must not be used in production builds.
	// This function must be used for testing only but NOT for PRODUCTION builds.

	// LockInsertBlock protects the entire block insertion process (Extend or ExtendCertified)
	// LockInsertBlock protects the entire block insertion process (`ParticipantState.Extend` or `FollowerState.ExtendCertified`)

	// LockFinalizeBlock protects the entire block finalization process (Finalize)
	// LockFinalizeBlock protects the entire block finalization process (`FollowerState.Finalize`)

-// MakeSingletonLockManager returns the lock manager used by the storage layer.
-// This function must be used for production builds and must be called exactly once process-wide.
-// If this function is called more than once, it will panic. This is strictly enforced because
-// correctness of the lock manager depends on the same set of locks being used everywhere.
+// MakeSingletonLockManager returns the lock manager used by the storage layer.
+// This function must be used for production builds and must be called exactly once process-wide.
+//
+// The Lock Manager is a core component enforcing atomicity of various storage operations across different
+// components. Therefore, the lock manager is a singleton instance, because its correctness depends on the
+// same set of locks being used everywhere.
+// By convention, the lock mananger singleton is injected into the node's components during their
+// initialization, following the same dependency-injection pattern as other components that are conceptually
+// singletons (e.g. the storage layer abstractions). Thereby, we explicitly codify in the constructor that a
+// component uses the lock mananger. We think it is helpful to emphasize that the component at times
+// will acquire _exclusive access_ to all key-value pairs in the database whose keys start with some specific
+// prefixes (see `storage/badger/operation/prefix.go` for an exhaustive list of prefixes).
+// In comparison, the alternative pattern (which we do not use) of retrieving a singleton instance via a
+// global variable would hide which components required exclusive storage access, and in addition, it would
+// break with our broadly established dependency-injection pattern. To enforce best practices, this function
+// will panic if it is called more than once.
+//
+// CAUTION:
+//   - The lock manager only guarantees atomicity of reads and writes for the thread holding the lock.
+//     Other threads can continue to read possibly stale values, while the lock is held by a different thread.
+//   - Furthermore, the writer must bundle all their writes into a _single_ Write Batch for atomicity. Even
+//     when holding the lock, reading threads can still observe the writes of one batch while not observing
+//     the writes of a second batch, despite the thread writing both batches while holding the lock. It was
+//     a deliberate choice for the sake of performance to allow reads without any locking - so instead of
+//     waiting for the newest value in case a write is currently ongoing, the reader will just retrieve the
+//     previous value. This aligns with our architecture of the node operating as an eventually-consistent
+//     system, which favors loose coupling and high throughput for different components within a node.

Pebble Lock Manager PoC #7364

Are you sure you want to change the base?

Pebble Lock Manager PoC #7364

Uh oh!

Conversation

jordanschalm commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benefits

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jordanschalm May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhangchiqing left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexHentschel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jordanschalm commented May 1, 2025 •

edited

Loading

jordanschalm May 6, 2025 •

edited

Loading