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

remove block fork guessing/inference from REST JSON decoding #6552

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

Conversation

tersec
Copy link
Contributor

@tersec tersec commented Sep 16, 2024

Better, more sustainable alternative to #6551

Not for September release cycle; would be for October cycle.

Copy link

github-actions bot commented Sep 16, 2024

Unit Test Results

         9 files  ±0    1 355 suites  ±0   52m 40s ⏱️ - 2m 40s
  5 146 tests  - 1    4 798 ✔️  - 1  348 💤 ±0  0 ±0 
21 507 runs   - 3  21 103 ✔️  - 3  404 💤 ±0  0 ±0 

Results for commit 6e60261. ± Comparison against base commit 6ab3568.

♻️ This comment has been updated with latest results.

Comment on lines 2876 to 2881
of ConsensusFork.Phase0: getBlck(phase0.SignedBeaconBlock)
of ConsensusFork.Altair: getBlck(altair.SignedBeaconBlock)
of ConsensusFork.Bellatrix: getBlck(bellatrix.SignedBeaconBlock)
of ConsensusFork.Capella: getBlck(capella.SignedBeaconBlock)
of ConsensusFork.Deneb: getBlck(deneb.SignedBeaconBlock)
of ConsensusFork.Electra: getBlck(electra.SignedBeaconBlock)
Copy link
Contributor

Choose a reason for hiding this comment

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

getBlck(consensusFork.SignedBeaconBlock) avoids the switch as well and makes this maintenance free

Copy link
Contributor

Choose a reason for hiding this comment

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

(withConsensusFork(consensusFork) to convert consensusFork from let to const)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, using the usual-fake-Nim-feature which has to be defined in forks and which Nim seems not to care much about supporting.

But sure, as long as that's maintained, then it can be used here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

template BeaconState*(kind: static ConsensusFork): auto =
when kind == ConsensusFork.Electra:
typedesc[electra.BeaconState]
elif kind == ConsensusFork.Deneb:
typedesc[deneb.BeaconState]
elif kind == ConsensusFork.Capella:
typedesc[capella.BeaconState]
elif kind == ConsensusFork.Bellatrix:
typedesc[bellatrix.BeaconState]
elif kind == ConsensusFork.Altair:
typedesc[altair.BeaconState]
elif kind == ConsensusFork.Phase0:
typedesc[phase0.BeaconState]
else:
static: raiseAssert "Unreachable"
template BeaconBlock*(kind: static ConsensusFork): auto =
when kind == ConsensusFork.Electra:
typedesc[electra.BeaconBlock]
elif kind == ConsensusFork.Deneb:
typedesc[deneb.BeaconBlock]
elif kind == ConsensusFork.Capella:
typedesc[capella.BeaconBlock]
elif kind == ConsensusFork.Bellatrix:
typedesc[bellatrix.BeaconBlock]
elif kind == ConsensusFork.Altair:
typedesc[altair.BeaconBlock]
elif kind == ConsensusFork.Phase0:
typedesc[phase0.BeaconBlock]
else:
static: raiseAssert "Unreachable"
template BeaconBlockBody*(kind: static ConsensusFork): auto =
when kind == ConsensusFork.Electra:
typedesc[electra.BeaconBlockBody]
elif kind == ConsensusFork.Deneb:
typedesc[deneb.BeaconBlockBody]
elif kind == ConsensusFork.Capella:
typedesc[capella.BeaconBlockBody]
elif kind == ConsensusFork.Bellatrix:
typedesc[bellatrix.BeaconBlockBody]
elif kind == ConsensusFork.Altair:
typedesc[altair.BeaconBlockBody]
elif kind == ConsensusFork.Phase0:
typedesc[phase0.BeaconBlockBody]
else:
static: raiseAssert "Unreachable"
template SignedBeaconBlock*(kind: static ConsensusFork): auto =
when kind == ConsensusFork.Electra:
typedesc[electra.SignedBeaconBlock]
elif kind == ConsensusFork.Deneb:
typedesc[deneb.SignedBeaconBlock]
elif kind == ConsensusFork.Capella:
typedesc[capella.SignedBeaconBlock]
elif kind == ConsensusFork.Bellatrix:
typedesc[bellatrix.SignedBeaconBlock]
elif kind == ConsensusFork.Altair:
typedesc[altair.SignedBeaconBlock]
elif kind == ConsensusFork.Phase0:
typedesc[phase0.SignedBeaconBlock]
else:
static: raiseAssert "Unreachable"
template TrustedSignedBeaconBlock*(kind: static ConsensusFork): auto =
when kind == ConsensusFork.Electra:
typedesc[electra.TrustedSignedBeaconBlock]
elif kind == ConsensusFork.Deneb:
typedesc[deneb.TrustedSignedBeaconBlock]
elif kind == ConsensusFork.Capella:
typedesc[capella.TrustedSignedBeaconBlock]
elif kind == ConsensusFork.Bellatrix:
typedesc[bellatrix.TrustedSignedBeaconBlock]
elif kind == ConsensusFork.Altair:
typedesc[altair.TrustedSignedBeaconBlock]
elif kind == ConsensusFork.Phase0:
typedesc[phase0.TrustedSignedBeaconBlock]
else:
static: raiseAssert "Unreachable"
template ExecutionPayloadForSigning*(kind: static ConsensusFork): auto =
when kind == ConsensusFork.Electra:
typedesc[electra.ExecutionPayloadForSigning]
elif kind == ConsensusFork.Deneb:
typedesc[deneb.ExecutionPayloadForSigning]
elif kind == ConsensusFork.Capella:
typedesc[capella.ExecutionPayloadForSigning]
elif kind == ConsensusFork.Bellatrix:
typedesc[bellatrix.ExecutionPayloadForSigning]
else:
static: raiseAssert "Unreachable"
template BlindedBeaconBlock*(kind: static ConsensusFork): auto =
when kind == ConsensusFork.Electra:
typedesc[electra_mev.BlindedBeaconBlock]
elif kind == ConsensusFork.Deneb:
typedesc[deneb_mev.BlindedBeaconBlock]
elif kind == ConsensusFork.Capella or kind == ConsensusFork.Bellatrix:
static: raiseAssert "Unsupported"
else:
static: raiseAssert "Unreachable"
template MaybeBlindedBeaconBlock*(kind: static ConsensusFork): auto =
when kind == ConsensusFork.Electra:
typedesc[electra_mev.MaybeBlindedBeaconBlock]
elif kind == ConsensusFork.Deneb:
typedesc[deneb_mev.MaybeBlindedBeaconBlock]
elif kind == ConsensusFork.Capella or kind == ConsensusFork.Bellatrix:
static: raiseAssert "Unsupported"
else:
static: raiseAssert "Unreachable"
template SignedBlindedBeaconBlock*(kind: static ConsensusFork): auto =
when kind == ConsensusFork.Electra:
typedesc[electra_mev.SignedBlindedBeaconBlock]
elif kind == ConsensusFork.Deneb:
typedesc[deneb_mev.SignedBlindedBeaconBlock]
elif kind == ConsensusFork.Capella or kind == ConsensusFork.Bellatrix:
static: raiseAssert "Unsupported"
else:
static: raiseAssert "Unreachable"

There's 120 lines of this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It also increases the coupling which makes for a higher risk/reward setup -- when withConsensusFork works automatically, it's nice, but when it doesn't, it's yet another thing which needs to be temporarily disabled/enabled during fork transitions.

The net effect of this kind of transformation is to centralize the case statements into fewer places, with more effect/impact each. That typically decreases maintenance, until it doesn't. Can't put some debugComment/debugAssert/etc on only the eth2 REST serialization, it has to be in the centralized forks module which affects that and a dozen other things.

But, sure. Maybe risk worth taking, since the point of this overall PR's refactoring is to have REST serialization not use entirely separate block reading for a few specific cases than anything else does, so it should follow other fixes more automatically regardless: 8644d89

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