Dandelion transaction relay (BIP 156)

MarcoFalke commented at 2:43 pm on August 12, 2018: member

The implementation of BIP 156 (Dandelion transaction relay) is done in three steps:

Add the protocol messages

dandeliontx, which contains a transaction serialized with all witness data
dandelionacc, which is always empty and only used to signal dandelion support to other peers

Shuffle dandelion stems

We shuffle a list of all peers that sent us the message that they accept dandelion txs (dandelionacc). This list can be used on demand by nodes that want to forward a dandelion transaction. (Actual receiving of dandelion txs is done in the next step).

Also, we add a command line option -dandelion, which can be used to opt out of all dandelion features.

Keep a cache of recent dandelion transactions

Dandelion transactions are never added to the global tx pool and only forwarded to one peer (the next hop in the stem). To guard against the next peer going offline, we keep a cache of recent dandelion transactions that are not yet fluffed (added to the mempool) or mined. Entries in this cache expire after some random timeout and transition to fluff phase.

protocol: Add BIP 156 message types 8c27c3b6bf

MarcoFalke commented at 2:49 pm on August 12, 2018: member

This is completely untested, so TODO:

add tests
use dandelion relay for our wallet txs

in src/net.h:800 in fb55529f30 outdated

777@@ -755,6 +778,10 @@ class CNode
778     // Our address, as reported by the peer
779     CService addrLocal;
780     mutable CCriticalSection cs_addrLocal;
781+
782+    /** Our current dandelion destination */
783+    CConnman::DandelionPeer* m_dandelion_destination /* GUARDED_BY(CConnman::cs_dandelion_peers) */ {nullptr};

practicalswift commented at 2:50 pm on August 12, 2018:

Any drawbacks from uncommenting the GUARDED_BY(…) annotation?

MarcoFalke commented at 3:05 pm on August 12, 2018:

The drawback was that it didn’t compile for me

MarcoFalke added the label P2P on Aug 12, 2018

MarcoFalke added this to the milestone 0.18.0 on Aug 12, 2018

MarcoFalke added the label Feature on Aug 12, 2018

in src/init.cpp:1036 in 615b7dc0b0 outdated

1030@@ -1027,6 +1031,12 @@ bool AppInitParameterInteraction()
1031         LogPrintf("Warning: nMinimumChainWork set below default value of %s\n", chainparams.GetConsensus().nMinimumChainWork.GetHex());
1032     }
1033 
1034+    {
1035+        const int64_t pct = gArgs.GetArg("-dandelion", DEFAULT_DANDELION_STEM_PERCENTAGE);
1036+        if (pct < 0 || 100 < pct) return InitError(_("-dandelion must be 0 or a percentage less than 100"));

practicalswift commented at 3:08 pm on August 12, 2018:

Should -dandelion=100 generate an error message? From the error message it looks like it should, but not from the logic :-)

MarcoFalke commented at 3:10 pm on August 12, 2018:

100% is allowed to always extend the stem by one hop. So the interval is [0, 100].

practicalswift commented at 7:42 am on August 13, 2018:

Then the error message should be -dandelion must be 0 or a percentage up to 100 or something equivalent?

DrahtBot commented at 5:02 pm on August 12, 2018: member

#14032 (Add p2p layer encryption with ECDH/ChaCha20Poly1305 by jonasschnelli)
#13946 (p2p: Clarify control flow in ProcessMessage by MarcoFalke)
#13743 (refactor: Replace std/boost::bind with lambda by ken2812221)
#13123 (net: Add Clang thread safety annotations for guarded variables in the networking code by practicalswift)
#10102 ([experimental] Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in doc/bips.md:36 in 615b7dc0b0 outdated

32@@ -33,6 +33,7 @@ BIPs that are implemented by Bitcoin Core (up-to-date up to **v0.17.0**):
33 * [`BIP 145`](https://github.com/bitcoin/bips/blob/master/bip-0145.mediawiki): getblocktemplate updates for Segregated Witness as of **v0.13.0** ([PR 8149](https://github.com/bitcoin/bitcoin/pull/8149)).
34 * [`BIP 147`](https://github.com/bitcoin/bips/blob/master/bip-0147.mediawiki): NULLDUMMY softfork as of **v0.13.1** ([PR 8636](https://github.com/bitcoin/bitcoin/pull/8636) and [PR 8937](https://github.com/bitcoin/bitcoin/pull/8937)).
35 * [`BIP 152`](https://github.com/bitcoin/bips/blob/master/bip-0152.mediawiki): Compact block transfer and related optimizations are used as of **v0.13.0** ([PR 8068](https://github.com/bitcoin/bitcoin/pull/8068)).
36+* [`BIP 156`](https://github.com/bitcoin/bips/blob/master/bip-0156.mediawiki): Dandelion transaction relay is supported as of **v0.18.0** ([PR 13947](https://github.com/bitcoin/bitcoin/pull/13947)).

clodhopp commented at 6:44 pm on August 12, 2018:

Link https://github.com/bitcoin/bips/blob/master/bip-0156.mediawiki gives error 404 page not found.

meshcollider commented at 7:38 am on August 13, 2018:

@clodhopp that’s because the BIP PR hasn’t been merged, you can see it here: https://github.com/bitcoin/bips/pull/703 Once merged, the link should work

sipa commented at 6:57 pm on August 12, 2018: member

This code doesn’t seem to include a “stempool”; how does it deal with dandelion transactions that depend on other dandelion transactions? ATMP will fail for those.

MarcoFalke commented at 8:02 pm on August 12, 2018: member

This code doesn’t seem to include a “stempool”; how does it deal with dandelion transactions that depend on other dandelion transactions? ATMP will fail for those.

Continued here https://botbot.me/freenode/bitcoin-core-dev/2018-08-12/?msg=103212266&page=2 EDIT by ajtowns: http://www.erisian.com.au/bitcoin-core-dev/log-2018-08-12.html#l-190

in src/init.cpp:801 in fb55529f30 outdated

793@@ -793,10 +794,13 @@ void InitParameterInteraction()
794             LogPrintf("%s: parameter interaction: -externalip set -> setting -discover=0\n", __func__);
795     }
796 
797-    // disable whitelistrelay in blocksonly mode
798     if (gArgs.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY)) {
799+        // disable whitelistrelay and dandelion in blocksonly mode
800         if (gArgs.SoftSetBoolArg("-whitelistrelay", false))
801             LogPrintf("%s: parameter interaction: -blocksonly=1 -> setting -whitelistrelay=0\n", __func__);
802+        if (gArgs.SoftSetBoolArg("-dandelion", false)) {

sdaftuar commented at 3:35 pm on August 13, 2018:

Do we use SoftSetBoolArg() like this elsewhere, on arguments that take numeric values? It was not clear to me that this worked until I saw that SoftSetBoolArg converts the false to std::string("0"). (Neat, though.)

stevenroose commented at 1:10 pm on August 20, 2018:

Just to not mix up values, this should set 0 instead of false, even if false would be converted automatically to 0.

in src/net.h:851 in 615b7dc0b0 outdated

846@@ -803,7 +847,19 @@ class CNode
847         nRefCount--;
848     }
849 
850+    /** If this peer is not yet connected or soon to be disconnected */
851+    bool IsEphermal() const

sdaftuar commented at 7:03 pm on August 13, 2018:

nit: IsEphemeral

in src/net_processing.cpp:2257 in 615b7dc0b0 outdated

2250@@ -2187,6 +2251,67 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
2251     }
2252 
2253 
2254+    else if (strCommand == NetMsgType::TX_DANDELION) {
2255+        // Stop processing the transaction early if
2256+        // dandelion is disabled and peer is either not whitelisted or whitelistrelay is off
2257+        if (!CNode::IsDandelionEnabled()&& (!pfrom->fWhitelisted || !gArgs.GetBoolArg("-whitelistrelay", DEFAULT_WHITELISTRELAY))) {

sdaftuar commented at 7:07 pm on August 13, 2018:

Some IRC discussion around not extending whitelist-relay behavior further with dandelion processing: https://botbot.me/freenode/bitcoin-core-dev/2018-08-13/?msg=103271568&page=3

in src/net_processing.cpp:2289 in 615b7dc0b0 outdated

2284+        if (missing_inputs) {
2285+            LogPrint(BCLog::DANDELION, "missing inputs\n");
2286+            return true;
2287+        }
2288+
2289+        if (pfrom->fWhitelisted && gArgs.GetBoolArg("-whitelistforcerelay", DEFAULT_WHITELISTFORCERELAY)) {

sdaftuar commented at 7:11 pm on August 13, 2018:

See above comment about dropping this behavior for whitelisted peers.

in src/protocol.h:127 in 8c27c3b6bf outdated

119@@ -120,6 +120,11 @@ extern const char *GETHEADERS;
120  * @see https://bitcoin.org/en/developer-reference#tx
121  */
122 extern const char *TX;
123+/**
124+ * The dandelion tx message transmits a single transaction.
125+ * @see https://github.com/bitcoin/bips/blob/master/bip-0156.mediawiki
126+ */
127+extern const char* TX_DANDELION;

sdaftuar commented at 7:23 pm on August 13, 2018:

I think in the past, we’ve usually bumped the protocol version when introducing new p2p messages so that we only send the new messages (like ACCEPT_DANDELION) to peers that are signaling that they’ve upgraded. Not sure how important that is…

MarcoFalke commented at 8:09 pm on August 13, 2018:

Note that we’d never send a TX_DANDELION to a peer that never sent us ACCEPT_DANDELION in the first place. So I don’t think this is an issue at all.

sdaftuar commented at 4:32 pm on August 14, 2018:

Perhaps my comment was not placed on the right line of this file; I was thinking of the dandelionacc message. With sendheaders and I think compact blocks, I believe we bumped the protocol version and only tried to send the new p2p messages (even for handshaking/negotiating the protocol to use) to peers with that higher version?

MarcoFalke commented at 4:55 pm on August 14, 2018:

Good point. I think it is a lot easier to do this without enforcing a global network version bump.

Note that the dandelionacc message is properly read by all nodes that implement dandelion (regardless of their network protocol version) or choose to ignore that message. And for all other nodes (that are completely unaware of dandelion) they simply treat this as unknown message (once per connection) in their logs. I think this one debug log is not enough reason to go through the hassle of bumping the protocol version.

https://github.com/bitcoin/bitcoin/blob/63f8b0128b2aac3b25c6ec4d2f5bda213033162a/src/net_processing.cpp#L2942

stevenroose commented at 7:37 pm on August 20, 2018:

Why not a service bit?

sdaftuar commented at 7:28 pm on August 13, 2018: member

I just left some initial comments from a quick-read of the code. As discussed on IRC we still need to figure out what the right behavior is for transaction chains and replacement transactions (and combinations of the two), and generally how to prevent DoS attacks during stem routing.

If we use a stempool to address those issues, then I’m not sure I understand how we could share a stempool across multiple inbound peers without leaking dandelion routing information to an adversary (and if we have separate stempools for each inbound peer, then that sounds like a lot of overhead).

in src/net_processing.cpp:2280 in f8386167e2 outdated

2276+        }
2277+
2278+        CValidationState state;
2279+        bool missing_inputs;
2280+        if (AcceptToMemoryPool(mempool, state, tx, &missing_inputs, nullptr /* lRemovedTxn */, false /* bypass_limits */, 0 /* nAbsurdFee */, true /* test_accept */)) {
2281+            RelayDandelionTransaction(mempool, pfrom, tx, msgMaker, connman);

sdaftuar commented at 7:25 pm on August 14, 2018:

I think we should queue transactions for relay to a peer, rather than doing a direct send as is done here. Otherwise transaction relay can fill up our send buffers and delay block relay.

Further our current transaction relay system batches transactions for relay and, at each broadcast time, we sort the to-be-relayed transactions for the purposes of prioritising transactions that have fewer ancestors and higher feerate. I think being able to prioritise transactions with higher feerate for relay is probably useful (to prevent low-feerate transactions from harming propagation times of higher feerate transactions).

gmaxwell commented at 6:55 am on August 15, 2018:

it’s also better for privacy against traffic analysis to batch things.

logging: Add BCLog::DANDELION 3c29217796

in src/net_processing.cpp:1973 in 615b7dc0b0 outdated

1966@@ -1908,6 +1967,11 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
1967         State(pfrom->GetId())->fPreferHeaders = true;
1968     }
1969 
1970+    else if (strCommand == NetMsgType::ACCEPT_DANDELION) {
1971+        pfrom->m_accept_dandelion = true;
1972+        return true;

MarcoFalke commented at 9:10 pm on August 14, 2018:

Should write something to debug log here.

MarcoFalke force-pushed on Aug 14, 2018

doc: Dandelion transaction relay (BIP 156) c5aba5c44e

MarcoFalke force-pushed on Aug 15, 2018

MarcoFalke force-pushed on Aug 17, 2018

in src/init.cpp:402 in b508ed10f5 outdated

398@@ -399,6 +399,7 @@ void SetupServerArgs()
399     gArgs.AddArg("-bantime=<n>", strprintf("Number of seconds to keep misbehaving peers from reconnecting (default: %u)", DEFAULT_MISBEHAVING_BANTIME), false, OptionsCategory::CONNECTION);
400     gArgs.AddArg("-bind=<addr>", "Bind to given address and always listen on it. Use [host]:port notation for IPv6", false, OptionsCategory::CONNECTION);
401     gArgs.AddArg("-connect=<ip>", "Connect only to the specified node; -connect=0 disables automatic connections (the rules for this peer are the same as for -addnode). This option can be specified multiple times to connect to multiple nodes.", false, OptionsCategory::CONNECTION);
402+    gArgs.AddArg("-dandelion", strprintf("Probability to extend the stem in Dandelion transaction relay by one hop (default: %d, 0 to disable dandelion)", DEFAULT_DANDELION_STEM_PERCENTAGE), false, OptionsCategory::CONNECTION);

stevenroose commented at 1:09 pm on August 20, 2018:

A simple “probability” should a number between 0 and 1. The variable used as default and the code below suggest a percentage is used instead. I think it would make sense to document it as such as well.

practicalswift commented at 11:23 pm on August 20, 2018:

I was thinking about this as well: normally I’d expect a probability parameter to be in the interval [0.0, 1.0]. Are we trying to be consistent with other parameters taking probabilities as [0, 100] here, or what is the rationale? :-)

stevenroose commented at 2:57 pm on October 3, 2018:

If that is the case, it should at least be mentioned in the help line.

Probability in % to extend ..

in src/net.cpp:1761 in b508ed10f5 outdated

1753@@ -1741,6 +1754,80 @@ int CConnman::GetExtraOutboundCount()
1754     return std::max(nOutbound - nMaxOutbound, 0);
1755 }
1756 
1757+void CConnman::AssignNewDandelionDestination(CNode* from)
1758+{
1759+    from->m_dandelion_destination = nullptr;
1760+
1761+    if (!CNode::IsDandelionEnabled()) return;

stevenroose commented at 1:37 pm on August 20, 2018:

This check should not be necessary, failing it means there’s a bug. I’d suggest either using an assertion or something more severe than this or just removing the check.

in src/net.h:330 in b508ed10f5 outdated

326@@ -317,6 +327,16 @@ class CConnman
327     */
328     int64_t PoissonNextSendInbound(int64_t now, int average_interval_seconds);
329 
330+    using DandelionPeer = std::pair<CNode*, /* use count */ unsigned>;

stevenroose commented at 2:15 pm on August 20, 2018:

Please elaborate a bit more on the documentation of the integer.

AM5800 commented at 7:52 am on October 25, 2018:

Maybe it is better to create a struct? This integer will look much better with a name. It is very hard to read all those peer.first and peer.second

in src/net.h:51 in b508ed10f5 outdated

46 /** Time after which to disconnect, after waiting for a ping response (or inactivity). */
47 static const int TIMEOUT_INTERVAL = 20 * 60;
48 /** Run the feeler connection loop once every 2 minutes or 120 seconds. **/
49 static const int FEELER_INTERVAL = 120;
50+/** Pick new dandelion peers once every 10 minutes or 600 seconds. */
51+static constexpr int INTERVAL_DANDELION = 600;

stevenroose commented at 3:18 pm on August 20, 2018:

The convention seems to be XXX_INTERVAL instead of INTERVAL_XXX.

stevenroose commented at 6:50 pm on August 20, 2018:

Also, why 10 minutes?

Sjors commented at 11:29 am on September 2, 2018:

@stevenroose as per the BIP: https://github.com/bitcoin/bips/blob/master/bip-0156.mediawiki#periodic-dandelion-route-shuffling (the value suggested in the BIP is best discussed on the bitcoin-dev mailinglist)

stevenroose commented at 9:01 am on September 4, 2018:

@Sjors this would do: /** As per BIP156, pick new dandelion peers once every 10 minutes or 600 seconds. */

in src/net.cpp:1811 in b508ed10f5 outdated

1806+                    // Ignore node->fRelayTxes here, to allow for dandelion-only nodes
1807+                    node->m_accept_dandelion;
1808+                bool weak_dandelion = //
1809+                    node->fRelayTxes /* ignore peers that reject normal txs */ &&
1810+                    !node->fClient /* ignore peers that don't serve blocks */ &&
1811+                    !node->nLastTXTime /* ignore peers that never sent us any tx */;

stevenroose commented at 6:10 pm on August 20, 2018:

First, I think this test should be performed as well for Dandelion advertising peers, because they could otherwise deny you service. Furthermore, I think it might be more useful to have a threshold time of last relay instead of just one. Like “relayed tx in last minute”.

With this, a peer could advertise dandelion and never relay anything.

in src/net.cpp:1758 in b508ed10f5 outdated

1758+{
1759+    from->m_dandelion_destination = nullptr;
1760+
1761+    if (!CNode::IsDandelionEnabled()) return;
1762+
1763+    unsigned min_use{std::numeric_limits<unsigned>::max()};

stevenroose commented at 6:30 pm on August 20, 2018:

Please explain how this variable is used and why. I fail to see its function.

Also since peers are checked with min_use > peer.second, this is an upper limit, not a lower limit. Making the name min_use not very telling.

in src/net.cpp:1756 in b508ed10f5 outdated

1753@@ -1741,6 +1754,80 @@ int CConnman::GetExtraOutboundCount()
1754     return std::max(nOutbound - nMaxOutbound, 0);
1755 }
1756 
1757+void CConnman::AssignNewDandelionDestination(CNode* from)

stevenroose commented at 6:40 pm on August 20, 2018:

I think this method could use some documentation on the strategy/algorithm used to assign destinations.

in src/net.h:43 in b508ed10f5 outdated

35@@ -36,13 +36,19 @@
36 
37 class CScheduler;
38 class CNode;
39+class CTransaction;
40+using CTransactionRef = std::shared_ptr<const CTransaction>;
41 
42+/** Default for -dandelion stem percentage */
43+static constexpr int64_t DEFAULT_DANDELION_STEM_PERCENTAGE = 90;

stevenroose commented at 6:50 pm on August 20, 2018:

Why 90? This is like the most important parameter of the dandelion algorithm, I think it deserves some motivation..

Sjors commented at 11:28 am on September 2, 2018:

@stevenroose the BIP specifies that:

When relaying a Dandelion transaction along a Dandelion route, there is a 10% chance that the Dandelion transaction becomes a typical Bitcoin transaction and is therefore relayed via diffusion.

stevenroose commented at 8:59 am on September 4, 2018:

@Sjors So I’d suggest referring to the BIP in the comment of that variable.

stevenroose commented at 9:04 am on September 4, 2018:

What I’m trying to say with this comments is that this is supposed to be an implementation of the BIP, so it should refer to the BIP for logic/params etc. It has happened too often that a BIP is just a description of the implementation.

in src/net_processing.cpp:1078 in b508ed10f5 outdated

1075     });
1076 }
1077 
1078+static void FluffTransaction(CTxMemPool& tx_pool, const CNode* from, const CTransactionRef& ptx, CConnman* connman)
1079+{
1080+    LogPrint(BCLog::DANDELION, "fluff\n");

stevenroose commented at 7:06 pm on August 20, 2018:

Perhaps mention txid?

in src/net_processing.cpp:1096 in b508ed10f5 outdated

1093+}
1094+
1095+static void RelayDandelionTransaction(CTxMemPool& tx_pool, CNode* node_from, const CTransactionRef& ptx, const CNetMsgMaker& msg_maker, CConnman* connman)
1096+{
1097+    bool fluff = GetRand(100 * 100) / 100. >= CNode::m_dandelion_stem_pct_threshold;
1098+    if (fluff) return FluffTransaction(tx_pool, node_from, ptx, connman);

stevenroose commented at 7:09 pm on August 20, 2018:

Should this check also be made for the wallet’s own transactions? Or should a forced non-fluff be done in that case?

in src/net_processing.cpp:1124 in b508ed10f5 outdated

1121+        connman->PushMessage(dest, msg_maker.Make(DANDELION_SEND_FLAGS, NetMsgType::TX, *ptx));
1122+        return;
1123+        }
1124+
1125+        LogPrint(BCLog::DANDELION, "dandelion relay to peer=%d\n", dest->GetId());
1126+        connman->PushMessage(dest, msg_maker.Make(DANDELION_SEND_FLAGS, NetMsgType::TX_DANDELION, *ptx));

stevenroose commented at 7:15 pm on August 20, 2018:

Wrong indentation since after return.

stevenroose changes_requested

MarcoFalke force-pushed on Aug 21, 2018

net: Add thread to shuffle potential dandelion stems bbe290e2a4

p2p: Keep cache of recent Dandelion transactions for stem expiry 8b533b25fd

MarcoFalke force-pushed on Aug 21, 2018

DrahtBot commented at 4:40 pm on August 25, 2018: member

DrahtBot added the label Needs rebase on Aug 25, 2018

Sjors commented at 12:10 pm on September 2, 2018: member

I don’t know how to test this without an (experimental) RPC method to broadcast a Dandelion transaction. It does compile on macOS, seems to recognize other peers with this feature and occasionally logs “Dandelion: Shuffle Dandelion Destinations”.

Same question as @stevenroose: why a dandelionacc message rather than a service bit? If we use a service bit, then it would be nice to have an updated version of the DNS Seeder, so it’s easier test this in the wild. I’d be happy to update my testnet seed for that.

Neither dandeliontx or dandelionacc are mentioned and/or specified in the BIP. Is that because the BIP is still work in progress?

Needs functional tests.

Maybe add a dandelion bool field to getpeerinfo?

scravy commented at 8:22 am on September 19, 2018: contributor

Shouldn’t the functional test from the reference implementation (https://github.com/dandelion-org/bitcoin/commit/d043e36bbe9249a78cf751c80b8d876b7d9f07ea#diff-21ab9447686d819eeeb7668ce8011e0d) run more-or-less unchanged on top of this branch?

in src/net.cpp:1801 in 8b533b25fd

1796+
1797+            // Collect all potential dandelion nodes
1798+            m_nodes_dandelion.clear();
1799+            for (CNode* node : all_nodes) {
1800+                // Ignore inbound nodes
1801+                if (node->fInbound) continue;

rodasmith commented at 11:10 pm on September 30, 2018:

Why ignore inbound nodes? Does this mean non-listening dandelion nodes send only their own transactions and that the peer who receives such will know with undeniable certainty that the non-listening node initiated the transaction?

instagibbs commented at 1:27 am on October 3, 2018:

Inbound nodes are much more likely to be snooping peers.

kek-coin commented at 6:15 am on October 3, 2018:

While that is true, aren’t there solutions that do not make dandelion break firewalled nodes’ privacy? For example, send only to outgoing peers if you originated the transaction, but send to incoming as well for forwarded txes? Perhaps weigh the outgoing peers more heavily than incoming peers when selecting a random peer to forward to (either by assigning a weight to individual peers based on incoming/outgoing status, or by first randomly selecting the outgoing or incoming pool with a fixed weighting before selecting a peer from whichever pool).

jb55 commented at 0:08 am on October 3, 2018: member

These commits could be documented more in general, I’m finding it hard to review when there are no descriptions about what is being implemented.

AM5800 commented at 12:01 pm on October 9, 2018: none

It has been mentioned several times that common stempool might leak dandelion routing information to an adversary. Can someone help me understand how exactly?

Also, what is the current status of this issue? I would like to offer some help. For example with tests

scravy commented at 12:30 pm on October 9, 2018: contributor

@jb55 BIP156 should explain a lot of things.

jb55 commented at 4:24 pm on October 9, 2018: member

@scravy I’m more interested in documentation of the implementation itself, how each commit relates to a part of the bip, etc. It’s much harder to verify an implementation without that, especially with important privacy/security features. I don’t think we should be making it harder on reviewers than it needs to be.

stevenroose commented at 5:30 pm on October 9, 2018: contributor

@jb55 Totally agree. All the non-trivial pieces of code should explain themselves, refer to the BIP when relevant, etc..

in src/net_processing.cpp:1076 in 8b533b25fd

1073         pnode->PushInventory(inv);
1074+        pnode->m_cache_dandelion.erase(inv.hash);
1075     });
1076 }
1077 
1078+static void FluffTransaction(CTxMemPool& tx_pool, const CNode* from, const CTransactionRef& ptx, CConnman* connman)

AM5800 commented at 7:57 am on October 25, 2018:

I was wandering… FluffTransaction is logically equivalent to receiving a transaction, right? Then shouldn’t we also execute all this code on fluff?

in src/net.cpp:2463 in 8b533b25fd

2459         threadOpenConnections = std::thread(&TraceThread<std::function<void()> >, "opencon", std::function<void()>(std::bind(&CConnman::ThreadOpenConnections, this, connOptions.m_specified_outgoing)));
2460+    }
2461+
2462+    if (CNode::IsDandelionEnabled()) {
2463+        // Shuffle dandelion destinations
2464+        m_thread_shuffle_dandelion = std::thread(&TraceThread<std::function<void()>>, "dandelionshuff", [&] { ThreadShuffleDandelionDestinations(); });

AM5800 commented at 8:05 am on October 25, 2018:

When I tried this PR in action - dandelion was turned off for the first epoch. I believe this happens because we do the very first shuffle when there are no connections. Maybe the first shuffle should be delayed until someone connects?

in src/net_processing.cpp:3090 in 8b533b25fd

3085+    // Otherwise walk the whole cache to look for the expired tx(s)
3086+    do {
3087+        from->m_cache_expiry.pop();
3088+    } while (from->m_cache_expiry.top() < now_millis);
3089+
3090+    for (auto it = from->m_cache_dandelion.cbegin(); it != from->m_cache_dandelion.cend(); ++it) {

AM5800 commented at 8:17 am on October 25, 2018:

An idea: if you define m_cache_expiry as std::map<int64_t, CTransactionRef> - you will get the same ‘Constant time lookup’ but you won’t need to walk the whole cache - expired transactions are already available

in src/net_processing.cpp:1107 in 8b533b25fd

1104+    }
1105+
1106+    // Dandelion relay (at least one hop)
1107+    const int64_t now_micros = GetTimeMicros();
1108+    const int64_t expiry = now_micros + EXPIRE_DANDELION_CACHE * 1000 * 1000 + PoissonNextSend(now_micros, EXPIRE_DANDELION_CACHE_AVG_ADD);
1109+    node_from->m_cache_dandelion.emplace(std::make_pair(ptx->GetWitnessHash(), std::make_pair(ptx, expiry)));

AM5800 commented at 8:22 am on October 25, 2018:

Nit: node_from->m_cache_dandelion.emplace(ptx->GetWitnessHash(), std::make_pair(ptx, expiry)) does absolutely the same, but shorter.

MarcoFalke removed this from the milestone 0.18.0 on Dec 3, 2018

riclas commented at 1:03 pm on February 4, 2019: none

I believe this explanation of the DDoS possibilities on Dandelion by @sdaftuar belongs here: https://bitcoin.stackexchange.com/questions/81503/what-is-the-tradeoff-between-privacy-and-implementation-complexity-of-dandelion

regarding option b), maybe dynamically adjusting stem % according to fullness of the stempool/cache could be a solution? at worst, we would fluff all the time just like a regular tx spam attack.

MarcoFalke added the label Up for grabs on Jul 11, 2019

MarcoFalke closed this on Jul 11, 2019

MarcoFalke deleted the branch on Jul 11, 2019

MarcoFalke commented at 4:33 pm on July 22, 2019: member

I closed this for now because it is non-trivial to allow for all wallet behavior (e.g. cpfp and rbf) while not opening DOS vectors. I think a simpler first step would be to go for “Dandelion Light” or “Dandelion one-hop”.

That approach would not need any DOS protection, since the only sender is our own wallet and the next hop would already fluff the tx (and send it back to us, so that our wallet can know it made it into the mempool eventually)

laanwj removed the label Needs rebase on Oct 24, 2019

gjhiggins referenced this in commit 699bbce0e3 on Apr 20, 2021

barrystyle referenced this in commit 544de5d342 on Nov 11, 2021

barrystyle referenced this in commit 03325743ae on Nov 15, 2021

DrahtBot locked this on Feb 15, 2022

fanquake removed the label Up for grabs on Aug 4, 2022

fanquake commented at 2:14 pm on August 4, 2022: member

Removed up for grabs.

in src/net_processing.cpp:1114 in 8b533b25fd

1111+    node_from->m_cache_expiry.push(expiry);
1112+
1113+    static constexpr int DANDELION_SEND_FLAGS{0}; // Always send as witness tx
1114+
1115+    // Check if the next peer would fluff (assume same probability)
1116+    bool force_fluff = GetRand(100 * 100) / 100. >= CNode::m_dandelion_stem_pct_threshold;

ajtowns commented at 9:43 am on November 4, 2022:

This doubles the odds of the next node fluffing – 10% chance that we thought they should fluff, but if we don’t think they should fluff, they’ll still make their own decision, giving another 9% (=90%*10%) chance that they’ll think they should fluff for a total of 19%.

(necroreviewing!)

MarcoFalke unlocked this on Nov 4, 2022

MarcoFalke locked this on Nov 4, 2022

Dandelion transaction relay (BIP 156) #13947

Add the protocol messages

Shuffle dandelion stems

Keep a cache of recent dandelion transactions