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

Several performance and privacy improvements to inv/mempool handling #7840

Merged
merged 5 commits into from May 5, 2016

Conversation

Projects
None yet
6 participants
@sipa
Member

sipa commented Apr 8, 2016

As we're increasingly changing how block invs and tx invs are treated, bite the bullet and separate them into two separate queues. This makes sense, as they are subject to very different behaviour: we want blocks to relay as fast as possible, while transactions are subject to rate limiting, deduplication, and source obscurity.

Once this is done, we can turn the tx queue into a set, and use it to prevent the mempool command from leaking unannounced transactions. The handling of such commands is also moved to the send loop, sorted by dependency order, and made subject to trickling.

Based on top of #7805.

@gmaxwell

View changes

Show outdated Hide outdated src/main.cpp
@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Apr 8, 2016

Member

This is a very good idea-- much better than my earlier ones around avoiding mempool requests breaking privacy.

Your commit message should explain that by eliminating queued entries from the mempool response and responding only at trickle time, this makes the mempool no longer leak transaction arrival order information (as the mempool itself is also sorted)-- at least no more than relay itself leaks it.

Member

gmaxwell commented Apr 8, 2016

This is a very good idea-- much better than my earlier ones around avoiding mempool requests breaking privacy.

Your commit message should explain that by eliminating queued entries from the mempool response and responding only at trickle time, this makes the mempool no longer leak transaction arrival order information (as the mempool itself is also sorted)-- at least no more than relay itself leaks it.

@gmaxwell

View changes

Show outdated Hide outdated src/main.cpp
@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Apr 9, 2016

Member

I have a test-harness now thats sutiable for testing this, a little 13 node network. Seems to work, it caught that the conversion to use the STL heap broke the behavior: the stl heap is a max heap.

Member

gmaxwell commented Apr 9, 2016

I have a test-harness now thats sutiable for testing this, a little 13 node network. Seems to work, it caught that the conversion to use the STL heap broke the behavior: the stl heap is a max heap.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Apr 9, 2016

Member

@thanks for fixing, looking good so far.

We still get a tiny number of orphans in a network the consists of nothing but this code. Here is one reason why node A sends you Tx1. You getdata Tx1 Node B sends you Tx1 and Tx2 and Tx1 is a child of Tx1. You getdata Tx2 from node2. Node2 responds faster. Tx2 is now an orphan.

Member

gmaxwell commented Apr 9, 2016

@thanks for fixing, looking good so far.

We still get a tiny number of orphans in a network the consists of nothing but this code. Here is one reason why node A sends you Tx1. You getdata Tx1 Node B sends you Tx1 and Tx2 and Tx1 is a child of Tx1. You getdata Tx2 from node2. Node2 responds faster. Tx2 is now an orphan.

@sipa sipa changed the title from Split and optimize inv queues and improve mempool privacy to Several performance and privacy improvements to inv/mempool handling Apr 10, 2016

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 10, 2016

Member

I added a commit to sort the response of mempool requests in dependency order, and renamed the PR to something more general.

Member

sipa commented Apr 10, 2016

I added a commit to sort the response of mempool requests in dependency order, and renamed the PR to something more general.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Apr 11, 2016

Contributor

concept ACK

Contributor

dcousens commented Apr 11, 2016

concept ACK

Show outdated Hide outdated src/main.cpp
Show outdated Hide outdated src/net.h
@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Apr 11, 2016

Member

Concept ACK, will test.

Member

sdaftuar commented Apr 11, 2016

Concept ACK, will test.

Show outdated Hide outdated src/main.h
@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 12, 2016

Member

Added two commits to address the comments.

Mempool requests are now not filtered by vInventoryTxToSend, but instead every mempool inv sent as a response is removed from vInventoryTxToSend.

Member

sipa commented Apr 12, 2016

Added two commits to address the comments.

Mempool requests are now not filtered by vInventoryTxToSend, but instead every mempool inv sent as a response is removed from vInventoryTxToSend.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Apr 12, 2016

Member

In a test topology over 48 hours this reduced the addition of orphan transactions from 436/hr to 2.3/hr.

Member

gmaxwell commented Apr 12, 2016

In a test topology over 48 hours this reduced the addition of orphan transactions from 436/hr to 2.3/hr.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Apr 12, 2016

Member

Also, utack latest improvements.

Member

gmaxwell commented Apr 12, 2016

Also, utack latest improvements.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Apr 13, 2016

Contributor

concept ACK, utACK 98305df

Contributor

dcousens commented Apr 13, 2016

concept ACK, utACK 98305df

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Apr 19, 2016

Member

@sipa: any squashing interest?

Member

gmaxwell commented Apr 19, 2016

@sipa: any squashing interest?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 19, 2016

Member

Rebased and squashed.

Member

sipa commented Apr 19, 2016

Rebased and squashed.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 19, 2016

Member

Also renamed the vInventoryTxToSend to setInventoryTxToSend, and restructured the tx send loop a bit to be less deeply nested.

Member

sipa commented Apr 19, 2016

Also renamed the vInventoryTxToSend to setInventoryTxToSend, and restructured the tx send loop a bit to be less deeply nested.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Apr 20, 2016

Member

Travis CI timeout, doesn't look like a real failure.

Member

gmaxwell commented Apr 20, 2016

Travis CI timeout, doesn't look like a real failure.

Eliminate TX trickle bypass, sort TX invs for privacy and priority.
Previously Bitcoin would send 1/4 of transactions out to all peers
 instantly.  This causes high overhead because it makes >80% of
 INVs size 1.  Doing so harms privacy, because it limits the
 amount of source obscurity a transaction can receive.

These randomized broadcasts also disobeyed transaction dependencies
 and required use of the orphan pool.  Because the orphan pool is
 so small this leads to poor propagation for dependent transactions.

When the bypass wasn't in effect, transactions were sent in the
 order they were received.  This avoided creating orphans but
 undermines privacy fairly significantly.

This commit:
 Eliminates the bypass. The bypass is replaced by halving the
  average delay for outbound peers.

 Sorts candidate transactions for INV by their topological
  depth then by their feerate (then hash); removing the
  information leakage and providing priority service to
  higher fee transactions.

 Limits the amount of transactions sent in a single INV to
  7tx/sec (and twice that for outbound); this limits the
  harm of low fee transaction floods, gives faster relay
  service to higher fee transactions. The 7 sounds lower
  than it really is because received advertisements need
  not be sent, and because the aggregate rate is multipled
  by the number of peers.
@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 20, 2016

Member

Added one more commit by @gmaxwell to move all inv filtering to the send stage (which avoids announcing things that are no longer in our mempool and bloom filter updates for those).

I'm sorry for the increasingly expansing scope this PR has had, so I'm not going to touch functionality any further unless bugs in it are found.

Member

sipa commented Apr 20, 2016

Added one more commit by @gmaxwell to move all inv filtering to the send stage (which avoids announcing things that are no longer in our mempool and bloom filter updates for those).

I'm sorry for the increasingly expansing scope this PR has had, so I'm not going to touch functionality any further unless bugs in it are found.

}
pto->filterInventoryKnown.insert(hash);
vInv.push_back(inv);
if (vInv.size() == MAX_INV_SZ) {

This comment has been minimized.

@jonasnick

jonasnick Apr 20, 2016

Contributor

It seems that in general it can happen that vInv.size() > MAX_INV_SZ, namely when pto->vInventoryBlockToSend.size() >= MAX_INV_SZ. Are we assuming that this isn't the case?

@jonasnick

jonasnick Apr 20, 2016

Contributor

It seems that in general it can happen that vInv.size() > MAX_INV_SZ, namely when pto->vInventoryBlockToSend.size() >= MAX_INV_SZ. Are we assuming that this isn't the case?

This comment has been minimized.

}
pto->filterInventoryKnown.insert(hash);
vInv.push_back(inv);
if (vInv.size() == MAX_INV_SZ) {

This comment has been minimized.

@sipa

sipa Apr 20, 2016

Member

Entries are added one by one to vInv, and all call sites where that happens are followed by a max size test, so I don't think vInv can exceed MAX_INV_SZ.

@sipa

sipa Apr 20, 2016

Member

Entries are added one by one to vInv, and all call sites where that happens are followed by a max size test, so I don't think vInv can exceed MAX_INV_SZ.

This comment has been minimized.

@sipa

sipa Apr 20, 2016

Member

Ah, good point. There may not be other mechanisms that prevent over 5000 blocks being inved at once.

@sipa

sipa Apr 20, 2016

Member

Ah, good point. There may not be other mechanisms that prevent over 5000 blocks being inved at once.

This comment has been minimized.

@sipa

sipa Apr 20, 2016

Member

Fixed.

@sipa

sipa Apr 20, 2016

Member

Fixed.

sipa added some commits Apr 7, 2016

Handle mempool requests in send loop, subject to trickle
By eliminating queued entries from the mempool response and responding only at
trickle time, this makes the mempool no longer leak transaction arrival order
information (as the mempool itself is also sorted)-- at least no more than
relay itself leaks it.

sipa and others added some commits Apr 10, 2016

Move bloom and feerate filtering to just prior to tx sending.
This will avoid sending more pointless INVs around updates, and
 prevents using filter updates to timetag transactions.

Also adds locking for fRelayTxes.
@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Apr 21, 2016

Contributor

@sipa could you update your original PR description with the full scope of this PR now? (for others)

Contributor

dcousens commented Apr 21, 2016

@sipa could you update your original PR description with the full scope of this PR now? (for others)

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Apr 21, 2016

Member

ACK.

Member

gmaxwell commented Apr 21, 2016

ACK.

@jonasnick

This comment has been minimized.

Show comment
Hide comment
@jonasnick

jonasnick Apr 22, 2016

Contributor

Ack

Contributor

jonasnick commented Apr 22, 2016

Ack

DepthAndScoreComparator(CTxMemPool *mempool) : mp(mempool) {}
bool operator()(const uint256& a, const uint256& b) { return mp->CompareDepthAndScore(a, b); }
};
}

This comment has been minimized.

@sdaftuar

sdaftuar Apr 22, 2016

Member

nit: What do you think of exposing the DepthAndScoreComparator defined here, and eliminating the CompareInvMempoolOrder object declared in main.cpp by moving the comparator it defines (which reverses the comparison) into this class?

@sdaftuar

sdaftuar Apr 22, 2016

Member

nit: What do you think of exposing the DepthAndScoreComparator defined here, and eliminating the CompareInvMempoolOrder object declared in main.cpp by moving the comparator it defines (which reverses the comparison) into this class?

This comment has been minimized.

@sipa

sipa Apr 22, 2016

Member

The comparator over there sorts iterators to set items, so there is more difference.

I do think the comparator (or at least a base comparator) should be exposed here, which grabs the mempool lock etc. Perhaps the other comparator can then take a ref to the first one as an argument to add the reversing and set iterator dereferencing.

@sipa

sipa Apr 22, 2016

Member

The comparator over there sorts iterators to set items, so there is more difference.

I do think the comparator (or at least a base comparator) should be exposed here, which grabs the mempool lock etc. Perhaps the other comparator can then take a ref to the first one as an argument to add the reversing and set iterator dereferencing.

This comment has been minimized.

@sdaftuar

sdaftuar Apr 22, 2016

Member

I was thinking that because the main.cpp comparator acts on different objects, it'd be easy to just move that function into this class (and explain in comments what the two comparators were for and why they are subtly different!), but your suggestion seems reasonable to me as well. I suppose its not awesome for the implementation details in main.cpp to leak into txmempool...

@sdaftuar

sdaftuar Apr 22, 2016

Member

I was thinking that because the main.cpp comparator acts on different objects, it'd be easy to just move that function into this class (and explain in comments what the two comparators were for and why they are subtly different!), but your suggestion seems reasonable to me as well. I suppose its not awesome for the implementation details in main.cpp to leak into txmempool...

This comment has been minimized.

@sipa

sipa Apr 22, 2016

Member
@sipa

sipa via email Apr 22, 2016

Member
@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Apr 22, 2016

Member

ACK, apart from my question about whether we should combine the two comparators that are being introduced.

Couple notes:

  • RelayTransaction no longer requires a feerate, and AcceptToMemoryPool no longer needs to fill a feerate in. We can clean this up in a future PR.
  • I noticed while testing that CTxMemPool::queryHashes is what we use in the RPC call getrawmempool. It's hard for me to imagine anyone would care (and in fact the new sort order on the results seems handy!), but perhaps we should flag the change in the release notes?
Member

sdaftuar commented Apr 22, 2016

ACK, apart from my question about whether we should combine the two comparators that are being introduced.

Couple notes:

  • RelayTransaction no longer requires a feerate, and AcceptToMemoryPool no longer needs to fill a feerate in. We can clean this up in a future PR.
  • I noticed while testing that CTxMemPool::queryHashes is what we use in the RPC call getrawmempool. It's hard for me to imagine anyone would care (and in fact the new sort order on the results seems handy!), but perhaps we should flag the change in the release notes?
@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Apr 26, 2016

Member

I've had this running in its various forms for two weeks on a couple of public nodes, as well as a seperate test topology that consisted of nothing but nodes running this code. What do we need to move this forward? I'm concerned that if I move to testing other PRs this will be forgotten.

Member

gmaxwell commented Apr 26, 2016

I've had this running in its various forms for two weeks on a couple of public nodes, as well as a seperate test topology that consisted of nothing but nodes running this code. What do we need to move this forward? I'm concerned that if I move to testing other PRs this will be forgotten.

@laanwj laanwj merged commit b559914 into bitcoin:master May 5, 2016

1 check passed

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

laanwj added a commit that referenced this pull request May 5, 2016

Merge #7840: Several performance and privacy improvements to inv/memp…
…ool handling

b559914 Move bloom and feerate filtering to just prior to tx sending. (Gregory Maxwell)
4578215 Return mempool queries in dependency order (Pieter Wuille)
ed70683 Handle mempool requests in send loop, subject to trickle (Pieter Wuille)
dc13dcd Split up and optimize transaction and block inv queues (Pieter Wuille)
f2d3ba7 Eliminate TX trickle bypass, sort TX invs for privacy and priority. (Gregory Maxwell)

@sdaftuar sdaftuar referenced this pull request May 16, 2016

Closed

TODO for release notes 0.13.0 #7678

14 of 16 tasks complete

@sipa sipa referenced this pull request Jun 13, 2016

Closed

Segregated witness #7910

5 of 7 tasks complete

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge #7840: Several performance and privacy improvements to inv/memp…
…ool handling

b559914 Move bloom and feerate filtering to just prior to tx sending. (Gregory Maxwell)
4578215 Return mempool queries in dependency order (Pieter Wuille)
ed70683 Handle mempool requests in send loop, subject to trickle (Pieter Wuille)
dc13dcd Split up and optimize transaction and block inv queues (Pieter Wuille)
f2d3ba7 Eliminate TX trickle bypass, sort TX invs for privacy and priority. (Gregory Maxwell)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #7840: Several performance and privacy improvements to inv/memp…
…ool handling

b559914 Move bloom and feerate filtering to just prior to tx sending. (Gregory Maxwell)
4578215 Return mempool queries in dependency order (Pieter Wuille)
ed70683 Handle mempool requests in send loop, subject to trickle (Pieter Wuille)
dc13dcd Split up and optimize transaction and block inv queues (Pieter Wuille)
f2d3ba7 Eliminate TX trickle bypass, sort TX invs for privacy and priority. (Gregory Maxwell)

codablock added a commit to codablock/dash that referenced this pull request Dec 21, 2017

Merge #7840: Several performance and privacy improvements to inv/memp…
…ool handling

b559914 Move bloom and feerate filtering to just prior to tx sending. (Gregory Maxwell)
4578215 Return mempool queries in dependency order (Pieter Wuille)
ed70683 Handle mempool requests in send loop, subject to trickle (Pieter Wuille)
dc13dcd Split up and optimize transaction and block inv queues (Pieter Wuille)
f2d3ba7 Eliminate TX trickle bypass, sort TX invs for privacy and priority. (Gregory Maxwell)

codablock added a commit to codablock/dash that referenced this pull request Dec 21, 2017

Send non-tx/non-block inventory items
Bitcoin #7840 has split the INVs to send into block and TX and completely
ignores non-tx/non-block items in PushInventory. This is fine for Bitcoin,
as they only use it for blocks and TXs, but we also have a lot of MN related
messages which also need to be relayed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment