Skip to content
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: Add test-before-evict discipline to addrman #9037

Merged
merged 1 commit into from Mar 6, 2018
Merged

net: Add test-before-evict discipline to addrman #9037

merged 1 commit into from Mar 6, 2018

Conversation

@EthanHeilman
Copy link
Contributor

@EthanHeilman EthanHeilman commented Oct 28, 2016

This change implement countermeasures 3 (test-before-evict) suggested in our paper: "Eclipse Attacks on Bitcoin’s Peer-to-Peer Network".

Design:

A collision occurs when an address, addr1, is being moved to the tried table from the new table, but maps to a position in the tried table which already contains an address (addr2). The current behavior is that addr1 would evict addr2 from the tried table.

This change ensures that during a collision, addr1 is not inserted into tried but instead inserted into a buffer (setTriedCollisions). The to-be-evicted address, addr2, is then tested by a feeler connection. If addr2 is found to be online, we remove addr1 from the buffer and addr2 is not evicted, on the other hand if addr2 is found be offline it is replaced by addr1.

An additional small advantage of this change is that, as no more than ten addresses can be in the test buffer at once, and addresses are only cleared one at a time from the test buffer (at 2 minute intervals), thus an attacker is forced to wait at least two minutes to insert a new address into tried after filling up the test buffer. This rate limits an attacker attempting to launch an eclipse attack.

Risk mitigation:

  • To prevent this functionality from being used as a DoS vector, we limit the number of addresses which are to be tested to ten. If we have more than ten addresses to test, we drop new addresses being added to tried if they would evict an address. Since the feeler thread only creates one new connection every 2 minutes the additional network overhead is limited.
  • An address in tried gains immunity from tests for 4 hours after it has been tested or successfully connected to.

Tests:

This change includes additional addrman unittests which test this behavior.

I ran an instance of this change with a much smaller tried table (2 buckets of 64 addresses) so that collisions were much more likely and observed evictions.

2016-10-27 07:20:26 Swapping 208.12.64.252:8333 for 68.62.95.247:8333 in tried table
2016-10-27 07:20:26 Moving 208.12.64.252:8333 to tried

I documented tests we ran against similar earlier versions of this change in #6355.

Security Benefit

This is was originally posted in PR #8282 see this comment for full details.

To determine the security benefit of these larger numbers of IPs in the tried table I modeled the attack presented in Eclipse Attacks on Bitcoin’s Peer-to-Peer Network.

attackergraph40000-10-1000short-line

Default node: 595 attacker IPs for ~50% attack success.
Default node + test-before-evict: 620 attacker IPs for ~50% attack success.
Feeler node: 5540 attacker IPs for ~50% attack success.
Feeler node + test-before-evict: 8600 attacker IPs for ~50% attack success.

The node running feeler connections has 10 times as many online IP addresses in its tried table making an attack 10 times harder (i.e. requiring the an attacker require 10 times as many IP addresses in different /16s). Adding test-before-evict increases resistance of the node by an additional 3000 attacker IP addresses.

Below I graph the attack over even greater attacker resources (i.e. more attacker controled IP addresses). Note that test-before-evict maintains some security far longer even against an attacker with 50,000 IPs. If this node had a larger tried table test-before-evict could greatly boost a nodes resistance to eclipse attacks.

attacker graph long view

@EthanHeilman
Copy link
Contributor Author

@EthanHeilman EthanHeilman commented Oct 28, 2016

This is the test-before-evict feature originally proposed in #6355.

@EthanHeilman
Copy link
Contributor Author

@EthanHeilman EthanHeilman commented Oct 28, 2016

One issue which needs to be address in my code above is that I read from mapInfo via [ ]. This violates the very sensible coding style guide requirement that [ ] is never used for map reads. I could use an assert as is used in other code in addrman:

assert(mapInfo.count(nId) == 1);

Instead I'd prefer to create a private method for accessing mapInfo elements by nId similar to CAddrMan::Find. As this is a refactor task which touches code outside of test-before-evict, I want to break it out into a separate commit to be included in this pull request.

kazcw
kazcw approved these changes Oct 28, 2016
Copy link
Contributor

@kazcw kazcw left a comment

utACK the implementation as of 18e642c, though I haven't read up on the details of the attack yet

src/addrman.cpp Outdated
} else if (GetAdjustedTime() - infoOld.nLastTry < ADDRMAN_REPLACEMENT_HOURS*(60*60)) {
LogPrint("addrman", "Swapping %s for %s in tried table\n", infoNew.ToString(), infoOld.ToString());
// Replaces an existing address already in the tried table with the new address
Good(infoNew, false);
Copy link
Contributor

@kazcw kazcw Oct 28, 2016

Lock is held, could use Good_

src/addrman.cpp Outdated
eraseCollision = true;
}
} else { // Collision is not actually a collision anymore
Good(infoNew, false);
Copy link
Contributor

@kazcw kazcw Oct 28, 2016

Lock held as above

Copy link
Contributor Author

@EthanHeilman EthanHeilman Oct 30, 2016

Good call!

src/addrman.cpp Outdated
void CAddrMan::ResolveCollisions_()
{
for (std::set<int>::iterator it = setTriedCollisions.begin(); it != setTriedCollisions.end();) {
int nIdnew = (int)*it;
Copy link
Contributor

@kazcw kazcw Oct 28, 2016

Nit: is this cast necessary?

Copy link
Contributor Author

@EthanHeilman EthanHeilman Oct 30, 2016

Nope, removed it.

src/addrman.cpp Outdated

int i = 0;
// Selects a random element from setTriedCollisions.
for (std::set<int>::iterator it = setTriedCollisions.begin(); it != setTriedCollisions.end(); it++) {
Copy link
Contributor

@kazcw kazcw Oct 28, 2016

Nit: I think std::advance would be clearer than a loop for picking one element

@fanquake fanquake added the P2P label Oct 29, 2016
@EthanHeilman
Copy link
Contributor Author

@EthanHeilman EthanHeilman commented Nov 1, 2016

I added some extra checks to ensure that if setTriedCollisions points to a key which doesn't exist in mapInfo it just removes that bad key.

@EthanHeilman
Copy link
Contributor Author

@EthanHeilman EthanHeilman commented Nov 21, 2016

Is this good to merge? Any changes I should make?

@EthanHeilman
Copy link
Contributor Author

@EthanHeilman EthanHeilman commented Dec 9, 2016

Fixed a merge conflict.

@EthanHeilman
Copy link
Contributor Author

@EthanHeilman EthanHeilman commented Dec 18, 2016

@sipa Any suggestions or improvements I can make to this?

@sipa
Copy link
Member

@sipa sipa commented Apr 9, 2017

utACK

@EthanHeilman Sorry for the delay here. I think we should try to get this in for 0.15. Would you mind rebasing again?

@EthanHeilman
Copy link
Contributor Author

@EthanHeilman EthanHeilman commented Apr 10, 2017

@sipa Will rebase today or tomorrow.

@EthanHeilman
Copy link
Contributor Author

@EthanHeilman EthanHeilman commented Apr 13, 2017

@sipa rebased, also updated some of the log statement to follow the new format.

@EthanHeilman
Copy link
Contributor Author

@EthanHeilman EthanHeilman commented Jul 27, 2017

Updated to obey new Bitcoin coding style guide

@EthanHeilman
Copy link
Contributor Author

@EthanHeilman EthanHeilman commented Aug 10, 2017

ping @theuni

@theuni
Copy link
Member

@theuni theuni commented Aug 16, 2017

@EthanHeilman Thanks for adapting the style. I'll review thoroughly soon.

Copy link
Member

@kallewoof kallewoof left a comment

utACK 8604020bab3963599ebfd933b91096ae1f686d69

src/addrman.cpp Outdated
erase_collision = true;

// The position in the tried bucket is not empty
} else if (vvTried[tried_bucket][tried_bucket_pos] != -1) {
Copy link
Member

@kallewoof kallewoof Feb 20, 2018

Nit: The above if case looks misleadingly like a single-no-curling-brace-block (I misread it as such) until you realize the thing below the comment (// The position...) is actually ending the block.

Maybe move the comment inside and indented inside the else block?

Copy link
Contributor Author

@EthanHeilman EthanHeilman Feb 26, 2018

It looked weird below the else if and in a brief search I couldn't find any other examples of that style in code. Instead I moved it to right of the if else.

Copy link
Member

@kallewoof kallewoof Feb 26, 2018

There are plenty of examples of this, I think.

E.g.

bitcoin/src/validation.cpp

Lines 1399 to 1402 in bf3353d

} else if (!check()) {
if (flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS) {
// Check whether the failure was caused by a
// non-mandatory script verification check, such as

bitcoin/src/validation.cpp

Lines 2416 to 2418 in bf3353d

} else if (fMissingData) {
// If we're missing data, then add back to mapBlocksUnlinked,
// so that if the block arrives in the future we can try adding

src/addrman.cpp Outdated
erase_collision = true;

// Has attempted to connect and failed in last X hours
} else if (GetAdjustedTime() - info_old.nLastTry < ADDRMAN_REPLACEMENT_HOURS*(60*60)) {
Copy link
Member

@kallewoof kallewoof Feb 20, 2018

Same here, in case you address the above indentation/readability concern.

for (std::set<int>::iterator it = m_tried_collisions.begin(); it != m_tried_collisions.end();) {
int id_new = *it;

bool erase_collision = false;
Copy link
Member

@kallewoof kallewoof Feb 20, 2018

It seems like you can reduce the lines below by defaulting to true here, as several cases simply turn it on. Unless it is too complicated to swap.

Copy link
Contributor Author

@EthanHeilman EthanHeilman Feb 26, 2018

I appreciate this comment, but I'm concerned it would be more confusing to read if I shortened it.

src/addrman.cpp Outdated
std::advance(it, GetRandInt(m_tried_collisions.size()));
int id_new = *it;

// If newId not found in mapInfo remove it from m_tried_collisions
Copy link
Member

@kallewoof kallewoof Feb 20, 2018

Comment mentions old name (newId).

Copy link
Contributor Author

@EthanHeilman EthanHeilman Feb 26, 2018

Good catch, thanks!

src/addrman.cpp Outdated
int id_old = vvTried[tried_bucket][tried_bucket_pos];
CAddrInfo& info_old = mapInfo[id_old];

return info_old;
Copy link
Member

@kallewoof kallewoof Feb 20, 2018

Maybe just return mapInfo[id_old];?

Copy link
Contributor Author

@EthanHeilman EthanHeilman Feb 26, 2018

done

// Add twenty two addresses.
CNetAddr source = ResolveIP("252.2.2.2");
for (unsigned int i = 1; i < 23; i++) {
CService addr = ResolveService("250.1.1."+boost::to_string(i));
Copy link
Member

@kallewoof kallewoof Feb 20, 2018

(Tests so maybe no biggie but) use std::to_string() not boost::to_string()


// Ensure Good handles duplicates well.
for (unsigned int i = 1; i < 23; i++) {
CService addr = ResolveService("250.1.1."+boost::to_string(i));
Copy link
Member

@kallewoof kallewoof Feb 20, 2018

std::to_string

Copy link
Contributor Author

@EthanHeilman EthanHeilman Feb 20, 2018

Let me fix these and all push later tonight

Copy link
Contributor Author

@EthanHeilman EthanHeilman Feb 26, 2018

Fixed

Copy link
Member

@kallewoof kallewoof left a comment

utACK 1fdbcc850ac5eaa5d6d51556f19bf0ff522b9ef9

Sorry, some more comments after re-reading.

src/addrman.cpp Outdated

// Will moving this address into tried evict another entry?
if (test_before_evict && (vvTried[tried_bucket][tried_bucket_pos] != -1)) {
LogPrint(BCLog::ADDRMAN, "addrman", "Collision inserting element into tried table, moving %s to m_tried_collisions=%d \n", addr.ToString(), m_tried_collisions.size());
Copy link
Member

@kallewoof kallewoof Feb 26, 2018

Nit: remove space in =%d \n

src/addrman.cpp Outdated
LogPrint(BCLog::ADDRMAN, "addrman", "Collision inserting element into tried table, moving %s to m_tried_collisions=%d \n", addr.ToString(), m_tried_collisions.size());

if (m_tried_collisions.size() < ADDRMAN_SET_TRIED_COLLISION_SIZE)
m_tried_collisions.insert(nId);
Copy link
Member

@kallewoof kallewoof Feb 26, 2018

Always use {} brackets for multiline ifs (new convention as of a while ago).

src/addrman.cpp Outdated

if (!info_new.IsValid()) { // id_new may no longer map to a valid address
erase_collision = true;

Copy link
Member

@kallewoof kallewoof Feb 26, 2018

Nit: IMO nix newline (I understand that you want to space out for readability but this isn't usually done, and some argue that more compact gives a better overview).

Copy link
Contributor Author

@EthanHeilman EthanHeilman Mar 3, 2018

Keeping new lines between code and comments as it more clearly distinguishes which line the comment is associated with. Removing newlines in other circumstances

src/addrman.cpp Outdated
erase_collision = true;

} else if (vvTried[tried_bucket][tried_bucket_pos] != -1) { // The position in the tried bucket is not empty

Copy link
Member

@kallewoof kallewoof Feb 26, 2018

Nit: see above (your call, of course)

src/addrman.cpp Outdated
// Has successfully connected in last X hours
if (GetAdjustedTime() - info_old.nLastSuccess < ADDRMAN_REPLACEMENT_HOURS*(60*60)) {
erase_collision = true;

Copy link
Member

@kallewoof kallewoof Feb 26, 2018

-'-

Copy link
Contributor Author

@EthanHeilman EthanHeilman Mar 3, 2018

What does -'- mean?

Copy link
Member

@kallewoof kallewoof Mar 4, 2018

Sorry, it means "same as above". Like "Ditto mark".

src/addrman.h Outdated
}

//! Randomly select an address in tried that another address is attempting to evict.
CAddrInfo SelectTriedCollision(){
Copy link
Member

@kallewoof kallewoof Feb 26, 2018

Nit on (){ (add space or newline)

src/addrman.h Outdated
//! Randomly select an address in tried that another address is attempting to evict.
CAddrInfo SelectTriedCollision(){
CAddrInfo ret;
{
Copy link
Member

@kallewoof kallewoof Feb 26, 2018

Same Q on {}. The lock will go out of scope before ret, but I don't think it will matter here as the lock will go out of scope on return regardless.

Copy link
Contributor Author

@EthanHeilman EthanHeilman Mar 3, 2018

I'm using the pattern from select. Since they essentially do the same thing they should look the same.
https://github.com/bitcoin/bitcoin/blob/master/src/addrman.h#L562

src/net.cpp Outdated
@@ -1824,11 +1824,18 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
}
}

addrman.ResolveCollisions();


Copy link
Member

@kallewoof kallewoof Feb 26, 2018

Remove extra newline.

Copy link
Contributor Author

@EthanHeilman EthanHeilman Mar 3, 2018

done

src/net.cpp Outdated

// SelectTriedCollision returns an invalid address if it is empty.
if (!fFeeler || !addr.IsValid())
addr = addrman.Select(fFeeler);
Copy link
Member

@kallewoof kallewoof Feb 26, 2018

Wrap multiline if in {}.

@@ -52,6 +52,13 @@ class CAddrManTest : public CAddrMan
{
CAddrMan::Delete(nId);
}

void SimConnFail(CService& addr){
Copy link
Member

@kallewoof kallewoof Feb 26, 2018

Space or newline before {

Copy link
Member

@kallewoof kallewoof left a comment

A few more nits! Also: amend the commit message to add an empty newline after Add test-before-evict discipline to addrman, as it will screw with the auto-commit-log script for releases otherwise.

src/addrman.cpp Outdated
if (m_tried_collisions.size() < ADDRMAN_SET_TRIED_COLLISION_SIZE) {
m_tried_collisions.insert(nId);
}
} else {
LogPrint(BCLog::ADDRMAN, "Moving %s to tried\n", addr.ToString());
Copy link
Member

@kallewoof kallewoof Mar 4, 2018

Indent LogPrint line? If you get a lot of indentation-only changes you can suggest people add ?w=1 to the diff page to ignore whitespace only changes.

@@ -52,6 +52,14 @@ class CAddrManTest : public CAddrMan
{
CAddrMan::Delete(nId);
}

void SimConnFail(CService& addr)
Copy link
Member

@kallewoof kallewoof Mar 4, 2018

It's not clear to me what this function's name is an abbreviation of. "Simulate connection failure?" Is it for testing/debugging?

Copy link
Contributor Author

@EthanHeilman EthanHeilman Mar 5, 2018

Added comment

Copy link
Contributor Author

@EthanHeilman EthanHeilman Mar 5, 2018

But yes, it is for testing. The test before evict logic needs connections to fail to exercise the eviction code. This allows when we test a particular addresses to force it to fail.

@fanquake fanquake added this to Blockers in High-priority for review Mar 5, 2018
Copy link
Member

@kallewoof kallewoof left a comment

utACK 8989b6f

@sdaftuar
Copy link
Member

@sdaftuar sdaftuar commented Mar 5, 2018

Code review ACK, though I haven't reviewed the tests. (Also I am new to this code, but I tried to come up with all the ways I thought this could be broken, and failed to come up with anything.)

EDIT: travis error is some sort of whitespace issue:

This diff appears to have added new lines with trailing whitespace.
The following changes were suspected:
diff --git a/src/addrman.cpp b/src/addrman.cpp
@@ -523,0 +536,74 @@ int CAddrMan::RandomInt(int nMax){
+
^---- failure generated from contrib/devtools/lint-whitespace.sh
The command "if [ "$CHECK_DOC" = 1 -a "$TRAVIS_EVENT_TYPE" = "pull_request" ]; then contrib/devtools/lint-all.sh; fi" failed and exited with 1 during .
Your build has been stopped.

@EthanHeilman
Copy link
Contributor Author

@EthanHeilman EthanHeilman commented Mar 5, 2018

@sdaftuar I've been trying to find the whitespace error for a while now. I can't seem to find it. The line number identified seems to just be a carriage return. Any help?

src/addrman.cpp Outdated
erase_collision = true;
} else if (GetAdjustedTime() - info_old.nLastTry < ADDRMAN_REPLACEMENT_HOURS*(60*60)) { // attempted to connect and failed in last X hours
LogPrint(BCLog::ADDRMAN, "addrman", "Swapping %s for %s in tried table\n", info_new.ToString(), info_old.ToString());

Copy link
Member

@sipa sipa Mar 5, 2018

The trailing spaces are on this line.

@sipa
Copy link
Member

@sipa sipa commented Mar 6, 2018

@EthanHeilman It seems the unit tests fail now.

int64_t time = 1;
Good_(addr, true, time); // Set last good connection in the deep past.
bool count_failure = false;
Attempt(addr, count_failure);
Copy link
Member

@sdaftuar sdaftuar Mar 6, 2018

Changing this to: Attempt(addr, count_failure, GetAdjustedTime() - 90); fixes the unit test failure.

nit: the indentation is off on the line right below

Changes addrman to use the test-before-evict discipline in which an
address is to be evicted from the tried table is first tested and if
it is still online it is not evicted.

Adds tests to provide test coverage for this change.

This change was suggested as Countermeasure 3 in
Eclipse Attacks on Bitcoin’s Peer-to-Peer Network, Ethan Heilman,
Alison Kendler, Aviv Zohar, Sharon Goldberg. ePrint Archive Report
2015/263. March 2015.
@sdaftuar
Copy link
Member

@sdaftuar sdaftuar commented Mar 6, 2018

I've reviewed the tests and the last change; utACK e68172e

@sipa
Copy link
Member

@sipa sipa commented Mar 6, 2018

utACK e68172e

@kallewoof
Copy link
Member

@kallewoof kallewoof commented Mar 6, 2018

utACK e68172e

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this issue Jul 31, 2018
These were introduced in bitcoin#9037.

Found by @theuni.
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jun 10, 2020
e68172e Add test-before-evict discipline to addrman (Ethan Heilman)

Pull request description:

  This change implement countermeasures 3 (test-before-evict) suggested in our paper: ["Eclipse Attacks on Bitcoin’s Peer-to-Peer Network"](http://cs-people.bu.edu/heilman/eclipse/).
  # Design:

  A collision occurs when an address, addr1, is being moved to the tried table from the new table, but maps to a position in the tried table which already contains an address (addr2). The current behavior is that addr1 would evict addr2 from the tried table.

  This change ensures that during a collision, addr1 is not inserted into tried but instead inserted into a buffer (setTriedCollisions). The to-be-evicted address, addr2, is then tested by [a feeler connection](bitcoin#8282). If addr2 is found to be online, we remove addr1 from the buffer and addr2 is not evicted, on the other hand if addr2 is found be offline it is replaced by addr1.

  An additional small advantage of this change is that, as no more than ten addresses can be in the test buffer at once, and addresses are only cleared one at a time from the test buffer (at 2 minute intervals), thus an attacker is forced to wait at least two minutes to insert a new address into tried after filling up the test buffer. This rate limits an attacker attempting to launch an eclipse attack.
  # Risk mitigation:
  - To prevent this functionality from being used as a DoS vector, we limit the number of addresses which are to be tested to ten. If we have more than ten addresses to test, we drop new addresses being added to tried if they would evict an address. Since the feeler thread only creates one new connection every 2 minutes the additional network overhead is limited.
  - An address in tried gains immunity from tests for 4 hours after it has been tested or successfully connected to.
  # Tests:

  This change includes additional addrman unittests which test this behavior.

  I ran an instance of this change with a much smaller tried table (2 buckets of 64 addresses) so that collisions were much more likely and observed evictions.

  ```
  2016-10-27 07:20:26 Swapping 208.12.64.252:8333 for 68.62.95.247:8333 in tried table
  2016-10-27 07:20:26 Moving 208.12.64.252:8333 to tried
  ```

  I documented tests we ran against similar earlier versions of this change in bitcoin#6355.
  # Security Benefit

  This is was originally posted in PR bitcoin#8282 see [this comment for full details](bitcoin#8282 (comment)).

  To determine the security benefit of these larger numbers of IPs in the tried table I modeled the attack presented in [Eclipse Attacks on Bitcoin’s Peer-to-Peer Network](https://eprint.iacr.org/2015/263).

  ![attackergraph40000-10-1000short-line](https://cloud.githubusercontent.com/assets/274814/17366828/372af458-595b-11e6-81e5-2c9f97282305.png)

  **Default node:** 595 attacker IPs for ~50% attack success.
  **Default node + test-before-evict:** 620 attacker IPs for ~50% attack success.
  **Feeler node:** 5540 attacker IPs for ~50% attack success.
  **Feeler node + test-before-evict:** 8600 attacker IPs for ~50% attack success.

  The node running feeler connections has 10 times as many online IP addresses in its tried table making an attack 10 times harder (i.e. requiring the an attacker require 10 times as many IP addresses in different /16s). Adding test-before-evict increases resistance of the node by an additional 3000 attacker IP addresses.

  Below I graph the attack over even greater attacker resources (i.e. more attacker controled IP addresses). Note that test-before-evict maintains some security far longer even against an attacker with 50,000 IPs. If this node had a larger tried table test-before-evict could greatly boost a nodes resistance to eclipse attacks.

  ![attacker graph long view](https://cloud.githubusercontent.com/assets/274814/17367108/96f46d64-595c-11e6-91cd-edba160598e7.png)

Tree-SHA512: fdad4d26aadeaad9bcdc71929b3eb4e1f855b3ee3541fbfbe25dca8d7d0a1667815402db0cb4319db6bd3fcd32d67b5bbc0e12045c4252d62d6239b7d77c4395
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jun 10, 2020
b4db76c net: Correct addrman logging (Wladimir J. van der Laan)

Pull request description:

  These were introduced in bitcoin#9037.

  Found by @theuni (bitcoin#9037 (review)).

Tree-SHA512: 9b5153da8a8e5d4ddf9513a5c453f9609cffd4df2924fd48c7b36c1b1055748c7077d4fc0e70be62ca36af87df7f621a744bb374a234baba271ce4982a240825
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jun 13, 2020
e68172e Add test-before-evict discipline to addrman (Ethan Heilman)

Pull request description:

  This change implement countermeasures 3 (test-before-evict) suggested in our paper: ["Eclipse Attacks on Bitcoin’s Peer-to-Peer Network"](http://cs-people.bu.edu/heilman/eclipse/).
  # Design:

  A collision occurs when an address, addr1, is being moved to the tried table from the new table, but maps to a position in the tried table which already contains an address (addr2). The current behavior is that addr1 would evict addr2 from the tried table.

  This change ensures that during a collision, addr1 is not inserted into tried but instead inserted into a buffer (setTriedCollisions). The to-be-evicted address, addr2, is then tested by [a feeler connection](bitcoin#8282). If addr2 is found to be online, we remove addr1 from the buffer and addr2 is not evicted, on the other hand if addr2 is found be offline it is replaced by addr1.

  An additional small advantage of this change is that, as no more than ten addresses can be in the test buffer at once, and addresses are only cleared one at a time from the test buffer (at 2 minute intervals), thus an attacker is forced to wait at least two minutes to insert a new address into tried after filling up the test buffer. This rate limits an attacker attempting to launch an eclipse attack.
  # Risk mitigation:
  - To prevent this functionality from being used as a DoS vector, we limit the number of addresses which are to be tested to ten. If we have more than ten addresses to test, we drop new addresses being added to tried if they would evict an address. Since the feeler thread only creates one new connection every 2 minutes the additional network overhead is limited.
  - An address in tried gains immunity from tests for 4 hours after it has been tested or successfully connected to.
  # Tests:

  This change includes additional addrman unittests which test this behavior.

  I ran an instance of this change with a much smaller tried table (2 buckets of 64 addresses) so that collisions were much more likely and observed evictions.

  ```
  2016-10-27 07:20:26 Swapping 208.12.64.252:8333 for 68.62.95.247:8333 in tried table
  2016-10-27 07:20:26 Moving 208.12.64.252:8333 to tried
  ```

  I documented tests we ran against similar earlier versions of this change in bitcoin#6355.
  # Security Benefit

  This is was originally posted in PR bitcoin#8282 see [this comment for full details](bitcoin#8282 (comment)).

  To determine the security benefit of these larger numbers of IPs in the tried table I modeled the attack presented in [Eclipse Attacks on Bitcoin’s Peer-to-Peer Network](https://eprint.iacr.org/2015/263).

  ![attackergraph40000-10-1000short-line](https://cloud.githubusercontent.com/assets/274814/17366828/372af458-595b-11e6-81e5-2c9f97282305.png)

  **Default node:** 595 attacker IPs for ~50% attack success.
  **Default node + test-before-evict:** 620 attacker IPs for ~50% attack success.
  **Feeler node:** 5540 attacker IPs for ~50% attack success.
  **Feeler node + test-before-evict:** 8600 attacker IPs for ~50% attack success.

  The node running feeler connections has 10 times as many online IP addresses in its tried table making an attack 10 times harder (i.e. requiring the an attacker require 10 times as many IP addresses in different /16s). Adding test-before-evict increases resistance of the node by an additional 3000 attacker IP addresses.

  Below I graph the attack over even greater attacker resources (i.e. more attacker controled IP addresses). Note that test-before-evict maintains some security far longer even against an attacker with 50,000 IPs. If this node had a larger tried table test-before-evict could greatly boost a nodes resistance to eclipse attacks.

  ![attacker graph long view](https://cloud.githubusercontent.com/assets/274814/17367108/96f46d64-595c-11e6-91cd-edba160598e7.png)

Tree-SHA512: fdad4d26aadeaad9bcdc71929b3eb4e1f855b3ee3541fbfbe25dca8d7d0a1667815402db0cb4319db6bd3fcd32d67b5bbc0e12045c4252d62d6239b7d77c4395
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jun 13, 2020
b4db76c net: Correct addrman logging (Wladimir J. van der Laan)

Pull request description:

  These were introduced in bitcoin#9037.

  Found by @theuni (bitcoin#9037 (review)).

Tree-SHA512: 9b5153da8a8e5d4ddf9513a5c453f9609cffd4df2924fd48c7b36c1b1055748c7077d4fc0e70be62ca36af87df7f621a744bb374a234baba271ce4982a240825
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jun 13, 2020
e68172e Add test-before-evict discipline to addrman (Ethan Heilman)

Pull request description:

  This change implement countermeasures 3 (test-before-evict) suggested in our paper: ["Eclipse Attacks on Bitcoin’s Peer-to-Peer Network"](http://cs-people.bu.edu/heilman/eclipse/).
  # Design:

  A collision occurs when an address, addr1, is being moved to the tried table from the new table, but maps to a position in the tried table which already contains an address (addr2). The current behavior is that addr1 would evict addr2 from the tried table.

  This change ensures that during a collision, addr1 is not inserted into tried but instead inserted into a buffer (setTriedCollisions). The to-be-evicted address, addr2, is then tested by [a feeler connection](bitcoin#8282). If addr2 is found to be online, we remove addr1 from the buffer and addr2 is not evicted, on the other hand if addr2 is found be offline it is replaced by addr1.

  An additional small advantage of this change is that, as no more than ten addresses can be in the test buffer at once, and addresses are only cleared one at a time from the test buffer (at 2 minute intervals), thus an attacker is forced to wait at least two minutes to insert a new address into tried after filling up the test buffer. This rate limits an attacker attempting to launch an eclipse attack.
  # Risk mitigation:
  - To prevent this functionality from being used as a DoS vector, we limit the number of addresses which are to be tested to ten. If we have more than ten addresses to test, we drop new addresses being added to tried if they would evict an address. Since the feeler thread only creates one new connection every 2 minutes the additional network overhead is limited.
  - An address in tried gains immunity from tests for 4 hours after it has been tested or successfully connected to.
  # Tests:

  This change includes additional addrman unittests which test this behavior.

  I ran an instance of this change with a much smaller tried table (2 buckets of 64 addresses) so that collisions were much more likely and observed evictions.

  ```
  2016-10-27 07:20:26 Swapping 208.12.64.252:8333 for 68.62.95.247:8333 in tried table
  2016-10-27 07:20:26 Moving 208.12.64.252:8333 to tried
  ```

  I documented tests we ran against similar earlier versions of this change in bitcoin#6355.
  # Security Benefit

  This is was originally posted in PR bitcoin#8282 see [this comment for full details](bitcoin#8282 (comment)).

  To determine the security benefit of these larger numbers of IPs in the tried table I modeled the attack presented in [Eclipse Attacks on Bitcoin’s Peer-to-Peer Network](https://eprint.iacr.org/2015/263).

  ![attackergraph40000-10-1000short-line](https://cloud.githubusercontent.com/assets/274814/17366828/372af458-595b-11e6-81e5-2c9f97282305.png)

  **Default node:** 595 attacker IPs for ~50% attack success.
  **Default node + test-before-evict:** 620 attacker IPs for ~50% attack success.
  **Feeler node:** 5540 attacker IPs for ~50% attack success.
  **Feeler node + test-before-evict:** 8600 attacker IPs for ~50% attack success.

  The node running feeler connections has 10 times as many online IP addresses in its tried table making an attack 10 times harder (i.e. requiring the an attacker require 10 times as many IP addresses in different /16s). Adding test-before-evict increases resistance of the node by an additional 3000 attacker IP addresses.

  Below I graph the attack over even greater attacker resources (i.e. more attacker controled IP addresses). Note that test-before-evict maintains some security far longer even against an attacker with 50,000 IPs. If this node had a larger tried table test-before-evict could greatly boost a nodes resistance to eclipse attacks.

  ![attacker graph long view](https://cloud.githubusercontent.com/assets/274814/17367108/96f46d64-595c-11e6-91cd-edba160598e7.png)

Tree-SHA512: fdad4d26aadeaad9bcdc71929b3eb4e1f855b3ee3541fbfbe25dca8d7d0a1667815402db0cb4319db6bd3fcd32d67b5bbc0e12045c4252d62d6239b7d77c4395
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jun 13, 2020
b4db76c net: Correct addrman logging (Wladimir J. van der Laan)

Pull request description:

  These were introduced in bitcoin#9037.

  Found by @theuni (bitcoin#9037 (review)).

Tree-SHA512: 9b5153da8a8e5d4ddf9513a5c453f9609cffd4df2924fd48c7b36c1b1055748c7077d4fc0e70be62ca36af87df7f621a744bb374a234baba271ce4982a240825
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jun 13, 2020
e68172e Add test-before-evict discipline to addrman (Ethan Heilman)

Pull request description:

  This change implement countermeasures 3 (test-before-evict) suggested in our paper: ["Eclipse Attacks on Bitcoin’s Peer-to-Peer Network"](http://cs-people.bu.edu/heilman/eclipse/).
  # Design:

  A collision occurs when an address, addr1, is being moved to the tried table from the new table, but maps to a position in the tried table which already contains an address (addr2). The current behavior is that addr1 would evict addr2 from the tried table.

  This change ensures that during a collision, addr1 is not inserted into tried but instead inserted into a buffer (setTriedCollisions). The to-be-evicted address, addr2, is then tested by [a feeler connection](bitcoin#8282). If addr2 is found to be online, we remove addr1 from the buffer and addr2 is not evicted, on the other hand if addr2 is found be offline it is replaced by addr1.

  An additional small advantage of this change is that, as no more than ten addresses can be in the test buffer at once, and addresses are only cleared one at a time from the test buffer (at 2 minute intervals), thus an attacker is forced to wait at least two minutes to insert a new address into tried after filling up the test buffer. This rate limits an attacker attempting to launch an eclipse attack.
  # Risk mitigation:
  - To prevent this functionality from being used as a DoS vector, we limit the number of addresses which are to be tested to ten. If we have more than ten addresses to test, we drop new addresses being added to tried if they would evict an address. Since the feeler thread only creates one new connection every 2 minutes the additional network overhead is limited.
  - An address in tried gains immunity from tests for 4 hours after it has been tested or successfully connected to.
  # Tests:

  This change includes additional addrman unittests which test this behavior.

  I ran an instance of this change with a much smaller tried table (2 buckets of 64 addresses) so that collisions were much more likely and observed evictions.

  ```
  2016-10-27 07:20:26 Swapping 208.12.64.252:8333 for 68.62.95.247:8333 in tried table
  2016-10-27 07:20:26 Moving 208.12.64.252:8333 to tried
  ```

  I documented tests we ran against similar earlier versions of this change in bitcoin#6355.
  # Security Benefit

  This is was originally posted in PR bitcoin#8282 see [this comment for full details](bitcoin#8282 (comment)).

  To determine the security benefit of these larger numbers of IPs in the tried table I modeled the attack presented in [Eclipse Attacks on Bitcoin’s Peer-to-Peer Network](https://eprint.iacr.org/2015/263).

  ![attackergraph40000-10-1000short-line](https://cloud.githubusercontent.com/assets/274814/17366828/372af458-595b-11e6-81e5-2c9f97282305.png)

  **Default node:** 595 attacker IPs for ~50% attack success.
  **Default node + test-before-evict:** 620 attacker IPs for ~50% attack success.
  **Feeler node:** 5540 attacker IPs for ~50% attack success.
  **Feeler node + test-before-evict:** 8600 attacker IPs for ~50% attack success.

  The node running feeler connections has 10 times as many online IP addresses in its tried table making an attack 10 times harder (i.e. requiring the an attacker require 10 times as many IP addresses in different /16s). Adding test-before-evict increases resistance of the node by an additional 3000 attacker IP addresses.

  Below I graph the attack over even greater attacker resources (i.e. more attacker controled IP addresses). Note that test-before-evict maintains some security far longer even against an attacker with 50,000 IPs. If this node had a larger tried table test-before-evict could greatly boost a nodes resistance to eclipse attacks.

  ![attacker graph long view](https://cloud.githubusercontent.com/assets/274814/17367108/96f46d64-595c-11e6-91cd-edba160598e7.png)

Tree-SHA512: fdad4d26aadeaad9bcdc71929b3eb4e1f855b3ee3541fbfbe25dca8d7d0a1667815402db0cb4319db6bd3fcd32d67b5bbc0e12045c4252d62d6239b7d77c4395
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jun 13, 2020
b4db76c net: Correct addrman logging (Wladimir J. van der Laan)

Pull request description:

  These were introduced in bitcoin#9037.

  Found by @theuni (bitcoin#9037 (review)).

Tree-SHA512: 9b5153da8a8e5d4ddf9513a5c453f9609cffd4df2924fd48c7b36c1b1055748c7077d4fc0e70be62ca36af87df7f621a744bb374a234baba271ce4982a240825
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jun 17, 2020
e68172e Add test-before-evict discipline to addrman (Ethan Heilman)

Pull request description:

  This change implement countermeasures 3 (test-before-evict) suggested in our paper: ["Eclipse Attacks on Bitcoin’s Peer-to-Peer Network"](http://cs-people.bu.edu/heilman/eclipse/).
  # Design:

  A collision occurs when an address, addr1, is being moved to the tried table from the new table, but maps to a position in the tried table which already contains an address (addr2). The current behavior is that addr1 would evict addr2 from the tried table.

  This change ensures that during a collision, addr1 is not inserted into tried but instead inserted into a buffer (setTriedCollisions). The to-be-evicted address, addr2, is then tested by [a feeler connection](bitcoin#8282). If addr2 is found to be online, we remove addr1 from the buffer and addr2 is not evicted, on the other hand if addr2 is found be offline it is replaced by addr1.

  An additional small advantage of this change is that, as no more than ten addresses can be in the test buffer at once, and addresses are only cleared one at a time from the test buffer (at 2 minute intervals), thus an attacker is forced to wait at least two minutes to insert a new address into tried after filling up the test buffer. This rate limits an attacker attempting to launch an eclipse attack.
  # Risk mitigation:
  - To prevent this functionality from being used as a DoS vector, we limit the number of addresses which are to be tested to ten. If we have more than ten addresses to test, we drop new addresses being added to tried if they would evict an address. Since the feeler thread only creates one new connection every 2 minutes the additional network overhead is limited.
  - An address in tried gains immunity from tests for 4 hours after it has been tested or successfully connected to.
  # Tests:

  This change includes additional addrman unittests which test this behavior.

  I ran an instance of this change with a much smaller tried table (2 buckets of 64 addresses) so that collisions were much more likely and observed evictions.

  ```
  2016-10-27 07:20:26 Swapping 208.12.64.252:8333 for 68.62.95.247:8333 in tried table
  2016-10-27 07:20:26 Moving 208.12.64.252:8333 to tried
  ```

  I documented tests we ran against similar earlier versions of this change in bitcoin#6355.
  # Security Benefit

  This is was originally posted in PR bitcoin#8282 see [this comment for full details](bitcoin#8282 (comment)).

  To determine the security benefit of these larger numbers of IPs in the tried table I modeled the attack presented in [Eclipse Attacks on Bitcoin’s Peer-to-Peer Network](https://eprint.iacr.org/2015/263).

  ![attackergraph40000-10-1000short-line](https://cloud.githubusercontent.com/assets/274814/17366828/372af458-595b-11e6-81e5-2c9f97282305.png)

  **Default node:** 595 attacker IPs for ~50% attack success.
  **Default node + test-before-evict:** 620 attacker IPs for ~50% attack success.
  **Feeler node:** 5540 attacker IPs for ~50% attack success.
  **Feeler node + test-before-evict:** 8600 attacker IPs for ~50% attack success.

  The node running feeler connections has 10 times as many online IP addresses in its tried table making an attack 10 times harder (i.e. requiring the an attacker require 10 times as many IP addresses in different /16s). Adding test-before-evict increases resistance of the node by an additional 3000 attacker IP addresses.

  Below I graph the attack over even greater attacker resources (i.e. more attacker controled IP addresses). Note that test-before-evict maintains some security far longer even against an attacker with 50,000 IPs. If this node had a larger tried table test-before-evict could greatly boost a nodes resistance to eclipse attacks.

  ![attacker graph long view](https://cloud.githubusercontent.com/assets/274814/17367108/96f46d64-595c-11e6-91cd-edba160598e7.png)

Tree-SHA512: fdad4d26aadeaad9bcdc71929b3eb4e1f855b3ee3541fbfbe25dca8d7d0a1667815402db0cb4319db6bd3fcd32d67b5bbc0e12045c4252d62d6239b7d77c4395
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jun 17, 2020
b4db76c net: Correct addrman logging (Wladimir J. van der Laan)

Pull request description:

  These were introduced in bitcoin#9037.

  Found by @theuni (bitcoin#9037 (review)).

Tree-SHA512: 9b5153da8a8e5d4ddf9513a5c453f9609cffd4df2924fd48c7b36c1b1055748c7077d4fc0e70be62ca36af87df7f621a744bb374a234baba271ce4982a240825
random-zebra added a commit to PIVX-Project/PIVX that referenced this issue Aug 27, 2020
e733939 [Cleanup] Remove CConnman::Copy(Release)NodeVector, now unused (random-zebra)
b09da57 [Refactor] Proper CConnman encapsulation of mnsync (random-zebra)
219061e [Refactor] Decouple SyncWithNode from CMasternodeSync::Process() (random-zebra)
e1f3620 [Refactor] Proper CConnman encapsulation of proposals / votes sync (random-zebra)
f38c9f9 miner: don't try to access connman if it doesn't exist (Fuzzbawls)
92ab619 Address feedback (Fuzzbawls)
9d5346a Do not set an addr time penalty when a peer advertises itself. (Gregory Maxwell)
fe12ff9 net: No longer send local address in addrMe (Wladimir J. van der Laan)
14d6917 net: misc header cleanups (Cory Fields)
34ba0de net: make proxy receives interruptible (Cory Fields)
1bd97ef net: make net processing interruptable (Fuzzbawls)
e24b4cc net: make net interruptible (Cory Fields)
814d0de net: add CThreadInterrupt and InterruptibleSleep (Cory Fields)
1443f2a net: a few small cleanups before replacing boost threads (Cory Fields)
58eabe4 net: move MAX_FEELER_CONNECTIONS into connman (Cory Fields)
874baba Convert ForEachNode* functions to take a templated function argument rather than a std::function to eliminate std::function overhead (Jeremy Rubin)
9485ab0 Made the ForEachNode* functions in src/net.cpp more pragmatic and self documenting (Jeremy Rubin)
c1e59ad minor net cleanups (Fuzzbawls)
07ae004 net: move vNodesDisconnected into CConnman (Cory Fields)
276c946 net: add nSendBufferMaxSize/nReceiveFloodSize to CConnection::Options (Cory Fields)
22a9aff net: Introduce CConnection::Options to avoid passing so many params (Cory Fields)
e4891bf net: Drop StartNode/StopNode and use CConnman directly (Cory Fields)
431575c net: pass CClientUIInterface into CConnman (Cory Fields)
48de47e net: Pass best block known height into CConnman (Cory Fields)
15eed91 net: move max/max-outbound to CConnman (Cory Fields)
2bf0921 net: move semOutbound to CConnman (Cory Fields)
481929f net: move nLocalServices/nRelevantServices to CConnman (Cory Fields)
bcee6ae net: move SendBufferSize/ReceiveFloodSize to CConnman (Cory Fields)
6865469 net: move send/recv statistics to CConnman (Cory Fields)
1cec418 net: SocketSendData returns written size (Cory Fields)
2bb9dfa net: move messageHandlerCondition to CConnman (Cory Fields)
9c5a0df net: move nLocalHostNonce to CConnman (Cory Fields)
a1394ef net: move nLastNodeId to CConnman (Cory Fields)
3804c29 net: move whitelist functions into CConnman (Cory Fields)
dbde9be net: create generic functor accessors and move vNodes to CConnman (Cory Fields)
2e02467 net: Add most functions needed for vNodes to CConnman (Cory Fields)
5667e61 net: move added node functions to CConnman (Cory Fields)
37487ed net: Add oneshot functions to CConnman (Cory Fields)
facf878 net: move ban and addrman functions into CConnman (Cory Fields)
091aaf2 net: handle nodesignals in CConnman (Cory Fields)
1e9fa0f net: move OpenNetworkConnection into CConnman (Cory Fields)
573200f net: Move socket binding into CConnman (Cory Fields)
7762b97 net: Pass CConnection to wallet rather than using the global (Fuzzbawls)
00591b8 net: Pass CConnman around as needed (Cory Fields)
2cd3d39 net: Add rpc error for missing/disabled p2p functionality (Cory Fields)
f08e316 net: Create CConnman to encapsulate p2p connections (Cory Fields)
66337dc net: move CBanDB and CAddrDB out of net.h/cpp (Fuzzbawls)
10969f6 gui: add NodeID to the peer table (Cory Fields)
58044ac Fix some locks (Pieter Wuille)
f9f8926 Do not shadow variables in networking code (Pavel Janík)

Pull request description:

  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:

  - bitcoin#8466
    Do not shadow variables in networking code
  - bitcoin#8606
    Fix some locks
  - bitcoin#8085
    Begin encapsulation
  - bitcoin#9289
    drop boost::thread_group
  - bitcoin#8740
    No longer send local address in addrMe
  - bitcoin#8661
    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:
  - bitcoin#8715
    only delete CConnman if it's been created

  Still TODO in future PRs:
  - bitcoin#9037
  - bitcoin#9441
  - bitcoin#9609
  - bitcoin#9626
  - bitcoin#9708
  - bitcoin#12381
  - bitcoin#18584

ACKs for top commit:
  furszy:
    same here, code ACK e733939. Nice work  .
  furszy:
    ACK e733939 .
  random-zebra:
    ACK e733939 and merging...

Tree-SHA512: 0fc3cca76d9ddc13f75fc9d48c48d215e6c0e0381377c0318436176a0e0ead73b511e0061a4b63f7052874730b6da8b0ffc2c94cba034bcc39aad4212b69ee22
random-zebra added a commit to PIVX-Project/PIVX that referenced this issue Sep 7, 2020
30d5c66 net: correct addrman logging (Fuzzbawls)
8a2b7fe Don't send layer2 messages to peers that haven't completed the handshake (Fuzzbawls)
dc10100 [bugfix] Making tier two thread interruptable. (furszy)
2ae76aa Move CNode::addrLocal access behind locked accessors (Fuzzbawls)
470482f Move CNode::addrName accesses behind locked accessors (Fuzzbawls)
35365e1 Move [clean|str]SubVer writes/copyStats into a lock (Fuzzbawls)
d816a86 Make nServices atomic (Matt Corallo)
8a66add Make nStartingHeight atomic (Matt Corallo)
567c9b5 Avoid copying CNodeStats to make helgrind OK with buggy std::string (Matt Corallo)
aea5211 Make nTimeConnected const in CNode (Matt Corallo)
cf46680 net: fix a few races. (Fuzzbawls)
c916fcf net: add a lock around hSocket (Cory Fields)
cc8a93c net: rearrange so that socket accesses can be grouped together (Cory Fields)
6f731dc Do not add to vNodes until fOneShot/fFeeler/fAddNode have been set (Matt Corallo)
07c8d33 Ensure cs_vNodes is held when using the return value from FindNode (Matt Corallo)
110a44b Delete some unused (and broken) functions in CConnman (Matt Corallo)
08a12e0 net: log an error rather than asserting if send version is misused (Cory Fields)
cd8b82c net: Disallow sending messages until the version handshake is complete (Fuzzbawls)
54b454b net: don't run callbacks on nodes that haven't completed the version handshake (Cory Fields)
2be6877 net: deserialize the entire version message locally (Fuzzbawls)
444f599 Dont deserialize nVersion into CNode (Fuzzbawls)
f30f10e net: remove cs_vRecvMsg (Fuzzbawls)
5812f9e net: add a flag to indicate when a node's send buffer is full (Fuzzbawls)
5ec4db2 net: Hardcode protocol sizes and use fixed-size types (Wladimir J. van der Laan)
de87ea6 net: Consistent checksum handling (Wladimir J. van der Laan)
d4bcd25 net: push only raw data into CConnman (Cory Fields)
b79e416 net: add CVectorWriter and CNetMsgMaker (Cory Fields)
63c51d3 net: No need to check individually for disconnection anymore (Cory Fields)
07d8c7b net: don't send any messages before handshake or after fdisconnect (Cory Fields)
9adfc7f net: Set feelers to disconnect at the end of the version message (Cory Fields)
f88c06c net: handle version push in InitializeNode (Cory Fields)
04d39c8 net: construct CNodeStates in place (Cory Fields)
40a6c5d net: remove now-unused ssSend and Fuzz (Cory Fields)
681c62d drop the optimistic write counter hack (Cory Fields)
9f939f3 net: switch all callers to connman for pushing messages (Cory Fields)
8f9011d connman is in charge of pushing messages (Cory Fields)
f558bb7 serialization: teach serializers variadics (Cory Fields)
01ea667 net: Use deterministic randomness for CNode's nonce, and make it const (Cory Fields)
de1ad13 net: constify a few CNode vars to indicate that they're threadsafe (Cory Fields)
34050a3 Move static global randomizer seeds into CConnman (Pieter Wuille)
1ce349f net: add a flag to indicate when a node's process queue is full (Fuzzbawls)
5581b47 net: add a new message queue for the message processor (Fuzzbawls)
701b578 net: rework the way that the messagehandler sleeps (Fuzzbawls)
7e55dbf net: Add a simple function for waking the message handler (Cory Fields)
47ea844 net: record bytes written before notifying the message processor (Cory Fields)
ffd4859 net: handle message accounting in ReceiveMsgBytes (Cory Fields)
8cee696 net: log bytes recv/sent per command (Fuzzbawls)
754400e net: set message deserialization version when it's time to deserialize (Fuzzbawls)
d2b8e0a net: make CMessageHeader a dumb storage class (Fuzzbawls)
cc24eff net: remove redundant max sendbuffer size check (Fuzzbawls)
32ab0c0 net: wait until the node is destroyed to delete its recv buffer (Cory Fields)
6e3f71b net: only disconnect if fDisconnect has been set (Cory Fields)
1b0beb6 net: make GetReceiveFloodSize public (Cory Fields)
229697a net: make vRecvMsg a list so that we can use splice() (Fuzzbawls)
d2d71ba net: fix typo causing the wrong receive buffer size (Cory Fields)
50bb09d Add test-before-evict discipline to addrman (Ethan Heilman)

Pull request description:

  This is a combination of multiple upstream PRs focused on optimizing the P2P networking flow after the introduction of CConnman encapsulation, and a few older PRs that were previously missed to support the later optimizations. The PRs are as follows:

  - bitcoin#9037 - net: Add test-before-evict discipline to addrman
  - bitcoin#5151 - make CMessageHeader a dumb storage class
  - bitcoin#6589 - log bytes recv/sent per command
  - bitcoin#8688 - Move static global randomizer seeds into CConnman
  - bitcoin#9050 - net: make a few values immutable, and use deterministic randomness for the localnonce
  - bitcoin#8708 - net: have CConnman handle message sending
  - bitcoin#9128 - net: Decouple CConnman and message serialization
  - bitcoin#8822 - net: Consistent checksum handling
  - bitcoin#9441 - Net: Massive speedup. Net locks overhaul
  - bitcoin#9609 - net: fix remaining net assertions
  - bitcoin#9626 - Clean up a few CConnman cs_vNodes/CNode things
  - bitcoin#9698 - net: fix socket close race
  - bitcoin#9708 - Clean up all known races/platform-specific UB at the time PR was opened
    - Excluded bitcoin/bitcoin@512731b and bitcoin/bitcoin@d8f2b8a, to be done in a separate PR

ACKs for top commit:
  furszy:
    code ACK 30d5c66 , testnet sync from scratch went well and tested with #1829 on top as well and all good.
  furszy:
     mainnet sync went fine, ACK 30d5c66 .
  random-zebra:
    ACK 30d5c66 and merging...

Tree-SHA512: 09689554f53115a45f810b47ff75d887fa9097ea05992a638dbb6055262aeecd82d6ce5aaa2284003399d839b6f2c36f897413da96cfa2cd3b858387c3f752c1
charlesrocket added a commit to charlesrocket/axe that referenced this issue Dec 26, 2020
b4db76c55 net: Correct addrman logging (Wladimir J. van der Laan)

Pull request description:

  These were introduced in #9037.

  Found by @theuni (bitcoin/bitcoin#9037 (review)).

Tree-SHA512: 9b5153da8a8e5d4ddf9513a5c453f9609cffd4df2924fd48c7b36c1b1055748c7077d4fc0e70be62ca36af87df7f621a744bb374a234baba271ce4982a240825
gades pushed a commit to cosanta/cosanta-core that referenced this issue Jun 25, 2021
e68172e Add test-before-evict discipline to addrman (Ethan Heilman)

Pull request description:

  This change implement countermeasures 3 (test-before-evict) suggested in our paper: ["Eclipse Attacks on Bitcoin’s Peer-to-Peer Network"](http://cs-people.bu.edu/heilman/eclipse/).
  # Design:

  A collision occurs when an address, addr1, is being moved to the tried table from the new table, but maps to a position in the tried table which already contains an address (addr2). The current behavior is that addr1 would evict addr2 from the tried table.

  This change ensures that during a collision, addr1 is not inserted into tried but instead inserted into a buffer (setTriedCollisions). The to-be-evicted address, addr2, is then tested by [a feeler connection](bitcoin#8282). If addr2 is found to be online, we remove addr1 from the buffer and addr2 is not evicted, on the other hand if addr2 is found be offline it is replaced by addr1.

  An additional small advantage of this change is that, as no more than ten addresses can be in the test buffer at once, and addresses are only cleared one at a time from the test buffer (at 2 minute intervals), thus an attacker is forced to wait at least two minutes to insert a new address into tried after filling up the test buffer. This rate limits an attacker attempting to launch an eclipse attack.
  # Risk mitigation:
  - To prevent this functionality from being used as a DoS vector, we limit the number of addresses which are to be tested to ten. If we have more than ten addresses to test, we drop new addresses being added to tried if they would evict an address. Since the feeler thread only creates one new connection every 2 minutes the additional network overhead is limited.
  - An address in tried gains immunity from tests for 4 hours after it has been tested or successfully connected to.
  # Tests:

  This change includes additional addrman unittests which test this behavior.

  I ran an instance of this change with a much smaller tried table (2 buckets of 64 addresses) so that collisions were much more likely and observed evictions.

  ```
  2016-10-27 07:20:26 Swapping 208.12.64.252:8333 for 68.62.95.247:8333 in tried table
  2016-10-27 07:20:26 Moving 208.12.64.252:8333 to tried
  ```

  I documented tests we ran against similar earlier versions of this change in bitcoin#6355.
  # Security Benefit

  This is was originally posted in PR bitcoin#8282 see [this comment for full details](bitcoin#8282 (comment)).

  To determine the security benefit of these larger numbers of IPs in the tried table I modeled the attack presented in [Eclipse Attacks on Bitcoin’s Peer-to-Peer Network](https://eprint.iacr.org/2015/263).

  ![attackergraph40000-10-1000short-line](https://cloud.githubusercontent.com/assets/274814/17366828/372af458-595b-11e6-81e5-2c9f97282305.png)

  **Default node:** 595 attacker IPs for ~50% attack success.
  **Default node + test-before-evict:** 620 attacker IPs for ~50% attack success.
  **Feeler node:** 5540 attacker IPs for ~50% attack success.
  **Feeler node + test-before-evict:** 8600 attacker IPs for ~50% attack success.

  The node running feeler connections has 10 times as many online IP addresses in its tried table making an attack 10 times harder (i.e. requiring the an attacker require 10 times as many IP addresses in different /16s). Adding test-before-evict increases resistance of the node by an additional 3000 attacker IP addresses.

  Below I graph the attack over even greater attacker resources (i.e. more attacker controled IP addresses). Note that test-before-evict maintains some security far longer even against an attacker with 50,000 IPs. If this node had a larger tried table test-before-evict could greatly boost a nodes resistance to eclipse attacks.

  ![attacker graph long view](https://cloud.githubusercontent.com/assets/274814/17367108/96f46d64-595c-11e6-91cd-edba160598e7.png)

Tree-SHA512: fdad4d26aadeaad9bcdc71929b3eb4e1f855b3ee3541fbfbe25dca8d7d0a1667815402db0cb4319db6bd3fcd32d67b5bbc0e12045c4252d62d6239b7d77c4395
gades added a commit to cosanta/cosanta-core that referenced this issue Jul 1, 2021
b4db76c net: Correct addrman logging (Wladimir J. van der Laan)

Pull request description:

  These were introduced in bitcoin#9037.

  Found by @theuni (bitcoin#9037 (review)).

Tree-SHA512: 9b5153da8a8e5d4ddf9513a5c453f9609cffd4df2924fd48c7b36c1b1055748c7077d4fc0e70be62ca36af87df7f621a744bb374a234baba271ce4982a240825
jnewbery added a commit to jnewbery/bitcoin that referenced this issue Jul 20, 2021
This has never been used in the public interface method since it was
introduced in bitcoin#9037.
jnewbery added a commit to jnewbery/bitcoin that referenced this issue Jul 20, 2021
This has never been used in the public interface method since it was
introduced in bitcoin#9037.
fanquake added a commit that referenced this issue Jul 21, 2021
…Good()

f036dfb [addrman] Remove unused test_before_evict argument from Good() (John Newbery)

Pull request description:

  This has never been used in the public interface method since it was
  introduced in #9037.

ACKs for top commit:
  lsilva01:
    Tested ACK f036dfb on Ubuntu 20.04.
  theStack:
    Code-review ACK f036dfb

Tree-SHA512: 98145d9596b4ae1f354cfa561be1a54c6b8057c920e0ac3d4c1d42c9326b2dad2d44320f4171bb701d97088b216760cca8017b84c8b5dcd2b1dc8f158f28066d
josibake added a commit to josibake/bitcoin that referenced this issue Jul 21, 2021
This has never been used in the public interface method since it was
introduced in bitcoin#9037.
sidhujag added a commit to syscoin/syscoin that referenced this issue Jul 23, 2021
…t from Good()

f036dfb [addrman] Remove unused test_before_evict argument from Good() (John Newbery)

Pull request description:

  This has never been used in the public interface method since it was
  introduced in bitcoin#9037.

ACKs for top commit:
  lsilva01:
    Tested ACK bitcoin@f036dfb on Ubuntu 20.04.
  theStack:
    Code-review ACK f036dfb

Tree-SHA512: 98145d9596b4ae1f354cfa561be1a54c6b8057c920e0ac3d4c1d42c9326b2dad2d44320f4171bb701d97088b216760cca8017b84c8b5dcd2b1dc8f158f28066d
rebroad pushed a commit to rebroad/bitcoin that referenced this issue Jul 29, 2021
This has never been used in the public interface method since it was
introduced in bitcoin#9037.
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants