EthanHeilman commented at 9:53 PM on October 28, 2016: contributor

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 commented at 9:54 PM on October 28, 2016: contributor

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

EthanHeilman commented at 10:04 PM on October 28, 2016: contributor

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.

in src/addrman.cpp:None in 18e642c9c5 outdated

 569 | +
 570 | +            // Has attempted to connect and failed in last X hours
 571 | +            } else if (GetAdjustedTime() - infoOld.nLastTry < ADDRMAN_REPLACEMENT_HOURS*(60*60)) {
 572 | +                LogPrint("addrman", "Swapping %s for %s in tried table\n", infoNew.ToString(), infoOld.ToString());
 573 | +                // Replaces an existing address already in the tried table with the new address
 574 | +                Good(infoNew, false);

kazcw commented at 10:27 PM on October 28, 2016:

Lock is held, could use Good_

in src/addrman.cpp:None in 18e642c9c5 outdated

 573 | +                // Replaces an existing address already in the tried table with the new address
 574 | +                Good(infoNew, false);
 575 | +                eraseCollision = true;
 576 | +            }
 577 | +        } else { // Collision is not actually a collision anymore
 578 | +            Good(infoNew, false);

kazcw commented at 10:28 PM on October 28, 2016:

Lock held as above

EthanHeilman commented at 2:00 AM on October 30, 2016:

Good call!

in src/addrman.cpp:None in 18e642c9c5 outdated

 539 | @@ -528,3 +540,75 @@ void CAddrMan::SetServices_(const CService& addr, ServiceFlags nServices)
 540 |  int CAddrMan::RandomInt(int nMax){
 541 |      return GetRandInt(nMax);
 542 |  }
 543 | +
 544 | +void CAddrMan::ResolveCollisions_()
 545 | +{
 546 | +    for (std::set<int>::iterator it = setTriedCollisions.begin(); it != setTriedCollisions.end();) {
 547 | +        int nIdnew = (int)*it;

kazcw commented at 10:29 PM on October 28, 2016:

Nit: is this cast necessary?

EthanHeilman commented at 2:00 AM on October 30, 2016:

Nope, removed it.

in src/addrman.cpp:None in 18e642c9c5 outdated

 591 | +{
 592 | +    int nRnd = GetRandInt(setTriedCollisions.size());
 593 | +
 594 | +    int i = 0;
 595 | +    // Selects a random element from setTriedCollisions.
 596 | +    for (std::set<int>::iterator it = setTriedCollisions.begin(); it != setTriedCollisions.end(); it++) {

kazcw commented at 11:15 PM on October 28, 2016:

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

kazcw approved

kazcw commented at 11:41 PM on October 28, 2016: contributor

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

fanquake added the label P2P on Oct 29, 2016

EthanHeilman commented at 7:38 PM on November 1, 2016: contributor

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 commented at 11:36 PM on November 21, 2016: contributor

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

EthanHeilman commented at 12:38 AM on December 9, 2016: contributor

Fixed a merge conflict.

EthanHeilman commented at 8:51 PM on December 18, 2016: contributor

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

sipa commented at 3:45 PM on April 9, 2017: member

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 commented at 5:05 PM on April 10, 2017: contributor

@sipa Will rebase today or tomorrow.

EthanHeilman commented at 1:43 PM on April 13, 2017: contributor

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

EthanHeilman commented at 8:35 AM on July 27, 2017: contributor

Updated to obey new Bitcoin coding style guide

EthanHeilman commented at 8:48 PM on August 10, 2017: contributor

ping @theuni

theuni commented at 9:51 PM on August 16, 2017: member

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

in src/addrman.cpp:558 in 8604020bab outdated

 553 | +
 554 | +            if (!info_new.IsValid()) { // id_new may no longer map to a valid address
 555 | +                erase_collision = true;
 556 | +
 557 | +            // The position in the tried bucket is not empty
 558 | +            } else if (vvTried[tried_bucket][tried_bucket_pos] != -1) {

kallewoof commented at 1:19 AM on February 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?

EthanHeilman commented at 2:28 AM on February 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.

kallewoof commented at 2:37 AM on February 26, 2018:

There are plenty of examples of this, I think.

E.g. https://github.com/bitcoin/bitcoin/blob/bf3353de90598f08a68d966c50b57ceaeb5b5d96/src/validation.cpp#L1399-L1402

https://github.com/bitcoin/bitcoin/blob/bf3353de90598f08a68d966c50b57ceaeb5b5d96/src/validation.cpp#L2416-L2418

in src/addrman.cpp:569 in 8604020bab outdated

 564 | +                // Has successfully connected in last X hours
 565 | +                if (GetAdjustedTime() - info_old.nLastSuccess < ADDRMAN_REPLACEMENT_HOURS*(60*60)) {
 566 | +                    erase_collision = true;
 567 | +
 568 | +                // Has attempted to connect and failed in last X hours
 569 | +                } else if (GetAdjustedTime() - info_old.nLastTry < ADDRMAN_REPLACEMENT_HOURS*(60*60)) {

kallewoof commented at 1:20 AM on February 20, 2018:

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

in src/addrman.cpp:542 in 8604020bab outdated

 537 | +void CAddrMan::ResolveCollisions_()
 538 | +{
 539 | +    for (std::set<int>::iterator it = m_tried_collisions.begin(); it != m_tried_collisions.end();) {
 540 | +        int id_new = *it;
 541 | +
 542 | +        bool erase_collision = false;

kallewoof commented at 1:24 AM on February 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.

EthanHeilman commented at 2:23 AM on February 26, 2018:

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

in src/addrman.cpp:600 in 8604020bab outdated

 595 | +
 596 | +    // Selects a random element from m_tried_collisions
 597 | +    std::advance(it, GetRandInt(m_tried_collisions.size()));
 598 | +    int id_new = *it;
 599 | +
 600 | +    // If newId not found in mapInfo remove it from m_tried_collisions

kallewoof commented at 1:26 AM on February 20, 2018:

Comment mentions old name (newId).

EthanHeilman commented at 2:24 AM on February 26, 2018:

Good catch, thanks!

in src/addrman.cpp:615 in 8604020bab outdated

 610 | +    int tried_bucket_pos = newInfo.GetBucketPosition(nKey, false, tried_bucket);
 611 | +
 612 | +    int id_old = vvTried[tried_bucket][tried_bucket_pos];
 613 | +    CAddrInfo& info_old = mapInfo[id_old];
 614 | +
 615 | +    return info_old;

kallewoof commented at 1:27 AM on February 20, 2018:

Maybe just return mapInfo[id_old];?

EthanHeilman commented at 2:28 AM on February 26, 2018:

done

in src/test/addrman_tests.cpp:577 in 8604020bab outdated

 572 | +    BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
 573 | +
 574 | +    // Add twenty two addresses.
 575 | +    CNetAddr source = ResolveIP("252.2.2.2");
 576 | +    for (unsigned int i = 1; i < 23; i++) {
 577 | +        CService addr = ResolveService("250.1.1."+boost::to_string(i));

kallewoof commented at 1:29 AM on February 20, 2018:

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

in src/test/addrman_tests.cpp:588 in 8604020bab outdated

 583 | +        BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
 584 | +    }
 585 | +
 586 | +    // Ensure Good handles duplicates well.
 587 | +    for (unsigned int i = 1; i < 23; i++) {
 588 | +        CService addr = ResolveService("250.1.1."+boost::to_string(i));

kallewoof commented at 1:29 AM on February 20, 2018:

std::to_string

EthanHeilman commented at 10:16 PM on February 20, 2018:

Let me fix these and all push later tonight

EthanHeilman commented at 2:36 AM on February 26, 2018:

Fixed

kallewoof commented at 1:30 AM on February 20, 2018: member

utACK 8604020bab3963599ebfd933b91096ae1f686d69

in src/addrman.cpp:242 in 1fdbcc850a outdated

 238 | +    int tried_bucket = info.GetTriedBucket(nKey);
 239 | +    int tried_bucket_pos = info.GetBucketPosition(nKey, false, tried_bucket);
 240 | +
 241 | +    // Will moving this address into tried evict another entry?
 242 | +    if (test_before_evict && (vvTried[tried_bucket][tried_bucket_pos] != -1)) {
 243 | +        LogPrint(BCLog::ADDRMAN, "addrman", "Collision inserting element into tried table, moving %s to m_tried_collisions=%d \n", addr.ToString(), m_tried_collisions.size());

kallewoof commented at 2:39 AM on February 26, 2018:

Nit: remove space in =%d \n

in src/addrman.cpp:244 in 1fdbcc850a outdated

 241 | +    // Will moving this address into tried evict another entry?
 242 | +    if (test_before_evict && (vvTried[tried_bucket][tried_bucket_pos] != -1)) {
 243 | +        LogPrint(BCLog::ADDRMAN, "addrman", "Collision inserting element into tried table, moving %s to m_tried_collisions=%d \n", addr.ToString(), m_tried_collisions.size());
 244 | +
 245 | +        if (m_tried_collisions.size() < ADDRMAN_SET_TRIED_COLLISION_SIZE)
 246 | +            m_tried_collisions.insert(nId);

kallewoof commented at 2:40 AM on February 26, 2018:

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

in src/addrman.cpp:556 in 1fdbcc850a outdated

 551 | +            int tried_bucket = info_new.GetTriedBucket(nKey);
 552 | +            int tried_bucket_pos = info_new.GetBucketPosition(nKey, false, tried_bucket);
 553 | +
 554 | +            if (!info_new.IsValid()) { // id_new may no longer map to a valid address
 555 | +                erase_collision = true;
 556 | +

kallewoof commented at 2:41 AM on February 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).

EthanHeilman commented at 4:51 PM on March 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

in src/addrman.cpp:556 in 1fdbcc850a outdated

 553 | +
 554 | +            if (!info_new.IsValid()) { // id_new may no longer map to a valid address
 555 | +                erase_collision = true;
 556 | +            
 557 | +            } else if (vvTried[tried_bucket][tried_bucket_pos] != -1) { // The position in the tried bucket is not empty
 558 | +

kallewoof commented at 2:42 AM on February 26, 2018:

Nit: see above (your call, of course)

in src/addrman.cpp:566 in 1fdbcc850a outdated

 561 | +                CAddrInfo& info_old = mapInfo[id_old];
 562 | +
 563 | +                // Has successfully connected in last X hours
 564 | +                if (GetAdjustedTime() - info_old.nLastSuccess < ADDRMAN_REPLACEMENT_HOURS*(60*60)) {
 565 | +                    erase_collision = true;
 566 | +

kallewoof commented at 2:42 AM on February 26, 2018:

-'-

EthanHeilman commented at 4:54 PM on March 3, 2018:

What does -'- mean?

kallewoof commented at 9:54 PM on March 4, 2018:

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

in src/addrman.cpp:590 in 1fdbcc850a outdated

 585 | +}
 586 | +
 587 | +CAddrInfo CAddrMan::SelectTriedCollision_()
 588 | +{
 589 | +    if (m_tried_collisions.size() == 0)
 590 | +        return CAddrInfo();

kallewoof commented at 2:43 AM on February 26, 2018:

Use {} for multiline if

EthanHeilman commented at 4:58 PM on March 3, 2018:

I reread the style guide more carefully and now I see what you mean

in src/addrman.h:557 in 1fdbcc850a outdated

 557 |      {
 558 | -        LOCK(cs);
 559 | -        Check();
 560 | -        Good_(addr, nTime);
 561 | -        Check();
 562 | +        {

kallewoof commented at 2:46 AM on February 26, 2018:

Why the extra {}?

EthanHeilman commented at 5:05 PM on March 3, 2018:

Not sure, Bitcoin used to have a second set of braces there when this pull request was originally created. They have been removed in the latest version.

https://github.com/bitcoin/bitcoin/blob/fb26bf0ea3822638b10a783f054c280fc053a2b5/src/addrman.h#L526

in src/addrman.h:576 in 1fdbcc850a outdated

 570 | @@ -554,6 +571,28 @@ class CAddrMan
 571 |          Check();
 572 |      }
 573 |  
 574 | +    //! See if any to-be-evicted tried table entries have been tested and if so resolve the collisions.
 575 | +    void ResolveCollisions(){
 576 | +         {

kallewoof commented at 2:47 AM on February 26, 2018:

Same question here (why double {}?)

in src/addrman.h:575 in 1fdbcc850a outdated

 570 | @@ -554,6 +571,28 @@ class CAddrMan
 571 |          Check();
 572 |      }
 573 |  
 574 | +    //! See if any to-be-evicted tried table entries have been tested and if so resolve the collisions.
 575 | +    void ResolveCollisions(){

kallewoof commented at 2:47 AM on February 26, 2018:

Nit: space after () before {. I think functions actually put the { on the next line, which you did in other cases.

EthanHeilman commented at 5:12 PM on March 3, 2018:

I just double checked the style guide and you are correct current style is { on a new line for methods.

in src/addrman.h:585 in 1fdbcc850a outdated

 580 | +            Check();
 581 | +        }
 582 | +    }
 583 | +
 584 | +    //! Randomly select an address in tried that another address is attempting to evict.
 585 | +    CAddrInfo SelectTriedCollision(){

kallewoof commented at 2:48 AM on February 26, 2018:

Nit on (){ (add space or newline)

in src/addrman.h:585 in 1fdbcc850a outdated

 582 | +    }
 583 | +
 584 | +    //! Randomly select an address in tried that another address is attempting to evict.
 585 | +    CAddrInfo SelectTriedCollision(){
 586 | +        CAddrInfo ret;
 587 | +        {

kallewoof commented at 2:48 AM on February 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.

EthanHeilman commented at 5:17 PM on March 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

in src/net.cpp:1829 in 1fdbcc850a outdated

1823 | @@ -1824,11 +1824,18 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1824 |              }
1825 |          }
1826 |  
1827 | +        addrman.ResolveCollisions();
1828 | +
1829 | +

kallewoof commented at 2:50 AM on February 26, 2018:

Remove extra newline.

EthanHeilman commented at 5:18 PM on March 3, 2018:

done

in src/net.cpp:1837 in 1fdbcc850a outdated

1834 | -            CAddrInfo addr = addrman.Select(fFeeler);
1835 | +            CAddrInfo addr = addrman.SelectTriedCollision();
1836 | +
1837 | +            // SelectTriedCollision returns an invalid address if it is empty.
1838 | +            if (!fFeeler || !addr.IsValid())
1839 | +                addr = addrman.Select(fFeeler);

kallewoof commented at 2:50 AM on February 26, 2018:

Wrap multiline if in {}.

in src/test/addrman_tests.cpp:56 in 1fdbcc850a outdated

  51 | @@ -52,6 +52,13 @@ class CAddrManTest : public CAddrMan
  52 |      {
  53 |          CAddrMan::Delete(nId);
  54 |      }
  55 | +
  56 | +    void SimConnFail(CService& addr){

kallewoof commented at 2:50 AM on February 26, 2018:

Space or newline before {

kallewoof commented at 2:51 AM on February 26, 2018: member

utACK 1fdbcc850ac5eaa5d6d51556f19bf0ff522b9ef9

Sorry, some more comments after re-reading.

in src/addrman.cpp:247 in 76179949e0 outdated

 242 | +        LogPrint(BCLog::ADDRMAN, "addrman", "Collision inserting element into tried table, moving %s to m_tried_collisions=%d\n", addr.ToString(), m_tried_collisions.size());
 243 | +        if (m_tried_collisions.size() < ADDRMAN_SET_TRIED_COLLISION_SIZE) {
 244 | +            m_tried_collisions.insert(nId);
 245 | +        }
 246 | +    } else {
 247 |      LogPrint(BCLog::ADDRMAN, "Moving %s to tried\n", addr.ToString());

kallewoof commented at 9:56 PM on March 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.

in src/test/addrman_tests.cpp:57 in 76179949e0 outdated

  51 | @@ -52,6 +52,14 @@ class CAddrManTest : public CAddrMan
  52 |      {
  53 |          CAddrMan::Delete(nId);
  54 |      }
  55 | +
  56 | +    void SimConnFail(CService& addr)

kallewoof commented at 9:59 PM on March 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?

EthanHeilman commented at 5:27 PM on March 5, 2018:

Added comment

EthanHeilman commented at 5:34 PM on March 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.

kallewoof commented at 10:00 PM on March 4, 2018: member

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.

fanquake added this to the "Blockers" column in a project

kallewoof approved

kallewoof commented at 6:46 PM on March 5, 2018: member

utACK 8989b6f1aee08e860cda864b085e8d33749dcebe

sdaftuar commented at 8:55 PM on March 5, 2018: member

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 commented at 9:48 PM on March 5, 2018: contributor

@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?

in src/addrman.cpp:565 in 8989b6f1ae outdated

 560 | +                // Has successfully connected in last X hours
 561 | +                if (GetAdjustedTime() - info_old.nLastSuccess < ADDRMAN_REPLACEMENT_HOURS*(60*60)) {
 562 | +                    erase_collision = true;
 563 | +                } else if (GetAdjustedTime() - info_old.nLastTry < ADDRMAN_REPLACEMENT_HOURS*(60*60)) { // attempted to connect and failed in last X hours
 564 | +                    LogPrint(BCLog::ADDRMAN, "addrman", "Swapping %s for %s in tried table\n", info_new.ToString(), info_old.ToString());
 565 | +

sipa commented at 9:52 PM on March 5, 2018:

The trailing spaces are on this line.

sipa commented at 3:20 PM on March 6, 2018: member

@EthanHeilman It seems the unit tests fail now.

Add test-before-evict discipline to addrman

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.

e68172ed9f

in src/test/addrman_tests.cpp:62 in 06464318cf outdated

  57 | +    void SimConnFail(CService& addr)
  58 | +    {
  59 | +         int64_t time = 1;
  60 | +         Good_(addr, true, time); // Set last good connection in the deep past.
  61 | +         bool count_failure = false;
  62 | +         Attempt(addr, count_failure);

sdaftuar commented at 4:04 PM on March 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

sdaftuar commented at 6:50 PM on March 6, 2018: member

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

sipa commented at 8:07 PM on March 6, 2018: member

utACK e68172ed9fd0e35d4e848142e9b36ffcc41de254

kallewoof commented at 8:28 PM on March 6, 2018: member

utACK e68172e

in src/addrman.cpp:242 in e68172ed9f

 238 | +    int tried_bucket = info.GetTriedBucket(nKey);
 239 | +    int tried_bucket_pos = info.GetBucketPosition(nKey, false, tried_bucket);
 240 | +
 241 | +    // Will moving this address into tried evict another entry?
 242 | +    if (test_before_evict && (vvTried[tried_bucket][tried_bucket_pos] != -1)) {
 243 | +        LogPrint(BCLog::ADDRMAN, "addrman", "Collision inserting element into tried table, moving %s to m_tried_collisions=%d\n", addr.ToString(), m_tried_collisions.size());

theuni commented at 8:32 PM on March 6, 2018:

I'm assuming this was a rebase issue, but the "addrman" param is unnecessary here. It's actually not clear to me what this would do.

sipa commented at 8:33 PM on March 6, 2018:

I think it will log "addrman" and nothing more.

laanwj commented at 8:43 PM on March 6, 2018:

Whoops, sorry @theuni. I think it will log "Error not enough conversion specifiers while formatting log message: addrman"

laanwj commented at 9:02 PM on March 6, 2018:

See #12622

theuni commented at 8:33 PM on March 6, 2018: member

utACK modulo the log comment.

laanwj merged this on Mar 6, 2018

laanwj closed this on Mar 6, 2018

laanwj referenced this in commit a36834f10b on Mar 6, 2018

laanwj referenced this in commit 3f5e116609 on Mar 6, 2018

laanwj referenced this in commit b4db76c550 on Mar 6, 2018

laanwj referenced this in commit e1d6e2af6d on Mar 6, 2018

fanquake removed this from the "Blockers" column in a project

sipsorcery referenced this in commit f6c6e65b35 on Mar 7, 2018

sipsorcery referenced this in commit fbfd1f0b24 on Mar 7, 2018

sickpig referenced this in commit 80bb2c96a5 on Apr 27, 2018

sickpig referenced this in commit c284916d03 on Apr 27, 2018

sickpig referenced this in commit 562fe5d417 on Apr 27, 2018

HashUnlimited referenced this in commit 95e636c735 on Jul 31, 2018

lionello referenced this in commit 67e4b519bb on Nov 7, 2018

PastaPastaPasta referenced this in commit 84620651c8 on Jun 10, 2020

PastaPastaPasta referenced this in commit 903d29cbe3 on Jun 10, 2020

PastaPastaPasta referenced this in commit 8e50809b48 on Jun 13, 2020

PastaPastaPasta referenced this in commit 231301b009 on Jun 13, 2020

PastaPastaPasta referenced this in commit 79b2ce9b86 on Jun 13, 2020

PastaPastaPasta referenced this in commit b13c43b89b on Jun 13, 2020

PastaPastaPasta referenced this in commit 8b9012d292 on Jun 13, 2020

PastaPastaPasta referenced this in commit a2a37735d9 on Jun 13, 2020

PastaPastaPasta referenced this in commit f442923aee on Jun 17, 2020

PastaPastaPasta referenced this in commit c94e51f0a5 on Jun 17, 2020

random-zebra referenced this in commit 777638e7bc on Aug 27, 2020

random-zebra referenced this in commit cbd9271afb on Sep 7, 2020

gades referenced this in commit 7ad9414b05 on Jun 25, 2021

gades referenced this in commit 2cdf49674c on Jul 1, 2021

jnewbery referenced this in commit e6caaea0ea on Jul 20, 2021

jnewbery referenced this in commit f036dfbb69 on Jul 20, 2021

fanquake referenced this in commit 0fffd6c4fb on Jul 21, 2021

josibake referenced this in commit a2ba966358 on Jul 21, 2021

sidhujag referenced this in commit 7baf6a896b on Jul 23, 2021

rebroad referenced this in commit b3a66956e9 on Jul 29, 2021

MarcoFalke locked this on Sep 8, 2021

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

Design:

Risk mitigation:

Tests:

Security Benefit