Commit Graph

2 Commits

Author SHA1 Message Date
Conrado Gouvea 46fb33a04f
Limit the size and age of the ZIP-401 rejected transaction ID list (#2932)
* Limit the size and age of the ZIP-401 rejected transaction ID list

* Apply suggestions from code review

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

* Fix bug in EvictionList; improve documentation

* Separate public and non-public parts of the documentation

* Fix tests

* Apply suggestions from code review

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

* Fix bug in EvictionList::len()

* Make EvictionList::len() mutable; prune the list inside it

* Limit the size of EvictedList::ordered_entries

* Increase eviction_list_time_mixed time constants to try to make it pass on MacOS

* Simplify logic by assuming refreshes will never happen

* Apply suggestions from code review

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

* Compiling fixes

* Remove MEMPOOL_SIZE and just rely on the ZIP-401 cost limit

Co-authored-by: teor <teor@riseup.net>
2021-10-27 20:27:00 +00:00
Janito Vaqueiro Ferreira Filho 9e78a8af40
Refactor mempool spend conflict checks to increase performance (#2826)
* Add `HashSet`s to help spend conflict detection

Keep track of the spent transparent outpoints and the revealed
nullifiers.

Clippy complained that the `ActiveState` had variants with large size
differences, but that was expected, so I disabled that lint on that
`enum`.

* Clear the `HashSet`s when clearing the mempool

Clear them so that they remain consistent with the set of verified
transactions.

* Use `HashSet`s to check for spend conflicts

Store new outputs into its respective `HashSet`, and abort if a
duplicate output is found.

* Remove inserted outputs when aborting

Restore the `HashSet` to its previous state.

* Remove tracked outputs when removing a transaction

Keep the mempool storage in a consistent state when a transaction is
removed.

* Remove tracked outputs when evicting from mempool

Ensure eviction also keeps the tracked outputs consistent with the
verified transactions.

* Refactor to create a `VerifiedSet` helper type

Move the code to handle the output caches into the new type. Also move
the eviction code to make things a little simpler.

* Refactor to have a single `remove` method

Centralize the code that handles the removal of a transaction to avoid
mistakes.

* Move mempool size limiting back to `Storage`

Because the evicted transactions must be added to the rejected list.

* Remove leftover `dbg!` statement

Leftover from some temporary testing code.

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

* Remove unnecessary `TODO`

It is more speculation than planning, so it doesn't add much value.

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

* Fix typo in documentation

The verb should match the subject "transactions" which is plural.

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

* Add a comment to warn about correctness

There's a subtle but important detail in the implementation that should
be made more visible to avoid mistakes in the future.

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

* Remove outdated comment

Left-over from the attempt to move the eviction into the `VerifiedSet`.

* Improve comment explaining lint removal

Rewrite the comment explaining why the Clippy lint was ignored.

* Check for spend conflicts in `VerifiedSet`

Refactor to avoid API misuse.

* Test rejected transaction rollback

Using two transactions, perform the same test adding a conflict to both
of them to check if the second inserted transaction is properly
rejected. Then remove any conflicts from the second transaction and add
it again. That should work, because if it doesn't it means that when the
second transaction was rejected it left things it shouldn't in the
cache.

* Test removal of multiple transactions

When removing multiple transactions from the mempool storage, all of the
ones requested should be removed and any other transaction should be
still be there afterwards.

* Increase mempool size to 4, so that spend conflict tests work

If the mempool size is smaller than 4,
these tests don't fail on a trivial removal bug.
Because we need a minimum number of transactions in the mempool
to trigger the bug.

Also commit a proptest seed that fails on a trivial removal bug.
(This seed fails if we remove indexes in order,
because every index past the first removes the wrong transaction.)

* Summarise transaction data in proptest error output

* Summarise spend conflict field data in proptest error output

* Summarise multiple removal field data in proptest error output

And replace the very large proptest debug output with the new summary.

Co-authored-by: teor <teor@riseup.net>
2021-10-10 23:54:46 +00:00