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(rpc): Select getblocktemplate RPC transactions according to ZIP-317 #5724

Merged
merged 7 commits into from
Dec 1, 2022

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Nov 25, 2022

Motivation

We want to select transactions for block production according to ZIP-317.

Close #5473.

Depends-On: #5761

Specifications

https://zips.z.cash/zip-0317#recommended-algorithm-for-block-template-construction

Designs

Follow the recommended algorithm in the specification.
Use f32 for efficient calculations and weight storage.

Use a minimum fee weight of 1.0 / 20.0 to simplify the implementation. (This is not in the spec, but changes are allowed because it is not consensus-critical. If the ZIP editors disagree with this, I'll open another PR to fix it.)

Use the same random weight APIs that we use in mempool transaction eviction.

Some parts of this PR are outside the getblocktemplate-rpcs feature, because we will probably use them in ticket #5336.

Solution

  • Add a block_production_fee_weight field to VerifiedUnminedTx
  • Add a custom Zebra minimum transaction weight for block production
  • Randomly select transactions for block production according to ZIP-317

Related Refactors:

  • Move getblocktemplate transaction selection into a new zip317 module
  • Split the conventional fee check into its own method

This PR has some testing from the existing test vectors and snapshot tests.
The rest of its tests will be implemented as part of ticket #5652.

Review

We should probably get two reviews on this.

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?

@teor2345 teor2345 added A-dependencies Area: Dependency file updates C-enhancement Category: This is an improvement P-Medium ⚡ C-security Category: Security issues I-remote-node-overload Zebra can overload other nodes on the network A-rpc Area: Remote Procedure Call interfaces labels Nov 25, 2022
@teor2345 teor2345 self-assigned this Nov 25, 2022
@codecov
Copy link

codecov bot commented Nov 25, 2022

Codecov Report

Merging #5724 (b1c03e5) into main (63124ba) will increase coverage by 0.41%.
The diff coverage is 94.73%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5724      +/-   ##
==========================================
+ Coverage   78.67%   79.09%   +0.41%     
==========================================
  Files         306      306              
  Lines       38552    39768    +1216     
==========================================
+ Hits        30331    31454    +1123     
- Misses       8221     8314      +93     

@teor2345 teor2345 marked this pull request as ready for review November 27, 2022 23:05
@teor2345 teor2345 requested review from a team as code owners November 27, 2022 23:05
@teor2345 teor2345 requested review from oxarbitrage and dconnolly and removed request for a team November 27, 2022 23:05
@mpguerra mpguerra requested review from arya2 and removed request for dconnolly November 30, 2022 09:25
@teor2345
Copy link
Contributor Author

The code in this PR will have further changes after a ZIP update. I suggest we review and merge it based on the current ZIP-317, and then do the update in a separate PR based on ticket #5752.

That will make reviewing the update easier, because the diff will be smaller.

@teor2345 teor2345 closed this Nov 30, 2022
@teor2345 teor2345 reopened this Nov 30, 2022
@teor2345
Copy link
Contributor Author

(Oops, pressed the wrong button.)

Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

Looks good to me.

zebra-chain/src/transaction/unmined/zip317.rs Show resolved Hide resolved
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.

This looks good!

@teor2345
Copy link
Contributor Author

teor2345 commented Dec 1, 2022

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Dec 1, 2022

refresh

✅ Pull request refreshed

mergify bot added a commit that referenced this pull request Dec 1, 2022
@daira
Copy link
Contributor

daira commented Dec 1, 2022

Note that the algorithm has changed, as discussed in ZIP sync. PR here: zcash/zips#650

@teor2345
Copy link
Contributor Author

teor2345 commented Dec 1, 2022

Please don't worry about restarting the tests in this PR, or merging in main, I'm going to update the code tomorrow anyway.

mergify bot added a commit that referenced this pull request Dec 1, 2022
@mergify mergify bot merged commit d778cae into main Dec 1, 2022
@mergify mergify bot deleted the zip-317-getblocktemplate branch December 1, 2022 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependencies Area: Dependency file updates A-rpc Area: Remote Procedure Call interfaces 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.

Select getblocktemplate transactions according to ZIP-317
4 participants