Create adr-050-improved-trusted-peering.md #4072

Hyung-bharvest · 2019-10-22T02:27:27Z

Referenced an issue explaining the need for the change
Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG_PENDING.md

docs/architecture/adr-050-improved-trusted-peering.md

Modify `maximum_dial_period` to `persistent_peers_maximum_dial_period`

Hyung-bharvest · 2019-11-11T05:47:43Z

@melekes @tessr @ebuchman
Is the spec discussed enough for us to PullRequest our implementation?
Waiting for confirmation.

melekes

LGTM

docs/architecture/adr-050-improved-trusted-peering.md

tessr · 2019-11-11T10:58:04Z

docs/architecture/adr-050-improved-trusted-peering.md

+1) `wildcard_peer_ids`
+
+A node operator inputs list of ids of peers which are allowed to be connected by both inbound or outbound regardless of 
+`max_num_inbound_peers` or `max_num_outbound_peers` of user's node reached or not.


Are there limits on the number of trusted/wildcard peers a node can specify? And does adding a trusted/wildcard peer effectively increase the maximum number of peers by 1 (e.g. is the maximum number of inbound peers specified by max_num_inbound_peers + n, where n is the number of wildcard peers)?

There is no limits for wildcard peers. I think it is OK without the limit. Do we need it?

The peers in wildcard_peer_ids is not objective to max_num_inbound_peers. So we don't add n. The state machine just does not check max_num_inbound_peers criteria for any wildcard_peer_ids. Also the state machine just does not count any peer in wildcard_peer_ids to compare max_num_inbound_peers

NOTE: we will have a problem with "wildcard_peer_ids is not objective to max_num_inbound_peers" because there's usually a limited number of connections per OS process (ulimit)

It sounds like we should have some kind of limit here.

I think the OS limit is much far larger than what we need to connect practically. 200 peers are what we practically think maximum, and it is nowhere close to default limit on OS.

Also users can aware that maxpeernum + numofunconditionalpeers = actual possible number of peers. So it is not a unpredictable risk to manage.

For such reason, I think it can be stated in ADR, but we dont think it is needed to be implemented in this spec.

docs/architecture/adr-050-improved-trusted-peering.md

tessr · 2019-11-11T10:59:57Z

docs/architecture/adr-050-improved-trusted-peering.md

+from/to peers in `wildcard_peer_ids`. Also he/she can assure that every persistent peer will be dialed at least once in every 
+`persistent_peers_max_dial_period` term. It achieves more stable and persistent peering for trusted peers.
+
+### Negative


Are there any possible performance downsides?

Additional calculation what we need for the new features

checking whether given peer is listed in wildcard_peer_ids

for persistent_peers, the state machine will dial indefinitely with given period

We think both calculation is not a significant factor for performance downsides.

Co-Authored-By: Tess Rinearson <tess.rinearson@gmail.com>

wildcard -> unconditional

tessr · 2019-11-15T11:58:41Z

I see that you checked the "referenced an issue" box, but I don't see the issue linked. Can you include it in the PR description and the ADR, please?

Hyung-bharvest · 2019-11-15T17:20:21Z

I didn't realize I missed that. Thank you.

//edit
Rechecked it and the link is located at the bottom of the ADR.(reference)

Do you want me to also locate the link in another location?

docs/architecture/adr-050-improved-trusted-peering.md

tessr · 2019-11-19T15:17:22Z

Do you want me to also locate the link in another location?

I think it would be helpful to put it in the PR description so that the Github UI links them together. Thanks!

melekes · 2019-11-20T08:52:03Z

I think it would be helpful to put it in the PR description so that the Github UI links them together. Thanks!

Done.

codecov-io · 2019-11-20T09:02:49Z

Codecov Report

Merging #4072 into master will increase coverage by 0.11%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4072      +/-   ##
==========================================
+ Coverage   66.57%   66.68%   +0.11%     
==========================================
  Files         246      246              
  Lines       21156    21148       -8     
==========================================
+ Hits        14085    14103      +18     
+ Misses       6008     5985      -23     
+ Partials     1063     1060       -3

Impacted Files	Coverage Δ
libs/events/events.go	`93.2% <0%> (-4.86%)`	⬇️
consensus/state.go	`77.84% <0%> (+0.1%)`	⬆️
tools/tm-bench/transacter.go	`8.96% <0%> (+0.46%)`	⬆️
p2p/pex/addrbook.go	`68.34% <0%> (+0.5%)`	⬆️
consensus/reactor.go	`78.49% <0%> (+1.61%)`	⬆️
blockchain/v2/reactor.go	`54.38% <0%> (+3.5%)`	⬆️
types/tx.go	`91.3% <0%> (+4.34%)`	⬆️
p2p/pex/errors.go	`25% <0%> (+12.5%)`	⬆️

…od` (#4176) implementation spec of Improved Trusted Peering ADR-050 by B-Harvest - add unconditional_peer_ids and persistent_peers_max_dial_period to config - add unconditionalPeerIDs map to Switch struct default config value of persistent_peers_max_dial_period is 0s(disabled) Refs #4072, #4053

Create adr-050-improved-trusted-peering.md

e7f2d1e

Hyung-bharvest requested review from ebuchman, melekes and tessr as code owners October 22, 2019 02:27

Hyung-bharvest mentioned this pull request Oct 22, 2019

two p2p feature enhancement proposal #4053

Closed

melekes reviewed Nov 4, 2019

View reviewed changes

docs/architecture/adr-050-improved-trusted-peering.md Outdated Show resolved Hide resolved

melekes reviewed Nov 4, 2019

View reviewed changes

docs/architecture/adr-050-improved-trusted-peering.md Outdated Show resolved Hide resolved

melekes reviewed Nov 4, 2019

View reviewed changes

docs/architecture/adr-050-improved-trusted-peering.md Outdated Show resolved Hide resolved

Hyung-bharvest added 2 commits November 5, 2019 15:15

Modify maximum_dial_period

aa387ab

Modify `maximum_dial_period` to `persistent_peers_maximum_dial_period`

Update adr-050-improved-trusted-peering.md

fc8c1ec

melekes approved these changes Nov 11, 2019

View reviewed changes

tessr reviewed Nov 11, 2019

View reviewed changes

docs/architecture/adr-050-improved-trusted-peering.md Outdated Show resolved Hide resolved

tessr reviewed Nov 11, 2019

View reviewed changes

docs/architecture/adr-050-improved-trusted-peering.md Outdated Show resolved Hide resolved

tessr reviewed Nov 11, 2019

View reviewed changes

docs/architecture/adr-050-improved-trusted-peering.md Outdated Show resolved Hide resolved

tessr reviewed Nov 11, 2019

View reviewed changes

docs/architecture/adr-050-improved-trusted-peering.md Outdated Show resolved Hide resolved

tessr reviewed Nov 11, 2019

View reviewed changes

Hyung-bharvest and others added 5 commits November 14, 2019 16:45

Update docs/architecture/adr-050-improved-trusted-peering.md

cef87d1

Co-Authored-By: Tess Rinearson <tess.rinearson@gmail.com>

Update docs/architecture/adr-050-improved-trusted-peering.md

c08f078

Co-Authored-By: Tess Rinearson <tess.rinearson@gmail.com>

Update docs/architecture/adr-050-improved-trusted-peering.md

4d76d4c

Co-Authored-By: Tess Rinearson <tess.rinearson@gmail.com>

Update docs/architecture/adr-050-improved-trusted-peering.md

0494b3e

Co-Authored-By: Tess Rinearson <tess.rinearson@gmail.com>

wildcard -> unconditional

bb728d0

wildcard -> unconditional

tessr reviewed Nov 19, 2019

View reviewed changes

docs/architecture/adr-050-improved-trusted-peering.md Outdated Show resolved Hide resolved

Hyung-bharvest and others added 2 commits November 20, 2019 01:11

Remove blank lines

604ad9a

Merge branch 'master' into master

95e886b

melekes added 2 commits November 20, 2019 12:53

fix spelling

1c07b2b

add quotes

ab2836a

melekes merged commit acac525 into tendermint:master Nov 20, 2019

dongsam mentioned this pull request Nov 21, 2019

p2p: implement adr-050 #4176

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create adr-050-improved-trusted-peering.md #4072

Create adr-050-improved-trusted-peering.md #4072

Hyung-bharvest commented Oct 22, 2019 •

edited by melekes

Loading

Hyung-bharvest commented Nov 11, 2019 •

edited

Loading

melekes left a comment

tessr Nov 11, 2019

Hyung-bharvest Nov 14, 2019

melekes Nov 15, 2019

tessr Nov 19, 2019

Hyung-bharvest Nov 20, 2019

tessr Nov 11, 2019

Hyung-bharvest Nov 14, 2019

tessr commented Nov 15, 2019

Hyung-bharvest commented Nov 15, 2019 •

edited

Loading

tessr commented Nov 19, 2019

melekes commented Nov 20, 2019

codecov-io commented Nov 20, 2019

Create adr-050-improved-trusted-peering.md #4072

Create adr-050-improved-trusted-peering.md #4072

Conversation

Hyung-bharvest commented Oct 22, 2019 • edited by melekes Loading

Hyung-bharvest commented Nov 11, 2019 • edited Loading

melekes left a comment

Choose a reason for hiding this comment

tessr Nov 11, 2019

Choose a reason for hiding this comment

Hyung-bharvest Nov 14, 2019

Choose a reason for hiding this comment

melekes Nov 15, 2019

Choose a reason for hiding this comment

tessr Nov 19, 2019

Choose a reason for hiding this comment

Hyung-bharvest Nov 20, 2019

Choose a reason for hiding this comment

tessr Nov 11, 2019

Choose a reason for hiding this comment

Hyung-bharvest Nov 14, 2019

Choose a reason for hiding this comment

tessr commented Nov 15, 2019

Hyung-bharvest commented Nov 15, 2019 • edited Loading

tessr commented Nov 19, 2019

melekes commented Nov 20, 2019

codecov-io commented Nov 20, 2019

Codecov Report

Hyung-bharvest commented Oct 22, 2019 •

edited by melekes

Loading

Hyung-bharvest commented Nov 11, 2019 •

edited

Loading

Hyung-bharvest commented Nov 15, 2019 •

edited

Loading