-
-
Notifications
You must be signed in to change notification settings - Fork 287
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
Handle skipped blobs side car for empty blobs in blobsSidecarsByRange #4936
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
4743820
Handle skipped blobs side car for empty blobs in blobsSidecarsByRange
g11tech aabd227
add another quick check
g11tech a32df41
relocate beacon blocks by range
g11tech 61fe565
use interface
g11tech a8a283f
setup mock test infra
g11tech 1a65881
make testformat scalable
g11tech 9bd5383
refac for better testing
g11tech File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
87 changes: 87 additions & 0 deletions
87
packages/beacon-node/src/network/reqresp/doBeaconBlocksMaybeBlobsByRange.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
import {PeerId} from "@libp2p/interface-peer-id"; | ||
import {IBeaconConfig} from "@lodestar/config"; | ||
import {eip4844, Epoch, phase0} from "@lodestar/types"; | ||
import {ForkSeq} from "@lodestar/params"; | ||
import {computeEpochAtSlot} from "@lodestar/state-transition"; | ||
|
||
import {BlockInput, getBlockInput} from "../../chain/blocks/types.js"; | ||
import {IReqRespBeaconNode} from "./interface.js"; | ||
|
||
export async function doBeaconBlocksMaybeBlobsByRange( | ||
config: IBeaconConfig, | ||
reqResp: IReqRespBeaconNode, | ||
peerId: PeerId, | ||
request: phase0.BeaconBlocksByRangeRequest, | ||
currentEpoch: Epoch, | ||
// To make test life easy without loading ckzg | ||
emptyKzgAggregatedProof: eip4844.BlobsSidecar["kzgAggregatedProof"] | ||
): Promise<BlockInput[]> { | ||
// TODO EIP-4844: Assumes all blocks in the same epoch | ||
// TODO EIP-4844: Ensure all blocks are in the same epoch | ||
if (config.getForkSeq(request.startSlot) < ForkSeq.eip4844) { | ||
const blocks = await reqResp.beaconBlocksByRange(peerId, request); | ||
return blocks.map((block) => getBlockInput.preEIP4844(config, block)); | ||
} | ||
|
||
// Only request blobs if they are recent enough | ||
else if (computeEpochAtSlot(request.startSlot) >= currentEpoch - config.MIN_EPOCHS_FOR_BLOBS_SIDECARS_REQUESTS) { | ||
// TODO EIP-4844: Do two requests at once for blocks and blobs | ||
const blocks = await reqResp.beaconBlocksByRange(peerId, request); | ||
const blobsSidecars = await reqResp.blobsSidecarsByRange(peerId, request); | ||
|
||
const blockInputs: BlockInput[] = []; | ||
let blobSideCarIndex = 0; | ||
let lastMatchedSlot = -1; | ||
|
||
// Match blobSideCar with the block as some blocks would have no blobs and hence | ||
// would be omitted from the response. If there are any inconsitencies in the | ||
// response, the validations during import will reject the block and hence this | ||
// entire segment. | ||
// | ||
// Assuming that the blocks and blobs will come in same sorted order | ||
for (let i = 0; i < blocks.length; i++) { | ||
const block = blocks[i]; | ||
let blobsSidecar: eip4844.BlobsSidecar; | ||
|
||
if (blobsSidecars[blobSideCarIndex]?.beaconBlockSlot === block.message.slot) { | ||
blobsSidecar = blobsSidecars[blobSideCarIndex]; | ||
lastMatchedSlot = block.message.slot; | ||
blobSideCarIndex++; | ||
} else { | ||
// Quick inspect if the blobsSidecar was expected | ||
const blobKzgCommitmentsLen = (block.message.body as eip4844.BeaconBlockBody).blobKzgCommitments.length; | ||
if (blobKzgCommitmentsLen !== 0) { | ||
throw Error( | ||
`Missing blobsSidecar for blockSlot=${block.message.slot} with blobKzgCommitmentsLen=${blobKzgCommitmentsLen}` | ||
); | ||
} | ||
blobsSidecar = { | ||
beaconBlockRoot: config.getForkTypes(block.message.slot).BeaconBlock.hashTreeRoot(block.message), | ||
beaconBlockSlot: block.message.slot, | ||
blobs: [], | ||
kzgAggregatedProof: emptyKzgAggregatedProof, | ||
}; | ||
} | ||
blockInputs.push(getBlockInput.postEIP4844(config, block, blobsSidecar)); | ||
} | ||
|
||
// If there are still unconsumed blobs this means that the response was inconsistent | ||
// and matching was wrong and hence we should throw error | ||
if (blobsSidecars[blobSideCarIndex] !== undefined) { | ||
throw Error( | ||
`Unmatched blobsSidecars, blocks=${blocks.length}, blobs=${ | ||
blobsSidecars.length | ||
} lastMatchedSlot=${lastMatchedSlot}, pending blobsSidecars slots=${blobsSidecars | ||
.slice(blobSideCarIndex) | ||
.map((blb) => blb.beaconBlockSlot)}` | ||
); | ||
} | ||
return blockInputs; | ||
} | ||
|
||
// Post EIP-4844 but old blobs | ||
else { | ||
const blocks = await reqResp.beaconBlocksByRange(peerId, request); | ||
return blocks.map((block) => getBlockInput.postEIP4844OldBlobs(config, block)); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,3 @@ | ||
export * from "./ReqRespBeaconNode.js"; | ||
export * from "./interface.js"; | ||
export * from "./doBeaconBlocksMaybeBlobsByRange.js"; |
88 changes: 88 additions & 0 deletions
88
packages/beacon-node/test/unit/network/doBeaconBlocksMaybeBlobsByRange.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
import sinon, {SinonStubbedInstance} from "sinon"; | ||
import {expect} from "chai"; | ||
import {peerIdFromString} from "@libp2p/peer-id"; | ||
import {ssz, eip4844} from "@lodestar/types"; | ||
import {createIBeaconConfig, createIChainForkConfig, defaultChainConfig} from "@lodestar/config"; | ||
|
||
import {doBeaconBlocksMaybeBlobsByRange, ReqRespBeaconNode} from "../../../src/network/reqresp/index.js"; | ||
import {BlockInputType} from "../../../src/chain/blocks/types.js"; | ||
|
||
describe("doBeaconBlocksMaybeBlobsByRange", function () { | ||
const sandbox = sinon.createSandbox(); | ||
const reqResp = sandbox.createStubInstance(ReqRespBeaconNode) as SinonStubbedInstance<ReqRespBeaconNode> & | ||
ReqRespBeaconNode; | ||
const peerId = peerIdFromString("Qma9T5YraSnpRDZqRR4krcSJabThc8nwZuJV3LercPHufi"); | ||
|
||
/* eslint-disable @typescript-eslint/naming-convention */ | ||
const chainConfig = createIChainForkConfig({ | ||
...defaultChainConfig, | ||
ALTAIR_FORK_EPOCH: 0, | ||
BELLATRIX_FORK_EPOCH: 0, | ||
CAPELLA_FORK_EPOCH: 0, | ||
EIP4844_FORK_EPOCH: 0, | ||
}); | ||
const genesisValidatorsRoot = Buffer.alloc(32, 0xaa); | ||
const config = createIBeaconConfig(chainConfig, genesisValidatorsRoot); | ||
const rangeRequest = ssz.phase0.BeaconBlocksByRangeRequest.defaultValue(); | ||
const emptyKzgAggregatedProof = ssz.eip4844.BlobsSidecar.defaultValue().kzgAggregatedProof; | ||
|
||
const block1 = ssz.eip4844.SignedBeaconBlock.defaultValue(); | ||
block1.message.slot = 1; | ||
const block2 = ssz.eip4844.SignedBeaconBlock.defaultValue(); | ||
block2.message.slot = 2; | ||
|
||
const blobsSidecar1 = ssz.eip4844.BlobsSidecar.defaultValue(); | ||
blobsSidecar1.beaconBlockSlot = 1; | ||
const blobsSidecar2 = ssz.eip4844.BlobsSidecar.defaultValue(); | ||
blobsSidecar2.beaconBlockSlot = 2; | ||
|
||
// Array of testcases which are array of matched blocks with/without (if empty) sidecars | ||
const testCases: [string, [eip4844.SignedBeaconBlock, eip4844.BlobsSidecar | undefined][]][] = [ | ||
["one block with sidecar", [[block1, blobsSidecar1]]], | ||
[ | ||
"two blocks with sidecar", | ||
[ | ||
[block1, blobsSidecar1], | ||
[block2, blobsSidecar2], | ||
], | ||
], | ||
["block with skipped sidecar", [[block1, undefined]]], | ||
]; | ||
testCases.map(([testName, blocksWithBlobs]) => { | ||
it(testName, async () => { | ||
const blocks = blocksWithBlobs.map(([block, _blobs]) => block as eip4844.SignedBeaconBlock); | ||
const blobsSidecars = blocksWithBlobs | ||
.map(([_block, blobs]) => blobs as eip4844.BlobsSidecar) | ||
.filter((blobs) => blobs !== undefined); | ||
|
||
const expectedResponse = blocksWithBlobs.map(([block, blobsSidecar]) => { | ||
const blobs = | ||
blobsSidecar !== undefined | ||
? blobsSidecar | ||
: { | ||
beaconBlockRoot: ssz.eip4844.BeaconBlock.hashTreeRoot(block.message), | ||
beaconBlockSlot: block.message.slot, | ||
blobs: [], | ||
kzgAggregatedProof: emptyKzgAggregatedProof, | ||
}; | ||
return { | ||
type: BlockInputType.postEIP4844, | ||
block, | ||
blobs, | ||
}; | ||
}); | ||
reqResp.beaconBlocksByRange.resolves(blocks); | ||
reqResp.blobsSidecarsByRange.resolves(blobsSidecars); | ||
|
||
const response = await doBeaconBlocksMaybeBlobsByRange( | ||
config, | ||
reqResp, | ||
peerId, | ||
rangeRequest, | ||
0, | ||
emptyKzgAggregatedProof | ||
); | ||
expect(response).to.be.deep.equal(expectedResponse); | ||
}); | ||
}); | ||
}); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Please always append
.test.ts
extension to test filesThere 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.
ahh thats my bad, here is the fix: #4949