-
Notifications
You must be signed in to change notification settings - Fork 28
DRAFT: feat(chain): check remaining blocks in IBD to enable blob validation #1721
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
base: chain-enable-blob-validation-during-ibd-base
Are you sure you want to change the base?
DRAFT: feat(chain): check remaining blocks in IBD to enable blob validation #1721
Conversation
a8b3b07
to
30710f1
Compare
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.
Not against this, but it's not critical so if we don't know yet how it will be used or have other reasons to postpone it (e.g. it will become clearer after other things have been finished) then it's perfectly fine to put it on hold
state_recording_interval: [ 60, 0 ] | ||
ibd: | ||
peers: [ ] | ||
last_blocks_with_blob_validation: 8640 |
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.
how should we interpret this value and how would one select it?
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 value must be defined in the DA spec, because this means the period how long DA nodes will keep blobs. But, it is not determined yet. I added this parameter here because we anyway need to for IBD.
pub delay_before_new_download: Duration, | ||
/// Threshold in number of remaining blocks to the target height | ||
/// at which blob validation becomes enabled during IBD. | ||
pub last_blocks_with_blob_validation: Length, |
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.
shouldn't this be first block with validation?
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 intended this parameter to represent the number of "remaining" blocks. So, I think that "last" makes more sense.
If there are N blocks in total that we have to catch up during IBD, and if there are last_blocks = 8000
,
we're going to disable blob validation while we process N - 8000
blocks. After that, we will enable blob validation for next 8000 blocks.
Please let me know if this naming doesn't sound good. I will try to think about other names (or updating comments).
1. What does this PR implement?
Partially resolves #1675.
We starts IBD with disabling blob validation.
At some point during IBD, we should enable the blob validation.
For now, we agreed on enabling it once entering the DA window (which is not defined yet).
So, this PR defines a parameter
ibd.last_blocks_with_blob_validation
. If there are remaining blocks to the target less than the parameter, IBD enables blob validation.This PR doesn't yet use the historical blob sampling API after enabling blob validation. Still it skips blob validation. I will change it once all other components (services) are ready. This PR just prepares the foundation.
Also, I promise I will open a refactoring PR soon to prevent making the chain service more verbose. I have an idea, but didn't want to make this PR hard to review.
2. Does the code have enough context to be clearly understood?
yes
3. Who are the specification authors and who is accountable for this PR?
@youngjoon-lee
4. Is the specification accurate and complete?
yes
5. Does the implementation introduce changes in the specification?
no
Checklist
Warning
Do not merge the PR if any of the following is missing: