Skip to content

Remove duplicate transaction check from tryAddTxs in mempool #1806

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 9, 2020
Merged

Remove duplicate transaction check from tryAddTxs in mempool #1806

merged 4 commits into from
Apr 9, 2020

Conversation

intricate
Copy link
Contributor

Closes #1801

@intricate intricate added the consensus issues related to ouroboros-consensus label Mar 13, 2020
@intricate intricate added this to the S9 2020-03-26 milestone Mar 13, 2020
@intricate intricate requested review from mrBliss, edsko and dcoutts March 13, 2020 15:45
@intricate intricate self-assigned this Mar 13, 2020
intricate
intricate commented Mar 13, 2020
View reviewed changes
ouroboros-consensus/src/Ouroboros/Consensus/Mempool/Impl.hs
-- Note that we don't treat this as an error/rejection case.
| implSnapshotHasTx is (txId firstTx)
= return $ go
((firstTx, MempoolTxAdded):acc)
Copy link
Contributor Author

@intricate intricate Mar 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It seems that I originally made a mistake here actually. This should have been MempoolTxAlreadyInMempool rather than MempoolTxAdded.

I also need to remove the MempoolTxAlreadyInMempool constructor of MempoolAddTxResult as part of this PR since we don't use it. Done.

@mrBliss mrBliss modified the milestones: S9 2020-03-26, S10 2020-04-09 Mar 27, 2020
@intricate intricate force-pushed the intricate/1801 branch 4 times, most recently from 92f316d to 4b12297 Compare April 1, 2020 15:31
@intricate
Copy link
Contributor Author

Just noting that this PR is waiting on IntersectMBO/cardano-ledger#759 to be resolved.

This is because the prop_simple_real_pbft_convergence-related tests expect that duplicate transactions are not accepted and, after this PR removes the duplicate tx check from the mempool, we now rely on the ledger rules to ensure this. However, as was discovered in IntersectMBO/cardano-ledger#759, it is possible for certain duplicate update proposals to be applied to the ledger state without being rejected.

@intricate intricate force-pushed the intricate/1801 branch 2 times, most recently from 21a711f to 9432bf2 Compare April 7, 2020 15:17
@intricate
Copy link
Contributor Author

Using IntersectMBO/cardano-ledger#766, the tests on this PR run and pass as expected.

Once that PR is merged, this should be good to go.

@intricate
Copy link
Contributor Author

IntersectMBO/cardano-ledger#766 has now been merged.

edsko
edsko approved these changes Apr 9, 2020
View reviewed changes
Copy link
Contributor

@edsko edsko left a comment

Choose a reason for hiding this comment

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

Approving but under protest :)

@intricate
Copy link
Contributor Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Apr 9, 2020

Build succeeded

@iohk-bors iohk-bors bot merged commit 6d5873a into master Apr 9, 2020
@iohk-bors iohk-bors bot deleted the intricate/1801 branch April 9, 2020 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edsko edsko edsko approved these changes

@mrBliss mrBliss Awaiting requested review from mrBliss

@dcoutts dcoutts Awaiting requested review from dcoutts

Assignees

@intricate intricate

Labels
consensus issues related to ouroboros-consensus
Projects
None yet
Milestone
S10 2020-04-09
Development

Successfully merging this pull request may close these issues.

Remove dup tx check from mempool
3 participants
@intricate @edsko @mrBliss