Skip to content

net: prerequisites for p2p encapsulation changes #7906

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
Merged
merged 7 commits into from
May 18, 2016
Merged

net: prerequisites for p2p encapsulation changes #7906

merged 7 commits into from
May 18, 2016

Conversation

theuni
Copy link
Member

@theuni theuni commented Apr 18, 2016

A few boring changes that are required before beginning to encapsulate the p2p code. For reference, https://github.com/theuni/bitcoin/tree/net-refactor12 builds on top of these as well as #7868. The high-level purpose of this work is to encapsulate p2p functionality into a single class rather than scattered globals, so that It may be safely changed without worrying about side-effects.

Regardless of that, I think these are all improvements in their own right. See individual commit messages for more info.

@sipa
Copy link
Member

sipa commented Apr 20, 2016

Is SetBannedIsDirty still needed after this?

@sipa
Copy link
Member

sipa commented Apr 20, 2016

utACK b33a17803a067d98c48f22b95eb4a5297eb03944

@btcdrak
Copy link
Contributor

btcdrak commented Apr 20, 2016

Concept ACK

maflcko
maflcko reviewed Apr 20, 2016
View reviewed changes
src/net.cpp
@@ -644,7 +679,7 @@ void CNode::copyStats(CNodeStats &stats)
stats.dPingMin = (((double)nMinPingUsecTime) / 1e6);
stats.dPingWait = (((double)nPingUsecWait) / 1e6);

// Leave string empty if addrLocal invalid (not filled in yet)
// Leave std::string empty if addrLocal invalid (not filled in yet)
Copy link
Member

Choose a reason for hiding this comment

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

No need to change this as well, but it doesn't hurt much, either.

Copy link
Member Author

Choose a reason for hiding this comment

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

Heh, overzealous sed. Thanks, will fix.

@maflcko
Copy link
Member

maflcko commented Apr 20, 2016

Concept ACK

@theuni
Copy link
Member Author

theuni commented Apr 20, 2016

@sipa The flag is still needed for non-user-initiated bans. As implemented here, Ban() doesn't flush to disk automatically for p2p violations.

@theuni
Copy link
Member Author

theuni commented Apr 20, 2016

Added a quick fixup for the string comment rather than rebasing and invalidating ACKs. I can squash before merge.

@sipa
Copy link
Member

sipa commented Apr 21, 2016

utACK 8d18913269e512b122e835b44ce1c3296a204177

@laanwj
Copy link
Member

laanwj commented Apr 21, 2016

Needs rebase after #7868

@theuni theuni force-pushed the net-refactor-prerequisites branch from 8d18913 to 8fe811b Compare April 21, 2016 17:13
@theuni
Copy link
Member Author

theuni commented Apr 21, 2016

Rebased

paveljanik
paveljanik reviewed Apr 22, 2016
View reviewed changes
src/qt/rpcconsole.cpp
@@ -892,8 +892,6 @@ void RPCConsole::banSelectedNode(int bantime)
SplitHostPort(nStr, port, addr);

CNode::Ban(CNetAddr(addr), BanReasonManuallyAdded, bantime);
bannedNode->fDisconnect = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

The variable bannedNode seems to be unused here now.

The similar code is a few lines above in RPCConsole::disconnectSelectedNode() - same changes needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

@paveljanik Yea, bannedNode is now unused here. I'll remove.

I only addressed banning here, so no changes needed for disconnectSelectedNode. Disconnect handling comes as a next step.

@paveljanik
Copy link
Contributor

Concept ACK

@laanwj
Copy link
Member

laanwj commented Apr 28, 2016

utACK 8fe811b (please get rid of the dummy commit)

@theuni theuni force-pushed the net-refactor-prerequisites branch from 0c756e0 to 8fe811b Compare April 29, 2016 19:35
@laanwj
Copy link
Member

laanwj commented May 4, 2016

Needs rebase

theuni added 4 commits May 5, 2016 13:22
@theuni
net: don't import std namespace
52cbce2 
This file is about to be broken up into chunks and moved around. Drop the
namespace now rather than requiring other files to use it.
@theuni
net: remove unused set
9faa490
@theuni
net: use the exposed GetNodeSignals() rather than g_signals directly
563f375
@theuni
net: Drop CNodeRef for AttemptToEvictConnection
cca221f 
Locking for each operation here is unnecessary, and solves the wrong problem.
Additionally, it introduces a problem when cs_vNodes is held in an owning
class, to which invididual CNodeRefs won't have access.

These should be weak pointers anyway, once vNodes contain shared pointers.

Rather than using a refcounting class, use a 3-step process instead.

1. Lock vNodes long enough to snapshot the fields necessary for comparing
2. Unlock and do the comparison
3. Re-lock and mark the resulting node for disconnection if it still exists
@theuni theuni force-pushed the net-refactor-prerequisites branch from 8fe811b to b01182f Compare May 5, 2016 18:06
@sipa
Copy link
Member

sipa commented May 7, 2016

@theuni Can you address d48445a#r60804931 ?

@theuni
Copy link
Member Author

theuni commented May 8, 2016

@sipa ah sorry, I missed that note. I'll have a look Monday.

theuni added 3 commits May 10, 2016 12:28
@theuni
net: make Ban/Unban/ClearBan functionality consistent
8b8f877 
- Ban/Unban/ClearBan call uiInterface.BannedListChanged() as necessary
- Ban/Unban/ClearBan sync to disk if the operation is user-invoked
- Mark node for disconnection automatically when banning
- Lock cs_vNodes while setting disconnected
- Don't spin in a tight loop while setting disconnected
@theuni
net: No need to export DumpBanlist
e9ed620
@theuni
net: No need to export ConnectNode
5d5e7a0
@theuni theuni force-pushed the net-refactor-prerequisites branch from b01182f to 5d5e7a0 Compare May 10, 2016 16:37
@sipa
Copy link
Member

sipa commented May 10, 2016

reutACK 5d5e7a0

@laanwj laanwj merged commit 5d5e7a0 into bitcoin:master May 18, 2016
laanwj added a commit that referenced this pull request May 18, 2016
@laanwj
Merge #7906: net: prerequisites for p2p encapsulation changes
83121cc 
5d5e7a0 net: No need to export ConnectNode (Cory Fields)
e9ed620 net: No need to export DumpBanlist (Cory Fields)
8b8f877 net: make Ban/Unban/ClearBan functionality consistent (Cory Fields)
cca221f net: Drop CNodeRef for AttemptToEvictConnection (Cory Fields)
563f375 net: use the exposed GetNodeSignals() rather than g_signals directly (Cory Fields)
9faa490 net: remove unused set (Cory Fields)
52cbce2 net: don't import std namespace (Cory Fields)
@OlegGirko OlegGirko mentioned this pull request Jul 11, 2017
Merged
@Fuzzbawls Fuzzbawls mentioned this pull request Jun 23, 2020
Merged
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request Jul 1, 2020
@random-zebra
Merge #1704: [Net] Pre-requirements for network encapsulation
8bbc065 
6f41b3e [QA] Missing mempool sync in pos_coldStaking and zc_publicspends tests (random-zebra)
efaf727 net: correctly initialize nMinPingUsecTime (Wladimir J. van der Laan)
61c8ffe Do not add random inbound peers to addrman. (Gregory Maxwell)
e6a1726 Added feeler connections increasing good addrs in the tried table. (Ethan Heilman)
070b6fb Actually only use filterInventoryKnown with MSG_TX inventory messages. (Gregory Maxwell)
9ac6b28 Only use filterInventoryKnown with MSG_TX inventory messages. (Patick Strateman)
01273db Rename setInventoryKnown filterInventoryKnown (Patick Strateman)
4f11eb2 Remove mruset as it is no longer used. (Gregory Maxwell)
2e3b05c Replace setInventoryKnown with a rolling bloom filter. (Gregory Maxwell)
409aa83 Replace trickle nodes with per-node/message Poisson delays (Pieter Wuille)
93e8c46 Move recentRejects initialization to top of InitBlockIndex (Wladimir J. van der Laan)
9a59420 Keep track of recently rejected transactions (Peter Todd)
34ee777 Only use randomly created nonces in CRollingBloomFilter. (Pieter Wuille)
338d346 Make CRollingBloomFilter set nTweak for you (Peter Todd)
dcd15bc Reuse vector hashing code for uint256 (Pieter Wuille)
3230143 Add uint256 support to CRollingBloomFilter (Peter Todd)
128d644 Better mruset unit test (Pieter Wuille)
89740ed Use ring buffer of set iterators instead of deque of copies in mruset (Pieter Wuille)
14c88ee Replace mruset setAddrKnown with CRollingBloomFilter addrKnown (Gavin Andresen)
e0bebbd Rolling bloom filter class (Gavin Andresen)
7c03bd5 Add correct bool combiner for net signals (Pieter Wuille)
5cb5fd6 Stop exporting ConnectNode (Fuzzbawls)
819295d Stop using ConnectNode in layer 2 code (Fuzzbawls)
851b6b4 net: No need to export DumpBanlist (Cory Fields)
4486d4e net: make Ban/Unban/ClearBan functionality consistent (Cory Fields)
a5e278d net: Drop CNodeRef for AttemptToEvictConnection (Cory Fields)
9fd357d net: use the exposed GetNodeSignals() rather than g_signals directly (Cory Fields)
7962bcc net: remove unused set (Cory Fields)
fabf358 Use network group instead of CNetAddr in final pass to select node to disconnect (Patrick Strateman)
7f030fe Fix comment (Patrick Strateman)
18af800 Acquire cs_vNodes before changing refrence counts (Patrick Strateman)
7aa827f CNodeRef copy constructor and assignment operator (Patrick Strateman)
b3f95e7 Return false early if vEvictionCandidates is empty (Patrick Strateman)
85886c9 Better support for nodes with non-standard nMaxConnections (Patrick Strateman)
9c9e55b RAII wrapper for CNode* (Patrick Strateman)
e92780d Add comments to AttemptToEvictConnection (Patrick Strateman)
0ca7ce3 Prefer to disconnect peers in favor of whitelisted peers (Patrick Strateman)
a1c4aaf AttemptToEvictConnection (Patrick Strateman)
aa7ce9b Record nMinPingUsecTime (Patrick Strateman)
fd7bab0 Refactor: Move failure conditions to the top of AcceptConnection (Patrick Strateman)
fcb732b Refactor: Bail early in AcceptConnection (Patrick Strateman)
411766d Refactor: AcceptConnection (Patrick Strateman)

Pull request description:

  This is a culmination of several upstream PRs touching the P2P/Networking code, and resulting in a state just prior to the P2P/Network encapsulation, which will be it's own PR.

  Backported upstream PRs included here:
  - bitcoin#5859
  - bitcoin#6064
  - bitcoin#6374
  - bitcoin#6498
  - bitcoin#6636
  - bitcoin#7133
  - bitcoin#7125
  - bitcoin#7906
  - bitcoin#8282
  - bitcoin#8594

ACKs for top commit:
  random-zebra:
    Great job. ACK 6f41b3e and merging...
  furszy:
    Have been running it the past days and all is looking good, ACK 6f41b3e .

Tree-SHA512: 1cc4b1271516f000a06141b5b069f3ee00f3eb77056e40d2c021c484f749d9d8db2b76ce490f63572372705b646fad342666f6f90ca5fc69abcacf7b207d058f
@str4d str4d mentioned this pull request Nov 11, 2020
Merged
zkbot added a commit to zcash/zcash that referenced this pull request Nov 12, 2020
@zkbot
Auto merge of #2812 - str4d:2074-store-banlist, r=<try>
f401214 
banlist.dat: store banlist on disk

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6310
- bitcoin/bitcoin#6315
  - Only the new signal and net changes, no QT.
- bitcoin/bitcoin#7350
- bitcoin/bitcoin#7458
- bitcoin/bitcoin#7696
- bitcoin/bitcoin#7959
- bitcoin/bitcoin#7906
  - Only the ban-related commits.
- bitcoin/bitcoin#8392
  - Only the second commit.
- bitcoin/bitcoin#8085
  - Only the second commit.
- bitcoin/bitcoin#10564

Part of #2074.
zkbot added a commit to zcash/zcash that referenced this pull request Feb 11, 2021
@zkbot
Auto merge of #2812 - str4d:2074-store-banlist, r=<try>
049951d 
banlist.dat: store banlist on disk

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6310
- bitcoin/bitcoin#6315
  - Only the new signal and net changes, no QT.
- bitcoin/bitcoin#7350
- bitcoin/bitcoin#7458
- bitcoin/bitcoin#7696
- bitcoin/bitcoin#7959
- bitcoin/bitcoin#7906
  - Only the ban-related commits.
- bitcoin/bitcoin#8392
  - Only the second commit.
- bitcoin/bitcoin#8085
  - Only the second commit.
- bitcoin/bitcoin#10564

Part of #2074.
This was referenced Feb 11, 2021
Closed
Merged
zkbot added a commit to zcash/zcash that referenced this pull request Feb 17, 2021
@zkbot
Auto merge of #2812 - str4d:2074-store-banlist, r=<try>
e85265f 
banlist.dat: store banlist on disk

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6310
- bitcoin/bitcoin#6315
  - Only the new signal and net changes, no QT.
- bitcoin/bitcoin#7350
- bitcoin/bitcoin#7458
- bitcoin/bitcoin#7696
- bitcoin/bitcoin#7959
- bitcoin/bitcoin#7906
  - Only the ban-related commits.
- bitcoin/bitcoin#8392
  - Only the second commit.
- bitcoin/bitcoin#8085
  - Only the second commit.
- bitcoin/bitcoin#10564
- bitcoin/bitcoin#13061
  - Wasn't needed when I first made this backport in 2017, but it is now!

Part of #2074.
zkbot added a commit to zcash/zcash that referenced this pull request Feb 17, 2021
@zkbot
Auto merge of #4985 - zcash:backport/8173-siphash_node_eviction, r=str4d
89d9e55 
Use SipHash for node eviction

Backport of bitcoin/bitcoin#8173

Commits are listed in stack order.

- pick bitcoin/bitcoin@eebc232187
- pick bitcoin/bitcoin@888483098e
- pick bitcoin/bitcoin@c31b24f745
- pick bitcoin/bitcoin@9bf156bb9e
- pick bitcoin/bitcoin@053930ffc4
   - missing bitcoin/bitcoin#5697 (not a candidate for direct inclusion), using pieces directly
   - conflicts with #1258 which it removes part of (this is ok)
      - ignore bitcoin/bitcoin#6374
      - requires bitcoin/bitcoin#7906, or at least bitcoin/bitcoin@cca221fd21
         - pick bitcoin/bitcoin@cca221fd21
   - conflicts with bitcoin/bitcoin#7181, not needed until later if at all
zkbot added a commit to zcash/zcash that referenced this pull request Feb 17, 2021
@zkbot
Auto merge of #2812 - str4d:2074-store-banlist, r=nuttycom
b4b07a1 
banlist.dat: store banlist on disk

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6310
- bitcoin/bitcoin#6315
  - Only the new signal and net changes, no QT.
- bitcoin/bitcoin#7350
- bitcoin/bitcoin#7458
- bitcoin/bitcoin#7696
- bitcoin/bitcoin#7959
- bitcoin/bitcoin#7906
  - Only the ban-related commits.
- bitcoin/bitcoin#8392
  - Only the second commit.
- bitcoin/bitcoin#8085
  - Only the second commit.
- bitcoin/bitcoin#10564
- bitcoin/bitcoin#13061
  - Wasn't needed when I first made this backport in 2017, but it is now!

Part of #2074.
This was referenced Feb 23, 2021
Draft
Draft
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
P2P
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@theuni @sipa @btcdrak @maflcko @laanwj @paveljanik @jonasschnelli