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

signet: fixing mining for OP_TRUE challenge #29032

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

Conversation

Sjors
Copy link
Member

@Sjors Sjors commented Dec 8, 2023

BIP325 mentions the following rule:

In the special case where an empty solution is valid (ie scriptSig and scriptWitness are both empty) this additional commitment can optionally be left out. This special case is to allow non-signet-aware block generation code to be used to test a custom signet chain where the challenge is trivially true.

Such a signet can be created using e.g. -signetchallenge=51 (OP_TRUE). However contrib/signet/miner can't handle this, as it fails with PSBT signing failed.

This PR improves the miner by skipping the PSBT for known trivial scripts (just OP_TRUE for now). This also avoids appending the 4 byte signet header to the witness commitment, as allowed by the above rule.

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 8, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #30203 (Enhance signet chain configuration in bitcoin.conf by BrandonOdiwuor)
  • #30130 (contrib/signet/miner: increase miner search space by edilmedeiros)
  • #29838 (Feature: Use different datadirs for different signets by BrandonOdiwuor)
  • #28417 (contrib/signet/miner updates by ajtowns)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@Sjors
Copy link
Member Author

Sjors commented Dec 8, 2023

My own goal with this is to test Stratum v2 stuff, e.g. #28983. It's nice to have a test network with difficulty adjustment. Regular testnet difficulty is too high for my humble S9 (let alone a CPU miner) to win blocks.

Using real mining equipment as opposed to a CPU miner is useful, because of the many quirks real devices have (e.g. version bit grinding). Afaik there's no good emulator.

Although I won't actually need this mining script to test local pool software, I patched it anyway to serve as a reference to compare the pool software against.

cc @ajtowns, @kallewoof

@kallewoof
Copy link
Member

This looks reasonable to me, although I'm not super familiar with all the code being changed.

Copy link
Contributor

@edilmedeiros edilmedeiros left a comment

Choose a reason for hiding this comment

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

Concept ACK

Code looks good, I'm still to test it.

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

Successfully merging this pull request may close these issues.

None yet

4 participants