Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions block/internal/executing/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

Using context.Background() here ensures the Sync operation completes even during a graceful shutdown, which is important for data integrity. However, this comes with a trade-off: if Sync were 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

how much does this cost in terms of overhead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
But the question is more safety vs speed. Without sync, there is a risk for double signing after kill/restart.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

@alpe alpe Nov 21, 2025

Choose a reason for hiding this comment

The 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. 👍
If the numbers are not acceptable for the use case, we could find some trade off to balance safety for speed with async call to sync (😄 ) or a sync every n blocks for example.

return fmt.Errorf("failed to sync store: %w", err)
}

// Update in-memory state after successful commit
e.setLastState(newState)
Expand Down
70 changes: 70 additions & 0 deletions pkg/store/benchmark_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
package store

import (
"fmt"
"testing"

"github.com/stretchr/testify/require"

"github.com/evstack/ev-node/types"
)

func BenchmarkStoreSync(b *testing.B) {
chainID := "BenchmarkStoreSync"
testCases := []struct {
name string
blockSize int
}{
{"empty", 0},
{"small", 10},
{"large", 1_000},
}

for _, tc := range testCases {
header, data := types.GetRandomBlock(1, tc.blockSize, chainID)
signature := &types.Signature{}

b.Run(fmt.Sprintf("WithoutSync_%s", tc.name), func(b *testing.B) {
tmpDir := b.TempDir()
kv, err := NewDefaultKVStore(tmpDir, "db", "test")
require.NoError(b, err)
store := New(kv)
b.Cleanup(func() { _ = store.Close() })

b.ResetTimer()
for i := 0; i < b.N; i++ {
batch, err := store.NewBatch(b.Context())
require.NoError(b, err)

err = batch.SaveBlockData(header, data, signature)
require.NoError(b, err)

err = batch.Commit()
require.NoError(b, err)
}
})

b.Run(fmt.Sprintf("WithSync_%s", tc.name), func(b *testing.B) {
tmpDir := b.TempDir()
kv, err := NewDefaultKVStore(tmpDir, "db", "test")
require.NoError(b, err)
store := New(kv)
b.Cleanup(func() { _ = store.Close() })

b.ResetTimer()
for i := 0; i < b.N; i++ {
batch, err := store.NewBatch(b.Context())
require.NoError(b, err)

err = batch.SaveBlockData(header, data, signature)
require.NoError(b, err)

err = batch.Commit()
require.NoError(b, err)

err = store.Sync(b.Context())
require.NoError(b, err)
}
})
}
}
5 changes: 5 additions & 0 deletions pkg/store/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
Expand Down
3 changes: 3 additions & 0 deletions pkg/store/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ type Store interface {

// NewBatch creates a new batch for atomic operations.
NewBatch(ctx context.Context) (Batch, error)

// Sync flushes the store state to disk
Sync(ctx context.Context) error
}

type Reader interface {
Expand Down
51 changes: 51 additions & 0 deletions test/mocks/store.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading