Add SQL storage support #20
Conversation
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
…rcuit Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
| // the node that doesn't exist yet. We can add the new leaf node here | ||
| return mt.addNode(newLeaf) |
There was a problem hiding this comment.
Do you need a write lock?
There was a problem hiding this comment.
It's confusing because inside addNode you also do a getNode and none of these have a lock so you are always relying on the calling function to lock which is dangerous. I understand why you are doing it because in theaddLeaf function you need to iterate and you don't want to lock and unlock on each iteration.
There was a problem hiding this comment.
the nested addLeaf() calls are the main reason for now doing more granular lock/unlock. The series of updates from the lead node up to the root with the branch nodes along the way must be atomic, otherwise we end up with a broken/inconsistent tree. thus I felt having an overall write lock on the calling function is the best way to go.
zkp/golang/internal/storage/sql.go
Outdated
| const ( | ||
| // we use a table to store the root node indexes for | ||
| // all the merkle trees in the database | ||
| TreeRootsTable = "smtRoots" |
There was a problem hiding this comment.
should it just be merkelTreeRoots instead of sparse merkle tree in case we implement another type of tree?
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package storage |
There was a problem hiding this comment.
I also feels like this package should be inside the merkle tree pkg and not storage, because it's not a generic read from this table. It's more like read a node from this merkle tree that happens to be stored in SQL and decode it in such a way to determine if it's a leaf or a branch node etc...
There was a problem hiding this comment.
good point. in fact there's a subsequent PR that move all the merkle tree specific packages into a top-level sparse-merkle-tree package. hope it's OK to keep it as-is for this PR for now
zkp/golang/internal/storage/sql.go
Outdated
| n := core.SMTNode{ | ||
| RefKey: ref.Hex(), | ||
| } | ||
| err := m.p.DB().Table(m.nodesTableName).First(&n).Error |
There was a problem hiding this comment.
I wonder what it safer:
- To have a Golang RLock?
- To have an actual DB lock on write?
There was a problem hiding this comment.
I think in our case implementing the locks in golang might be the way to do, as there are complex call paths that goes from writing to reading, such as when addLeaf() calls getNode(). it'd be very complex and dead-lock prone to try to do the locks inside the DB calls in the storage package.
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
| - uses: codecov/codecov-action@v4 | ||
| with: | ||
| codecov_yml_path: ./codecov.yml | ||
| token: ${{ secrets.CODECOV_TOKEN }} |
There was a problem hiding this comment.
awesome, I have touched base in the pass with the Hyperledger team to setup codecov! Can reach out to set it up for this repo
zkp/golang/pkg/core/storage.go
Outdated
| Type byte | ||
| Index *string // only leaf nodes have an index | ||
| LeftChild *string // only branch nodes have children | ||
| RightChild *string |
There was a problem hiding this comment.
do you need to annotate this with gorm of it will default to VARCHAR for strings?
There was a problem hiding this comment.
good question, they are 64 character hex strings, is there a better SQL data type to use than VARCHAR?
There was a problem hiding this comment.
What is default length that GROM generated for VARCHAR? For example SQLITE VARCHAR is unbounded but PSQL is bounded
There was a problem hiding this comment.
looks like there's a way to specify a size in the gorm annotation. so added that to the structs for both tables
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
No description provided.