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(rpc): return an error from getblocktemplate method if Zebra is not synced to network tip #5623

Merged
merged 6 commits into from
Nov 18, 2022

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Nov 11, 2022

Motivation

We want to return an error if there's distance between Zebra's best chain tip height and the estimated network chain tip height because mining pools may use the getblocktemplate method to check if the node is synced.

Closes #5466

Solution

  • Return a server error if the estimated network height is higher than Zebra's chain tip height

Review

This PR is part of regular scheduled work.

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?

@arya2 arya2 added A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement P-Medium ⚡ A-rpc Area: Remote Procedure Call interfaces C-feature Category: New features labels Nov 11, 2022
@arya2 arya2 requested a review from a team as a code owner November 11, 2022 19:40
@arya2 arya2 requested review from dconnolly and removed request for a team November 11, 2022 19:40
@arya2 arya2 self-assigned this Nov 11, 2022
@arya2 arya2 requested a review from oxarbitrage November 11, 2022 19:40
@dconnolly dconnolly changed the title change(rpc) return an error from getblocktemplate method if Zebra is not synced to network tip change(rpc): return an error from getblocktemplate method if Zebra is not synced to network tip Nov 11, 2022
@codecov
Copy link

codecov bot commented Nov 11, 2022

Codecov Report

Merging #5623 (7d2fd6d) into main (61af406) will decrease coverage by 0.12%.
The diff coverage is 12.12%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5623      +/-   ##
==========================================
- Coverage   78.83%   78.71%   -0.13%     
==========================================
  Files         305      305              
  Lines       38463    38496      +33     
==========================================
- Hits        30324    30301      -23     
- Misses       8139     8195      +56     

@dconnolly
Copy link
Contributor

Restarting this, this failure may be related to flipping backend switches in gcloud related to OS Login:

#5602

https://github.com/ZcashFoundation/zebra/actions/runs/3447560707/jobs/5754223166
image

@gustavovalverde
Copy link
Member

Restarting this, this failure may be related to flipping backend switches in gcloud related to OS Login

@dconnolly this is a missing sudo command. I added it in #5602 but any of you can add it here if needed (not sure why this didn't fail before)

@arya2 arya2 requested a review from a team as a code owner November 14, 2022 18:17
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Nov 14, 2022
@arya2 arya2 removed the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Nov 14, 2022
dconnolly
dconnolly previously approved these changes Nov 14, 2022
Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

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

lgtm!

@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Nov 15, 2022
@arya2 arya2 force-pushed the add-not-synced-to-tip-gbt-error branch from c648048 to 88022ad Compare November 16, 2022 01:32
@arya2
Copy link
Contributor Author

arya2 commented Nov 16, 2022

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Nov 16, 2022

update

✅ Branch has been successfully updated

@oxarbitrage
Copy link
Contributor

This looks really good but i will lime to see a test with a chain synced up to tip and a call to getblocktemplate making sure we don't hit the error and the function is still usable.

We don't need to add this to CI but just a log here will be enough.

@arya2
Copy link
Contributor Author

arya2 commented Nov 16, 2022

This looks really good but i will lime to see a test with a chain synced up to tip and a call to getblocktemplate making sure we don't hit the error and the function is still usable.

We don't need to add this to CI but just a log here will be enough.

This helped catch an issue!

I'm thinking I want to add a test that syncs to the tip for getblocktemplate in CI too for testing #5630 with the non-finalized state, but for now:

{
  "jsonrpc": "2.0",
  "result": {
    "capabilities": [],
    "version": 4,
    "previousblockhash": "0000000000000000000000000000000000000000000000000000000000000000",
    "blockcommitmentshash": "0000000000000000000000000000000000000000000000000000000000000000",
    "lightclientroothash": "0000000000000000000000000000000000000000000000000000000000000000",
    "finalsaplingroothash": "0000000000000000000000000000000000000000000000000000000000000000",
    "defaultroots": {
      "merkleroot": "f00a46d38fccf8b48554c1f4651eec6bbc294c79ac2e2bc91e63bfcdc574eaa9",
      "chainhistoryroot": "0000000000000000000000000000000000000000000000000000000000000000",
      "authdataroot": "8231fdfe4e63846249d0880168deb69bcf7d81cc2f762937677f203fa8d8fd2a",
      "blockcommitmentshash": "0000000000000000000000000000000000000000000000000000000000000000"
    },
    "transactions": [],
    "coinbasetxn": {
      "data": "050000800a27a726b4d0d6c20000000082b31c00010000000000000000000000000000000000000000000000000000000000000000ffffffff040382b31c0000000004286bee000000000017a914d45cb1adffb5215a42720532a076f02c7c778c908738c94d010000000017a914fd237ff37cc91b87b7cef8408ade1bc14a4e67158740787d010000000017a914931fec54c1fea86e574462cc32013f5400b891298780b2e60e0000000017a9147d46a730d31f97b1930d3368a967c309bd4d136a87000000",
      "hash": "f00a46d38fccf8b48554c1f4651eec6bbc294c79ac2e2bc91e63bfcdc574eaa9",
      "authdigest": "8231fdfe4e63846249d0880168deb69bcf7d81cc2f762937677f203fa8d8fd2a",
      "depends": [],
      "fee": 0,
      "sigops": 0,
      "required": true
    },
    "target": "",
    "mintime": 0,
    "mutable": ["time", "transactions", "prevblock"],
    "noncerange": "00000000ffffffff",
    "sigoplimit": 20000,
    "sizelimit": 2000000,
    "curtime": 0,
    "bits": "",
    "height": 0
  },
  "id": 123
}

adds info log when estimated_distance_to_chain_tip is too high
@arya2 arya2 force-pushed the add-not-synced-to-tip-gbt-error branch from 87bc022 to 38420cb Compare November 16, 2022 21:02
@arya2
Copy link
Contributor Author

arya2 commented Nov 16, 2022

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Nov 16, 2022

update

✅ Branch has been successfully updated

@arya2 arya2 requested a review from dconnolly November 17, 2022 17:07
mergify bot added a commit that referenced this pull request Nov 17, 2022
@mergify mergify bot merged commit 57fde15 into main Nov 18, 2022
@mergify mergify bot deleted the add-not-synced-to-tip-gbt-error branch November 18, 2022 00:12
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 C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return an error from getblocktemplate if Zebra isn't synced to the tip
4 participants