-
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
improve deposit processing performance #4082
Conversation
When there are a lot of deposits, we decompress the public key into a crypto cache. To avoid having those caches grow unreasonably big, make sure to operate on the decompressed pubkey instead.
beacon_chain/spec/datatypes/base.nim
Outdated
@@ -537,6 +537,15 @@ func getImmutableValidatorData*(validator: Validator): ImmutableValidatorData2 = | |||
pubkey: cookedKey.get(), | |||
withdrawal_credentials: validator.withdrawal_credentials) | |||
|
|||
func getImmutableValidatorData*( | |||
deposit: DepositData): Opt[ImmutableValidatorData2] = | |||
let cookedKey = deposit.pubkey.load() |
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.
this is the operation that is slow ;)
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.
let cookedKey = validator.pubkey.load() # Loading the pubkey is slow! | ||
doAssert cookedKey.isSome, | ||
"Cannot parse validator key: " & toHex(validator.pubkey) | ||
let cookedKey = validator.pubkey.loadValid() # `Validator` has valid key |
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.
this effectively skips blst_p1_affine_in_g1
just to assert :)
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.
Validator
instances may come from an untrusted source (like the REST API or an invalid checkpoint sync state) - as long as this function is public, there's no guarantee that the pubkey inside is "valid enough"
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.
it's an odd function in general - I think it's worth rethinking this flow in general and see if there's low-hanging room for improvement
To give some figures on the pubkey deserialization speed. Source mratsim/constantine#183 Pubkey deserialization without check: 11us This is the same speed in Constantine, which will allow me to extrapolate uncompressed deserialization speed Compression explanationA BLS12-381 G1 (pubkey) point has coordinate (x, y) with formula: Difference between compressed and uncompressed speedSo the differences between compressed and uncompressed representation are:
Conclusion99.9% of deserialization cost without checks is due to compression and we can make it virtually instant with uncompressed pubkeys |
unfortunately we don't have uncompressed deposit pubkeys - so the only thing we can do with deposits that I know is to batch-verify all deposits in a block (unless there's a way to batch-load keys as well?).. oh, and as etan points out, we could avoid decompressing the keys twice, but that's probably a non-trivial flow to introduce |
hm, maybe we can have a variant pubkey type that holds either compressed or uncompressed key |
We can load them in parallel but there is no math / cryptography or number-theoretic construct to improve batch-load like we can batch-verify.
In terms of memory, the variant take as much space as the bigger type + 1 byte. Anyway the question becomes, are there cases where we would load a compressed pubkey without using it right away and so we want to delay paying that cost? |
The flow here is (1) deposit processing, as part of which pubkey is fully loaded and checked; (2) create In this PR, (1) is modified to not pollute the last-resort crypto key cache, and (3) is modified to use the decompression without repeating the entire verification process. |
If someone becomes a validator they will attest once every 6 min anyway so it will be cached and we only delay the inevitable, or did I miss something? |
There is a separate crypto key cache when using |
When there are a lot of deposits, we decompress the public key into a crypto cache. To avoid having those caches grow unreasonably big, make sure to operate on the decompressed pubkey instead.