Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revive sync #70

Merged
merged 10 commits into from
Nov 11, 2020
Merged

Revive sync #70

merged 10 commits into from
Nov 11, 2020

Conversation

vaibhavchellani
Copy link
Contributor

Fixes #31

@vaibhavchellani
Copy link
Contributor Author

vaibhavchellani commented Nov 10, 2020

State validation during sync is now complete, opened a new issue for signature validation: #74

@vaibhavchellani vaibhavchellani changed the title WIP: Revive sync Revive sync Nov 10, 2020
Copy link
Collaborator

@ChihChengLiang ChihChengLiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! I left comments on minor issues to address, looks good to me in general!

core/batch.go Outdated
return err
}

if batch.Status != BATCH_BROADCASTED && batch.Status == BATCH_COMMITTED {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can safely remove batch.Status != BATCH_BROADCASTED.

  • When batch.Status != BATCH_COMMITTED the whole condition evaluates false
  • when batch.Status == BATCH_COMMITTED, batch.Status != BATCH_BROADCASTED evaluates true, so the whole condition evaluates true too.
Suggested change
if batch.Status != BATCH_BROADCASTED && batch.Status == BATCH_COMMITTED {
if batch.Status == BATCH_COMMITTED {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think your suggestion assumes that the status can be either BATCH_BROADCASTED or BATCH_COMMITTED while I was trying to protect from invalid status codes too, which I think is stupid🤐

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing

newRoot, err = bz.ProcessTx(currentRoot, *tx, fromStateProof, toStateProof)
if err != nil {
if txDBConn.Instance != nil {
txDBConn.Instance.Rollback()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is rollbacked here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If processTx is invalid we can rollback the changes made while witness creation

return
}
if txDBConn.Instance != nil {
txDBConn.Instance.Commit()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What insert or update is committed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are updates performed in GetTxVerificationData() via a MySQL transaction, we can rollback those updates if processTx is invalid.

Comment on lines +320 to +321
if isSyncing && err == ErrSignatureNotPresent {
continue
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the continue here mean the line commitments = append(commitments, commitment) is skipped?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it goes there, it simply means that if we are syncing and tx.Sig is nil we don't have to bubble up error as that is expected. I do think this piece is smelly and should be refactored in the future.

if err != nil {
return
}
// func (b *Bazooka) applyMassMigrationTx(sender, receiver []byte, tx Tx) (updatedSender, updatedReceiver []byte, err error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: For these areas of commented off code, it's fine to just remove them.

@vaibhavchellani vaibhavchellani merged commit 5a9dc97 into master Nov 11, 2020
@vaibhavchellani vaibhavchellani deleted the add-sync branch November 11, 2020 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add batch processors
2 participants