From 47b74de518347f5470ce28bf2e2e2a3a71b02cd4 Mon Sep 17 00:00:00 2001 From: Daniel Olshansky Date: Mon, 3 Jun 2024 12:48:06 -0700 Subject: [PATCH] Self review --- docs/audit.md | 91 --------------------------------------------------- docs/faq.md | 2 -- docs/smt.md | 4 +-- e2e_review.md | 67 ------------------------------------- hasher.go | 4 +-- options.go | 2 +- smst.go | 4 +-- trie_spec.go | 2 +- types.go | 2 +- 9 files changed, 9 insertions(+), 169 deletions(-) delete mode 100644 docs/audit.md delete mode 100644 e2e_review.md diff --git a/docs/audit.md b/docs/audit.md deleted file mode 100644 index f1852f1..0000000 --- a/docs/audit.md +++ /dev/null @@ -1,91 +0,0 @@ -# Audit - -- [Audit](#audit) - -## Pre-Audit Checklist - -### Checklist Requirements - -Pre-Audit Checklist: - -A reminder to provide us with the must haves 3 business days prior to the audit start days to avoid delays: - -**Must haves:** - -- A URL of the repository containing the source code -- The release branch / commit hash to be reviewed -- An explicit list of files in scope and out of scope for the security audit -- Robust and comprehensive documentation describing the intended functionality of the system - -**Nice-to-haves:** - -- Clear instructions for setting up the system and run the tests (usually in the README file) -- Any past audits -- Any tooling output logs -- Output generated by running the test suite -- Test coverage report - -Please disregard what may be irrelevant in the above list for this particular audit. -Reminder that we cannot accept any scope changes during the course of the audit. - -And more on audit preparation in this blogpost: https://medium.com/thesis-defense/why-crypto-needs-security-audits-d12f3909ac21 - thanks! - -### Checklist Response - -**Repository**: https://github.com/pokt-network/smt - -- **Branch**: `main` -- **Hash**: `868237978c0b3c0e2added161b36eeb7a3dc93b0` - -**Documentation** - -- **Background**: [Relay Mining](https://arxiv.org/abs/2305.10672) -- **Technical Documentation**: https://github.com/pokt-network/smt/tree/main/docs -- _NOTE: we may integrate this into [https://dev.poktroll.com](https://dev.poktroll.com/) (which is out of scope) in the future_ - -**Files** - -- Nothing is explicitly out of scope but the focus should be on the files below -- The following is a manually filtered list of files after running `tree -P '*.go' -I '*_test.go'` - -```bash - . - ├── errors.go - ├── hasher.go - ├── kvstore - │   ├── badger - │   │   ├── errors.go - │   │   ├── godoc.go - │   │   ├── interface.go - │   │   └── kvstore.go - │   ├── interfaces.go - ├── options.go - ├── proofs.go - ├── smst.go - ├── smt.go - ├── types.go - └── utils.go -``` - -**Makefile** - -- Running `make` in the root of the repo shows a list of options -- This gives access to tests, benchmarks, etc... - -```bash -make -help Prints all the targets in all the Makefiles -list List all make targets -test_all runs the test suite -test_badger runs the badger KVStore submodule's test suite -mod_tidy runs go mod tidy for all (sub)modules -go_docs Generate documentation for the project -benchmark_all runs all benchmarks -benchmark_smt runs all benchmarks for the SMT -benchmark_smt_fill runs a benchmark on filling the SMT with different amounts of values -benchmark_smt_ops runs the benchmarks testing different operations on the SMT against different sized tries -benchmark_smst runs all benchmarks for the SMST -benchmark_smst_fill runs a benchmark on filling the SMST with different amounts of values -benchmark_smst_ops runs the benchmarks test different operations on the SMST against different sized tries -benchmark_proof_sizes runs the benchmarks test the proof sizes for different sized tries -``` diff --git a/docs/faq.md b/docs/faq.md index d66b496..7dcd4c4 100644 --- a/docs/faq.md +++ b/docs/faq.md @@ -23,5 +23,3 @@ The [SMT extension node](./smt.md#extension-nodes) is very similar to that of Ethereum's [Modified Merkle Patricia Trie](https://ethereum.org/developers/docs/data-structures-and-encoding/patricia-merkle-trie). A quick primer on it can be found in this [5P;1R post](https://olshansky.substack.com/p/5p1r-ethereums-modified-merkle-patricia). - -WIP diff --git a/docs/smt.md b/docs/smt.md index eae4ddc..01b2ecc 100644 --- a/docs/smt.md +++ b/docs/smt.md @@ -86,8 +86,8 @@ Extension nodes represent a singly linked chain of inner nodes, with a single child. In other words, they are an optimization to avoid having a long chain of inner nodes where each inner node only has one child. -In other words, they are used to -represent a common path in the trie and as such contain the path and bounds of the path they represent. The `digest` of an extension +In other words, they are used to represent a common path in the trie and as such +contain the path and bounds of the path they represent. The `digest` of an extension node is the hash of its path bounds, the path itself and the child nodes digest concatenated. diff --git a/e2e_review.md b/e2e_review.md deleted file mode 100644 index 99da43b..0000000 --- a/e2e_review.md +++ /dev/null @@ -1,67 +0,0 @@ -# E2E Code Review - - -[ ] . - -[ ] ├── LICENSE - -[ ] ├── Makefile - -[ ] ├── README.md - -[ ] ├── benchmarks - -[ ] │   ├── bench_leaf_test.go - -[ ] │   ├── bench_smst_test.go - -[ ] │   ├── bench_smt_test.go - -[ ] │   ├── bench_utils_test.go - -[ ] │   └── proof_sizes_test.go - -[ ] ├── bulk_test.go - -[ ] ├── docs - -[ ] │   ├── audit.md - -[ ] │   ├── badger-store.md - -[ ] │   ├── benchmarks.md - -[ ] │   ├── faq.md - -[ ] │   ├── mapstore.md - -[ ] │   ├── merkle-sum-trie.md - -[ ] │   └── smt.md - -[ ] ├── errors.go - -[ ] ├── extension_node.go - -[ ] ├── fuzz_test.go - -[x] ├── go.mod - -[x] ├── go.sum - -[x] ├── go.work - -[x] ├── go.work.sum - -[x] ├── godoc.go - -[ ] ├── hasher.go - -[x] ├── inner_node.go - -[ ] ├── kvstore - -[ ] │   ├── badger - -[ ] │   │   ├── errors.go - -[ ] │   │   ├── go.mod - -[ ] │   │   ├── go.sum - -[ ] │   │   ├── godoc.go - -[ ] │   │   ├── interface.go - -[ ] │   │   ├── kvstore.go - -[ ] │   │   └── kvstore_test.go - -[ ] │   ├── godoc.go - -[ ] │   ├── interfaces.go - -[ ] │   └── simplemap - -[ ] │   ├── errors.go - -[ ] │   ├── godoc.go - -[ ] │   ├── simplemap.go - -[ ] │   └── simplemap_test.go - -[x] ├── lazy_node.go - -[x] ├── leaf_node.go - -[x] ├── node_encoders.go - -[x] ├── options.go - -[ ] ├── proofs.go - -[ ] ├── proofs_test.go - -[ ] ├── reviewpad.yml - -[ ] ├── root_test.go - -[ ] ├── smst.go - -[ ] ├── smst_example_test.go - -[ ] ├── smst_proofs_test.go - -[ ] ├── smst_test.go - -[ ] ├── smst_utils_test.go - -[ ] ├── smt.go - -[ ] ├── smt_example_test.go - -[ ] ├── smt_proofs_test.go - -[ ] ├── smt_test.go - -[ ] ├── smt_utils_test.go - -[ ] ├── types.go - -[ ] └── utils.go diff --git a/hasher.go b/hasher.go index 220eddd..dace46b 100644 --- a/hasher.go +++ b/hasher.go @@ -5,8 +5,8 @@ import ( "hash" ) -// TODO_IN_THIS_PR: Improve how the `hasher` file is consolidated (or not) -// with `node_encoders.go` since the two are very similar. +// TODO_IMPROVE:: Improve how the `hasher` file is consolidated with +// `node_encoders.go` since the two are very similar. // Ensure the hasher interfaces are satisfied var ( diff --git a/options.go b/options.go index 4556a4c..c3fea18 100644 --- a/options.go +++ b/options.go @@ -22,7 +22,7 @@ func WithValueHasher(vh ValueHasher) TrieSpecOption { // used instead of a key during proof verification. Otherwise, this will lead // double hashing and product an incorrect leaf digest, thereby invalidating // the proof. -// TODO_IN_THIS_PR: Need to understand this part more. +// TODO_TECHDEBT: Document better when/why this is needed. func NoHasherSpec(hasher hash.Hash, sumTrie bool) *TrieSpec { spec := newTrieSpec(hasher, sumTrie) diff --git a/smst.go b/smst.go index 64f8e94..27d6c28 100644 --- a/smst.go +++ b/smst.go @@ -33,8 +33,8 @@ func NewSparseMerkleSumTrie( } // Initialize a non-sum SMT and modify it to have a nil value hasher - // TODO_IN_THIS_PR: Understand the purpose of the nilValueHasher and why - // we're not applying it to the smst but we need it for the smt. + // TODO_UPNEXT(@Olshansk): Understand the purpose of the nilValueHasher and + // why we're not applying it to the smst but we need it for the smt. smt := &SMT{ TrieSpec: trieSpec, nodes: nodes, diff --git a/trie_spec.go b/trie_spec.go index 55a0af0..2b3ab73 100644 --- a/trie_spec.go +++ b/trie_spec.go @@ -112,7 +112,7 @@ func (spec *TrieSpec) hashSumSerialization(data []byte) []byte { // depth returns the maximum depth of the trie. // Since this tree is a binary tree, the depth is the number of bits in the path -// TODO_IN_THIS_PR: Try to understand why we're not taking the log of the output +// TODO_UPNEXT(@Olshansk):: Try to understand why we're not taking the log of the output func (spec *TrieSpec) depth() int { return spec.ph.PathSize() * 8 // path size is in bytes so multiply by 8 to get num bits } diff --git a/types.go b/types.go index 0be9de6..93ed09f 100644 --- a/types.go +++ b/types.go @@ -1,6 +1,6 @@ package smt -// TODO_DISCUSS_IN_THIS_PR_IMPROVEMENTS: +// TODO_DISCUSS_IN_THE_FUTURE: // 1. Should we rename all instances of digest to hash? // 2. Should we introduce a shared interface between SparseMerkleTrie and SparseMerkleSumTrie? // 3. Should we rename Commit to FlushToDisk?