Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 20 additions & 13 deletions packages/beacon-node/src/api/impl/beacon/blocks/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ import {kzg} from "../../../../util/kzg.js";
import {promiseAllMaybeAsync} from "../../../../util/promises.js";
import {ApiModules} from "../../types.js";
import {assertUniqueItems} from "../../utils.js";
import {getBlockResponse, toBeaconHeaderResponse} from "./utils.js";
import {countColumnsPublishedWithZeroPeers, getBlockResponse, toBeaconHeaderResponse} from "./utils.js";

type PublishBlockOpts = ImportBlockOpts;

Expand Down Expand Up @@ -739,16 +739,20 @@ export function getBeaconBlockApi({
peerIdStr: undefined,
});

if (dataColumnSidecars.length > 0) {
for (const columnSidecar of dataColumnSidecars) {
payloadInput.addColumn({
// Track which columns were already present in the shared payload input before we self-publish.
// addColumn() returns false when the column index is already cached, i.e. a peer gossiped the
// column to us first (block published first -> peers getBlobs from EL -> disseminate columns)
// before we published the envelope. Such columns are already on the network, so a subsequent
// 0-peers publish is an expected duplicate rather than a propagation failure (see #9527).
const columnAlreadyPresent = dataColumnSidecars.map(
(columnSidecar) =>
!payloadInput.addColumn({

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Distinguish cached columns from gossiped duplicates

This uses addColumn() returning false as proof that a peer already gossiped the column, but PayloadEnvelopeInput can also be pre-populated from non-gossip paths such as execution-engine getBlobsV2, req/resp, or recovery. In those cases the later publishDataColumnSidecar() can still return 0 because there are no topic peers, yet the column is excluded from the reorg-risk warning, and the earlier non-gossip population path does not report this zero-peer condition.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch — you're right, and it's reachable. I was keying the suppression off cache presence (addColumn() returning false), but the payload-envelope cache is also populated by non-gossip paths — verified: engine/getBlobsV2 (util/execution.ts), recovery (util/dataColumns.ts), and req/resp by-range/by-root — all into the same payloadInput. Those columns aren't necessarily on the network, so a 0-peer publish of them is a real propagation failure that my check was silently suppressing.

Fixed in aff57d1: added PayloadEnvelopeInput.getColumnSource() and now suppress the warning only when the already-present column was gossip-sourced (a peer actively pushed it to us → it's propagating). Columns cached from engine/recovery/req_resp still count toward the zero-peer warning. Added a regression test for the non-gossip case.

columnSidecar,
source: PayloadEnvelopeInputSource.api,
seenTimestampSec,
peerIdStr: undefined,
});
}
}
})
);

const valLogMeta = {
slot,
Expand Down Expand Up @@ -787,15 +791,18 @@ export function getBeaconBlockApi({

// Track metrics for data column publishing
if (dataColumnSidecars.length > 0) {
let columnsPublishedWithZeroPeers = 0;
// Skip first entry (envelope); the final entry is processExecutionPayload(), which returns void.
for (let i = 0; i < dataColumnSidecars.length; i++) {
const sentPeers = sentPeersArr[i + 1] as number;
const sentPeersPerColumn = dataColumnSidecars.map((_, i) => sentPeersArr[i + 1] as number);
for (const sentPeers of sentPeersPerColumn) {
metrics?.dataColumns.sentPeersPerSubnet.observe(sentPeers);
if (sentPeers === 0) {
columnsPublishedWithZeroPeers++;
}
}
// Columns already in our seen cache were gossiped to us first, so publishing them is a no-op
// duplicate with 0 recipients — expected, not a propagation failure. Only warn for columns we
// actually introduced to the network. See https://github.com/ChainSafe/lodestar/issues/9527.
const columnsPublishedWithZeroPeers = countColumnsPublishedWithZeroPeers(
sentPeersPerColumn,
columnAlreadyPresent
);
if (columnsPublishedWithZeroPeers > 0) {
chain.logger.warn("Published data columns to 0 peers, increased risk of reorg", {
slot,
Expand Down
27 changes: 27 additions & 0 deletions packages/beacon-node/src/api/impl/beacon/blocks/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,33 @@ export function resolveBlockId(forkChoice: IForkChoice, blockId: routes.beacon.B
return blockSlot;
}

/**
* Count data columns that were published to zero peers AND were newly introduced to the network by us.
*
* When self-building post-gloas, the block is published first, then peers fetch the blobs from their EL
* and disseminate the columns via gossip before we publish the execution payload envelope. Those columns
* can therefore already be in our seen cache by the time we publish them, in which case our publish is a
* no-op duplicate that resolves to zero recipients — expected, not a propagation failure. Only columns we
* actually introduced to the network (not already present) should count towards the zero-peers warning.
*
* See https://github.com/ChainSafe/lodestar/issues/9527.
*
* @param sentPeersPerColumn number of peers each column was published to, aligned with `columnAlreadyPresent`
* @param columnAlreadyPresent whether each column was already in our seen cache before we published it
*/
export function countColumnsPublishedWithZeroPeers(
sentPeersPerColumn: number[],
columnAlreadyPresent: boolean[]
): number {
let count = 0;
for (let i = 0; i < sentPeersPerColumn.length; i++) {
if (sentPeersPerColumn[i] === 0 && !columnAlreadyPresent[i]) {
count++;
}
}
return count;
}

export async function getBlockResponse(
chain: IBeaconChain,
blockId: routes.beacon.BlockId
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import {describe, expect, it} from "vitest";
import {countColumnsPublishedWithZeroPeers} from "../../../../../../src/api/impl/beacon/blocks/utils.js";

describe("api - beacon - blocks - countColumnsPublishedWithZeroPeers", () => {
it("counts newly introduced columns published to zero peers", () => {
// none of the columns were already in the seen cache, all reached zero peers
expect(countColumnsPublishedWithZeroPeers([0, 0, 0], [false, false, false])).toBe(3);
});

it("ignores zero-peer columns that were already in the seen cache (benign duplicates)", () => {
// the buildoor case: peers gossiped every column to us first, so all publishes are duplicates
expect(countColumnsPublishedWithZeroPeers([0, 0, 0], [true, true, true])).toBe(0);
});

it("only counts newly introduced zero-peer columns in a mixed batch", () => {
// index 0: newly introduced, zero peers -> counted
// index 1: already present, zero peers -> benign duplicate, not counted
// index 2: newly introduced, reached peers -> not counted
// index 3: already present, reached peers -> not counted
expect(countColumnsPublishedWithZeroPeers([0, 0, 5, 5], [false, true, false, true])).toBe(1);
});

it("never counts columns that reached at least one peer", () => {
expect(countColumnsPublishedWithZeroPeers([1, 2, 3], [false, false, false])).toBe(0);
});

it("returns 0 for an empty batch", () => {
expect(countColumnsPublishedWithZeroPeers([], [])).toBe(0);
});
});