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

Match rejected transactions correctly by TXID or WTXID #2819

Closed
Tracked by #2744 ...
conradoplg opened this issue Oct 1, 2021 · 3 comments · Fixed by #2833
Closed
Tracked by #2744 ...

Match rejected transactions correctly by TXID or WTXID #2819

conradoplg opened this issue Oct 1, 2021 · 3 comments · Fixed by #2833
Assignees
Labels
C-enhancement Category: This is an improvement

Comments

@conradoplg
Copy link
Collaborator

conradoplg commented Oct 1, 2021

Motivation

When receiving a mempool-bound transaction we check if it's in the rejected list, and if it is, we avoid downloading/verifying it.

However, this check currently uses UnminedTxId which is the WTXID for V5-onward.

Specifications

https://zips.z.cash/zip-0401

Each node also MUST hold a FIFO queue RecentlyEvicted of pairs (txid, time), where the time indicates when the transaction with the given txid was evicted. The txid (rather than the wtxid ...) is used even for version 5 transactions after activation of NU5.

Designs

For rejected invalid transactions (i.e. transactions that failed verification or storage), Zebra should match:

If it's difficult to distinguish between verifier errors, all verification failures can be matched by WTXID. Then we can open a follow-up ticket to match missing inputs, nullifier conflicts, and Expired by mined_id().

This may require some refactoring to allow looking up rejected transactions by their TXID, or splitting the rejected list in two (evicted and rejected?)

Related Work

Invalid transactions are not currently being added to the rejected list; this is #2818, so that must be done before this one.

@conradoplg conradoplg added C-enhancement Category: This is an improvement S-needs-triage Status: A bug report needs triage labels Oct 1, 2021
@daira
Copy link
Contributor

daira commented Oct 2, 2021

Check if there are any rejected invalid transactions (i.e. transactions that failed verification) with the same WTXID (UnminedTxId).

If the reason for failed verification was a problem with the witness data (i.e. the transaction is otherwise valid), then if it's submitted with different witness data you need to revalidate that. That is, you probably need caches for which txids refer to transactions that are invalidated regardless of witness data, and which wtxids are invalidated by the witness data.

Apologies for the lack of explicit specification here. I'm not entirely sure zcashd does it right; it's something we should check.

@str4d
Copy link
Contributor

str4d commented Oct 2, 2021

zcashd currently has the same split that this issue was opened to create:

  • A Bloom filter that tracks wtxids for invalid transactions.
  • A list that tracks txids for evicted transactions.

We could add another Bloom filter to track txids of transactions that are rejected due to invalid effecting data, but the zcashd consensus stack does not currently make that distinction. I'd also argue it would only provide a modest performance optimisation, as I believe the most costly part of transaction validation is verifying the authorizing data (proofs and signatures), which a txid filter would not prevent.

@teor2345
Copy link
Contributor

teor2345 commented Oct 5, 2021

I unblocked this task by moving the blocked parts into other tickets (ZIP-401 and #2818).

@teor2345 teor2345 self-assigned this Oct 5, 2021
teor2345 added a commit that referenced this issue Oct 5, 2021
Preparation for ticket #2819.

Removing these errors means that we don't have to decide
which type of transaction ID match we want for them.
teor2345 added a commit that referenced this issue Oct 6, 2021
Preparation for ticket #2819.

Removing these errors means that we don't have to decide
which type of transaction ID match we want for them.
@teor2345 teor2345 linked a pull request Oct 6, 2021 that will close this issue
3 tasks
@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Oct 6, 2021
teor2345 added a commit that referenced this issue Oct 6, 2021
Preparation for ticket #2819.

Removing these errors means that we don't have to decide
which type of transaction ID match we want for them.
teor2345 added a commit that referenced this issue Oct 6, 2021
Preparation for ticket #2819.

Removing these errors means that we don't have to decide
which type of transaction ID match we want for them.
teor2345 added a commit that referenced this issue Oct 7, 2021
* Remove unused mempool storage errors

Preparation for ticket #2819.

Removing these errors means that we don't have to decide
which type of transaction ID match we want for them.

* Remove unused mempool errors, and deduplicate storage errors

* rustfmt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: This is an improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants