mining: add TxCollection to bandwidth-efficiently validate external block templates #35671

In "mining: add TxCollection addMissingTxs" ee1c43e3ceb21fdf38340801221e9fa42c8e6993

AddMissingTxs is order-dependent, a bad entry aborts the loop at that point, so [valid, invalid] keeps valid but [invalid, valid] doesn't. Is this intentional? Correct me if I am wrong but validate-then-apply seems to be a safer approach.

In "mining: add TxCollection addMissingTxs" ee1c43e3ceb21fdf38340801221e9fa42c8e6993

nit: there could be a test case for "unexpected null transaction" when calling addMissingTxs with a null tx

Added.

In "mining: add TxCollection to bandwidth-efficiently validate external block templates" 9bb89df02fc3dbcb870b36d4a339986089ea40e2

not blocking/design question: would it be worth splitting into two capnp interfaces instead of throwing on methods: an ExternalBlockTemplate with just getBlockHeader/getBlock/getCoinbaseMerklePath/submitSolution and BlockTemplate extends(ExternalBlockTemplate) adding getTxFees/getTxSigops/getCoinbaseTx/waitNext/interruptWait? Then TxCollection::makeTemplate() returns the narrower type and the disallowed methods aren't callable, rather than compiling and throwing on call.

I took the liberty to prototype this here: https://github.com/ViniciusCestarii/bitcoin/compare/9bb89df02fc3dbcb870b36d4a339986089ea40e2...pr-35671-new-interface

In "ipc: add TxCollection scaffold" 19bae00aa17be0964e28529ecc348a94522204af

nit: commit states that it adds CollectedTxs class but there's no CollectedTxs class

Two interfaces might require a bunch of code duplication on the Rust client side though? Also several disabled methods could be made to work with external templates, if someone actually needs them.

Fixed commit message.

On the duplication concern: extends() gives upcasting, so shared logic that only needs the common subset doesn't need duplicating. Where branching is needed, it's because the type system is now correctly rejecting a call that would've thrown at runtime anyway and that's the point of the split, not an added cost.

On the disabled methods: if they could be implemented for external templates later, they could just as well be added to the split interfaces later too. The split just removes what's currently unclear to a client consuming the interface (methods that appear callable but are guaranteed to throw).

Just my two cents. I'm fine either way

Should the size of wtxids be checked before the loop (e.g. perhaps using existing constants — MAX_BLOCK_WEIGHT / MIN_TRANSACTION_WEIGHT)?

Added (similar check as in merkleblock.cpp), along with a test.

I added a similar guard to addMissingTxs(), but for it to be actually useful, we'd need to modify libmultiprocess to enforce things like a maximum param size.

nit: does the m_mutex need to be locked in the constructor? — the object isn't shared until after construction returns, so no concurrent access is possible yet.

Dropped

nit: these invariants are guaranteed by the constructor and the any_of check above — I think Assert would be stronger than Assume here to catch future regressions.

More importantly it->second below would crash, so Assert is appropriate. Fixed.

In commit https://github.com/bitcoin/bitcoin/pull/35671/changes/02ed6eafc42e73cc0637bc3c681ee208487dd2ab ipc: add TxCollection scaffold

nit:

I am a bit confused with the naming here, I initially read m_collected_txs as a transaction container. I assume the name refers to the transactions held internally in m_transactions, since most operations inspect or update them.

But i think this would be better named m_tx_collection, as this would make it read less as a container and more as an object

Renamed.

In commit https://github.com/bitcoin/bitcoin/pull/35671/changes/02ed6eafc42e73cc0637bc3c681ee208487dd2ab ipc: add TxCollection scaffold

Supposing a client passes [A, B, C, A] wtxids. The second A would be rejected as the same transaction is requested twice. The current code here would check and recheck this invalid transaction while the mempool is locked.

I think the constructor could check for duplicates before locking the mempool? As the duplicate is entirely visible from the input. A possible implementation could be

index 5d7440fd90..950d1abe9c 100644
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -66,6 +66,15 @@ TxCollection::TxCollection(std::vector<Wtxid> wtxids, const NodeContext& node)
     if (m_wtxids.size() > MAX_BLOCK_WEIGHT / MIN_TRANSACTION_WEIGHT) {
         throw std::runtime_error(strprintf("too many wtxids (%d > %d)", m_wtxids.size(), MAX_BLOCK_WEIGHT / MIN_TRANSACTION_WEIGHT));
     }
+
+    std::unordered_set<Wtxid, SaltedWtxidHasher> seen;
+
+    for (const auto& wtxid : m_wtxids) {
+        if (!seen.insert(wtxid).second) {
+            throw(std::runtime_error("duplicate wtxid"));
+        }
+    }
+
     CTxMemPool& mempool{*Assert(m_node.mempool)};
     LOCK(mempool.cs);
     for (const auto& wtxid : m_wtxids) {

In commit 02ed6ea ipc: add TxCollection scaffold

Here, the entries are inserted into an unordered_map, this has internal storage that might need to be reallocated depending on the number of entries.

The check above this line helps keep it within the proper limit, but since the collection size is known, we could reserve it before acquiring the mempool lock to avoid the reallocating and rehashing while the mempool is locked

m_transactions.reserve(m_wtxids.size());

Done

Done, checking duplicates first without a lock now.

In commit https://github.com/bitcoin/bitcoin/pull/35671/changes/e23f21b5cba0883e5d667321ffb0d5f7dabe87f0 mining: add TxCollection unknownTxPos

I think an improvement to this test would be adding a test to verify the mempool snapshot behavior. such that it tests that after the collection is constructed with a transaction in the mempool, if the transaction is removed from the mempool it does not report it as missing.

a rough implementation would look somehting like

index 83cc2a6462..ced11bbbe5 100755
--- a/test/functional/interface_ipc_mining_tx_collection.py
+++ b/test/functional/interface_ipc_mining_tx_collection.py
@@ -99,8 +99,31 @@ class IPCMiningTxCollectionTest(BitcoinTestFramework):
                         assert_equal(e.description, f"remote exception: std::exception: {method_name} is unavailable for externally generated templates")
                         assert_equal(e.type, "FAILED")

-            self.log.debug("Run the TxCollection workflow")
             remote_wallet.rescan_utxos()
+
+            self.log.debug("TxCollection should retain transactions after they leave the mempool")
+            snapshot_tx = remote_wallet.send_self_transfer(
+                from_node=remote_node,
+                fee_rate=10,
+                confirmed_only=True,
+            )
+            self.sync_mempools()
+            assert snapshot_tx["txid"] in node.getrawmempool()
+
+            async with AsyncExitStack() as snapshot_stack:
+                snapshot_collection = await mining_collect_txs(
+                    mining0,
+                    snapshot_stack,
+                    ctx0,
+                    [snapshot_tx["tx"].wtxid],
+                )
+                assert_equal(await tx_collection_unknown_pos(snapshot_collection, ctx0), [])
+
+                # collection should continue holding its CTransactionRef.
+                self.generate(remote_node, 1)
+                assert snapshot_tx["txid"] not in node.getrawmempool()
+                assert_equal(await tx_collection_unknown_pos(snapshot_collection, ctx0), [])
+
             self.log.debug("Create a transaction that is shared by both mempools before disconnecting")
             shared_tx = remote_wallet.send_self_transfer(
                 from_node=remote_node,

Taken

In commit https://github.com/bitcoin/bitcoin/pull/35671/changes/02d8f439592026975ca6adceb19222adf91976ff mining: add coinbase transaction helper

This method does more than create a basic coinbase transaction, as it also creates the witness commitment.

    m_chainstate.m_chainman.GenerateCoinbaseCommitment(block, &pindexPrev);

And since the witness commitment commits to all transactions in the block, adding a transaction after calling this method would make it outdated?

The current documentation here does not mention that all the transactions must be present before calling this so that this does not happen. An improvement could be

index 43dfa52fea..486534add7 100644
--- a/src/node/miner.h
+++ b/src/node/miner.h
@@ -90,6 +90,9 @@ public:
     /**
      * Create a coinbase transaction and insert it into block.vtx[0].
      *
+     * All non coinbase transactions must already be present so the generated
+     * witness commitment commits to the transaction set
+     *
      * [@param](/bitcoin-bitcoin/contributor/param/)[in,out] block       Block whose coinbase transaction is replaced.
      * [@param](/bitcoin-bitcoin/contributor/param/)[in]     pindexPrev  Previous block index. Used to derive the
      *                            coinbase height, subsidy, and commitment.

Taken.

In commit 02d8f43959 mining: add coinbase transaction helper

This updates the height since the block being created must have a height one greater than its previous block. The ordinary createNewBlock already sets this member, and so does this helper; it is harmless but redundant.

Seems it is needed here because in later commits this helper is called directly on a new BlockAssembler without first calling CreateNewBlock. In that situation, the member might not have been initialized

I think a cleaner approach to this helper would be to have it calculate its own height

const int height{pIndexPrev.nHeight + 1}

and then we use in the code

const CAmount block_reward{
      fees + GetBlockSubsidy(height, chainparams.GetConsensus())
  };

  coinbaseTx.vin[0].scriptSig = CScript() << height;
  coinbaseTx.nLockTime = static_cast<uint32_t>(height - 1);

The difference is that changing nHeight changes the BlockAssembler object even if it is not needer here, while this approach creates a value used only inside this helper. This makes this helper easier to reason about and safer to call independently

Taken, and made it a const member function.

In commit https://github.com/bitcoin/bitcoin/pull/35671/changes/a4a3db4577a43578951758406c01aa982452f9c5 mining: make TxCollection create a BlockTemplate

nit: the commit follows the intended flow for creating a template without using CreateNewBlock but i do have a concern about the commit structure.

In this commit the usual template metadata such as vTxFees, vTxSigOpsCost, m_coinbase_tx are not populated. The next commit addresses this by marking the template as external and causing them to throw. But i think this makes this specific commit a bit incomplete.

Perhaps the sixth commit should be introduced before this one? or together in this commit?

I'd rather not grow the commit, and reversing them makes it hard to test. Going to leave this alone for now. The commits are bisect-safe.

In commit a4a3db4 mining: make TxCollection create a BlockTemplate

nit: It looks like m_mutex is held longer than necessary here? Could we limit its scope to checking completeness and copying the ordered m_transactions into a local vector?

The references remain valid after the lock is released, so block construction and TestBlockValidity would not need to hold the collection mutex. This would also mirror CreateNewBlock, which releases the mempool lock after selecting transactions and before final validation

This is a rough implementation of this

index 5d7440fd90..6f1b47423f 100644
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -121,12 +121,23 @@ std::unique_ptr<CBlockTemplate> TxCollection::MakeTemplate(const uint256& prevha
     debug.clear();

     auto block_template{[&]() -> std::unique_ptr<CBlockTemplate> {
-        LOCK(m_mutex);
-        if (std::ranges::any_of(m_transactions, [](const auto& entry) { return !entry.second; })) {
-            reason = "missing-txs";
-            debug = "collected transaction(s) still missing";
-            return nullptr;
+        std::vector<CTransactionRef> transactions;
+        {
+            LOCK(m_mutex);
+            if (std::ranges::any_of(m_transactions, [](const auto& entry) { return !entry.second; })) {
+                reason = "missing-txs";
+                debug = "collected transaction(s) still missing";
+                return nullptr;
+            }
+            transactions.reserve(m_wtxids.size());
+            for (const auto& wtxid : m_wtxids) {
+                const auto it{m_transactions.find(wtxid)};
+                Assert(it != m_transactions.end());
+                Assert(it->second);
+                transactions.push_back(it->second);
+            }
         }
+
         ChainstateManager& chainman{*Assert(m_node.chainman)};
         LOCK(chainman.GetMutex());
         const auto current_tip{GetTip(chainman)};
@@ -157,11 +168,8 @@ std::unique_ptr<CBlockTemplate> TxCollection::MakeTemplate(const uint256& prevha
         // transactions are added so the witness commitment is current.
         block.vtx.emplace_back();

-        for (const auto& wtxid : m_wtxids) {
-            const auto it{m_transactions.find(wtxid)};
-            Assert(it != m_transactions.end());
-            Assert(it->second);
-            block.vtx.push_back(it->second);
+        for (const auto& tx : transactions) {
+            block.vtx.push_back(tx);
         }

         if (coinbase) {

Done

In commit https://github.com/bitcoin/bitcoin/pull/35671/changes/d0c20776abe1ad14073c3328ef8c64434b0f92c7 mining: restrict externally generated templates

waitNext is disabled for external templates, but interruptWait is still allowed? Is there a reason for this?

I believe the relationship is that while waitNext executes on a thread, interruptWait can be called to stop the wait. For external clients, waitNext will throw, so even though interruptWait can execute, it seems redundant; a call to it would just set the flag to true and wake unrelated waiters unnecessarily

Good catch, I probably missed this in a rebase (before I opened the PR).

In commit https://github.com/bitcoin/bitcoin/pull/35671/changes/f4d53d163615d09650375317faa617eaaf6a07dc mining: add TxCollection addMissingTxs

The transaction passed here can be much larger than the one rejected by the constructor for TxCollection. The check here is against the size, but it does not check the weight. So even though it can reject a million tiny transactions, it can accept 16666 large ones

This would be eventually rejected as oversized in makeTemplate when TestBlockValidity is called, but that is a bit too late.

A way to track the weight of the transactions currently stored in m_transactions and then calculate the weight when adding here could resolve the issue

I don't think we should take over too much work that TestBlockValidity() does for us. Duplicating consensus checks could introduce new bugs.