Consider state slot in getDomain #4430
Merged
Conversation
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
Performance Report✔️ no performance regression detected Full benchmark results
|
dapplion
reviewed
Aug 17, 2022
dapplion
reviewed
Aug 17, 2022
dapplion
suggested changes
Aug 17, 2022
There was a problem hiding this comment.
I'm not sure this solution is correct. What about doing this?
Pushed the solution only on config package here https://github.com/ChainSafe/lodestar/compare/cayman/getDomain-alternative?expand=1
return {
getDomain(domainType: DomainType, slot: Slot, state?: {slot: Slot}): Uint8Array {
// ```py
// def get_domain(state: BeaconState, domain_type: DomainType, epoch: Epoch=None) -> Domain:
// """
// Return the signature domain (fork version concatenated with domain type) of a message.
// """
// epoch = get_current_epoch(state) if epoch is None else epoch
// fork_version = state.fork.previous_version if epoch < state.fork.epoch else state.fork.current_version
// return compute_domain(domain_type, fork_version, state.genesis_validators_root)
// ```
// If not state argument is passed, assume requested slot is the state slot
const stateSlot = state?.slot ?? slot;
const epoch = Math.floor(slot / SLOTS_PER_EPOCH);
// Get pre-computed fork schedule, which _should_ match the one in the state
const forkInfo = chainForkConfig.getForkInfo(stateSlot);
// Only allow to select either current or previous fork respective of the fork schedule at stateSlot
const forkName = epoch < forkInfo.epoch ? forkInfo.prevForkName : forkInfo.name;
let domainByType = domainCache.get(forkName);
if (!domainByType) {
domainByType = new Map<DomainType, Uint8Array>();
domainCache.set(forkInfo.name, domainByType);
}
let domain = domainByType.get(domainType);
if (!domain) {
domain = computeDomain(domainType, forkInfo.version, genesisValidatorsRoot);
domainByType.set(domainType, domain);
}
return domain;
},
wemeetagain
force-pushed
the
cayman/getDomain
branch
from
eb987aa
to
8345d05
Compare
wemeetagain
changed the title
|
refactored similarly to your solution, please rereview |
g11tech
force-pushed
the
cayman/getDomain
branch
from
8345d05
to
44b9216
Compare
29 tasks
g11tech
force-pushed
the
cayman/getDomain
branch
from
869adc2
to
4ddf2ae
Compare
dapplion
reviewed
Aug 20, 2022
| @@ -182,7 +182,7 @@ export class ValidatorStore { | |||
| // Duties are filtered before-hard by doppelganger-safe, this assert should never throw | |||
| this.assertDoppelgangerSafe(pubkey); | |||
|
|
|||
| const proposerDomain = this.config.getDomain(DOMAIN_BEACON_PROPOSER, blindedOrFull.slot); | |||
| const proposerDomain = this.config.getDomain(currentSlot, DOMAIN_BEACON_PROPOSER, blindedOrFull.slot); | |||
There was a problem hiding this comment.
Should this really be the currentSlot? Without checking the specs, sounds it should be the state slot, so block's slot
There was a problem hiding this comment.
currentslot should be same as blindedorfull slot same as state slot for signing block here
There was a problem hiding this comment.
should is not enough, this can lead to bad signatures on the fork slot if there are clock issues. There's no solid reason to not use blindedOrFull.slot
g11tech
approved these changes
Aug 20, 2022
dapplion
approved these changes
Aug 20, 2022
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Resolves #4428
Add a
stateSlot: Slotparameter togetDomain. This matches the ergonomics of the consensus function.Note:
getDomainstill relies on the configured fork schedule rather than the on-chain fork schedule (state.fork)