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

Split Anonymity Mempool #5965

Open
wants to merge 3 commits into
base: master
from

Conversation

@vtnerd
Copy link
Contributor

commented Oct 7, 2019

The first two commits are from PR 5921 and from PR 5944 respectively. So inspect the last commit only for changes relevant to this PR.

This is a large chunk of my last CCS (arguably Dandelion++ needs to be implemented for full collection). The mempool now has a separate flag for anonymity which indicates that the tx originated at a particular node that had i2p/tor enabled. Unfortunately this means the harddisk will temporarily store information indicating that a particular txid originated at a node. I didn't see a way around this, because the "stem" transactions (Dandelion++ relaying) and "anonymity" transactions (origin relaying) must be kept separate for timeout purposes, etc.

Also take close inspection on how I changed/merged kept_by_block and do_not_relay into a single enum. I believe this to be the desired behavior, but I had to manually scrub/audit parts of the code multiple times. All of the core tests still pass.

I will be probing this for more tests and just remembered that the markdown file for anonymity networks needs to be updated. But feedback on the high-level approach will be valuable.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2019

Also take close inspection on how I changed/merged kept_by_block and do_not_relay into a single enum.

That part seems dangerous. kept_by_block is used to skip some checks when syncing with the fast sync method of using compiled-in block hashes for example, and while that particular use does not read the flag from the txpool, a future change might change this. It also feels wrong semantically. I think this goes squarely in the "if it ain't broke, don't fix it" category, unless you have an argument in favour of the change.

@vtnerd vtnerd force-pushed the vtnerd:fix/split_mempool branch from b6d5467 to cfe35e7 Oct 8, 2019
@vtnerd

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2019

I've previously argued that kept_by_block is a problem/hack because tx_pool is simultaneously holding pending and non-pending transactions. It always seemed wrong conceptually because tx_pool was doing two related but slightly different tasks.

The primary issue I have is that the upcoming relay_method "states" are:

  • do_not_relay (local mining)
  • Dandelion++ stem
  • Dandelion++ fluff
  • Anonymity (origin relaying via i2p/tor)
  • keeped_by_block (included in validly mined block).

Basically, there is a decent amount of state information needed for pending txs, and using a set of booleans to track this information is harder to maintain. I don't see a valid reason to use relay_method::fluff, relay_method::stem, or relay_method::anonymity once a transaction is marked with relay_method::block, because that takes precedence over all methods of relaying a transaction. It should be safe to always assume that such a tx was most recently relayed using the block relay method (currently "flooding"). So at a minimum, I think its safe to say that these can be grouped into an enum that tracks the most recent technique for relaying a transaction.

I think the "core" of your concern is highlighted in this comment written by you. kept_by_block arguably conflicts with do_not_relay specifically. One possible conflict is when a chain is rewound (above link is such case); it might be useful to know that originally the tx was marked with do_not_relay. That information cannot be tracked currently, because once the local node "mines" the newest main chain, the tx is removed from the mempool and original metadata (including do_not_relay) is purged. So I haven't run into this issue currently, but theoretically this could happen in some future refactor. The only possible fix is to track do_not_relay txes in some data structure, so that when things are unwound the tx returns to the do_not_relay state. This information would need to be kept until the next hardfork/checkpoint.

I think your concerns about syncing, etc., are handled already because everywhere kept_by_block was set to true, now uses relay_method::block to track the same information. I don't recall seeing a single place where kept_by_block and do_not_relay were simultaneously set to true, although theoretically there could be a reason to change this in the future. The situation would be unique though.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

I'm totally fine with "kept_by_block" overriding "do_not_relay", assuming the semantics of these are what they are now. The comment you point to is a corner case that should ideally never happen unless bug in the source. I've just gone over the diff quicky so far, so maybe it's actually OK with the exact changes. I'll know when I've read it in more detail.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2019

github won't post that comment inline, so:

> -      if (keeped_by_block)
> +      if (tx_relay == relay_method::block)
>          get_blockchain_storage().on_new_tx_from_block(results[i].tx);

That's one very good reason to not merge keeped_by_block and do_not_relay. This could make the fast sync go out of sync.

@vtnerd

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2019

I don't understand what you mean exactly. The tx_relay value is from the function parameter, not anything existing in the tx pool. Invocations of the function handle_incoming_txs can currently never have keeped_by_block and do_not_relay set to true simultaneously. keeped_by_block is never dynamic, and the only places where do_not_relay is dynamic keeped_by_block is set to false. The simultaneous concepts of do_not_relay and "this just came from a block" are somewhat awkward - the tx has already been relayed if its in a block (with selfish mining being an execption, etc.).

I'll run a testnet sync with this patch from genesis later (this afternoon!?) and report though.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2019

I mean:

  • you said you merged kept_by_block and do_not_relay
  • the code I pointed to uses kept_by_block to decide whether to check some things
  • these semantics seem entirely different

Therefore, I am saying I think merging do_not_relay and kept_by_block is dangerous, even though I do not have an actual bug to point to.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2019

OK, I see where you're going. do_not_relay is essentially a "subdivision" of the false state of kept_by_block, right ? It might be OK in practice.

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