-
-
Notifications
You must be signed in to change notification settings - Fork 300
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
feat: n-historical states feature flags #6538
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,7 @@ import { | |
ForkChoiceStore, | ||
ExecutionStatus, | ||
JustifiedBalancesGetter, | ||
ForkChoiceOpts, | ||
ForkChoiceOpts as RealForkChoiceOpts, | ||
} from "@lodestar/fork-choice"; | ||
import { | ||
CachedBeaconStateAllForks, | ||
|
@@ -21,7 +21,10 @@ import {ChainEventEmitter} from "../emitter.js"; | |
import {ChainEvent} from "../emitter.js"; | ||
import {GENESIS_SLOT} from "../../constants/index.js"; | ||
|
||
export type {ForkChoiceOpts}; | ||
export type ForkChoiceOpts = RealForkChoiceOpts & { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like the use of word "real" is a little unconventional here though I can't come up with a better name. Maybe |
||
// for testing only | ||
forkchoiceConstructor?: typeof ForkChoice; | ||
}; | ||
|
||
/** | ||
* Fork Choice extended with a ChainEventEmitter | ||
|
@@ -47,7 +50,11 @@ export function initializeForkChoice( | |
|
||
const justifiedBalances = getEffectiveBalanceIncrementsZeroInactive(state); | ||
|
||
return new ForkChoice( | ||
// forkchoiceConstructor is only used for some test cases | ||
// production code use ForkChoice constructor directly | ||
const forkchoiceConstructor = opts.forkchoiceConstructor ?? ForkChoice; | ||
|
||
return new forkchoiceConstructor( | ||
config, | ||
|
||
new ForkChoiceStore( | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -111,13 +111,13 @@ export async function validateGossipBlock( | |||
}); | ||||
} | ||||
|
||||
// getBlockSlotState also checks for whether the current finalized checkpoint is an ancestor of the block. | ||||
// use getPreState to reload state if needed. It also checks for whether the current finalized checkpoint is an ancestor of the block. | ||||
// As a result, we throw an IGNORE (whereas the spec says we should REJECT for this scenario). | ||||
// this is something we should change this in the future to make the code airtight to the spec. | ||||
// [IGNORE] The block's parent (defined by block.parent_root) has been seen (via both gossip and non-gossip sources) (a client MAY queue blocks for processing once the parent block is retrieved). | ||||
// [REJECT] The block's parent (defined by block.parent_root) passes validation. | ||||
const blockState = await chain.regen | ||||
.getBlockSlotState(parentRoot, blockSlot, {dontTransferCache: true}, RegenCaller.validateGossipBlock) | ||||
.getPreState(block, {dontTransferCache: true}, RegenCaller.validateGossipBlock) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is the same to
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that this will reduce number of calls to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's just the same, if parentEpoch < blockEpoch it'll get checkpoint state. The |
||||
.catch(() => { | ||||
throw new BlockGossipError(GossipAction.IGNORE, {code: BlockErrorCode.PARENT_UNKNOWN, parentRoot}); | ||||
}); | ||||
|
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.
Dumb question: Why don't we need to
regen.init()
in theBeaconChain
constructor?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.
it's a conventional place to do async init, see the
this.opPool.fromPersisted(this.db)
call below