“feeler” and “test-before-evict” are two different strategies suggest in Eclipse Attacks on Bitcoin’s Peer-to-Peer Network. In our codebase, we use ConnType::FEELER to implement both.
It is confusing, up to the point that our documentation was just incorrect.
This PR:
Not quite sure what the travis error is. It produces a huge log while executing the diff script though, maybe it’s sending output to stdout accidentally or such?
The sed commands in the scripted diff are missing the -i (in place) option.
I’m definitely in favor of fixing the documentation- thanks :)
I’m not entirely convinced about the rename. I understand how its confusing with the context of terminology presented in the paper, but from a codebase perspective it doesn’t feel super different to say “probes have two functions” vs “feelers have two functions”. If the goal of the rename to disambiguate, why not separate the two? ( but personally I’d find the added docs sufficient )
it doesn’t feel super different to say “probes have two functions” vs “feelers have two functions”.
“feelers have two functions” is incorrect because the paper which is often referred to (and introduced them) says that feelers have 1 function.
Also, just by meaning, imo “feeler” well describes the first feature and not the second.
why not separate the two?
You suggesting having 2 ConnTypes? I don’t see a problem with having PROBE allowing both features, let’s just not call them feelers, as per above. If we have 2 ConnTypes, it will be if (feeler || test_before_evict) do {} everywhere except one place :)
If the goal of the rename to disambiguate, why not separate the two?
Agree here, I think as long as the same concept is used, and documented, consistently, it’s good. You could, for example, add a note in a comment that it’s referred to differently in the paper. Splitting the ConnTypes into more specific cases would also be acceptable, if it clarifies things.
A wholesale search/replace over the source code is not the kind of change we usually merge. For example it causes conflicts with other PRs that make more significant changes.
1918- conn_type = ConnectionType::FEELER;
1919- fFeeler = true;
1920+ } else if (nTime > next_probe_time) {
1921+ next_probe_time = PoissonNextSend(nTime, PROBE_INTERVAL);
1922+ conn_type = ConnectionType::PROBE;
1923+ make_probe = true;
IsProbeConn() here. We can’t right now because type asserting methods are part of CNode not ConnectionType but maybe we could move them there and make CConnman a friend class ? Just saying if you have room somewhere on one of your PRs :)
I think this is confusing for now to have a connection type bearing both the name of a first function and also executing a second different function. It would be better to move towards the paper as these connection strategies design goals are clearly explained there. I’m fine with the current renaming as a way to dissociate ambiguity.
Note, introducing yet another connection type (ConnectionType::TEST_COLLISION) would work too but I fear the proliferation of connection types in the near-term (like anchors, negotiated-block-relay-only, …) might hinder the code clarification improvement from the connection refactor.
142@@ -143,12 +143,18 @@ enum class ConnectionType {
143 MANUAL,
144
145 /**
146- * Feeler connections are short lived connections used to increase the
147- * number of connectable addresses in our AddrMan. Approximately every
148- * FEELER_INTERVAL, we attempt to connect to a random address from the new
149- * table. If successful, we add it to the tried table.
150+ * Probe connections are short lived connections made to check that a node
In 7a8dbdf412aa7a9676f2efeab246c143 “Improve docs about probe connections”, a few nit fixups
s/short lived/short-lived/
s/It can be useful to/They can be useful for/
s/feelers/feeler connections/
ACK 7a8dbdf412aa7a9676f2efeab246c143a74bae6f
I think this is an improvement on its own, and separating them into two connection types might be a further improvement for observing and testing them.
I don’t have a strong opinion on renaming feeler to probe. I just to provide some background on why I decided to reuse feeler connections for test-before-evict.
Avoid additional locks or complexity by adding a thread or events for test-before-evict. I believe my original design required adding a lock and I was paranoid about causing deadlocks.
I wanted a single place to rate limit “helper” connections and their buffers. A feeler connection can attempt to push something from new to tried which is might trigger test-before-evict probe connection. Having them share the same fixed size buffer limits the number of connections they both can make.
Reduce the diff by not having separate mechanisms and variables for each case since they are basically doing the same thing. In some sense we just want a generic probe service that other features can use to probe addrs and to see if they are online.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Since there was a significant hesitation to renaming enums, and even more hesitation to splitting the enums into two, I’d just take the least intrusive approach: just improve the documentation.
We can make those other clarifications later, although keep in mind it’s better be done before exposing conn-types in RPC. So if you strongly want to see those changes, please let us know (maybe once again?).
1783@@ -1784,7 +1784,7 @@ void CConnman::SetTryNewOutboundPeer(bool flag)
1784
1785 // Return the number of peers we have over our outbound connection limit
1786 // Exclude peers that are marked for disconnect, or are going to be
1787-// disconnected soon (eg one-shots and feelers)
1788+// disconnected soon (eg ADDR_FETCH and PROBE)
