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

2. change(rpc): Add some transaction fields to the getblocktemplate RPC #5496

Merged
merged 26 commits into from
Nov 3, 2022
Merged

2. change(rpc): Add some transaction fields to the getblocktemplate RPC #5496

merged 26 commits into from
Nov 3, 2022

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Oct 28, 2022 •
edited by arya2
Loading

Motivation

This PR adds all the TransactionTemplate fields except fee and sigops, which need changes to the mempool.

Part of #5454
Depends-On: #5493

Specifications

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

Solution

Mempool:

  • Add a new mempool::Request::Transactions behind the getblocktemplate-rpcs feature flag

RPCs & zebra-chain:

  • Fill in the getblocktemplate merkle root and auth data root fields
    • Add conversions from Vec<UnminedTx> to merkle::Root and AuthDataRoot
    • Add Hex serialization support to some zebra-chain types
  • Delete the Coinbase type, it's the same as TransactionTemplate (except for the sign of the fee)
    • Add a new NegativeOrZero amount constraint
  • Add TransactionTemplate fields and fill most of them in
  • Add documentation for the getblocktemplate RPC

Review

@arya2 might want to review this PR, it will make more sense along with PR #5493.

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

  • Store fee and sigops in the mempool
  • Add them to the RPC

@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 28, 2022
@teor2345 teor2345 requested a review from a team as a code owner October 28, 2022 03:50
@teor2345 teor2345 self-assigned this Oct 28, 2022
@teor2345 teor2345 requested a review from a team as a code owner October 28, 2022 03:50
@teor2345 teor2345 requested review from oxarbitrage and removed request for a team October 28, 2022 03:50
@github-actions github-actions bot added the C-feature Category: New features label Oct 28, 2022
Base automatically changed from getblocktemplate-mempool to main October 28, 2022 18:34
teor2345 added 17 commits October 31, 2022 07:19
@teor2345
Add documentation for the getblocktemplate RPC
4b9d9c2
@teor2345
Add a new mempool::Request::Transactions
ac7f03b
@teor2345
Add conversions from Vec<UnminedTx> to merkle::Root and AuthDataRoot
d0cc321
@teor2345
Fill in the merkle root and auth data root fields
daf5c18
@teor2345
Delete the Coinbase type, it's the same as Transaction
377df8d
@teor2345
Fill in some other existing types
ae38114
@teor2345
Add Hex serialization support to some zebra-chain types
0b65145
@teor2345
Add TransactionTemplate fields and fill some in
13eff5c
@teor2345
Fix test hangs by spawning async tasks
cdc0fb8
@teor2345
Add temporary workaround for no transactions in the block
b16d019
@teor2345
Encode hashes and roots as hex
3d63ab8
@teor2345
Update RPC snapshots
747798f
@teor2345
Add a missing Request::Transactions handler
498af2d
@teor2345
Fix doc warnings
5b41ff2
@teor2345
Fix fee serialization
09ae3d4
@teor2345
Update snapshots for serialization changes
72c984e
@teor2345
Add a missing Cargo.lock change
4f5ce34
@teor2345 teor2345 force-pushed the getblocktemplate-transactions branch from 3e5e15f to 4f5ce34 Compare October 30, 2022 22:03
@codecov
Copy link

codecov bot commented Oct 30, 2022 •
edited
Loading

Codecov Report

Merging #5496 (9929d84) into main (c5e6adc) will decrease coverage by 0.42%.
The diff coverage is 8.04%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #5496      +/-   ##
==========================================
- Coverage   79.23%   78.80%   -0.43%     
==========================================
  Files         305      305              
  Lines       37877    38061     +184     
==========================================
- Hits        30010    29995      -15     
- Misses       7867     8066     +199

@teor2345 teor2345 changed the title change(rpc): Add some transaction fields to the getblocktemplate RPC 2. change(rpc): Add some transaction fields to the getblocktemplate RPC Oct 31, 2022
@teor2345 teor2345 mentioned this pull request Oct 31, 2022
Merged
5 tasks
@teor2345
Copy link
Contributor Author

teor2345 commented Nov 1, 2022

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Nov 1, 2022

update

✅ Branch has been successfully updated

@teor2345
Copy link
Contributor Author

teor2345 commented Nov 1, 2022

This seems to be a problem with the get-params-path command:

Error: Input required and not supplied: path

https://github.com/ZcashFoundation/zebra/actions/runs/3366620412/jobs/5595829752#step:8:15

oxarbitrage
oxarbitrage previously approved these changes Nov 2, 2022
View reviewed changes
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 really good to me, i left a few minor comments around.

@teor2345 @oxarbitrage
Fix a comment typo
d1f57bc 
Co-authored-by: Alfredo Garcia <oxarbitrage@gmail.com>
@teor2345
Copy link
Contributor Author

teor2345 commented Nov 2, 2022

@Mergifyio update

@teor2345 teor2345 requested a review from oxarbitrage November 2, 2022 20:22
@mergify
Copy link
Contributor

mergify bot commented Nov 2, 2022

update

✅ Branch has been successfully updated

oxarbitrage
oxarbitrage previously approved these changes Nov 2, 2022
View reviewed changes
arya2
arya2 previously approved these changes Nov 2, 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.

This looks great, I don't have much to add.

@teor2345 teor2345 dismissed stale reviews from arya2 and oxarbitrage via 9929d84 November 2, 2022 23:43
@teor2345 @arya2
Fix a comment typo
9929d84 
Co-authored-by: Arya <aryasolhi@gmail.com>
@teor2345 teor2345 requested review from arya2 and oxarbitrage November 2, 2022 23:46
mergify bot added a commit that referenced this pull request Nov 3, 2022
@mergify
Merge of #5496
ca6adde
@mergify mergify bot mentioned this pull request Nov 3, 2022
Closed
@mergify mergify bot merged commit 1424115 into main Nov 3, 2022
@mergify mergify bot deleted the getblocktemplate-transactions branch November 3, 2022 03:25
@arya2 arya2 mentioned this pull request Dec 5, 2022
Merged
33 tasks
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

@oxarbitrage oxarbitrage Awaiting requested review from oxarbitrage

Assignees

@teor2345 teor2345

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@teor2345 @arya2 @oxarbitrage