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

doc: add comment explaining recentRejects-DoS behavior #14436

Merged
merged 1 commit into from Nov 7, 2018

Conversation

Projects
None yet
5 participants
@jamesob
Copy link
Member

commented Oct 9, 2018

When we receive invalid txs for the first time, we mark the sender as
misbehaving. If we receive the same tx before a new block is seen, we don't
punish the second sender (in the same way we do the original sender). It wasn't
initially clear to me that this is intentional, so add a clarifying comment.

@fanquake fanquake added the Docs label Oct 9, 2018

@jamesob jamesob force-pushed the jamesob:2018-10-recentrejects-doc branch Oct 9, 2018

@fanquake fanquake requested a review from sdaftuar Oct 10, 2018

@conscott

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2018

Concept ACK

doc: add comment explaining recentRejects-DoS behavior
When we receive invalid txs for the first time, we mark the sender as
misbehaving. If we receive the same tx before a new block is seen, we *don't*
punish the second sender (in the same way we do the original sender). It wasn't
initially clear to me that this is intentional, so add a clarifying comment.

@jamesob jamesob force-pushed the jamesob:2018-10-recentrejects-doc branch to b191c7d Oct 16, 2018

@conscott

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

Re-ACK b191c7d

@sipa

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

ACK

// tx (even if we penalized the first peer who gave it to us) because
// we have to account for recentRejects showing false positives. In
// other words, we shouldn't penalize a peer if we aren't *sure* they
// submitted a DoSy tx.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Nov 7, 2018

Member

It sounds a bit like we want to penalize subsequent peers if we could. Though, I think this is not true, since we have not experienced any further damage by this "malicious" action (compared to lets say a "honest" relay of a transaction that we already have).

So if we hit recentRejects, we are not interested in the DoS score because it would be too expensive to calculate for almost no gain (could detect rare false positives).

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Nov 7, 2018

ACK b191c7d

ken2812221 pushed a commit to ken2812221/bitcoin that referenced this pull request Nov 7, 2018

Merge bitcoin#14436: doc: add comment explaining recentRejects-DoS be…
…havior

b191c7d doc: add comment explaining recentRejects-DoS behavior (James O'Beirne)

Pull request description:

  When we receive invalid txs for the first time, we mark the sender as
  misbehaving. If we receive the same tx before a new block is seen, we *don't*
  punish the second sender (in the same way we do the original sender). It wasn't
  initially clear to me that this is intentional, so add a clarifying comment.

Tree-SHA512: d12c674db137ed3ad83e0b941bffe6ddcd2982238048742afa574a4235881f0e58cfc0a4a576a0503e74c5c5240c270b9520fa30221e8b43a371fb3e0b37066b

@MarcoFalke MarcoFalke merged commit b191c7d into bitcoin:master Nov 7, 2018

2 checks passed

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

@jamesob jamesob deleted the jamesob:2018-10-recentrejects-doc branch Nov 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.