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

change(ci): add acceptance test for getblocktemplate RPC in CI, and fix RPC bugs #5761

Merged
merged 6 commits into from
Dec 1, 2022

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Dec 1, 2022

Motivation

We want to test the getblocktemplate RPC with a synced Zebra instance.
We tried this in PR #5653, but the RPC had some bugs, so we had to revert it.

Closes #5652.

Solution

  • Re-apply PR change(tests): add acceptance test for getblocktemplate method in CI #5653 (do a revert of the revert)
  • Fix bugs in the RPC method
    • An assert that was doing the wrong check, causing a panic
    • A fee that wasn't actually negated, causing a panic
  • Fix coverage bugs in the test
    • The original RPC query is sent right after Zebra activates the mempool, when it is usually empty
    • Wait for transactions to download and verify in the test, then call the RPC again

Review

This is a high priority, because it stops us adding more bugs to the getblocktemplate RPC.

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

@teor2345 teor2345 added C-bug Category: This is a bug A-devops Area: Pipelines, CI/CD and Dockerfiles P-Medium ⚡ I-integration-fail Continuous integration fails, including build and test failures C-testing Category: These are tests A-rpc Area: Remote Procedure Call interfaces labels Dec 1, 2022
@teor2345 teor2345 self-assigned this Dec 1, 2022
@github-actions github-actions bot added C-enhancement Category: This is an improvement C-feature Category: New features C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Dec 1, 2022
@teor2345 teor2345 marked this pull request as ready for review December 1, 2022 03:32
@teor2345 teor2345 requested review from a team as code owners December 1, 2022 03:32
@teor2345 teor2345 requested review from arya2 and removed request for a team December 1, 2022 03:32
@teor2345
Copy link
Contributor Author

teor2345 commented Dec 1, 2022

I have run this RPC manually thousands of times, and it seems to work (and it's very fast)

@codecov
Copy link

codecov bot commented Dec 1, 2022

Codecov Report

Merging #5761 (7859d08) into main (f573365) will increase coverage by 0.04%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5761      +/-   ##
==========================================
+ Coverage   78.69%   78.73%   +0.04%     
==========================================
  Files         306      306              
  Lines       38687    38687              
==========================================
+ Hits        30445    30461      +16     
+ Misses       8242     8226      -16     

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 to me!

mergify bot added a commit that referenced this pull request Dec 1, 2022
@mergify mergify bot merged commit afdb3a7 into main Dec 1, 2022
@mergify mergify bot deleted the gbt-sync-until-test-fix branch December 1, 2022 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-devops Area: Pipelines, CI/CD and Dockerfiles A-rpc Area: Remote Procedure Call interfaces C-bug Category: This is a bug C-enhancement Category: This is an improvement C-feature Category: New features C-testing Category: These are tests C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG I-integration-fail Continuous integration fails, including build and test failures
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an acceptance test for the getblocktemplate method
2 participants