feat(ctb): MerkleTrie fuzzing#4008
feat(ctb): MerkleTrie fuzzing#4008clabby wants to merge 15 commits intosc/ctb-mini-trie-refactorfrom
Conversation
Small refactor that has MerkleTrie.get throw explicitly when an element does not exist, rather than returning a non-existence boolean. Makes the code much cleaner. We need to review this change very carefully.
🦋 Changeset detectedLatest commit: c84e567 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
2-reviewers
A-pkg-contracts-bedrock
clabby
force-pushed
the
@clabby/ctb/merkle-trie-fuzz
branch
from
11be109 to
f6d0738
Compare
packages/contracts-bedrock/scripts/merkle-trie-fuzzer/README.md
Outdated
Show resolved
Hide resolved
clabby
force-pushed
the
@clabby/ctb/merkle-trie-fuzz
branch
from
f1001d0 to
d033241
Compare
github-actions
bot
added
C-protocol-critical
clabby
force-pushed
the
@clabby/ctb/merkle-trie-fuzz
branch
from
82f42d1 to
eb29cf9
Compare
github-actions
bot
removed
M-contracts
A-cannon
trianglesphere
left a comment
trianglesphere
left a comment
There was a problem hiding this comment.
Really nice work. I've left a bunch of comments, but they're primarily focused on go tips & tricks and go style.
| // Helper that generates a cryptographically secure random 64-bit integer | ||
| // between the range [min, max] | ||
| func randRange(min int64, max int64) int64 { | ||
| r, err := rand.Int(rand.Reader, new(big.Int).Sub(new(big.Int).SetInt64(max), new(big.Int).SetInt64(min))) |
There was a problem hiding this comment.
I'd suggest using math/rand.Intn here. I don't think we need a cryptographically secure random number for fuzzing. math/rand will be faster in general & will avoid all of the big int math.
There was a problem hiding this comment.
The idea behind switching to crypto/rand was to allow for randomness while executing these tests in parallel. Originally, I had passed the hash of a 32 byte fuzz input from the foundry test + the current time as the seed for math/rand, but felt this was a cleaner solution. See mark's above comment
|
|
||
| var testCase trieTestCase | ||
| switch variant { | ||
| case "valid": |
There was a problem hiding this comment.
I'd prefer an enum (+ documentation) for the variants. In go they're typically implemented as follows (though we probably forgot the type inside the const statement).
optimism/op-node/rollup/derive/batches.go
Lines 15 to 26 in c3493a0
There was a problem hiding this comment.
Swapped over to using enums- went with string variants so that they are readable within foundry tests w/o having to reference the variant docs.
i.e.
ffi.getMerkleTrieFuzzCase("valid") reads better than ffi.getMerkleTrieFuzzCase(0)
| @@ -0,0 +1,13 @@ | |||
| # `go-fuzz` | |||
There was a problem hiding this comment.
go-fuzz conflicts with some existing projects (https://github.com/google/gofuzz & https://github.com/dvyukov/go-fuzz). This isn't the end of the world, but I think that a more descriptive name (something along of the lines of TrieTestCaseGenerator would be a bit better).
smartcontracts
force-pushed
the
sc/ctb-mini-trie-refactor
branch
from
b6058a3 to
991120f
Compare
|
Hey @clabby! This PR has merge conflicts. Please fix them before continuing review. |
|
Reopened in #4142 - certain CI tasks didn't like the branch name. |
5 participants
Overview
WIP
Adds a fuzz test input generator for testing Bedrock's
MerkleTrie.sol.Go version of #3847
Tests
Adds fuzz tests for
MerkleTrieAdditional context
n/a
Metadata
n/a