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

Cleanup usage of SyncManager and Settings across codebase. #680

Merged
merged 2 commits into from
Jan 23, 2024

Conversation

piotrm50
Copy link
Collaborator

@piotrm50 piotrm50 commented Jan 18, 2024

I changes Settings.LatestCommitment and Settings.LastFinalizedSlot usages that require taking the values from the underlying KVStore, to use SyncManager which stores the value in memory.
Values from Settings are still used during component initialization to avoid potential problems due to order in which components are initialized and in some low-level Storage logic.

Copy link
Contributor

@cyberphysic4l cyberphysic4l left a comment

Choose a reason for hiding this comment

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

Would it also be worth considering an in-memory latestCommitment for the notarization manager? storage.Settings() is accessed very frequently there.

Apart from that consideration, it all looks good to me

Copy link
Contributor

@jonastheis jonastheis left a comment

Choose a reason for hiding this comment

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

I think this PR is fine for now.

However, we should re-think Settings and the SyncManager altogether imo.
Currently, Settings is a weird mix of the state of an engine and its configuration. This becomes quite clear when looking at its Export method. Where we only export

  • latest commitment
  • latest finalized slot (is this even good? as it might lead to different snapshots (when comparing directly or just hashes) as it incorporates a subjective measure. maybe it should be the finalized slot at the export slot or something)
  • protocol versions
  • future protocol parameters
  • protocol parameters

All the rest is more of a state.

As for the SyncManager this is more like a state of the engine. In a future PR we should probably move all state related properties to the SyncManager, maybe consider renaming it and implement a reactive wrapper around kvstore.TypedValue for convenience access and subscription to changes.

@jonastheis
Copy link
Contributor

Would it also be worth considering an in-memory latestCommitment for the notarization manager? storage.Settings() is accessed very frequently there.

It is used in a few different places across the code, however to me it looks like it's only executed when the node is not bootstrapped yet or when we're committing (which happens only every 10s).

Looking a bit closer at the settings we can see that the latestCommitment is stored as *kvstore.TypedValue[*model.Commitment] which actually manages an in-memory version abstracted away.

@piotrm50 piotrm50 merged commit 85d47f0 into develop Jan 23, 2024
4 checks passed
@piotrm50 piotrm50 deleted the feat/cleanup-syncmanager-settings-usage branch January 23, 2024 11:16
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.

Reassess the use of LatestCommitment from Settings or SyncManager over all components
3 participants