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
Adding Dandelion++ support to public networks: #6314
Conversation
vtnerd
commented
Jan 31, 2020
- New flag in NOTIFY_NEW_TRANSACTION to indicate stem mode
- Stem loops detected in tx_pool.cpp
- Embargo timeout for a blackhole attack during stem phase
@@ -160,21 +160,25 @@ struct txpool_tx_meta_t | |||
uint64_t max_used_block_height; | |||
uint64_t last_failed_height; | |||
uint64_t receive_time; | |||
uint64_t last_relayed_time; | |||
uint64_t last_relayed_time; //!< If Dandelion++ stem, randomized embargo timestamp. Otherwise, last relayed timestmap. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewers may want to take notice: I hijacked last_relayed_time
in mempool to mean "embargo timeout timestamp" when the tx is marked as Dandelion++ stem. There's some padding space that could be used instead, if preferred.
src/cryptonote_core/tx_pool.cpp
Outdated
tx_relay = relay_method::fluff; | ||
} | ||
else | ||
meta.set_relay_method(relay_method::none); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That else if for hte "if (existing_tx)" right ? It seems indented off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes, a tab probably slipped in here. Will fix.
{ | ||
tx_relay = relay_method::stem; | ||
fluff_txs.reserve(arg.txs.size()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the reserved vectors swapped w.r.t arg.dandelionpp_fluff ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, I botched this. Initially the field was called dandelionpp_stem
, but I flipped the logic so that zero initialization of the struct
would default to stem mode. And since this can't be easily observed through testing ... anyway will change.
A question: How to best test that "from the outside"? I.e. as someone that runs a Dandelion++ enabled node and wants to see from log output whether some Dandelion++ enabled transactions arrive and are either passed on or are finally accepted into the mempool and re-broadcast (if I understand the protocoll correctly)? I looked over the code but did not find something like a new Dandelion++ related log category. Maybe something like that could be very useful? I imagine only a few log lines at strategic points would already help a great deal watching a daemon "doing its thing". |
There is log of every outgoing p2p message ( |
@vtnerd i'm gonna make a reddit post later today asking people to build this PR and run a testnet node with it, to test dandelion. I see this branch is 58 commits behind upstream master, could you rebase? Shouldn't be an issue anyway, but since people are going to run a testnet node with this branch, better having them running current master + this PR, if possible. |
I could not find On mainnet that leads to quite some amount of messages where it may be quite difficult to watch out for the communication patterns that you describe. On testnet most of the time nothing happens regarding transactions, so there it's probably feasible. |
rebased and added log statements in the |
Call for testers: https://www.reddit.com/r/Monero/comments/f9cksh/help_us_test_dandelion_on_testnet_instructions/
|
I just tried with the new code, and somehow I could not get it to work: Everything worked transaction-wise, i.e. transactions were pooled, broadcasted and mined, but I never saw any additional log message using the new category But my daemon now gives me the following line four times, whether the new log category is set or not:
Any idea what I might do wrong, or what might be the problem here? |
gh-actions functional tests failed. |
This is "noise" that can be ignored for now - it occurs when the tx comes back to the node. The problem is that the tx is removed from the DB, then key_images are inserted into a local map which does a DB lookup (which is new), and then the tx is added to the DB. This was always the call sequence, so I avoided changing it. But this error needs to be removed before a release - investigating what to do about it.
That's because I pushed the rebase, but not the log additions. Crap. The latest push should have the log statements. |
Now seems to be working. After sending a transaction i get:
The Failed to get tx meta from txpool message four times and then
|
A comment on reddit pointed to a discussion on Github/Bitcoin about DoSing with Dandelion. One thing I think should be added is a rejection if an existing key-image with a new tx_hash is seen. Currently this is not rejected, because its treated as seeing the same key-image/tx_hash pair a second time (which it pretends it never saw for privacy reasons). This would hurt privacy in the rare situation that a wallet tries to use the same unspent output in two different transactions before the transaction ages out of the mempool. However, this is already disallowed. @moneromooo-monero ? You were asking about these key-image checks, in a prior review (the relevant code isn't actually changing in this diff). This is probably worthy of a new PR since its already in the codebase. |
Rebased (there was no conflicts, but users attempting this weekends stress test will testing latest HEAD). Also, fixed the error log issue, see my new inline comment. |
if (!insert_key_images(tx, id, tx_relay)) | ||
return false; | ||
|
||
m_blockchain.remove_txpool_tx(id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the force push, this was moved here below the call to insert_key_images
now on line 345.
Explanation:
Initially the call sequence was remove_txpool_tx
-> add_txpool_tx
-> insert_key_images
. In PR 5965 I flipped insert_key_images
-> add_txpool_tx
because the insert_key_images
was modified to make a DB check to get the "state" of the transaction. This was necessary because the key image check should not fail if the tx state is going from stem->fluff.
This meant that if the tx was going from stem -> fluff the tx had its metadata removed before the key images check. This was/is hard to detect via the core_tests
because cryptonote_core::core
will filter double spends before it even gets to this line. Txes going from stem->fluff are not filtered, which is why the error was logged.
This tests now pass, with the exception of an unrelated windows macro issue. |
- New flag in NOTIFY_NEW_TRANSACTION to indicate stem mode - Stem loops detected in tx_pool.cpp - Embargo timeout for a blackhole attack during stem phase
@vtnerd - Could you rebase? |
Rebased. |
Some core tests are now fail. ahhh hold on. |
Ok, now that should be a proper rebase. |
Functional tests failed. |
Rebased to fix conflicts. PR 6445 should hopefully fix functional tests. Or at least be closer. Enough changes were made, I decided to spin that into its own PR. |
Rebased again. |
(I did not check the diffs in this rebase were all actual diffs from the master merge)