Merge #19993: refactor: Signet fixups

facaf9e doc: Document signet BIP (MarcoFalke)
faf0a26 doc: Update comments for new chain settings (-signet and -chain=signet) (MarcoFalke)
fae0548 fuzz: Remove needless guard (MarcoFalke)
77771a0 refactor: Remove SignetTxs::m_valid and use optional instead (MarcoFalke)
fa2ad5d test: Run signet test even when wallet was not compiled (MarcoFalke)

Pull request description:

  Some doc and test fixups for #18267

ACKs for top commit:
  ajtowns:
    ACK facaf9e -- code review only
  dr-orlovsky:
    Reviewed & ACK bitcoin/bitcoin@facaf9e
  kallewoof:
    Code review ACK facaf9e

Tree-SHA512: 8085027c488d84bb4bddccba78bd2d4c5af0d8e2644ee72265f1f30fa8c83f61a961d9da2c796f2940e69682291cbee7b1028b6a6ce123ad9134c0ebbf4723b0

doc/bips.md

@@ -42,4 +42,5 @@ BIPs that are implemented by Bitcoin Core (up-to-date up to **v0.21.0**):
 * [`BIP 173`](https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki): Bech32 addresses for native Segregated Witness outputs are supported as of **v0.16.0** ([PR 11167](https://github.com/bitcoin/bitcoin/pull/11167)). Bech32 addresses are generated by default as of **v0.20.0** ([PR 16884](https://github.com/bitcoin/bitcoin/pull/16884)).
 * [`BIP 174`](https://github.com/bitcoin/bips/blob/master/bip-0174.mediawiki): RPCs to operate on Partially Signed Bitcoin Transactions (PSBT) are present as of **v0.17.0** ([PR 13557](https://github.com/bitcoin/bitcoin/pull/13557)).
 * [`BIP 176`](https://github.com/bitcoin/bips/blob/master/bip-0176.mediawiki): Bits Denomination [QT only] is supported as of **v0.16.0** ([PR 12035](https://github.com/bitcoin/bitcoin/pull/12035)).
+* [`BIP 325`](https://github.com/bitcoin/bips/blob/master/bip-0325.mediawiki): Signet test network is supported as of **v0.21.0** ([PR 18267](https://github.com/bitcoin/bitcoin/pull/18267)).
 * [`BIP 339`](https://github.com/bitcoin/bips/blob/master/bip-0339.mediawiki): Relay of transactions by wtxid is supported as of **v0.21.0** ([PR 18044](https://github.com/bitcoin/bitcoin/pull/18044)).

doc/release-notes.md

@@ -335,6 +335,10 @@ RPC
 Tests
 -----
 
+- The BIP 325 default signet can be enabled by the `-chain=signet` or `-signet`
+  setting. The settings `-signetchallenge` and `-signetseednode` allow
+  enabling a custom signet.
+
 Credits
 =======
 

src/bitcoin-cli.cpp

@@ -87,11 +87,6 @@ static void libevent_log_cb(int severity, const char *msg)
     }
 }
 
-//////////////////////////////////////////////////////////////////////////////
-//
-// Start
-//
-
 //
 // Exception thrown on connection error.  This error is used to determine
 // when to wait if -rpcwait is given.
@@ -112,9 +107,6 @@ class CConnectionFailed : public std::runtime_error
 //
 static int AppInitRPC(int argc, char* argv[])
 {
-    //
-    // Parameters
-    //
     SetupCliArgs(gArgs);
     std::string error;
     if (!gArgs.ParseParameters(argc, argv, error)) {
@@ -147,7 +139,7 @@ static int AppInitRPC(int argc, char* argv[])
         tfm::format(std::cerr, "Error reading configuration file: %s\n", error);
         return EXIT_FAILURE;
     }
-    // Check for -chain, -testnet or -regtest parameter (BaseParams() calls are only valid after this clause)
+    // Check for chain settings (BaseParams() calls are only valid after this clause)
     try {
         SelectBaseParams(gArgs.GetChainName());
     } catch (const std::exception& e) {

src/bitcoin-tx.cpp

@@ -78,17 +78,14 @@ static void SetupBitcoinTxArgs(ArgsManager &argsman)
 //
 static int AppInitRawTx(int argc, char* argv[])
 {
-    //
-    // Parameters
-    //
     SetupBitcoinTxArgs(gArgs);
     std::string error;
     if (!gArgs.ParseParameters(argc, argv, error)) {
         tfm::format(std::cerr, "Error parsing command line arguments: %s\n", error);
         return EXIT_FAILURE;
     }
 
-    // Check for -chain, -testnet or -regtest parameter (Params() calls are only valid after this clause)
+    // Check for chain settings (Params() calls are only valid after this clause)
     try {
         SelectParams(gArgs.GetChainName());
     } catch (const std::exception& e) {

src/bitcoin-wallet.cpp

@@ -62,7 +62,7 @@ static bool WalletAppInit(int argc, char* argv[])
         tfm::format(std::cerr, "Error: Specified data directory \"%s\" does not exist.\n", gArgs.GetArg("-datadir", ""));
         return false;
     }
-    // Check for -testnet or -regtest parameter (Params() calls are only valid after this clause)
+    // Check for chain settings (Params() calls are only valid after this clause)
     SelectParams(gArgs.GetChainName());
 
     return true;

src/bitcoind.cpp

@@ -37,10 +37,6 @@ static void WaitForShutdown(NodeContext& node)
     Interrupt(node);
 }
 
-//////////////////////////////////////////////////////////////////////////////
-//
-// Start
-//
 static bool AppInit(int argc, char* argv[])
 {
     NodeContext node;
@@ -81,7 +77,7 @@ static bool AppInit(int argc, char* argv[])
         if (!args.ReadConfigFiles(error, true)) {
             return InitError(Untranslated(strprintf("Error reading configuration file: %s\n", error)));
         }
-        // Check for -chain, -testnet or -regtest parameter (Params() calls are only valid after this clause)
+        // Check for chain settings (Params() calls are only valid after this clause)
         try {
             SelectParams(args.GetChainName());
         } catch (const std::exception& e) {

src/qt/bitcoin.cpp

@@ -533,7 +533,7 @@ int GuiMain(int argc, char* argv[])
     // - QSettings() will use the new application name after this, resulting in network-specific settings
     // - Needs to be done before createOptionsModel
 
-    // Check for -chain, -testnet or -regtest parameter (Params() calls are only valid after this clause)
+    // Check for chain settings (Params() calls are only valid after this clause)
     try {
         SelectParams(gArgs.GetChainName());
     } catch(std::exception &e) {

src/signet.cpp

@@ -65,7 +65,7 @@ static uint256 ComputeModifiedMerkleRoot(const CMutableTransaction& cb, const CB
     return ComputeMerkleRoot(std::move(leaves));
 }
 
-SignetTxs SignetTxs::Create(const CBlock& block, const CScript& challenge)
+Optional<SignetTxs> SignetTxs::Create(const CBlock& block, const CScript& challenge)
 {
     CMutableTransaction tx_to_spend;
     tx_to_spend.nVersion = 0;
@@ -83,12 +83,12 @@ SignetTxs SignetTxs::Create(const CBlock& block, const CScript& challenge)
     // responses from block coinbase tx
 
     // find and delete signet signature
-    if (block.vtx.empty()) return invalid(); // no coinbase tx in block; invalid
+    if (block.vtx.empty()) return nullopt; // no coinbase tx in block; invalid
     CMutableTransaction modified_cb(*block.vtx.at(0));
 
     const int cidx = GetWitnessCommitmentIndex(block);
     if (cidx == NO_WITNESS_COMMITMENT) {
-        return invalid(); // require a witness commitment
+        return nullopt; // require a witness commitment
     }
 
     CScript& witness_commitment = modified_cb.vout.at(cidx).scriptPubKey;
@@ -101,9 +101,9 @@ SignetTxs SignetTxs::Create(const CBlock& block, const CScript& challenge)
             VectorReader v(SER_NETWORK, INIT_PROTO_VERSION, signet_solution, 0);
             v >> tx_spending.vin[0].scriptSig;
             v >> tx_spending.vin[0].scriptWitness.stack;
-            if (!v.empty()) return invalid(); // extraneous data encountered
+            if (!v.empty()) return nullopt; // extraneous data encountered
         } catch (const std::exception&) {
-            return invalid(); // parsing error
+            return nullopt; // parsing error
         }
     }
     uint256 signet_merkle = ComputeModifiedMerkleRoot(modified_cb, block);
@@ -117,7 +117,7 @@ SignetTxs SignetTxs::Create(const CBlock& block, const CScript& challenge)
     tx_to_spend.vin[0].scriptSig << block_data;
     tx_spending.vin[0].prevout = COutPoint(tx_to_spend.GetHash(), 0);
 
-    return {tx_to_spend, tx_spending};
+    return SignetTxs{tx_to_spend, tx_spending};
 }
 
 // Signet block solution checker
@@ -129,19 +129,19 @@ bool CheckSignetBlockSolution(const CBlock& block, const Consensus::Params& cons
     }
 
     const CScript challenge(consensusParams.signet_challenge.begin(), consensusParams.signet_challenge.end());
-    const SignetTxs signet_txs(block, challenge);
+    const Optional<SignetTxs> signet_txs = SignetTxs::Create(block, challenge);
 
-    if (!signet_txs.m_valid) {
+    if (!signet_txs) {
         LogPrint(BCLog::VALIDATION, "CheckSignetBlockSolution: Errors in block (block solution parse failure)\n");
         return false;
     }
 
-    const CScript& scriptSig = signet_txs.m_to_sign.vin[0].scriptSig;
-    const CScriptWitness& witness = signet_txs.m_to_sign.vin[0].scriptWitness;
+    const CScript& scriptSig = signet_txs->m_to_sign.vin[0].scriptSig;
+    const CScriptWitness& witness = signet_txs->m_to_sign.vin[0].scriptWitness;
 
-    TransactionSignatureChecker sigcheck(&signet_txs.m_to_sign, /*nIn=*/ 0, /*amount=*/ signet_txs.m_to_spend.vout[0].nValue);
+    TransactionSignatureChecker sigcheck(&signet_txs->m_to_sign, /*nIn=*/ 0, /*amount=*/ signet_txs->m_to_spend.vout[0].nValue);
 
-    if (!VerifyScript(scriptSig, signet_txs.m_to_spend.vout[0].scriptPubKey, &witness, BLOCK_SCRIPT_VERIFY_FLAGS, sigcheck)) {
+    if (!VerifyScript(scriptSig, signet_txs->m_to_spend.vout[0].scriptPubKey, &witness, BLOCK_SCRIPT_VERIFY_FLAGS, sigcheck)) {
         LogPrint(BCLog::VALIDATION, "CheckSignetBlockSolution: Errors in block (block solution invalid)\n");
         return false;
     }

src/signet.h

@@ -9,6 +9,8 @@
 #include <primitives/block.h>
 #include <primitives/transaction.h>
 
+#include <optional.h>
+
 /**
  * Extract signature and check whether a block has a valid solution
  */
@@ -22,21 +24,14 @@ bool CheckSignetBlockSolution(const CBlock& block, const Consensus::Params& cons
  * 2. It skips the nonce.
  */
 class SignetTxs {
-private:
-    struct invalid {};
-    SignetTxs(invalid i) : m_to_spend(), m_to_sign(), m_valid(false) { }
-
     template<class T1, class T2>
-    SignetTxs(const T1& to_spend, const T2& to_sign) : m_to_spend{to_spend}, m_to_sign{to_sign}, m_valid(true) { }
-
-    static SignetTxs Create(const CBlock& block, const CScript& challenge);
+    SignetTxs(const T1& to_spend, const T2& to_sign) : m_to_spend{to_spend}, m_to_sign{to_sign} { }
 
 public:
-    SignetTxs(const CBlock& block, const CScript& challenge) : SignetTxs(Create(block, challenge)) { }
+    static Optional<SignetTxs> Create(const CBlock& block, const CScript& challenge);
 
     const CTransaction m_to_spend;
     const CTransaction m_to_sign;
-    const bool m_valid;
 };
 
 #endif // BITCOIN_SIGNET_H

src/test/fuzz/signet.cpp

@@ -7,8 +7,8 @@
 #include <primitives/block.h>
 #include <signet.h>
 #include <streams.h>
-#include <test/fuzz/fuzz.h>
 #include <test/fuzz/FuzzedDataProvider.h>
+#include <test/fuzz/fuzz.h>
 #include <test/fuzz/util.h>
 
 #include <cstdint>
@@ -28,7 +28,5 @@ void test_one_input(const std::vector<uint8_t>& buffer)
         return;
     }
     (void)CheckSignetBlockSolution(*block, Params().GetConsensus());
-    if (GetWitnessCommitmentIndex(*block) != NO_WITNESS_COMMITMENT) {
-        (void)SignetTxs(*block, ConsumeScript(fuzzed_data_provider));
-    }
+    (void)SignetTxs::Create(*block, ConsumeScript(fuzzed_data_provider));
 }

test/functional/feature_signet.py

@@ -22,13 +22,14 @@
     '00000020a868e8514be5e46dabd6a122132f423f36a43b716a40c394e2a8d063e1010000f4c6c717e99d800c699c25a2006a75a0c5c09f432a936f385e6fce139cdbd1a5e9964d5fae77031e7d026e0001010000000001010000000000000000000000000000000000000000000000000000000000000000ffffffff025a51feffffff0200f2052a01000000160014aaa671c82b138e3b8f510cd801e5f2bd0aa305940000000000000000776a24aa21a9ede2f61c3f71d1defd3fa999dfa36953755c690689799962b48bebd836974e8cf94c4fecc7daa24900473044022042309f4c3c7a1a2ac8c24f890f962df1c0086cec10be0868087cfc427520cb2702201dafee8911c269b7e786e242045bb57cef3f5b0f177010c6159abae42f646cc501000120000000000000000000000000000000000000000000000000000000000000000000000000',
 ]
 
+
 class SignetBasicTest(BitcoinTestFramework):
     def set_test_params(self):
         self.chain = "signet"
         self.num_nodes = 6
         self.setup_clean_chain = True
-        shared_args1 = ["-signetchallenge=51"] # OP_TRUE
-        shared_args2 = []                         # default challenge
+        shared_args1 = ["-signetchallenge=51"]  # OP_TRUE
+        shared_args2 = []  # default challenge
         # we use the exact same challenge except we do it as a 2-of-2, which means it should fail
         shared_args3 = ["-signetchallenge=522103ad5e0edad18cb1f0fc0d28a3d4f1f3e445640337489abb10404f2d1e086be430210359ef5021964fe22d6f8e05b2463c9540ce96883fe3b278760f048f5189f2e6c452ae"]
 
@@ -38,9 +39,6 @@ def set_test_params(self):
             shared_args3, shared_args3,
         ]
 
-    def skip_test_if_missing_module(self):
-        self.skip_if_no_wallet()
-
     def run_test(self):
         self.log.info("basic tests using OP_TRUE challenge")
 
@@ -53,19 +51,20 @@ def run_test(self):
         assert_equal(mining_info['networkhashps'], Decimal('0'))
         assert_equal(mining_info['pooledtx'], 0)
 
-        self.nodes[0].generatetoaddress(1, self.nodes[0].getnewaddress())
+        self.nodes[0].generate(1)
 
         self.log.info("pregenerated signet blocks check")
 
         height = 0
         for block in signet_blocks:
             assert_equal(self.nodes[2].submitblock(block), None)
-            height = height + 1
+            height += 1
             assert_equal(self.nodes[2].getblockcount(), height)
 
         self.log.info("pregenerated signet blocks check (incompatible solution)")
 
         assert_equal(self.nodes[4].submitblock(signet_blocks[0]), 'bad-signet-blksig')
 
+
 if __name__ == '__main__':
     SignetBasicTest().main()