statetrie: nibbles #5719
statetrie: nibbles #5719
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5719 +/- ##
==========================================
+ Coverage 55.51% 55.57% +0.06%
==========================================
Files 473 474 +1
Lines 66316 66378 +62
==========================================
+ Hits 36814 36891 +77
+ Misses 27005 26996 -9
+ Partials 2497 2491 -6
... and 13 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Good start to the statetrie code.
However, I wonder if putting the nibble code in its own package (e.g. crypto/statetrie/nibbles or crypto/nibbles) might be beneficial. The advantages would be:
- Since the nibble code doesn't import any other go-algorand packages (and assuming the broader
crypto/statetriepackage would certainly do so), it would make a nice standalone package that could be used anywhere without risk of circular imports. - Other packages would see its exported symbols as
nibbles.*. This can simplify naming of your functions. For example, instead ofequalNibbles, you could just exportEquals, and consumers would see it asnibbles.Equals.
crypto/statetrie/nibbles.go
Outdated
| j := 0 | ||
| for i := 0; i < length; i++ { | ||
| halfWidth = !halfWidth | ||
| if halfWidth { |
There was a problem hiding this comment.
nit: I'd personally find i % 2 == 0 to be a clearer condition than halfWidth (and I see you use it in pack)
crypto/statetrie/nibbles.go
Outdated
| // [0x1, 0x2, 0x3, 0x4] -> [0x12, 0x34], false | ||
| // [0x1] -> [0x10], true | ||
| // [] -> [], false | ||
| func (ns *Nibbles) pack() ([]byte, bool) { |
There was a problem hiding this comment.
Why use a pointer receiver?
crypto/statetrie/nibbles.go
Outdated
| buf.WriteByte(3) | ||
| } | ||
|
|
||
| return buf.Bytes() |
There was a problem hiding this comment.
minor: since you know the output length, it's likely more efficient to make a byte array than use bytes.Buffer
crypto/statetrie/nibbles_test.go
Outdated
| } | ||
| for i, n := range sampleNibbles { | ||
| require.True(t, bytes.Equal(shiftNibbles(n, 1), sampleNibblesShifted1[i])) | ||
| require.True(t, bytes.Equal(shiftNibbles(n, 2), sampleNibblesShifted2[i])) |
There was a problem hiding this comment.
Can you test 0 shift as well? And might as well do a negative number since you built in support for that case
edit: I see you do that a few lines down. Could you relocate that closer to here? And might as well put it in the loop instead of just testing with sampleNibbles[0]
crypto/statetrie/nibbles_test.go
Outdated
| {{}, {0x1}}, | ||
| } | ||
| for _, n := range sampleEqualTrue { | ||
| require.True(t, equalNibbles(n[0], n[1])) |
There was a problem hiding this comment.
nit: no need for the sampleEqualTrue array, you could just iterate over sampleEqualsFalse and test equalNibbles(n[0], n[0])
crypto/statetrie/nibbles_test.go
Outdated
| require.True(t, bytes.Equal(b, sampleNibblesPacked[i])) | ||
|
|
||
| unp := unpack(b, half) | ||
| require.True(t, bytes.Equal(unp, n)) |
There was a problem hiding this comment.
I'd like to see more usage of require.Equals and related purpose-driven functions over require.True. There are two benefits to this:
- (subjective) Assertion code is simpler and easier to read
- (objective) When an error occurs, more context is printed in the error message, which makes it way easier to debug. With
require.True, all we'd get is a message that says something like "got false when expected true"
|
|
||
| func TestNibbles(t *testing.T) { // nolint:paralleltest // Serial tests for trie for the moment | ||
| partitiontest.PartitionTest(t) | ||
| // t.Parallel() |
There was a problem hiding this comment.
why not parallel? there is not external dependencies
|
superseeded by #5759 |
This PR adds types and utility functions for manipulating nibbles (4-bit value arrays). It is specifically to support a new package, statetrie, which will use Nibbles and their associated utility functions.
The statetrie is designed to have keys with sequences of 4-bit bytes. A statetrie with 16-ary branch nodes has smaller proofs than a full byte (256-ary).