-
Notifications
You must be signed in to change notification settings - Fork 231
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.
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
clean up / document init #3387
clean up / document init #3387
Conversation
* drop `immutable_validators` data (pre-altair) * document versions where data is first added * avoid needlessly loading genesis block data on startup * add a few more internal database consistency checks * remove duplicate state root lookup on state load
ee4c8b0
to
f183084
Compare
# should not be removed until after altair, to permit downgrades. | ||
# uncompressed keys instead. We still support upgrading a database from the | ||
# old format, but don't need to support downgrading, and therefore safely can | ||
# remove the keys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember having a bench for this.
I only bench compressed key deserialization: https://github.com/status-im/nim-blscurve/blob/0237e4e/benchmarks/bls_signature.nim#L37-L43
Though it makes sense, decompression involves modular square root, which is over 400x more expensive than other operations:
The tradeoff is 2x the size.
Assuming 300k validators that's 48 bytes (381-bit.nextPowerOfTwo() / 8) x 2 x 300k = 28.8MB (instead of 14.4)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, this was benchmarked at the time it was introduced and the difference was significant - you can still see it today: the time to write a checkpoint state from SSZ to the database on first startup is dominated by the time it takes to convert the keys from compressed to uncompressed
@@ -446,6 +449,10 @@ proc new*(T: type BeaconChainDB, | |||
)) | |||
immutableValidatorsDb1.close() | |||
|
|||
# Safe because nobody will be downgrading to pre-altair versions | |||
# TODO: drop table maybe? that would require not creating the table just above | |||
discard db.exec("DELETE FROM immutable_validators;") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to drop the table. And I should do the same with the old slashing DB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as the comment says, the check above recreates the table - this would need more surgery on the codebase
immutable_validators
data (pre-altair)