Skip to content
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.

Sign up for GitHub

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

Jump to bottom

No delay when fetching blobs with known block and no attempt to recover blobs for unknown block #8927

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

No delay when fetching blobs with known block and no attempt to recover blobs for unknown block #8927

wants to merge 14 commits into from
+236 −397

Conversation

StefanBratanov
Copy link
Contributor

@StefanBratanov StefanBratanov commented Dec 16, 2024 •
edited
Loading

PR Description

  • Separate between RPC block fetching and RPC blobs fetching

Changed the flow to be as such:

  • If we first receive a blob sidecar: Only trigger block fetch after a delay (same as before)
  • If we first receive block: No delay and trigger EL lookup. RPC lookup is attempted after a delay (same as before)
  • On arrival of block, if tracker exists: We trigger the blobs lookup from EL again and RPC lookup (if tracker is not complete)

Fixed Issue(s)

fixes #8796

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

@StefanBratanov StefanBratanov force-pushed the refactor_block_blob_trackers branch from e03e619 to 9126d62 Compare December 16, 2024 16:42
@StefanBratanov StefanBratanov changed the title Remove attempt to recover blobs when we don't have block No delay when fetching blobs with known block and no attempt to recover blobs for unknown Dec 17, 2024
@StefanBratanov StefanBratanov changed the title No delay when fetching blobs with known block and no attempt to recover blobs for unknown No delay when fetching blobs with known block and no attempt to recover blobs for unknown block Dec 17, 2024
@StefanBratanov StefanBratanov force-pushed the refactor_block_blob_trackers branch from be5f764 to 4d0a6c8 Compare December 17, 2024 14:57
@StefanBratanov StefanBratanov marked this pull request as ready for review December 17, 2024 14:57
@tbenr
Copy link
Contributor

tbenr commented Dec 17, 2024

nice! I'll play around with this tomorrow

@StefanBratanov StefanBratanov force-pushed the refactor_block_blob_trackers branch 2 times, most recently from 13721c4 to a09c5ff Compare December 18, 2024 12:22
tbenr
tbenr reviewed Dec 18, 2024
View reviewed changes
Copy link
Contributor

@tbenr tbenr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed I'd add an delay before requesting blobs via RPC when local el doesn't have the missing blob(s)

...ansition/src/main/java/tech/pegasys/teku/statetransition/blobs/BlockBlobSidecarsTracker.java Outdated Show resolved Hide resolved
@StefanBratanov StefanBratanov force-pushed the refactor_block_blob_trackers branch from 66c272a to 9d1d5d9 Compare December 19, 2024 10:22
mehdi-aouadi
mehdi-aouadi reviewed Dec 20, 2024
View reviewed changes
...ansition/src/main/java/tech/pegasys/teku/statetransition/blobs/BlockBlobSidecarsTracker.java
.map(blobIndex -> new BlobIdentifier(slotAndBlockRoot.getBlockRoot(), blobIndex));
}

public Stream<BlobIdentifier> getUnusedBlobSidecarsForBlock() {
final Optional<Integer> blockCommitmentsCount = getBlockKzgCommitmentsCount();
checkState(blockCommitmentsCount.isPresent(), "Block must me known to call this method");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're at it: must be know

mehdi-aouadi
mehdi-aouadi approved these changes Dec 20, 2024
View reviewed changes
Copy link
Contributor

@mehdi-aouadi mehdi-aouadi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
I'd suggest running a local interop or deploy it to a Holesky node and see how it goes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tbenr tbenr tbenr left review comments

@mehdi-aouadi mehdi-aouadi mehdi-aouadi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove delay when looking up blobs via local EL
3 participants
@StefanBratanov @tbenr @mehdi-aouadi