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

add(mempool): Verify transactions with unmined inputs in the mempool #8857

Open
wants to merge 72 commits into
base: main
Choose a base branch
from

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Sep 9, 2024

Motivation

We want to verify transactions in the mempool that spend the transparent outputs of other transactions in the mempool.

Closes #8777.

Solution

  • Adds a handle to the mempool service in the transaction verifier
  • Adds AwaitOutput request in the mempool service
  • Updates the spent_utxos() method in the transaction verifier to try querying the mempool for spent outpoints before returning a TransparentInputNotFound error.
  • Polls the mempool service from the transaction verifier after verifying a mempool transaction that creates new UTXOs (in case they're spent by other mempool transactions that are still being verified)
  • Updates getblocktemplate method and ZIP-317 transaction selection to include transactions that spent the outputs of other transactions in the mempool when all of their dependencies have been selected

Tests

Added tests to check that:

  • Inserting a transaction also updates the transaction dependencies tracked in the verified set
  • The Storage.remove() method works as expected (the rest of evict_one() should be covered already)
  • The mempool responds to AwaitOutput requests when the output for the queried outpoint is:
    • Added to the mempool
    • Already available in the mempool
  • The transaction verifier:
    • Calls the mempool with AwaitOutput requests as expected and processes UnspentOutput responses correctly
    • Polls the mempool after verifying a mempool transaction that creates new transparent outputs
    • Includes all of the expected spent_mempool_outpoints in its response
  • Long poll ids remaining consistent as long the mempool contents are unchanged (and everything else is unchanged)
  • ZIP-317 mempool transaction selection excludes transactions that have dependencies unless their dependencies have already been selected

If needed, this PR could also add tests for:

  • ZIP-317 mempool transaction selection always returns the right order of transactions (this could be unnecessary since they’re sorted afterwards anyway but it could be a good sanity check)
  • The getblocktemplate method returns transactions with dependencies in the right order when the transaction hashes would have them sorted in the wrong order
  • Ideally at least a basic acceptance test that checks that it all works via RPCs

Follow Up Work

The mempool's Downloads sends a result to the response channel too early, it should wait until the transaction is being inserted into the mempool's verified set.

PR Author's Checklist

  • The PR name will make sense to users.
  • The PR provides a CHANGELOG summary.
  • The solution is tested.
  • The documentation is up to date.
  • The PR has a priority label.

PR Reviewer's Checklist

  • The PR Author's checklist is complete.
  • The PR resolves the issue.

… mempool setup argument, adds and uses an `init_test()` fn for passing a closed channel receiver in tests where no mempool service is needed in the transaction verifier.
…nd a `new_for_tests()` constructor for convenience)
…nsparent outputs has been verified.

adds a TODO for adding a `pending_outputs` field to the mempool Storage
…ng outputs requests when inserting new transactions into the mempool's verified set
@arya2 arya2 added A-mempool Area: Memory pool transactions P-Medium ⚡ labels Sep 9, 2024
@arya2 arya2 self-assigned this Sep 9, 2024
@github-actions github-actions bot added C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Sep 9, 2024
@arya2 arya2 changed the title add(mempool): Verify orphan transactions in the mempool add(mempool): Verify transactions in the mempool with unmined inputs Nov 6, 2024
Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to figure out what happens if someone submits txs containing a circular dependency. We should make sure we can handle such cases well.

@arya2
Copy link
Contributor Author

arya2 commented Nov 8, 2024

I'm trying to figure out what happens if someone submits txs containing a circular dependency. We should make sure we can handle such cases well.

The transaction verifier would return an TransparentInputNotFound error for both because (unlike pending_utxos in the state service) the mempool only responds to AwaitOutput requests once the transaction creating that output has been added to the mempool's verified set.

A similar question around ordering the transactions in block templates caused me some strife (the code should work as expected there too, and it's been excluded from release compilations, but it was difficult to reason about). That exact question may have also caused me some strife a little earlier, as well as what happens to long chains of dependent transactions.

Avoids unnecessarily rejecting dependent transactions of randomly evicted mempool transactions.

Updates `TransactionDependencies::remove_all()` to omit provided transaction id from the list of removed transaction ids.
@arya2 arya2 force-pushed the verify-orphaned-mempool-txs branch from e213f22 to 5829ef3 Compare November 9, 2024 00:46
@arya2 arya2 changed the title add(mempool): Verify transactions in the mempool with unmined inputs add(mempool): Verify transactions with unmined inputs in the mempool Nov 9, 2024
upbqdn
upbqdn previously approved these changes Nov 11, 2024
@upbqdn upbqdn removed the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Nov 12, 2024
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Nov 12, 2024
@upbqdn upbqdn self-requested a review November 12, 2024 15:32
@arya2 arya2 removed the extra-reviews This PR needs at least 2 reviews to merge label Nov 12, 2024
@gustavovalverde
Copy link
Member

@mergify refresh

Copy link
Contributor

mergify bot commented Nov 14, 2024

refresh

✅ Pull request refreshed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-concurrency Area: Async code, needs extra work to make it work properly. A-consensus Area: Consensus rule updates A-mempool Area: Memory pool transactions C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG P-Medium ⚡
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature: support transactions with unmined inputs in the mempool
5 participants