signet: omit commitment for some trivial challenges #29032

pull Sjors wants to merge 3 commits into bitcoin:master from Sjors:2023/12/signet-no-challenge changing 3 files +148 −30
  1. Sjors commented at 2:46 pm on December 8, 2023: member

    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 won’t omit the commitment.

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

    Previously the script would fail with PSBT signing failed, making it difficult to mine. This is no longer the case.

  2. DrahtBot commented at 2:46 pm on December 8, 2023: contributor

    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.

    Type Reviewers
    Concept ACK edilmedeiros, danielabrozzoni
    Stale ACK ajtowns

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #29838 (Feature: Use different datadirs for different signets by BrandonOdiwuor)

    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.

  3. Sjors commented at 2:50 pm on December 8, 2023: member

    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

  4. kallewoof commented at 12:36 pm on December 11, 2023: member
    This looks reasonable to me, although I’m not super familiar with all the code being changed.
  5. fanquake requested review from ajtowns on Jan 15, 2024
  6. DrahtBot added the label CI failed on Jan 16, 2024
  7. Sjors force-pushed on Feb 13, 2024
  8. DrahtBot removed the label CI failed on Feb 13, 2024
  9. edilmedeiros commented at 3:48 am on May 16, 2024: contributor

    Concept ACK

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

  10. DrahtBot added the label Needs rebase on Sep 4, 2024
  11. Sjors force-pushed on Sep 5, 2024
  12. Sjors commented at 6:40 am on September 5, 2024: member
    Rebased after #28417.
  13. DrahtBot removed the label Needs rebase on Sep 5, 2024
  14. in test/functional/tool_signet_miner.py:70 in c8bd9193b3 outdated 
    65@@ -61,6 +66,14 @@ def run_test(self):
66             ], check=True, stderr=subprocess.STDOUT)
67         assert_equal(node.getblockcount(), 1)
68 
69+    def run_test(self):
70+        self.log.info("Signet node with trivial challenge (OP_RETURN)")
    ajtowns commented at 11:16 pm on September 5, 2024:
    OP_RETURN ?
    Sjors commented at 7:16 am on September 6, 2024:
    Oops, should be OP_TRUE
  15. in contrib/signet/miner:338 in c8bd9193b3 outdated 
    341+        # BIP325 allows omitting the signet commitment when scriptSig and
342+        # scriptWitness are both empty. This is the case for trivial
343+        # challenges such as OP_TRUE
344+        signet_solution = None
345+        signet_spk = tmpl["signet_challenge"]
346+        if signet_spk != "51":
    ajtowns commented at 11:19 pm on September 5, 2024:

    Might be better as something like:

    0    signet_spk = tmpl["signet_challenge"]
1    if trivial_challenge(signet_spk):
2        signet_solution = None:
3    else:
4        ...

    and

    0def trivial_challenge(spkhex):
1    """BIP325 allows ..."""
2    if spkhex == "51":
3        return True
4    return False
  16. in contrib/signet/miner:82 in c8bd9193b3 outdated 
    78@@ -79,9 +79,10 @@ def decode_psbt(b64psbt):
79     return from_binary(CBlock, psbt.g.map[PSBT_SIGNET_BLOCK]), ser_string(scriptSig) + scriptWitness
80 
81 def finish_block(block, signet_solution, grind_cmd):
82-    block.vtx[0].vout[-1].scriptPubKey += CScriptOp.encode_op_pushdata(SIGNET_HEADER + signet_solution)
83-    block.vtx[0].rehash()
84-    block.hashMerkleRoot = block.calc_merkle_root()
85+    if signet_solution is not None:
    ajtowns commented at 11:21 pm on September 5, 2024:

    nit: I’d probably write this as

    0if signet_solution is None:
1    pass # Don't need to add a signet commitment if there's no signet signature needed
2else:
3    ...
  17. in contrib/signet/miner:97 in c8bd9193b3 outdated 
    93@@ -93,10 +94,20 @@ def finish_block(block, signet_solution, grind_cmd):
94         block.rehash()
95     return block
96 
97-def generate_psbt(tmpl, reward_spk, *, blocktime=None, poolid=None):
98-    signet_spk = tmpl["signet_challenge"]
99+def generate_psbt(block, signet_spk):
    ajtowns commented at 11:24 pm on September 5, 2024:
    Better to move def generate_psbt(..) down to its code, rather than moving the code up?
  18. in contrib/signet/miner:346 in c8bd9193b3 outdated 
    349+            psbt_signed = json.loads(bcli("-stdin", "walletprocesspsbt", input=input_stream))
350+            if not psbt_signed.get("complete",False):
351+               logging.debug("Generated PSBT: %s" % (psbt,))
352+               sys.stderr.write("PSBT signing failed\n")
353+               return None
354+            block, signet_solution = decode_psbt(psbt_signed["psbt"])
    ajtowns commented at 11:27 pm on September 5, 2024:
    Does it still make sense to grab the block from decode_psbt here? You already passed it in to generate_psbt. Could just be _, signet_solution = decode_psbt(...), or perhaps have separate functions get_solution_from_psbt() and get_block_from_psbt() ?
    Sjors commented at 8:05 am on September 6, 2024:
    I ended up making separate functions in 6f5419e3953781ade75d7986f293ce0fc13e90ce.
  19. ajtowns commented at 11:28 pm on September 5, 2024: contributor
    Concept ACK
  20. ajtowns commented at 11:31 pm on September 5, 2024: contributor

    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.

    FWIW, I did a patch for bitcoin-util to do version bit grinding: https://github.com/ajtowns/bitcoin/commit/634e72cbfc0aa3f657a35c7b597f688bb2bb29a6 (Note that it lightly depends on the changes in #28802) Version bit grinding is incompatible with signet though (at least if the challenge requires a signature), as the version is committed to by the signature.

  21. Sjors force-pushed on Sep 6, 2024
  22. Sjors commented at 8:05 am on September 6, 2024: member

    Thanks for the review, I took the suggestions.

    It could be useful to add version grinding support to OP_TRUE signets.

  23. in contrib/signet/miner:254 in ecee62d2ab outdated 
    242+    scriptWitness are both empty. This is the case for trivial
243+    challenges such as OP_TRUE
244+    """
245+    if spkhex == "51":
246+        return True
247+    return False
    ajtowns commented at 3:31 am on September 12, 2024:

    Would it be interesting to also special case:

    0    spk = bytes.fromhex(spkhex)
1    ...
2    elif 2 <= len(spk) <= 76 and spk[0] + 1 == len(spk):
3        return True

    ie, a single fixed push of 1-75 bytes is also treated as trivial, so you can make your challenge be a push of sha256(your-email-address) to get a unique PoW-only signet.

  24. in contrib/signet/miner:103 in ecee62d2ab outdated 
     99@@ -97,10 +100,7 @@ def finish_block(block, signet_solution, grind_cmd):
100         block.rehash()
101     return block
102 
103-def generate_psbt(tmpl, reward_spk, *, blocktime=None, poolid=None):
104-    signet_spk = tmpl["signet_challenge"]
105-    signet_spk_bin = bytes.fromhex(signet_spk)
106-
107+def new_block(tmpl, reward_spk, blocktime=None, poolid=None):
    ajtowns commented at 4:54 am on September 12, 2024:
    Should keep the , *, to ensure blocktime and poolid are keyword-only args.
  25. ajtowns commented at 4:58 am on September 12, 2024: contributor
    ACK ecee62d2ab63685455b428db4e832827b6ce36f1 with nits
  26. DrahtBot requested review from edilmedeiros on Sep 12, 2024
  27. Sjors force-pushed on Sep 12, 2024
  28. Sjors commented at 8:30 am on September 12, 2024: member

    Taken both suggestions.

    I modified the test to actually check the signet commitment absence.

  29. in test/functional/tool_signet_miner.py:95 in ae86b8c8ed outdated 
    90+        assert(SIGNET_COMMITMENT not in get_segwit_commitment(node))
91+
92+        node = self.nodes[2]
93+        self.log.info("Signet node with trivial challenge (push sha256 hash)")
94+        self.mine_block(node)
95+        assert(SIGNET_COMMITMENT not in get_segwit_commitment(node))
    ajtowns commented at 9:05 am on September 12, 2024:

    python assert doesn’t need brackets

    This could have (rare, but not cryptographically-rare) false positives if the signet commitment value appears within the segwit commitment; could check it correctly with:

    0from test_framework.script import CScript
1
2def get_signet_commitment(segwit_commitment):
3    for el in CScript.fromhex(segwit_commitment):
4        if isinstance(el, bytes) and el[0:4].hex() == SIGNET_COMMITMENT:
5            return el[4:].hex()
6    return None
7
8#assert(SIGNET_COMMITMENT not in get_segwit_commitment(node))
9assert get_signet_commitment(get_segwit_commitment(node)) is None
    Sjors commented at 8:04 am on September 13, 2024:
    Taken
  30. in contrib/signet/README.md:47 in ae86b8c8ed outdated 
    43@@ -44,6 +44,8 @@ Adding the --ongoing parameter will then cause the signet miner to create blocks
44 
45     $MINER --cli="$CLI" generate --grind-cmd="$GRIND" --address="$ADDR" --nbits=$NBITS --ongoing
46 
47+For custom signets with a trivial challenge a PSBT is not necessary. The miner detects this for `OP_TRUE`.
    ajtowns commented at 4:15 am on September 13, 2024:

    This comment should probably be in the “advanced usage” section at the bottom? I think the PSBT is still technically necessary there, it’s just that you’d go getblocktemplate | genpsbt | solvepsbt | submitblock skipping the walletprocesspsbt | jq -r .psbt step as there’s nothing to sign.

    Hmm, except that doesn’t actually work, since only generate has been updated to work with trivial challenges. I think solvepsbt should be also, which can be done by:

     0-def get_solution_from_psbt(psbt):
 1+def get_solution_from_psbt(psbt, emptyok=False):
 2     scriptSig = psbt.i[0].map.get(PSBT_IN_FINAL_SCRIPTSIG, b"")
 3     scriptWitness = psbt.i[0].map.get(PSBT_IN_FINAL_SCRIPTWITNESS, b"\x00")
 4+    if emptyok and len(scriptSig) == 0 and scriptWitness == b"\x00":
 5+        return None
 6     return ser_string(scriptSig) + scriptWitness
 7
 8
 9 def do_solvepsbt(args):
10     psbt = decode_challenge_psbt(sys.stdin.read())
11     block = get_block_from_psbt(psbt)
12-    signet_solution = get_solution_from_psbt(psbt)
13+    signet_solution = get_solution_from_psbt(psbt, emptyok=True)
14     block = finish_block(block, signet_solution, args.grind_cmd)
15     print(block.serialize().hex())
    Sjors commented at 8:04 am on September 13, 2024:
    Moved the (rephrased) comment down and taken the suggestion.
  31. DrahtBot added the label CI failed on Sep 13, 2024
  32. in test/functional/tool_signet_miner.py:66 in ae86b8c8ed outdated 
    61+        self.setup_nodes()
62+        # Nodes with different signet networks are not connected
63 
64-        # generate block with signet miner tool
65+    # generate block with signet miner tool
66+    def mine_block(self, node):
    ajtowns commented at 5:56 am on September 13, 2024:

    Could consider checking manual mining as well:

     0    # generate block with signet miner tool
 1    def mine_block_manual(self, node):
 2        assert_equal(node.getblockcount(), 1)
 3        base_dir = self.config["environment"]["SRCDIR"]
 4        signet_miner_path = os.path.join(base_dir, "contrib", "signet", "miner")
 5        base_cmd = [
 6                sys.executable,
 7                signet_miner_path,
 8                f'--cli={node.cli.binary} -datadir={node.cli.datadir}',
 9        ]
10
11        template = node.getblocktemplate(dict(rules=["signet","segwit"]))
12        genpsbt = subprocess.run(base_cmd + [
13                'genpsbt',
14                f'--address={node.getnewaddress()}',
15                '--poolnum=98',
16            ], check=True, input=json.dumps(template).encode('utf8'), capture_output=True)
17        solvepsbt = subprocess.run(base_cmd + [
18                'solvepsbt',
19                f'--grind-cmd={self.options.bitcoinutil} grind',
20            ], check=True, input=genpsbt.stdout, capture_output=True)
21        node.submitblock(solvepsbt.stdout.decode('utf8').strip())
22        assert_equal(node.getblockcount(), 2)
23
24...
25        node = self.nodes[2]
26        self.log.info("Signet node with trivial challenge (push sha256 hash)")
27        self.mine_block(node)
28        assert(SIGNET_COMMITMENT not in get_segwit_commitment(node))
29        self.mine_block_manual(node)
30        assert(SIGNET_COMMITMENT not in get_segwit_commitment(node))
    Sjors commented at 8:04 am on September 13, 2024:
    Also taken, and added a signed example.
  33. signet: split decode_psbt miner helper 86920823a4
  34. signet: miner skips PSBT step for OP_TRUE b0b77eb378
  35. test: signet tool genpsbt and solvepsbt commands 
    Co-authored-by: Anthony Towns <aj@erisian.com.au>
    19d78e1c41
  36. Sjors force-pushed on Sep 13, 2024
  37. Sjors commented at 8:07 am on September 13, 2024: member
    Also rebased, just in case that helps with CI.
  38. DrahtBot removed the label CI failed on Sep 13, 2024
  39. in contrib/signet/README.md:84 in 19d78e1c41 
    80@@ -81,3 +81,4 @@ These steps can instead be done explicitly:
81 
82 This is intended to allow you to replace part of the pipeline for further experimentation (eg, to sign the block with a hardware wallet).
83 
84+For custom signets with a trivial challenge such as  `OP_TRUE` the walletprocesspsbt step can be skipped.
    danielabrozzoni commented at 4:38 pm on September 13, 2024:

    I would rephrase as “the walletprocesspsbt step has to be skipped”, as it currently returns an error:

    0❯ $CLI -signet getblocktemplate '{"rules": ["signet","segwit"]}' | $MINER --cli="$CLI" genpsbt --address="$ADDR" | $CLI -signet -stdin walletprocesspsbt
1error code: -22
2error message:
3Specified sighash value does not match value stored in PSBT

    Edit: well, I’m not sure if it would fail with every trivial challenge, but it does with OP_TRUE.

    Sjors commented at 5:37 pm on September 13, 2024:

    sighash value does not match

    That’s a bug I think, but I’m not sure what’s causing it.

    I suppose there could be cases where the challenge is trivial but the wallet doesn’t know what to do with it. I think normally it would just not sign anything though. @achow101?

    danielabrozzoni commented at 12:00 pm on September 15, 2024:

    I think walletprocesspsbt is trying to sign the PSBT using SIGHASH_DEFAULT, while the PSBT specifies to use SIGHASH_ALL.

    Using $CLI -signet walletprocesspsbt $PSBT true ALL works for me.

  40. danielabrozzoni commented at 4:40 pm on September 13, 2024: contributor

    Concept ACK

    However contrib/signet/miner can’t handle this, as it fails with PSBT signing failed.

    I can’t seem to have contrib/signet/miner fail on master, am I doing something wrong? I’m on 1d5b2406bb9ce619219a3b76608bd764a3b162c3, I’m testing manually with bitcoind -signet -signetchallenge=51 and:

    0❯ $MINER --cli="$CLI" generate --grind-cmd="$GRIND" --address="$ADDR" --nbits=$NBITS --ongoing
12024-09-13 18:29:59 INFO Mined block at height 24; next in -30h15m33s (mine)
22024-09-13 18:30:17 INFO Mined block at height 25; next in -30h10m28s (mine)
32024-09-13 18:30:20 INFO Mined block at height 26; next in -30h5m8s (mine)
42024-09-13 18:30:28 INFO Mined block at height 27; next in -29h59m53s (mine)
52024-09-13 18:30:32 INFO Mined block at height 28; next in -29h54m35s (mine)
62024-09-13 18:31:36 INFO Mined block at height 29; next in -29h50m16s (mine)
72024-09-13 18:31:41 INFO Mined block at height 30; next in -29h44m57s (mine)
82024-09-13 18:31:47 INFO Mined block at height 31; next in -29h39m40s (mine)
92024-09-13 18:32:02 INFO Mined block at height 32; next in -29h34m32s (mine)
  41. Sjors commented at 8:02 am on September 17, 2024: member

    I can’t seem to have contrib/signet/miner fail on master, am I doing something wrong?

    It’s possible that this changed in some recent rebase. Instead of failing it produces an empty witness and sticks that in the signet commitment.

    I updated the PR description.

  42. Sjors renamed this:
    signet: fixing mining for OP_TRUE challenge
    signet: omit commitment for some trivial challenges
    on Sep 17, 2024


Sjors DrahtBot kallewoof edilmedeiros ajtowns danielabrozzoni


edilmedeiros

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-26 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me