Skip to content

Commit

Permalink
Clean up merkleDB interface and duplicate code (#2445)
Browse files Browse the repository at this point in the history 
Co-authored-by: Dan Laine <[email protected]>
  • Loading branch information
@dboehm-avalabs
dboehm-avalabs and Dan Laine authored Dec 28, 2023
1 parent aa509e7 commit 0c7ff5a
Show file tree
Hide file tree
Showing 10 changed files with 853 additions and 802 deletions.
12 changes: 6 additions & 6 deletions x/merkledb/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -427,18 +427,18 @@ Not using extension nodes results in worse storage efficiency (some nodes may ha
`merkleDB` has a `RWMutex` named `lock`. Its read operations don't store data in a map, so a read lock suffices for read operations.
`merkleDB` has a `Mutex` named `commitLock`. It enforces that only a single view/batch is attempting to commit to the database at one time. `lock` is insufficient because there is a period of view preparation where read access should still be allowed, followed by a period where a full write lock is needed. The `commitLock` ensures that only a single goroutine makes the transition from read => write.

A `trieView` is built atop another trie, which may be the underlying `merkleDB` or another `trieView`.
A `view` is built atop another trie, which may be the underlying `merkleDB` or another `view`.
We use locking to guarantee atomicity/consistency of trie operations.

`trieView` has a `RWMutex` named `commitLock` which ensures that we don't create a view atop the `trieView` while it's being committed.
It also has a `RWMutex` named `validityTrackingLock` that is held during methods that change the view's validity, tracking of child views' validity, or of the `trieView` parent trie. This lock ensures that writing/reading from `trieView` or any of its descendants is safe.
The `CommitToDB` method grabs the `merkleDB`'s `commitLock`. This is the only `trieView` method that modifies the underlying `merkleDB`.
`view` has a `RWMutex` named `commitLock` which ensures that we don't create a view atop the `view` while it's being committed.
It also has a `RWMutex` named `validityTrackingLock` that is held during methods that change the view's validity, tracking of child views' validity, or of the `view` parent trie. This lock ensures that writing/reading from `view` or any of its descendants is safe.
The `CommitToDB` method grabs the `merkleDB`'s `commitLock`. This is the only `view` method that modifies the underlying `merkleDB`.

In some of `merkleDB`'s methods, we create a `trieView` and call unexported methods on it without locking it.
In some of `merkleDB`'s methods, we create a `view` and call unexported methods on it without locking it.
We do so because the exported counterpart of the method read locks the `merkleDB`, which is already locked.
This pattern is safe because the `merkleDB` is locked, so no data under the view is changing, and nobody else has a reference to the view, so there can't be any concurrent access.

To prevent deadlocks, `trieView` and `merkleDB` never acquire the `commitLock` of descendant views.
To prevent deadlocks, `view` and `merkleDB` never acquire the `commitLock` of descendant views.
That is, locking is always done from a view toward to the underlying `merkleDB`, never the other way around.
The `validityTrackingLock` goes the opposite way. A view can lock the `validityTrackingLock` of its children, but not its ancestors. Because of this, any function that takes the `validityTrackingLock` must not take the `commitLock` as this may cause a deadlock. Keeping `commitLock` solely in the ancestor direction and `validityTrackingLock` solely in the descendant direction prevents deadlocks from occurring.

Expand Down
125 changes: 56 additions & 69 deletions x/merkledb/db.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ type Config struct {
Tracer trace.Tracer
}

// merkleDB can only be edited by committing changes from a trieView.
// merkleDB can only be edited by committing changes from a view.
type merkleDB struct {
// Must be held when reading/writing fields.
lock sync.RWMutex
Expand Down Expand Up @@ -216,7 +216,7 @@ type merkleDB struct {
rootID ids.ID

// Valid children of this trie.
childViews []*trieView
childViews []*view

// calculateNodeIDsSema controls the number of goroutines inside
// [calculateNodeIDsHelper] at any given time.
Expand Down Expand Up @@ -264,7 +264,7 @@ func newDatabase(
history: newTrieHistory(int(config.HistoryLength)),
debugTracer: getTracerIfEnabled(config.TraceLevel, DebugTrace, config.Tracer),
infoTracer: getTracerIfEnabled(config.TraceLevel, InfoTrace, config.Tracer),
childViews: make([]*trieView, 0, defaultPreallocationSize),
childViews: make([]*view, 0, defaultPreallocationSize),
calculateNodeIDsSema: semaphore.NewWeighted(int64(rootGenConcurrency)),
tokenSize: BranchFactorToTokenSize[config.BranchFactor],
}
Expand Down Expand Up @@ -324,7 +324,7 @@ func (db *merkleDB) rebuild(ctx context.Context, cacheSize int) error {
defer valueIt.Release()
for valueIt.Next() {
if len(currentOps) >= opsSizeLimit {
view, err := newTrieView(db, db, ViewChanges{BatchOps: currentOps, ConsumeBytes: true})
view, err := newView(db, db, ViewChanges{BatchOps: currentOps, ConsumeBytes: true})
if err != nil {
return err
}
Expand All @@ -347,7 +347,7 @@ func (db *merkleDB) rebuild(ctx context.Context, cacheSize int) error {
if err := valueIt.Error(); err != nil {
return err
}
view, err := newTrieView(db, db, ViewChanges{BatchOps: currentOps, ConsumeBytes: true})
view, err := newView(db, db, ViewChanges{BatchOps: currentOps, ConsumeBytes: true})
if err != nil {
return err
}
Expand All @@ -373,7 +373,7 @@ func (db *merkleDB) CommitChangeProof(ctx context.Context, proof *ChangeProof) e
}
}

view, err := newTrieView(db, db, ViewChanges{BatchOps: ops})
view, err := newView(db, db, ViewChanges{BatchOps: ops})
if err != nil {
return err
}
Expand Down Expand Up @@ -414,7 +414,7 @@ func (db *merkleDB) CommitRangeProof(ctx context.Context, start, end maybe.Maybe
}

// Don't need to lock [view] because nobody else has a reference to it.
view, err := newTrieView(db, db, ViewChanges{BatchOps: ops})
view, err := newView(db, db, ViewChanges{BatchOps: ops})
if err != nil {
return err
}
Expand Down Expand Up @@ -462,13 +462,8 @@ func (db *merkleDB) PrefetchPaths(keys [][]byte) error {
return database.ErrClosed
}

// reuse the view so that it can keep repeated nodes in memory
tempView, err := newTrieView(db, db, ViewChanges{})
if err != nil {
return err
}
for _, key := range keys {
if err := db.prefetchPath(tempView, key); err != nil {
if err := db.prefetchPath(key); err != nil {
return err
}
}
Expand All @@ -483,16 +478,11 @@ func (db *merkleDB) PrefetchPath(key []byte) error {
if db.closed {
return database.ErrClosed
}
tempView, err := newTrieView(db, db, ViewChanges{})
if err != nil {
return err
}

return db.prefetchPath(tempView, key)
return db.prefetchPath(key)
}

func (db *merkleDB) prefetchPath(view *trieView, keyBytes []byte) error {
return view.visitPathToKey(ToKey(keyBytes), func(n *node) error {
func (db *merkleDB) prefetchPath(keyBytes []byte) error {
return visitPathToKey(db, ToKey(keyBytes), func(n *node) error {
if !n.hasValue() {
// this value is already in the cache, so skip writing
// to avoid grabbing the cache write lock
Expand Down Expand Up @@ -599,7 +589,7 @@ func (db *merkleDB) GetMerkleRoot(ctx context.Context) (ids.ID, error) {
return db.getMerkleRoot(), nil
}

// Assumes [db.lock] is read locked.
// Assumes [db.lock] or [db.commitLock] is read locked.
func (db *merkleDB) getMerkleRoot() ids.ID {
return db.rootID
}
Expand All @@ -608,21 +598,14 @@ func (db *merkleDB) GetProof(ctx context.Context, key []byte) (*Proof, error) {
db.commitLock.RLock()
defer db.commitLock.RUnlock()

return db.getProof(ctx, key)
}
_, span := db.infoTracer.Start(ctx, "MerkleDB.GetProof")
defer span.End()

// Assumes [db.commitLock] is read locked.
func (db *merkleDB) getProof(ctx context.Context, key []byte) (*Proof, error) {
if db.closed {
return nil, database.ErrClosed
}

view, err := newTrieView(db, db, ViewChanges{})
if err != nil {
return nil, err
}
// Don't need to lock [view] because nobody else has a reference to it.
return view.getProof(ctx, key)
return getProof(db, key)
}

func (db *merkleDB) GetRangeProof(
Expand All @@ -634,7 +617,14 @@ func (db *merkleDB) GetRangeProof(
db.commitLock.RLock()
defer db.commitLock.RUnlock()

return db.getRangeProofAtRoot(ctx, db.getMerkleRoot(), start, end, maxLength)
_, span := db.infoTracer.Start(ctx, "MerkleDB.GetRangeProof")
defer span.End()

if db.closed {
return nil, database.ErrClosed
}

return getRangeProof(db, start, end, maxLength)
}

func (db *merkleDB) GetRangeProofAtRoot(
Expand All @@ -647,18 +637,9 @@ func (db *merkleDB) GetRangeProofAtRoot(
db.commitLock.RLock()
defer db.commitLock.RUnlock()

return db.getRangeProofAtRoot(ctx, rootID, start, end, maxLength)
}
_, span := db.infoTracer.Start(ctx, "MerkleDB.GetRangeProofAtRoot")
defer span.End()

// Assumes [db.commitLock] is read locked.
// Assumes [db.lock] is not held
func (db *merkleDB) getRangeProofAtRoot(
ctx context.Context,
rootID ids.ID,
start maybe.Maybe[[]byte],
end maybe.Maybe[[]byte],
maxLength int,
) (*RangeProof, error) {
switch {
case db.closed:
return nil, database.ErrClosed
Expand All @@ -668,11 +649,11 @@ func (db *merkleDB) getRangeProofAtRoot(
return nil, ErrEmptyProof
}

historicalView, err := db.getHistoricalViewForRange(rootID, start, end)
historicalTrie, err := db.getTrieAtRootForRange(rootID, start, end)
if err != nil {
return nil, err
}
return historicalView.GetRangeProof(ctx, start, end, maxLength)
return getRangeProof(historicalTrie, start, end, maxLength)
}

func (db *merkleDB) GetChangeProof(
Expand All @@ -683,6 +664,9 @@ func (db *merkleDB) GetChangeProof(
end maybe.Maybe[[]byte],
maxLength int,
) (*ChangeProof, error) {
_, span := db.infoTracer.Start(ctx, "MerkleDB.GetChangeProof")
defer span.End()

switch {
case start.HasValue() && end.HasValue() && bytes.Compare(start.Value(), end.Value()) == 1:
return nil, ErrStartAfterEnd
Expand Down Expand Up @@ -731,21 +715,21 @@ func (db *merkleDB) GetChangeProof(

// Since we hold [db.commitlock] we must still have sufficient
// history to recreate the trie at [endRootID].
historicalView, err := db.getHistoricalViewForRange(endRootID, start, largestKey)
historicalTrie, err := db.getTrieAtRootForRange(endRootID, start, largestKey)
if err != nil {
return nil, err
}

if largestKey.HasValue() {
endProof, err := historicalView.getProof(ctx, largestKey.Value())
endProof, err := getProof(historicalTrie, largestKey.Value())
if err != nil {
return nil, err
}
result.EndProof = endProof.Path
}

if start.HasValue() {
startProof, err := historicalView.getProof(ctx, start.Value())
startProof, err := getProof(historicalTrie, start.Value())
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -782,7 +766,7 @@ func (db *merkleDB) GetChangeProof(
func (db *merkleDB) NewView(
_ context.Context,
changes ViewChanges,
) (TrieView, error) {
) (View, error) {
// ensure the db doesn't change while creating the new view
db.commitLock.RLock()
defer db.commitLock.RUnlock()
Expand All @@ -791,7 +775,7 @@ func (db *merkleDB) NewView(
return nil, database.ErrClosed
}

newView, err := newTrieView(db, db, changes)
newView, err := newView(db, db, changes)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -864,7 +848,7 @@ func (db *merkleDB) PutContext(ctx context.Context, k, v []byte) error {
return database.ErrClosed
}

view, err := newTrieView(db, db, ViewChanges{BatchOps: []database.BatchOp{{Key: k, Value: v}}})
view, err := newView(db, db, ViewChanges{BatchOps: []database.BatchOp{{Key: k, Value: v}}})
if err != nil {
return err
}
Expand All @@ -883,7 +867,7 @@ func (db *merkleDB) DeleteContext(ctx context.Context, key []byte) error {
return database.ErrClosed
}

view, err := newTrieView(db, db,
view, err := newView(db, db,
ViewChanges{
BatchOps: []database.BatchOp{{
Key: key,
Expand All @@ -907,7 +891,7 @@ func (db *merkleDB) commitBatch(ops []database.BatchOp) error {
return database.ErrClosed
}

view, err := newTrieView(db, db, ViewChanges{BatchOps: ops, ConsumeBytes: true})
view, err := newView(db, db, ViewChanges{BatchOps: ops, ConsumeBytes: true})
if err != nil {
return err
}
Expand All @@ -916,7 +900,8 @@ func (db *merkleDB) commitBatch(ops []database.BatchOp) error {

// commitChanges commits the changes in [trieToCommit] to [db].
// Assumes [trieToCommit]'s node IDs have been calculated.
func (db *merkleDB) commitChanges(ctx context.Context, trieToCommit *trieView) error {
// Assumes [db.commitLock] is held.
func (db *merkleDB) commitChanges(ctx context.Context, trieToCommit *view) error {
db.lock.Lock()
defer db.lock.Unlock()

Expand Down Expand Up @@ -1002,19 +987,19 @@ func (db *merkleDB) commitChanges(ctx context.Context, trieToCommit *trieView) e

// moveChildViewsToDB removes any child views from the trieToCommit and moves them to the db
// assumes [db.lock] is held
func (db *merkleDB) moveChildViewsToDB(trieToCommit *trieView) {
func (db *merkleDB) moveChildViewsToDB(trieToCommit *view) {
trieToCommit.validityTrackingLock.Lock()
defer trieToCommit.validityTrackingLock.Unlock()

for _, childView := range trieToCommit.childViews {
childView.updateParent(db)
db.childViews = append(db.childViews, childView)
}
trieToCommit.childViews = make([]*trieView, 0, defaultPreallocationSize)
trieToCommit.childViews = make([]*view, 0, defaultPreallocationSize)
}

// CommitToDB is a no-op for db since it is already in sync with itself.
// This exists to satisfy the TrieView interface.
// This exists to satisfy the View interface.
func (*merkleDB) CommitToDB(context.Context) error {
return nil
}
Expand Down Expand Up @@ -1119,7 +1104,7 @@ func (db *merkleDB) VerifyChangeProof(
}

// Don't need to lock [view] because nobody else has a reference to it.
view, err := newTrieView(db, db, ViewChanges{BatchOps: ops, ConsumeBytes: true})
view, err := newView(db, db, ViewChanges{BatchOps: ops, ConsumeBytes: true})
if err != nil {
return err
}
Expand Down Expand Up @@ -1159,7 +1144,7 @@ func (db *merkleDB) VerifyChangeProof(

// Invalidates and removes any child views that aren't [exception].
// Assumes [db.lock] is held.
func (db *merkleDB) invalidateChildrenExcept(exception *trieView) {
func (db *merkleDB) invalidateChildrenExcept(exception *view) {
isTrackedView := false

for _, childView := range db.childViews {
Expand All @@ -1169,7 +1154,7 @@ func (db *merkleDB) invalidateChildrenExcept(exception *trieView) {
isTrackedView = true
}
}
db.childViews = make([]*trieView, 0, defaultPreallocationSize)
db.childViews = make([]*view, 0, defaultPreallocationSize)
if isTrackedView {
db.childViews = append(db.childViews, exception)
}
Expand Down Expand Up @@ -1217,24 +1202,21 @@ func (db *merkleDB) initializeRoot() error {
// If [start] is Nothing, there's no lower bound on the range.
// If [end] is Nothing, there's no upper bound on the range.
// Assumes [db.commitLock] is read locked.
func (db *merkleDB) getHistoricalViewForRange(
func (db *merkleDB) getTrieAtRootForRange(
rootID ids.ID,
start maybe.Maybe[[]byte],
end maybe.Maybe[[]byte],
) (*trieView, error) {
currentRootID := db.getMerkleRoot()

) (Trie, error) {
// looking for the trie's current root id, so return the trie unmodified
if currentRootID == rootID {
// create an empty trie
return newTrieView(db, db, ViewChanges{})
if rootID == db.getMerkleRoot() {
return db, nil
}

changeHistory, err := db.history.getChangesToGetToRoot(rootID, start, end)
if err != nil {
return nil, err
}
return newHistoricalTrieView(db, changeHistory)
return newViewWithChanges(db, changeHistory)
}

// Returns all keys in range [start, end] that aren't in [keySet].
Expand Down Expand Up @@ -1294,6 +1276,7 @@ func (db *merkleDB) getNode(key Key, hasValue bool) (*node, error) {
}
}

// Assumes [db.lock] or [db.commitLock] is read locked.
func (db *merkleDB) getRoot() maybe.Maybe[*node] {
return db.root
}
Expand Down Expand Up @@ -1327,6 +1310,10 @@ func (db *merkleDB) Clear() error {
return nil
}

func (db *merkleDB) getTokenSize() int {
return db.tokenSize
}

// Returns [key] prefixed by [prefix].
// The returned []byte is taken from [bufferPool] and
// should be returned to it when the caller is done with it.
Expand Down
Loading

0 comments on commit 0c7ff5a

Please sign in to comment.