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

Add support for block proposals to getblocktemplate method #5684

Closed
arya2 opened this issue Nov 21, 2022 · 7 comments · Fixed by #5870
Closed

Add support for block proposals to getblocktemplate method #5684

arya2 opened this issue Nov 21, 2022 · 7 comments · Fixed by #5870
Assignees
Labels
A-consensus Area: Consensus rule updates A-rpc Area: Remote Procedure Call interfaces A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement

Comments

@arya2
Copy link
Contributor

arya2 commented Nov 21, 2022

Motivation

zcashd supports the proposal capability, so that if miners want to modify the template and then check that their modifications are still valid before mining, they could propose the new template.

This would also be useful for testing purposes, as we could propose the getblocktemplate's response and confirm that the block will be accepted if it has a valid solution.

This would require a Request enum in the block verifier with a variant for speculatively validating a block without checking its solution or committing it.

It also requires a new state request.

See #5630 for the state request.

Specifications

The block data MUST be validated and checked against the server's usual acceptance rules (excluding the check for a valid proof-of-work)

https://en.bitcoin.it/wiki/BIP_0023#Block_Proposal
https://en.bitcoin.it/wiki/BIP_0022#appendix-example-rejection-reasons

Designs

See zcashd impl: https://github.com/zcash/zcash/blob/master/src/rpc/mining.cpp#L538-L568

Out Of Scope:

  • Detailed error handling
@arya2 arya2 added C-enhancement Category: This is an improvement S-needs-triage Status: A bug report needs triage labels Nov 21, 2022
@arya2 arya2 added A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code A-rpc Area: Remote Procedure Call interfaces labels Nov 21, 2022
@teor2345
Copy link
Contributor

All the miners we've seen so far use submitblock, so I think this RPC feature is optional.

@arya2
Copy link
Contributor Author

arya2 commented Nov 22, 2022

All the miners we've seen so far use submitblock, so I think this RPC feature is optional.

It would be really convenient for testing, but this does seem like a potentially bad case of scope creep.

@teor2345
Copy link
Contributor

All the miners we've seen so far use submitblock, so I think this RPC feature is optional.

It would be really convenient for testing, but this does seem like a potentially bad case of scope creep.

Let's think about how we can minimise the scope?

  • don't return detailed errors
  • split the verification function into PoW (equihash, header hash/difficulty) and the rest of the verification (difficulty target, header, transactions), we should be doing PoW first anyway for security
  • add new chain verifier, block verifier, and state request types

Hmm that still seems like a lot.

@arya2
Copy link
Contributor Author

arya2 commented Nov 22, 2022

Let's think about how we can minimise the scope?

I added a design, it's not as bad if we can skip returning detailed errors.

@mpguerra
Copy link
Contributor

I really like this idea for testing! Let's definitely see if we can minimise the scope

@teor2345
Copy link
Contributor

I really like this idea for testing! Let's definitely see if we can minimise the scope

I like this idea too!

If we design it right, we can add a feature that automatically tests each getblocktemplate RPC response before we send it. (Convert the response to a block, then call the proposal function with that block, then assert that it is valid.)

We might be able to automate this test, as long as we ignore errors when the height has changed. (There's a race condition between creating and checking the block.)

We will get better test coverage if we do this with a populated mempool.

@mpguerra
Copy link
Contributor

@arya2 can you please add an estimate here?

@mpguerra mpguerra moved this to ✅ Done in Zebra Jan 24, 2023
@mpguerra mpguerra added this to Zebra Jan 24, 2023
@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Consensus rule updates A-rpc Area: Remote Procedure Call interfaces A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants