For Review: P2P docs by ebuchman · Pull Request #1057 · tendermint/tendermint

ebuchman · 2018-01-04T17:34:55Z

These docs are already on develop but should have gone through PR for review tools

codecov-io · 2018-01-04T18:07:51Z

Codecov Report

❗ No coverage uploaded for pull request base (p2p-docs-compare@008de93). Click here to learn what that means.
The diff coverage is n/a.

@@                 Coverage Diff                 @@
##             p2p-docs-compare    #1057   +/-   ##
===================================================
  Coverage                    ?   59.95%           
===================================================
  Files                       ?      116           
  Lines                       ?    10667           
  Branches                    ?        0           
===================================================
  Hits                        ?     6395           
  Misses                      ?     3701           
  Partials                    ?      571

milosevic · 2018-01-05T10:37:00Z

p2p/docs/config.md

+
+`--p2p.seed_mode`
+
+The node operates in seed mode. It will kick incoming peers after sharing some peers.


What about: The node operates in seed mode. In seed mode, a node continuously crawls the network for peers, and upon incoming connection shares some peers and disconnect.

milosevic · 2018-01-05T10:41:35Z

p2p/docs/config.md

+Note that the auto-redial uses exponential backoff and will give up
+after a day of trying to connect.
+
+NOTE: If `dial_seeds` and `persistent_peers` intersect,


Is it seeds instead of dial_seeds?

milosevic · 2018-01-05T10:46:03Z

p2p/docs/connection.md

+
+`MConnection` is a multiplex connection:
+
+__multiplex__ *noun* a system or signal involving simultaneous transmission of


I don't understand this definition. If I am not wrong any TCP connection satisfies this definition as you can send several messages over it.

This is the stock google definition.

Afaik, TCP is typically considered a single stream - you read and write one stream of messages. This is an abstraction to support reading/writing on multiple streams

milosevic · 2018-01-05T10:49:16Z

p2p/docs/connection.md

+
+Each `MConnection` handles message transmission on multiple abstract communication
+`Channel`s.  Each channel has a globally unique byte id.
+The byte id and the relative priorities of each `Channel` are configured upon


If I understand well, MConnection is different than "simple" TCP connection as it provides support for different quality of service guarantees (priorities, bandwidth provided, etc) for different channels sent over it. Is this right? Maybe it can be phrased slightly different so it's more clear what is special about MConnection.

milosevic · 2018-01-05T10:50:10Z

p2p/docs/connection.md

+### Ping and Pong
+
+The ping and pong messages consist of writing a single byte to the connection; 0x1 and 0x2, respectively
+


. missing at the end of previous sentence.

milosevic · 2018-01-05T11:02:11Z

p2p/docs/connection.md

+The ping and pong messages consist of writing a single byte to the connection; 0x1 and 0x2, respectively
+
+When we haven't received any messages on an `MConnection` in a time `pingTimeout`, we send a ping message.
+When a ping is received on the `MConnection`, a pong is sent in response.


Do we always send pong or only if there is no msg to be sent? Also we probably want to have some timeout between two consecutive pongs, i.e., we don't want to react upon every ping as this might be DDoS attack.

this is a very good question

there is a TODO in code

milosevic · 2018-01-05T11:07:02Z

p2p/docs/connection.md

+
+### Msg
+
+Messages in channels are chopped into smaller msgPackets for multiplexing.


Not clear why this is done? TCP already provides some support cutting big messages in smaller chunks. I guess it's part of MConnection way of providing different QoS for different channels?

Yes I believe that's right

milosevic · 2018-01-05T11:17:18Z

p2p/docs/connection.md

+
+Messages are sent from a single `sendRoutine`, which loops over a select statement that results in the sending
+of a ping, a pong, or a batch of data messages. The batch of data messages may include messages from multiple channels.
+Message bytes are queued for sending in their respective channel, with each channel holding one unsent message at a time.


Why only one unsent message at a time? We might be missing some high level design goal regarding multiplexing. We should maybe try to take advantage with writing this spec to state what we want and why it matters, and then the code will be the current best way of how to provide it. By stating what and why we basically open call for innovation (different ways of implementing it) by community.

milosevic · 2018-01-05T11:35:31Z

p2p/docs/connection.md

+for the channel with the given id byte `chID`.  The message `msg` is serialized
+using the `tendermint/wire` submodule's `WriteBinary()` reflection routine.
+
+`TrySend(chID, msg)` is a nonblocking call that returns false if the channel's


What about: is a nonblocking call that queues the message msg in the channel with the given id byte chID if the queue is not full; otherwise it returns false immediately.

milosevic · 2018-01-05T11:40:05Z

p2p/docs/connection.md

+
+### PexReactor/AddrBook
+
+A `PEXReactor` reactor implementation is provided to automate peer discovery.


What PEX stands for?

Peer Exchange

milosevic · 2018-01-05T11:42:17Z

p2p/docs/connection.md

+...
+
+// Send a random message to all outbound connections
+for _, peer := range switch.Peers().List() {


What is illustrated with the following code snippet?

just an example of using the library. in this case to send a random message to all outbound connections (ie. peers that we dialed)

milosevic · 2018-01-05T17:47:42Z

p2p/docs/node.md

+
+Restarted full nodes can run the `blockchain` or `consensus` reactor protocols to sync up
+to the latest state of the blockchain, assuming they aren't too far behind.
+If they are too far behind, they may need to validate a recent `H` and `HASH` out-of-band again.


Don't understand this. In order to validate recent hash it needs to start from the genesis, right? Isn't it always slower then catching up?

"validate recent hash" is a social excercise. the idea here is if you're far enough behind, you are vulnerable to nothing-at-stake attacks, so when you sync up to some recent block, you need to validate its hash socially to know you're on the right chain

milosevic · 2018-01-05T18:03:18Z

p2p/docs/node.md

+## Sentry Node
+
+Sentry nodes are guardians of a validator node and provide it access to the rest of the network.
+Sentry nodes may be dynamic, but should maintain persistent connections to some evolving random subset of each other.


Sentry node can also probably connect to Full Nodes.

milosevic · 2018-01-05T18:06:33Z

p2p/docs/peer.md

+
+## Peer Identity
+
+Tendermint peers are expected to maintain long-term persistent identities in the form of a private key.


Should it be public key?

milosevic · 2018-01-05T18:08:24Z

p2p/docs/peer.md

+## Peer Identity
+
+Tendermint peers are expected to maintain long-term persistent identities in the form of a private key.
+Each peer has an ID defined as `peer.ID == peer.PrivKey.Address()`, where `Address` uses the scheme defined in go-crypto.


Should it be peer.ID == peer.PubKey.Address()?

milosevic · 2018-01-05T18:09:06Z

p2p/docs/peer.md

+Tendermint peers are expected to maintain long-term persistent identities in the form of a private key.
+Each peer has an ID defined as `peer.ID == peer.PrivKey.Address()`, where `Address` uses the scheme defined in go-crypto.
+
+Peer ID's must come with some Proof-of-Work; that is,


Can be removed as PoW for peer id is dropped.

milosevic · 2018-01-05T18:10:02Z

p2p/docs/peer.md

+This ensures they are not too easy to generate. To begin, let `target == 2^240`.
+
+A single peer ID can have multiple IP addresses associated with it.
+For simplicity, we only keep track of the latest one.


If I am not wrong, there was a case mentioned on meeting where we benefit from keeping multiple keys.

milosevic · 2018-01-05T18:10:51Z

p2p/docs/peer.md

+
+Peers can also be connected to without specifying an ID, ie. just `<IP>:<PORT>`.
+In this case, the peer must be authenticated out-of-band of Tendermint,
+for instance via VPN


. is missing at the end of the sentence.

milosevic · 2018-01-05T18:12:19Z

p2p/docs/peer.md

+    - flip the last bit of nonce1 to get nonce2
+    - if we had the smaller ephemeral pubkey, use nonce1 for receiving, nonce2 for sending;
+        else the opposite
+- all communications from now on are encrypted using the shared secret and the nonces, where each nonce


It looks like sentence is not complete.

milosevic · 2018-01-05T18:16:29Z

p2p/docs/peer.md

+
+### Peer Filter
+
+Before continuing, we check if the new peer has the same ID as ourselves or


As we don't assume some kind of central CA to exist for id/key management, it can happen (although with very small probability I guess) that two nodes exist in the network with the same id (public key). Is this a problem?

No - that would require a private key collision and we consider those sufficiently improbable. If we had to worry about it here, we'd have to worry about it in the app layer too, and cryptocurrencies wouldn't work

milosevic · 2018-01-05T18:17:15Z

p2p/docs/peer.md

+
+We also check the peer's address and public key against
+an optional whitelist which can be managed through the ABCI app -
+if the whitelist is enabled and the peer does not qualigy, the connection is


Typo: qualify instead of qualigy.

milosevic · 2018-01-05T18:18:24Z

p2p/docs/peer.md

+
+```
+type NodeInfo struct {
+	PubKey     crypto.PubKey `json:"pub_key"`


Maybe remove json stuff.

melekes · 2018-01-06T15:25:25Z

p2p/docs/config.md

+
+`--p2p.seeds “1.2.3.4:466656,2.3.4.5:4444”`
+
+Dials these seeds when we need more peers. They will return a list of peers and then disconnect.


will -> should?? i.e. what if one node from this list is not in "seed mode"?

melekes · 2018-01-06T15:50:54Z

p2p/docs/peer.md

+- all communications from now on are encrypted using the shared secret and the nonces, where each nonce
+- we now have an encrypted channel, but still need to authenticate
+increments by 2 every time it is used
+- generate a common challenge to sign:


why this is called a challenge? i.e. I do not see any challenge here? what's challenging about signing a msg?

The challenge is: "prove you have the privkey for this pubkey by signing this msg"

ebuchman · 2018-01-07T22:30:02Z

thanks for the feedback folks. see updates in https://github.com/tendermint/tendermint/pull/1076/files

milosevic · 2018-01-08T11:22:26Z

Looks great!

caffix

Overall, the information provided within these files is quite useful. Great!

caffix · 2018-01-15T17:52:43Z

p2p/docs/connection.md

+Messages are sent from a single `sendRoutine`, which loops over a select statement that results in the sending
+of a ping, a pong, or a batch of data messages. The batch of data messages may include messages from multiple channels.
+Message bytes are queued for sending in their respective channel, with each channel holding one unsent message at a time.
+Messages are chosen for a batch one a time from the channel with the lowest ratio of recently sent bytes to channel priority.


Typo: I believe it was supposed to say, "Messages are chosen for a batch one at a time ..."

caffix · 2018-01-15T20:59:58Z

p2p/docs/peer.md

+# Tendermint Peers
+
+This document explains how Tendermint Peers are identified, how they connect to one another,
+and how other peers are found.


This file does not yet appear to discuss how peers are found as this section claims. Perhaps further discussion is in order, or this claim should be removed?

caffix · 2018-01-15T21:44:22Z

p2p/docs/pex.md

+There are various cases where we decide a peer has misbehaved and we disconnect from them.
+When this happens, the peer is removed from the address book and black listed for
+some amount of time. We call this "Disconnect and Mark".
+Note that the bad behaviour may be detected outside the PEX reactor itseld


Typo: "itseld" was probably meant to be "itself"

caffix · 2018-01-15T21:49:10Z

p2p/docs/pex.md

+## Trust Metric
+
+The quality of peers can be tracked in more fine-grained detail using a
+Proportional-Integral-Derrivative (PID) controller that incorporates


Typo: "Derrivative" should be "Derivative"

caffix · 2018-01-15T21:57:38Z

p2p/docs/trustmetric.md

+Behaviours are defined as one of:
+    - fatal - something outright malicious. we should disconnect and remember them.
+    - bad - any kind of timeout, msgs that dont unmarshal, or fail other validity checks, or msgs we didn't ask for or arent expecting
+    - neutral - normal correct behaviour. unknown channels/msg types (version upgrades).


We discussed neutral behavior being used for unknown channels, etc, and a separate type of behavior named "Correct" being used as the opposite of Bad. This way, good is reserved for behavior that is exceptionally positive.

caffix · 2018-01-15T21:59:09Z

p2p/docs/trustmetric.md

@@ -0,0 +1,16 @@
+
+The trust metric tracks the quality of the peers.


Additional detail could be provided in this file using information from the architecture documents if you wish.

ebuchman · 2018-01-19T22:52:30Z

Thanks @caffix . Fixes in #1123

…#1311) * node/state:bootstrap state api (tendermint#1057) Co-authored-by: HuangYi <huang@crypto.com> Co-authored-by: yihuang <yi.codeplayer@gmail.com> Co-authored-by: Thane Thomson <connect@thanethomson.com> Co-authored-by: Sergio Mena <sergio@informal.systems> (cherry picked from commit 49127d2) # Conflicts: # state/mocks/store.go * Fixed merge conflict --------- Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com>

* add bootstrap state cmd * add a missing line * Initial API impl * Added error message for missing appHash * Added changelog, removed cli * Added PR number * Unified hex encoding with rest of the code * Applied PR review comments * Proper blockstore initialization in case of offline statesync * Reverted forcing blocksync, not needed for correct operation * Added changelog and comments * Removed printfs, added check for empty state store * Fixed linter * Apply minor suggestions from code review Co-authored-by: Thane Thomson <connect@thanethomson.com> * Moved the appHash check up * Apply minor suggestions from code review Co-authored-by: Sergio Mena <sergio@informal.systems> * Apply suggestions from code review Co-authored-by: Sergio Mena <sergio@informal.systems> * Fixed linter * Do not look for VE when starting up after offline statesync * Extracted check for offline statesync outside load commit * Reconstruct seen commit after offline statesync * Call reconstructSeenCommit from reconstructLastCommit * Reading offline statesync height only once and passing it as a parameter * Moved up option initialization to make sure offline statesync is enabled * Added error to panic message * Update consensus/state.go Co-authored-by: Sergio Mena <sergio@informal.systems> * Apply suggestions from code review Co-authored-by: Sergio Mena <sergio@informal.systems> * Adjusted new lines * Added unit test to test int conversion and fixed linter * Apply suggestions from code review Co-authored-by: Thane Thomson <connect@thanethomson.com> * Replaced closing ifs with defer, added errors to error messages * linter fix * Adapted bootstrap code to use proper genesis file functions * Reverted genesis doc changes * Moved deferred closing before checking for whether the store is empty * Moved deferred close before error check --------- Co-authored-by: HuangYi <huang@crypto.com> Co-authored-by: yihuang <yi.codeplayer@gmail.com> Co-authored-by: Thane Thomson <connect@thanethomson.com> Co-authored-by: Sergio Mena <sergio@informal.systems>

* node/state:bootstrap state api (tendermint#1057) * add bootstrap state cmd * add a missing line * Initial API impl * Added error message for missing appHash * Added changelog, removed cli * Added PR number * Unified hex encoding with rest of the code * Applied PR review comments * Proper blockstore initialization in case of offline statesync * Reverted forcing blocksync, not needed for correct operation * Added changelog and comments * Removed printfs, added check for empty state store * Fixed linter * Apply minor suggestions from code review Co-authored-by: Thane Thomson <connect@thanethomson.com> * Moved the appHash check up * Apply minor suggestions from code review Co-authored-by: Sergio Mena <sergio@informal.systems> * Apply suggestions from code review Co-authored-by: Sergio Mena <sergio@informal.systems> * Fixed linter * Do not look for VE when starting up after offline statesync * Extracted check for offline statesync outside load commit * Reconstruct seen commit after offline statesync * Call reconstructSeenCommit from reconstructLastCommit * Reading offline statesync height only once and passing it as a parameter * Moved up option initialization to make sure offline statesync is enabled * Added error to panic message * Update consensus/state.go Co-authored-by: Sergio Mena <sergio@informal.systems> * Apply suggestions from code review Co-authored-by: Sergio Mena <sergio@informal.systems> * Adjusted new lines * Added unit test to test int conversion and fixed linter * Apply suggestions from code review Co-authored-by: Thane Thomson <connect@thanethomson.com> * Replaced closing ifs with defer, added errors to error messages * linter fix * Adapted bootstrap code to use proper genesis file functions * Reverted genesis doc changes * Moved deferred closing before checking for whether the store is empty * Moved deferred close before error check --------- Co-authored-by: HuangYi <huang@crypto.com> Co-authored-by: yihuang <yi.codeplayer@gmail.com> Co-authored-by: Thane Thomson <connect@thanethomson.com> Co-authored-by: Sergio Mena <sergio@informal.systems> * Fixed cherry pick conflicts * Removed go API changes in state store * Remove breaking go API changes in the blocksync reactor * Removed breaking changelog and fixed mocks * [pair programming] Found a more readable way to add the methods needed for the store * Removed duplicated code from blocksync reactor * Apply suggestions from code review Co-authored-by: Sergio Mena <sergio@informal.systems> * Removed comments * Removed unused variable from state --------- Co-authored-by: HuangYi <huang@crypto.com> Co-authored-by: yihuang <yi.codeplayer@gmail.com> Co-authored-by: Thane Thomson <connect@thanethomson.com> Co-authored-by: Sergio Mena <sergio@informal.systems>

ebuchman added 2 commits December 31, 2017 17:11

p2p docs

1acb12e

more p2p docs

bc71840

ebuchman requested a review from melekes as a code owner January 4, 2018 17:34

ebuchman mentioned this pull request Jan 4, 2018

docs: P2P Documentation #1052

Merged

milosevic reviewed Jan 5, 2018

View reviewed changes

p2p/docs/peer.md

```

type NodeInfo struct {

PubKey crypto.PubKey `json:"pub_key"`

Copy link

Contributor

milosevic Jan 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe remove json stuff.

melekes reviewed Jan 6, 2018

View reviewed changes

ebuchman mentioned this pull request Jan 7, 2018

docs/p2p: updates from review #1076

Merged

caffix suggested changes Jan 15, 2018

View reviewed changes

ebuchman closed this Jan 19, 2018

ebuchman deleted the p2p-docs branch January 21, 2018 05:39


		`--p2p.seed_mode`

		The node operates in seed mode. It will kick incoming peers after sharing some peers.


		`MConnection` is a multiplex connection:

		__multiplex__ noun a system or signal involving simultaneous transmission of

		### Ping and Pong

		The ping and pong messages consist of writing a single byte to the connection; 0x1 and 0x2, respectively


		### Msg

		Messages in channels are chopped into smaller msgPackets for multiplexing.


		### PexReactor/AddrBook

		A `PEXReactor` reactor implementation is provided to automate peer discovery.


		## Peer Identity

		Tendermint peers are expected to maintain long-term persistent identities in the form of a private key.


		### Peer Filter

		Before continuing, we check if the new peer has the same ID as ourselves or


		`--p2p.seeds “1.2.3.4:466656,2.3.4.5:4444”`

		Dials these seeds when we need more peers. They will return a list of peers and then disconnect.

		@@ -0,0 +1,16 @@

		The trust metric tracks the quality of the peers.

Conversation

ebuchman commented Jan 4, 2018

Uh oh!

codecov-io commented Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

milosevic Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milosevic Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milosevic Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebuchman commented Jan 7, 2018

codecov-io commented Jan 4, 2018 •

edited

Loading

milosevic Jan 5, 2018 •

edited

Loading

milosevic Jan 5, 2018 •

edited

Loading

milosevic Jan 5, 2018 •

edited

Loading