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

Draft: Wire protocol (DEP) #8

Merged
merged 23 commits into from Feb 27, 2019

Conversation

Projects
None yet
7 participants
@pfrazee
Copy link
Member

pfrazee commented Mar 4, 2018

Rendered: Draft

Submitted for review. Working group will vote in next meeting (late February).

@bnewbold
Copy link
Contributor

bnewbold left a comment

Good to see this coming along! It's a long and involved one.

Show resolved Hide resolved proposals/0000-wire-protocol.md Outdated
Show resolved Hide resolved proposals/0000-wire-protocol.md
Show resolved Hide resolved proposals/0000-wire-protocol.md Outdated
Show resolved Hide resolved proposals/0000-wire-protocol.md Outdated
Show resolved Hide resolved proposals/0000-wire-protocol.md Outdated
Show resolved Hide resolved proposals/0000-wire-protocol.md Outdated
Show resolved Hide resolved proposals/0000-wire-protocol.md Outdated
Show resolved Hide resolved proposals/0000-wire-protocol.md Outdated
Show resolved Hide resolved proposals/0000-wire-protocol.md Outdated
Show resolved Hide resolved proposals/0000-wire-protocol.md
@bnewbold

This comment has been minimized.

Copy link
Contributor

bnewbold commented Mar 5, 2018

We should also write "Security and Privacy Concerns" and "References" sections for this one.

For references I could imagine:

  • bittorrent, other "who are your influences?" protocol specs
  • papers/references around choking and other scheduling issues (eg, see links from this ebay post
  • more protobuf references (we have a single link right now)
  • libsodium and hash/crypto algorithm references

For security/privacy:

  • enumerate what a persistent observer can learn about peers
  • enumerate some specific threat models and trust assumptions (eg, which properties of which hash and crypto algorithms are we assuming and relying on; distinction between, eg, targeted censorship and data manipulation/corruption)
discoveryKey, // out
'HYPERCORE', // in (message)
publicKey // key
)

This comment has been minimized.

@mafintosh

mafintosh May 2, 2018

(use Buffer.alloc) also this is basically an HMAC and we should note that

This comment has been minimized.

@bnewbold

bnewbold Nov 18, 2018

Contributor

I added Buffer.alloc().

Could you recommend language describing this as an HMAC?

This comment has been minimized.

@emilbayes

emilbayes Nov 19, 2018

I would personally either keep this pseudocode or actual implementation. Pseudo code would be something like discoveryKey = blake2b(outputLength = 32, key = 'HYPERCORE', data = publicKey), outputLength being another important parameter (256 bit output)

This comment has been minimized.

@bnewbold

bnewbold Jan 20, 2019

Contributor

I moved to pseudo-code (which I also prefer to implementation or any specific language in the DEPs).

I think "keyed hash" clarifies things; maf if you want to include a note about HMAC I need something more specific.

@pfrazee

This comment has been minimized.

Copy link
Member Author

pfrazee commented Jun 1, 2018

@bnewbold

This comment has been minimized.

Copy link
Contributor

bnewbold commented Jun 17, 2018

@pfrazee what's the status on this DEP? Are we blocked waiting on anything? If you move this somewhere I have git push privs I can try to push it through in the next couple weeks.

@pfrazee

This comment has been minimized.

Copy link
Member Author

pfrazee commented Jun 19, 2018

@bnewbold I think it just needs to get written, I don't think there are any blockers in dev work. I'll give you push rights on my fork.

@pfrazee

This comment has been minimized.

Copy link
Member Author

pfrazee commented Jun 19, 2018

Apologies for letting it stall!

@yoshuawuyts yoshuawuyts referenced this pull request Oct 28, 2018

Open

Implement peers / network #11

9 of 17 tasks complete
@bnewbold

This comment has been minimized.

Copy link
Contributor

bnewbold commented Nov 18, 2018

Made some more progress on this, resolving most of the TODOs and comments. Still need clarification/feedback on:

  • does ack flag actually do anything
  • HMAC language

The security/privacy section in particular should get reviewed, and the whole thing probably needs a copy-edit and review for missing content (which I haven't done yet).

jIt would also be nice to have more examples, but those can take a long time to write up, and at this point, with effort being put in elsewhere (like the protocol book), I don't think it's necessary here for Draft status. There are no "rationale" or "alternatives" sections; I think those make more sense for new proposals and less for these older/existing protocol bits, but i'm open to objections.

Would be great to have this ready to vote, or at least review, by next WG meeting!

@bnewbold bnewbold referenced this pull request Nov 18, 2018

Closed

Upcoming Meeting Agenda - 7 November 2018 #36

0 of 6 tasks complete
@pfrazee

This comment has been minimized.

Copy link
Member Author

pfrazee commented Nov 19, 2018

Updates look good to me!

@emilbayes

This comment has been minimized.

Copy link

emilbayes commented Nov 19, 2018

I particularly liked the privacy/security discussion at the end. It was honest and covered all my "concerns". It's important to know that the public key is a very strong read capability, eg. that anyone with the public key can decrypt any traffic passively observed by any other peers communicating using that public key in their initial connection. So hypercores that are meant to be very sparsely replicated (eg. internet size ones) will leak a lot of metadata.

6
```

If we want to fetch block 0, we need to communicate whether of not we already have the uncles (2, 5) and the parent (3). This information can be compressed into very small bit vector using the following scheme:

This comment has been minimized.

@martinheidegger

martinheidegger Jan 11, 2019

Contributor

@vtduncan and I have been struggling to understand this section.

  1. Why is 1 not required? - After thinking about it, I was able to answer answered this with "1 is not required because we only need to validate the topmost-hash (3) but in order to calculate it we need 2 to calculate one and 1 + 5 to calculate 3". Is this correct? Can this be amended?
  2. How do we know that there are 7 entries? Do we need to figure out first that there are 7 entries before we can do the hash? What if the list grows in the meantime? My answer is: the digest's length specifies how big the merkle tree is we know: If we send 4bit, it means we know 3to7 entries, if we send 8bit, it means we know 8to63 entries?
  3. How would this look like with a less-symetric tree like 8 or 9? (that would make for a better example imo.)
  4. The namings "parent" and "uncle" may be common in merkle-tree nomenclature but it might help to explain the difference briefly.

This comment has been minimized.

@vtduncan

vtduncan Jan 11, 2019

I’m trying using figures to understand how this works. I still don’t grasp it, but here is where I’m at trying to draw the example shown in this section:

hash_tree_digest

This comment has been minimized.

@vtduncan

vtduncan Jan 11, 2019

My question here would be: If you just want to verify chunk 0, why not send this instead?

hash_tree_digest

Another question I had is: when you receive a signature, how do you know what hash that signature is for?

This comment has been minimized.

@vtduncan

vtduncan Jan 11, 2019

For future reference, the code that implements this is in hypercore/tree-index.js and the flat functions within that come from flat-tree/index.js.

This comment has been minimized.

@bnewbold

bnewbold Jan 20, 2019

Contributor

I rewrote this section (and will push in a minute); please let me know if it clarified things. To answer a few questions directly:

The example of wanting to verify block 0 is bad; there's a direct signature of it. I believe that when you request a block, you always get the signature for the root hash as if that was the largest block index, and you're always trying to verify that signature, so I flipped the example to block 6.

This also answers the "how do you know how high the tree is" question.

You don't need hash 1 (in the old example) because you can calculate it from hash 0 and hash 2 (you calculate hash 2 directly from the data transmitted). I guess this is also why you don't need hash 3. Note that in other situations you would need other parent hashes.

I added a reference to the Hypercore DEP which is where the parent/uncle terminology should be defined (if it's unclear there we can take a patch to improve).

A more complex (non-symmetric) example would be nice, but also maybe more confusing here, where we're really just trying to explain the binary packing, not the whole Merkle hashing scheme?

@bnewbold

This comment has been minimized.

Copy link
Contributor

bnewbold commented Jan 20, 2019

I think I've addressed everything above; we should give folks a few days to respond to things before a vote.

The block digest section should get review from an actual implementer (I never implemented that bit; cc: @mafintosh @yoshuawuyts).

Could probably also use another careful proof-read, but for Draft status I think it would be good to get this out and have eyes on it.

bnewbold added some commits Jan 20, 2019

@bnewbold

This comment has been minimized.

Copy link
Contributor

bnewbold commented Jan 20, 2019

To set the timer, I will say I am submitting this for review to be voted on at the next WG meeting, though if there are significant changes needed we can push that off.
I think we discussed "should we wait for any forthcoming implementation changes/refactors in the pipe" and my feeling is to get something out that documents current released/implemented behavior, and do an update or "v2" if needed.

@martinheidegger
Copy link
Contributor

martinheidegger left a comment

Some comments to ease the understanding of the bitfield.

Show resolved Hide resolved proposals/0000-wire-protocol.md Outdated
Show resolved Hide resolved proposals/0000-wire-protocol.md Outdated
Show resolved Hide resolved proposals/0000-wire-protocol.md Outdated
Show resolved Hide resolved proposals/0000-wire-protocol.md Outdated

@bnewbold bnewbold changed the title WIP: Wire protocol (DEP) Draft: Wire protocol (DEP) Feb 13, 2019

@bnewbold bnewbold merged commit 03a1f1d into datprotocol:master Feb 27, 2019

@bnewbold

This comment has been minimized.

Copy link
Contributor

bnewbold commented Feb 27, 2019

Thanks for review anybody!

@bnewbold bnewbold deleted the pfrazee:dep-wire-protocol branch Feb 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.