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

Streamable Snapshot #2358

Merged
merged 57 commits into from
Aug 3, 2022
Merged

Streamable Snapshot #2358

merged 57 commits into from
Aug 3, 2022

Conversation

jkrvivian
Copy link
Contributor

@jkrvivian jkrvivian commented Jul 22, 2022

Description of change

Implement #2288

Type of change

  • Breaking change

Change checklist

  • My code follows the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

packages/core/ledger/ledger.go Outdated Show resolved Hide resolved
packages/core/ledger/snapshot.go Outdated Show resolved Hide resolved
packages/core/ledger/ledger.go Outdated Show resolved Hide resolved
packages/core/snapshot/write.go Outdated Show resolved Hide resolved
packages/core/notarization/manager.go Outdated Show resolved Hide resolved
DiffEpochIndex: committableEC.EI(),
LatestECRecord: committableEC,
// CreateSnapshot creates a snapshot file to the given file path.
func CreateSnapshot(filePath string,
Copy link
Contributor

@karimodm karimodm Jul 28, 2022

Choose a reason for hiding this comment

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

Through the producers you are extracting data over a certain amount of time; as you are not locking the notarization manager in between these producer calls, it could be possible that your header will contain data that won't match your UTXOs or Diffs. Imagine for instance that a new epoch is confirmed in between asking for the latest confirmed epoch and your epoch diffs: you will have unmatched header and snapshot contents!
I think you should make sure that the ledgerstate and the diffs of the notarization manager do not change after you determine what epochs you are going to dump into the snapshot: 1) by locking the ledgerstate so it cannot be updated in the meantime, 2) passing the epochs you want the diffs for to the producers calls.

plugins/webapi/snapshot/plugin.go Outdated Show resolved Hide resolved
Copy link
Contributor

@karimodm karimodm left a comment

Choose a reason for hiding this comment

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

Just change the ledgerstate**s** references to ledgerstate. It sounds weird =)

packages/core/notarization/commitments.go Outdated Show resolved Hide resolved
return stringify.Struct("Snapshot",
stringify.StructField("LedgerSnapshot", s.LedgerSnapshot),
)
return
}

func (s *Snapshot) updateConsensusManaDetails(nodeSnapshot *mana.SnapshotNode, output devnetvm.Output, outputMetadata *ledger.OutputMetadata) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this function used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, nope, I'll remove it.

@karimodm karimodm changed the title feat: Streamable Snapshot Streamable Snapshot Aug 3, 2022
@karimodm karimodm merged commit c58fdaf into develop Aug 3, 2022
@karimodm karimodm deleted the feat/streamable-snapshot branch August 3, 2022 09:38
@karimodm karimodm mentioned this pull request Aug 8, 2022
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.

3 participants