Skip to content
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

Dandelion transaction relay (BIP 156) #13947

Open
wants to merge 5 commits into
base: master
from

Conversation

@MarcoFalke
Copy link
Member

commented Aug 12, 2018

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.

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2018

This is completely untested, so TODO:

  • add tests
  • use dandelion relay for our wallet txs
@@ -755,6 +778,10 @@ class CNode
// Our address, as reported by the peer
CService addrLocal;
mutable CCriticalSection cs_addrLocal;

/** Our current dandelion destination */
CConnman::DandelionPeer* m_dandelion_destination /* GUARDED_BY(CConnman::cs_dandelion_peers) */ {nullptr};

This comment has been minimized.

Copy link
@practicalswift

practicalswift Aug 12, 2018

Member

Any drawbacks from uncommenting the GUARDED_BY(…) annotation?

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Aug 12, 2018

Author Member

The drawback was that it didn't compile for me

@MarcoFalke MarcoFalke added the P2P label Aug 12, 2018

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

@MarcoFalke MarcoFalke added the Feature label Aug 12, 2018

src/init.cpp Outdated
@@ -1027,6 +1031,12 @@ bool AppInitParameterInteraction()
LogPrintf("Warning: nMinimumChainWork set below default value of %s\n", chainparams.GetConsensus().nMinimumChainWork.GetHex());
}

{
const int64_t pct = gArgs.GetArg("-dandelion", DEFAULT_DANDELION_STEM_PERCENTAGE);
if (pct < 0 || 100 < pct) return InitError(_("-dandelion must be 0 or a percentage less than 100"));

This comment has been minimized.

Copy link
@practicalswift

practicalswift Aug 12, 2018

Member

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

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Aug 12, 2018

Author Member

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

This comment has been minimized.

Copy link
@practicalswift

practicalswift Aug 13, 2018

Member

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

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2018

Note to reviewers: This pull request conflicts with the following ones:
  • #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.

@@ -33,6 +33,7 @@ BIPs that are implemented by Bitcoin Core (up-to-date up to **v0.17.0**):
* [`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)).
* [`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)).
* [`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)).
* [`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)).

This comment has been minimized.

Copy link
@clodhopp

This comment has been minimized.

Copy link
@meshcollider

meshcollider Aug 13, 2018

Member

@clodhopp that's because the BIP PR hasn't been merged, you can see it here: bitcoin/bips#703
Once merged, the link should work

@sipa

This comment has been minimized.

Copy link
Member

commented Aug 12, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2018

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

@sdaftuar
Copy link
Member

left a comment

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).

if (gArgs.SoftSetBoolArg("-whitelistrelay", false))
LogPrintf("%s: parameter interaction: -blocksonly=1 -> setting -whitelistrelay=0\n", __func__);
if (gArgs.SoftSetBoolArg("-dandelion", false)) {

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Aug 13, 2018

Member

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.)

This comment has been minimized.

Copy link
@stevenroose

stevenroose Aug 20, 2018

Contributor

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

src/net.h Outdated
@@ -803,7 +847,19 @@ class CNode
nRefCount--;
}

/** If this peer is not yet connected or soon to be disconnected */
bool IsEphermal() const

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Aug 13, 2018

Member

nit: IsEphemeral

src/net_processing.cpp Outdated
else if (strCommand == NetMsgType::TX_DANDELION) {
// Stop processing the transaction early if
// dandelion is disabled and peer is either not whitelisted or whitelistrelay is off
if (!CNode::IsDandelionEnabled()&& (!pfrom->fWhitelisted || !gArgs.GetBoolArg("-whitelistrelay", DEFAULT_WHITELISTRELAY))) {

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Aug 13, 2018

Member

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

src/net_processing.cpp Outdated
return true;
}

if (pfrom->fWhitelisted && gArgs.GetBoolArg("-whitelistforcerelay", DEFAULT_WHITELISTFORCERELAY)) {

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Aug 13, 2018

Member

See above comment about dropping this behavior for whitelisted peers.

* The dandelion tx message transmits a single transaction.
* @see https://github.com/bitcoin/bips/blob/master/bip-0156.mediawiki
*/
extern const char* TX_DANDELION;

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Aug 13, 2018

Member

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...

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Aug 13, 2018

Author Member

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.

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Aug 14, 2018

Member

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?

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Aug 14, 2018

Author Member

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.

LogPrint(BCLog::NET, "Unknown command \"%s\" from peer=%d\n", SanitizeString(strCommand), pfrom->GetId());

This comment has been minimized.

Copy link
@stevenroose

stevenroose Aug 20, 2018

Contributor

Why not a service bit?

CValidationState state;
bool missing_inputs;
if (AcceptToMemoryPool(mempool, state, tx, &missing_inputs, nullptr /* lRemovedTxn */, false /* bypass_limits */, 0 /* nAbsurdFee */, true /* test_accept */)) {
RelayDandelionTransaction(mempool, pfrom, tx, msgMaker, connman);

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Aug 14, 2018

Member

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).

This comment has been minimized.

Copy link
@gmaxwell

gmaxwell Aug 15, 2018

Member

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

src/net_processing.cpp Outdated
@@ -1908,6 +1967,11 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
State(pfrom->GetId())->fPreferHeaders = true;
}

else if (strCommand == NetMsgType::ACCEPT_DANDELION) {
pfrom->m_accept_dandelion = true;
return true;

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Aug 14, 2018

Author Member

Should write something to debug log here.

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1804-dandelion branch 3 times, most recently Aug 14, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1804-dandelion branch 4 times, most recently Aug 15, 2018

src/init.cpp Outdated
@@ -399,6 +399,7 @@ void SetupServerArgs()
gArgs.AddArg("-bantime=<n>", strprintf("Number of seconds to keep misbehaving peers from reconnecting (default: %u)", DEFAULT_MISBEHAVING_BANTIME), false, OptionsCategory::CONNECTION);
gArgs.AddArg("-bind=<addr>", "Bind to given address and always listen on it. Use [host]:port notation for IPv6", false, OptionsCategory::CONNECTION);
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);
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);

This comment has been minimized.

Copy link
@stevenroose

stevenroose Aug 20, 2018

Contributor

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.

This comment has been minimized.

Copy link
@practicalswift

practicalswift Aug 20, 2018

Member

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? :-)

This comment has been minimized.

Copy link
@stevenroose

stevenroose Oct 3, 2018

Contributor

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

Probability in % to extend ..

if (gArgs.SoftSetBoolArg("-whitelistrelay", false))
LogPrintf("%s: parameter interaction: -blocksonly=1 -> setting -whitelistrelay=0\n", __func__);
if (gArgs.SoftSetBoolArg("-dandelion", false)) {

This comment has been minimized.

Copy link
@stevenroose

stevenroose Aug 20, 2018

Contributor

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

src/net.cpp Outdated
{
from->m_dandelion_destination = nullptr;

if (!CNode::IsDandelionEnabled()) return;

This comment has been minimized.

Copy link
@stevenroose

stevenroose Aug 20, 2018

Contributor

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.

@@ -317,6 +327,16 @@ class CConnman
*/
int64_t PoissonNextSendInbound(int64_t now, int average_interval_seconds);

using DandelionPeer = std::pair<CNode*, /* use count */ unsigned>;

This comment has been minimized.

Copy link
@stevenroose

stevenroose Aug 20, 2018

Contributor

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

This comment has been minimized.

Copy link
@AM5800

AM5800 Oct 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

/** Time between pings automatically sent out for latency probing and keepalive (in seconds). */
static const int PING_INTERVAL = 2 * 60;
/** Time after which to disconnect, after waiting for a ping response (or inactivity). */
static const int TIMEOUT_INTERVAL = 20 * 60;
/** Run the feeler connection loop once every 2 minutes or 120 seconds. **/
static const int FEELER_INTERVAL = 120;
/** Pick new dandelion peers once every 10 minutes or 600 seconds. */
static constexpr int INTERVAL_DANDELION = 600;

This comment has been minimized.

Copy link
@stevenroose

stevenroose Aug 20, 2018

Contributor

The convention seems to be XXX_INTERVAL instead of INTERVAL_XXX.

This comment has been minimized.

Copy link
@stevenroose

stevenroose Aug 20, 2018

Contributor

Also, why 10 minutes?

This comment has been minimized.

Copy link
@Sjors

Sjors Sep 2, 2018

Member

@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)

This comment has been minimized.

Copy link
@stevenroose

stevenroose Sep 4, 2018

Contributor

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


/** Default for -dandelion stem percentage */
static constexpr int64_t DEFAULT_DANDELION_STEM_PERCENTAGE = 90;

This comment has been minimized.

Copy link
@stevenroose

stevenroose Aug 20, 2018

Contributor

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

This comment has been minimized.

Copy link
@Sjors

Sjors Sep 2, 2018

Member

@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.

This comment has been minimized.

Copy link
@stevenroose

stevenroose Sep 4, 2018

Contributor

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

This comment has been minimized.

Copy link
@stevenroose

stevenroose Sep 4, 2018

Contributor

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.

});
}

static void FluffTransaction(CTxMemPool& tx_pool, const CNode* from, const CTransactionRef& ptx, CConnman* connman)
{
LogPrint(BCLog::DANDELION, "fluff\n");

This comment has been minimized.

Copy link
@stevenroose

stevenroose Aug 20, 2018

Contributor

Perhaps mention txid?

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

This comment has been minimized.

Copy link
@stevenroose

stevenroose Aug 20, 2018

Contributor

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

src/net_processing.cpp Outdated
}

LogPrint(BCLog::DANDELION, "dandelion relay to peer=%d\n", dest->GetId());
connman->PushMessage(dest, msg_maker.Make(DANDELION_SEND_FLAGS, NetMsgType::TX_DANDELION, *ptx));

This comment has been minimized.

Copy link
@stevenroose

stevenroose Aug 20, 2018

Contributor

Wrong indentation since after return.

* The dandelion tx message transmits a single transaction.
* @see https://github.com/bitcoin/bips/blob/master/bip-0156.mediawiki
*/
extern const char* TX_DANDELION;

This comment has been minimized.

Copy link
@stevenroose

stevenroose Aug 20, 2018

Contributor

Why not a service bit?

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1804-dandelion branch 3 times, most recently Aug 21, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1804-dandelion branch to 8b533b2 Aug 21, 2018

@DrahtBot DrahtBot referenced this pull request Aug 23, 2018

Open

Add p2p layer encryption with ECDH/ChaCha20Poly1305 #14032

0 of 1 task complete
@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2018

Needs rebase
@Sjors
Copy link
Member

left a comment

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?


/** Default for -dandelion stem percentage */
static constexpr int64_t DEFAULT_DANDELION_STEM_PERCENTAGE = 90;

This comment has been minimized.

Copy link
@Sjors

Sjors Sep 2, 2018

Member

@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.

/** Time between pings automatically sent out for latency probing and keepalive (in seconds). */
static const int PING_INTERVAL = 2 * 60;
/** Time after which to disconnect, after waiting for a ping response (or inactivity). */
static const int TIMEOUT_INTERVAL = 20 * 60;
/** Run the feeler connection loop once every 2 minutes or 120 seconds. **/
static const int FEELER_INTERVAL = 120;
/** Pick new dandelion peers once every 10 minutes or 600 seconds. */
static constexpr int INTERVAL_DANDELION = 600;

This comment has been minimized.

Copy link
@Sjors

Sjors Sep 2, 2018

Member

@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)

@thothd thothd referenced this pull request Sep 14, 2018

Closed

Dandelion (BIP 156) #100

@scravy

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2018

Shouldn't the functional test from the reference implementation (dandelion-org@d043e36#diff-21ab9447686d819eeeb7668ce8011e0d) run more-or-less unchanged on top of this branch?

m_nodes_dandelion.clear();
for (CNode* node : all_nodes) {
// Ignore inbound nodes
if (node->fInbound) continue;

This comment has been minimized.

Copy link
@rodasmith

rodasmith Sep 30, 2018

Contributor

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?

This comment has been minimized.

Copy link
@instagibbs

instagibbs Oct 3, 2018

Member

Inbound nodes are much more likely to be snooping peers.

This comment has been minimized.

Copy link
@kek-coin

kek-coin Oct 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

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2018

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

This comment has been minimized.

Copy link

commented Oct 9, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2018

@jb55 BIP156 should explain a lot of things.

@jb55

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2018

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

});
}

static void FluffTransaction(CTxMemPool& tx_pool, const CNode* from, const CTransactionRef& ptx, CConnman* connman)

This comment has been minimized.

Copy link
@AM5800

AM5800 Oct 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?


if (CNode::IsDandelionEnabled()) {
// Shuffle dandelion destinations
m_thread_shuffle_dandelion = std::thread(&TraceThread<std::function<void()>>, "dandelionshuff", [&] { ThreadShuffleDandelionDestinations(); });

This comment has been minimized.

Copy link
@AM5800

AM5800 Oct 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?

from->m_cache_expiry.pop();
} while (from->m_cache_expiry.top() < now_millis);

for (auto it = from->m_cache_dandelion.cbegin(); it != from->m_cache_dandelion.cend(); ++it) {

This comment has been minimized.

Copy link
@AM5800

AM5800 Oct 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

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

This comment has been minimized.

Copy link
@AM5800

AM5800 Oct 25, 2018

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

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

@riclas

This comment has been minimized.

Copy link

commented Feb 4, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.