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 storage errors by match type: TXID or WTXID #2833

Merged
merged 3 commits into from
Oct 7, 2021

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Oct 6, 2021

Motivation

ZIP-401 requires mempools to match some transaction rejections by transaction::Hash, rather than UnminedTxId.

Zebra already implements some rejections that should be matched by transaction::Hash. So we can modify our existing matches, and prepare for ZIP-401.

Closes #2819.

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:

Solution

  • match SpendConflict, Expired, and RandomlyEvicted by transaction::Hash
  • match FailedVerification by UnminedTxId

Review

I'd like an initial review from @conradoplg and @upbqdn, because they're implementing the related ZIP-401 tickets.

Does this design work for you?
Is this a minimal change?

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

We might want to split FailedVerification into effects failures and authorization failures, see #2834.
But this change is not required for a minimal mempool implementation.

@teor2345 teor2345 added A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement P-Medium labels Oct 6, 2021
@teor2345 teor2345 requested review from conradoplg and upbqdn October 6, 2021 03:23
@teor2345 teor2345 self-assigned this Oct 6, 2021
@teor2345 teor2345 linked an issue Oct 6, 2021 that may be closed by this pull request
@conradoplg
Copy link
Collaborator

I like it!

For #2821 I think we'll need to split this even more:

  • rejected_exact (invalid) (cleared on grow)
  • rejected_same_effects:
    • expired (cleared on reset)
    • spend_conflict (cleared on grow)
    • excess (cleared on ZIP-401 timeout)

Should we do this in this PR, or in a following one?

@teor2345
Copy link
Contributor Author

teor2345 commented Oct 6, 2021

For #2821 I think we'll need to split this even more:

* `rejected_exact` (`invalid`) (cleared on `grow`)

* `rejected_same_effects`:
  
  * `expired` (cleared on `reset`)
  * `spend_conflict` (cleared on `grow`)
  * `excess` (cleared on ZIP-401 timeout)

Should we do this in this PR, or in a following one?

I was planning on doing some of that split in #2694, and then whatever is left over we can do in #2821.

@teor2345 teor2345 marked this pull request as ready for review October 6, 2021 20:47
@teor2345

This comment has been minimized.

@teor2345 teor2345 force-pushed the mempool-tx-id-match branch from 3a0a315 to b6b1810 Compare October 6, 2021 22:03
@teor2345 teor2345 force-pushed the mempool-unused-errors branch 2 times, most recently from ec8614b to e515fc9 Compare October 6, 2021 23:48
Base automatically changed from mempool-unused-errors to main October 7, 2021 01:20
@teor2345 teor2345 force-pushed the mempool-tx-id-match branch from b6b1810 to 1570afb Compare October 7, 2021 02:24
@teor2345 teor2345 changed the title Split storage errors into same effects and exact matches Split storage errors by match type: TXID or WTXID Oct 7, 2021
Copy link
Collaborator

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

Looks good!

@conradoplg conradoplg enabled auto-merge (squash) October 7, 2021 14:44
@conradoplg conradoplg merged commit 964c819 into main Oct 7, 2021
@conradoplg conradoplg deleted the mempool-tx-id-match branch October 7, 2021 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Match rejected transactions correctly by TXID or WTXID
2 participants