Add randomized delay when forwarding txes from i2p/tor -> ipv4/6 - #6354
Conversation
There was a problem hiding this comment.
This drops a mempool meta bit flag that has never been in a release, but has been in master. I realized it was more efficient to use kept_by_block instead. Anyone running from master that has a is_local flag in their pool will see incorrect behavior when running this change. I think its better to clean this up before a release, but ... ?
There was a problem hiding this comment.
Are you saying kept_by_block is new ? It's not. Or are you talking about something else here ? nvm, the relevant part is not what's displayed in this bit of code.
788c4d9 to
51adada
Compare
|
Rebased, and made changes suggested by @xiphon . |
There was a problem hiding this comment.
tbh, this is really dodgy. Does this really need to be obfuscated like this ?
There was a problem hiding this comment.
The code could continue using the inefficient bit mechanism, or could "break" the existing txpool between releases (this code breaks the txpool for users running master branch only).
There was a problem hiding this comment.
Why do we care about efficient bit twiddling when it's about I/O ?
There was a problem hiding this comment.
Its less wasted space and less CPU cycles to just store/retrieve this value as the enum value.
There was a problem hiding this comment.
Well, whatever. If you prefer saving a few cycles hidden next to massive amounts of them for confusing code, fine.
There was a problem hiding this comment.
No, my preference was to break the existing txpool (pain should be minimal) to do: m_state = uint8_t(tx_relay);.
I think in the end, I will drop this. This creates a hard dependency on the enum value and DB value. How about something like:
const uin8_t state =
uint8_t(do_not_relay) +
uint8_t(kept_by_block) << 1 +
uint8_t(forward) << 2;
switch (state)
{
default:
case 0:
return relay_method::fluff;
case 1:
return relay_method::none;
case 2:
return relay_method::block;
case 4:
return relay_method::forward:
}which also handles the incorrect DB state, and maps it to fluff. Or the if-else ladder (which maps the incorrect DB state to some other value - but I doubt that matters anyway).
There was a problem hiding this comment.
I much prefer this since I find it much clearer. It looks equivalent to the even simpler:
if (forward)
return relay_method::forward;
if (kept_by_block)
return relay_method::block;
if (do_not_relay)
return relay_method::none;
return fluff;
There was a problem hiding this comment.
Amusingly, it looks close to what you had before. The thing I was finding objectionable was not the set of tests, it was the case on enum+1.
There was a problem hiding this comment.
Yes, the original version was iffy because it required knowledge of the enum values to read it correctly. And a change in enum values could break the function. This version is similar looking, but everything is self-contained within the function.
FWIW, I disassembled this version and gcc 9 was able to optimize the function into a single comparison for the "error" case. Otherwise it used adds, shifts and a 64-bit constant to put the enum value in the return register. Roughly half of the instructions could be done in parallel by the CPU backend.
The if-ladder is less x64-64 instructions but 3-total comparison instructions. The compiler can still generate the same ASM with the switch version, but chose one with more math instructions and less comparisons.
There was a problem hiding this comment.
For clarity, I really don't care about a dozen cycles saved for this. What I care about is the complexity of the C code.
51adada to
db8606f
Compare
|
Rebased and reverted to a bitfield. Reviewers should inspect new implementation of |
|
@vtnerd - Needs a rebase. |
b3df447 to
20d26bb
Compare
There was a problem hiding this comment.
Rebased. This was not trivial after the Dandelion++ changes. I provided comments to help point out the changes. Please inspect.
Committers - this PR should be considered unsafe due to the one issue pointed out in this review (caused by complications with Dandelion++ changes). Sorry for not noticing this potential issue earlier, it wasn't immediately obvious. All tests are expected to pass.
There was a problem hiding this comment.
This merge here was a little more than usual, might be worth inspecting.
There was a problem hiding this comment.
And @moneromooo-monero I ruined tabs again just for you. I switched to emacs recently :/ will fix.
There was a problem hiding this comment.
Changed (see other comment).
There was a problem hiding this comment.
Changed (see other comment).
There was a problem hiding this comment.
See removal of tx_relay = relay_method::fluff. @moneromooo-monero asked about this in the Dandelion++ review, and the original code was/is incorrect. I thought it was appropriate to mark the txes as "fluff" because the fluff routine was being used. However, if someone disables i2p/tor white noise, its only using the fluff algorithm, and isn't intended to be marked as public. So the "state" change is not made here. This is verified in the unit tests (see other comments).
There was a problem hiding this comment.
Another reasonable big change. Transactions in the forward state get relayed over Dandelion++ stems when the timer expires.
20d26bb to
37f836c
Compare
|
Tabs removed, and everything should go green. But see my own review above. |
a6505b7 to
7392e65
Compare
vtnerd
left a comment
vtnerd
left a comment
There was a problem hiding this comment.
Marked two of my own "issues" as resolved. This PR should be ready, pending reviewers doing a partial "re-review" after the Dandelion++ changes were merged.
@moneromooo-monero @xiphon @SarangNoether (others?) - Another semi-major change. If the user disables "noise" (i.e. --tx-proxy,127.0.0.1:9050,disable_noise), then the tx is "fluffed" over outbound Tor connections, and the receiving hidden service will immediately fluff the transaction over ipv4/6 upon receive. The i2p/tor network is replacing Dandelion++ stem - (hopefully) breaking the p2p sybil attacks that Dandelion++ was designed to mitigate. This also allows users to use i2p/tor, but still get reasonable delays when sending a tx (the noise feature is more aggressive in its privacy and latency).
If the user leaves the default ("noise"), then the stem flag is set over i2p/tor. The receiving hidden service will forward over ipv4/6 using Dandelion++ stem after a randomized delay. The delay is to help mitigate against ISP packet+timing analysis (which is only useful if noise is being used). Arguably it could "fluff" mode after all this, but if the user waits that long for the send, whats a few more for Dandelion++ stem.
Any of this can be tweaked as more analysis is done.
There was a problem hiding this comment.
Marking the above comment as resolved, see comment here about the change to the PR.
There was a problem hiding this comment.
Marked this change earlier with poor tabbing (which is now marked as resolved), and re-marking to highlight the somewhat complex rebase since earlier reviews.
There was a problem hiding this comment.
See the change here, and associated comment above.
There was a problem hiding this comment.
Changed within this PR (see other comment within this review in levin_notify.cpp). All similar changes were for same reason.
7392e65 to
67ade80
Compare
5 participants
The last bullet point on the CCS for transaction privacy is a randomized delay before a node takes an incoming transaction from i2p/tor and sends it over ipv4/6. My thought was an attempt to conceal the bandwidth/timing analysis of i2p/tor. If the the i2p/tor hidden service only has one incoming connection, this delay possibly/probably does nothing. But if the hidden service has 2+ incoming connections sending white noise, the transaction could've been received over any of the white noise connections.
This creates another mempool state.
@SarangNoether might be interested.