[P2P] Begin Network Encapsulation #1812
Conversation
random-zebra
left a comment
random-zebra
left a comment
There was a problem hiding this comment.
Halfway through it. Looking pretty good so far. Few minor nits (feel free to ignore) and a couple questions (mostly around the masternode-sync area).
| if (connman) { | ||
| connman->ForEachNode([nBlockEstimate, hashNewTip](CNode* pnode) { | ||
| if (chainActive.Height() > (pnode->nStartingHeight != -1 ? pnode->nStartingHeight - 2000 : nBlockEstimate)) { | ||
| connman->ForEachNode([pindexNewTip, nBlockEstimate, hashNewTip](CNode* pnode) { |
There was a problem hiding this comment.
nit:
would be good to standardize this and not input two different arguments from the same object. pindexNewTip arg, used to get the height, and hashNewTip are related to the same block.
We should pass only the primitive values or pass the block index pointer, not both. (I'm more inclined to pass both primitive values. We don't know how this will evolve in the future).
In other words, instead of passing pindexNewTip and removing nNewHeight from this function, input nNewHeight.
random-zebra
left a comment
random-zebra
left a comment
There was a problem hiding this comment.
Missed few ReleaseNodeVector in sneaky inlined if () return.
|
would be nice to add a comment on top of the Something like: A further work would be to refactor |
This makes sure that cs_filter is never held while taking cs_main or CNode::cs_vSend.
This will eventually solve a circular dependency
This behavior seems to have been quite racy and broken. Move nLocalHostNonce into CNode, and check received nonces against all non-fully-connected nodes. If there's a match, assume we've connected to ourself.
…rather than a std::function to eliminate std::function overhead
- Drop the interruption point directly after the pnode allocation. This would
be leaky if hit.
- Rearrange thread creation so that the socket handler comes first
Also now that net threads are interruptible, switch them to use std threads/binds/mutexes/condvars.
After bitcoin#8594 the addrFrom sent by a node is not used anymore at all, so don't bother sending it. Also mitigates the privacy issue in (bitcoin#8616). It doesn't completely solve the issue as GetLocalAddress is also called in AdvertiseLocal, but at least when advertising addresses it stands out less as *our* address.
Claims a peer makes about itself are inherently more credible.
Fuzzbawls
force-pushed
the
2020_network-encapsulation
branch
from
1ac7d33 to
f38c9f9
Compare
Using ForEachNode ensures that we are still holding cs_vNodes when dealing with a CNode* (as we should, for anything aside from existance checking).
Using ForEachNodeContinueIf ensures that we are still holding cs_vNodes when dealing with a CNode* (as we should, for anything aside from existance checking).
random-zebra
force-pushed
the
2020_network-encapsulation
branch
from
973b9d7 to
e733939
Compare
|
Just pushed a few commits to properly lock |
random-zebra
left a comment
random-zebra
left a comment
There was a problem hiding this comment.
Code ACK. Really great backport. Will give it some live testing.
|
same here, code ACK e733939. Nice work ☕ . |
9 participants
This is the finalization of the upstream PRs being tracked in #1374 to update much of our networking code to more closely follow upstream improvements. The following PRs are included here:
Do not shadow variables in networking code
Fix some locks
Begin encapsulation
drop boost::thread_group
No longer send local address in addrMe
Do not set an addr time penalty when a peer advertises itself
Additionally, during conflict resolution and backporting of 8085, the following additional upstream PR was included:
only delete CConnman if it's been created
Still TODO in future PRs: