Make merkle tree leaf addition operations atomic #71
Conversation
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
| // the node's reference key (not the index) is used as the key to | ||
| // store the node in the DB | ||
| n := core.SMTNode{ | ||
| RefKey: ref.Hex(), | ||
| } | ||
| err := s.p.DB().Table(s.nodesTableName).First(&n).Error | ||
| err := batchOrDb.Table(nodesTableName).First(&n).Error |
There was a problem hiding this comment.
I'm struggling why a read operation would need a batch transaction
There was a problem hiding this comment.
here we want the query to hit the records that have been added in the batch, but have not been committed.
There was a problem hiding this comment.
but if you are looking relying on getNode before inserting then you need to make sure to use the DB directly instead of batch
There was a problem hiding this comment.
I feel batchOrDb should be dbTx here. this layer shouldn't care about that difference.
| if _, err := batch.GetNode(k); err == nil { | ||
| return nil, ErrNodeIndexAlreadyExists | ||
| } | ||
| err := mt.db.InsertNode(n) | ||
| err := batch.InsertNode(n) |
There was a problem hiding this comment.
This is where I don't understand the batch function where you get a node but the TX is not committed yet and then you are adding an insert to the same transaction
There was a problem hiding this comment.
for example here, you would use db instead of batch for GetNode?
There was a problem hiding this comment.
After a conversation with @jimthematrix , we can move to ON CONFLICT insert and if there is on conflict do nothing and rework the way the read works
Signed-off-by: Jim Zhang <[email protected]>
|
@EnriqueL8 thanks for catching that. just added a commit to make the inserts `ON CONFLICT DO NOTHING" with a justification in the inline comments |
There was a problem hiding this comment.
@jimthematrix core logic looks good to me
I think we should address the following before the merge:
- test temp db file cleanup check
- log rollback failures
I also made a suggestion for readability, which is to consider renaming batch to highlight it's a DB tx.
go-sdk/integration-test/db_test.go
Outdated
| } | ||
|
|
||
| func (s *SqliteTestSuite) TearDownTest() { | ||
| os.Remove(s.dbfile.Name()) |
There was a problem hiding this comment.
error check is regressed, this will cause removal failure due to the wrong file name not being captured.
| batch, err := mt.db.BeginBatch() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| newRootKey, err := mt.addLeaf(batch, node, mt.rootKey, 0, path) | ||
| if err != nil { | ||
| log.L().Errorf("Error adding leaf node %s: %v, rolling back", node.Ref().Hex(), err) | ||
| _ = batch.Rollback() | ||
| return err | ||
| } | ||
| mt.rootKey = newRootKey | ||
|
|
||
| // update the root node index in the storage | ||
| err = mt.db.UpsertRootNodeIndex(mt.rootKey) | ||
| log.L().Infof("Upserting root node index to %s", mt.rootKey.Hex()) | ||
| err = batch.UpsertRootNodeIndex(mt.rootKey) | ||
| if err != nil { | ||
| log.L().Errorf("Error upserting root node %s: %v, rolling back", mt.rootKey.Hex(), err) | ||
| _ = batch.Rollback() | ||
| return err | ||
| } | ||
| log.L().Infof("Committing batch operations for adding leaf node %s", node.Ref().Hex()) | ||
| err = batch.Commit() | ||
| if err != nil { | ||
| log.L().Errorf("Error committing batch operations for adding leaf node %s: %v", node.Ref().Hex(), err) | ||
| _ = batch.Rollback() |
There was a problem hiding this comment.
This is the core logic implements Make merkle tree leaf addition operations atomic. Some readability suggestions. Here are the steps as per my understanding:
- A single DB tx is created to ensure atomicity of multiple DB txs
- Add a new leaf node as part of the started DB TX
- Upsert MT root in the same DB TX
- Commit only if both queries succeeded, otherwise rollback.
So I think it would improve the readability if batch is renamed to reflect it's a DB tx.
There was a problem hiding this comment.
Also, we should log Rollback errors as warnings so leakage of DB txs can be monitored.
go-sdk/integration-test/db_test.go
Outdated
| } | ||
|
|
||
| func (s *SqliteTestSuite) TearDownTest() { | ||
| os.Remove(s.dbfile.Name()) |
There was a problem hiding this comment.
| os.Remove(s.dbfile.Name()) | |
| if err := os.Remove(s.dbfile.Name()); err != nil { | |
| panic(err); | |
| } |
| } | ||
|
|
||
| func (s *MerkleTreeTestSuite) TearDownTest() { | ||
| os.Remove(s.dbfile.Name()) |
There was a problem hiding this comment.
| os.Remove(s.dbfile.Name()) | |
| if err := os.Remove(s.dbfile.Name()); err != nil { | |
| panic(err); | |
| } |
Co-authored-by: Chengxuan Xing <[email protected]> Signed-off-by: jimthematrix <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
|
@Chengxuan the remaining comments should be addressed. I think panic'ing on file remove failure in the test temp folder during test cleanup feels too heavy handed. but a test failure seems reasonable |
fixes #21