-
-
Notifications
You must be signed in to change notification settings - Fork 289
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
feat: batch io operations when verifying & importing block #5473
Conversation
Performance Report✔️ no performance regression detected Full benchmark results
|
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.
Pushed some changes, see
lodestar/packages/beacon-node/src/chain/blocks/index.ts
Lines 145 to 159 in 4ce2967
// Clean db if we don't have blocks in forkchoice but already persisted them to db | |
// | |
// NOTE: this function is awaited to ensure that DB size remains constant, otherwise an attacker may bloat the | |
// disk with big malicious payloads. Our sequential block importer will wait for this promise before importing | |
// another block. The removal call error is not propagated since that would halt the chain. | |
// | |
// LOG: Because the error is not propagated and there's a risk of db bloat, the error is logged at warn level | |
// to alert the user of potential db bloat. This error _should_ never happen user must act and report to us | |
await removeEagerlyPeristedBlockInputs.call(this, blocks).catch((e) => { | |
this.logger.warn( | |
"Error pruning eagerly imported block inputs, DB may grow in size if this error happens frequently", | |
{slot: blocks.map((block) => block.block.message.slot).join(",")}, | |
e | |
); | |
}); |
@tuyennhv we have to be careful here since this may open up attacks. We are exposing DB growth to potentially untrusted data. Blocks from gossip have proposer sig validated but blocks from sync have no validation at all. We may want to disable this feature for sync and only do for payloads with some validation like gossip or unknown block sync
4ce2967
to
71c1dbd
Compare
@dapplion I added |
agreed, anyway eager saving on sync makes no effective impact because of checkpoint sync |
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.
Looks great! Both gossip and unknown block root are safe for eagerPersistBlock
. Can you open an issue to add a test that asserts that unknown block sync won't send to the chain blocks with invalid root?
🎉 This PR is included in v1.9.0 🎉 |
Motivation
Description
Promise.all()
inverifyBlock()
, take that chance to persist blocks (and blobs) to dbCloses #5415