-
Notifications
You must be signed in to change notification settings - Fork 240
chore: sync store on new block #2869
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -413,6 +413,9 @@ func (e *Executor) produceBlock() error { | |
| if err := batch.Commit(); err != nil { | ||
| return fmt.Errorf("failed to commit batch: %w", err) | ||
| } | ||
| if err := e.store.Sync(context.Background()); err != nil { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how much does this cost in terms of overhead?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a fair question. I have not benchmarked this as I can only provide random data which may be very different to what happens on a chain.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should benchmark this. if on restart we are losing the previous block and reproposing a new block then i think its an issue in how we handle the flow, taking a performance hit like this is something to be careful about. Currently we are using the badgerdb system with its WAL and memory store before this change.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have added a benchmark and updated the description with some results. You were right to ask for the benchmark. 👍 |
||
| return fmt.Errorf("failed to sync store: %w", err) | ||
| } | ||
|
|
||
| // Update in-memory state after successful commit | ||
| e.setLastState(newState) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -190,6 +190,11 @@ func (s *DefaultStore) GetMetadata(ctx context.Context, key string) ([]byte, err | |
| return data, nil | ||
| } | ||
|
|
||
| // Sync flushes the store state to disk | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be abstracted within Commit? Keeping this method out of the interface, and just have Commit handle saving.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good discussion point. I don't mind the additional method on the interface but depending on the internal implementation less data piles up at the block production. |
||
| func (s *DefaultStore) Sync(ctx context.Context) error { | ||
| return s.db.Sync(ctx, ds.NewKey("/")) | ||
| } | ||
|
|
||
| // Rollback rolls back block data until the given height from the store. | ||
| // When aggregator is true, it will check the latest data included height and prevent rollback further than that. | ||
| // NOTE: this function does not rollback metadata. Those should be handled separately if required. | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using
context.Background()here ensures theSyncoperation completes even during a graceful shutdown, which is important for data integrity. However, this comes with a trade-off: ifSyncwere to hang, it would block the shutdown process indefinitely because it's not cancellable. While this might be an acceptable risk, a more robust solution could involve using a context with a timeout for this critical operation. This would prevent indefinite hangs while still giving the sync operation a grace period to complete during shutdown.