From 5b1b3d840fcfab06c6399b114b05247c1a2058e2 Mon Sep 17 00:00:00 2001 From: bruscar010 <28549164+bruscar010@users.noreply.github.com> Date: Fri, 17 Jan 2025 15:36:41 +0000 Subject: [PATCH] fix: reduce calls to MinNamespace & MaxNamespace (#285) In this issue https://github.com/celestiaorg/nmt/issues/216, it was raised that 1. More time was spent allocating than hashing in the HashLeaf function 2. The min and max namespace functions were taking up a large amount of RAM --- ## More time was spent allocating than hashing in the HashLeaf function The benchmark test described in the issue listed above no longer exists, and when I run the `BenchmarkComputeRoot` in the nmt package [HERE](https://github.com/bruscar010/nmt/blob/main/nmt_test.go#L696) I can't replicate memory allocation taking longer than the hashing, although this may be related to my test parameters. Below are FlameGraphs of CPU/Mem usage (I've updated the Benchmark to have 20k leaf nodes with 512 bytes transaction size) [Base Mem Flamegraph](https://flamegraph.com/share/6a04ffab-c866-11ef-9832-26c3e5347170) [Base CPU Flamegraph](https://flamegraph.com/share/efce1d16-c868-11ef-b027-3a58f36d25ce) ``` BenchmarkComputeRoot/64-leaves-12 100 141785 ns/op 65166 B/op 1535 allocs/op BenchmarkComputeRoot/128-leaves-12 100 280652 ns/op 123894 B/op 3074 allocs/op BenchmarkComputeRoot/256-leaves-12 100 501392 ns/op 254611 B/op 6154 allocs/op ``` I've instead focused on the second issue --- ## The min and max namespace functions were taking up a large amount of RAM ### Solution 1: Reduce the number of calls to MinNamespace & MaxNamespace The HashNode function made 4 duplicate function calls to MaxNamespace/MinNamespace for the same node. ``` 1. HashNode -> ValidateNodes -> ValidateNodeFormat.Min/MaxNamespace 2. HashNode -> ValidateNodes -> validateSiblingsNamespaceOrder -> ValidateNodeFormat(left|right).Min/MaxNamespace 3. HashNode -> ValidateNodes -> validateSiblingsNamespaceOrder.Min/MaxNamespace 4. HashNode.Min/MaxNamespace ``` After reducing to a single call, here is the new benchmark output (running `go test -run="none" -bench=. -benchtime=100x -benchmem`) ``` BenchmarkComputeRoot/64-leaves-12 100 137292 ns/op 60100 B/op 905 allocs/op BenchmarkComputeRoot/128-leaves-12 100 237664 ns/op 113758 B/op 1804 allocs/op BenchmarkComputeRoot/256-leaves-12 100 462802 ns/op 234241 B/op 3604 allocs/op ``` ### Solution 2: Update the MinNamespace/MaxNamespace to return a slice of the existing array rather than creating a new underlying array Copying the slice stops the possibility [array capacity memory leaks](https://itnext.io/mastering-memory-management-in-go-avoiding-slice-related-leaks-7d5e97ee48d4) if for some reason the namespace ID variable's lifetime is longer than the lifetime of the original slice, but I don't think that is the case. [Slicing with a max capacity also stops the potential for appends to the namespace ID slice editing the underlying](https://build-your-own.org/blog/20230316_go_full_slice/) `ns|ns|hash` array. Benchmark output after this change ``` BenchmarkComputeRoot/64-leaves-12 100 136436 ns/op 58180 B/op 653 allocs/op BenchmarkComputeRoot/128-leaves-12 100 241836 ns/op 109747 B/op 1297 allocs/op BenchmarkComputeRoot/256-leaves-12 100 470231 ns/op 225924 B/op 2583 allocs/op ``` [New Mem Flamegraph](https://flamegraph.com/share/71b8ef18-c86a-11ef-b027-3a58f36d25ce) [New CPU Flamegraph](https://flamegraph.com/share/7f46d1b4-c86a-11ef-9832-26c3e5347170) --- The improvements made by the first solution are made redundant by the second, but I was initially reluctant to replace the slice copy in case it would lead to a memory leak. As I got more familiar with the package, I became more confident that a slicing the array may be more appropriate. **Note** - Given that validation is done in the `Push` method, adding an `UnsafeHashNode` that skips validation was also a method I considered, but I felt that this could lead to more complications & would not make any notable differences compared to either of the other solutions. - After reworking the `ValidateSiblings` function, I also removed the associated tests, but it looks like the same test cases also exist in `TestValidateNodes` [HERE](https://github.com/bruscar010/nmt/blob/main/hasher_test.go#L684) ## Summary by CodeRabbit - **New Features** - Enhanced namespace validation methods with improved error reporting and context. - Introduced `nsIDRange` struct for better namespace ID range management. - **Refactor** - Streamlined namespace-related functions in `MinNamespace` and `MaxNamespace`. - Improved code readability and maintainability in namespace merkle tree hasher. - **Tests** - Added a benchmark for `ComputeRoot` with 20,000 leaves. - Removed existing sibling node validation test. --------- Co-authored-by: francis <28549164+FrancisLennon17@users.noreply.github.com> --- hasher.go | 86 +++++++++++++++++++++++++++++--------------------- hasher_test.go | 50 ----------------------------- nmt.go | 6 ++-- nmt_test.go | 1 + 4 files changed, 53 insertions(+), 90 deletions(-) diff --git a/hasher.go b/hasher.go index cb5b3fb8..f0d174ff 100644 --- a/hasher.go +++ b/hasher.go @@ -154,9 +154,8 @@ func (n *NmtHasher) BlockSize() int { func (n *NmtHasher) EmptyRoot() []byte { n.baseHasher.Reset() - emptyNs := bytes.Repeat([]byte{0}, int(n.NamespaceLen)) h := n.baseHasher.Sum(nil) - digest := append(append(emptyNs, emptyNs...), h...) + digest := append(make([]byte, int(n.NamespaceLen)*2), h...) return digest } @@ -212,42 +211,64 @@ func (n *NmtHasher) MustHashLeaf(ndata []byte) []byte { return res } -// ValidateNodeFormat checks whether the supplied node conforms to the -// namespaced hash format and returns ErrInvalidNodeLen if not. -func (n *NmtHasher) ValidateNodeFormat(node []byte) (err error) { +// nsIDRange represents the range of namespace IDs with minimum and maximum values. +type nsIDRange struct { + Min, Max namespace.ID +} + +// tryFetchNodeNSRange attempts to return the min and max namespace ids. +// It will return an ErrInvalidNodeLen | ErrInvalidNodeNamespaceOrder +// if the supplied node does not conform to the namespaced hash format. +func (n *NmtHasher) tryFetchNodeNSRange(node []byte) (nsIDRange, error) { expectedNodeLen := n.Size() nodeLen := len(node) if nodeLen != expectedNodeLen { - return fmt.Errorf("%w: got: %v, want %v", ErrInvalidNodeLen, nodeLen, expectedNodeLen) + return nsIDRange{}, fmt.Errorf("%w: got: %v, want %v", ErrInvalidNodeLen, nodeLen, expectedNodeLen) } // check the namespace order minNID := namespace.ID(MinNamespace(node, n.NamespaceSize())) maxNID := namespace.ID(MaxNamespace(node, n.NamespaceSize())) if maxNID.Less(minNID) { - return fmt.Errorf("%w: max namespace ID %d is less than min namespace ID %d ", ErrInvalidNodeNamespaceOrder, maxNID, minNID) + return nsIDRange{}, fmt.Errorf("%w: max namespace ID %d is less than min namespace ID %d ", ErrInvalidNodeNamespaceOrder, maxNID, minNID) } - return nil + return nsIDRange{Min: minNID, Max: maxNID}, nil } -// validateSiblingsNamespaceOrder checks whether left and right as two sibling -// nodes in an NMT have correct namespace IDs relative to each other, more -// specifically, the maximum namespace ID of the left sibling should not exceed -// the minimum namespace ID of the right sibling. It returns ErrUnorderedSiblings error if the check fails. -func (n *NmtHasher) validateSiblingsNamespaceOrder(left, right []byte) (err error) { - if err := n.ValidateNodeFormat(left); err != nil { - return fmt.Errorf("%w: left node does not match the namesapce hash format", err) +// ValidateNodeFormat checks whether the supplied node conforms to the +// namespaced hash format and returns an error if not. +func (n *NmtHasher) ValidateNodeFormat(node []byte) error { + _, err := n.tryFetchNodeNSRange(node) + return err +} + +// tryFetchLeftAndRightNSRange attempts to return the min/max namespace ids of both +// the left and right nodes. It verifies whether left +// and right comply by the namespace hash format, and are correctly ordered +// according to their namespace IDs. +func (n *NmtHasher) tryFetchLeftAndRightNSRanges(left, right []byte) ( + nsIDRange, + nsIDRange, + error, +) { + var lNsRange nsIDRange + var rNsRange nsIDRange + var err error + + lNsRange, err = n.tryFetchNodeNSRange(left) + if err != nil { + return lNsRange, rNsRange, err } - if err := n.ValidateNodeFormat(right); err != nil { - return fmt.Errorf("%w: right node does not match the namesapce hash format", err) + rNsRange, err = n.tryFetchNodeNSRange(right) + if err != nil { + return lNsRange, rNsRange, err } - leftMaxNs := namespace.ID(MaxNamespace(left, n.NamespaceSize())) - rightMinNs := namespace.ID(MinNamespace(right, n.NamespaceSize())) // check the namespace range of the left and right children - if rightMinNs.Less(leftMaxNs) { - return fmt.Errorf("%w: the maximum namespace of the left child %x is greater than the min namespace of the right child %x", ErrUnorderedSiblings, leftMaxNs, rightMinNs) + if rNsRange.Min.Less(lNsRange.Max) { + err = fmt.Errorf("%w: the min namespace ID of the right child %d is less than the max namespace ID of the left child %d", ErrUnorderedSiblings, rNsRange.Min, lNsRange.Max) } - return nil + + return lNsRange, rNsRange, err } // ValidateNodes is a helper function to verify the @@ -255,13 +276,8 @@ func (n *NmtHasher) validateSiblingsNamespaceOrder(left, right []byte) (err erro // and right comply by the namespace hash format, and are correctly ordered // according to their namespace IDs. func (n *NmtHasher) ValidateNodes(left, right []byte) error { - if err := n.ValidateNodeFormat(left); err != nil { - return err - } - if err := n.ValidateNodeFormat(right); err != nil { - return err - } - return n.validateSiblingsNamespaceOrder(left, right) + _, _, err := n.tryFetchLeftAndRightNSRanges(left, right) + return err } // HashNode calculates a namespaced hash of a node using the supplied left and @@ -278,21 +294,19 @@ func (n *NmtHasher) ValidateNodes(left, right []byte) error { // If the namespace range of the right child is start=end=MAXNID, indicating that it represents the root of a subtree whose leaves all have the namespace ID of `MAXNID`, then exclude the right child from the namespace range calculation. Instead, // assign the namespace range of the left child as the parent's namespace range. func (n *NmtHasher) HashNode(left, right []byte) ([]byte, error) { - // validate the inputs - if err := n.ValidateNodes(left, right); err != nil { + // validate the inputs & fetch the namespace ranges + lRange, rRange, err := n.tryFetchLeftAndRightNSRanges(left, right) + if err != nil { return nil, err } h := n.baseHasher h.Reset() - leftMinNs, leftMaxNs := MinNamespace(left, n.NamespaceLen), MaxNamespace(left, n.NamespaceLen) - rightMinNs, rightMaxNs := MinNamespace(right, n.NamespaceLen), MaxNamespace(right, n.NamespaceLen) - // compute the namespace range of the parent node - minNs, maxNs := computeNsRange(leftMinNs, leftMaxNs, rightMinNs, rightMaxNs, n.ignoreMaxNs, n.precomputedMaxNs) + minNs, maxNs := computeNsRange(lRange.Min, lRange.Max, rRange.Min, rRange.Max, n.ignoreMaxNs, n.precomputedMaxNs) - res := make([]byte, 0) + res := make([]byte, 0, len(minNs)*2) res = append(res, minNs...) res = append(res, maxNs...) diff --git a/hasher_test.go b/hasher_test.go index 0bf1178e..8014b0b3 100644 --- a/hasher_test.go +++ b/hasher_test.go @@ -283,56 +283,6 @@ func TestHashNode_Error(t *testing.T) { } } -func TestValidateSiblings(t *testing.T) { - // create a dummy hash to use as the digest of the left and right child - randHash := createByteSlice(sha256.Size, 0x01) - - type children struct { - l []byte // namespace hash of the left child with the format of MinNs||MaxNs||h - r []byte // namespace hash of the right child with the format of MinNs||MaxNs||h - } - - tests := []struct { - name string - nidLen namespace.IDSize - children children - wantErr bool - }{ - { - "wrong left node format", 2, - children{concat([]byte{0, 0, 1, 1}, randHash[:len(randHash)-1]), concat([]byte{0, 0, 1, 1}, randHash)}, - true, - }, - { - "wrong right node format", 2, - children{concat([]byte{0, 0, 1, 1}, randHash), concat([]byte{0, 0, 1, 1}, randHash[:len(randHash)-1])}, - true, - }, - { - "left.maxNs>right.minNs", 2, - children{concat([]byte{0, 0, 1, 1}, randHash), concat([]byte{0, 0, 1, 1}, randHash)}, - true, - }, - { - "left.maxNs=right.minNs", 2, - children{concat([]byte{0, 0, 1, 1}, randHash), concat([]byte{1, 1, 2, 2}, randHash)}, - false, - }, - { - "left.maxNs