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 getblocktemplate specification #469

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gandrewstone
Copy link
Contributor

No description provided.

Copy link
Collaborator

@jasonbcox jasonbcox left a comment

Choose a reason for hiding this comment

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

CLIs across implementations are rarely compatible. I don't think this belongs here.

Consider submitting patches to ABC and other projects that will be implementing this spec.

@gandrewstone
Copy link
Contributor Author

I disagree that we should make no specification attempt to make compatible RPC calls, especially for legacy RPCs that are provided by every implementation.

@dagurval
Copy link

getblocktemplate is covered by BIP22 and BIP23. It makes sense that any required changes to generate a valid block template are documented in a specification.

@ftrader
Copy link
Contributor

ftrader commented Apr 14, 2020

concept ACK - not converting the fields in getblocktemplate to new ones seems like an oversight.

@sickpig
Copy link
Collaborator

sickpig commented Apr 14, 2020

CLIs across implementations are rarely compatible. I don't think this belongs here.

There are different class of CLIs, getblocktemplate belongs to the one covered by actual specifications. see:

https://github.com/bitcoin/bips/blob/master/bip-0022.mediawiki
https://github.com/bitcoin/bips/blob/master/bip-0023.mediawiki

sigopslimit and sigops fields are specifically mentioned in BIP22 and, eg ABC is currently following that spec. This won't be the case anymore once May 2020 upgrade will activate.

Unfortunately the SigChecks spec in their current state neglect to define the needed changes mentioned in this PR by @gandrewstone.

Currently both ABC and BCHN will production an incoherent block template once may 2020 network upgrade will be activated. In fact sigopslimit and sigops fields will be misleading at best in the context of an upgraded network.

Consider submitting patches to ABC and other projects that will be implementing this spec.

I don't think this is a matter of submitting code against a particular implementation codebase or another. Here we are facing an inconsistency in the definition of the spec for a new feature, hence the spec should be fixed.

@BigBlockIfTrue
Copy link
Contributor

BigBlockIfTrue commented Apr 14, 2020

I agree with @dagurval and appreciate the effort to specify what should happen to getblocktemplate. I've previously created BCHN Issue #42 for this.

That said, while I agree with removing the SigOps fields, I am not sure we should bother adding new fields for SigChecks. Does mining software actually make use of this information? Because of the mess that SigOps counting was, I expect not.

@BigBlockIfTrue
Copy link
Contributor

For Bitcoin Cash Node v22.0.0 we decided to simply repurpose both the existing sigops and sigoplimit fields to report SigChecks instead, as this would likely give the least compatibility issues with existing mining setups. Our decision matches Bitcoin ABC behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants