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.

Sign up for GitHub

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

Jump to bottom

[P2P][RPC] Rework addnode behaviour #1640

Merged
merged 2 commits into from
May 26, 2020
Merged

[P2P][RPC] Rework addnode behaviour #1640

merged 2 commits into from
May 26, 2020

Conversation

Fuzzbawls
Copy link
Collaborator

Partial backport of bitcoin#8113 as part of #1374.

Original Description:

  • Maintains consistency between ThreadOpenAddedNodeConnections and getaddednodeinfo, without any DNS resolving from RPC.
  • Deals with the use case of added names whose IP mapping changes over time (switching over to the new IP only when the connection to the old one closes, though).
  • Gets rid of the fDns argument to getaddednodeinfo (turning it into a dummy)
  • Simplifies the code significantly by introducing a shared GetAddedNodeInfo in net.cpp.
  • The existing addnode logic that tries all resolved addresses for a name is pushed down into ConnectSocketByName (picking at random).
  • The existing addnode logic to prevent multiple connections to the same IP is pushed down into ConnectNode (by storing colliding names into the CNode's addrName field).

I omitted the last point (commit f9f5cfc) as it conflicts with how our current regtest tests function.

sipa added 2 commits May 22, 2020 20:23
@sipa @Fuzzbawls
Rework addnode behaviour
cdc1d9f 
* Use CNode::addeName to track whether a connection to a name is already open
  * A new connection to a previously-connected by-name addednode is only opened when
    the previous one closes (even if the name starts resolving to something else)
  * At most one connection is opened per addednode (even if the name resolves to multiple)
* Unify the code between ThreadOpenAddedNodeConnections and getaddednodeinfo
  * Information about open connections is always returned, and the dns argument becomes a dummy
  * An IP address and inbound/outbound is only reported for the (at most 1) open connection
@sipa @Fuzzbawls
Randomize name lookup result in ConnectSocketByName
5e79989
@Fuzzbawls Fuzzbawls added RPC P2P Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes labels May 23, 2020
@Fuzzbawls Fuzzbawls added this to the 5.0.0 milestone May 23, 2020
@Fuzzbawls Fuzzbawls self-assigned this May 23, 2020
This was referenced May 23, 2020
Closed
Merged
furszy
furszy approved these changes May 26, 2020
View reviewed changes
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

tested ACK 5e79989

@random-zebra random-zebra added this to Reviewer approved in Update network code from upstream via automation May 26, 2020
random-zebra
random-zebra approved these changes May 26, 2020
View reviewed changes
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

ACK 5e79989 and merging

@random-zebra random-zebra merged commit 0b84a50 into PIVX-Project:master May 26, 2020
Update network code from upstream automation moved this from Reviewer approved to Done May 26, 2020
random-zebra added a commit that referenced this pull request Jun 15, 2020
@random-zebra
Merge #1646: [Net] Turn net structures into dumb storage classes
4c829bb 
b36f743 Fix masternode service lookup (furszy)
65feb7f fixup styling (Fuzzbawls)
059ca2c net: fixup nits (Cory Fields)
5a51a5f net: Have LookupNumeric return a CService directly (Cory Fields)
acd22b7 net: narrow include scope after moving to netaddress (Cory Fields)
e5bea4b net: move CNetAddr/CService/CSubNet out of netbase (Cory Fields)
840d826 net: Add direct tests for new CSubNet constructors (Cory Fields)
63c46c6 net: Split resolving out of CSubNet (Fuzzbawls)
a801a9e net: layer 2 CService abstraction (Fuzzbawls)
94732d2 Simplify checking of masternode.conf (Fuzzbawls)
d6f81b5 RPC: Remove masternodeconnect command (Fuzzbawls)
c4fe27e net: Split resolving out of CService (Cory Fields)
502343a net: Split resolving out of CNetAddr (Cory Fields)

Pull request description:

  Backport of bitcoin#8128 as part of #1374. Based on top of #1640

  Original Description:

  > CNetAddr/CService/CSubNet lose their string constructors, they must now have lookup operations performed externally. This means that functions/classes that depend on them are no longer dependent on any particular lookup mechanism.
  >
  > From a high level: New resolvers/parsers may now be used for net operations. libbtcnet will replace what remains of netbase with async versions.

  Additional work has been done in the following commits:
  * fcef585 - Removes a useless RPC command
  * 235c33e - Refactors the masternode.conf parsing with regards to valid ports
  * 3ddd35e - Adds additional sanity checks during client init for MN port numbers and listen options.

ACKs for top commit:
  furszy:
    tested ACK b36f743
  random-zebra:
    ACK b36f743 and merging...

Tree-SHA512: 1663dc0591e3a4eeb50c1d3265ae125451bd9185e1e44e0cf36d0675fe93daf21d873ba0baa48f0e0e50b20f9313de2da5ee257eeb75c779cd07cebca61a3f99
@Fuzzbawls Fuzzbawls removed the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Sep 26, 2020
@lyricidal lyricidal mentioned this pull request Dec 20, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@furszy furszy furszy approved these changes

@random-zebra random-zebra random-zebra approved these changes

Assignees

@Fuzzbawls Fuzzbawls

Labels
P2P RPC
Projects
No open projects
Update network code from upstream
  
Done
Milestone
4.2.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Fuzzbawls @furszy @random-zebra @sipa