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

feat(rpc): populate some getblocktemplate RPC block header fields using the state best chain tip #5659

Merged
merged 45 commits into from
Nov 28, 2022

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Nov 18, 2022

Motivation

[More to be added]

Populate the following fields:

  • capabilities
  • target
  • min time
  • cur time
  • bits
  • height
  • previous block hash

#5455

Solution

[TBA]

Review

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

Populate root/commitment fields

@github-actions github-actions bot added the C-feature Category: New features label Nov 18, 2022
@codecov
Copy link

codecov bot commented Nov 18, 2022

Codecov Report

Merging #5659 (8ef27df) into main (ea21e64) will increase coverage by 0.02%.
The diff coverage is 0.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5659      +/-   ##
==========================================
+ Coverage   78.69%   78.71%   +0.02%     
==========================================
  Files         306      306              
  Lines       38552    38552              
==========================================
+ Hits        30340    30348       +8     
+ Misses       8212     8204       -8     

@oxarbitrage
Copy link
Contributor Author

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! 🎉

zebra-chain/src/work/difficulty.rs Show resolved Hide resolved
zebra-chain/src/work/difficulty.rs Outdated Show resolved Hide resolved
zebra-chain/src/work/difficulty.rs Show resolved Hide resolved
zebra-rpc/src/methods/get_block_template_rpcs.rs Outdated Show resolved Hide resolved
zebra-state/src/request.rs Outdated Show resolved Hide resolved
zebra-state/src/service.rs Outdated Show resolved Hide resolved
zebra-state/src/service.rs Outdated Show resolved Hide resolved
zebra-state/src/service.rs Outdated Show resolved Hide resolved
zebra-state/src/service/check/difficulty.rs Show resolved Hide resolved
@oxarbitrage oxarbitrage marked this pull request as ready for review November 19, 2022 14:56
@oxarbitrage oxarbitrage requested a review from a team as a code owner November 19, 2022 14:56
@oxarbitrage oxarbitrage removed the request for review from a team November 19, 2022 14:56
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I think we're almost there, just need to deal with some tricky edge-cases with time and difficulty on testnet.

zebra-rpc/src/methods/get_block_template_rpcs.rs Outdated Show resolved Hide resolved
zebra-rpc/src/methods/get_block_template_rpcs.rs Outdated Show resolved Hide resolved
zebra-rpc/src/methods/get_block_template_rpcs.rs Outdated Show resolved Hide resolved
zebra-rpc/src/methods/get_block_template_rpcs.rs Outdated Show resolved Hide resolved
Copy link
Contributor Author

@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.

Some comments that i am not totally sure for the testnet consensus rule.

zebra-state/src/service/read/difficulty.rs Outdated Show resolved Hide resolved
zebra-state/src/service/read/difficulty.rs Outdated Show resolved Hide resolved
zebra-state/src/service/read/difficulty.rs Outdated Show resolved Hide resolved
@oxarbitrage
Copy link
Contributor Author

By now, we don't have any tests for testnet specific code under minimum difficulty block mining.

@teor2345 teor2345 changed the title feat(rpc): populate some getblocktemplate fields feat(rpc): populate some getblocktemplate RPC block header fields using the state best chain tip Nov 24, 2022
@teor2345 teor2345 added the A-state Area: State / database changes label Nov 24, 2022
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, I think we're almost there.

I'm happy to help write these changes, but I need to finish the ZIP-317 work first. So feel free to keep making changes here, or ask me to submit a PR on top of this one.

@teor2345
Copy link
Contributor

By now, we don't have any tests for testnet specific code under minimum difficulty block mining.

This existing test might help?

/// Test that testnet minimum-difficulty blocks are valid
#[test]
#[spandoc::spandoc]
fn testnet_minimum_difficulty() -> Result<(), Report> {

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Thanks, I think we're almost there, let me know if you want me to do the minimum difficulty "time" change.

zebra-state/src/service/read/difficulty.rs Outdated Show resolved Hide resolved
@oxarbitrage
Copy link
Contributor Author

Please apply any of the missing stuff and merge if you can. Thank you.

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Let's see if that works

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 A-state Area: State / database changes C-feature Category: New features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants