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

net: prerequisites for p2p encapsulation changes #7906

Merged
merged 7 commits into from May 18, 2016

Conversation

7 participants
@theuni
Member

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.

@jonasschnelli jonasschnelli added the P2P label Apr 19, 2016

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 20, 2016

Member

Is SetBannedIsDirty still needed after this?

Member

sipa commented Apr 20, 2016

Is SetBannedIsDirty still needed after this?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 20, 2016

Member

utACK b33a17803a067d98c48f22b95eb4a5297eb03944

Member

sipa commented Apr 20, 2016

utACK b33a17803a067d98c48f22b95eb4a5297eb03944

@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Apr 20, 2016

Member

Concept ACK

Member

btcdrak commented Apr 20, 2016

Concept ACK

@MarcoFalke

View changes

Show outdated Hide outdated src/net.cpp
@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Apr 20, 2016

Member

Concept ACK

Member

MarcoFalke commented Apr 20, 2016

Concept ACK

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Apr 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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Apr 20, 2016

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 21, 2016

Member

utACK 8d18913269e512b122e835b44ce1c3296a204177

Member

sipa commented Apr 21, 2016

utACK 8d18913269e512b122e835b44ce1c3296a204177

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 21, 2016

Member

Needs rebase after #7868

Member

laanwj commented Apr 21, 2016

Needs rebase after #7868

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Apr 21, 2016

Member

Rebased

Member

theuni commented Apr 21, 2016

Rebased

@@ -892,8 +892,6 @@ void RPCConsole::banSelectedNode(int bantime)
SplitHostPort(nStr, port, addr);
CNode::Ban(CNetAddr(addr), BanReasonManuallyAdded, bantime);
bannedNode->fDisconnect = true;

This comment has been minimized.

@paveljanik

paveljanik Apr 22, 2016

Contributor

The variable bannedNode seems to be unused here now.

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

@paveljanik

paveljanik Apr 22, 2016

Contributor

The variable bannedNode seems to be unused here now.

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

This comment has been minimized.

@theuni

theuni May 10, 2016

Member

@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.

@theuni

theuni May 10, 2016

Member

@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

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Apr 22, 2016

Contributor

Concept ACK

Contributor

paveljanik commented Apr 22, 2016

Concept ACK

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 28, 2016

Member

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

Member

laanwj commented Apr 28, 2016

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 4, 2016

Member

Needs rebase

Member

laanwj commented May 4, 2016

Needs rebase

theuni added some commits Apr 15, 2016

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.
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
@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 7, 2016

Member

@theuni Can you address d48445a#r60804931 ?

Member

sipa commented May 7, 2016

@theuni Can you address d48445a#r60804931 ?

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni May 8, 2016

Member

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

Member

theuni commented May 8, 2016

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

theuni added some commits Apr 18, 2016

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
@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 10, 2016

Member

reutACK 5d5e7a0

Member

sipa commented May 10, 2016

reutACK 5d5e7a0

@laanwj laanwj merged commit 5d5e7a0 into bitcoin:master May 18, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request May 18, 2016

Merge #7906: net: prerequisites for p2p encapsulation changes
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment