-
-
Notifications
You must be signed in to change notification settings - Fork 306
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
fix: avoid using BigInt for slashing classes #6116
Conversation
Performance Report✔️ no performance regression detected Full benchmark results
|
Please can you show evidence that proofs that the observed event (gc spike) is actually caused by the bigint field in the slashing messages? |
@dapplion I couldn't come up with a test to prove it, I'll try to setup a devnet and try to reproduce there. I guess the spike not only come from AttesterSlashings specifically but also some slashing process in state transition too. In the past there are a lot of issues regarding BigInt so I'd not use it if possible, we switched slot, epoch, effectiveBalanceByIncrement. Also the recent BigInt usage in gossipsub caused issue #5892 cc @wemeetagain Using |
closing the PR as I couldn't reproduce this issue in devnet, will look into this if performance issue happens again for AttesterSlashing messages, also #6121 could be enough |
Motivation
gc
spiked, state transition time spiked and node was not able to recover even 6 days after the incident, see Improve gossip block receive time #6105Description
BigInt
:hashTreeRoot()
andserialize()
should be the sametoJson
andfromJson
as string number in order to run spec tests, so created a new typeBytes8UintJson
part of #6112
part of #5892