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

change(mempool): Evict transactions from the mempool using the ZIP-317 conventional fee #5703

Merged
merged 9 commits into from
Nov 24, 2022

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Nov 23, 2022

Motivation

We want to implement the ZIP-317 conventional fee calculation, and use it to evict transactions from the mempool.

This is a security fix independent of the getblocktemplate work.
(But we also use the conventional fee calculation when selecting transactions for new blocks.)

Close #5335.

Specifications

https://zips.z.cash/zip-0317#fee-calculation
https://zips.z.cash/zip-0317#mempool-size-limiting

This change can be deployed immediately:
https://zips.z.cash/zip-0317#deployment

Designs

Do the fee calculation in the specified order, using similar names for everything:
https://zips.z.cash/zip-0317#fee-calculation

Cache the result as a field on UnminedTx, because it only changes when the transaction data changes. This way, it is available during block construction and transaction relaying.

This is a standard rule, not a consensus rule, so variations in the implementation are acceptable.

Solution

  • Add a ZIP-317 conventional fee module
  • Add ZIP-317 constants
  • Calculate the ZIP-317 conventional fee
  • Use the new conventional fee for mempool size limiting
  • Update existing tests

Related changes:

  • Just return a usize from zcash_serialized_size(), removing the unused Result<_, io::Error>
    • This makes the new code a lot simpler

Review

I'd like two people to review this code, including @dconnolly because she helped write the spec.

Here is the corresponding code in zcash_primitives (it doesn't have Orchard support yet):
https://github.com/zcash/librustzcash/blob/main/zcash_primitives/src/transaction/fees/zip317.rs#L117

Do we need any extra tests?

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?

Follow Up Work

The changes to transaction relaying and block production depend on enough wallets updating to pay these fees:
https://zips.z.cash/zip-0317#deployment

So they have separate tickets:

@teor2345 teor2345 added A-rust Area: Updates to Rust code P-Medium ⚡ C-security Category: Security issues I-remote-node-overload Zebra can overload other nodes on the network labels Nov 23, 2022
@teor2345 teor2345 self-assigned this Nov 23, 2022
@teor2345 teor2345 requested a review from a team as a code owner November 23, 2022 03:59
@teor2345 teor2345 requested review from dconnolly and arya2 and removed request for a team November 23, 2022 03:59
@github-actions github-actions bot added the C-enhancement Category: This is an improvement label Nov 23, 2022
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Nov 23, 2022
@codecov
Copy link

codecov bot commented Nov 23, 2022

Codecov Report

Merging #5703 (a111fd6) into main (d5597f8) will increase coverage by 0.05%.
The diff coverage is 93.05%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5703      +/-   ##
==========================================
+ Coverage   78.73%   78.78%   +0.05%     
==========================================
  Files         305      306       +1     
  Lines       38503    38552      +49     
==========================================
+ Hits        30314    30372      +58     
+ Misses       8189     8180       -9     

Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

Code looks clean and correct, just needs minor updates to a couple doc comments.

I like that zcash_serialized_size was updated to return a usize.

zebra-chain/src/transaction/unmined/zip317.rs Show resolved Hide resolved
teor2345 and others added 2 commits November 24, 2022 06:56
Co-authored-by: Daira Hopwood <daira@jacaranda.org>
Co-authored-by: Arya <aryasolhi@gmail.com>
@teor2345 teor2345 requested a review from arya2 November 23, 2022 21:00
Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

Thank you!

@teor2345 teor2345 removed the request for review from dconnolly November 23, 2022 22:27
@teor2345 teor2345 removed the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Nov 23, 2022
mergify bot added a commit that referenced this pull request Nov 23, 2022
mergify bot added a commit that referenced this pull request Nov 23, 2022
@mergify mergify bot merged commit 63124ba into main Nov 24, 2022
@mergify mergify bot deleted the zip-317-mempool branch November 24, 2022 01:27
teor2345 added a commit that referenced this pull request Feb 6, 2023
…7 conventional fee (#5703) - manual fix changelog

* Add a ZIP-317 conventional fee module

* Add a conventional fee calculation stub, and use it for mempool size limiting

* Just return a usize from zcash_serialized_size(), removing the unused Result

* Add ZIP-317 constants

* Calculate the ZIP-317 conventional fee

* Update tests

* Add a CHANGELOG entry

* Fix a comment typo

Co-authored-by: Daira Hopwood <daira@jacaranda.org>

* Fix some missing words in a comment

Co-authored-by: Arya <aryasolhi@gmail.com>

Co-authored-by: Daira Hopwood <daira@jacaranda.org>
Co-authored-by: Arya <aryasolhi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement C-security Category: Security issues I-remote-node-overload Zebra can overload other nodes on the network
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Evict transactions from the mempool based on the ZIP-317 fee rules
3 participants