Reject conflicting mempool transactions #2765

jvff · 2021-09-16T13:53:34Z

Motivation

The mempool should ideally provide a set of transactions that can be included in a block to be mined. This means that it should not contain any conflicting transactions, i.e., transactions that spend the same UTXOs or reveal the same nullifiers.

This closes #2682.

Specifications

Solution

Change the mempool::Storage to check if transactions to be included don't conflict with any existing transactions. Reject transactions that conflict with any existing transactions in the mempool.

Review

Reviewer Checklist

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

Follow Up Work

Consider replacing existing transactions in the mempool with a transaction that has conflicts but has a higher fee?

conradoplg · 2021-09-16T17:13:39Z

I posted this on Discord, but for the record:

I think you need to check if transactions have the same input, not the same output
Terminology seems correct to me
Maybe it's a bit convoluted, but it's easy to understand what is going on since the code is so small. It seems better than having four separate functions... (It's also a bit unfortunate that it needs so many explicit lifetimes, but removing them leads to the error you posted, right?)

teor2345

This PR looks good.

There's a non-blocking performance issue that we will probably want to fix later, once more mempool components are in place.

I'm looking forward to seeing some tests, when the testing framework all comes together.

zebrad/src/components/mempool/storage.rs

zebrad/src/components/mempool/error.rs

zebrad/src/components/mempool/storage.rs

teor2345

I missed some Conflict to SpendConflict tweaks.

zebrad/src/components/mempool/storage.rs

jvff

Updated. Changes can be seen here.

teor2345

Looks great, just one documentation clarification

zebra-chain/src/transaction.rs

teor2345

This looks great!

I opened #2787 to check for duplicate spends within the same mempool transaction.
(Currently, that consensus rule is checked in zebra-state, so it doesn't apply to mempool transactions.)

jvff · 2021-09-22T23:01:10Z

I've implemented a test case, but it's currently blocked by depending on #2771. If that gets merged, I can update this PR and remove the draft status. In the meantime, here's the test code in case you want to take an initial look.

I thought about implementing the same test case using nullifiers for the different pools, but I'm not 100% sure how to create mock transactions with them 😅

teor2345 · 2021-09-23T03:58:53Z

I thought about implementing the same test case using nullifiers for the different pools, but I'm not 100% sure how to create mock transactions with them 😅

It's so complicated, and it works differently for every shielded pool.
So I just used the Arbitrary impl to do it:

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

Lines 460 to 475 in e2cc6e1

    
           #[test] 
        
           fn reject_duplicate_sapling_nullifiers_in_block( 
        
               spend1 in TypeNameToDebug::<sapling::Spend<PerSpendAnchor>>::arbitrary(), 
        
               mut spend2 in TypeNameToDebug::<sapling::Spend<PerSpendAnchor>>::arbitrary(), 
        
               sapling_shielded_data1 in TypeNameToDebug::<sapling::ShieldedData<PerSpendAnchor>>::arbitrary(), 
        
               sapling_shielded_data2 in TypeNameToDebug::<sapling::ShieldedData<PerSpendAnchor>>::arbitrary(), 
        
           ) { 
        
               zebra_test::init(); 
        
               let mut block1 = zebra_test::vectors::BLOCK_MAINNET_1_BYTES 
        
                   .zcash_deserialize_into::<Block>() 
        
                   .expect("block should deserialize"); 
        
               // create a double-spend across two transactions 
        
               let duplicate_nullifier = spend1.nullifier; 
        
               spend2.nullifier = duplicate_nullifier;

Feel free to copy and paste the 3 similar tests for each pool.

teor2345

Looks good, feel free to merge once you've resolved the conflicts.

(As long as the tests run for a reasonable amount of time.)

zebrad/src/components/mempool/storage/tests/prop.rs

Returns an iterator over the UTXO `OutPoint`s spent by the transaction.

An error representing that a transaction was rejected because it conflicts with another transaction that's already in the mempool.

Reject including a transaction in the mempool if it spends outputs already spent by, or reveals nullifiers already revealed by another transaction in the mempool.

Remove the `r` that was incorrectly added. Co-authored-by: teor <teor@riseup.net>

Make the situation clearer, because there are other types of conflict. Co-authored-by: teor <teor@riseup.net>

Because otherwise it could lead to confusion because it could also mean the outputs of the transaction represented as `OutPoint` references. Co-authored-by: teor <teor@riseup.net>

Refactor to follow the convention used for other tests.

A getter to allow changing the first element.

Allow appending elements to the collection.

This is just to make the code that generates arbitrary anchors a bit simpler.

Generate two transactions (either V4 or V5) and insert a conflicting spend, which can be either a transparent UTXO, or a nullifier for one of the shielded pools. Check that any attempt to insert both transactions causes one to be accepted and the other to be rejected.

jvff · 2021-09-26T14:09:10Z

I've updated the PR to fix the merge conflict, fix a clippy lint and add some more documentation. I know you said I could merge this, but I thought the documentation might want another quick round of check. Feel free to change anything and merge it afterwards 👍

The isolated changes can be seen here.

zebrad/src/components/mempool/storage.rs

teor2345

Looks good to me, I just removed an outdated TODO comment

jvff requested a review from a team September 16, 2021 13:53

jvff self-assigned this Sep 16, 2021

jvff force-pushed the reject-conflicting-mempool-transactions branch from 3832ea4 to 22d8ffd Compare September 16, 2021 20:05

teor2345 reviewed Sep 20, 2021

View reviewed changes

zebrad/src/components/mempool/storage.rs Outdated Show resolved Hide resolved

zebrad/src/components/mempool/storage.rs Outdated Show resolved Hide resolved

teor2345 reviewed Sep 20, 2021

View reviewed changes

zebrad/src/components/mempool/storage.rs Outdated Show resolved Hide resolved

jvff force-pushed the reject-conflicting-mempool-transactions branch from 4f1c778 to d94a58f Compare September 21, 2021 01:05

This was referenced Sep 21, 2021

Consider replacing mempool transactions based on the transaction fee #2781

Closed

Refactor spend conflict checks in the mempool storage to increase performance #2784

Closed

jvff force-pushed the reject-conflicting-mempool-transactions branch from d94a58f to 17f217b Compare September 21, 2021 15:17

jvff commented Sep 21, 2021

View reviewed changes

teor2345 reviewed Sep 21, 2021

View reviewed changes

zebra-chain/src/transaction.rs Outdated Show resolved Hide resolved

teor2345 previously approved these changes Sep 21, 2021

View reviewed changes

teor2345 mentioned this pull request Sep 22, 2021

Send mined transaction IDs to the download/verify task for cancellation #2786

Merged

3 tasks

jvff dismissed teor2345’s stale review via 8df91d9 September 22, 2021 22:49

jvff force-pushed the reject-conflicting-mempool-transactions branch from 8df91d9 to 0af839d Compare September 24, 2021 01:22

teor2345 previously approved these changes Sep 24, 2021

View reviewed changes

zebrad/src/components/mempool/storage/tests/prop.rs Show resolved Hide resolved

jvff and others added 10 commits September 24, 2021 11:53

Add Transaction::spent_outpoints getter method

b614d42

Returns an iterator over the UTXO `OutPoint`s spent by the transaction.

Add mempool::Error::Conflict variant

49856a6

An error representing that a transaction was rejected because it conflicts with another transaction that's already in the mempool.

Reject conflicting mempool transactions

2e4dd3b

Reject including a transaction in the mempool if it spends outputs already spent by, or reveals nullifiers already revealed by another transaction in the mempool.

Fix typo in documentation

3778015

Remove the `r` that was incorrectly added. Co-authored-by: teor <teor@riseup.net>

Specify that the conflict is a spend conflict

fcf87cd

Make the situation clearer, because there are other types of conflict. Co-authored-by: teor <teor@riseup.net>

Clarify that the outpoints are from inputs

bc46fcf

Because otherwise it could lead to confusion because it could also mean the outputs of the transaction represented as `OutPoint` references. Co-authored-by: teor <teor@riseup.net>

Create storage::tests::vectors module

9003da8

Refactor to follow the convention used for other tests.

Add an AtLeastOne::first_mut method

610b040

A getter to allow changing the first element.

Add an AtLeastOne::push method

fe8836c

Allow appending elements to the collection.

Derive Arbitrary for FieldNotPresent

4f05b32

This is just to make the code that generates arbitrary anchors a bit simpler.

jvff dismissed teor2345’s stale review via c87b067 September 24, 2021 12:05

jvff force-pushed the reject-conflicting-mempool-transactions branch from 0af839d to c87b067 Compare September 24, 2021 12:05

jvff marked this pull request as ready for review September 24, 2021 15:09

jvff requested a review from teor2345 September 27, 2021 10:32

teor2345 reviewed Sep 28, 2021

View reviewed changes

zebrad/src/components/mempool/storage.rs Outdated Show resolved Hide resolved

teor2345 added 2 commits September 28, 2021 10:24

Delete a TODO comment that we decided not to do

a56ff45

Merge branch 'main' into reject-conflicting-mempool-transactions

f1e01be

teor2345 enabled auto-merge (squash) September 28, 2021 00:24

teor2345 approved these changes Sep 28, 2021

View reviewed changes

teor2345 mentioned this pull request Sep 28, 2021

Cancel download and verify tasks when the mempool is deactivated #2764

Merged

3 tasks

teor2345 merged commit a0d45c3 into ZcashFoundation:main Sep 28, 2021

jvff deleted the reject-conflicting-mempool-transactions branch September 28, 2021 12:01

jvff mentioned this pull request Oct 5, 2021

Refactor mempool spend conflict checks to increase performance #2826

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reject conflicting mempool transactions #2765

Reject conflicting mempool transactions #2765

jvff commented Sep 16, 2021 •

edited

Loading

conradoplg commented Sep 16, 2021

teor2345 left a comment

teor2345 left a comment

jvff left a comment

teor2345 left a comment

teor2345 left a comment

jvff commented Sep 22, 2021 •

edited

Loading

teor2345 commented Sep 23, 2021 •

edited

Loading

teor2345 left a comment

jvff commented Sep 26, 2021

teor2345 left a comment

Reject conflicting mempool transactions #2765

Reject conflicting mempool transactions #2765

Conversation

jvff commented Sep 16, 2021 • edited Loading

Motivation

Specifications

Solution

Review

Reviewer Checklist

Follow Up Work

conradoplg commented Sep 16, 2021

teor2345 left a comment

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

jvff left a comment

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

jvff commented Sep 22, 2021 • edited Loading

teor2345 commented Sep 23, 2021 • edited Loading

teor2345 left a comment

Choose a reason for hiding this comment

jvff commented Sep 26, 2021

teor2345 left a comment

Choose a reason for hiding this comment

jvff commented Sep 16, 2021 •

edited

Loading

jvff commented Sep 22, 2021 •

edited

Loading

teor2345 commented Sep 23, 2021 •

edited

Loading