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.

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: beaconBlocksMaybeBlobsByRoot() to handle single block root #7468

Draft
wants to merge 4 commits into
base: peerDAS
Choose a base branch
from

Conversation

twoeths
Copy link
Contributor

@twoeths twoeths commented Feb 14, 2025

Motivation

^[[39m: matchBlockWithDataColumns2 dataColumnIndexes=11 15, reque        stedColumnsPresent=false, slot=38584, peerClient=Lighthouse-cgc:128
2609158 Feb-12 18:58:44.857[sync]            ^[[34mdebug^[[39m: Missing or mismatching dataColumnSidecars from peerId=16U        iu2HAmJyaVGkRGR9ACqompkoge8T2x4KFH4KbzDkh7zz6uN2JX for blockSlot=38584 with numColumns=8 dataColumnSidecars=2 req        uestedColumnsPresent=false received dataColumnIndexes=11 15 requested=11 15 65 71 72 73 83 108 allBlocks=1, allDa        taColumnSidecars=2, peerId=16Uiu2HAmJyaVGkRGR9ACqompkoge8T2x4KFH4KbzDkh7zz6uN2JX, nodeId=0x70a752f736fd0254c715ae        31a3b64d9f3e011c1ca38e7f828d6d7f7bdf4391aa, blobKzgCommitmentsLen=6, peerClient=Lighthouse-cgc:128
  • this is the logic inside beaconBlocksMaybeBlobsByRoot()

Description

  • at gossip time, beaconBlocksMaybeBlobsByRoot() call range sync's matchBlockWithDataColumns() which is a mistake because we should not expect peer has enough columns for us at gossip time. Spec: https://github.com/ethereum/consensus-specs/blob/dev/specs/fulu/p2p-interface.md#datacolumnsidecarsbyroot-v1
  • instead of that, just get as much columns as we can and return data promise at that time
  • more details: bring matchBlockWithDataColumns() logic without matching code
  • also this beaconBlocksMaybeBlobsByRoot() should not support multiple roots because:
    • consumer only need 1 root
    • from fulu, for each block peer may has different data for it at gossip time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant