-
Notifications
You must be signed in to change notification settings - Fork 231
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
[WIP] epbs (eip-7732) #6443
base: unstable
Are you sure you want to change the base?
[WIP] epbs (eip-7732) #6443
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to call nimpretty
on them. Thank you
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the # # https://...
intentional?
next_epoch = get_current_epoch(state) + 1 | ||
doAssert epoch <= next_epoch | ||
|
||
for slot in start_slot .. start_slot + SLOTS_PER_EPOCH - 1: |
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.
for slot in start_slot .. start_slot + SLOTS_PER_EPOCH - 1: | |
for slot in start_slot .. <end_slot: |
Isn't it better to take a variable like end_slot = start_slot + SLOTS_PER_EPOCH:
that would be more readable ib.
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.
Does this give any performance advantage? I don't think so
|
Co-authored-by: Md Amaan <114795592+Redidacove@users.noreply.github.com>
Done removed👍 |
Execution Requests redefinition removed
https://github.com/ethereum/consensus-specs/releases/tag/v1.5.0-alpha.7 contains ethereum/consensus-specs#3818 which replaces |
Strange there are conflicts in vendor packages. |
They aren't conflicts in the vendor packages as such, but different versions of the packages specified somehow in the git repository history so that the merge process can't automatically complete. Specifically, the PRs:
|
The files:
should not be part of the git repository. |
.vscode/tasks.json
Outdated
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.
Why was this file deleted?
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.
Why was this file deleted?
I used github codespaces, might have been deleted in Codespaces due to automatic environment configuration.
Current build failure (locally reproducible via
|
:'( |
The CI checks that maximum per-function static stack usage is 1MB: nimbus-eth2/.github/workflows/ci.yml Lines 129 to 138 in ec0accf
|
No description provided.