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

nim-serialization does not support generics properly #2219

Closed
mratsim opened this issue Jan 8, 2021 · 1 comment
Closed

nim-serialization does not support generics properly #2219

mratsim opened this issue Jan 8, 2021 · 1 comment
Labels
🕤 Postponed Not scheduled for the current sprint refactoring upstream

Comments

@mratsim
Copy link
Contributor

mratsim commented Jan 8, 2021

In the course of refactoring the block validation flow as a preparation to multithreading, there are been several issues linked to Nim type system (type, typedesc, auto) and working with types in macros (nim-lang/RFCs#44).

Given the timesink they are, we will instead use dumb code duplication for now.

The code is stashed in this commit 798e135 and requires this commit/branch to nim-serialization status-im/nim-serialization@82cdd5c.

A recap on difficulties encountered and their status:

  1. Static enum were used at first, see https://gist.github.com/mratsim/2e29324d9c6064e547790c7579f4607f
    They caused visibility issues with chronicles.
type
  TrustLevel* = enum
    ## The trust level of a block, attestation or signature
    ##
    ## When we receive blocks from outside sources, they are untrusted and go
    ## through several layers of validation. Blocks that have gone through
    ## validations can be trusted to be well-formed, with a correct signature,
    ## having a parent and applying cleanly to the state that their parent
    ## left them with.
    ##
    ## When loading such blocks from the database, to rewind states for example,
    ## it is expensive to redo the validations (in particular, the signature
    ## checks), thus `TrustedBlock` uses a `TrustedSig` type to mark that these
    ## checks can be skipped.
    ##
    ## docs/block_validation_flow.md
    Unchecked # Object hasn't gone through any verification
    SigVerified # Object cryptographic signature has been verified
    TransitionVerified # Object is valid according to the protocol state transition logic
    Trusted # Object is fully verified both cryptographically and logically

    # TODO: we might want to compile with deduplication on
    # as the C code will be the same but we will have 1 duplicate per trust level
    # https://stackoverflow.com/questions/15168924/gcc-clang-merging-functions-with-identical-instructions-comdat-folding
    # linker: --icf (identical COMDAT folding)
    # GCC: -ffunction-sections

  Attestation*[Trust: static TrustLevel] = object
    aggregation_bits*: CommitteeValidatorsBits
    data*: AttestationData
    when Trust in {SigVerified, Trusted}:
      signature*: TrustedSig
    else:
      signature*: ValidatorSig
  1. Instead Tags using generics were used 0a0a304, the logic compiled and passed tests but quickly compiler crashes appear in our SSZ serialization
  2. Changes were added so that testutils didn't require SSZ all the time adding more noise to find out what was wrong and what worked d78b945
  3. Part of the EF tests were made working, those that didn't involve generics
    6df6931
    PASS
    - const_sanity_check
    - operations_deposits
    - operations_proposer_slashings
    - operations_voluntary_exit
    - rewards
    - sanity_slots
    - ssz_generic_types
    - state_transition_epoch
    
    FAIL runtime
    - operations_attestations
    
    Compiler Crash
    - operations_attester_slashings
    - operations_block_header
    - sanity_blocks
    - ssz_consensus_objects
    

Compiler crashes are addressed in 5 and due to the Nim compiler.
The runtime error is addressed in 6 and due to the when not being properly accounted for offset computation.

  1. Compiler crashes required strange fixes like:
  1. Additional hacks to support when statement within an object declaration were needed or the serialization offsets were wrong: status-im/nim-serialization@dfd6e34 (note that this one is on us, Nim fieldPairs properly skips the wrong when instantiation, but we might need better macro tools to work with that).
  2. Then fixing generic sandwiches between std/sets and std/sequtils 3366b75#diff-d50a8590babef6121b5e3bd1904cb23e2d3e0507a5f7d52097252832bccbb902R18-R535 Generics "sandwiched" between two modules don't mixin their scope symbols properly nim-lang/Nim#11225
  3. Then additional fixes when the when branches reuse the same name status-im/nim-serialization@4e2ffe3 it relies on when compiles using the fact that on the wrong branch the type will be wrong.
  4. Unfixed, the recordFields macro in stew/shims doesn't save instantiated generics and in some cases return the abstract generic type instead of the instantiated one. There is an attempt to monkey patch around that in nim-serialization status-im/nim-serialization@82cdd5c.
    It mostly works, except when getTypeInst decides to return a dumb symbol instead of the type we actually want for unknown reason.
    image while the previous type worked just fine
    image. This workaround also required working around getImpl on types return incorrect tree in when branches nim-lang/Nim#16639
@arnetheduck
Copy link
Member

nim-ssz-serialization has been significantly refactored since, specifically to avoid fancy generics, import order issues and other stuff - closing this for now, I don't think there's anything actionable based on today's codebase and compiler version other than what is tracked elsewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🕤 Postponed Not scheduled for the current sprint refactoring upstream
Projects
None yet
Development

No branches or pull requests

2 participants