Add REST endpoint to retrieve historical_summaries #6675
Conversation
|
One consideration for me is that if this does get standardized, we don't end up with both a pre-standardized and standardized version which needs to be supported. |
One approach could be that I move it under the |
kdeme
force-pushed
the
historical-summaries-endpoint
branch
from
e3d435b to
d7047dd
Compare
Why you need to remove it? You can establish |
Well, part of the point is not to end up with extra, redundant endpoints. The point of this subject to removal without notice would be to exactly allow experimentation without obligation such directions later. |
There could be exactly 2 situations:
In both this cases it could be better to start with |
kdeme
force-pushed
the
historical-summaries-endpoint
branch
from
d7047dd to
566e69b
Compare
kdeme
force-pushed
the
historical-summaries-endpoint
branch
3 times, most recently
from
e123f65 to
631a155
Compare
|
@etan-status and/or @cheatfate Would you mind having a look at this again? It is something crucial for our development on Portal network. As mentioned in the comments, fork management is added, which I think was the only issue (at least for getting it under nimbus/debug namespace)? Some remarks:
|
There was a problem hiding this comment.
There's also an effort from @Inspector-Butters and @wemeetagain to look into a general proof API. But I think as part of Nimbus namespace it's alright to have specific endpoints to unblock portal progress.
Code looks correct to me, I have put some remarks to make it less maintenance-heavy and reduce duplication. Once comments are addressed, good to go.
| template init*( | ||
| T: type ForkedHistoricalSummariesWithProof, | ||
| historical_summaries: GetHistoricalSummariesV1Response, | ||
| fork: ConsensusFork, |
There was a problem hiding this comment.
Hmm. Do we really need to retain the exact consensusFork?
Or would a HistoricalSummariesFork make sense, similar to LightClientDataFork, EpochInfoFork or (experimental BlobFork in #6451)? That way, this code no longer has to be maintained with every fork, by simply having a capellaData and electraData member, and no denebData / fuluData etc
case x.kind
of HistoricalSummariesFork.Electra:
const historicalFork {.inject, used.} = HistoricalSummariesFork.Electra
template forkySummaries: untyped {.inject, used.} = x.electraData
body
of ConsensusFork.Capella:
const historicalFork {.inject, used.} = HistoricalSummariesFork.Capella
template forkySummaries: untyped {.inject, used.} = x.capellaData
bodyOne can still recover exact consensusFork if needed from Eth-Consensus-Version HTTP header, if needed.
There was a problem hiding this comment.
No, I don't think we need it.
In the past, I mostly found it confusing to work with these different fork types and always had to look back at which one contained which forks, hence why I did not go for that.
But I think I can understand how it makes maintenance probably quite a bit easier when you are dealing with that all the time. Though for an external/new reader of the code, having all these different fork types without a reference in the spec, might be confusing.
There was a problem hiding this comment.
So in the case we would add a HistoricalSummariesFork, I assume the code would work in such way that in the metrics API where there is now the statement:
node.withStateForBlockSlotId(bslot):
return withState(state):
when consensusFork >= ConsensusFork.Capella:
we would set the HistoricalSummariesFork based on the ConsensusFork?
And then from then onward work with the HistoricalSummariesFork?
I can see the benefit of not having to update the fork templates for that, but it also allows for not failing compilation when a fork gets added which might require a change again?
I am currently undecided what I like the most, but if there is a strong pull towards adding HistoricalSummariesFork then I don't mind doing so (especially since it is already the habit in the code base).
There was a problem hiding this comment.
@tersec any opinion on this? Else I'll go ahead and merge as is now.
There was a problem hiding this comment.
but it also allows for not failing compilation when a fork gets added which might require a change again
Typically, adding a new fork is quite a mechanical process, so I guess if this doesn't compile, in practice the first step I expect is that the person adding a new fork would do a simple copy paste of the prior fork's logic, effectively ending up with the same bugs as if the code would implicitly inherit the prior fork's logic rather than explicitly. The mental effort is comparable, one has to be actually aware of generalized indices getting re-indexed during a fork to prevent the mistake.
If you want to protect against the bugs, you can add a static: doAssert that checks that log2trunc(consensusFork.BeaconState.totalSerializedFields) matches the compatible value, to prevent someone from messing it up with a mechanical change when adding a future fork. Such an assertion cannot be lazily adjusted to make the code work and requires more thought to address :-) That way, you have the maintenance-free implicit inheritance for all the forks that don't need a new HistoricalSummariesFork, and you also have the compile-time error in case a new HistoricalSummariesFork is required.
Most ConsensusFork won't need a new HistoricalSummariesFork, I would personally slightly tend towards the HistoricalSummariesFork. But I'm also fine with the current solution. Up to @tersec
There was a problem hiding this comment.
And, consensusFork.BeaconState actually works. There's a template in forks.nim to support accessing fork specific types based on static ConsensusFork constant.
withConsensusFork(consensusFork): # or withState / withBlock, they also inject `consensusFork` constant
static: doAssert log2trunc(consensusFork.BeaconState.totalSerializedFields) == ....There was a problem hiding this comment.
Introduced HistoricalSummariesFork in c3166c5
Assuming this still is the preferred way, if not I can revert the last commit and we go with that.
kdeme
force-pushed
the
historical-summaries-endpoint
branch
from
62293d6 to
ebb2ec4
Compare
|
@etan-status I addressed latest feedback, aside from the specific fork addition, which I commented on (but am open to add still). |
|
Is this still blocked? Because I want to use it! |
|
Ideally we standardized this endpoint across the board and with loadstar ChainSafe/lodestar#7245 |
kdeme
force-pushed
the
historical-summaries-endpoint
branch
from
6cebf61 to
2050f15
Compare
|
The failing CI, https://ci.status.im/blue/organizations/jenkins/nimbus-eth2%2Fplatforms%2Flinux%2Fx86_64%2Fnimv2_2/detail/PR-6675/2/pipeline is a bit confused; it's supposed to be Nim 2.2, but it's apparently using Nim 2.0.6: It can be safely ignored. |
So I added this historical summaries fork in c3166c5 (see msg #6675 (comment)). @etan-status @tersec Any preference on with or without historical summaries fork implementation? |
And remove unneeded default forkySummaries.
kdeme
force-pushed
the
historical-summaries-endpoint
branch
from
c3166c5 to
fbbbdda
Compare
kdeme
force-pushed
the
historical-summaries-endpoint
branch
from
fbbbdda to
baff4da
Compare
In draft currently as it is not in spec. Needs to be accompanied still with a proposal for beacon REST API specifications.
The idea is that
historical_summariesis very useful for verifying if beacon blocks (with their accompanied proofs) are part of the canonical chain, however there is no way to actually retrieve thehistorical_summariesexcept by using the/eth/v2/debug/beacon/states/{state_id}endpoint, which is quite the download in terms of size (We are currently using this endpoint in Portal/fluffy for ourportal_bridge, but it takes long).This PR adds an endpoint that provides the
historical_summarieswith its proof against the state root. This is done so that thehistorical_summariescan then be verified for example with the finalized state root that a (light) node might hold from its finality update.Again, this is specifically useful in Portal (as a
portal_bridgecan inject it in the p2p network as is, and receiving nodes can verify it if they are light client synced) . But you could make an argument that it could also be useful on a consensus light client (assuming blocks with proofs would be somehow available).edit: moved to
/nimbus/v1/debug/namespace and removed draft