Add 100-500ms of per-peer inventory delay

[jrick]

Similar to a recent dcrd change, we want published inventory by SPV wallet peers be submitted with a small random amount of delay. In this change we use the same logic that dcrd now uses to pick, for each connected peer, a random delay between 100-500ms before the inventory is sent.

[matthawkins90]

Something I noticed in decred/dcrd#3363 was that the address list was shuffled.

Should you also shuffle the address list here?

I know you said:

Currently, with every peer using a 500ms ticker, if no changes occur to the graph of connected nodes, messages will always propagate the nodes in the same order. However, if all nodes are randomizing their inventory batching delays, the next peer who will relay a message first during the next hop will always be different.

So it's possible that shuffling the address list is redundant, but I figured I'd ask if it was worth doing.

[matthawkins90]

What's the point of setting the variable timer here? It doesn't look like it's used, and you're already using timer.Reset(timeout()) below on line 526.

[matthawkins90]

Oh I'm sorry, I spoke too soon. You have to wait for timer.C, and then you reset it with a new randomized timeout(). I misread it as doubling-up the time-duration.

[jrick]

Something I noticed in decred/dcrd#3363 was that the address list was shuffled.

Should you also shuffle the address list here?

dcrwallet does not respond to getaddr. It only requests them.

[jrick]

or did you mean this, shuffling the inventory before we publish?

diff /home/jrick/src/dcrwallet
commit - b226415203f66ccaaad9d5e70bdb5adfa2e2e565
path + /home/jrick/src/dcrwallet
blob - 6e4effdee7b8a6306d5b60586d3a498b605aa6a6
file + p2p/peering.go
--- p2p/peering.go
+++ p2p/peering.go
@@ -542,6 +542,7 @@ func (rp *RemotePeer) sendWaitingInventory(ctx context
 	}
 
 	s := rp.waitingInvs
+	rand.ShuffleSlice(s)
 	for len(s) != 0 {
 		l := len(s)
 		if l > wire.MaxInvPerMsg {

[matthawkins90]

Yes, sorry. Shuffling the inventory. Just wondering if it was worth doing.

[jrick]

it's a good idea. we shouldn't need to depend on the order being dependency order, as both mempool and mixpool handle orphans.

[matthawkins90]

I like the changes to PublishTransactions and PublishMixMessages. A lot easier to read.

[davecgh]

it's a good idea. we shouldn't need to depend on the order being dependency order, as both mempool and mixpool handle orphans.

I actually disagree with this. In fact, it's a bad idea, imo. It's true that mempool handles orphans, but only a pretty limited number of them on purpose to prevent malicious actors being able to fill up memory without actually spending any real coins. It assumes that they are going to mostly be advertised in dependency order. Changing that would make it so malicious actors can hinder propagation by intentionally sending a bunch of junk to flush orphan buffers.