Align core types with execution spec #733
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kdeme
reviewed
Sep 23, 2024
arnetheduck
force-pushed
the
wip-types
branch
2 times, most recently
from
a157a70
to
f7a788d
Compare
tersec
reviewed
Sep 24, 2024
arnetheduck
force-pushed
the
wip-types
branch
3 times, most recently
from
470c8ac
to
eed6dd1
Compare
Since these types were written, we've gained an executable spec: https://github.com/ethereum/execution-specs This PR aligns some of the types we use with this spec to simplify comparisons and cross-referencing. Using a `distinct` type is a tradeoff between nim ergonomics, type safety and the ability to work around nim quirks and stdlib weaknesses. In particular, it allows us to overload common functions such as `hash` with correct and performant versions as well as maintain control over string conversions etc at the cost of a little bit of ceremony when instantiating them. Apart from distinct byte types, `Hash32`, is introduced in lieu of the existing `Hash256`, again aligning this commonly used type with the spec which picks bytes rather than bits in the name.
arnetheduck
force-pushed
the
wip-types
branch
from
eed6dd1
to
b4d1ef5
Compare
tersec
reviewed
Sep 26, 2024
tersec
reviewed
Sep 26, 2024
|
|
||
| template default*[N](T: type FixedBytes[N]): T = | ||
| # Avoid bad codegen where fixed bytes are zeroed byte-by-byte at call site | ||
| const def = system.default(T) |
There was a problem hiding this comment.
noticed anonConst in stew/objects, useful pattern to enshrine in general
There was a problem hiding this comment.
hm, yeah, didn't know about that one - one thing though is that it generates a new, separate const on every instantiation even if it's the same value/type which I'm not too happy about - ie each default(Bytes32) would result in a separately declared constant in the generated code which is not great
There was a problem hiding this comment.
In theory especially with LTO these could be folded into a single constant. My understanding is that this is a typical linker optimization
There was a problem hiding this comment.
depends - for that to happen, it must determine that the address of the variable is not used for uniqueness / comparisons which doesn't necessarily happen and/or requires special flags.
tersec
reviewed
Sep 26, 2024
tersec
reviewed
Sep 26, 2024
tersec
reviewed
Sep 26, 2024
arnetheduck
added a commit
to status-im/nimbus-eth1
that referenced
this pull request
Sep 26, 2024
status-im/nim-eth#733 updates `Hash256` to become an array instead of an object - unfortunately, nim does not allow constructing arrays with `name()`, so this PR changes it to `default` which works with both.
arnetheduck
added a commit
to status-im/nimbus-eth1
that referenced
this pull request
Sep 26, 2024
* init style for Hash256 status-im/nim-eth#733 updates `Hash256` to become an array instead of an object - unfortunately, nim does not allow constructing arrays with `name()`, so this PR changes it to `default` which works with both. * lint
arnetheduck
added a commit
to status-im/nimbus-eth2
that referenced
this pull request
Sep 26, 2024
Minimal changes neede for compatiblity with status-im/nim-eth#733 which aligns core types with execution spec.
arnetheduck
added a commit
to status-im/nimbus-eth2
that referenced
this pull request
Sep 29, 2024
Minimal changes needed for compatiblity with status-im/nim-eth#733 which aligns core types with execution spec.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since these types were written, we've gained an executable spec:
https://github.com/ethereum/execution-specs
This PR aligns some of the types we use with this spec to simplify comparisons and cross-referencing.
Using a
distincttype is a tradeoff between nim ergonomics, type safety and the ability to work around nim quirks and stdlib weaknesses.In particular, it allows us to overload common functions such as
hashwith correct and performant versions as well as maintain control over string conversions etc at the cost of a little bit of ceremony when instantiating them.Apart from distinct byte types,
Hash32, is introduced in lieu of the existingHash256, again aligning this commonly used type with the spec which picks bytes rather than bits in the name.