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

add Capella support to Forked* #4276

Merged
merged 3 commits into from
Nov 2, 2022
Merged

add Capella support to Forked* #4276

merged 3 commits into from
Nov 2, 2022

Conversation

tersec
Copy link
Contributor

@tersec tersec commented Nov 1, 2022

  • a large no-op, essentially, the goal of which is from this point to allow safe, incremental Capella development/fleshing-out within the unstable branch
  • general pattern was to flesh out spec/forks to its almost Capella-final form, but the rest to stub where feasible, to preserve the no-op aspect in a PR which already in an essentially minimal form adds 500 lines and changes 42 files
  • capellaImplementationMissing serves both as a grep (rg, ag, etc) anchor, but also as a compile-time anchor. Once the capellaImplementationMissings in an area of the codebase is complete, and it compiles without it, it implies that the Capella aspects should be done there
  • where the data is under internal control, it does raiseAssert $capellaImplementationMissing to be as visible as possible, because after all, checkForkConsistency() ensures that nimbus_beacon_node can't run yet with a Capella network:
    func checkForkConsistency*(cfg: RuntimeConfig) =
    doAssert cfg.CAPELLA_FORK_EPOCH == FAR_FUTURE_EPOCH
    doAssert cfg.SHARDING_FORK_EPOCH == FAR_FUTURE_EPOCH

However, beacon_chain/spec/eth2_apis/eth2_rest_serialization has places where otherwise, this would allow random unauthenticated external input to crash Nimbus at will, obviously suboptimal -- therefore it returns as a more controlled parsing error, but still both greppable and dependent on this symbol that allows compile-time checking. When writing, it stays with the raiseAssert, because that is under internal control and if that happens, that's a logic error.

  • There's a change between Nim 1.2 and 1.6 whereby
proc f() =
  raiseAssert "foo"
  echo "hello world"

changes from an error around the unreachable echo statement in Nim 1.2 to a warning. Therefore, in several places, for compatibility between Nim 1.2 and 1.6, this PR uses:

proc f() =
  if true: raiseAssert "foo"
  echo "hello world"

@github-actions
Copy link

github-actions bot commented Nov 1, 2022

Unit Test Results

         9 files  ±    0       849 suites  ±0   23m 31s ⏱️ - 1m 7s
  2 481 tests +  96    2 236 ✔️ ±0  245 💤 +  96  0 ±0 
10 970 runs  +144  10 653 ✔️ ±0  317 💤 +144  0 ±0 

Results for commit fa830d1. ± Comparison against base commit 3ef09ff.

@@ -36,7 +39,7 @@ proc addResolvedHeadBlock(
trustedBlock: ForkyTrustedSignedBeaconBlock,
blockVerified: bool,
parent: BlockRef, cache: var StateCache,
onBlockAdded: OnPhase0BlockAdded | OnAltairBlockAdded | OnBellatrixBlockAdded,
onBlockAdded: OnPhase0BlockAdded | OnAltairBlockAdded | OnBellatrixBlockAdded | OnCapellaBlockAdded,
Copy link
Member

@arnetheduck arnetheduck Nov 2, 2022

Choose a reason for hiding this comment

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

time for OnForkyBlockAdded I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't something I do routinely now, since nim-lang/Nim#18095 still isn't fixed and it's a gamble each time whether attempting to do so will have been a waste of time, but in this case, bf785d6 seems to work on both Nim 1.2 and Nim 1.6.

@@ -120,6 +124,9 @@ proc loadForkedState*(
# prevent temporaries created by case objects
let forkedState = new ForkedHashedBeaconState
case fork
of BeaconStateFork.Capella:
doAssert capellaImplementationMissing
Copy link
Member

Choose a reason for hiding this comment

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

cruft

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tersec tersec merged commit 5b46f0b into unstable Nov 2, 2022
@tersec tersec deleted the dXn branch November 2, 2022 16:23
henridf added a commit to henridf/nimbus-eth2 that referenced this pull request Nov 28, 2022
This commit adds the EIP-4844 spec types, and fills in
scaffolding/boilerplate for the use of these types across the repo.

None of the actual EIP-4844 logic is introduced yet.

This follows the pattern used by @tersec when introducing Capella (status-im#4276).
tersec pushed a commit that referenced this pull request Dec 5, 2022
* Types and scaffolding for EIP-4844

This commit adds the EIP-4844 spec types, and fills in
scaffolding/boilerplate for the use of these types across the repo.

None of the actual EIP-4844 logic is introduced yet.

This follows the pattern used by @tersec when introducing Capella (#4276).

* use eth2-networks fork

* review feedback: add static check EIP4844_FORK_EPOCH == FAR_FUTURE_EPOCH

* review feedback: remove EIP4844 from /eth/v1/config/spec response

* Cleanup / review feedback

* Fix REST test
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.

2 participants