net: prerequisites for p2p encapsulation changes #7906

pull theuni wants to merge 7 commits into bitcoin:master from theuni:net-refactor-prerequisites changing 4 files +117 −146

theuni commented at 10:46 PM on April 18, 2016: member

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.
jonasschnelli added the label P2P on Apr 19, 2016
sipa commented at 10:05 AM on April 20, 2016: member

Is SetBannedIsDirty still needed after this?
sipa commented at 10:06 AM on April 20, 2016: member

utACK b33a17803a067d98c48f22b95eb4a5297eb03944
btcdrak commented at 11:10 AM on April 20, 2016: contributor

Concept ACK

in src/net.cpp:None in e9e5946a12 outdated

 678 | @@ -644,7 +679,7 @@ void CNode::copyStats(CNodeStats &stats)
 679 |      stats.dPingMin  = (((double)nMinPingUsecTime) / 1e6);
 680 |      stats.dPingWait = (((double)nPingUsecWait) / 1e6);
 681 |  
 682 | -    // Leave string empty if addrLocal invalid (not filled in yet)
 683 | +    // Leave std::string empty if addrLocal invalid (not filled in yet)

MarcoFalke commented at 12:09 PM on April 20, 2016:

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

theuni commented at 5:18 PM on April 20, 2016:

Heh, overzealous sed. Thanks, will fix.

MarcoFalke commented at 12:09 PM on April 20, 2016: member

Concept ACK
theuni commented at 5:29 PM on April 20, 2016: member

@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 commented at 5:32 PM on April 20, 2016: member

Added a quick fixup for the string comment rather than rebasing and invalidating ACKs. I can squash before merge.
sipa commented at 5:05 AM on April 21, 2016: member

utACK 8d18913269e512b122e835b44ce1c3296a204177
laanwj commented at 12:34 PM on April 21, 2016: member

Needs rebase after #7868
theuni force-pushed on Apr 21, 2016
theuni commented at 5:43 PM on April 21, 2016: member

Rebased
in src/qt/rpcconsole.cpp:None in 8fe811b641 outdated
```
 891 | @@ -892,8 +892,6 @@ void RPCConsole::banSelectedNode(int bantime)
 892 |          SplitHostPort(nStr, port, addr);
 893 |  
 894 |          CNode::Ban(CNetAddr(addr), BanReasonManuallyAdded, bantime);
 895 | -        bannedNode->fDisconnect = true;
```
paveljanik commented at 9:22 PM on April 22, 2016:

The variable bannedNode seems to be unused here now.

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

theuni commented at 4:23 PM on May 10, 2016:

@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 commented at 9:30 PM on April 22, 2016: contributor

Concept ACK
laanwj commented at 1:09 PM on April 28, 2016: member

utACK 8fe811b (please get rid of the dummy commit)
theuni force-pushed on Apr 29, 2016
laanwj commented at 10:05 AM on May 4, 2016: member

Needs rebase

net: don't import std namespace

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.

52cbce287a

net: remove unused set 9faa4902cd
net: use the exposed GetNodeSignals() rather than g_signals directly 563f375cde

net: Drop CNodeRef for AttemptToEvictConnection

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

cca221fd21

theuni force-pushed on May 5, 2016
sipa commented at 10:50 PM on May 7, 2016: member

@theuni Can you address https://github.com/bitcoin/bitcoin/pull/7906/commits/d48445a32faa50ff028bc58add54e67b33528c25#r60804931 ?
theuni commented at 8:40 AM on May 8, 2016: member

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

net: make Ban/Unban/ClearBan functionality consistent

- 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

8b8f87714d

net: No need to export DumpBanlist e9ed6206b3
net: No need to export ConnectNode 5d5e7a097a
theuni force-pushed on May 10, 2016
sipa commented at 6:04 PM on May 10, 2016: member

reutACK 5d5e7a097a87e0fe6efb5e2d622daadc10c2ad79
laanwj merged this on May 18, 2016
laanwj closed this on May 18, 2016
laanwj referenced this in commit 83121cca75 on May 18, 2016
random-zebra referenced this in commit 8bbc0650e6 on Jul 1, 2020
zkbot referenced this in commit f40121446d on Nov 12, 2020
zkbot referenced this in commit 049951dc45 on Feb 11, 2021
zkbot referenced this in commit b3a6729944 on Feb 16, 2021
zkbot referenced this in commit e85265fbd5 on Feb 17, 2021
zkbot referenced this in commit 89d9e557e1 on Feb 17, 2021
zkbot referenced this in commit b4b07a1bbd on Feb 17, 2021
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#7868 net: Split DNS resolving functionality out of net structures