Skip to content

Conversation

gmaxwell
Copy link
Contributor

@gmaxwell gmaxwell commented Apr 4, 2016

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.

@laanwj
Copy link
Member

laanwj commented Apr 4, 2016

Concept ACK


bool operator()(const CInv &a, const CInv &b)
{
if (a.type != MSG_TX) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you verified that using this comparator does not change the order of blocks?

Alternatively, if both entries are blocks, perhaps they should be sorted by height?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably does change it (the sort isn't stable). I'll make it sort them by height... or I could split transactions off entirely.

@sipa
Copy link
Member

sipa commented Apr 4, 2016

@gmaxwell As long as we intend to support serving blocks to pre-0.10 (and especially 0.9 with the limited block orphan pools), we should not reorder block invs.

@theuni
Copy link
Member

theuni commented Apr 4, 2016

Concept ACK. Nit: looks like most of these functions should be const.

@laanwj laanwj added the P2P label Apr 5, 2016
@gmaxwell
Copy link
Contributor Author

gmaxwell commented Apr 5, 2016

Updated to make the sort stable and use static variables.

/** Average delay between trickled inventory transmissions in seconds.
* Blocks and whitelisted receivers bypass this, outbound peers get half this delay. */
static const unsigned int INVENTORY_BROADCAST_INTERVAL = 5;
/** Maximum number of inventor items to send per transmission.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inventor -> inventory

@sipa
Copy link
Member

sipa commented Apr 6, 2016

Concept ACK, going to test.

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.
@gmaxwell
Copy link
Contributor Author

gmaxwell commented Apr 6, 2016

I think it's updated for all comments now.

@sipa
Copy link
Member

sipa commented Apr 7, 2016

Midly-tested ACK using sipa@115157b to monitor the sizes of invs-to-send. 80% of the observed queues have less than 10 elements, 99.3% have less than 100, 99.9% have less than 1000. The highest observed was 2698. The average is 10.05 elements, with standard deviation 16.65.

@gmaxwell
Copy link
Contributor Author

gmaxwell commented Apr 8, 2016

Closing in favor of #7840.

@gmaxwell gmaxwell closed this Apr 8, 2016
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants