Keep track of recently rejected transactions with a rolling bloom filter (cont'd) #6498

Merged
merged 6 commits into from Aug 3, 2015

Conversation

Projects
None yet
4 participants
@laanwj
Member

laanwj commented Jul 31, 2015

Peter Todd is on holiday, so continued from #6452
Added a commit that moves recentRejects initialization to the top of InitBlockIndex to avoid not initializing it under some conditions.

petertodd and others added some commits Jul 17, 2015

Make CRollingBloomFilter set nTweak for you
While CBloomFilter is usually used with an explicitly set nTweak,
CRollingBloomFilter is only used internally. Requiring every caller to
set nTweak is error-prone and redundant; better to have the class handle
that for you with a high-quality randomness source.

Additionally when clearing the filter it makes sense to change nTweak as
well to recover from a bad setting, e.g. due to insufficient randomness
at initialization, so the clear() method is replaced by a reset() method
that sets a new, random, nTweak value.
Keep track of recently rejected transactions
Nodes can have divergent policies on which transactions they will accept
and relay.  This can cause you to repeatedly request and reject the same
tx after its inved to you from various peers which have accepted it.
Here we add rolling bloom filter to keep track of such rejections,
clearing the filter every time the chain tip changes.

Credit goes to Alex Morcos, who created the patch that this code is
based on.

Original code by Peter Todd. Refactored to not construct the
filter at startup time by Pieter Wuille.
Move recentRejects initialization to top of InitBlockIndex
This avoids that premature return in the condition that a new chain is initialized
results in NULL pointer errors due to recentReject not being constructed.

Also add assertions where it is used.
@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Aug 3, 2015

Member

ACK (tested as part of #6470).

Member

sipa commented Aug 3, 2015

ACK (tested as part of #6470).

@laanwj laanwj merged commit a8d0407 into bitcoin:master Aug 3, 2015

1 check passed

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

laanwj added a commit that referenced this pull request Aug 3, 2015

Merge pull request #6498
a8d0407 Move recentRejects initialization to top of InitBlockIndex (Wladimir J. van der Laan)
0847d9c Keep track of recently rejected transactions (Peter Todd)
d741371 Only use randomly created nonces in CRollingBloomFilter. (Pieter Wuille)
d2d7ee0 Make CRollingBloomFilter set nTweak for you (Peter Todd)
a3d65fe Reuse vector hashing code for uint256 (Pieter Wuille)
bbe4108 Add uint256 support to CRollingBloomFilter (Peter Todd)

@morcos morcos referenced this pull request Aug 5, 2015

Closed

Super Duper MemPool Limiter #6470

@dgenr8 dgenr8 referenced this pull request in bitcoinxt/bitcoinxt Aug 25, 2015

Merged

Track recently rejected transactions; don't reprocess #50

+ * peers, half of which relay a tx we don't accept, that might be a 50x
+ * bandwidth increase. A flooding attacker attempting to roll-over the
+ * filter using minimum-sized, 60byte, transactions might manage to send
+ * 1000/sec if we have fast peers, so we pick 120,000 to give our peers a

This comment has been minimized.

@dcousens

dcousens Oct 4, 2015

Contributor

Pick 120,000 what?

@dcousens

dcousens Oct 4, 2015

Contributor

Pick 120,000 what?

+ *
+ * Decreasing the false positive rate is fairly cheap, so we pick one in a
+ * million to make it highly unlikely for users to have issues with this
+ * filter.

This comment has been minimized.

@dcousens

dcousens Oct 4, 2015

Contributor

Decreasing the false positive rate [for a valid transaction?] is fairly cheap, so we pick one in [a] million [1/1e6, what is this unit?] to make it highly unlikely for users [legitimate transactions?] to have issues with this [passing this?] filter.

Could this be clarified a little?
I marked my assumptions/questions in [ ].

edit: if I can get clarifications to the above to cement my own understanding, I'll happily make the change/PR

@dcousens

dcousens Oct 4, 2015

Contributor

Decreasing the false positive rate [for a valid transaction?] is fairly cheap, so we pick one in [a] million [1/1e6, what is this unit?] to make it highly unlikely for users [legitimate transactions?] to have issues with this [passing this?] filter.

Could this be clarified a little?
I marked my assumptions/questions in [ ].

edit: if I can get clarifications to the above to cement my own understanding, I'll happily make the change/PR

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Oct 4, 2015

Contributor

concept ACK

Contributor

dcousens commented Oct 4, 2015

concept ACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment