[WIP] epbs (eip-7732) #6443
[WIP] epbs (eip-7732) #6443
Conversation
Tomi-3-0
changed the title
| proc is_valid_indexed_payload_attestation( | ||
| state: capella.BeaconState, # [TODO] to be replaced with epbs.BeaconState | ||
| indexed_payload_attestation: IndexedPayloadAttestation): bool = | ||
| # Check if ``indexed_payload_attestation`` is not empty, has sorted and unique indices, and has a valid aggregate signature. |
There was a problem hiding this comment.
https://status-im.github.io/nim-style-guide/formatting.style.html
We strive to follow NEP-1 for style matters - naming, capitalization, 80-character limit etc.
This comment line is quite a bit longer than 80 characters.
This doesn't mean split (break) all the URLs, etc, or other things which for intrinsic reasons just must have longer than 80 character lines, but where it's feasible/practical to wrap at 80, that's the goal.
There was a problem hiding this comment.
I forgot to call nimpretty on them. Thank you
There was a problem hiding this comment.
Regarding nimpretty, see https://status-im.github.io/nim-style-guide/formatting.style.html#practical-notes
We do not use nimpretty - as of writing (Nim 1.6), it is not stable enough for daily use:
- Can break working code
- Naive formatting algorithm
if it works for you, that's great, but it has many flaws, so shouldn't be trusted too much, and shouldn't be used to reformat existing Nimbus code.
the development branches should generally track |
tracks unstable from my end: |
| indexed_payload_attestation.signature) | ||
|
|
||
| # # https://github.com/ethereum/consensus-specs/blob/1508f51b80df5488a515bfedf486f98435200e02/specs/_features/eipxxxx/beacon-chain.md#is_parent_block_full |
There was a problem hiding this comment.
is the # # https://... intentional?
|
Are the deletions of
|
|
Basically I think ef50df0 should be reconsidered |
This makes a lot of sense. Thank you! |
This reverts commit ef50df0.
| {.push raises: [].} | ||
|
|
||
| # Minimal preset - epbs | ||
| # https://github.com/ethereum/consensus-specs/blob/1508f51b80df5488a515bfedf486f98435200e02/specs/_features/eipxxxx/beacon-chain.md#preset |
There was a problem hiding this comment.
| default(ExitQueueInfo) # not used | ||
|
|
||
| bsv_use = | ||
| when typeof(body).kind >= ConsensusFork.Electra: |
There was a problem hiding this comment.
This when will always be true. ePBS won't happen before Electra
| cfg, state, op, flags, exit_queue_info, cache | ||
| ) | ||
|
|
||
| when typeof(body).kind >= ConsensusFork.Capella: |
There was a problem hiding this comment.
This when will always be true. ePBS won't happen before Capella
|
|
||
| # https://github.com/ethereum/consensus-specs/blob/v1.5.0-alpha.4/specs/_features/eip7732/beacon-chain.md#modified-process_execution_payload | ||
| proc process_execution_payload*( | ||
| cfg: RuntimeConfig, body: SomeForkyBeaconBlockBody, |
There was a problem hiding this comment.
This can't apply to any random ForkyBlindedBeaconBlockBody, because it has to be a body which applies to an epbs.BeaconState, which means it must be an ePBS block body. No generics necessary/useful here.
|
|
||
| var | ||
| bsv_use = | ||
| when typeof(body).kind >= ConsensusFork.Electra: |
There was a problem hiding this comment.
This when will always be true. ePBS won't happen before Electra
| else: | ||
| nil # this is a logic error, effectively assert | ||
|
|
||
| when typeof(body).kind >= ConsensusFork.Electra: |
There was a problem hiding this comment.
This when will always be true. ePBS won't happen before Electra
| state, indexed_payload_attestation): | ||
| return err("process_payload_attestation: signature verification failed") | ||
|
|
||
| var epochParticipation: EpochParticipationFlags |
There was a problem hiding this comment.
This whole section can be more safely phrased as
let epochParticipation =
if state.slot mod SLOTS_PER_EPOCH == 0:
state.previous_epoch_participation
else:
state.current_epoch_participationi.e. a https://nim-lang.org/docs/manual.html#statements-and-expressions-if-expression instead of a https://nim-lang.org/docs/manual.html#statements-and-expressions-if-statement
which works with https://status-im.github.io/nim-style-guide/language.vardecl.html
There was a problem hiding this comment.
I think var epochParticipation should still be used since it needs to be passed as a mutable structure, changing just the let to var like so:
var epochParticipation =
if state.slot mod SLOTS_PER_EPOCH == 0:
state.previous_epoch_participation
else:
state.current_epoch_participation
There was a problem hiding this comment.
One issue though is that if the resulting epochParticipation local variable is modified, the underlying state is not. So it won't accomplish that goal.
|
|
||
| for i, index in ptc.pairs: | ||
| if payload_attestation.aggregation_bits[i]: | ||
| output[pos] = index |
There was a problem hiding this comment.
var
output: List[ValidatorIndex, Limit PTC_SIZE]constructs a default 0-item list (it's basically the same thing as a seq[ValidatorIndex] for Nim), so there aren't any existing elements to index into. This line is guaranteed to generate an index out of range defect.
beacon_chain/spec/signatures.nim
Outdated
| genesisFork: Fork, genesis_validators_root: Eth2Digest, | ||
| msg: epbs.SignedExecutionPayloadHeader): Eth2Digest = | ||
| # So the epoch doesn't matter when calling get_domain | ||
| doAssert genesisFork.previous_version == genesisFork.current_version |
There was a problem hiding this comment.
The execution payload headers are not fork-agnostic and this condition won't in general hold. https://github.com/ethereum/consensus-specs/blob/v1.5.0-alpha.5/specs/_features/eip7732/beacon-chain.md#new-verify_execution_payload_header_signature says:
def verify_execution_payload_header_signature(state: BeaconState,
signed_header: SignedExecutionPayloadHeader) -> bool:
# Check the signature
builder = state.validators[signed_header.message.builder_index]
signing_root = compute_signing_root(signed_header.message, get_domain(state, DOMAIN_BEACON_BUILDER))
return bls.Verify(builder.pubkey, signing_root, signed_header.signature)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)That is, when it's called from verify_execution_payload_header_signature this way, with two arguments, get_domain does not use GENESIS_EPOCH but rather get_current_epoch(state).
The signatures are a function of fork in this case, and that needs to be reflected in this compute_execution_payload_header_signing_root function.
beacon_chain/spec/signatures.nim
Outdated
|
|
||
| # https://github.com/ethereum/consensus-specs/blob/v1.5.0-alpha.4/specs/_features/eip7732/beacon-chain.md#new-verify_execution_payload_envelope_signature | ||
| func compute_execution_payload_envelope_signing_root*( | ||
| genesisFork: Fork, genesis_validators_root: Eth2Digest, |
There was a problem hiding this comment.
Also not genesis fork.
def verify_execution_payload_envelope_signature(
state: BeaconState, signed_envelope: SignedExecutionPayloadEnvelope) -> bool:
builder = state.validators[signed_envelope.message.builder_index]
signing_root = compute_signing_root(signed_envelope.message, get_domain(state, DOMAIN_BEACON_BUILDER))
return bls.Verify(builder.pubkey, signing_root, signed_envelope.signature)so it's another two-parameter call to get_domain(...), regarding which see https://github.com/status-im/nimbus-eth2/pull/6443/files#r1756088823
beacon_chain/spec/signatures.nim
Outdated
| compute_signing_root(msg.message, domain) | ||
|
|
||
| func get_execution_payload_header_signature*( | ||
| genesisFork: Fork, genesis_validators_root: Eth2Digest, |
There was a problem hiding this comment.
None of these are genesisFork (in get_execution_payload_header_signature, verify_execution_payload_header_signature, compute_execution_payload_envelope_signing_root, etc)
beacon_chain/spec/signatures.nim
Outdated
| genesisFork: Fork, genesis_validators_root: Eth2Digest, | ||
| msg: epbs.SignedExecutionPayloadEnvelope): Eth2Digest = | ||
| # So the epoch doesn't matter when calling get_domain | ||
| doAssert genesisFork.previous_version == genesisFork.current_version |
There was a problem hiding this comment.
Assertion will in general not hold true, because not genesis fork. That's basically what it's testing for; there's by definition no previous fork for the genesis fork
| state.latest_full_slot = state.slot | ||
|
|
||
| if verify: | ||
| assert envelope.state_root == hash_tree_root(state) |
There was a problem hiding this comment.
Should use https://nim-lang.org/docs/assertions.html#doAssert.t%2Cuntyped%2Cstring rather than https://nim-lang.org/docs/assertions.html#assert.t%2Cuntyped%2Cstring because the former:
is always turned on regardless of
--assertions.
No description provided.