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

Move ssz code from nimbus-eth2 repo and adjust #1

Closed
wants to merge 3 commits into from
Closed

Conversation

kdeme
Copy link
Collaborator

@kdeme kdeme commented May 31, 2021

Needs more work.

Mostly on the merkleization code, which is rather entangled with other nimbus-eth2 code. While it will work for nimbus-eth2 in the currently adjusted form, moving the merkleization test code is still a pain to fix.
And then there is the need for blst (due to the two sha256 options), which seems to also cause issues in building, sigh.

@arnetheduck
Copy link
Member

well, blst and that should not be in this repo - ssz doesn't talk about it - if we have a separate repo for ssz code it should implement the ssz spec and nothing else

@arnetheduck
Copy link
Member

https://github.com/ethereum/eth2.0-specs/blob/v1.0.0/ssz/simple-serialize.md is the whole spec and that's exactly what should be in here

@kdeme
Copy link
Collaborator Author

kdeme commented May 31, 2021

Yeah, well, it is not that I actually want it there, I don't like it either. Just that Merkleization is very much mixed with some of that eth2 work, so turns out to be a nice rabbit hole.
I was going to a) not add Merkleization (for now..?), or b) try to separate it. @zah however mentioned that it is part of the specification (which I realized anyhow) so should stay with the general ssz code, and that blst could be added as dep. I'd rather see it go than stay, just not sure how convenient that is now.

@arnetheduck
Copy link
Member

Well, if we're moving things to a separate non-eth repo, it could at least be done properly, else this will continue to be a mess and a burden on all projects that use it.

This is by and large what one could expect, the ssz code is a hotspot for compiler bugs, and it has its fair share of hacks and special cases just for eth2 because it's tightly interwoven with caching and other aspects that are kind of not really SSZ (like the root trick, the way distinct types are shaved off etc).

Also, the way custom type support is is .. problematic - to make it a generic general-purpose library, one would really need to rethink several parts - the current code is not sustainably written for other uses.

The spec is comparatively simple and small - the cost of reimplementing might be lower than trying to work around all the nim issues you'll hit when trying to extract it.

eth2 doesn't even need to use the rewritten version - notably, eth2 is not using nim-serialization / faststreams any more for reading, in many cases

@arnetheduck
Copy link
Member

arnetheduck commented May 31, 2021

The other issue is that once npln starts using SSZ, it will need to add it's own share of hacks and workarounds - these necessarily must not end up in eth2 because then npln needs to be a lot more careful about not touching the wrong levers, or we'll end up in a situation where the two projects mutually are slowing each other down. Decentralizing ssz support might be the better option for this reason as well.

@kdeme
Copy link
Collaborator Author

kdeme commented May 31, 2021

This was actually the approach I started with. I just dropped in (duplicated) the bare minimum SSZ code that I needed for now in nim-eth in order to get a working npln wire protocol version to test with. I started like that also because I didn't know the SSZ code at all, and wanted to understand what was needed for now. Got this working in no time.

I understood however that the idea was to get all (generic) code to either nim-eth or nim-ssz-serialization (I don't really care which one, issues remain the same however). So I've been attempting that, in several ways, but yes, I'm hitting several issues and I've spend quite some time on this here with little progress.

I'm fine with going back to the original test I did, just wish this direction was clear from the start, as I was apparently missing some background/context.

@arnetheduck
Copy link
Member

the way I'd approach it would probably be just that: write a separate library, maybe copypasting some stuff, then when everything is working, see what common, low-level ground remains - this is a much less disruptive way than trying to solve all problems at once, and potentially ending up breaking eth2 in the process.

@KonradStaniec
Copy link

As I understand approach in this pr copy ssz serialization from nimbus-eth (stripping some of the domain types) which brings some baggage and hack. And in perfect world, we would write smaller szz serialization library which would get rid of that baggage and be a little better designed.

Just asking here as, for fluffy will need working ssz with merkelization support to support https://ethresear.ch/t/double-batched-merkle-log-accumulator/571, and I want to establish what is best way forward here.

@kdeme
Copy link
Collaborator Author

kdeme commented Oct 21, 2021

#2

@kdeme kdeme closed this Oct 21, 2021
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.

3 participants