feat!: change getnewblockhex to take multiple commitments

Modifies the getnewblockhex json rpc call to accept an array of
commitments instead of a single commitment.

This is a breaking change since calls to getnewblockhex passing a string
as the 3rd param will now fail as it expects an array of (hex) strings.

.gitignore

@@ -151,3 +151,5 @@ dist/
 *.background.tiff
 
 /guix-build-*
+
+.vscode

src/miner.cpp

@@ -112,7 +112,7 @@ void BlockAssembler::resetBlock()
     nFees = 0;
 }
 
-std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn, std::chrono::seconds min_tx_age, DynaFedParamEntry* proposed_entry, CScript const* commit_script)
+std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn, std::chrono::seconds min_tx_age, DynaFedParamEntry* proposed_entry, const std::set<CScript>* commit_scripts)
 {
     assert(min_tx_age >= std::chrono::seconds(0));
     int64_t nTimeStart = GetTimeMicros();
@@ -206,8 +206,10 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
     }
     coinbaseTx.vin[0].scriptSig = CScript() << nHeight << OP_0;
     // Non-consensus commitment output before finishing coinbase transaction
-    if (commit_script) {
-        coinbaseTx.vout.insert(coinbaseTx.vout.begin(), CTxOut(policyAsset, 0, *commit_script));
+    if (commit_scripts && !commit_scripts->empty()) {
+        for (auto commit_script: *commit_scripts) {
+            coinbaseTx.vout.insert(coinbaseTx.vout.begin(), CTxOut(policyAsset, 0, commit_script));
+        }
     }
     pblock->vtx[0] = MakeTransactionRef(std::move(coinbaseTx));
     pblocktemplate->vchCoinbaseCommitment = GenerateCoinbaseCommitment(*pblock, pindexPrev, chainparams.GetConsensus());

src/miner.h

@@ -159,7 +159,7 @@ class BlockAssembler
     explicit BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool, const CChainParams& params, const Options& options);
 
     /** Construct a new block template with coinbase to scriptPubKeyIn */
-    std::unique_ptr<CBlockTemplate> CreateNewBlock(const CScript& scriptPubKeyIn, std::chrono::seconds min_tx_age = std::chrono::seconds(0), DynaFedParamEntry* = nullptr, CScript const* commit_script = nullptr);
+    std::unique_ptr<CBlockTemplate> CreateNewBlock(const CScript& scriptPubKeyIn, std::chrono::seconds min_tx_age = std::chrono::seconds(0), DynaFedParamEntry* = nullptr, const std::set<CScript>* commit_scripts = nullptr);
 
     inline static std::optional<int64_t> m_last_block_num_txs{};
     inline static std::optional<int64_t> m_last_block_weight{};

src/rpc/mining.cpp

@@ -1277,7 +1277,7 @@ static RPCHelpMan getnewblockhex()
                 "\nGets hex representation of a proposed, unmined new block\n",
                 {
                     {"min_tx_age", RPCArg::Type::NUM, RPCArg::Default{0}, "How many seconds a transaction must have been in the mempool to be included in the block proposal. This may help with faster block convergence among functionaries using compact blocks."},
-                    {"proposed_parameters", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED , "Parameters to be used in dynamic federations blocks as proposals. During a period of `-dynamic_epoch_length` blocks, 4/5 of total blocks must signal these parameters for the proposal to become activated in the next epoch.",
+                    {"proposed_parameters", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "Parameters to be used in dynamic federations blocks as proposals. During a period of `-dynamic_epoch_length` blocks, 4/5 of total blocks must signal these parameters for the proposal to become activated in the next epoch.",
                         {
                             {"signblockscript", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "Hex-encoded block signing script to propose"},
                             {"max_block_witness", RPCArg::Type::NUM, RPCArg::Optional::NO, "Total size in witness bytes that are allowed in the dynamic federations block witness for blocksigning"},
@@ -1289,7 +1289,11 @@ static RPCHelpMan getnewblockhex()
                             },
                         },
                         "proposed_parameters"},
-                        {"commit_data", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED_NAMED_ARG, "Data in hex to be committed to in an additional coinbase output."},
+                    {"commit_data", RPCArg::Type::ARR, RPCArg::Optional::OMITTED, "Array of data in hex to be committed to in additional coinbase outputs.",
+                        {
+                            {"", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "Hex encoded string for commit data"},
+                        },
+                    },
                 },
                 RPCResult{
                     RPCResult::Type::STR_HEX, "blockhex", "the block hex",
@@ -1346,18 +1350,26 @@ static RPCHelpMan getnewblockhex()
         proposed.m_serialize_type = 2;
     }
 
-    // Any commitment required for non-consensus reasons.
-    // This will be placed in the first coinbase output.
-    CScript data_commitment;
+    // Any commitments required for non-consensus reasons.
+    // These will be placed in the first coinbase outputs.
+    std::set<CScript> data_commitments;
     if (!request.params[2].isNull()) {
-        std::vector<unsigned char> data_bytes = ParseHex(request.params[2].get_str());
-        data_commitment = CScript() << OP_RETURN << data_bytes;
+        UniValue commitments = request.params[2].get_array();
+
+        for (unsigned int i = 0; i < commitments.size(); i++) {
+            std::vector<unsigned char> data_bytes = ParseHex(commitments[i].get_str());
+            CScript data_commitment = CScript() << OP_RETURN << data_bytes;
+            auto res = data_commitments.insert(data_commitment);
+            if (!res.second) {
+                throw JSONRPCError(RPC_INVALID_PARAMETER, "Duplicate commitment");
+            }
+        }
     }
 
     CScript feeDestinationScript = Params().GetConsensus().mandatory_coinbase_destination;
     if (feeDestinationScript == CScript()) feeDestinationScript = CScript() << OP_TRUE;
     const NodeContext& node = EnsureAnyNodeContext(request.context);
-    std::unique_ptr<CBlockTemplate> pblocktemplate(BlockAssembler(chainman.ActiveChainstate(), *node.mempool, Params()).CreateNewBlock(feeDestinationScript, std::chrono::seconds(required_wait), &proposed, data_commitment.empty() ? nullptr : &data_commitment));
+    std::unique_ptr<CBlockTemplate> pblocktemplate(BlockAssembler(chainman.ActiveChainstate(), *node.mempool, Params()).CreateNewBlock(feeDestinationScript, std::chrono::seconds(required_wait), &proposed, data_commitments.empty() ? nullptr : &data_commitments));
     if (!pblocktemplate.get()) {
         throw JSONRPCError(RPC_INTERNAL_ERROR, "Wallet keypool empty");
     }

test/functional/rpc_getnewblockhex.py

@@ -0,0 +1,109 @@
+#!/usr/bin/env python3
+"""Test getnewblockhex
+"""
+from io import BytesIO
+
+from test_framework.messages import CBlock
+from test_framework.p2p import (
+    P2PDataStore,
+)
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import (
+    assert_equal,
+    assert_raises_rpc_error,
+    hex_str_to_bytes,
+)
+
+class GetNewBlockHexTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.setup_clean_chain = True
+        self.num_nodes = 1
+
+    def skip_test_if_missing_module(self):
+        self.skip_if_no_wallet()
+
+    def run_test(self):
+        self.log.info("Starting test...")
+        self.bootstrap_p2p()
+
+        node = self.nodes[0]
+
+        height = node.getblockcount()
+        assert_equal(height, 0)
+
+        self.log.info("Call getnewblockhex with no args")
+        hex = node.getnewblockhex()
+        (height, hash) = self.mine_block(hex)
+        assert_equal(height, 1)
+
+        self.log.info("Call getnewblockhex with single commitment")
+        hex = node.getnewblockhex(0, None, ["1234"])
+        assert "6a021234" in hex
+        (height, hash) = self.mine_block(hex)
+        assert_equal(height, 2)
+        block = node.getblock(hash, 2)
+        vout = block["tx"][0]["vout"]
+        assert_equal(vout[0]["scriptPubKey"]["hex"], "6a021234")
+
+        self.log.info("Call getnewblockhex with multiple commitments")
+        hex = node.getnewblockhex(0, None, ["1234", "deadbeef"])
+        assert "6a021234" in hex
+        assert "6a04deadbeef" in hex
+        (height, hash) = self.mine_block(hex)
+        assert_equal(height, 3)
+        block = node.getblock(hash, 2)
+        vout = block["tx"][0]["vout"]
+        assert_equal(vout[0]["scriptPubKey"]["hex"], "6a04deadbeef")
+        assert_equal(vout[1]["scriptPubKey"]["hex"], "6a021234")
+
+        self.log.info("Call getnewblockhex with duplicate commitments")
+        assert_raises_rpc_error(-8, "Duplicate commitment", node.getnewblockhex, 0, None, ["1234", "1234"])
+
+        self.log.info("Done.")
+
+    def mine_block(self, hex):
+        """Mine and submit a block from a given hex template.
+
+        Returns a tuple of the new chain height and the block hash."""
+
+        bytes = hex_str_to_bytes(hex)
+        block = CBlock()
+        block.deserialize(BytesIO(bytes))
+        block.solve()
+        self.send_blocks([block], success=True)
+        height = self.nodes[0].getblockcount()
+        return (height, block.hash)
+
+    def bootstrap_p2p(self, timeout=10):
+        """Add a P2P connection to the node.
+
+        Helper to connect and wait for version handshake."""
+        self.helper_peer = self.nodes[0].add_p2p_connection(P2PDataStore())
+        # We need to wait for the initial getheaders from the peer before we
+        # start populating our blockstore. If we don't, then we may run ahead
+        # to the next subtest before we receive the getheaders. We'd then send
+        # an INV for the next block and receive two getheaders - one for the
+        # IBD and one for the INV. We'd respond to both and could get
+        # unexpectedly disconnected if the DoS score for that error is 50.
+        self.helper_peer.wait_for_getheaders(timeout=timeout)
+
+    def reconnect_p2p(self, timeout=60):
+        """Tear down and bootstrap the P2P connection to the node.
+
+        The node gets disconnected several times in this test. This helper
+        method reconnects the p2p and restarts the network thread."""
+        self.nodes[0].disconnect_p2ps()
+        self.bootstrap_p2p(timeout=timeout)
+
+    def send_blocks(self, blocks, success=True, reject_reason=None, force_send=False, reconnect=False, timeout=960):
+        """Sends blocks to test node. Syncs and verifies that tip has advanced to most recent block.
+
+        Call with success = False if the tip shouldn't advance to the most recent block."""
+        self.helper_peer.send_blocks_and_test(blocks, self.nodes[0], success=success, reject_reason=reject_reason, force_send=force_send, timeout=timeout, expect_disconnect=reconnect)
+
+        if reconnect:
+            self.reconnect_p2p(timeout=timeout)
+
+
+if __name__ == '__main__':
+    GetNewBlockHexTest().main()

test/functional/test_runner.py

@@ -108,6 +108,7 @@
     'feature_assetsdir.py',
     'feature_initial_reissuance_token.py',
     'feature_progress.py',
+    'rpc_getnewblockhex.py',
     # Longest test should go first, to favor running tests in parallel
     'wallet_hd.py --legacy-wallet',
     'wallet_hd.py --descriptors',