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

3. change(rpc): Add fee and sigops fields to getblocktemplate transactions #5508

Merged
merged 6 commits into from
Nov 3, 2022

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Oct 31, 2022

Motivation

We need to add a mempool query to get the fee and sigops fields for the getblocktemplate RPC.

Closes #5454
Depends-On: #5496

Specifications

https://zcash.github.io/rpc/getblocktemplate.html

And whatever zcashd does.

Solution

  • Add a new FullTransactions mempool request
  • Return VerifiedUnminedTx with a new legacy_sigop_count field
  • Add the fee and sigops fields to the transaction templates
  • Add conversions from VerifiedUnminedTx to the block header roots
  • Update tests for the new API

Review

I think @oxarbitrage is reviewing this PR series, but anyone else can review it as well.

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 rest of the getblocktemplate RPC, probably the coinbase transaction or state request next

@teor2345 teor2345 added A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement P-Medium ⚡ A-rpc Area: Remote Procedure Call interfaces labels Oct 31, 2022
@teor2345 teor2345 requested a review from a team as a code owner October 31, 2022 04:00
@teor2345 teor2345 self-assigned this Oct 31, 2022
@teor2345 teor2345 requested a review from a team as a code owner October 31, 2022 04:00
@teor2345 teor2345 requested review from arya2 and removed request for a team October 31, 2022 04:00
@github-actions github-actions bot added the C-feature Category: New features label Oct 31, 2022
@codecov
Copy link

codecov bot commented Oct 31, 2022

Codecov Report

Merging #5508 (985e2db) into main (1424115) will decrease coverage by 0.19%.
The diff coverage is 23.91%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5508      +/-   ##
==========================================
- Coverage   78.91%   78.72%   -0.20%     
==========================================
  Files         305      305              
  Lines       38061    38099      +38     
==========================================
- Hits        30037    29993      -44     
- Misses       8024     8106      +82     

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 good

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, lets try Arya suggestion at least for the one marked as allow(dead_code) before merging.

zebra-chain/src/transaction/unmined.rs Outdated Show resolved Hide resolved
@teor2345 teor2345 marked this pull request as draft November 2, 2022 23:49
@teor2345 teor2345 marked this pull request as ready for review November 2, 2022 23:49
@teor2345
Copy link
Contributor Author

teor2345 commented Nov 3, 2022

Job failure but not a PR failure, see #5494 (comment)

Base automatically changed from getblocktemplate-transactions to main November 3, 2022 03:25
mergify bot added a commit that referenced this pull request Nov 3, 2022
@mergify mergify bot merged commit 71f5e63 into main Nov 3, 2022
@mergify mergify bot deleted the mempool-fee-sigops branch November 3, 2022 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc Area: Remote Procedure Call interfaces A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement C-feature Category: New features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Populate transactions of BlockTemplate
3 participants