search

crate-crypto / rust-eth-kzg

Apache License 2.0
13 stars 5 forks source link

Tracking Issue: PeerDAS-KZG Integration (Consensus Layer) #42

Open kevaundray opened 4 months ago

kevaundray commented 4 months ago

This will track the main integration PR(s) and also the refactor PRs which may need to be merged before integration.

Lighthouse

Reviewer: @jimmygchen

Integration

Refactor

Teku

Reviewer: @zilm13

Integration

Refactor

None yet

Lodestar

Reviewer: Possibly @matthewkeil / @g11tech

Integration

Refactor

None yet

Prysm

Prysm is integrating go-eth-kzg, its easier to track all clients in one place, so it is included here

Reviewer: @nalepae

Integration

Refactor

Nimbus

Reviewer: Still unclear; tagging relevant nim-kzg folks @tersec, @cheatfate, @jangko, @arnetheduck

Integration

None yet

Refactor

None yet

kevaundray commented 4 months ago

I'd like to gather more information on nimbus integration as its the only library where the end-to-end integration is not clear.

From what I understand, it pulls in c-kzg using git-submodules into https://github.com/status-im/nim-kzg4844. It also seems to ignore the fact that c-kzg is a nimble package and "creates" a nimble package in nim-kzg4844 and just takes the relevant parts from c-kzg.

I do wonder if this can be simplified since the release cycle is (1) update c-kzg, then (2) update nim-kzg (3) update nimbus

tersec commented 4 months ago

Nimbus has a few KZG-related PRs/branches:

some of which are more refactoring and some more integration, by the categorization employed in this issue.

Regarding:

From what I understand, it pulls in c-kzg using git-submodules into https://github.com/status-im/nim-kzg4844. It also seems to ignore the fact that c-kzg is a nimble package and "creates" a nimble package in nim-kzg4844 and just takes the relevant parts from c-kzg.

I do wonder if this can be simplified since the release cycle is (1) update c-kzg, then (2) update nim-kzg (3) update nimbus

nimbus-eth2 does not actively use Nimble at all. It does not care that any of it dependencies (nim-stew, nim-web3, nim-json-serialization, et cetera) are also Nimble packages. nim-kzg4844 is no different in this regard than anything else in https://github.com/status-im/nimbus-eth2/tree/unstable/vendor which uses a pinned commit via git submodule. This applies recursively: when nim-kzg4844 has dependencies such as c-kzg, it does this via git submodule too.

I'm not sure precisely what simplification you have in mind. We're working on improving the Nimble-based tooling so we can use that, but until then, yes, updating these recursively dependent modules involves multiple steps, sort of walking back up the dependency tree towards a (sub)root.

One of the integration challenges on our end thus far has been that all this lives in the das branch, not the main branch, so have an increasingly divergent PeerDAS codebase because of this increasingly divergent upstream dependency. Some of the changes are intrinsic, others are probably less so and might be able to live on main too. Regardless of what makes sense in terms of c-kzg branch management in isolation, it means that we're going to be looking at a somewhat messy merge because we (unless we switch nimbus-eth2 globally to use the das branch and not the main branch via nim-kzg4844's submodule commit) can't really gradually merge anything, and have to have this long-lived feature branch, which typically we avoid doing.

The ask here is, if people prioritize this that highly for Pectra, that there's movement on getting das merged into main at some point, well in advance of the first "full" integration expected as opposed to the PeerDAS devnets being this separate side-track which is thus far ignored by the main Pectra devnets.

Finally, in general, @agnxsh has been doing most of the PeerDAS-specific integration work on this, so he's a good person to communicate with as well on this topic in addition to some of the other people you've listed (e.g., @jangko and @cheatfate have been fixing the gcc 14 compatibility recently in c-kzg, but that's not especially PeerDAS-specific).

kevaundray commented 4 months ago

The ask here is, if people prioritize this that highly for Pectra, that there's movement on getting das merged into main at some point, well in advance of the first "full" integration expected as opposed to the PeerDAS devnets being this separate side-track which is thus far ignored by the main Pectra devnets.

Noting that this is slated for this month, so somewhat soon -- I'm not sure when the full integration is expected to be

kevaundray commented 4 months ago

I'll CC @asn-d6 and @jtraglia for visibiility