doc: Better document features of feelers #19958

naumenkogs · 2020-09-15T08:04:57Z

"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:

~~clarifies this aspect by renaming "ConnType::FEELER" to "ConnType::PROBE", meaning that this connections only probes that the node is operational, and then disconnects.~~
fixes the documentation

naumenkogs · 2020-09-15T08:05:40Z

@amitiuttarwar @sdaftuar @EthanHeilman

laanwj · 2020-09-15T11:22:55Z

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?

sipa · 2020-09-15T17:20:00Z

It seems this unconditionally renames all uses of "feeler" to "probe". How does that clarify anything?

ajtowns · 2020-09-16T00:57:39Z

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.

naumenkogs · 2020-09-16T06:48:26Z

@sipa

All uses of "feeler" in our codebase referred to not just feeleres, but also "test before evict conns", which have different goals per the paper. I suggest that connections encompassing both features should not be called as one of those features, but rather should have its own name ("probe"). Otherwise, it's very easy to forget that "feeler conns" also do test-before-evict. And we actually had that mistake in the codebase (see the second commit)
The second commit was dropped while I was figuring out travis issues, it's back now. See extra documentation, where the distinction between two features is clarified.

practicalswift · 2020-09-16T09:31:33Z

Concept ACK: disambiguation is good

amitiuttarwar · 2020-09-16T20:14:02Z

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 )

naumenkogs · 2020-09-17T06:57:53Z

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 :)

laanwj · 2020-09-17T09:13:40Z

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.

ariard

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.

ariard · 2020-09-17T22:47:51Z

src/net.cpp

+        } else if (nTime > next_probe_time) {
+            next_probe_time = PoissonNextSend(nTime, PROBE_INTERVAL);
+            conn_type = ConnectionType::PROBE;
+            make_probe = true;


I've already made the point elsewhere that it would be better to use 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 :)

jonatack

ACK 7a8dbdf

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.

src/net.h

EthanHeilman · 2020-09-18T19:19:39Z

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.

DrahtBot · 2020-09-19T13:31:10Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

Better intervals between feelers #19869 (Better intervals between feelers by naumenkogs)

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.

naumenkogs · 2020-09-23T07:20:34Z

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?).

src/net.cpp

practicalswift · 2020-09-24T06:01:46Z

ACK 2ea62ca

amitiuttarwar · 2020-09-24T16:39:41Z

ACK 2ea62ca. thank you!

EthanHeilman

This change does a better job explaining feeler connections

fanquake · 2020-09-29T12:40:29Z

@sdaftuar do you have an opinion here?

sdaftuar · 2020-09-29T15:54:37Z

utACK, looks good to me.

2ea62ca Improve docs about feeler connections (Gleb Naumenko) Pull request description: "feeler" and "test-before-evict" are two different strategies suggest in [Eclipse Attacks on Bitcoin’s Peer-to-Peer Network](https://www.usenix.org/system/files/conference/usenixsecurity15/sec15-paper-heilman.pdf). 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: - ~clarifies this aspect by renaming "ConnType::FEELER" to "ConnType::PROBE", meaning that this connections only probes that the node is operational, and then disconnects.~ - fixes the documentation ACKs for top commit: amitiuttarwar: ACK 2ea62ca. thank you! practicalswift: ACK 2ea62ca Tree-SHA512: c9c03c09eefeacec28ea199cc3f697b0a98723f2f849f7a8115edc43791f8165e296e0e25a82f0b5a4a781a7de38c8954b48bf74c714eba02cdc21f7460673e5

Summary: This is a backport of [[bitcoin/bitcoin#19958 | core#19958]] Test Plan: proof-reading Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10402

naumenkogs mentioned this pull request Sep 15, 2020

Bugfix: don't make collision from "tried" a feeler #19906

Closed

fanquake added the P2P label Sep 15, 2020

naumenkogs mentioned this pull request Sep 15, 2020

Better intervals between feelers #19869

Closed

naumenkogs force-pushed the 2020-09-rename-feeler-to-probe branch 2 times, most recently from 50a89f4 to 1cc4b92 Compare September 15, 2020 09:07

naumenkogs force-pushed the 2020-09-rename-feeler-to-probe branch 2 times, most recently from 5589252 to e895da0 Compare September 15, 2020 14:49

naumenkogs force-pushed the 2020-09-rename-feeler-to-probe branch from e895da0 to 7a8dbdf Compare September 16, 2020 06:35

ariard reviewed Sep 17, 2020

View reviewed changes

jonatack reviewed Sep 18, 2020

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

jonatack mentioned this pull request Sep 20, 2020

net, rpc: expose connection type in getpeerinfo #19883

Closed

naumenkogs force-pushed the 2020-09-rename-feeler-to-probe branch from 7a8dbdf to aa64b59 Compare September 23, 2020 07:10

naumenkogs force-pushed the 2020-09-rename-feeler-to-probe branch from aa64b59 to ed737a9 Compare September 23, 2020 07:22

naumenkogs changed the title ~~Rename feelers to probes~~ p2p: Better document feelers' features Sep 23, 2020

naumenkogs changed the title ~~p2p: Better document feelers' features~~ p2p: Better document features of feelers Sep 23, 2020

amitiuttarwar reviewed Sep 24, 2020

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

Improve docs about feeler connections

2ea62ca

naumenkogs force-pushed the 2020-09-rename-feeler-to-probe branch from ed737a9 to 2ea62ca Compare September 24, 2020 05:52

naumenkogs closed this Sep 24, 2020

naumenkogs reopened this Sep 24, 2020

naumenkogs changed the title ~~p2p: Better document features of feelers~~ doc, p2p: Better document features of feelers Sep 24, 2020

naumenkogs changed the title ~~doc, p2p: Better document features of feelers~~ doc: Better document features of feelers Sep 24, 2020

EthanHeilman approved these changes Sep 25, 2020

View reviewed changes

fanquake merged commit c7ad944 into bitcoin:master Sep 30, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

doc: Better document features of feelers #19958

doc: Better document features of feelers #19958

Uh oh!

Conversation

naumenkogs commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

naumenkogs commented Sep 15, 2020

Uh oh!

laanwj commented Sep 15, 2020

Uh oh!

sipa commented Sep 15, 2020

Uh oh!

ajtowns commented Sep 16, 2020

Uh oh!

naumenkogs commented Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Sep 16, 2020

Uh oh!

amitiuttarwar commented Sep 16, 2020

Uh oh!

naumenkogs commented Sep 17, 2020

Uh oh!

laanwj commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ariard left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard Sep 17, 2020

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

EthanHeilman commented Sep 18, 2020

Uh oh!

DrahtBot commented Sep 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

naumenkogs commented Sep 23, 2020

Uh oh!

Uh oh!

practicalswift commented Sep 24, 2020

Uh oh!

amitiuttarwar commented Sep 24, 2020

Uh oh!

EthanHeilman left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Sep 29, 2020

Uh oh!

sdaftuar commented Sep 29, 2020

Uh oh!

Uh oh!

naumenkogs commented Sep 15, 2020 •

edited

Loading

naumenkogs commented Sep 16, 2020 •

edited

Loading

laanwj commented Sep 17, 2020 •

edited

Loading

ariard left a comment •

edited

Loading

DrahtBot commented Sep 19, 2020 •

edited

Loading