Add recently accepted blocks and txn to AttemptToEvictConnection.

gmaxwell commented at 5:59 am on May 22, 2016: contributor

This protect any not-already-protected peers who were the most recent to relay transactions and blocks to us.

This also takes increases the eviction agressiveness by making it willing to disconnect a netgroup with only one member.

in src/net.cpp: in bba64dde5f outdated

956+    vEvictionCandidates.erase(vEvictionCandidates.end() - std::min(4, static_cast<int>(vEvictionCandidates.size())), vEvictionCandidates.end());
957+
958+    if (vEvictionCandidates.empty()) return false;
959+
960+    // Protect 4 nodes that most recently sent us blocks.
961+    // An attacker cannot manipulate this metric without performing useful work.

wallclockbuilder commented at 6:04 am on May 22, 2016:

Ack

rebroad commented at 12:34 pm on August 24, 2016:

Actually, I think an attacker can manipulate this metric without performing useful work. It’s quite trivial to for a node to send blocks invs to all its connected nodes as soon as it sees a block inv for a new block, and this can mean that many nodes see the block inv from this “cheating” node first since it didn’t even need to start downloading the block before it was able to do this. Nodes try to download the block from the first node that alert about it, which would mean it was this “cheating” node that would be the node that provides the full block, and therefore this would mean that BOTH the block propagation is slowed down AND that the node(s) responsible for this happening continue to be prioritized due to this code. A better solution would be to regularly test which of the connected nodes actually deliver the block quickest - perhaps by requesting blocks during quiet periods (when a new block hasn’t just been released) and see which deliver it fastest. Also, it might be that the node which broadcasts the block inv first becomes one of the slowest nodes to relay the actual block due to the fact that so many nodes are requesting the block from it. Perhaps the 2nd or 3rd node that sent the block inv would be able to deliver the block quicker. Perhaps, depending on the bandwidth of the sending and the receiving node, it would be more efficient and safer to request the newly announced block from 2 or more nodes concurrently, and reward the node (in this code) that delivered it soonest. The bandwidth of nodes (ours and theirs) could be tested and this information used to determine the best way to request the blocks.

in src/net.cpp: in bba64dde5f outdated

856@@ -852,6 +857,21 @@ static bool ReverseCompareNodeTimeConnected(const NodeEvictionCandidate &a, cons
857     return a.nTimeConnected > b.nTimeConnected;
858 }
859 
860+static bool CompareNodeBlockTime(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b)
861+{
862+    if (a.nLastBlockTime != b.nLastBlockTime) return a.nLastBlockTime < b.nLastBlockTime;

sipa commented at 8:30 am on May 22, 2016:

Isn’t this backward? This sorts by increasing last block time, and erases the last items.

gmaxwell commented at 8:43 am on May 22, 2016:

Erasing protects them from being evicted, thus keeping the most recent sources.

sipa commented at 9:02 am on May 22, 2016:

Ah!

CodeShark commented at 10:48 am on May 22, 2016:

Can be attacked by sending the same block repeatedly.

gmaxwell commented at 1:16 pm on May 22, 2016:

Fixed. (@sipa is this what you suggested?)

sipa commented at 12:40 pm on May 26, 2016:

Indeed!

laanwj added the label P2P on May 22, 2016

arowser commented at 8:43 am on May 25, 2016: contributor

Can one of the admins verify this patch?

sipa commented at 12:40 pm on May 26, 2016: member

utACK e6d8aaed0e4cc2a82715969371a0c026a3fc47fd

sipa commented at 5:35 pm on May 31, 2016: member

Needs rebase.

gmaxwell commented at 8:42 pm on May 31, 2016: contributor

Rebased.

btcdrak commented at 7:57 pm on June 9, 2016: contributor

needs rebase.

gmaxwell commented at 10:10 pm on June 9, 2016: contributor

Rebased.

pstratem commented at 2:34 am on June 10, 2016: contributor

@gmaxwell Can you split this into two commits?

gmaxwell commented at 2:40 am on June 10, 2016: contributor

No problem.

in src/net.cpp: in fa9d28da1a outdated

858@@ -854,7 +859,22 @@ static bool ReverseCompareNodeTimeConnected(const NodeEvictionCandidate &a, cons
859 
860 static bool CompareNetGroupKeyed(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b) {
861     return a.nKeyedNetGroup < b.nKeyedNetGroup;
862-};
863+}
864+
865+static bool CompareNodeBlockTime(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b)
866+{
867+    if (a.nLastBlockTime != b.nLastBlockTime) return a.nLastBlockTime < b.nLastBlockTime;

pstratem commented at 2:56 am on June 10, 2016:

Please add a comment here explaining why there is a fail through.

pstratem commented at 3:16 am on June 10, 2016: contributor

ACK 3b3f45d532bd8f4c917ff78340b63e355da0cf13

instagibbs commented at 1:59 pm on June 10, 2016: member

utACK https://github.com/bitcoin/bitcoin/pull/8084/commits/3b3f45d532bd8f4c917ff78340b63e355da0cf13

gmaxwell commented at 10:21 am on June 15, 2016: contributor

Anything more needed here?

Add recently accepted blocks and txn to AttemptToEvictConnection.

This protects any not-already-protected peers who were the most
 recent four to relay transactions and most recent four to send
 blocks to us.

5d0ca81f74

Allow disconnecting a netgroup with only one member in eviction.

With the latest additions there are enough protective measures that
 we can take the training wheels off.

6ee7f05622

sdaftuar commented at 8:49 pm on June 15, 2016: member

utACK

sipa commented at 11:39 pm on June 15, 2016: member

utACK 6ee7f05622c32431a9815a96b31a6a65a821fdcc

pstratem commented at 0:35 am on June 16, 2016: contributor

ACK 6ee7f05622c32431a9815a96b31a6a65a821fdcc

laanwj merged this on Jun 16, 2016

laanwj closed this on Jun 16, 2016

laanwj referenced this in commit e4bb4a85a5 on Jun 16, 2016

MarcoFalke locked this on Sep 8, 2021

Add recently accepted blocks and txn to AttemptToEvictConnection. #8084