Skip to content

Expose merkledb defaults #3748

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 25, 2025
Merged

Expose merkledb defaults #3748

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 15 additions & 0 deletions x/merkledb/db.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,21 @@ type MerkleDB interface {
Prefetcher
}

func NewConfig() Config {
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels a bit odd to default to populating mockMetrics - I'm wondering if the newDefaultConfig needs to exist at all or if we should just use this. (and populate the metrics reg here)..

Although that feels a bit odd as well... so 🤷 maybe the issue is with the mockMetrics as a whole...

It feels like whoever made mockMetrics didn't know that "github.com/prometheus/client_golang/prometheus/testutil" existed.

return Config{
BranchFactor: BranchFactor16,
Hasher: DefaultHasher,
RootGenConcurrency: 0,
HistoryLength: 300,
ValueNodeCacheSize: units.MiB,
IntermediateNodeCacheSize: units.MiB,
IntermediateWriteBufferSize: units.KiB,
IntermediateWriteBatchSize: 256 * units.KiB,
TraceLevel: InfoTrace,
Tracer: trace.Noop,
}
}

type Config struct {
// BranchFactor determines the number of children each node can have.
BranchFactor BranchFactor
Expand Down
46 changes: 13 additions & 33 deletions x/merkledb/db_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,22 +14,18 @@ import (
"testing"
"time"

"github.com/prometheus/client_golang/prometheus"
"github.com/stretchr/testify/require"

"github.com/ava-labs/avalanchego/database"
"github.com/ava-labs/avalanchego/database/dbtest"
"github.com/ava-labs/avalanchego/database/memdb"
"github.com/ava-labs/avalanchego/ids"
"github.com/ava-labs/avalanchego/trace"
"github.com/ava-labs/avalanchego/utils/hashing"
"github.com/ava-labs/avalanchego/utils/maybe"
"github.com/ava-labs/avalanchego/utils/set"
"github.com/ava-labs/avalanchego/utils/units"
)

const defaultHistoryLength = 300

// newDB returns a new merkle database with the underlying type so that tests can access unexported fields
func newDB(ctx context.Context, db database.Database, config Config) (*merkleDB, error) {
db, err := New(ctx, db, config)
Expand All @@ -39,22 +35,6 @@ func newDB(ctx context.Context, db database.Database, config Config) (*merkleDB,
return db.(*merkleDB), nil
}

func newDefaultConfig() Config {
return Config{
BranchFactor: BranchFactor16,
Hasher: DefaultHasher,
RootGenConcurrency: 0,
HistoryLength: defaultHistoryLength,
ValueNodeCacheSize: units.MiB,
IntermediateNodeCacheSize: units.MiB,
IntermediateWriteBufferSize: units.KiB,
IntermediateWriteBatchSize: 256 * units.KiB,
Reg: prometheus.NewRegistry(),
TraceLevel: InfoTrace,
Tracer: trace.Noop,
}
}

func Test_MerkleDB_Get_Safety(t *testing.T) {
require := require.New(t)

Expand Down Expand Up @@ -134,7 +114,7 @@ func Test_MerkleDB_DB_Load_Root_From_DB(t *testing.T) {
db, err := New(
context.Background(),
baseDB,
newDefaultConfig(),
NewConfig(),
)
require.NoError(err)

Expand Down Expand Up @@ -162,7 +142,7 @@ func Test_MerkleDB_DB_Load_Root_From_DB(t *testing.T) {
db, err = New(
context.Background(),
baseDB,
newDefaultConfig(),
NewConfig(),
)
require.NoError(err)

Expand All @@ -176,7 +156,7 @@ func Test_MerkleDB_DB_Rebuild(t *testing.T) {

initialSize := 5_000

config := newDefaultConfig()
config := NewConfig()
config.ValueNodeCacheSize = uint(initialSize)
config.IntermediateNodeCacheSize = uint(initialSize)

Expand Down Expand Up @@ -233,7 +213,7 @@ func Test_MerkleDB_Failed_Batch_Commit(t *testing.T) {
db, err := New(
context.Background(),
memDB,
newDefaultConfig(),
NewConfig(),
)
require.NoError(err)

Expand All @@ -254,7 +234,7 @@ func Test_MerkleDB_Value_Cache(t *testing.T) {
db, err := New(
context.Background(),
memDB,
newDefaultConfig(),
NewConfig(),
)
require.NoError(err)

Expand Down Expand Up @@ -903,13 +883,13 @@ const (
)

func runRandDBTest(require *require.Assertions, r *rand.Rand, rt randTest, tokenSize int) {
db, err := getBasicDBWithBranchFactor(tokenSizeToBranchFactor[tokenSize])
config := NewConfig()
config.BranchFactor = tokenSizeToBranchFactor[tokenSize]
db, err := New(context.Background(), memdb.New(), config)
require.NoError(err)

const (
maxProofLen = 100
maxPastRoots = defaultHistoryLength
)
maxProofLen := 100
Copy link
Contributor

Choose a reason for hiding this comment

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

(No action required) Are you sure you wouldn't rather satisfy the gods of var()? ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know when we're supposed to use var blocks... it seems to make sense for package-level vars, when you want a short-hand for the zero value, or where you want to choose the type of something, but when those criteria aren't met I usually don't use them (and for zero values I usually explicitly assign to the zero value...).

maxPastRoots := int(config.HistoryLength)

var (
values = make(map[Key][]byte) // tracks content of the trie
Expand Down Expand Up @@ -966,7 +946,7 @@ func runRandDBTest(require *require.Assertions, r *rand.Rand, rt randTest, token
end,
root,
tokenSize,
db.hasher,
config.Hasher,
))
case opGenerateChangeProof:
root, err := db.GetMerkleRoot(context.Background())
Expand Down Expand Up @@ -1301,7 +1281,7 @@ func TestCrashRecovery(t *testing.T) {
merkleDB, err := newDatabase(
context.Background(),
baseDB,
newDefaultConfig(),
NewConfig(),
&mockMetrics{},
)
require.NoError(err)
Expand All @@ -1319,7 +1299,7 @@ func TestCrashRecovery(t *testing.T) {
newMerkleDB, err := newDatabase(
context.Background(),
baseDB,
newDefaultConfig(),
NewConfig(),
&mockMetrics{},
)
require.NoError(err)
Expand Down
9 changes: 4 additions & 5 deletions x/merkledb/helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,18 @@ func getBasicDB() (*merkleDB, error) {
return newDatabase(
context.Background(),
memdb.New(),
newDefaultConfig(),
NewConfig(),
&mockMetrics{},
)
}

func getBasicDBWithBranchFactor(bf BranchFactor) (*merkleDB, error) {
config := newDefaultConfig()
func getBasicDBWithBranchFactor(bf BranchFactor) (MerkleDB, error) {
config := NewConfig()
config.BranchFactor = bf
return newDatabase(
return New(
context.Background(),
memdb.New(),
config,
&mockMetrics{},
)
}

Expand Down
24 changes: 12 additions & 12 deletions x/merkledb/history_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ func Test_History_Simple(t *testing.T) {
db, err := newDB(
context.Background(),
memdb.New(),
newDefaultConfig(),
NewConfig(),
)
require.NoError(err)
batch := db.NewBatch()
Expand Down Expand Up @@ -87,7 +87,7 @@ func Test_History_Large(t *testing.T) {
numIters := 250

for i := 1; i < 5; i++ {
config := newDefaultConfig()
config := NewConfig()
// History must be large enough to get the change proof
// after this loop.
config.HistoryLength = uint(numIters)
Expand Down Expand Up @@ -149,7 +149,7 @@ func Test_History_Large(t *testing.T) {
func Test_History_Bad_GetValueChanges_Input(t *testing.T) {
require := require.New(t)

config := newDefaultConfig()
config := NewConfig()
config.HistoryLength = 5

db, err := newDB(
Expand Down Expand Up @@ -215,7 +215,7 @@ func Test_History_Bad_GetValueChanges_Input(t *testing.T) {
func Test_History_Trigger_History_Queue_Looping(t *testing.T) {
require := require.New(t)

config := newDefaultConfig()
config := NewConfig()
config.HistoryLength = 2

db, err := newDB(
Expand Down Expand Up @@ -274,7 +274,7 @@ func Test_History_Trigger_History_Queue_Looping(t *testing.T) {
func Test_History_Values_Lookup_Over_Queue_Break(t *testing.T) {
require := require.New(t)

config := newDefaultConfig()
config := NewConfig()
config.HistoryLength = 4
db, err := newDB(
context.Background(),
Expand Down Expand Up @@ -327,7 +327,7 @@ func Test_History_RepeatedRoot(t *testing.T) {
db, err := newDB(
context.Background(),
memdb.New(),
newDefaultConfig(),
NewConfig(),
)
require.NoError(err)
batch := db.NewBatch()
Expand Down Expand Up @@ -371,7 +371,7 @@ func Test_History_ExcessDeletes(t *testing.T) {
db, err := newDB(
context.Background(),
memdb.New(),
newDefaultConfig(),
NewConfig(),
)
require.NoError(err)
batch := db.NewBatch()
Expand Down Expand Up @@ -403,7 +403,7 @@ func Test_History_DontIncludeAllNodes(t *testing.T) {
db, err := newDB(
context.Background(),
memdb.New(),
newDefaultConfig(),
NewConfig(),
)
require.NoError(err)
batch := db.NewBatch()
Expand Down Expand Up @@ -431,7 +431,7 @@ func Test_History_Branching2Nodes(t *testing.T) {
db, err := newDB(
context.Background(),
memdb.New(),
newDefaultConfig(),
NewConfig(),
)
require.NoError(err)
batch := db.NewBatch()
Expand Down Expand Up @@ -459,7 +459,7 @@ func Test_History_Branching3Nodes(t *testing.T) {
db, err := newDB(
context.Background(),
memdb.New(),
newDefaultConfig(),
NewConfig(),
)
require.NoError(err)
batch := db.NewBatch()
Expand All @@ -484,7 +484,7 @@ func Test_History_Branching3Nodes(t *testing.T) {
func Test_History_MaxLength(t *testing.T) {
require := require.New(t)

config := newDefaultConfig()
config := NewConfig()
config.HistoryLength = 2
db, err := newDB(
context.Background(),
Expand Down Expand Up @@ -519,7 +519,7 @@ func Test_Change_List(t *testing.T) {
db, err := newDB(
context.Background(),
memdb.New(),
newDefaultConfig(),
NewConfig(),
)
require.NoError(err)

Expand Down
4 changes: 2 additions & 2 deletions x/merkledb/metrics_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (
)

func Test_Metrics_Basic_Usage(t *testing.T) {
config := newDefaultConfig()
config := NewConfig()
// Set to nil so that we use a mockMetrics instead of the real one inside
// merkledb.
config.Reg = nil
Expand Down Expand Up @@ -54,7 +54,7 @@ func Test_Metrics_Initialize(t *testing.T) {
db, err := New(
context.Background(),
memdb.New(),
newDefaultConfig(),
NewConfig(),
)
require.NoError(t, err)

Expand Down
28 changes: 18 additions & 10 deletions x/merkledb/proof_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/stretchr/testify/require"

"github.com/ava-labs/avalanchego/database"
"github.com/ava-labs/avalanchego/database/memdb"
"github.com/ava-labs/avalanchego/ids"
"github.com/ava-labs/avalanchego/utils/hashing"
"github.com/ava-labs/avalanchego/utils/maybe"
Expand Down Expand Up @@ -675,11 +676,17 @@ func Test_ChangeProof_Missing_History_For_EndRoot(t *testing.T) {
t.Logf("Seed: %d", seed)
rand := rand.New(rand.NewSource(seed)) // #nosec G404

db, err := getBasicDB()
config := NewConfig()
db, err := newDatabase(
context.Background(),
memdb.New(),
config,
&mockMetrics{},
)
require.NoError(err)

roots := []ids.ID{}
for i := 0; i < defaultHistoryLength+1; i++ {
for i := 0; i < int(config.HistoryLength)+1; i++ {
key := make([]byte, 16)
_, _ = rand.Read(key)
require.NoError(db.Put(key, nil))
Expand Down Expand Up @@ -1879,11 +1886,6 @@ func FuzzProofVerification(f *testing.F) {

// Generate change proofs and verify that they are valid.
func FuzzChangeProofVerification(f *testing.F) {
const (
numKeyValues = defaultHistoryLength / 2
deletePortion = 0.25
)

f.Fuzz(func(
t *testing.T,
startBytes []byte,
Expand All @@ -1894,7 +1896,13 @@ func FuzzChangeProofVerification(f *testing.F) {
require := require.New(t)
rand := rand.New(rand.NewSource(randSeed)) // #nosec G404

db, err := getBasicDB()
config := NewConfig()
db, err := newDatabase(
context.Background(),
memdb.New(),
config,
&mockMetrics{},
)
require.NoError(err)

startRootID, err := db.GetMerkleRoot(context.Background())
Expand All @@ -1906,8 +1914,8 @@ func FuzzChangeProofVerification(f *testing.F) {
require,
rand,
[]database.Database{db},
numKeyValues,
deletePortion,
config.HistoryLength/2,
0.25,
Copy link
Contributor

Choose a reason for hiding this comment

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

(No action required) Maybe continue to use a named var for this magic number so that it can be self-documenting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just moved it into the function signature because the function signature self-documents what these variables mean

)

endRootID, err := db.GetMerkleRoot(context.Background())
Expand Down
2 changes: 1 addition & 1 deletion x/merkledb/trie_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -917,7 +917,7 @@ func Test_Trie_MultipleStates(t *testing.T) {
db, err := New(
context.Background(),
rdb,
newDefaultConfig(),
NewConfig(),
)
require.NoError(err)
defer db.Close()
Expand Down
2 changes: 1 addition & 1 deletion x/merkledb/view_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ var hashChangedNodesTests = []struct {
}

func makeViewForHashChangedNodes(t require.TestingT, numKeys uint64, parallelism uint) *view {
config := newDefaultConfig()
config := NewConfig()
config.RootGenConcurrency = parallelism
db, err := newDatabase(
context.Background(),
Expand Down
Loading