From c94ff4e3a4d881ce6277d3422be8220bb7751746 Mon Sep 17 00:00:00 2001 From: David Boehm <91908103+dboehm-avalabs@users.noreply.github.com> Date: Wed, 8 Nov 2023 17:55:02 -0500 Subject: [PATCH] MerkleDB:Naming and comments cleanup (#2274) Co-authored-by: Dan Laine --- x/merkledb/cache.go | 2 +- x/merkledb/codec.go | 6 +-- x/merkledb/db.go | 18 ++++---- x/merkledb/history.go | 2 +- x/merkledb/key.go | 6 +-- x/merkledb/proof.go | 90 ++++++++++++++++++++-------------------- x/merkledb/trieview.go | 10 ++--- x/sync/network_server.go | 1 - 8 files changed, 67 insertions(+), 68 deletions(-) diff --git a/x/merkledb/cache.go b/x/merkledb/cache.go index 57d674ed63ef..7b280c1208d4 100644 --- a/x/merkledb/cache.go +++ b/x/merkledb/cache.go @@ -48,7 +48,7 @@ func (c *onEvictCache[K, V]) Get(key K) (V, bool) { // Put an element into this cache. If this causes an element // to be evicted, calls [c.onEviction] on the evicted element -// and returns the error from [c.onEviction]. Otherwise returns nil. +// and returns the error from [c.onEviction]. Otherwise, returns nil. func (c *onEvictCache[K, V]) Put(key K, value V) error { c.lock.Lock() defer c.lock.Unlock() diff --git a/x/merkledb/codec.go b/x/merkledb/codec.go index a7decc6f6436..c9837abb509f 100644 --- a/x/merkledb/codec.go +++ b/x/merkledb/codec.go @@ -83,7 +83,7 @@ func newCodec() encoderDecoder { } } -// Note that bytes.Buffer.Write always returns nil so we +// Note that bytes.Buffer.Write always returns nil, so we // can ignore its return values in [codecImpl] methods. type codecImpl struct { // Invariant: Every byte slice returned by [varIntPool] has @@ -277,12 +277,12 @@ func (c *codecImpl) decodeMaybeByteSlice(src *bytes.Reader) (maybe.Maybe[[]byte] return maybe.Nothing[[]byte](), err } - bytes, err := c.decodeByteSlice(src) + rawBytes, err := c.decodeByteSlice(src) if err != nil { return maybe.Nothing[[]byte](), err } - return maybe.Some(bytes), nil + return maybe.Some(rawBytes), nil } func (c *codecImpl) decodeByteSlice(src *bytes.Reader) ([]byte, error) { diff --git a/x/merkledb/db.go b/x/merkledb/db.go index 88dd667ae22a..7e52e1fa9ecf 100644 --- a/x/merkledb/db.go +++ b/x/merkledb/db.go @@ -73,7 +73,7 @@ type ChangeProofer interface { maxLength int, ) (*ChangeProof, error) - // Returns nil iff all of the following hold: + // Returns nil iff all the following hold: // - [start] <= [end]. // - [proof] is non-empty. // - All keys in [proof.KeyValues] and [proof.DeletedKeys] are in [start, end]. @@ -175,7 +175,7 @@ type merkleDB struct { // Should be held before taking [db.lock] commitLock sync.RWMutex - // Contains all of the key-value pairs stored by this database, + // Contains all the key-value pairs stored by this database, // including metadata, intermediate nodes and value nodes. baseDB database.Database @@ -495,11 +495,11 @@ func (db *merkleDB) GetValues(ctx context.Context, keys [][]byte) ([][]byte, []e defer db.lock.RUnlock() values := make([][]byte, len(keys)) - errors := make([]error, len(keys)) + getErrors := make([]error, len(keys)) for i, key := range keys { - values[i], errors[i] = db.getValueCopy(ToKey(key)) + values[i], getErrors[i] = db.getValueCopy(ToKey(key)) } - return values, errors + return values, getErrors } // GetValue returns the value associated with [key]. @@ -778,7 +778,7 @@ func (db *merkleDB) Has(k []byte) (bool, error) { } _, err := db.getValueWithoutLock(ToKey(k)) - if err == database.ErrNotFound { + if errors.Is(err, database.ErrNotFound) { return false, nil } return err == nil, err @@ -862,7 +862,7 @@ func (db *merkleDB) DeleteContext(ctx context.Context, key []byte) error { return view.commitToDB(ctx) } -// Assumes values inside of [ops] are safe to reference after the function +// Assumes values inside [ops] are safe to reference after the function // returns. Assumes [db.lock] isn't held. func (db *merkleDB) commitBatch(ops []database.BatchOp) error { db.commitLock.Lock() @@ -1144,7 +1144,7 @@ func (db *merkleDB) initializeRootIfNeeded() (ids.ID, error) { // check under both prefixes var err error db.root, err = db.intermediateNodeDB.Get(Key{}) - if err == database.ErrNotFound { + if errors.Is(err, database.ErrNotFound) { db.root, err = db.valueNodeDB.Get(Key{}) } if err == nil { @@ -1152,7 +1152,7 @@ func (db *merkleDB) initializeRootIfNeeded() (ids.ID, error) { db.root.calculateID(db.metrics) return db.root.id, nil } - if err != database.ErrNotFound { + if !errors.Is(err, database.ErrNotFound) { return ids.Empty, err } diff --git a/x/merkledb/history.go b/x/merkledb/history.go index 103c4c9357e8..c52385445cd2 100644 --- a/x/merkledb/history.go +++ b/x/merkledb/history.go @@ -49,7 +49,7 @@ type changeSummaryAndInsertNumber struct { insertNumber uint64 } -// Tracks all of the node and value changes that resulted in the rootID. +// Tracks all the node and value changes that resulted in the rootID. type changeSummary struct { rootID ids.ID nodes map[Key]*change[*node] diff --git a/x/merkledb/key.go b/x/merkledb/key.go index b92ac2d7ceec..d65d9b74a0a6 100644 --- a/x/merkledb/key.go +++ b/x/merkledb/key.go @@ -68,7 +68,7 @@ func ToToken(val byte, tokenSize int) Key { } // Token returns the token at the specified index, -// Assumes that bitindex + tokenSize doesn't cross a byte boundary +// Assumes that bitIndex + tokenSize doesn't cross a byte boundary func (k Key) Token(bitIndex int, tokenSize int) byte { storageByte := k.value[bitIndex/8] // Shift the byte right to get the last bit to the rightmost position. @@ -145,7 +145,7 @@ func (k Key) HasPrefix(prefix Key) bool { } // Note that this will never be an index OOB because len(prefix.value) > 0. - // If len(prefix.value) == 0 were true, [remainderTokens] would be 0 so we + // If len(prefix.value) == 0 were true, [remainderTokens] would be 0, so we // would have returned above. prefixWithoutPartialByte := prefix.value[:len(prefix.value)-1] return strings.HasPrefix(k.value, prefixWithoutPartialByte) @@ -167,7 +167,7 @@ func (k Key) Greater(other Key) bool { return k.value > other.value || (k.value == other.value && k.length > other.length) } -// Less returns true if current Key is less than other Key +// Less will return true if current Key is less than other Key func (k Key) Less(other Key) bool { return k.value < other.value || (k.value == other.value && k.length < other.length) } diff --git a/x/merkledb/proof.go b/x/merkledb/proof.go index c94a68f7db5b..e348a83f0f13 100644 --- a/x/merkledb/proof.go +++ b/x/merkledb/proof.go @@ -50,7 +50,6 @@ var ( ErrNilProof = errors.New("proof is nil") ErrNilValue = errors.New("value is nil") ErrUnexpectedEndProof = errors.New("end proof should be empty") - ErrInconsistentBranchFactor = errors.New("all keys in proof nodes should have the same branch factor") ) type ProofNode struct { @@ -62,6 +61,7 @@ type ProofNode struct { Children map[byte]ids.ID } +// ToProto converts the ProofNode into the protobuf version of a proof node // Assumes [node.Key.Key.length] <= math.MaxUint64. func (node *ProofNode) ToProto() *pb.ProofNode { pbNode := &pb.ProofNode{ @@ -127,11 +127,11 @@ type Proof struct { Key Key // Nothing if [Key] isn't in the trie. - // Otherwise the value corresponding to [Key]. + // Otherwise, the value corresponding to [Key]. Value maybe.Maybe[[]byte] } -// Returns nil if the trie given in [proof] has root [expectedRootID]. +// Verify returns nil if the trie given in [proof] has root [expectedRootID]. // That is, this is a valid proof that [proof.Key] exists/doesn't exist // in the trie with root [expectedRootID]. func (proof *Proof) Verify(ctx context.Context, expectedRootID ids.ID, tokenSize int) error { @@ -172,11 +172,11 @@ func (proof *Proof) Verify(ctx context.Context, expectedRootID ids.ID, tokenSize } // Insert all proof nodes. - // [provenPath] is the path that we are proving exists, or the path - // that is where the path we are proving doesn't exist should be. - provenPath := maybe.Some(proof.Path[len(proof.Path)-1].Key) + // [provenKey] is the key that we are proving exists, or the key + // that is the next key along the node path, proving that [proof.Key] doesn't exist in the trie. + provenKey := maybe.Some(proof.Path[len(proof.Path)-1].Key) - if err = addPathInfo(view, proof.Path, provenPath, provenPath); err != nil { + if err = addPathInfo(view, proof.Path, provenKey, provenKey); err != nil { return err } @@ -240,7 +240,7 @@ type KeyValue struct { Value []byte } -// A proof that a given set of key-value pairs are in a trie. +// RangeProof is a proof that a given set of key-value pairs are in a trie. type RangeProof struct { // Invariant: At least one of [StartProof], [EndProof], [KeyValues] is non-empty. @@ -302,21 +302,21 @@ func (proof *RangeProof) Verify( } // [proof] allegedly provides and proves all key-value - // pairs in [smallestProvenPath, largestProvenPath]. - // If [smallestProvenPath] is Nothing, [proof] should - // provide and prove all keys < [largestProvenPath]. - // If [largestProvenPath] is Nothing, [proof] should - // provide and prove all keys > [smallestProvenPath]. + // pairs in [smallestProvenKey, largestProvenKey]. + // If [smallestProvenKey] is Nothing, [proof] should + // provide and prove all keys < [largestProvenKey]. + // If [largestProvenKey] is Nothing, [proof] should + // provide and prove all keys > [smallestProvenKey]. // If both are Nothing, [proof] should prove the entire trie. - smallestProvenPath := maybe.Bind(start, ToKey) + smallestProvenKey := maybe.Bind(start, ToKey) - largestProvenPath := maybe.Bind(end, ToKey) + largestProvenKey := maybe.Bind(end, ToKey) if len(proof.KeyValues) > 0 { // If [proof] has key-value pairs, we should insert children - // greater than [largestProvenPath] to ancestors of the node containing - // [largestProvenPath] so that we get the expected root ID. - largestProvenPath = maybe.Some(ToKey(proof.KeyValues[len(proof.KeyValues)-1].Key)) + // greater than [largestProvenKey] to ancestors of the node containing + // [largestProvenKey] so that we get the expected root ID. + largestProvenKey = maybe.Some(ToKey(proof.KeyValues[len(proof.KeyValues)-1].Key)) } // The key-value pairs (allegedly) proven by [proof]. @@ -327,13 +327,13 @@ func (proof *RangeProof) Verify( // Ensure that the start proof is valid and contains values that // match the key/values that were sent. - if err := verifyProofPath(proof.StartProof, smallestProvenPath); err != nil { + if err := verifyProofPath(proof.StartProof, smallestProvenKey); err != nil { return err } if err := verifyAllRangeProofKeyValuesPresent( proof.StartProof, - smallestProvenPath, - largestProvenPath, + smallestProvenKey, + largestProvenKey, keyValues, ); err != nil { return err @@ -341,13 +341,13 @@ func (proof *RangeProof) Verify( // Ensure that the end proof is valid and contains values that // match the key/values that were sent. - if err := verifyProofPath(proof.EndProof, largestProvenPath); err != nil { + if err := verifyProofPath(proof.EndProof, largestProvenKey); err != nil { return err } if err := verifyAllRangeProofKeyValuesPresent( proof.EndProof, - smallestProvenPath, - largestProvenPath, + smallestProvenKey, + largestProvenKey, keyValues, ); err != nil { return err @@ -369,24 +369,24 @@ func (proof *RangeProof) Verify( } // For all the nodes along the edges of the proof, insert children - // < [smallestProvenPath] and > [largestProvenPath] + // < [smallestProvenKey] and > [largestProvenKey] // into the trie so that we get the expected root ID (if this proof is valid). - // By inserting all children < [smallestProvenPath], we prove that there are no keys - // > [smallestProvenPath] but less than the first key given. + // By inserting all children < [smallestProvenKey], we prove that there are no keys + // > [smallestProvenKey] but less than the first key given. // That is, the peer who gave us this proof is not omitting nodes. if err := addPathInfo( view, proof.StartProof, - smallestProvenPath, - largestProvenPath, + smallestProvenKey, + largestProvenKey, ); err != nil { return err } if err := addPathInfo( view, proof.EndProof, - smallestProvenPath, - largestProvenPath, + smallestProvenKey, + largestProvenKey, ); err != nil { return err } @@ -462,13 +462,13 @@ func (proof *RangeProof) UnmarshalProto(pbProof *pb.RangeProof) error { func verifyAllRangeProofKeyValuesPresent(proof []ProofNode, start maybe.Maybe[Key], end maybe.Maybe[Key], keysValues map[Key][]byte) error { for i := 0; i < len(proof); i++ { var ( - node = proof[i] - nodePath = node.Key + node = proof[i] + nodeKey = node.Key ) // Skip keys that cannot have a value (enforced by [verifyProofPath]). - if !nodePath.hasPartialByte() && (start.IsNothing() || !nodePath.Less(start.Value())) && (end.IsNothing() || !nodePath.Greater(end.Value())) { - value, ok := keysValues[nodePath] + if !nodeKey.hasPartialByte() && (start.IsNothing() || !nodeKey.Less(start.Value())) && (end.IsNothing() || !nodeKey.Greater(end.Value())) { + value, ok := keysValues[nodeKey] if !ok && node.ValueOrHash.HasValue() { // We didn't get a key-value pair for this key, but the proof node has a value. return ErrProofNodeHasUnincludedValue @@ -488,7 +488,7 @@ type KeyChange struct { Value maybe.Maybe[[]byte] } -// A change proof proves that a set of key-value changes occurred +// ChangeProof proves that a set of key-value changes occurred // between two trie roots, where each key-value pair's key is // between some lower and upper bound (inclusive). type ChangeProof struct { @@ -622,8 +622,8 @@ func (proof *ChangeProof) UnmarshalProto(pbProof *pb.ChangeProof) error { } // Verifies that all values present in the [proof]: -// - Are nothing when deleted, not in the db, or the node has path partial byte length -// - if the node's path is within the key range, that has a value that matches the value passed in the change list or in the db +// - Are nothing when deleted, not in the db, or the node has key partial byte length +// - if the node's key is within the key range, that has a value that matches the value passed in the change list or in the db func verifyAllChangeProofKeyValuesPresent( ctx context.Context, db MerkleDB, @@ -634,19 +634,19 @@ func verifyAllChangeProofKeyValuesPresent( ) error { for i := 0; i < len(proof); i++ { var ( - node = proof[i] - nodePath = node.Key + node = proof[i] + nodeKey = node.Key ) // Check the value of any node with a key that is within the range. // Skip keys that cannot have a value (enforced by [verifyProofPath]). - if !nodePath.hasPartialByte() && (start.IsNothing() || !nodePath.Less(start.Value())) && (end.IsNothing() || !nodePath.Greater(end.Value())) { - value, ok := keysValues[nodePath] + if !nodeKey.hasPartialByte() && (start.IsNothing() || !nodeKey.Less(start.Value())) && (end.IsNothing() || !nodeKey.Greater(end.Value())) { + value, ok := keysValues[nodeKey] if !ok { // This value isn't in the list of key-value pairs we got. - dbValue, err := db.GetValue(ctx, nodePath.Bytes()) + dbValue, err := db.GetValue(ctx, nodeKey.Bytes()) if err != nil { - if err != database.ErrNotFound { + if !errors.Is(err, database.ErrNotFound) { return err } // This key isn't in the database so proof node should have Nothing. @@ -669,7 +669,7 @@ func (proof *ChangeProof) Empty() bool { len(proof.StartProof) == 0 && len(proof.EndProof) == 0 } -// Exactly one of [ChangeProof] or [RangeProof] is non-nil. +// ChangeOrRangeProof has exactly one of [ChangeProof] or [RangeProof] is non-nil. type ChangeOrRangeProof struct { ChangeProof *ChangeProof RangeProof *RangeProof diff --git a/x/merkledb/trieview.go b/x/merkledb/trieview.go index c905cb82c218..d8d9cfbdeb28 100644 --- a/x/merkledb/trieview.go +++ b/x/merkledb/trieview.go @@ -149,7 +149,7 @@ func newTrieView( ) (*trieView, error) { root, err := parentTrie.getEditableNode(Key{}, false /* hasValue */) if err != nil { - if err == database.ErrNotFound { + if errors.Is(err, database.ErrNotFound) { return nil, ErrNoValidRoot } return nil, err @@ -273,8 +273,8 @@ func (t *trieView) calculateNodeIDsHelper(n *node) { ) for childIndex, child := range n.children { - childPath := n.key.Extend(ToToken(childIndex, t.tokenSize), child.compressedKey) - childNodeChange, ok := t.changes.nodes[childPath] + childKey := n.key.Extend(ToToken(childIndex, t.tokenSize), child.compressedKey) + childNodeChange, ok := t.changes.nodes[childKey] if !ok { // This child wasn't changed. continue @@ -811,7 +811,7 @@ func (t *trieView) insert( // have the existing path node and the value being inserted as children. // generate the new branch node - // find how many tokens are common between the existing child's compressed path and + // find how many tokens are common between the existing child's compressed key and // the current key(offset by the closest node's key), // then move all the common tokens into the branch node commonPrefixLength := getLengthOfCommonPrefix( @@ -910,7 +910,7 @@ func (t *trieView) recordKeyChange(key Key, after *node, hadValue bool, newNode } before, err := t.getParentTrie().getEditableNode(key, hadValue) - if err != nil && err != database.ErrNotFound { + if err != nil && !errors.Is(err, database.ErrNotFound) { return err } t.changes.nodes[key] = &change[*node]{ diff --git a/x/sync/network_server.go b/x/sync/network_server.go index 6f21702ce397..c213bee6a739 100644 --- a/x/sync/network_server.go +++ b/x/sync/network_server.go @@ -39,7 +39,6 @@ const ( // TODO: refine this estimate. This is almost certainly a large overestimate. estimatedMessageOverhead = 4 * units.KiB maxByteSizeLimit = constants.DefaultMaxMessageSize - estimatedMessageOverhead - endProofSizeBufferAmount = 2 * units.KiB ) var (