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

Add recently accepted blocks and txn to AttemptToEvictConnection. #8084

Merged
merged 2 commits into from Jun 16, 2016

Conversation

Projects
None yet
@gmaxwell
Member

gmaxwell commented May 22, 2016

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.

if (vEvictionCandidates.empty()) return false;
// Protect 4 nodes that most recently sent us blocks.
// An attacker cannot manipulate this metric without performing useful work.

This comment has been minimized.

@wallclockbuilder
@wallclockbuilder

This comment has been minimized.

@rebroad

rebroad Aug 24, 2016

Contributor

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.

@rebroad

rebroad Aug 24, 2016

Contributor

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.

@sipa

View changes

Show outdated Hide outdated src/net.cpp

@laanwj laanwj added the P2P label May 22, 2016

@arowser

This comment has been minimized.

Show comment
Hide comment
@arowser

arowser May 25, 2016

Contributor

Can one of the admins verify this patch?

Contributor

arowser commented May 25, 2016

Can one of the admins verify this patch?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 26, 2016

Member

utACK e6d8aae

Member

sipa commented May 26, 2016

utACK e6d8aae

@sipa sipa referenced this pull request May 26, 2016

Merged

Addrman offline attempts #8065

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 31, 2016

Member

Needs rebase.

Member

sipa commented May 31, 2016

Needs rebase.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell May 31, 2016

Member

Rebased.

Member

gmaxwell commented May 31, 2016

Rebased.

@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Jun 9, 2016

Member

needs rebase.

Member

btcdrak commented Jun 9, 2016

needs rebase.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jun 9, 2016

Member

Rebased.

Member

gmaxwell commented Jun 9, 2016

Rebased.

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Jun 10, 2016

Contributor

@gmaxwell Can you split this into two commits?

Contributor

pstratem commented Jun 10, 2016

@gmaxwell Can you split this into two commits?

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jun 10, 2016

Member

No problem.

Member

gmaxwell commented Jun 10, 2016

No problem.

@pstratem

View changes

Show outdated Hide outdated src/net.cpp
@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Jun 10, 2016

Contributor

ACK 3b3f45d532bd8f4c917ff78340b63e355da0cf13

Contributor

pstratem commented Jun 10, 2016

ACK 3b3f45d532bd8f4c917ff78340b63e355da0cf13

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs
Member

instagibbs commented Jun 10, 2016

utACK 3b3f45d

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jun 15, 2016

Member

Anything more needed here?

Member

gmaxwell commented Jun 15, 2016

Anything more needed here?

gmaxwell added some commits May 22, 2016

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.
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.
@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jun 15, 2016

Member

utACK

Member

sdaftuar commented Jun 15, 2016

utACK

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 15, 2016

Member

utACK 6ee7f05

Member

sipa commented Jun 15, 2016

utACK 6ee7f05

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Jun 16, 2016

Contributor

ACK 6ee7f05

Contributor

pstratem commented Jun 16, 2016

ACK 6ee7f05

@laanwj laanwj merged commit 6ee7f05 into bitcoin:master Jun 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jun 16, 2016

Merge #8084: Add recently accepted blocks and txn to AttemptToEvictCo…
…nnection.

6ee7f05 Allow disconnecting a netgroup with only one member in eviction. (Gregory Maxwell)
5d0ca81 Add recently accepted blocks and txn to AttemptToEvictConnection. (Gregory Maxwell)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment