feat: preverify builder deposit signatures - #9436
Closed
twoeths wants to merge 22 commits into
Closed
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces optimizations for onboarding builders from pending deposits during the Gloas fork transition and post-Gloas block processing. It implements a lazy, batch-verification mechanism for builder deposit signatures using a new BuilderDepositSignatureCache and BatchOnboardBuilder utility, replacing the previous eager, one-by-one verification logic. The code reviewer provided valuable feedback, identifying a potential runtime crash on pre-Gloas forks that can be resolved with a isStatePostGloas guard. Additionally, the reviewer suggested simplifying the BatchOnboardBuilder design by reading directly from the state instead of maintaining a duplicate array of plain objects, and recommended changing the _lastVerifiedSlot sentinel value from 0 to -1 to prevent skipping pre-verification for slot 0.
Comment on lines
+298
to
+301
| if (builderDeposits.length > 0) { | ||
| callInNextEventLoop(() => { | ||
| try { | ||
| const result = blockState.preVerifyPayloadBuilderDeposits(blockHashHex, builderDeposits); |
Contributor
There was a problem hiding this comment.
The blockState view might be from a pre-Gloas fork (e.g., Bellatrix, Capella, Deneb, Electra, Fulu) where preVerifyPayloadBuilderDeposits is not defined. Calling this method without a guard will cause a runtime TypeError and crash the beacon node during block import on those forks. Guard the pre-verification logic with isStatePostGloas(blockState).
Suggested change
| if (builderDeposits.length > 0) { | |
| callInNextEventLoop(() => { | |
| try { | |
| const result = blockState.preVerifyPayloadBuilderDeposits(blockHashHex, builderDeposits); | |
| if (isStatePostGloas(blockState) && builderDeposits.length > 0) { | |
| callInNextEventLoop(() => { | |
| try { | |
| const result = blockState.preVerifyPayloadBuilderDeposits(blockHashHex, builderDeposits); |
Comment on lines
+61
to
+80
| // this is use to scan for reused builder index | ||
| private preExistingBuilders: gloas.Builder[]; | ||
| private nextReuseIndexCheck = 0; | ||
|
|
||
| constructor(private readonly state: CachedBeaconStateGloas) { | ||
| this.preExistingBuilders = this.state.builders.getAllReadonlyValues(); | ||
| this.builderIndexByPubkey = new Map(); | ||
| const currentEpoch = computeEpochAtSlot(this.state.slot); | ||
| // Sentinel = preExistingBuilders.length means "no eligible slot found yet". | ||
| // Since i increases monotonically, (i < firstReuseIdx) is true only until we | ||
| // assign, so this records the FIRST eligible slot's index. | ||
| let firstReuseIdx = this.preExistingBuilders.length; | ||
| for (const [i, builder] of this.preExistingBuilders.entries()) { | ||
| this.builderIndexByPubkey.set(toPubkeyHex(builder.pubkey), i); | ||
| if (i < firstReuseIdx && isBuilderExited(builder, currentEpoch)) { | ||
| firstReuseIdx = i; | ||
| } | ||
| } | ||
| this.nextReuseIndexCheck = firstReuseIdx; | ||
| } |
Contributor
There was a problem hiding this comment.
Instead of maintaining a duplicate preExistingBuilders array of plain objects and manually synchronizing it during top-ups and additions, we can simplify the design by storing only the initial length of the builders list (initialBuildersLength) and reading directly from this.state.builders using getReadonly(i). This avoids duplicate state, potential out-of-sync bugs, and the need for the getAllReadonlyValues() helper.
private readonly initialBuildersLength: number;
private nextReuseIndexCheck = 0;
constructor(private readonly state: CachedBeaconStateGloas) {
this.builderIndexByPubkey = new Map();
this.initialBuildersLength = this.state.builders.length;
const currentEpoch = computeEpochAtSlot(this.state.slot);
let firstReuseIdx = this.initialBuildersLength;
for (let i = 0; i < this.initialBuildersLength; i++) {
const builder = this.state.builders.getReadonly(i);
this.builderIndexByPubkey.set(toPubkeyHex(builder.pubkey), i);
if (i < firstReuseIdx && isBuilderExited(builder, currentEpoch)) {
firstReuseIdx = i;
}
}
this.nextReuseIndexCheck = firstReuseIdx;
}
Comment on lines
+105
to
+108
| if ( | ||
| this.nextReuseIndexCheck < this.preExistingBuilders.length || | ||
| this.queuedBuilderDeposits.size >= BUILDER_DEPOSIT_BATCH_SIZE | ||
| ) { |
Contributor
There was a problem hiding this comment.
Update the queue flush condition to use initialBuildersLength instead of preExistingBuilders.length.
Suggested change
| if ( | |
| this.nextReuseIndexCheck < this.preExistingBuilders.length || | |
| this.queuedBuilderDeposits.size >= BUILDER_DEPOSIT_BATCH_SIZE | |
| ) { | |
| if ( | |
| this.nextReuseIndexCheck < this.initialBuildersLength || | |
| this.queuedBuilderDeposits.size >= BUILDER_DEPOSIT_BATCH_SIZE | |
| ) { |
Comment on lines
+120
to
+126
| topupBuilder(builderIndex: BuilderIndex, amount: UintNum64): void { | ||
| const builder = this.state.builders.get(builderIndex); | ||
| builder.balance += amount; | ||
| if (builderIndex < this.preExistingBuilders.length) { | ||
| this.preExistingBuilders[builderIndex].balance += amount; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
With the simplified design reading directly from this.state.builders, we no longer need to manually update the duplicate preExistingBuilders array during top-ups.
/** Top up an already-onboarded builder's balance. No signature verification needed. */
topupBuilder(builderIndex: BuilderIndex, amount: UintNum64): void {
const builder = this.state.builders.get(builderIndex);
builder.balance += amount;
}
Comment on lines
+170
to
+183
| for (let i = this.nextReuseIndexCheck; i < this.preExistingBuilders.length; i++) { | ||
| const builder = this.preExistingBuilders[i]; | ||
| if (isBuilderExited(builder, currentEpoch)) { | ||
| this.state.builders.set(i, newBuilder); | ||
| this.preExistingBuilders[i] = newBuilder.toValue(); | ||
| this.builderIndexByPubkey.delete(toPubkeyHex(builder.pubkey)); | ||
| this.builderIndexByPubkey.set(toPubkeyHex(newBuilder.pubkey), i); | ||
| this.nextReuseIndexCheck = i + 1; | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| // don't have to scan again the next time | ||
| this.nextReuseIndexCheck = this.preExistingBuilders.length; |
Contributor
There was a problem hiding this comment.
Update addBuilderToRegistry to read directly from this.state.builders.getReadonly(i) and use initialBuildersLength instead of preExistingBuilders.
Suggested change
| for (let i = this.nextReuseIndexCheck; i < this.preExistingBuilders.length; i++) { | |
| const builder = this.preExistingBuilders[i]; | |
| if (isBuilderExited(builder, currentEpoch)) { | |
| this.state.builders.set(i, newBuilder); | |
| this.preExistingBuilders[i] = newBuilder.toValue(); | |
| this.builderIndexByPubkey.delete(toPubkeyHex(builder.pubkey)); | |
| this.builderIndexByPubkey.set(toPubkeyHex(newBuilder.pubkey), i); | |
| this.nextReuseIndexCheck = i + 1; | |
| return; | |
| } | |
| } | |
| // don't have to scan again the next time | |
| this.nextReuseIndexCheck = this.preExistingBuilders.length; | |
| for (let i = this.nextReuseIndexCheck; i < this.initialBuildersLength; i++) { | |
| const builder = this.state.builders.getReadonly(i); | |
| if (isBuilderExited(builder, currentEpoch)) { | |
| this.state.builders.set(i, newBuilder); | |
| this.builderIndexByPubkey.delete(toPubkeyHex(builder.pubkey)); | |
| this.builderIndexByPubkey.set(toPubkeyHex(newBuilder.pubkey), i); | |
| this.nextReuseIndexCheck = i + 1; | |
| return; | |
| } | |
| } | |
| // don't have to scan again the next time | |
| this.nextReuseIndexCheck = this.initialBuildersLength; |
Comment on lines
+191
to
+193
| function isBuilderExited(builder: gloas.Builder, currentEpoch: Epoch): boolean { | ||
| return builder.withdrawableEpoch <= currentEpoch && builder.balance === 0; | ||
| } |
Contributor
There was a problem hiding this comment.
Update isBuilderExited to accept a structurally typed object, making it compatible with both plain objects and SSZ views.
Suggested change
| function isBuilderExited(builder: gloas.Builder, currentEpoch: Epoch): boolean { | |
| return builder.withdrawableEpoch <= currentEpoch && builder.balance === 0; | |
| } | |
| function isBuilderExited(builder: {withdrawableEpoch: Epoch; balance: UintNum64}, currentEpoch: Epoch): boolean { | |
| return builder.withdrawableEpoch <= currentEpoch && builder.balance === 0; | |
| } |
Comment on lines
+47
to
+57
| private _lastVerifiedSlot: Slot = 0; | ||
|
|
||
| get lastVerifiedSlot(): Slot { | ||
| return this._lastVerifiedSlot; | ||
| } | ||
|
|
||
| set lastVerifiedSlot(slot: Slot) { | ||
| if (slot > this._lastVerifiedSlot) { | ||
| this._lastVerifiedSlot = slot; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Initializing _lastVerifiedSlot to 0 creates ambiguity because 0 is a valid slot (GENESIS_SLOT). Any builder deposits at slot 0 would be skipped during pre-verification because deposit.slot <= cursor (where cursor is 0) evaluates to true. Initialize _lastVerifiedSlot to -1 as a proper sentinel value to avoid skipping slot 0.
Suggested change
| private _lastVerifiedSlot: Slot = 0; | |
| get lastVerifiedSlot(): Slot { | |
| return this._lastVerifiedSlot; | |
| } | |
| set lastVerifiedSlot(slot: Slot) { | |
| if (slot > this._lastVerifiedSlot) { | |
| this._lastVerifiedSlot = slot; | |
| } | |
| } | |
| private _lastVerifiedSlot: Slot = -1; | |
| get lastVerifiedSlot(): Slot { | |
| return this._lastVerifiedSlot; | |
| } | |
| set lastVerifiedSlot(slot: Slot) { | |
| if (slot > this._lastVerifiedSlot) { | |
| this._lastVerifiedSlot = slot; | |
| } | |
| } |
Comment on lines
+99
to
+102
| clearPreGloasCache(): void { | ||
| this.preGloasResultsBySlot.clear(); | ||
| this._lastVerifiedSlot = 0; | ||
| } |
Contributor
There was a problem hiding this comment.
Reset _lastVerifiedSlot to -1 when clearing the pre-Gloas cache.
Suggested change
| clearPreGloasCache(): void { | |
| this.preGloasResultsBySlot.clear(); | |
| this._lastVerifiedSlot = 0; | |
| } | |
| clearPreGloasCache(): void { | |
| this.preGloasResultsBySlot.clear(); | |
| this._lastVerifiedSlot = -1; | |
| } |
Comment on lines
+594
to
+597
| clearPreGloasBuilderDepositCache(): void { | ||
| const cache = this.cachedState.epochCtx.builderDepositSignatureCache; | ||
| if (cache.lastVerifiedSlot !== 0) cache.clearPreGloasCache(); | ||
| } |
Contributor
There was a problem hiding this comment.
Update the cache clearing condition to check against -1 instead of 0 to match the new sentinel value.
Suggested change
| clearPreGloasBuilderDepositCache(): void { | |
| const cache = this.cachedState.epochCtx.builderDepositSignatureCache; | |
| if (cache.lastVerifiedSlot !== 0) cache.clearPreGloasCache(); | |
| } | |
| clearPreGloasBuilderDepositCache(): void { | |
| const cache = this.cachedState.epochCtx.builderDepositSignatureCache; | |
| if (cache.lastVerifiedSlot !== -1) cache.clearPreGloasCache(); | |
| } |
Contributor
Performance Report馃殌馃殌 Significant benchmark improvement detected
Full benchmark results
|
Member
Author
|
closing in favor of #9727 |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Motivation
Description
PendingDepositNoSlotfor thatPendingDepositNoSlotcontains 4 fields so we save some hash costs with that, compared toPendingDepositBatchOnboardBuilderto verify builder deposit signatures in batchBuilderDepositSignatureCachefor NodeJSAI Assistance Disclosure