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

HOTFIX: AddressBook requires addresses to have IDs; Do not close conn immediately after sending pex addrs in seed mode #2797

Merged
merged 4 commits into from Nov 11, 2018

Conversation

jaekwon
Copy link
Contributor

@jaekwon jaekwon commented Nov 9, 2018

This solves an issue of having peers without IDs in the address book.

It also fixes #2773, which was a likely cause of there being peers without IDs in the address book in the first place.

It also includes a temporary hack fix for #2092.

@codecov-io
Copy link

codecov-io commented Nov 10, 2018

Codecov Report

Merging #2797 into develop will decrease coverage by 0.06%.
The diff coverage is 36.84%.

@@             Coverage Diff             @@
##           develop    #2797      +/-   ##
===========================================
- Coverage     62.3%   62.23%   -0.07%     
===========================================
  Files          212      212              
  Lines        17210    17225      +15     
===========================================
- Hits         10722    10720       -2     
- Misses        5588     5603      +15     
- Partials       900      902       +2
Impacted Files Coverage Δ
p2p/pex/errors.go 0% <0%> (ø) ⬆️
p2p/pex/addrbook.go 68.91% <0%> (-0.34%) ⬇️
p2p/pex/pex_reactor.go 72.66% <0%> (-0.39%) ⬇️
p2p/node_info.go 89.39% <100%> (+0.16%) ⬆️
p2p/switch.go 51.65% <100%> (ø) ⬆️
p2p/netaddress.go 71.17% <50%> (-1.22%) ⬇️
libs/autofile/autofile.go 76.31% <0%> (-4.51%) ⬇️
privval/tcp_server.go 78.57% <0%> (-2.86%) ⬇️
evidence/reactor.go 63% <0%> (-2%) ⬇️
consensus/wal.go 62.79% <0%> (-0.85%) ⬇️
... and 3 more

@jaekwon jaekwon changed the title HOTFIX: AddressBook requires addresses to have IDs HOTFIX: AddressBook requires addresses to have IDs; Do not crap out immediately after sending pex addrs Nov 10, 2018
@jaekwon jaekwon changed the title HOTFIX: AddressBook requires addresses to have IDs; Do not crap out immediately after sending pex addrs HOTFIX: AddressBook requires addresses to have IDs; Do not crap out immediately after sending pex addrs in seed mode Nov 10, 2018
@melekes
Copy link
Contributor

melekes commented Nov 10, 2018

Looks like a lot of commits are from the current develop. Was this unintentional?

@ebuchman ebuchman changed the base branch from master to develop November 10, 2018 23:47
@ebuchman
Copy link
Contributor

Thanks. Retargeted to develop. Will merge and release v0.26.1.

I took a stab at flushing messages on the conn before stopping in #2802 - should address https://github.com/tendermint/tendermint/pull/2797/files#diff-8c6b1a350a161ced01d68514ccda7484R224

@@ -221,6 +221,7 @@ func (r *PEXReactor) Receive(chID byte, src Peer, msgBytes []byte) {
// 2) limit the output size
if r.config.SeedMode {
r.SendAddrs(src, r.book.GetSelectionWithBias(biasToSelectNewPeers))
time.Sleep(time.Second * 3) // TODO Rethink this. Without it, above may not actually send.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should put these in a go-routine so as to not block Receive

@ebuchman ebuchman changed the title HOTFIX: AddressBook requires addresses to have IDs; Do not crap out immediately after sending pex addrs in seed mode HOTFIX: AddressBook requires addresses to have IDs; Do not close conn immediately after sending pex addrs in seed mode Nov 11, 2018
@ebuchman ebuchman merged commit 5b19fcf into develop Nov 11, 2018
@ebuchman ebuchman deleted the jae/addr_noid branch November 11, 2018 11:50
- [p2p] [\#2668](https://github.com/tendermint/tendermint/issues/2668) Reconnect to originally dialed address (not self-reported
address) for persistent peers
- [p2p] [\#2668](https://github.com/tendermint/tendermint/issues/2668) Reconnect to originally dialed address (not self-reported address) for persistent peers
- [p2p] [\#2797](https://github.com/tendermint/tendermint/pull/2797) AddressBook requires addresses to have IDs; Do not crap out immediately after sending pex addrs in seed mode
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong changelog file :o ! Please use CHANGELOG_PENDING.md

maxim-levy pushed a commit to maxim-levy/tendermint that referenced this pull request Nov 13, 2018
* 'master' of https://github.com/tendermint/tendermint: (330 commits)
  Release/v0.26.1 (tendermint#2803)
  fix amino overhead computation for Tx (tendermint#2792)
  p2p: re-check after sleeps (tendermint#2664)
  check the result of `ps.peer.Send` before calling `ps.setHasVote` (tendermint#2787)
  p2p: AddressBook requires addresses to have IDs; Do not close conn immediately after sending pex addrs in seed mode (tendermint#2797)
  test AutoFile#Size (happy path)
  [autofile/group] do not panic when checking size
  openFile creates a file if not exist => ErrNotExist is not possible
  use our logger in autofile/group
  Add tests for ValidateBasic methods (tendermint#2754)
  [docs] improve organization of ABCI docs & fix links (tendermint#2749)
  p2p: peer-id -> peer_id (tendermint#2771)
  mempool: print postCheck error (tendermint#2762)
  Fix crypto/merkle ProofOperators.Verify to check bounds on keypath pa… (tendermint#2756)
  Mempool WAL is still created by default in home directory, leads to permission errors (tendermint#2758)
  mempool: ErrPreCheck and more log info (tendermint#2724)
  Release/v0.26.0 (tendermint#2726)
  [ADR] [DRAFT] pubsub 2.0 (tendermint#2532)
  validate reactor messages (tendermint#2711)
  TMHASH is 32 bytes. Closes tendermint#1990 (tendermint#2732)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants