status-im / nimbus-eth2

Nim implementation of the Ethereum Beacon Chain
https://nimbus.guide
Other
523 stars 227 forks source link

[WIP] epbs (eip-7732) #6443

Open Tomi-3-0 opened 2 months ago

github-actions[bot] commented 2 months ago

Unit Test Results

         9 files  ±    0    1 355 suites  +18   38m 20s :stopwatch: - 5m 6s   5 149 tests +  83    4 801 :heavy_check_mark: +  83  348 :zzz: ±0  0 :x: ±0  21 516 runs  +519  21 112 :heavy_check_mark: +519  404 :zzz: ±0  0 :x: ±0 

Results for commit da6a4767. ± Comparison against base commit f9e44b2a.

:recycle: This comment has been updated with latest results.

tersec commented 1 month ago

Merge branch 'status-im:stable' into epbs

the development branches should generally track unstable, not stable

Tomi-3-0 commented 1 month ago

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

tersec commented 1 month ago

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

Tomi-3-0 commented 1 month ago

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

Yes, they are. Not sure if I was supposed to push those on to github

tersec commented 1 month ago

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

Yes, they are. Not sure if I was supposed to push those on to github

Well, this branch/PR/commit sequence should contain the changes, and unless you're changing those dependencies, they should be left unaltered. That is, by keeping them around, you're not pushing them onto GitHub exactly [1, for caveats, but this is conceptually true for how one generally uses Git].

As is, if one just merges this branch into unstable it will break the unstable branch and makes it trickier to review and to update/rebase/merge into newer versions of unstable.

So it doesn't help and does hinder development to remove some of the subrepositories in this PR.

[1] Well, at some level, sort of are: one can look up details of how Git stores submodule information and how it stores files when they're uploaded each time as part of a commit. The short version is, it uses a kind of sorting then run-length compression of identical blob [read: file, sort of] contents in a way which makes the additional commit's "copy" basically free. So at a sufficiently plumbing level, yes, it's uploading pointers-to-subrepositories, but at a porcelain level, that's how Git works, and it's more disruptive to overall workflows to remove files than do nothing at all.

Tomi-3-0 commented 1 month ago

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

Yes, they are. Not sure if I was supposed to push those on to github

Well, this branch/PR/commit sequence should contain the changes, and unless you're changing those dependencies, they should be left unaltered. That is, by keeping them around, you're not pushing them onto GitHub exactly [1, for caveats, but this is conceptually true for how one generally uses Git].

As is, if one just merges this branch into unstable it will break the unstable branch and makes it trickier to review and to update/rebase/merge into newer versions of unstable.

  • when one runs ./env.sh nim c beacon_chain/spec/payload_attestations.nim, it displays the errors:
    /usr/bin/ld: cannot find nimbus-eth2/vendor/nim-libbacktrace/install/usr/lib/libbacktracenim.a: No such file or directory
    /usr/bin/ld: cannot find nimbus-eth2/vendor/nim-libbacktrace/install/usr/lib/libbacktrace.a: No such file or directory
    collect2: error: ld returned 1 exit status

    and does not properly build because of the removal of nim-libbacktrace

  • by removing sepolia, you're creating an artificial merge conflict unrelated to the substantive contents of this PR, because there was a sepolia update: 2024-08-11T23:59:31,930163907+00:00 and because of that, several of the CIs can't run (because they first rebase things to unstable).

So it doesn't help and does hinder development to remove some of the subrepositories in this PR.

[1] Well, at some level, sort of are: one can look up details of how Git stores submodule information and how it stores files when they're uploaded each time as part of a commit. The short version is, it uses a kind of sorting then run-length compression of identical blob [read: file, sort of] contents in a way which makes the additional commit's "copy" basically free. So at a sufficiently plumbing level, yes, it's uploading pointers-to-subrepositories, but at a porcelain level, that's how Git works, and it's more disruptive to overall workflows to remove files than do nothing at all.

So detailed. Thank you

tersec commented 1 month ago

The CI still isn't running, partly because of the merge conflicts. 2024-08-15T15:41:02,100365934+00:00

https://github.com/status-im/nimbus-eth2/pull/6443/commits/b922f277ab3c2b920a9b9e681e034752a82919d0 does add back the removed submodules, but doesn't fully address sepolia; if nothing else, it seems to move all five submodules to slightly different commits than they started from: 2024-08-15T15:40:08,096645337+00:00

tersec commented 1 month ago

CI error from https://github.com/status-im/nimbus-eth2/actions/runs/10436092895/job/28915055678?pr=6443:

/github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/networking/network_metadata.nim(312, 53)
/github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/networking/network_metadata.nim(231, 11) loadCompileTimeNetworkMetadata
/github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/networking/network_metadata.nim(231, 11) Error: config.yaml not found for network '/github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/networking/../../vendor/sepolia/metadata
make: *** [Makefile:447: wss_sim] Error 1

It's because the vendor/sepolia submodule isn't correct.

tersec commented 1 month ago

Current CI build error from https://github.com/status-im/nimbus-eth2/actions/runs/10485514869/job/29049260286?pr=6443

2024-08-21T10:33:59.3219729Z ...................................................../github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/networking/eth2_network.nim(26, 18) Error: cannot open file: libp2p/services/wildcardresolverservice
2024-08-21T10:33:59.4585071Z ........................./github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/networking/eth2_network.nim(26, 18) Error: cannot open file: libp2p/services/wildcardresolverservice
2024-08-21T10:33:59.5158541Z .........make: *** [Makefile:447: mev_mock] Error 1
2024-08-21T10:33:59.5159233Z make: *** Waiting for unfinished jobs....
2024-08-21T10:33:59.6455231Z .......make: *** [Makefile:447: ncli_testnet] Error 1
--
2024-08-21T10:34:06.6268785Z Build completed successfully: build/ncli_split_keystore
2024-08-21T10:34:06.8125123Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/networking/eth2_network.nim(26, 18) Error: cannot open file: libp2p/services/wildcardresolverservice
2024-08-21T10:34:07.0563211Z make: *** [Makefile:447: nimbus_light_client] Error 1

i.e. the nim-libp2p commit isn't correct and should match what's currently in unstable, which does have this libp2p/services/wildcardresolverservice module.

Tomi-3-0 commented 1 month ago

Okay

tersec commented 1 month ago

Same issue after https://github.com/status-im/nimbus-eth2/pull/6443/commits/7a8f3d28671381a8c6ee8ec39c6d0bf47528029f from https://github.com/status-im/nimbus-eth2/actions/runs/10495776698/job/29095113051?pr=6443

2024-08-22T06:41:11.3486196Z ....../github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/networking/eth2_network.nim(26, 18) Error: cannot open file: libp2p/services/wildcardresolverservice
2024-08-22T06:41:11.5353221Z make: *** [Makefile:447: mev_mock] Error 1
2024-08-22T06:41:11.5354120Z make: *** Waiting for unfinished jobs....
2024-08-22T06:41:11.7260264Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/networking/eth2_network.nim(26, 18) Error: cannot open file: libp2p/services/wildcardresolverservice
2024-08-22T06:41:11.8989887Z ...................make: *** [Makefile:447: ncli_testnet] Error 1
2024-08-22T06:41:14.4235842Z ................
2024-08-22T06:41:14.4237608Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/vendor/nim-ssz-serialization/ssz_serialization/digest.nim(35, 9) Hint: BLST SHA256 backend enabled [User]
2024-08-22T06:41:14.4245272Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/vendor/nim-ssz-serialization/ssz_serialization/digest.nim(49, 9) Hint: Hashtree SHA256 backend enabled [User]
2024-08-22T06:41:17.9723237Z Hint: mm: refc; threads: on; opt: none (DEBUG BUILD, `-d:release` generates faster code)
2024-08-22T06:41:17.9725687Z 261407 lines; 108.718s; 3.389GiB peakmem; proj: /github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/nimbus_validator_client.nim; out: /github-runner/workspace/nimbus-eth2/nimbus-eth2/nimcache/release/nimbus_validator_client/nimbus_validator_client.json [SuccessX]
2024-08-22T06:41:18.4726446Z Build completed successfully: build/ncli
2024-08-22T06:41:19.3225322Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/networking/eth2_network.nim(26, 18) Error: cannot open file: libp2p/services/wildcardresolverservice
2024-08-22T06:41:19.4667763Z Build completed successfully: build/ncli_split_keystore
2024-08-22T06:41:19.5530132Z make: *** [Makefile:447: nimbus_light_client] Error 1

Can test this locally by running, e.g., make nimbus_light_client.

https://github.com/status-im/nimbus-eth2/tree/unstable/vendor shows what nim-libp2p commit unstable is expecting: https://github.com/vacp2p/nim-libp2p/tree/b5fb7b3a97d8977d969d786633f70c4094cd0eaf which indeed has https://github.com/vacp2p/nim-libp2p/blob/b5fb7b3a97d8977d969d786633f70c4094cd0eaf/libp2p/services/wildcardresolverservice.nim

Tomi-3-0 commented 1 month ago

Okay.

tersec commented 1 month ago

https://github.com/status-im/nimbus-eth2/actions/runs/10542331340/job/29213098600?pr=6443

...
2024-08-25T05:06:54.2440825Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/spec/datatypes/epbs.nim(61, 52) Error: undeclared identifier: 'PTC_SIZE'
2024-08-25T05:06:54.2442766Z candidates (edit distance, scope distance); see '--spellSuggest': 
2024-08-25T05:06:54.2443694Z  (2, 3): 'csize'
2024-08-25T05:06:54.2695466Z make: *** [Makefile:647: gnosis-build] Error 1
...

because https://github.com/Tomi-3-0/nimbus-eth2/tree/epbs/beacon_chain/spec/presets/gnosis needs the same PTC_SIZE and MAX_PAYLOAD_ATTESTATIONS presets as the EF mainnet and minimal ePBS presets.

Practically, it's fine to just copy https://github.com/Tomi-3-0/nimbus-eth2/blob/epbs/beacon_chain/spec/presets/mainnet/epbs_preset.nim for this purpose, maybe replacing "Mainnet preset" comments with "Gnosis preset", but the substantive contents can be identical since as far as I know, Gnosis hasn't taken a position on this so Nimbus just needs reasonable placeholders here for now.

Tomi-3-0 commented 1 month ago

Thanks

tersec commented 2 weeks ago

https://github.com/status-im/nimbus-eth2/actions/runs/10752309189/job/29884832856?pr=6443

2024-09-09T16:08:28.1595661Z stack trace: (most recent call last)
2024-09-09T16:08:28.1599116Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/networking/network_metadata.nim(324, 11) network_metadata
2024-09-09T16:08:28.1602221Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/vendor/nimbus-build-system/vendor/Nim/lib/std/assertions.nim(41, 14) failedAssertImpl
2024-09-09T16:08:28.1605340Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/vendor/nimbus-build-system/vendor/Nim/lib/std/assertions.nim(36, 13) raiseAssert
2024-09-09T16:08:28.1607975Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/vendor/nimbus-build-system/vendor/Nim/lib/system/fatal.nim(53, 5) sysFatal
2024-09-09T16:08:28.1614180Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/vendor/nimbus-build-system/vendor/Nim/lib/system/fatal.nim(53, 5) Error: unhandled exception: /github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/networking/network_metadata.nim(324, 15) `ConsensusFork.high == ConsensusFork.Electra`  [AssertionDefect]
2024-09-09T16:08:28.2286000Z make: *** [Makefile:447: nimbus_beacon_node] Error 1

can replicate locally by running make nimbus_beacon_node.

General risk with the adding the ConsensusFork: it makes rebasing, etc more difficult. It's a very invasive change and practically, when Nimbus does hardfork updates, I try to separate out sequences of minimal-risk changes, rather than one big feature branch of sorts. This one error is just the beginning -- yes, of course, it's possible to fix, but then there's the next one and the next one and ...

It depends how ePBS should be tested -- my inclination is to probably treat it as a kind of part of the the Electra fork (but declared symbolically, const EbpsFork = ConsensusFork.Electra and systematically using that instead, or similar, to avoid confusion of actually-electra and ePBS), and just figure that this branch can't yet be used properly as part of unstable. Then if/when it's clear which fork it's part of, switch that EpbsFork to the fork in question, after it's already added to ConsensusFork. Combining the two will probably get messy.

We're not going to merge a tentative ConsensusFork anyway so by including that it means it's not going to be merge able anyway.

tersec commented 2 weeks ago

Basically I think https://github.com/status-im/nimbus-eth2/pull/6443/commits/ef50df0b4cf9a074207e6b2fb67b32aab56cec8e should be reconsidered

Tomi-3-0 commented 2 weeks ago

https://github.com/status-im/nimbus-eth2/actions/runs/10752309189/job/29884832856?pr=6443

2024-09-09T16:08:28.1595661Z stack trace: (most recent call last)
2024-09-09T16:08:28.1599116Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/networking/network_metadata.nim(324, 11) network_metadata
2024-09-09T16:08:28.1602221Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/vendor/nimbus-build-system/vendor/Nim/lib/std/assertions.nim(41, 14) failedAssertImpl
2024-09-09T16:08:28.1605340Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/vendor/nimbus-build-system/vendor/Nim/lib/std/assertions.nim(36, 13) raiseAssert
2024-09-09T16:08:28.1607975Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/vendor/nimbus-build-system/vendor/Nim/lib/system/fatal.nim(53, 5) sysFatal
2024-09-09T16:08:28.1614180Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/vendor/nimbus-build-system/vendor/Nim/lib/system/fatal.nim(53, 5) Error: unhandled exception: /github-runner/workspace/nimbus-eth2/nimbus-eth2/beacon_chain/networking/network_metadata.nim(324, 15) `ConsensusFork.high == ConsensusFork.Electra`  [AssertionDefect]
2024-09-09T16:08:28.2286000Z make: *** [Makefile:447: nimbus_beacon_node] Error 1

can replicate locally by running make nimbus_beacon_node.

General risk with the adding the ConsensusFork: it makes rebasing, etc more difficult. It's a very invasive change and practically, when Nimbus does hardfork updates, I try to separate out sequences of minimal-risk changes, rather than one big feature branch of sorts. This one error is just the beginning -- yes, of course, it's possible to fix, but then there's the next one and the next one and ...

It depends how ePBS should be tested -- my inclination is to probably treat it as a kind of part of the the Electra fork (but declared symbolically, const EbpsFork = ConsensusFork.Electra and systematically using that instead, or similar, to avoid confusion of actually-electra and ePBS), and just figure that this branch can't yet be used properly as part of unstable. Then if/when it's clear which fork it's part of, switch that EpbsFork to the fork in question, after it's already added to ConsensusFork. Combining the two will probably get messy.

We're not going to merge a tentative ConsensusFork anyway so by including that it means it's not going to be merge able anyway.

This makes a lot of sense. Thank you!