Add validator data retention by pubkey #102
Conversation
aBMania
force-pushed
the
feature/validator-prune-retain-pubkey
branch
from
a542bf3
to
a3194a7
Compare
aBMania
force-pushed
the
feature/validator-prune-retain-pubkey
branch
from
a3194a7
to
728083b
Compare
There was a problem hiding this comment.
Thanks for this PR. In general it looks good, there are a few items to address.
One question: have you analyzed the runtime of the DELETE with and without USING? If it is significantly longer in the latter case due to the join then it may make sense to rewrite the query builder so that it only includes USING if there are pubkeys to retain.
| @@ -518,7 +518,7 @@ WHERE f_validator_index = $1 | |||
| } | |||
|
|
|||
| // PruneValidatorEpochSummaries prunes validator epoch summaries up to (but not including) the given point. | |||
| func (s *Service) PruneValidatorEpochSummaries(ctx context.Context, to phase0.Epoch, retain []phase0.ValidatorIndex) error { | |||
| func (s *Service) PruneValidatorEpochSummaries(ctx context.Context, to phase0.Epoch, retainPubkeys []phase0.BLSPubKey) error { | |||
There was a problem hiding this comment.
I think we can keep this value as retain given that its type is BLSPubKey
| pubkeyBytes := make([]byte, len(pubkey)) | ||
| copy(pubkeyBytes, pubkey[:]) |
There was a problem hiding this comment.
Is this required? Could it be replaced by just pubKey[:] to reduce allocations and copying?
There was a problem hiding this comment.
Replacing by pubKey[:] would work with 1.22 go for loops.
It does not work with 1.21 go (project use 1.20 go, and i did not want to change version)
See example here: https://go.dev/play/p/K5u3iVSeWe_e
There was a problem hiding this comment.
I think you should be able to avoid that by only using the loop variable:
for i := range retain {
pubkeysBytes = append(pubkeysBytes, retain[i][:])
}
There was a problem hiding this comment.
Oh, that's better ! Thanks.
| @@ -716,7 +716,7 @@ func validatorBalanceFromRow(rows pgx.Rows) (*chaindb.ValidatorBalance, error) { | |||
| } | |||
|
|
|||
| // PruneValidatorBalances prunes validator balances up to (but not including) the given epoch. | |||
| func (s *Service) PruneValidatorBalances(ctx context.Context, to phase0.Epoch, retain []phase0.ValidatorIndex) error { | |||
| func (s *Service) PruneValidatorBalances(ctx context.Context, to phase0.Epoch, retainPubkeys []phase0.BLSPubKey) error { | |||
There was a problem hiding this comment.
Same comment as above on variable name.
| retainPubkeysBytes := make([][]byte, 0, len(retainPubkeys)) | ||
|
|
||
| for _, pubkey := range retainPubkeys { | ||
| pubkeyBytes := make([]byte, len(pubkey)) |
There was a problem hiding this comment.
Same comment as above on variable creation.
| @@ -104,6 +108,25 @@ func WithValidatorSummaries(enabled bool) Parameter { | |||
| }) | |||
| } | |||
|
|
|||
| // WithvalidatorRetainPubkeys states if the module should retain balance and epoch summaries for a subset of validator. | |||
There was a problem hiding this comment.
Please update comment to match the function.
| @@ -104,6 +108,25 @@ func WithValidatorSummaries(enabled bool) Parameter { | |||
| }) | |||
| } | |||
|
|
|||
| // WithvalidatorRetainPubkeys states if the module should retain balance and epoch summaries for a subset of validator. | |||
| func WithvalidatorRetainPubkeys(validatorRetainPubkeys []string) Parameter { | |||
There was a problem hiding this comment.
This should have a capital "v".
Also, the pubkeys should not be passed in as a string slice. This function should accept []BLSPubKey to allow for catching and reporting errors in the input.
aBMania
force-pushed
the
feature/validator-prune-retain-pubkey
branch
from
1c26370
to
6c4633e
Compare
|
I did not notice any changes in deletion time. (my bottleneck is io) But, in any way I've updated so that it only add |
|
Thanks for making the changes. I think that the SQL is now broken in the case that there are no items in |
aBMania
force-pushed
the
feature/validator-prune-retain-pubkey
branch
from
534c4ed
to
a8633bf
Compare
|
It should be okay now. I've tested it with and without retain values. |
|
Thanks for your work on this. |
Hello,
I needed to retain data indefinitly for certain pubkeys and saw you already had a
retain []phase0.ValidatorIndexparameter in pruning methodsPruneValidatorEpochSummariesandPruneValidatorBalances.I've made the choice to change validator index to pubkey since I know ahead of time which pubkey are going to be my validator, but not the indices. So i can set all my pubkeys even if my deposits are not yet made. Could do a quick adaptation to handle both index and pubkeys.
Thanks,
Sylvain