-
Notifications
You must be signed in to change notification settings - Fork 6
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
Copy ssz code from nimbus-eth2 and adjust to stand on its own #2
Conversation
# size of the dynamic portion to consume the right number of bytes. | ||
readSszBytes(r.stream.read(r.stream.len.get), val) | ||
|
||
proc readSszBytes*[T](data: openArray[byte], val: var T) {. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was really intended as an eth2 specific thing - but one thought I've had is to remove nim-serialization completely from the process so as to have a stand-alone SSZ reading library that operates on openArray
- then nim-ser
can be glued on top for any .. uh .. advanced use cases should they materialize.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The codec
module already provides this (reading that only depends on the compile-time type metadata aspects of nim-serialization such as the dontSerialize
pragma). I don't see any good reason for aiming to drop this dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, codec
was written with this in mind and readSszBytes
as well, to a certain extent, and it should perhaps be moved there so that when using only readSszBytes
, the symbol table isn't infected with the rest of nim-ser
- the aim is to get rid of some of the generic instantiation code and other reader/writer magic that isn't really relevant when reading from openArray
- this is also "usually" a good way to factor this kind of code into orthogonal responsibilities anyway: one layer that deals with concrete raw byte stuff and another layer that deals with magic object transformations and high-level representations and opinions about what should be magic and implicit and what shouldn't be.
@@ -9,7 +9,12 @@ skipDirs = @["tests"] | |||
|
|||
requires "nim >= 1.2.0", | |||
"serialization", | |||
"stew" | |||
"json_serialization", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is an .. unfortunate compromise - ie an ssz library shouldn't depend on json, yet nim:s / nim-ser:s import model with open generics and different behavior, even within an application, depending on imports really leaves no other practical option than this - cc @zah
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for example, importing jsonser/std/option
instead of std/option
to get sane logging vs a memory dump is a trivial example of this, where the consequences are contained to ugly logs - it's however not humanly possible to keep track of whether something is imported or not in every place that matters specially when sandwitches come into play - it gets a lot worse when trying to implement a standard where a particular format is mandated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Err, why is this dependency necessary? The correct action here is just to remove it IMO.
The JSON serialization definitions of HashList
, etc can sit in modules such as eth2_rest_serialization
, eth2_json_serialization
, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you are trying to provide an excuse for why you structured the code this way.
I think some problems just have to be addressed at the language level, because these inappropriate compromises can get you only thus far - you cannot fix the options
module for example (or any other module developed by a third party).
I've suggested one possible language solution multiple times, the last one has been here:
nim-lang/RFCs#380 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, in fantasy-land, anything is possible.
In Nim, the best thing that could happen with the current, arguably broken nim-ser design, would actually be that json-ser includes serializers for all std types it supports out-of-the box without magic imports - or at least the "common" ones, and libraries that are used with nim-json-ser or some other ser always declare support for those serialization formats in the same module as the type itself is declared - anything else doesn't work, in actual real code - for this reason, the best thing this library can do is to depend on json-ser because json-ser is more "commonly" used than ssz.
The other, better option is that nim-ser and json-ser no longer contain generic catch-alls for objects at all: every type you want to serialize, you are forced to declare support for it up-front - this doesn't require any language changes: it simply requires that under no circumstances is there something that matches object
(and tuple of course).
This way, when you "forget" to import the right serializer, you get a compile error instead of a runtime explosion. It's 100% wrong that objects are automatically serialized this way - the only thing that does is make the simple trivial action of adding an import easier, while making the already difficult task of remembering what needs to be imported where impossible. From a design perspective of any library, that's simply wrong - hard things should never be made be made harder, and it's really no loss that if you want to add json-ser support for your type, you need to tell the library "hey, please support this type for me". It can be as trivial as a {.xxx.}
annotation or a forwarded call to a "default" serializer for objects.
70964f3
to
4db97a7
Compare
To see more easily the differences: https://gist.github.com/kdeme/1cdcbf6f8eba8fade7be857a8b6154c6 |
7d3e309
to
c560203
Compare
c560203
to
b8c89d4
Compare
else: | ||
type DigestCtx* = sha2.sha256 | ||
|
||
template computeDigest*(body: untyped): Digest = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eth2Digest
-> Digest
and the move of this template (and its when clauses) is basically the only "real" change in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did just realize that sha256 is not actual part of the SSZ specification (only defined at beacon chain specs level: https://github.com/ethereum/consensus-specs/blob/2c632c0087f0a692ab74987229524edbb941eeb3/specs/phase0/beacon-chain.md#hash), so we could also still decide to move the selection of the hashing call to the "application" layer.
|
||
task test, "Run all tests": | ||
test "--threads:off", "tests/test_all" | ||
test "--threads:on", "tests/test_all" | ||
test "--threads:off -d:PREFER_BLST_SHA256=false", "tests/test_all" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Building it currently only using the sha256 implementation of nimcrypto as I'm having issues with BLST outside of the nimbus-eth2 env.
serialization/testing/tracing, | ||
"."/[bitseqs, codec, types] | ||
|
||
const PREFER_BLST_SHA256* {.booldefine.} = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allow for not using BLST sha256 implementation, but default on so that nimbus-eth2 doesn't need changes.
Seems to conflict with SszWriter.init ... sometimes. Need a magician for this.
b4b8c23
to
9e54552
Compare
I'll go ahead and merge this so this (more updated) code can be used in nimbus-eth1 fluffy instead of the nim-eth code. Necessary adjustments that come out of the discussion can be taken care of later. |
Take 2