change(mempool): Contextually validates mempool transactions in best chain #5716

arya2 · 2022-11-24T19:53:36Z

Motivation

Mempool transactions collected into a block template need to be contextually valid in the best chain to produce a block template that's contextually valid on the best chain.

Closes #5376

Solution

Add check::nullifiers::no_duplicates_in_chain that searches the non-finalized state as well as the finalized state for duplicate nullifiers
Update check::anchors fns and errors accommodate transactions without a PreparedBlock
Add new state request to check that the anchors and nullifiers of a transaction are valid on the best chain
Use the new state request in the transactions' verifier for mempool transactions

Review

This PR is part of regular scheduled work.

Reviewer Checklist

Will the PR name make sense to users?
- Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
Are the PR labels correct?
Does the code do what the ticket and PR says?
How do you know it works? Does it have tests?

- check::anchors fn for tx iter - TODO comments for unifying nullifiers and anchors checks - new state request Updates unknown anchor errors to accomodate tx-only check Calls new state fn from transaction verifier

updates TransactionContextualValidity request to check sprout anchors adds comment mentioning TransactionContextualValidity ignores UTXOs

updates tests

teor2345

Looks great, seems like it should work!

Since the mempool currently has a lot of very large transactions, we need to be careful of performance. I've also made some suggestions for improving various APIs so they are easier to call correctly.

I'd like to see some unit tests for the new and modified consensus-critical checks.

zebra-consensus/src/transaction/tests.rs

zebra-state/src/request.rs

zebra-consensus/src/transaction.rs

zebra-state/src/response.rs

zebra-state/src/service/check/anchors.rs

zebrad/src/components/mempool/downloads.rs

zebra-state/src/service/check/nullifier.rs

Co-authored-by: teor <teor@riseup.net>

moves if let statement into for loop

renames transaction_in_state to transaction_in_chain

deletes broken test

codecov · 2022-11-25T21:07:47Z

Codecov Report

Merging #5716 (a01ab07) into main (63124ba) will increase coverage by 0.12%.
The diff coverage is 53.12%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5716      +/-   ##
==========================================
+ Coverage   78.67%   78.79%   +0.12%     
==========================================
  Files         306      306              
  Lines       38552    38676     +124     
==========================================
+ Hits        30331    30475     +144     
+ Misses       8221     8201      -20

- comments / docs - a TransactionError variant for ValidateContextError

teor2345

Looks good, I just have some questions and ideas around error handling.

Good catch with the expensive Sprout checks!

zebra-consensus/src/transaction.rs

zebra-state/src/request.rs

zebra-state/src/service/check/anchors.rs

zebrad/src/components/mempool/downloads.rs

zebra-consensus/src/error.rs

zebra-state/src/service/check/anchors.rs

Co-authored-by: teor <teor@riseup.net>

rustfmt

updates comments and naming

slightly improves formatting

teor2345

Looking good, I don't mind if we do the joinsplit parallelisation in a different PR.

(But it's important to do soon, because it is a denial of service risk.)

zebra-consensus/src/error.rs

zebra-state/src/service/check/anchors.rs

Co-authored-by: teor <teor@riseup.net>

arya2 · 2022-11-29T18:44:28Z

Any idea what "the trait bound zcash_history::Error: Clone is not satisfied" was about?

Worked fine locally. 😢

teor2345 · 2022-11-29T23:28:32Z

Any idea what "the trait bound zcash_history::Error: Clone is not satisfied" was about?

Worked fine locally. 😢

As far as I can see, zcash_history::Error does not impl Clone:
https://docs.rs/zcash_history/latest/zcash_history/enum.Error.html
https://github.com/zcash/librustzcash/blob/580f87dc3ec5d12a449b437a0358228b0fe936f3/zcash_history/src/lib.rs#L20-L26

teor2345 · 2022-11-29T23:28:51Z

Feel free to wrap it in an Arc.

teor2345

Thanks, looks good!

arya2 · 2022-11-30T00:35:35Z

As far as I can see, zcash_history::Error does not impl Clone

Well, that would do it. I wonder how this happened:

Oh well.

…chain (#5716) - manual conflicts with getblocktemplate-rpcs * updates comments * adds check nullifier no dup fns for transactions * Adds: - check::anchors fn for tx iter - TODO comments for unifying nullifiers and anchors checks - new state request Updates unknown anchor errors to accomodate tx-only check Calls new state fn from transaction verifier * updates check::anchors fns to use transactions updates TransactionContextualValidity request to check sprout anchors adds comment mentioning TransactionContextualValidity ignores UTXOs * conditions new state req call on is_mempool updates tests * fix doc link / lint error * checks for duplicate nullifiers with closures * Update zebra-state/src/service/check/nullifier.rs Co-authored-by: teor <teor@riseup.net> * documents find_duplicate_nullifier params moves if let statement into for loop * renames new state req/res * asserts correct response variant in tx verifier * adds CheckBestChainTipShieldedSpends call in tx verifier to async checks * re-adds tracing instrumentation to check::anchors fn renames transaction_in_state to transaction_in_chain * adds block/tx wrapper fns for anchors checks * uses UnminedTx instead of transaction.hash() deletes broken test * updates new state req/res name * updates tests and uses par_iter for anchors checks * Updates check::anchors pub fn docs. * Adds: - comments / docs - a TransactionError variant for ValidateContextError * Apply suggestions from code review Co-authored-by: teor <teor@riseup.net> * moves downcast to From impl rustfmt * moves the ValidateContextError into an Arc updates comments and naming * leaves par_iter for another PR * puts io::Error in an Arc * updates anchors tests to call tx_anchors check * updates tests to call tx_no_duplicates_in_chain slightly improves formatting * Update zebra-consensus/src/error.rs Co-authored-by: teor <teor@riseup.net> * moves Arc from HistoryError to ValidateContextError Co-authored-by: teor <teor@riseup.net>

arya2 added 5 commits November 24, 2022 14:53

updates comments

5011db0

adds check nullifier no dup fns for transactions

430ed61

Adds:

0789aec

- check::anchors fn for tx iter - TODO comments for unifying nullifiers and anchors checks - new state request Updates unknown anchor errors to accomodate tx-only check Calls new state fn from transaction verifier

updates check::anchors fns to use transactions

72acc4d

updates TransactionContextualValidity request to check sprout anchors adds comment mentioning TransactionContextualValidity ignores UTXOs

conditions new state req call on is_mempool

ad30194

updates tests

arya2 added C-bug Category: This is a bug A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code P-Medium ⚡ A-state Area: State / database changes labels Nov 24, 2022

arya2 requested review from a team as code owners November 24, 2022 19:53

arya2 requested review from teor2345 and removed request for a team November 24, 2022 19:53

github-actions bot added the C-enhancement Category: This is an improvement label Nov 24, 2022

arya2 mentioned this pull request Nov 24, 2022

Suggestion: use closures for finding duplicate nullifiers with/without non-finalized state #5717

Merged

5 tasks

arya2 self-assigned this Nov 24, 2022

teor2345 removed the request for review from a team November 24, 2022 20:03

fix doc link / lint error

616e826

teor2345 suggested changes Nov 24, 2022

View reviewed changes

arya2 and others added 10 commits November 24, 2022 17:38

checks for duplicate nullifiers with closures

1b12334

Update zebra-state/src/service/check/nullifier.rs

6295a8e

Co-authored-by: teor <teor@riseup.net>

documents find_duplicate_nullifier params

4cad214

moves if let statement into for loop

renames new state req/res

2e48738

asserts correct response variant in tx verifier

03eba19

adds CheckBestChainTipShieldedSpends call in tx verifier to async checks

be723fe

re-adds tracing instrumentation to check::anchors fn

65f39ba

renames transaction_in_state to transaction_in_chain

adds block/tx wrapper fns for anchors checks

00c2a4b

uses UnminedTx instead of transaction.hash()

55232d1

deletes broken test

updates new state req/res name

3f5efe3

arya2 added 2 commits November 25, 2022 14:51

updates tests and uses par_iter for anchors checks

c02a5bf

Updates check::anchors pub fn docs.

5e1f103

Adds:

115cd6a

- comments / docs - a TransactionError variant for ValidateContextError

teor2345 reviewed Nov 28, 2022

View reviewed changes

arya2 and others added 7 commits November 28, 2022 12:49

Apply suggestions from code review

c5c88a0

Co-authored-by: teor <teor@riseup.net>

moves downcast to From impl

42352a5

rustfmt

moves the ValidateContextError into an Arc

5a064c0

updates comments and naming

leaves par_iter for another PR

84aa68f

puts io::Error in an Arc

6796fa1

updates anchors tests to call tx_anchors check

1b0d532

updates tests to call tx_no_duplicates_in_chain

513b811

slightly improves formatting

arya2 requested a review from a team as a code owner November 29, 2022 00:45

arya2 force-pushed the mempool-context-valid branch from 388a3f4 to 513b811 Compare November 29, 2022 01:00

teor2345 reviewed Nov 29, 2022

View reviewed changes

zebra-consensus/src/error.rs Outdated Show resolved Hide resolved

zebra-state/src/service/check/anchors.rs Show resolved Hide resolved

Update zebra-consensus/src/error.rs

827ac57

Co-authored-by: teor <teor@riseup.net>

moves Arc from HistoryError to ValidateContextError

a01ab07

teor2345 approved these changes Nov 29, 2022

View reviewed changes

teor2345 mentioned this pull request Nov 29, 2022

change(state): Check block and transaction Sprout anchors in parallel #5742

Merged

5 tasks

mergify bot added a commit that referenced this pull request Nov 30, 2022

Merge of #5716

396fbc6

mergify bot mentioned this pull request Nov 30, 2022

merge-queue: embarking main (035fad5) and [#5733 + #5716] together #5746

Closed

mergify bot merged commit eb0a2ef into main Nov 30, 2022

mergify bot deleted the mempool-context-valid branch November 30, 2022 04:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change(mempool): Contextually validates mempool transactions in best chain #5716

change(mempool): Contextually validates mempool transactions in best chain #5716

arya2 commented Nov 24, 2022 •

edited

Loading

teor2345 left a comment •

edited

Loading

codecov bot commented Nov 25, 2022 •

edited

Loading

teor2345 left a comment

teor2345 left a comment

arya2 commented Nov 29, 2022 •

edited

Loading

teor2345 commented Nov 29, 2022

teor2345 commented Nov 29, 2022

teor2345 left a comment

arya2 commented Nov 30, 2022

change(mempool): Contextually validates mempool transactions in best chain #5716

change(mempool): Contextually validates mempool transactions in best chain #5716

Conversation

arya2 commented Nov 24, 2022 • edited Loading

Motivation

Solution

Review

Reviewer Checklist

teor2345 left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Nov 25, 2022 • edited Loading

Codecov Report

teor2345 left a comment

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

arya2 commented Nov 29, 2022 • edited Loading

teor2345 commented Nov 29, 2022

teor2345 commented Nov 29, 2022

teor2345 left a comment

Choose a reason for hiding this comment

arya2 commented Nov 30, 2022

arya2 commented Nov 24, 2022 •

edited

Loading

teor2345 left a comment •

edited

Loading

codecov bot commented Nov 25, 2022 •

edited

Loading

arya2 commented Nov 29, 2022 •

edited

Loading