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

WIP: Tracking Transaction Pool Implementation #48

Merged
merged 43 commits into from
May 19, 2017

Conversation

MoaningMyrtle
Copy link
Contributor

@MoaningMyrtle MoaningMyrtle commented May 3, 2017

Howdy,

This pull request covers the current state of the transaction pool implementation I've been working on. I'd love some feedback- I know there's stuff that can be improved.

This is a WIP, the things under "What's missing" are being worked on and will get merged into this branch as they near completion.

What's there:

  1. Base types for pool and orphans, based on graph data structures
  2. An implementation of pool transaction adding which takes into account connectivity
  3. An implementation of new block reconciliation which maintains pool consistency with the outside world
  4. Test cases for the above

What's missing:

  1. Orphans reconciliation- entrypoints are there, but no attempt is made to reconnect orphans to the pool
  2. New block building- working on a mechanism to do basic transaction selection based on connectivity, age, and compressibility
  3. Size constraints/DoS protection- reporting on various metrics are in place, they just need to be plugged in
  4. A helper to resolve compact block input/output ids- waiting on a bit more implementation details to land.
  5. A lot of optimization, refactoring, and general cleanliness.

Open questions:

  1. Transaction signature/range proof validation- this will be based on Transaction::validate, but I think this is best done outside of the pool (or at best, in a trivial method at the pool level), as it is not concerned with connectivity.
  2. Transaction identifiers- this implementation borrows the transaction::merkle_inputs_outputs method from core, which has some gaps in coverage (notably the range proofs and anything in the kernel that isn't fees)
  3. Orphans set handling- bit torn on how to handle orphans. As of now, this is a graph container similar to the pool itself. This is workable, but it may be advantageous to use subgraphs (which could be connected in their entirety when certain criteria are met). On the other extreme, this could just be a heap-allocated pile of disconnected transactions, reconciled outside of the primary write lock through the standard transaction path.
  4. Connectivity to the blockchain- this pool implementation is based on a DummyPool and DummyChain visible in blockchain.rs. It is not intended to be a perfect match for a future blockchain component and delegated UTXO set resolver. How should we proceed in terms of integration?

Thanks for any comments and review!

…Contains lots of cleanup on the internal flow. Still TODO: Double spend within orphans detection
…ut block reconciliation test; bugfix for mark_transaction
/// Happens under an exclusive mutable reference gated by the write portion
/// of a RWLock.
///
pub fn add_to_memory_pool(&mut self, source: TxSource, tx: transaction::Transaction) -> Result<(), PoolError> {

This comment was marked as spam.

This comment was marked as spam.

@ignopeverell
Copy link
Contributor

You win the prize of the largest, most comprehensive pull request to date!!

About the PR itself, I just had one minor comment. The rest looked reasonable to me. Regarding your open questions:

  1. I believe transaction validation already does that.
  2. I've created a new issue (Fix transaction Merkle tree to cover everything that's not covered somewhere else #49) so we don't forget this.
  3. I trust your judgment :)
  4. I'll do a pass at that in the near future. I may get back to you if I have questions or requests but I'm hoping we can keep that glue code reasonably clean.

Now for process, I'll have a need for a transaction pool soon, so I'm tempted to merge this very soon. Anything you'd absolutely want to finish up first? Otherwise I believe everything that's missing could be addressed in more isolated, smaller PRs. What do you think?

@MoaningMyrtle
Copy link
Contributor Author

Thanks for the comments!

Currently, to its detriment, the transaction pool is almost entirely disconnected from the rest of the system. This does work to our advantage in terms of merging early without too much pain, however. I'm happy to do whatever you feel makes sense.

IMO, the highest priority of the remaining tasks is to get mining transaction selection working. I have an implementation in the pipeline based on a topological graph sorting algorithm, but if you prefer I can merge in the simplest possible solution to my branch (which would be returning the list of "roots" of the pool set) at an earlier time. Given the current isolation between the pool and everything else, I have no preference between doing this before or after merging in the pool chunk.

@MoaningMyrtle
Copy link
Contributor Author

Minimum transaction selection is now merged into my branch; still working on the cleanup in add_to_memory_pool.

@MoaningMyrtle
Copy link
Contributor Author

cc @ignopeverell

add_to_memory_pool has been delegated better to submethods, does this look OK to you? If so, all your outstanding concerns should be resolved.

@ignopeverell ignopeverell merged commit 23fd07b into mimblewimble:master May 19, 2017
@ignopeverell
Copy link
Contributor

That looked good, merged. Congrats and thanks for the awesome work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants