feat(db): add pebbledb support to 'improvements' branch

[anatolie-ssv]

Put pebbledb support on top of v2.2.1-unstable.2 changeset

deployed to stage exporter-2: https://grafana.ops.ssvlabsinternal.com/goto/VxWc2GJHR?orgId=1

deployed to:

• ssv-node-45-46(badger): https://grafana.ops.ssvlabsinternal.com/goto/lXbJkM1Ng?orgId=1
• ssv-node-47-48(pebble): https://grafana.ops.ssvlabsinternal.com/goto/gqS-kM1HR?orgId=1

[codecov]

Codecov Report

Attention: Patch coverage is 67.10963% with 99 lines in your changes missing coverage. Please review.

Project coverage is 49.9%. Comparing base (ab48181) to head (db508dd).
Report is 9 commits behind head on fix/exporter-improv.

Files with missing lines	Patch %	Lines
cli/operator/node.go	0.0%	47 Missing ⚠️
storage/pebble/pebble.go	76.7%	20 Missing and 10 partials ⚠️
storage/pebble/common.go	80.8%	9 Missing and 4 partials ⚠️
storage/pebble/txn.go	84.2%	6 Missing and 3 partials ⚠️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

[moshe-blox]

is it possible to separate the implementations to two different packages?

• rename storage/kv to storage/badger
• move pebble code to storage/pebble

this is a common practice in Go: one package for the interfaces, and one package for each implementation

also this should leave storage/kv package untouched (just renamed) which is easier to review

[moshe-blox]

lets pls hide this from examples & docs until we test it properly with operator nodes (in the future)

for now its for exporter only and we dont want operators to play with it yet :)

[y0sher]

shouldn't we make pebble work only with exporter for now then?

[anatolie-ssv]

removed

[y0sher]

MO this is not fully resolved, if we really never want to see a case where someone 'accidentally' uses pebble for an operator node, we should restrict it and not just not mention it.
wdyt? @anatolie-ssv @moshe-blox @MatusKysel

[iurii-ssv]

MO this is not fully resolved, if we really never want to see a case where someone 'accidentally' uses pebble for an operator node, we should restrict it and not just not mention it.

just as counter-argument:

• typically using "undocumented" feature is considered "at your own risk" (and it's gonna be an intentional usage because it's impossible to set pebble here instead of badger "by accident")
• does anything prevent operator from forking the code-base and removing any constraining check we might add if they really want to try it out ?

[iurii-ssv]

Some suggestions

[iurii-ssv]

What about this TODO, could you expand it a bit if we are gonna merge it (I can only really guess what it means) ?

[anatolie-ssv]

this TODO (and all the following comments and TODOs) are from the original PR from @y0sher and I'm afraid I don't know their context; I left them intact so we could tackle them when Yosher gets around to review this PR; @y0sher please, what did you mean by 'reconsider prefixes' here? is is safe to be removed or does it require any action?

[iurii-ssv]

This looks a bit unnecessarily hard to read to me, can't we refactor it into something like this (by rearranging some code a bit) ?

func (pdb *PebbleDB) SetMany(prefix []byte, n int, next func(int) (basedb.Obj, error)) error {
	txn := pdb.NewTxn()
	if err := txn.SetMany(prefix, n, next); err != nil {
		return err
	}
	return txn.Commit()
}

I guess this applies to other methods as well.

[iurii-ssv]

It seems here in GetMany we propagate error up the stack from closer.Close() while in Get we skip such an error ... shouldn't we handle both cases the same way (probably propagate errors) ?

[iurii-ssv]

I think this comment:

• either should have more info in it (on what it's about)
• or shouldn't be here since it is an implementation detail this code doesn't care about

note, there are 2 places with this comment

[moshe-blox]

i think we should return an error if this failed to close (it might do more than just closed in some cases)

[anatolie-ssv]

the errors returned from the .Close() method are the same as the errors returned from the iter.Error() at the end of the function, so we're not ignoring them.

[iurii-ssv]

the errors returned from the .Close() method are the same as the errors returned from the iter.Error() at the end of the function, so we're not ignoring them.

It might be better to not rely on implementation details like that (even if they "promise in a comment" that's how it works - they might change implementation and we won't even know about it)

but also it seems we could always use .Close() (and handle error returned) every time instead of using iter.Error() (do we ever need iter.Error() specifically) ?

[anatolie-ssv]

the only reason I defer the call to Close() is because - if a panic happens in the callback function - the defer statement will be executed regardless, but if we return Close() instead of iter.Error() that would lead to a leak.

[iurii-ssv]

We could keep using defer if we want and handle error from Close in there ...

but I wouldn't do that - it's a bit bulky and IMO unnecessary since panic in callback function would mean we have a bug in the code that needs to be addressed ASAP, I imagine not calling Close in this case is of smaller importance

[anatolie-ssv]

I think the conclusion of this article got stuck with me too much and that's why I went by the route of deferring Close statements here.

[iurii-ssv]

Yeah defer is a very nice thing to have/use - I don't mind it,

although here it seems to me panic is unlikely to happen without us noticing it during testing of the code involved

[moshe-blox]

@CodiumAI-Agent /review

[Copilot]

Pull Request Overview

This PR adds support for PebbleDB alongside Badger by updating the database initialization and configuration. The changes include updating import references in tests, modifying CLI node startup to select the database engine based on configuration, and updating error wrap messages for consistency.

Reviewed Changes

Copilot reviewed 37 out of 37 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
protocol/v2/qbft/testing/storage.go	Updated storage import from kv to badger in testing.
operator/validator/controller_test.go	Updated storage import from kv to badger in tests.
operator/storage/storage_test.go	Updated storage import from kv to badger in tests.
operator/fee_recipient/controller_test.go	Updated storage import from kv to badger in tests.
network/topics/msg_validator_test.go	Updated storage import from kv to badger in tests.
network/topics/controller_test.go	Updated storage import from kv to badger in tests.
network/p2p/test_utils.go	Updated storage import from kv to badger in tests.
migrations/migrations_test.go	Updated storage import from kv to badger in tests.
message/validation/validation_test.go	Updated storage import from kv to badger in tests.
identity/store_test.go	Updated storage import from kv to badger in tests.
ibft/storage/store_test.go	Updated storage import from kv to badger in tests.
exporter/api/query_handlers_test.go	Updated storage import from kv to badger in tests.
eth/eventsyncer/event_syncer_test.go	Updated storage import from kv to badger in tests.
eth/eventhandler/validation_test.go	Updated storage import from kv to badger in tests.
eth/eventhandler/event_handler_test.go	Updated storage import from kv to badger in tests.
eth/ethtest/utils_test.go	Updated storage import from kv to badger in tests.
e2e/cmd/ssv-e2e/share_update.go	Updated storage import from kv to badger in tests.
config/config.exporter.example.yaml	Updated config to set Engine to pebble.
cli/operator/node_test.go	Updated storage import from kv to badger in tests.
cli/operator/node.go	Added engine switch support for PebbleDB and BadgerDB.

Comments suppressed due to low confidence (2)

cli/operator/node.go:145

• Consider adding tests that validate both 'pebble' and 'badger' database initializations to ensure the engine switching works as expected under different configurations.

switch cfg.DBOptions.Engine {

cli/operator/node.go:773

• [nitpick] Consider documenting the rationale for appending '-pebble' to the database path or providing a configuration option for the suffix to improve clarity and maintainability.

dbPath := cfg.DBOptions.Path + "-pebble" // opinionated approach to avoid corrupting old db location

[moshe-blox]

@CodiumAI-Agent /describe

[CodiumAI-Agent]

Title

feat(db): add pebbledb support

---

User description

Put pebbledb support on top of v2.2.1-unstable.2 changeset

deployed to stage exporter-2: https://grafana.ops.ssvlabsinternal.com/goto/VxWc2GJHR?orgId=1

deployed to:

• ssv-node-45-46(badger): https://grafana.ops.ssvlabsinternal.com/goto/lXbJkM1Ng?orgId=1
• ssv-node-47-48(pebble): https://grafana.ops.ssvlabsinternal.com/goto/gqS-kM1HR?orgId=1

---

PR Type

Enhancement, Tests

---

Description

• Add configurable DB engine switch (badger/pebble)

• Implement setupPebbleDB and full GC post-migrations

• Introduce new storage/pebble package implementation

• Update imports and add comprehensive pebble tests

---

Changes walkthrough 📝

	Relevant files
Enhancement	4 files node.go Add DB engine switch and setup functions                                  +59/-9    common.go Add pebble getters and iteration helpers                                  +103/-0  pebble.go Implement PebbleDB storage backend                                              +202/-0  txn.go Implement pebble transaction wrapper                                          +109/-0
Configuration changes	2 files storage.go Add Engine option to DB config                                                      +1/-0      config.exporter.example.yaml Add Engine field defaulting to pebble                                        +1/-0
Refactoring	4 files badger.go Rename package from kv to badger                                                  +1/-1      gc.go Rename garbage collection package file                                      +1/-1      logger.go Rename logger package file                                                              +1/-1      txn.go Rename txn package file                                                                    +1/-1
Tests	10 files node_test.go Update test import to badger backend                                          +1/-1      pebble_test.go Add tests for PebbleDB implementation                                        +434/-0  txn_test.go Add tests for PebbleDB transactions                                            +622/-0  migrations_test.go Update migration tests to use badger import                            +1/-1      storage_test.go Update storage tests to badger import                                        +1/-1      share_update.go Update e2e share update to badger import                                  +1/-1      test_utils.go Update p2p test utils to badger import                                      +1/-1      controller_test.go Update topics controller tests for badger                                +1/-1      msg_validator_test.go Update msg validator tests to badger import                            +1/-1      store_test.go Update identity store tests to badger import                          +1/-1
Additional files	17 files utils_test.go +1/-1      event_handler_test.go +1/-1      validation_test.go +1/-1      event_syncer_test.go +1/-1      query_handlers_test.go +1/-1      store_test.go +1/-1      validation_test.go +1/-1      controller_test.go +1/-1      controller_test.go +1/-1      storage.go +1/-1      operators_test.go +1/-1      recipients_test.go +1/-1      shares_test.go +1/-1      badger_test.go +1/-1      gc_test.go +1/-1      logger_test.go +1/-1      txn_test.go +1/-1

---

Need help?

Type /help how to ... in the comments thread for any questions about PR-Agent usage.

Check out the documentation for more information.

[moshe-blox]

@CodiumAI-Agent /review

[CodiumAI-Agent]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 5 🔵🔵🔵🔵🔵

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Missing Import New code references fmt.Errorf and context.WithTimeout but neither fmt nor context is imported, causing build failures. cfg.DBOptions.Ctx = cmd.Context() var db basedb.Database switch cfg.DBOptions.Engine { case "pebble": logger.Info("using pebble db") db, err = setupPebbleDB(logger, networkConfig) case "badger": logger.Info("using badger db") db, err = setupBadgerDB(logger, networkConfig) default: err = fmt.Errorf("invalid db engine: %s", cfg.DBOptions.Engine) Alias Naming The import alias cockroachdb for the Pebble package is misleading; consider renaming it to better reflect its purpose. cockroachdb "github.com/cockroachdb/pebble"

[moshe-blox]

@CodiumAI-Agent /improve

[CodiumAI-Agent]

PR Code Suggestions ✨

Latest suggestions up to db508dd

Category	Suggestion	Impact
Possible issue	Use zap.Duration for elapsed time fields.Duration is undefined and likely meant to be a Zap field for duration. Replace it with zap.Duration and compute the elapsed time with time.Since. cli/operator/node.go [795] -logger.Debug("post-migrations garbage collection completed", fields.Duration(start)) +logger.Debug( + "post-migrations garbage collection completed", + zap.Duration("duration", time.Since(start)), +) Suggestion importance[1-10]: 9 __ Why: The call to fields.Duration(start) references an undefined fields package and should use zap.Duration("duration", time.Since(start)) to correctly log elapsed time.	High
	Fix SetMany loop syntax Ranging over an integer is invalid Go syntax. Use a traditional for‐loop to iterate from 0 to n-1. storage/pebble/txn.go [36-48] func (t *pebbleTxn) SetMany(prefix []byte, n int, next func(int) (basedb.Obj, error)) error { - for i := range n { + for i := 0; i < n; i++ { item, err := next(i) if err != nil { return err } key := append(prefix, item.Key...) - err = t.Batch.Set(key, item.Value, nil) - if err != nil { + if err := t.Batch.Set(key, item.Value, nil); err != nil { return err } } return nil } Suggestion importance[1-10]: 9 __ Why: The loop for i := range n is invalid Go syntax for an integer and must be replaced with for i := 0; i < n; i++ to iterate correctly.	High

---

Previous suggestions

Suggestions up to commit 452e686

Category	Suggestion	Impact
Possible issue	Fix loop to iterate with index The for i := range n loop is invalid since n is an integer; use a C-style for loop for i := 0; i < n; i++ to iterate the correct number of times. storage/pebble/txn.go [32-43] func (t *pebbleTxn) SetMany(prefix []byte, n int, next func(int) (basedb.Obj, error)) error { - for i := range n { + for i := 0; i < n; i++ { item, err := next(i) if err != nil { return err } key := append(prefix, item.Key...) - err = t.Batch.Set(key, item.Value, nil) - if err != nil { + if err := t.Batch.Set(key, item.Value, nil); err != nil { return err } } return nil } Suggestion importance[1-10]: 9 __ Why: Changing for i := range n to for i := 0; i < n; i++ corrects a compilation error and enables SetMany to function as intended.	High
General	Default nil options before opening Guard against a nil opts parameter by defaulting it to an empty &pebble.Options{} before calling pebble.Open, so that tests passing nil won't panic. storage/pebble/pebble.go [22-31] func NewPebbleDB(logger *zap.Logger, path string, opts *pebble.Options) (*PebbleDB, error) { pbdb := &PebbleDB{ logger: logger, + } + if opts == nil { + opts = &pebble.Options{} } pdb, err := pebble.Open(path, opts) if err != nil { return nil, err } pbdb.DB = pdb return pbdb, nil } Suggestion importance[1-10]: 6 __ Why: Guarding opts against nil prevents potential panics when opening the DB with no options, and aligns with usage in tests without impacting existing callers.	Low
	Use errors.Errorf instead of fmt.Errorf Use the existing github.com/pkg/errors package to format this error instead of fmt.Errorf, avoiding a new import and keeping error handling consistent. cli/operator/node.go [153] -err = fmt.Errorf("invalid db engine: %s", cfg.DBOptions.Engine) +err = errors.Errorf("invalid db engine: %s", cfg.DBOptions.Engine) Suggestion importance[1-10]: 5 __ Why: Replacing the fmt.Errorf with errors.Errorf leverages the existing github.com/pkg/errors import and keeps error handling consistent without introducing new imports.	Low