Dandelion cycle fix

[antiochp]

Related discussion #2176.

This plugs a recently discovered hole in our Dandelion implementation.

It is possible for any given stem path to contain a loop or cycle.

A-> B -> C -> ... -> A

In this situation a tx will propagate along the stem and A will see the same tx twice (or potentially an aggregated version of the original tx).

Currently we handle this by silently dropping the duplicate tx. But this results in the stem phase coming to a halt with no more tx propagation.
And the embargo timer eventually expires.

This PR changes this behavior to fall back to fluffing the duplicate tx if we have seen it (or any subset of it) before.

So in the previous example -
A-> B -> C -> ... -> A

If A sees Tx1, stems it and forwards it on to B, then subsequently sees (Tx1, Tx2) still in stem phase we identify this as a cycle in the stem path and fall back to fluffing (Tx1, Tx2).

Same thing would happen if A sees a duplicate Tx1 without any aggregation having taken place. In this case we simply fluff the unaggregated tx Tx1.

This way we benefit from any prior aggregation that may have occurred along the stem path. But we also handle the cycle in the stem path robustly without needing to rely on the embargo timer kicking in.

We suspect that the combination of a relatively small number of nodes in testnet4 with relatively high connectedness between nodes means we have a good chance of hitting a cycle in any given stem path, which would explain why we see a high rate of embargo timer expirations.

[quentinlesceller]

This definitely makes sense. Let's see if we see a diminution of embargo timer expiration with that.

[antiochp]

Guessing we need to PR this against floonet as well for this.
Pointed this PR at floonet branch.

[0xmichalis]

This definitely makes sense. Let's see if we see a diminution of embargo timer expiration with that.

@quentinlesceller isn't your concern in #2176 (comment) still a thing here?

[quentinlesceller]

@Kargakis Ah yes definitely... @antiochp mind holding on a bit until we put more thoughts in this?

[antiochp]

@quentinlesceller Sure thing.

I'm thinking its ok for 2 reasons -

1. the Dandelion++ paper talks about doing this in their impl

Our implementation enters fluff mode if there is a loop in the stem.

2. if the node seeing the duplicate tx fluffs the tx, no other node knows why it fluffed (cycle vs. change in epoch vs. embargo timer expiration vs. simply fluffing arbitrarily)

In our current impl we determine fluff vs stem per tx, so choosing to fluff a given tx is random.
We just force the decision (to fluff) in the presence of a cycle in the stem path.

[sesam]

if the node seeing the duplicate tx fluffs the tx, no other node knows why it fluffed (cycle vs. change in epoch vs. embargo timer expiration vs. simply fluffing arbitrarily)

Correct, should be safe iff we're waiting 0..10s both if the stemming-wanted tx is seen or not seen before.

Should we ping gfanti for an additional opinion?

[ignopeverell]

@quentinlesceller can you describe how one would try to detect transactions from your stempool with this? It seems you'd have to be in the stem already. Then prodding would only work once. And with the randomness in fluffing and stem changes, analysis would be rather tricky?

[0xmichalis]

If it's a real concern, it should be easy to identify that a peer is continuously sending you duplicate transactions.

…

On Sat, Dec 22, 2018, 20:02 Ignotus Peverell ***@***.*** wrote: @quentinlesceller <https://github.com/quentinlesceller> can you describe how one would try to detect transactions from your stempool with this? It seems you'd have to be in the stem already. Then prodding would only work once. And with the randomness in fluffing and stem changes, analysis would be rather tricky? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2185 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADuFf7PX3ovymCobcxa-qhe2ZN7yKeDCks5u7oHOgaJpZM4ZaTI8> .

[antiochp]

Where did we end up with this? @quentinlesceller you wanted some time to think through the implications here?

[quentinlesceller]

Hey sorry for the delay, after putting some thoughts and based on the fact this solution is also part of the Dandelion++ paper I think it's safe to merge.