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.

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

2nd leveldb database instance removal #168

Merged
merged 2 commits into from
Mar 27, 2024
Merged

2nd leveldb database instance removal #168

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
51dea59
2nd database instance eliminated
oliverbundalo Mar 27, 2024
98bec50
Merge branch 'develop' into remove-multiple-leveldb-instances
oliverbundalo Mar 27, 2024
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
38 changes: 11 additions & 27 deletions blockchain/storagev2/leveldb/leveldb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,16 @@ type levelDB struct {
}

var tableMapper = map[uint8][]byte{
// Main DB
storagev2.BODY: []byte("b"), // DB key = block number + block hash + mapper, value = block body
storagev2.DIFFICULTY: []byte("d"), // DB key = block number + block hash + mapper, value = block total diffculty
storagev2.HEADER: []byte("h"), // DB key = block number + block hash + mapper, value = block header
storagev2.RECEIPTS: []byte("r"), // DB key = block number + block hash + mapper, value = block receipts
storagev2.CANONICAL: {}, // DB key = block number + mapper, value = block hash

// Lookup DB
storagev2.FORK: {}, // DB key = FORK_KEY + mapper, value = fork hashes
storagev2.HEAD_HASH: {}, // DB key = HEAD_HASH_KEY + mapper, value = head hash
storagev2.HEAD_NUMBER: {}, // DB key = HEAD_NUMBER_KEY + mapper, value = head number
storagev2.BLOCK_LOOKUP: {}, // DB key = block hash + mapper, value = block number
storagev2.TX_LOOKUP: {}, // DB key = tx hash + mapper, value = block number
storagev2.BODY: []byte("b"), // DB key = block number + block hash + mapper, value = block body
storagev2.DIFFICULTY: []byte("d"), // DB key = block number + block hash + mapper, value = block total diffculty
storagev2.HEADER: []byte("h"), // DB key = block number + block hash + mapper, value = block header
storagev2.RECEIPTS: []byte("r"), // DB key = block number + block hash + mapper, value = block receipts
storagev2.CANONICAL: {}, // DB key = block number + mapper, value = block hash
storagev2.FORK: {}, // DB key = FORK_KEY + mapper, value = fork hashes
storagev2.HEAD_HASH: {}, // DB key = HEAD_HASH_KEY + mapper, value = head hash
Stefan-Ethernal marked this conversation as resolved.
Show resolved Hide resolved
storagev2.HEAD_NUMBER: {}, // DB key = HEAD_NUMBER_KEY + mapper, value = head number
Stefan-Ethernal marked this conversation as resolved.
Show resolved Hide resolved
storagev2.BLOCK_LOOKUP: {}, // DB key = block hash + mapper, value = block number
storagev2.TX_LOOKUP: {}, // DB key = tx hash + mapper, value = block number
}

// NewLevelDBStorage creates the new storage reference with leveldb default options
Expand All @@ -44,21 +41,8 @@ func NewLevelDBStorage(path string, logger hclog.Logger) (*storagev2.Storage, er
return nil, err
}

// Open Lookup
// Set default options
options = &opt.Options{
BlockCacheCapacity: 64 * opt.MiB,
WriteBuffer: opt.DefaultWriteBuffer,
}
path += "/lookup"

lookup, err := openLevelDBStorage(path, options)
if err != nil {
return nil, err
}

ldbs[0] = &levelDB{maindb}
ldbs[1] = &levelDB{lookup}
ldbs[1] = nil
Copy link

Choose a reason for hiding this comment

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

Will this cause us some issues? IMO it would be better if we would follow a null object pattern and therefore create the NoOpDatabase implementation, that would implement the following interface:

type Database interface {
	Close() error
	Get(t uint8, k []byte) ([]byte, bool, error)
	NewBatch() Batch
}

...and eliminate the nil checks in the storagev2/storage.go. WDYT?

Copy link
Author

Choose a reason for hiding this comment

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

I need some indication do we have 2 instances or one. If we have only one, then we should always default to index 0 for all read/write operations. So if the 2nd element of array is nil then we know we should pickup db instance with index 0. With this dummy interface implementation we can't accomplish that

Method doing database mapping:
func (s *Storage) getDB(t uint8) Database {
i := getIndex(t)
if s.db[i] != nil {
return s.db[i]
}

return s.db[MAINDB_INDEX]

}

Copy link

@Stefan-Ethernal Stefan-Ethernal Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

But again, instead of checking if it is nil, you can check whether the concrete type is the NoOpDatabase.

But in the other places, it would be safe to invoke the function against the instance (and remove the nil checks), because it would not be nil.

Copy link
Author

Choose a reason for hiding this comment

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

Maybe, but that way we will not know if we are calling a bad instance or a good one. In my opinion it is better to crash then to call interface doing nothing. But anyway it will never happen to call a nil instance because all calls must go through this getDB mapper first.


return storagev2.Open(logger.Named("leveldb"), ldbs)
}
Expand Down
7 changes: 6 additions & 1 deletion blockchain/storagev2/storage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
//nolint:stylecheck
package storagev2

import (
Expand Down Expand Up @@ -39,6 +38,8 @@ const (
)

// Lookup tables
//
//nolint:stylecheck // needed because linter considers _ in name as an error
const (
FORK = uint8(0) | LOOKUP_INDEX
HEAD_HASH = uint8(2) | LOOKUP_INDEX
Expand All @@ -47,14 +48,18 @@ const (
TX_LOOKUP = uint8(8) | LOOKUP_INDEX
)

//nolint:stylecheck // needed because linter considers _ in name as an error
const MAX_TABLES = uint8(20)

// Database indexes
//
//nolint:stylecheck // needed because linter considers _ in name as an error
const (
MAINDB_INDEX = uint8(0)
LOOKUP_INDEX = uint8(1)
)

//nolint:stylecheck // needed because linter considers _ in name as an error
var (
FORK_KEY = []byte("0000000f")
HEAD_HASH_KEY = []byte("0000000h")
Expand Down
Loading