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

feat: block network processor when processing current slot block #5458

Merged
merged 3 commits into from
May 4, 2023
Merged

feat: block network processor when processing current slot block #5458

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2d13e55
feat: block network processor if processing current slot block
twoeths May 3, 2023
d8a9884
fix: remove default maxGossipTopicConcurrency value
twoeths May 3, 2023
7331f4e
fix: move isProcessingCurrentSlotBlock to network processor
twoeths May 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 14 additions & 1 deletion packages/beacon-node/src/chain/chain.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ export class BeaconChain implements IBeaconChain {
private readonly exchangeTransitionConfigurationEverySlots: number;

private processShutdownCallback: ProcessShutdownCallback;
private _isProcessingCurrentSlotBlock = false;

constructor(
opts: IChainOptions,
Expand Down Expand Up @@ -284,6 +285,10 @@ export class BeaconChain implements IBeaconChain {
await this.bls.close();
}

isProcessingCurrentSlotBlock(): boolean {
return this._isProcessingCurrentSlotBlock;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this logic and _isProcessingCurrentSlotBlock be in the network processor class? It knows the block's slot and current clock

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I can move the logic there. It means we block network processor messages right after we receive a gossip block message instead of at the time we process block


regenCanAcceptWork(): boolean {
return this.regen.canAcceptWork();
}
Expand Down Expand Up @@ -451,7 +456,14 @@ export class BeaconChain implements IBeaconChain {
}

async processBlock(block: BlockInput, opts?: ImportBlockOpts): Promise<void> {
return this.blockProcessor.processBlocksJob([block], opts);
try {
if (block.block.message.slot === this.clock.currentSlot) {
this._isProcessingCurrentSlotBlock = true;
}
await this.blockProcessor.processBlocksJob([block], opts);
} finally {
this._isProcessingCurrentSlotBlock = false;
}
}

async processChainSegment(blocks: BlockInput[], opts?: ImportBlockOpts): Promise<void> {
Expand Down Expand Up @@ -647,6 +659,7 @@ export class BeaconChain implements IBeaconChain {

private onClockSlot(slot: Slot): void {
this.logger.verbose("Clock slot", {slot});
this._isProcessingCurrentSlotBlock = false;

// CRITICAL UPDATE
if (this.forkChoice.irrecoverableError) {
Expand Down
1 change: 1 addition & 0 deletions packages/beacon-node/src/chain/interface.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ export interface IBeaconChain {
persistInvalidSszView(view: TreeView<CompositeTypeAny>, suffix?: string): void;
updateBuilderStatus(clockSlot: Slot): void;

isProcessingCurrentSlotBlock(): boolean;
regenCanAcceptWork(): boolean;
blsThreadPoolCanAcceptWork(): boolean;
}
Expand Down
3 changes: 2 additions & 1 deletion packages/beacon-node/src/metrics/metrics/lodestar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,10 @@ export function createLodestarMetrics(
help: "Total calls to network processor execute work fn",
buckets: [0, 1, 5, 128],
}),
canNotAcceptWork: register.gauge({
canNotAcceptWork: register.gauge<"reason">({
name: "lodestar_network_processor_can_not_accept_work_total",
help: "Total times network processor can not accept work on executeWork",
labelNames: ["reason"],
}),
},

Expand Down
3 changes: 0 additions & 3 deletions packages/beacon-node/src/network/options.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,5 @@ export const defaultNetworkOptions: NetworkOptions = {
// TODO: this value is 12 per spec, however lodestar has performance issue if there are too many mesh peers
// see https://github.com/ChainSafe/lodestar/issues/5420
gossipsubDHigh: 9,
// TODO: with this value, lodestar drops about 35% of attestation messages on a test mainnet node subscribed to all subnets
// see https://github.com/ChainSafe/lodestar/issues/5441
maxGossipTopicConcurrency: 512,
...defaultGossipHandlerOpts,
};
34 changes: 30 additions & 4 deletions packages/beacon-node/src/network/processor/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,13 +281,14 @@ export class NetworkProcessor {
let jobsSubmitted = 0;

job_loop: while (jobsSubmitted < MAX_JOBS_SUBMITTED_PER_TICK) {
// Check canAcceptWork before calling queue.next() since it consumes the items
const canAcceptWork = this.chain.blsThreadPoolCanAcceptWork() && this.chain.regenCanAcceptWork();
// Check if chain can accept work before calling queue.next() since it consumes the items
const reason = checkAcceptWork(this.chain);

for (const topic of executeGossipWorkOrder) {
// beacon block is guaranteed to be processed immedately
if (!canAcceptWork && !executeGossipWorkOrderObj[topic]?.bypassQueue) {
this.metrics?.networkProcessor.canNotAcceptWork.inc();
// reason !== null means cannot accept work
if (reason !== null && !executeGossipWorkOrderObj[topic]?.bypassQueue) {
this.metrics?.networkProcessor.canNotAcceptWork.inc({reason});
break job_loop;
}
if (
Expand Down Expand Up @@ -321,3 +322,28 @@ export class NetworkProcessor {
}
}
}

enum CannotAcceptWorkReason {
processingCurrentSlotBlock = "processing_current_slot_block",
bls = "bls_busy",
regen = "regen_busy",
}

/**
* Return null if chain can accept work, otherwise return the reason why it cannot accept work
*/
function checkAcceptWork(chain: IBeaconChain): null | CannotAcceptWorkReason {
if (chain.isProcessingCurrentSlotBlock()) {
return CannotAcceptWorkReason.processingCurrentSlotBlock;
}

if (!chain.blsThreadPoolCanAcceptWork()) {
return CannotAcceptWorkReason.bls;
}

if (!chain.regenCanAcceptWork()) {
return CannotAcceptWorkReason.regen;
}

return null;
}
4 changes: 4 additions & 0 deletions packages/beacon-node/test/utils/mocks/chain/chain.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,10 @@ export class MockBeaconChain implements IBeaconChain {
async updateBeaconProposerData(): Promise<void> {}
updateBuilderStatus(): void {}

isProcessingCurrentSlotBlock(): boolean {
return false;
}

regenCanAcceptWork(): boolean {
return true;
}
Expand Down