p2p/pex: Connect to peers from a seed node immediately by ValarDragon · Pull Request #2115 · tendermint/tendermint

ValarDragon · 2018-07-31T07:41:34Z

This is to reduce wait times when initially connecting. This still runs checks
such as whether you still want additional peers.

A test case has been created, which fails if this change is not included.

This is to reduce wait times when initially connecting. This still runs checks
such as whether you still want additional peers.

A test case has been created, which fails if this change is not included.

Closes #2093

Updated all relevant documentation in docs - should I update the docs here?
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG.md

This is to reduce wait times when initially connecting. This still runs checks such as whether you still want additional peers. A test case has been created, which fails if this change is not included.

codecov-io · 2018-07-31T07:50:26Z

Codecov Report

Merging #2115 into develop will decrease coverage by 0.01%.
The diff coverage is 83.33%.

@@             Coverage Diff             @@
##           develop    #2115      +/-   ##
===========================================
- Coverage    62.37%   62.36%   -0.02%     
===========================================
  Files          215      215              
  Lines        17388    17383       -5     
===========================================
- Hits         10846    10841       -5     
- Misses        5653     5655       +2     
+ Partials       889      887       -2

Impacted Files	Coverage Δ
p2p/pex/pex_reactor.go	`73.24% <83.33%> (+1.37%)`	⬆️
rpc/grpc/client_server.go	`66.66% <0%> (-4.77%)`	⬇️
libs/autofile/autofile.go	`66.66% <0%> (-4.77%)`	⬇️
libs/common/repeat_timer.go	`95.23% <0%> (-3.04%)`	⬇️
types/event_bus.go	`81.39% <0%> (-1.22%)`	⬇️
consensus/state.go	`76.47% <0%> (-0.5%)`	⬇️
consensus/reactor.go	`73.55% <0%> (-0.27%)`	⬇️
p2p/peer.go	`57.95% <0%> (ø)`	⬆️
libs/clist/clist.go	`69.06% <0%> (+1.65%)`	⬆️

melekes · 2018-07-31T08:11:08Z

should I update the docs here?

Ideally, you should update p2p pex spec https://github.com/tendermint/tendermint/blob/develop/docs/spec/reactors/pex/pex.md

melekes

🥇 🏎 🍰

xla

👍 🍡

Some minor improvements and I think we can be more efficient with the seed lookup.

xla · 2018-07-31T16:33:02Z

CHANGELOG_PENDING.md

    - only process one block at a time to avoid starving
 - [crypto] Switch hkdfchachapoly1305 to xchachapoly1305
 - [common] bit array functions which take in another parameter are now thread safe
+- [p2p] \#2093 begin connecting to peers as soon a seed node provides them to you


Let's create a markdown link to the issue and have it at the end.

Not sure what you mean by "have it at the end". Do you mean make this a bug fix?

Sorry I was ambiguous. Make the issue reference an actual link and have it at the end of the line, like this:

[p2p] begin connecting to peers as soon a seed node provides them to you ([#2093](https://github.com/tendermint/tendermint/issues/2093))

xla · 2018-07-31T16:34:26Z

p2p/pex/pex_reactor_test.go


+// Creates a seed which knows about the provided addresses / source address pairs.
+// Starting and stopping the seed is left to the caller
+func createSeed(dir string, id int, knownAddrs, srcAddrs []*p2p.NetAddress) *p2p.Switch {


We should make clear this is for testing purposes with a test prefix to avoid collision.

xla · 2018-07-31T16:34:33Z

p2p/pex/pex_reactor_test.go

+
+// Creates a peer which knows about the provided seed.
+// Starting and stopping the peer is left to the caller
+func createPeerWithSeed(dir string, id int, seed *p2p.Switch) *p2p.Switch {


Same as above.

xla · 2018-07-31T16:36:35Z

p2p/pex/pex_reactor.go

+
+		// If this address came from a seed node, try to connect to it without waiting.
+		// TODO: Change this to an O(log(N)) check on a sorted seedAddr list
+		seedAddrs, _ := p2p.NewNetAddressStrings(r.config.Seeds)


As seeds are not changing throughout the lifecycle of the process wouldn't it make sense to create a lookup (map[NetAddress]struct{}) once and use that instead.

Great point!

I'll change it to this for now, but on further thought I'm worried that the constant overhead of a hash function may make this less efficient in the expected case of a few seeds. But this is definitely on the order of things we can optimize post launch!

Which hash function are you referring to?

the internal hash function which will be used in the hashmap. (Namely its memhash here on 64 bit systems: https://golang.org/src/runtime/hash64.go)

I don't think this is something we need to optimise for, even post-launch.

Just ran the golang source runtime benchmarks. Slightly skewed speeds on my system, since it has the very nice hardware AES hash :)

BenchmarkHash5-8 200000000 9.12 ns/op 548.11 MB/s BenchmarkHash16-8 200000000 9.59 ns/op 1668.44 MB/s BenchmarkHash64-8 100000000 13.4 ns/op 4777.66 MB/s BenchmarkHash1024-8 20000000 76.0 ns/op 13467.29 MB/s BenchmarkHash65536-8 300000 4340 ns/op 15097.94 MB/s

For a small number of seeds, a linear scan would probably be faster. Would it be worth benchmarking this, or should I just switch to a hashmap for better asymptotics, and we can optimize this later?

Sorry didn't see your comment. Okay, I'll just switch to a hashmap!

I kept this as a linear scan due to that seeming more readable to me + that being reusable in dialseeds. If you'd prefer a map for readability, I can definitely switch to that :)

p2p/pex: Connect to peers from a seed node immediately

d925a31

This is to reduce wait times when initially connecting. This still runs checks such as whether you still want additional peers. A test case has been created, which fails if this change is not included.

ValarDragon added the C:p2p Component: P2P pkg label Jul 31, 2018

ValarDragon requested a review from xla July 31, 2018 07:41

ValarDragon requested review from ebuchman and melekes as code owners July 31, 2018 07:41

melekes approved these changes Jul 31, 2018

View reviewed changes

xla added this to the launch milestone Jul 31, 2018

xla approved these changes Jul 31, 2018

View reviewed changes

xla added the T:enhancement Type: Enhancement label Jul 31, 2018

xla self-assigned this Jul 31, 2018

ValarDragon added 2 commits July 31, 2018 12:17

(squash this) address PR comments

5fd8523

(squash this) == vs Equals strikes again!

7c8d042

xla merged commit 6fb2f44 into develop Jul 31, 2018

xla deleted the dev/connect_to_seed_peers branch July 31, 2018 20:09

ValarDragon mentioned this pull request Aug 8, 2018

Comments on ADR 18 (ABCI validators) #2177

Closed

cason mentioned this pull request Oct 13, 2022

p2p: dialing addresses provided by a seed node disregards MaxNumOutboundPeers parameter #9548

Closed

cason mentioned this pull request Mar 8, 2023

p2p: dialing addresses provided by a seed node disregards MaxNumOutboundPeers parameter cometbft/cometbft#486

Closed

Conversation

ValarDragon commented Jul 31, 2018

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

melekes commented Jul 31, 2018

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

xla left a comment

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!

Choose a reason for hiding this comment

Uh oh!

ValarDragon Jul 31, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Jul 31, 2018 •

edited

Loading

ValarDragon Jul 31, 2018 •

edited

Loading