Conversation
p2p/pex/reactor.go
Outdated
| const ( | ||
| maxAddresses uint16 = 100 | ||
| resolveTimeout = 3 * time.Second | ||
| // the minimum time one peer can send another request to the same peer | ||
| minReceiveRequestInterval = 300 * time.Millisecond | ||
|
|
||
| // the maximum amount of addresses that can be included in a response | ||
| maxAddresses uint16 = 100 | ||
|
|
||
| // allocated time to resolve a node address into a set of endpoints | ||
| resolveTimeout = 3 * time.Second | ||
|
|
||
| // How long to wait when there are no peers available before trying again | ||
| noAvailablePeersWaitPeriod = 1 * time.Second | ||
|
|
||
| // indicates the ping rate of the pex reactor when the peer store is full. | ||
| // The reactor should still look to add new peers in order to flush out low | ||
| // scoring peers that are still in the peer store | ||
| fullCapacityInterval = 10 * time.Minute |
There was a problem hiding this comment.
Do we want to make any of these variables configurable?
I made a few little cosmetic changes, and swapped the setup order so that the test starts up (no deadlock) but it still fails.
|
So at the moment we have a disparity between a PEX address: type PexAddress struct {
ID string
IP string
Port uint32
}and an Endpoint: type Endpoint struct {
Protocol Protocol
IP net.IP
Port uint16
Path string
}The peer manager stores NodeAddress 's which can be resolved into an array of endpoints. The PEX reactor will take this array of endpoints and create the PEXAddress that it will send to other peers like so: pexAddresses = append(pexAddresses, protop2p.PexAddress{
ID: string(address.NodeID),
IP: endpoint.IP.String(),
Port: uint32(endpoint.Port),
})As you can see it truncates the Protocol . When the PEX reactor receives a PEXAddress , it turns it into a scheme-less URL string like so: peerAddress, err := p2p.ParseNodeAddress(
fmt.Sprintf("%s@%s:%d", pexAddress.ID, pexAddress.IP, pexAddress.Port))As we don't have a Protocol we default to using mconn . All this is to say that the PEX reactor is incapable of supporting any other transport other than mconn. There are two options that I see. Either we change the messages the PEX reactor sends to be URLs (this is a breaking change and something I know we've wanted to avoid) or we do a sort of hack where we attach the protocol to the nodeID like so: pexAddresses = append(pexAddresses, protop2p.PexAddress{
ID: fmt.Sprintf("%s://%v", endpoint.Protocol, address.NodeID),
IP: endpoint.IP.String(),
Port: uint32(endpoint.Port),
})Erik already mentions this problem in the code here: What do people think? |
Let's avoid breaking the protocol for sure. But I'm also not totally thrilled with the idea of the hack because it doesn't really end up being an Can we instead introduce new message(s) into the PEX reactor, where the legacy/current messages imply |
|
@alexanderbez would something like this work? message PexAddressV2 {
string url = 1;
}
message PexRequestV2 {}
message PexResponseV2 {
repeated PexAddressV2 addresses = 1 [(gogoproto.nullable) = false];
}
message PexMessage {
oneof sum {
PexRequest pex_request = 1;
PexResponse pex_response = 2;
PexRequestV2 pex_request_v2 = 3;
PexResponseV2 pex_response_v2 = 4;
}
} |
|
@cmwaters looks good! |
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
| // running peer manager, but does not add it to the existing | ||
| // network. Callers are responsible for updating peering relationships. | ||
| func (n *Network) MakeNode(t *testing.T) *Node { | ||
| func (n *Network) MakeNode(t *testing.T, opts NodeOptions) *Node { |
There was a problem hiding this comment.
are there cases where the NodeOptions are different fro different members of the network, or is it just safe to set it (and the options, on the network itself?
There was a problem hiding this comment.
Hmmm probs not. I was thinking that you may want to test a network with differently configured nodes but the default was that they were the same. Are you suggesting that the Network saves a copy of the options and just uses that whenever MakeNode is called?
There was a problem hiding this comment.
I think this change is fine, but I think the more options and whatever you have to pass around the harder it is to get the right thing to happen in your tests, so I'm just sort of generically/reflexively coming down on the side of "make interfaces simpler/smaller".
| func (m *PeerManager) Add(address NodeAddress) error { | ||
| // exists, the address is added to it if it isn't already present. This will push | ||
| // low scoring peers out of the address book if it exceeds the maximum size. | ||
| func (m *PeerManager) Add(address NodeAddress) (bool, error) { |
There was a problem hiding this comment.
do we ever handle the false, nil case differently than the true, nil case?
There was a problem hiding this comment.
Yup, we count how many nodes are new vs how many are already in the peer store and use it as a heuristic for calculating how often we should ping nodes for more addresses.
The thinking is that if I'm seeing a lot of new addresses I should be pinging more often to full up the peer store and once I stop seeing new addresses I slow down the cadence with which I send requests
tychoish
left a comment
There was a problem hiding this comment.
comments/questions aside, I think once the tests pass this is in a mergable state.
| // to its upper bound. | ||
| // MaxInterval = 100 * 100 * baseTime ~= 16mins for 10 peers | ||
| // CONTRACT: Must use a write lock as nextRequestTime is updated | ||
| func (r *ReactorV2) calculateNextRequestTime() { |
There was a problem hiding this comment.
I haven't fully reviewed this in detail but I trust your judgement here 👍
There was a problem hiding this comment.
Thanks. I'm hoping I've tested/thought about the heuristics here sufficiently for this to be solid.
Codecov Report
@@ Coverage Diff @@
## master #6305 +/- ##
==========================================
+ Coverage 60.70% 61.12% +0.42%
==========================================
Files 283 283
Lines 26866 26992 +126
==========================================
+ Hits 16309 16500 +191
+ Misses 8856 8775 -81
- Partials 1701 1717 +16
|
Decisions / Changes Made
SeedMode:
Discovery Algorithm:
DOS Protection:
requestsSentmap to stop peers from spuriously sending responses when no request was sentlastReceivedRequestsmap to prevent peers from sending requests too often.Testing:
Closes: #6271
Outstanding Work