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

Bump nim-serialization to potentially solve #435 #436

Merged
merged 4 commits into from
Sep 12, 2019

Conversation

mratsim
Copy link
Contributor

@mratsim mratsim commented Sep 11, 2019

@mratsim
Copy link
Contributor Author

mratsim commented Sep 11, 2019

Raised status-im/nim-serialization#11 for a loadFile smoked test in nim-serialization.
The error is strange:

../beacon_chain/eth2_network.nim(241, 9) template/generic instantiation from here
../vendor/nim-serialization/serialization.nim(92, 16) Error: type mismatch: got <ByteStreamVar>

but expected one of: 

proc close(f: File)
proc close[TMsg](c: var Channel[TMsg])
proc close(s: var ByteStream)
template close(s: AsciiStream | UnicodeStream | ObjectStream)
proc close(transp: StreamTransport)
proc close(server: StreamServer)
proc close(rw: AsyncStreamRW)
proc close(socket: Socket)
proc close(transp: DatagramTransport)
proc close(stream: P2PStream): Future[void]
proc close(api: DaemonAPI): Future[void]

However according to Nim auto-dereferencing rule, Nim should be able to autodereference ByteStreamVar to ByteStream:

https://github.com/status-im/nim-faststreams/blob/e7a34b74f298eaabd7bdbe2f080bf9747ed6bfd9/faststreams/input_stream.nim#L14-L166

type
  # ...
  ByteStream* = object of RootObj
    head*: ptr byte
    bufferSize: int
    bufferStart, bufferEnd: ptr byte
    cursorsList: ptr StreamCursor
    reader: StreamReader
    asyncReader: AsyncStreamReader
    closeStream: CloseStreamProc
    bufferEndPos: int

  # ...

  # TODO: ByteStreamVar should become a `var` short-lived pointer once this is supported
  ByteStreamVar* = ref ByteStream
 
proc openFile*(filename: string): ByteStreamVar =
  new result
  var memFile = memfiles.open(filename)
  result.head = cast[ptr byte](memFile.mem)
  when debugHelpers:
    result.bufferStart = result.head
  result.bufferEnd = result.head.shift memFile.size
  result.bufferEndPos = memFile.size
  result.closeStream = proc = close memFile

# ...

# TODO: use a destructor once we migrate to Nim 0.20
proc close*(s: var ByteStream) =
  if s.closeStream != nil:
    s.closeStream()

Solutions:

  • either expose close for ByteStreamVar
  • or do stream[].close() in nim-serialization loadFile
  • or use a finalizer in nim-faststreams (which is undeterministic but ensure that we do not forget to close the stream)

@mratsim mratsim requested a review from zah September 11, 2019 13:53
@jangko
Copy link
Contributor

jangko commented Sep 11, 2019

cannot solve #435

@mratsim mratsim reopened this Sep 11, 2019
@mratsim
Copy link
Contributor Author

mratsim commented Sep 11, 2019

turns out the illegal storage access is in the Nim compiler, it probably doesn't like defer

compiling beacon_node crashes

sync_protocol.nim(51, 6) Hint: 'sync_protocol.fromHeaderAndBody(b: var BeaconBlock, h: BeaconBlockHeader, body: BeaconBlockBody)[declared in sync_protocol.nim(51, 5)]' is declared but not used [XDeclaredButNotUsed]
sync_protocol.nim(42, 6) Hint: 'sync_protocol.toHeader(b: BeaconBlock)[declared in sync_protocol.nim(42, 5)]' is declared but not used [XDeclaredButNotUsed]
Hint: request_manager [Processing]
Hint: validator_keygen [Processing]
Hint: interop [Processing]
beacon_node.nim(222, 41) Hint: conversion from string to itself is pointless [ConvFromXtoItselfNotNeeded]
SIGSEGV: Illegal storage access. (Attempt to read from nil?)

@mratsim
Copy link
Contributor Author

mratsim commented Sep 11, 2019

So now #435 is triggered on a month old test that never failed before: https://ci.appveyor.com/project/nimbus/nim-beacon-chain/builds/27346548/job/gki7ew7tkda8ou8w#L242

[Suite] Official - Shuffling tests [Preset:  [Preset: minimal]
  [OK] Shuffling a sequence of N validators [Preset: minimal]
[Suite] Official - BLS tests
    Unhandled exception: The parameter is incorrect.
 [OSError]
  [FAILED] Private to public key conversion
  [OK] Message signing
  [OK] Aggregating signatures
  [OK] Aggregating public keys

@mratsim
Copy link
Contributor Author

mratsim commented Sep 12, 2019

Can be rebased and merged once #441 is merged into master as #440 requires the same fix anyway.

@tersec
Copy link
Contributor

tersec commented Sep 12, 2019

Apparently no rebasing required.

@tersec tersec closed this Sep 12, 2019
@tersec tersec reopened this Sep 12, 2019
@mratsim mratsim merged commit aed770d into master Sep 12, 2019
@delete-merged-branch delete-merged-branch bot deleted the update-nim-serialization branch September 12, 2019 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants