feat: waku store sync 2.0 common types & codec #3213
Conversation
SionoiS
requested review from
darshankabariya,
gabrielmer,
NagyZoltanPeter and
Ivansete-status
|
You can find the image built from this PR at Built from 7460edc |
SionoiS
changed the title
The goto should be my research issue. waku-org/research#102 and wip spec https://github.com/waku-org/specs/blob/feat--waku-sync-2/standards/core/sync.md Although a lot of details are implementation specific which is not mentioned anywhere 🤔 maybe more code comment are needed. |
There was a problem hiding this comment.
Thanks a lot for the (significant!) effort, also in the follow-up PRs! 🚀 I've requested some changes, mostly related to helping reviewers understand the code/protocol a bit better. For example, there are some magic numbers that I still can't quite figure out, some codec types don't quite match the spec or need some context, etc.
For this PR, I'd be happy to approve if the code is made clearer (with comments, reduced magic numbers, etc.). However, I think it will be easier for this (and follow-up) PRs to expand the wire protocol specification itself and get an agreed-upon design as spec first so that reviewers wouldn't have to infer how the protocol works only from code.
waku/waku_store_sync/common.nim
Outdated
| SyncPayload* = object | ||
| ranges*: seq[(Slice[ID], RangeType)] | ||
| fingerprints*: seq[Fingerprint] | ||
| itemSets*: seq[ItemSet] |
There was a problem hiding this comment.
I think we really need a proper wire protocol specification to both understand (and review) what SyncPayload does, when and how it's exchanged and interpreted, procedural flow, etc. We can then review the implementation as an accurate reflection of the agreed upon design/specification. For example, are Waku Sync nodes simply exchanging SyncPayload as handshake messages? Is there no negotiation about what shards (or time range) to sync? If not, do we assume that this Sync protocol will eventually be coupled with a negotiation protocol that will first establish the set of shards and time range (and later content topics) the nodes will sync? If not, I can imagine two Sync nodes storing completely different shards routinely happily reconciling their 100% divergent "sets", even if the two sets have nothing to do with each other. metadata protocol can also not be relied upon, as (i) this will create a strong dependency between the two protocols (ii) Sync nodes store sets may not match all shards they subscribe to/advertise in metadata protocol.
There was a problem hiding this comment.
At this stage, the impl. is 100% equivalent to the previous one in features. There's no negotiation. We can start discussing limited shard support in my research issue if you want.
waku/waku_store_sync/codec.nim
Outdated
|
|
||
| return idx | ||
|
|
||
| proc deltaDecode*(T: type SyncPayload, buffer: seq[byte]): T = |
There was a problem hiding this comment.
I think we can reference the spec here in the comments.
There was a problem hiding this comment.
I don't think it's a good idea to add links in the code. Link always end up broken anyway.
Maybe a section at the top explaining that there's a spec?
| time*: Timestamp | ||
| hash*: WakuMessageHash | ||
|
|
||
| ItemSet* = object |
There was a problem hiding this comment.
I think every type, and its procs, should be contained in a separate file
There was a problem hiding this comment.
Come on, the file is not even 100 lines.
Description
First PR for the new Waku Store Sync 2.0
Include types and encode/decode of payloads and also tests.
Specification
Research issue
Changes
Followed by #3215
Issue