Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up merkleDB interface and duplicate code #2445

Merged
merged 36 commits into from
Dec 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
c417eec
Clean Up interfaces
dboehm-avalabs Nov 22, 2023
e54d60c
Merge branch 'dev' into CleanUpInterfaces
dboehm-avalabs Dec 7, 2023
0de83eb
rename
dboehm-avalabs Dec 7, 2023
e193cd7
Update db.go
dboehm-avalabs Dec 7, 2023
494de38
cleanup
dboehm-avalabs Dec 12, 2023
84df3c9
Update mock_db.go
dboehm-avalabs Dec 13, 2023
18ffba9
Merge branch 'dev' into CleanUpInterfaces
dboehm-avalabs Dec 13, 2023
8a56516
Update trieview.go
dboehm-avalabs Dec 13, 2023
7334c03
Merge branch 'dev' into CleanUpInterfaces
dboehm-avalabs Dec 14, 2023
127f367
Update db_test.go
dboehm-avalabs Dec 14, 2023
7a1029c
Update db.go
dboehm-avalabs Dec 14, 2023
a455451
rename trieview -> view
dboehm-avalabs Dec 14, 2023
b96ffe8
rename
dboehm-avalabs Dec 14, 2023
60a919d
gofumpt
dboehm-avalabs Dec 14, 2023
d5187f1
Update db.go
dboehm-avalabs Dec 14, 2023
6c5f4dc
Merge remote-tracking branch 'upstream/dev' into CleanUpInterfaces
Dec 19, 2023
22eec2d
newline nits
Dec 19, 2023
6cf97ec
nit if --> switch
Dec 19, 2023
0637ed9
prioritize returning error from getting proof, not validity
Dec 19, 2023
6438c68
Merge branch 'dev' into CleanUpInterfaces
Dec 19, 2023
07eaf12
Merge branch 'dev' into CleanUpInterfaces
dboehm-avalabs Dec 19, 2023
1a6dc5e
Merge branch 'CleanUpInterfaces' of https://github.com/ava-labs/avala…
dboehm-avalabs Dec 19, 2023
6428ae1
comments
dboehm-avalabs Dec 19, 2023
ad6bcf0
Update db.go
dboehm-avalabs Dec 19, 2023
95c7eca
Update trie_test.go
dboehm-avalabs Dec 19, 2023
a5c23d8
comments
dboehm-avalabs Dec 19, 2023
4cdb1aa
remove references to trieView
Dec 22, 2023
dc31fe1
fix test name
Dec 27, 2023
84c35a0
Merge branch 'dev' into CleanUpInterfaces
Dec 27, 2023
5cba51d
re-add database closed checks in GetProof
Dec 27, 2023
01bc75c
Merge branch 'CleanUpInterfaces' of github.com:ava-labs/avalanchego i…
Dec 27, 2023
ff774ad
nit
Dec 27, 2023
82019a0
nits
Dec 27, 2023
dc36c65
comments
Dec 27, 2023
2b5982e
comment
Dec 27, 2023
837a1b7
Merge branch 'dev' into CleanUpInterfaces
Dec 28, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions x/merkledb/README.md
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
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)
dboehm-avalabs marked this conversation as resolved.
Show resolved Hide resolved
}

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
Loading