gfanti / bitcoin Goto Github PK

As we mentioned in the draft BIP:

For example, an attacker seeing tx1 could create an invalid transaction tx2 that purports to spend an output from tx1, and send it to victim node Alice. If Alice has placed tx1 in mempool, then Alice will reject the attacker and disconnect it; if Alice has not seen tx1, then tx2 would instead go to the orphan cache.

Pramod suggested that we mitigate this by implementing a policy of delaying the disconnecting of a misbehaving peer. This seems viable, though it could potentially invite DoS where a node sends lots of transactions with invalid signatures to tie up the CPU. We could combat this with more logic to skip further signature checking from such a node.

Add similar DoS protections to mapDandelionRelay to those currently implemented for mapRelay. E.g., cap the size.

Right now the vStemNodes set is updated only every 10 minutes, but it would be better if this were done a) at random times, and b) updated whenever one of the nodes disconnects... or c) updated if there are already too few (fewer than two) and a new suitable connection is established.

Like what BIP 152 does https://github.com/bitcoin/bips/blob/master/bip-0152.mediawiki#Protocol_Versioning

The idea would be that we identify ourselves as a dandelion peer by sending a Dandelion-specific sample message, e.g. a INV message with a CInv(type=MSG_DANDELION_TX) and if the peer recognizes it, they know we're a dandelion peer.

vectors can be array[indexed] directly, you don't need to advance an iterator

Do not use rand() it isn't cryptographic-- use GetRand(range) or FastRandomContext's randrange (esp for using many rand calls)
(thanks gmaxwell for this suggestion)

Accesses to stemset may be concurrency unsafe--what lock is protecting it from concurrent access?

(thanks gmaxwell for this suggestion)

Dandelion messages might need to be another inv type, rather than a new kind of inv (currently DINV).
(thanks gmaxwell for this suggestion)

Elsewhere when we need to do processing outside use the node ids, e.g. when you collect the candidate nodes, collect their ids. Then pick an id. and when you loop over the nodes for the send, use the id. cfields may have better advice since the recent changes to the peer handling API are his.

(thanks gmaxwell for this suggestion)

Add random timers for robustness to black hole attacks.

RIght now, an item is inserted into stemSet upon receiving a D_INV message, and erased from stemSet upon receiving a matching TX message. So, this could grow unbounded by sending random hashes. A solution to this might be to use a time-ordered map for stemSet instead of a plain set.

I am very interested in seeing this or a similar technology implemented into Bitcoin. Are you guys still actively working on it?

The candidates set should exclude the peer the transaction was just received from. It should also consider the peer's fee-filter-- no reason to send a transaction that will just be rejected.

(thanks gmaxwell for this suggestion)

RPC calls like SendRawTransaction should also attempts to use Dandelion propagation. Unlike transactions that we receive, RelayTransactionDandelion is not called in this instance. It appears that the function to modify in this case is SendMessages.

If the active stem node disappears, the effect would be similar to a black hole. This means that we'd rely on the random blackhole timer to kick in to switch to fluff mode.

It would make sense to immediately select a new stem node if we can tell that the current stem node is disconnected. However, now we might need to worry about orphan transactions. Suppose tx1 is sent to A. Then A disconnects and the transaction is paused until the embargo timer goes off. Now tx2 is sent to the new stem node B. The tx2 would be rejected since its sent out of order.

A solution is to:

1. When a stem node A disconnects, select a new stem node B to replace it.
2. For any currently-embargoed transactions stored at A, send these transactions also to B.

A received dinv should be checked against the same filters that a normal inv is (mempool, setknown, rejects) and then requested even if the txn has already been fetched before. When the txn is received it should be tested against the mempool but not inserted into the mempool-- because the presences of a transaction can be interrogated out of a nodes mempool (e.g. via mempool rpc or by observing the effects of conflicts). A separate peer-specific relay pool may be required to prevent third parties from querying for propagation by attempting to fetch a txn which only passed through a node in stem mode.
(thanks gmaxwell for this suggestion)

As of 66d0cf2, if a node receives an DINV(tx) corresponding to a tx already under embargo, then embargo ends immediately and the transaction is propagated like usual in "fluff" mode. This is intended to help handle "cycles" gracefully without the transaction getting stuck.

However, this leads to a possible strategy for a supernode to identify the nodes involved in a stem. Upon receiving a stem message DTX(tx), the attacker could send DTX(tx) to every node. For nodes that already have the stem, this would certainly cause them to "fluff" immediately; for nodes that don't, they would fluff with only 0.1 probability.

A mitigation would be to handle cycles a different way... enter fluff mode upon receiving a DINV from the same peer a second time. This would require storing some additional data somewhere (right now we forget who told us about the tx).