Skip to content
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

Open
wants to merge 62 commits into
base: unstable
Choose a base branch
from
Open

[WIP] epbs (eip-7732) #6443

wants to merge 62 commits into from

Conversation

Tomi-3-0
Copy link
Contributor

No description provided.

@Tomi-3-0 Tomi-3-0 changed the title Initial dummy commit epbs (eip-7732) implementation Jul 23, 2024
@Tomi-3-0 Tomi-3-0 changed the title epbs (eip-7732) implementation [WIP] epbs (eip-7732) implementation Jul 27, 2024
@Tomi-3-0 Tomi-3-0 changed the title [WIP] epbs (eip-7732) implementation [WIP] epbs (eip-7732) Jul 27, 2024
Copy link

github-actions bot commented Jul 27, 2024

Unit Test Results

0 files   -          9  0 suites   - 1 355   0s ⏱️ - 38m 26s
0 tests  -   5 218  0 ✔️  -   4 870  0 💤  - 348  0 ±0 
0 runs   - 21 738  0 ✔️  - 21 334  0 💤  - 404  0 ±0 

Results for commit af91e67. ± Comparison against base commit ab4574e.

♻️ This comment has been updated with latest results.

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.
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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.

@tersec
Copy link
Contributor

tersec commented Aug 6, 2024

Merge branch 'status-im:stable' into epbs

the development branches should generally track unstable, not stable

@Tomi-3-0
Copy link
Contributor Author

Tomi-3-0 commented Aug 6, 2024

Merge branch 'status-im:stable' into epbs

the development branches should generally track unstable, not stable

tracks unstable from my end:
...wants to merge 22 commits into status-im:unstable from Tomi-3-0:epbs

indexed_payload_attestation.signature)

# # https://github.com/ethereum/consensus-specs/blob/1508f51b80df5488a515bfedf486f98435200e02/specs/_features/eipxxxx/beacon-chain.md#is_parent_block_full
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the # # https://... intentional?

@tersec
Copy link
Contributor

tersec commented Aug 11, 2024

Are the deletions of NimYAML, gnosis-chain-configs, nim-libbacktrace, nim-libp2p, and sepolia vendored git subrepositories intentional?

2024-08-11T03:19:16,587536883+00:00

next_epoch = get_current_epoch(state) + 1
doAssert epoch <= next_epoch

for slot in start_slot .. start_slot + SLOTS_PER_EPOCH - 1:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Contributor Author

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

beacon_chain/spec/datatypes/epbs.nim Outdated Show resolved Hide resolved
@tersec
Copy link
Contributor

tersec commented Oct 8, 2024

2024-10-08T03:19:48.2521970Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/spec/datatypes/electra.nim(306, 3) Error: redefinition of 'ExecutionRequests'; previous declaration here: /github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/spec/datatypes/electra.nim(193, 3)

tersec and others added 2 commits October 8, 2024 03:31
Co-authored-by: Md Amaan <114795592+Redidacove@users.noreply.github.com>
@Redidacove
Copy link

Redidacove commented Oct 8, 2024

2024-10-08T03:19:48.2521970Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/spec/datatypes/electra.nim(306, 3) Error: redefinition of 'ExecutionRequests'; previous declaration here: /github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/spec/datatypes/electra.nim(193, 3)

Done removed👍

Execution Requests redefinition removed
@tersec
Copy link
Contributor

tersec commented Oct 8, 2024

2024-10-08T12:12:55.6224986Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/spec/datatypes/epbs.nim(37, 23) Error: undeclared identifier: 'PendingBalanceDeposit'
2024-10-08T12:12:55.6458434Z make: *** [Makefile:447: wss_sim] Error 1
--
2024-10-08T12:12:56.6417447Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/spec/datatypes/epbs.nim(37, 23) Error: undeclared identifier: 'PendingBalanceDeposit'
2024-10-08T12:12:56.6684431Z make: *** [Makefile:447: ncli_db] Error 1
--
2024-10-08T12:12:57.2701357Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/spec/datatypes/epbs.nim(37, 23) Error: undeclared identifier: 'PendingBalanceDeposit'
2024-10-08T12:12:57.2781920Z make: *** [Makefile:447: deposit_contract] Error 1
2024-10-08T12:12:57.3029205Z make: *** [Makefile:447: ncli] Error 1
--
2024-10-08T12:12:58.3929001Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/spec/datatypes/epbs.nim(37, 23) Error: undeclared identifier: 'PendingBalanceDeposit'
2024-10-08T12:12:58.4317242Z make: *** [Makefile:447: nimbus_validator_client] Error 1
2024-10-08T12:12:58.6439010Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/spec/datatypes/epbs.nim(37, 23) Error: undeclared identifier: 'PendingBalanceDeposit'
2024-10-08T12:12:58.6893775Z make: *** [Makefile:447: mev_mock] Error 1
--
2024-10-08T12:13:03.5207018Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/spec/datatypes/epbs.nim(37, 23) Error: undeclared identifier: 'PendingBalanceDeposit'
2024-10-08T12:13:03.5736294Z make: *** [Makefile:447: nimbus_light_client] Error 1

https://github.com/ethereum/consensus-specs/releases/tag/v1.5.0-alpha.7 contains ethereum/consensus-specs#3818 which replaces PendingBalanceDeposit with PendingDeposit

@tersec
Copy link
Contributor

tersec commented Oct 9, 2024

Haven't checked in detail yet, but something went awry with that merge. Some conflicts that I don't think were there before:
2024-10-09T00:31:48,158859098+00:00

@Redidacove
Copy link

Strange there are conflicts in vendor packages.

@tersec
Copy link
Contributor

tersec commented Oct 9, 2024

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:

@tersec
Copy link
Contributor

tersec commented Oct 14, 2024

The files:

  • the binary executable tests/consensus_spec/bellatrix/test_fixture_operations (the one without the .nim extension);
  • the binary executable tests/consensus_spec/electra/test_fixture_operations (ditto, the one without the .nim extension)
  • the binary executable beacon_chain/spec/helpers (also, the one without the .nim extension); and
  • ubmodule update --init --recursive (as in, there's a file literally named ubmodule update --init --recursive in this branch)

should not be part of the git repository.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

@tersec
Copy link
Contributor

tersec commented Oct 14, 2024

Current build failure (locally reproducible via make all_tests):

/github-runner/workspace/nimbus-eth2/nimbus-eth2/tests/test_helpers.nim(276, 33) Error: type mismatch
Expression: remove_flag(flag, flag_index)
  [1] flag: uint8
  [2] flag_index: int

Expected one of (first mismatch at [position]):
[2] func remove_flag(flags: ParticipationFlags; flag_index: TimelyFlag): ParticipationFlags

@Tomi-3-0 Tomi-3-0 closed this Oct 14, 2024
@Tomi-3-0 Tomi-3-0 reopened this Oct 14, 2024
@Tomi-3-0
Copy link
Contributor Author

Current build failure (locally reproducible via make all_tests):

/github-runner/workspace/nimbus-eth2/nimbus-eth2/tests/test_helpers.nim(276, 33) Error: type mismatch
Expression: remove_flag(flag, flag_index)
  [1] flag: uint8
  [2] flag_index: int

Expected one of (first mismatch at [position]):
[2] func remove_flag(flags: ParticipationFlags; flag_index: TimelyFlag): ParticipationFlags

:'(

@tersec
Copy link
Contributor

tersec commented Oct 16, 2024

/github-runner/workspace/nimbus-eth2/nimbus-eth2/vendor/nim-unittest2/unittest2.nim: In function ‘_ZN23test_message_signatures20runTestX60gensym450_E6string6string’:
/github-runner/workspace/nimbus-eth2/nimbus-eth2/vendor/nim-unittest2/unittest2.nim:1101:15: error: stack usage might be 5658432 bytes [-Werror=stack-usage=]
 1101 |   proc runTest(suiteName, testName: string): TestStatus {.raises: [], gensym.} =
      |               ^
/github-runner/workspace/nimbus-eth2/nimbus-eth2/vendor/nim-unittest2/unittest2.nim: In function ‘_ZN23test_message_signatures20runTestX60gensym509_E6string6string’:
/github-runner/workspace/nimbus-eth2/nimbus-eth2/vendor/nim-unittest2/unittest2.nim:1101:15: error: stack usage might be 5658944 bytes [-Werror=stack-usage=]
/github-runner/workspace/nimbus-eth2/nimbus-eth2/vendor/nim-unittest2/unittest2.nim: In function ‘_ZN23test_message_signatures20runTestX60gensym568_E6string6string’:
/github-runner/workspace/nimbus-eth2/nimbus-eth2/vendor/nim-unittest2/unittest2.nim:1101:15: error: stack usage might be 5658352 bytes [-Werror=stack-usage=]
lto1: some warnings being treated as errors
make[2]: *** [/tmp/cckbluCt.mk:263: /tmp/ccXwZhhR.ltrans87.ltrans.o] Error 1
make[2]: *** Waiting for unfinished jobs....
lto-wrapper: fatal error: make returned 2 exit status
compilation terminated.
/usr/bin/ld: error: lto-wrapper failed
collect2: error: ld returned 1 exit status
make[1]: *** [nimcache/release/all_tests/all_tests.makefile:3752: build] Error 1

The CI checks that maximum per-function static stack usage is 1MB:

# Stack usage test and UBSAN on recent enough gcc:
if [[ '${{ runner.os }}' == 'Linux' && '${{ matrix.target.cpu }}' == 'amd64' ]]; then
if [[ '${{ github.sha }}' =~ ^7 ]]; then
export WITH_UBSAN=1
echo "WITH_UBSAN=1" >> $GITHUB_ENV
export NIMFLAGS="${NIMFLAGS} -d:limitStackUsage --passC:-fsanitize=undefined --passL:-fsanitize=undefined"
else
export NIMFLAGS="${NIMFLAGS} -d:limitStackUsage"
fi
fi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants