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.

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

change(rpc): Update ZIP-317 transaction selection algorithm #5776

Merged
merged 10 commits into from
Dec 8, 2022
Merged

change(rpc): Update ZIP-317 transaction selection algorithm #5776

merged 10 commits into from
Dec 8, 2022

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Dec 2, 2022 •
edited
Loading

Motivation

The previous transaction selection algorithm was easy to work around. So we want to implement a simpler one instead.

Close #5752.

Specifications

https://github.com/zcash/zips/blob/a9ef04852749027e515b258dc60c1fff4f87e5e2/zip-0317.rst#block-production

Designs

Similar to the previous design, but update the calculations.

Solution

  • Update transaction weight ratio calculation
  • Add unpaid actions calculation
  • Update transaction selection

Related changes:

  • Rename fields
  • Refactor and simplify code

Review

Anyone can review this code, it's a routine change.

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 C-enhancement Category: This is an improvement P-Medium ⚡ A-rpc Area: Remote Procedure Call interfaces labels Dec 2, 2022
@teor2345 teor2345 self-assigned this Dec 2, 2022
@teor2345 teor2345 requested a review from a team as a code owner December 2, 2022 01:48
@teor2345 teor2345 requested review from dconnolly and removed request for a team December 2, 2022 01:48
@teor2345 teor2345 mentioned this pull request Dec 2, 2022
Merged
5 tasks
@codecov
Copy link

codecov bot commented Dec 2, 2022 •
edited
Loading

Codecov Report

Merging #5776 (e60f386) into main (678c519) will decrease coverage by 0.07%.
The diff coverage is 94.11%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #5776      +/-   ##
==========================================
- Coverage   78.82%   78.75%   -0.08%     
==========================================
  Files         307      307              
  Lines       38737    38773      +36     
==========================================
- Hits        30536    30535       -1     
- Misses       8201     8238      +37

@teor2345 teor2345 added C-security Category: Security issues I-remote-node-overload Zebra can overload other nodes on the network labels Dec 4, 2022
@teor2345
Copy link
Contributor Author

teor2345 commented Dec 5, 2022

Since Friday I added commit f8d781f, which subtracts the coinbase transaction from the block limits.

@teor2345 teor2345 marked this pull request as draft December 5, 2022 07:56
@teor2345
Copy link
Contributor Author

teor2345 commented Dec 5, 2022

This change panics in debug mode, and returns wrong but irrelevant fake coinbase block subsidies in release mode.

@teor2345 teor2345 marked this pull request as ready for review December 5, 2022 08:29
@teor2345
Copy link
Contributor Author

teor2345 commented Dec 5, 2022

This change panics in debug mode, and returns wrong but irrelevant fake coinbase block subsidies in release mode.

Fixed in PR #5787.

@teor2345 teor2345 changed the base branch from main to block-subsidy-panic December 6, 2022 23:10
arya2
arya2 previously approved these changes Dec 7, 2022
View reviewed changes
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.

Looks great!

Nice catch subtracting the coinbase transaction from the block sigop and size limits.

zebra-chain/src/transaction/unmined/zip317.rs Outdated Show resolved Hide resolved
zebra-chain/src/transaction/unmined/zip317.rs Outdated Show resolved Hide resolved
zebra-rpc/src/methods/get_block_template_rpcs/zip317.rs Show resolved Hide resolved
zebra-rpc/src/methods/get_block_template_rpcs.rs Show resolved Hide resolved
zebra-rpc/src/methods/get_block_template_rpcs/zip317.rs Show resolved Hide resolved
zebra-rpc/src/methods/get_block_template_rpcs/zip317.rs Show resolved Hide resolved
@teor2345 teor2345 requested review from a team as code owners December 7, 2022 20:11
@teor2345
Copy link
Contributor Author

teor2345 commented Dec 7, 2022

Oops, too many commits in that last push 😶‍🌫️

@teor2345 teor2345 requested review from arya2 and removed request for dconnolly, a team and oxarbitrage December 7, 2022 20:19
arya2
arya2 previously approved these changes Dec 7, 2022
View reviewed changes
@teor2345
Copy link
Contributor Author

teor2345 commented Dec 7, 2022

Network timeout:

docker: error pulling image configuration: Get "https://us-docker.pkg.dev/artifacts-downloads/namespaces/zealous-zebra/repositories/zebra/downloads/AHi26s-xGU31nIfup2vjJB8eIGmyJtAUNrsSK9rHXtgvD5msfz9Sxl804rwnUBJ9t3t5-jOTHeGk8rBzY7oXXIXLB52DGXiGSIlAOBHU1jn-xxlmI9r4cljGDzvo4H8RINxkt_f1odo60aiO2k3HvZNMgauQFJQtWpWA6J4DMaaXrq5wqSnDiQZaIyE9G7OL7yNpb54PioeKSHib2DLOVmwaNFIrlNV5h2zk47LcdZfv4ATlN72oMLPjDanidvCvM5kXv2BYAS62b67vJHGRVw240yzIaAeXaSqwHR8usD_zJoY-DRxQcrest26_hVw8Gqr2ECsWNHB8HPqtb36mVDoQr_W04IVCY56PcKEu4SJlr6ZuIiZSMuNpahjGqJO8qeEZXEGah99XQxSPIMLcSJmxwDCzLcNNzLGmNdBwE8vbt9zBpzaq33UFEuzeyAR2gDflt3J4iwWKqXDKiNws8tpsec5EjGWY4RPQ3p_VpWA5-5rRPIGA9iGymZjVzE2FQt9Tjbgp-PfNeyB2t-DrzvSrMnTxnRYlMB833-8NiFmB426cvTimYbaa9_KnkW5XepkUZxt0-EdgXAWT6FU2Ee1S7QO9JciR-LYTuocKFzRgwDgbbIm3gHRgXVyHwEMDo2e87H0oF2pc_ILIUOTrFJQ=": dial tcp 173.194.198.82:443: i/o timeout.
Error: Process completed with exit code 125.

https://github.com/ZcashFoundation/zebra/actions/runs/3642500278/jobs/6150421912#step:9:158

@teor2345 teor2345 requested a review from arya2 December 7, 2022 23:09
@teor2345
Copy link
Contributor Author

teor2345 commented Dec 7, 2022

Just a rebase and resolving some trivial conflicts.

mergify bot added a commit that referenced this pull request Dec 8, 2022
@mergify
Merge of #5776
8772ec3
@mergify mergify bot mentioned this pull request Dec 8, 2022
Closed
@mergify mergify bot merged commit bb5f934 into main Dec 8, 2022
@mergify mergify bot deleted the zip317-revise-tx-selection branch December 8, 2022 04:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arya2 arya2 arya2 approved these changes

Assignees

@teor2345 teor2345

Labels
A-rpc Area: Remote Procedure Call interfaces C-enhancement Category: This is an improvement C-security Category: Security issues C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG I-remote-node-overload Zebra can overload other nodes on the network
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update block production algorithm once ZIP-317 updates
2 participants
@teor2345 @arya2