Split validator databases #6048
Conversation
…abases # Conflicts: # validator/db/BUILD.bazel # validator/db/manage.go # validator/db/manage_test.go
# Conflicts: # validator/accounts/account_test.go # validator/db/manage.go # validator/db/manage_test.go
| return nil | ||
| } | ||
|
|
||
| func addAttestations(bucket *bolt.Bucket, attestations pubKeyAttestations) error { |
There was a problem hiding this comment.
This function seems to be assuming that it will called immediately after addProposals right? It doesn't feel right to make that assumption, as someone can use this function incorrectly for some other use case. Perhaps refactor to resolve that risk.
There was a problem hiding this comment.
Even though I do always add attestations after adding proposals, there is no direct dependency between them. One can add attestations before proposals, or even add one but not the other. Attestations and proposals live in separate buckets.
| var allAttestations []pubKeyAttestations | ||
|
|
||
| for _, store := range stores { | ||
| if err := store.db.View(func(tx *bolt.Tx) error { |
There was a problem hiding this comment.
It's risky to clog up the boltdb thread and do all these expensive operations within a single bolt transaction. Any chance we could do these as a separate bolt tx's?
There was a problem hiding this comment.
I found a way to do this, but it's not too elegant.
Firstly I open a transaction in which I extract all keys and assign them to a variable for later use. Then I open one transaction for every stored key and get proposals/attestations for that key.
There might be a better way to do this, but this is the best I could come up with.
| @@ -54,6 +58,220 @@ func TestMerge(t *testing.T) { | |||
| assertMergedStore(t, mergedStore, firstStorePubKeys, secondStorePubKeys, history) | |||
| } | |||
|
|
|||
| func TestSplit(t *testing.T) { | |||
There was a problem hiding this comment.
This test is extremely long. Any way it can be simplified? Perhaps the setup can be a somewhat general helper function, or perhaps can be split into several tests and not just one big TestSplit. For example, TestSplit_OK, TestSplit_CouldNotDoSomething, etc.
There was a problem hiding this comment.
I redesigned the tests, I made helper functions more granular so that they can be reused by tests for merging and splitting. I hope most of these changes will make it easier to write tests for future functionalities, e.g. importing.
|
I had to rename the flags related with merging and splitting to avoid duplicate names. I find it a bit awkward that now the commands have redundancy in flag names, e.g. |
Codecov Report
@@ Coverage Diff @@
## master #6048 +/- ##
==========================================
- Coverage 59.51% 59.50% -0.02%
==========================================
Files 320 320
Lines 27023 27129 +106
==========================================
+ Hits 16083 16143 +60
- Misses 8749 8777 +28
- Partials 2191 2209 +18 |
validator/db/manage.go
Outdated
| allAttestations []pubKeyAttestations) (err error) { | ||
|
|
||
| var storesToClose []*Store | ||
| defer (func() { |
There was a problem hiding this comment.
| defer (func() { | |
| defer func() { |
validator/db/manage.go
Outdated
| err = errors.New(errorMessage) | ||
| } | ||
| } | ||
| })() |
There was a problem hiding this comment.
| })() | |
| }() |
validator/db/manage.go
Outdated
| if err != nil { | ||
| err = errors.Wrapf(err, errorMessage) | ||
| } else { | ||
| err = errors.New(errorMessage) |
There was a problem hiding this comment.
You need to do something about these errors in this deferred function. The function will not run until createSplitTargetStores returns, so this error will have nowhere to do. I would suggest creating an error log here instead. There's no way to control nor extract the outputs of deferred functions
There was a problem hiding this comment.
Actually, there is:
#6027 (comment)
There was a problem hiding this comment.
Wow awesome! Thanks for sharing, good thing to know :)
validator/accounts/account.go
Outdated
| @@ -225,17 +225,26 @@ func HandleEmptyKeystoreFlags(cliCtx *cli.Context, confirmPassword bool) (string | |||
| func Merge(ctx context.Context, sourceDirectories []string, targetDirectory string) (err error) { | |||
| var sourceStores []*db.Store | |||
| defer func() { | |||
| errorMessage := "failed to close one or more source databases" | |||
There was a problem hiding this comment.
can you make this error a package variable ?
ex:
errSource := errors.New("failed to close one or more source databases")You can then resuse the error here and in other places instead of redefining it again.
rauljordan
dismissed
their stale review
Making errors package variables would be helpful
What type of PR is this?
Feature
What does this PR do? Why is it needed?
This PR introduces a new command:
validator accounts split. Is takes a source directory and a target directory as inputs and creates one validator database for each public key in the source directory's validator database. Each database is saved in a subdirectory of the target directory. The subdirectory's name is the hex-encoded public key whose data has been split into the database. The encoding is the same that is used for naming validator private key files. This way it will be easy to identify which database belongs to which key.Which issues(s) does this PR fix?
This PR addresses #5638, however it does not close it yet. There are two other operations to implement: importing and exporting data for selected keys.