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

feat(rpc): add getblockhash rpc method #4967

Merged
merged 19 commits into from
Oct 21, 2022
Merged

feat(rpc): add getblockhash rpc method #4967

merged 19 commits into from
Oct 21, 2022

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Aug 27, 2022 •
edited by teor2345
Loading

Motivation

After the release candidate Zebra will continue the development of more API calls. getblockhash is one of tme methods we will implement almost for sure.

This is low priority however as it is pretty much done i want to push it to zebra reoo as a PR.

This is a draft because is low priority however in other scenario it should be ready for review.

Close #5268.

Solution

Implement getblockhash.

Required Tests

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

@codecov
Copy link

codecov bot commented Aug 28, 2022 •
edited
Loading

Codecov Report

Merging #4967 (fbe223a) into main (f31609e) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4967      +/-   ##
==========================================
- Coverage   79.15%   79.14%   -0.02%     
==========================================
  Files         308      308              
  Lines       39324    39324              
==========================================
- Hits        31128    31123       -5     
- Misses       8196     8201       +5

@teor2345 teor2345 added the S-blocked Status: Blocked on other tasks label Aug 28, 2022
@teor2345
Copy link
Contributor

This is blocked by tagging the release we send to the auditors, because it's part of a new feature.

@teor2345 teor2345 added C-feature Category: New features S-incomplete and removed S-incomplete labels Sep 20, 2022
@teor2345 teor2345 mentioned this pull request Sep 22, 2022
Closed
34 tasks
@mpguerra mpguerra linked an issue Sep 27, 2022 that may be closed by this pull request
Add support for getblockhash RPC call #5268
Closed
3 tasks
@teor2345 teor2345 added do-not-merge Tells Mergify not to merge this PR and removed S-blocked Status: Blocked on other tasks labels Oct 6, 2022
teor2345
teor2345 reviewed Oct 10, 2022
View reviewed changes
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.

Sorry it has taken me a while to review this PR, I've been focused on CI and release candidate work.

Can you rebase this PR on top of the getblocktemplate-rpcs feature in PR #5357, once that's done?

zebra-rpc/src/methods/tests/snapshot.rs Show resolved Hide resolved
zebra-rpc/src/methods.rs Show resolved Hide resolved
zebra-rpc/src/methods.rs Outdated Show resolved Hide resolved
@teor2345 teor2345 assigned oxarbitrage and unassigned oxarbitrage Oct 10, 2022
@teor2345 teor2345 removed P-Optional ✨ do-not-merge Tells Mergify not to merge this PR labels Oct 13, 2022
@teor2345
Copy link
Contributor

We tagged the release candidate, so new features can merge now.

Can you add a priority label to this ticket, so people know how urgent it is?

@teor2345
Suggestion for "add getblockhash rpc method" PR (#5428)
8f990bf 
* Always immediately return errors in get_height_from_int()

* Explain why calculations can't overflow
teor2345
teor2345 suggested changes Oct 19, 2022
View reviewed changes
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.

The changes in this PR cause memory corruption in the RPC tests on macOS:

process didn't exit successfully:
/Users/runner/work/zebra/zebra/target/debug/deps/zebra_rpc-194938bf1186cc48 --nocapture (signal: 10, SIGBUS: access to undefined memory)

https://github.com/ZcashFoundation/zebra/actions/runs/3284184984/jobs/5409834731#step:14:6231

Since most of these changes are behind a feature, and the new state requests aren't called by the old tests, this could be an existing bug, or bug from PR #5357 that only happens sometimes.

zebra-rpc/src/methods/tests/vectors.rs Outdated Show resolved Hide resolved
zebra-state/src/request.rs Outdated Show resolved Hide resolved
zebra-state/src/response.rs Show resolved Hide resolved
zebra-state/src/service/read/block.rs 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
@teor2345
Copy link
Contributor

this could be an existing bug, or bug from PR #5357 that only happens sometimes.

I can't see anything obvious in PR #5357, so here's what it could be:

Maybe adding the getblocktemplate tests to CI (#5405) will help us diagnose the issue?

@oxarbitrage
Copy link
Contributor Author

Failing in building the zebra-rpc crate individually, ill try to fix tomorrow and the PR should be ready.

@oxarbitrage
Copy link
Contributor Author

The changes in this PR cause memory corruption in the RPC tests on macOS:

process didn't exit successfully:
/Users/runner/work/zebra/zebra/target/debug/deps/zebra_rpc-194938bf1186cc48 --nocapture (signal: 10, SIGBUS: access to undefined memory)

https://github.com/ZcashFoundation/zebra/actions/runs/3284184984/jobs/5409834731#step:14:6231

Since most of these changes are behind a feature, and the new state requests aren't called by the old tests, this could be an existing bug, or bug from PR #5357 that only happens sometimes.

This PR was not ready when you reviewed, i was still working on it. it seems that error is gone. I will remove it from draft state when it is ready, there is still a known issue i will need to fix.

@oxarbitrage oxarbitrage marked this pull request as ready for review October 20, 2022 12:43
@oxarbitrage oxarbitrage requested a review from a team as a code owner October 20, 2022 12:43
@oxarbitrage oxarbitrage requested review from arya2 and removed request for a team October 20, 2022 12:43
@teor2345 teor2345 added the A-rpc Area: Remote Procedure Call interfaces label Oct 21, 2022
teor2345
teor2345 approved these changes Oct 21, 2022
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks!

We need to get this tested in CI pretty soon. I don't want to block this PR, but we should get ticket #5435 done next.

@teor2345 teor2345 added the A-state Area: State / database changes label Oct 21, 2022
@teor2345
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Oct 21, 2022

update

✅ Branch has been successfully updated

mergify bot added a commit that referenced this pull request Oct 21, 2022
@mergify
Merge of #4967
102bc98
@mergify mergify bot mentioned this pull request Oct 21, 2022
Closed
@mergify mergify bot merged commit 8d777c8 into main Oct 21, 2022
@mergify mergify bot deleted the getblockhash branch October 21, 2022 06:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teor2345 teor2345 teor2345 approved these changes

@arya2 arya2 Awaiting requested review from arya2 arya2 is a code owner automatically assigned from ZcashFoundation/general-rust-reviewers

Assignees

@oxarbitrage oxarbitrage

Labels
A-rpc Area: Remote Procedure Call interfaces A-state Area: State / database changes C-feature Category: New features
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for getblockhash RPC call
2 participants
@oxarbitrage @teor2345