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
Start implementation of waku node #437
Conversation
eth/[common, rlp, keys, p2p], eth/p2p/rlpx_protocols/waku_protocol, | ||
nimcrypto/[sysrand, hmac, sha2, pbkdf2] | ||
|
||
from stew/byteutils import hexToSeqByte, hexToByteArray |
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.
Is this expected to differ considerably from Whisper? Perhaps the RCP service can use a wakuMode
flag enabling the different behavior instead of duplicating the entire implementation.
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.
Yes, such thing has crossed my mind, as there is indeed no real difference right now.
However, the best I could come up with for now is some compile time flag to use either waku or whisper protocol.
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've decided to leave it as is. I don't think it is worth it to rework this with either compile time define or some templated / generic version. In the end I believe it will make it only more difficult when changes need to be made on Waku without touching Whisper.
I did however separate out the keys part. And if wanted, I could move it to the waku/rpc directory.
I think it's fine for this to remain in the nimbus repo for now. |
0ea6406
to
6825e46
Compare
Was a gotcha for me
So, ready for review/merge |
|
||
if config.logMetrics: | ||
proc logMetrics(udata: pointer) {.closure, gcsafe.} = | ||
{.gcsafe.}: |
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.
Why is this override necessary? Is it because reading a metric is considered non gcsafe?
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.
Yes, initially I tried removing that but without it would error with something like is not GC-safe as accesses global using GC'ed memory
.
I mostly just copied this from here: https://github.com/status-im/nimbus/blob/master/nimbus/nimbus.nim#L65 , but I didn't like the default over verbose logs.
This is still WIP, issue: #426 (comment)
First question would be if this code belongs in this repository or should be moved?
Point for now is to have a Node that implements Waku protocol to be able to test:
Still requires quite some work, but for now it can be used by launching a bootnode and several nodes connecting to that bootnode. Next, if RPC is enabled one can subscribe to topics and post message, e.g. via web3, as shown in this quick test: https://gist.github.com/kdeme/17006627154cbb796df845a0b7a14fb2
When https://github.com/status-im/nim-web3 gets updated with that RPC interface we can write an application in nim that controls e.g. the simulation process.