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

change(test): Create test harness for calling getblocktemplate in proposal mode, but don't use it yet #5884

Merged
merged 48 commits into from
Jan 17, 2023
Merged

change(test): Create test harness for calling getblocktemplate in proposal mode, but don't use it yet #5884

merged 48 commits into from
Jan 17, 2023

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Dec 19, 2022 •
edited by teor2345
Loading

Motivation

We want to use getblocktemplate in proposal mode in the acceptance test to check that it's producing valid templates.

Solution

  • Add proposal_block_from_template fn for making a block proposal from GetBlockTemplate data
  • Add a test harness for calling getblocktemplate in proposal mode with the hex-encoded block proposal data, but don't use it yet

Review

Anyone can 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?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

arya2 added 17 commits December 15, 2022 00:41
@arya2
adds ValidateBlock request to state
37d5afd
@arya2
adds Request enum in block verifier
6eb6048 
skips solution check for BlockProposal requests

calls CheckBlockValidity instead of Commit block for BlockProposal requests
@arya2
uses new Request in references to chain verifier
162dedb
@arya2
adds getblocktemplate proposal mode response type
f7c5941
@arya2
makes getblocktemplate-rpcs feature in zebra-consensus select getbloc…
9e0b8d1 
…ktemplate-rpcs in zebra-state
@arya2
Adds PR review revisions
8420b75
@arya2
adds info log in CheckBlockProposalValidity
e3b6d28
@arya2
Reverts replacement of match statement
95cea02
@arya2
adds GetBlockTemplate::capabilities fn
31472a8
@arya2
conditions calling checkpoint verifier on !request.is_proposal
ba302f7
@arya2
updates references to validate_and_commit_non_finalized
02b6758
@arya2
adds snapshot test, updates test vectors
2faa823
@arya2
adds should_count_metrics to NonFinalizedState
32e6ac9
@arya2
Returns an error from chain verifier for block proposal requests belo…
a2ae3fb 
…w checkpoint height

adds feature flags
@arya2
adds "proposal" to GET_BLOCK_TEMPLATE_CAPABILITIES_FIELD
40d002a
@arya2
Merge branch 'main' into gbt-proposal-mode
1b658a1
@arya2
adds back block::Request to zebra-consensus lib
da1b308
@arya2 arya2 self-assigned this Dec 19, 2022
@github-actions github-actions bot added the C-enhancement Category: This is an improvement label Dec 19, 2022
@arya2 arya2 added P-Low ❄️ C-testing Category: These are tests A-rpc Area: Remote Procedure Call interfaces and removed C-enhancement Category: This is an improvement labels Dec 19, 2022
@arya2 arya2 marked this pull request as ready for review December 19, 2022 20:12
@arya2 arya2 requested a review from a team as a code owner December 19, 2022 20:12
@arya2 arya2 requested review from teor2345 and removed request for a team December 19, 2022 20:12
@arya2 arya2 added the no-review-reminders Turn off review reminders label Dec 19, 2022
@codecov
Copy link

codecov bot commented Dec 19, 2022 •
edited
Loading

Codecov Report

Merging #5884 (dd2438b) into main (99a2da6) will increase coverage by 0.04%.
The diff coverage is n/a.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #5884      +/-   ##
==========================================
+ Coverage   78.06%   78.11%   +0.04%     
==========================================
  Files         311      311              
  Lines       38919    38919              
==========================================
+ Hits        30384    30401      +17     
+ Misses       8535     8518      -17

@teor2345 teor2345 added the do-not-merge Tells Mergify not to merge this PR label Jan 15, 2023
@teor2345
Copy link
Contributor

This PR can't merge for now:

I don't think we should do this in CI until we can confirm it manually

#5685 (comment)

@teor2345 teor2345 changed the title change(test): Call getblocktemplate in proposal mode from acceptance test change(test): Create test harness for calling getblocktemplate in proposal mode, but don't use it yet Jan 15, 2023
@teor2345 teor2345 added A-consensus Area: Consensus rule updates and removed C-enhancement Category: This is an improvement do-not-merge Tells Mergify not to merge this PR labels Jan 15, 2023
@teor2345
Copy link
Contributor

This PR can't merge for now:

I don't think we should do this in CI until we can confirm it manually

#5685 (comment)

I unblocked this PR by disabling the test for now, and updating the PR description.

So we're ok to merge the test harness, and all the PRs that depend on it, because it doesn't actually change CI yet.

@teor2345
Copy link
Contributor

I can't work out how to stop it closing issue #5685 though!

Base automatically changed from fix-gbt-merkle-root to main January 16, 2023 03:37
@arya2 arya2 force-pushed the gbt-proposal-test branch from 07e3f55 to dd2438b Compare January 16, 2023 18:53
@github-actions github-actions bot added the C-enhancement Category: This is an improvement label Jan 16, 2023
mergify bot added a commit that referenced this pull request Jan 16, 2023
@mergify
Merge of #5884
ee181cc
@mergify mergify bot mentioned this pull request Jan 16, 2023
Closed
@teor2345
Copy link
Contributor

Failed due to a GitHub runner network or setup issue in the merge queue:
https://github.com/ZcashFoundation/zebra/actions/runs/3934132575

@teor2345
Copy link
Contributor

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Jan 16, 2023

refresh

✅ Pull request refreshed

@arya2 arya2 mentioned this pull request Jan 17, 2023
Merged
6 tasks
mergify bot added a commit that referenced this pull request Jan 17, 2023
@mergify
Merge of #5884
8358627
@mergify mergify bot mentioned this pull request Jan 17, 2023
Closed
mergify bot added a commit that referenced this pull request Jan 17, 2023
@mergify
Merge of #5884
0ca86bd
@mergify mergify bot mentioned this pull request Jan 17, 2023
Closed
@mergify mergify bot merged commit b0ba920 into main Jan 17, 2023
@mergify mergify bot deleted the gbt-proposal-test branch January 17, 2023 04:03
@oxarbitrage oxarbitrage mentioned this pull request Jan 30, 2023
Merged
36 tasks
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

Assignees

@arya2 arya2

Labels
A-consensus Area: Consensus rule updates A-rpc Area: Remote Procedure Call interfaces C-enhancement Category: This is an improvement C-testing Category: These are tests no-review-reminders Turn off review reminders
Projects
None yet
Milestone
No milestone
Development

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