Wallet: Nondescript error message for 502nd unconfirmed transaction #29711

Is there an existing issue for this?

• I have searched the existing issues

Current behaviour

When creating a large number of unconfirmed transactions (e.g. 500+) with bitcoin-cli by spending/sending from a wallet, sending/spending the 502nd time results in a nondescript error message. It may be the case that this is expected behavior (e.g. reaching a limit), and the error message could be made more descriptive/informative.

error code: -1
error message:
map::at

This was encountered when creating a test to populate the mempool with a high number of transactions for the v27.0 RC Testing Guide. This behavior was observed both on v26.0 and v27.0 rc1.

Confirming the transactions in a block allows the user to perform additional spending.

Expected behaviour

A more descriptive error message would be provided to the user. In this case, if a limit is being reached, the user could be informed and take appropriate action (e.g. wait until a block confirms transactions, etc.).

Steps to reproduce

Start a node on regtest with its own datadir/bitcoin.conf:

export DATA_DIR=/tmp/tmpdatadir
mkdir $DATA_DIR
export BINARY_PATH=/path/to/dir/containing/bitcoind
alias bitcoind-test="$BINARY_PATH/bitcoind -datadir=$DATA_DIR"
alias bcli="$BINARY_PATH/bitcoin-cli -datadir=$DATA_DIR"
echo "regtest=1" > $DATA_DIR/bitcoin.conf
bitcoind-test -daemon

Create a wallet, fund it with coinbase outputs, and send 501 times:

bcli createwallet test
export ADDRESS=$(bcli -rpcwallet=test getnewaddress)
bcli -rpcwallet=test -generate 701
for i in $(seq 1 499); do bcli -rpcwallet=test -named send outputs="{\"$ADDRESS\": 1}" fee_rate=10; done
bcli -rpcwallet=test getmempoolinfo
bcli -rpcwallet=test -named send outputs="{\"$ADDRESS\": 1}" fee_rate=10
bcli -rpcwallet=test getmempoolinfo
bcli -rpcwallet=test -named send outputs="{\"$ADDRESS\": 1}" fee_rate=10
bcli -rpcwallet=test getmempoolinfo

Induce the error on the 502nd send:

bcli -rpcwallet=test -named send outputs="{\"$ADDRESS\": 1}" fee_rate=10
error code: -1
error message:
map::at

Confirm transactions, then add a new transaction:

bcli -rpcwallet=test -generate 1
bcli -rpcwallet=test -named send outputs="{\"$ADDRESS\": 1}" fee_rate=10
bcli -rpcwallet=test getmempoolinfo

Relevant log output

Nothing relevant observed in debug.log at the time of the error received.

How did you obtain Bitcoin Core

Pre-built binaries

What version of Bitcoin Core are you using?

v26.0.0

Operating system and version

Ubuntu 22.04 LTS

Machine specifications

amd64

It's hitting the 500 tx cluster limit in MiniMiner, which I think we should keep. The error message just needs to be better. The error is happening when the result of CalculateIndividualBumpFees is an empty map, but we try to get things from it, so the fix would be to return a proper error when that map is empty.

It's hitting the 500 tx cluster limit in MiniMiner, which I think we should keep. The error message just needs to be better. The error is happening when the result of CalculateIndividualBumpFees is an empty map, but we try to get things from it, so the fix would be to return a proper error when that map is empty.

I'm unsure whether a wallet containing a group of 500 independent unconfirmed transactions should block transaction creation until those transactions are confirmed. Would be good to obtain some usage metrics. Because this shouldn't present a DoS vector, and we could expand the limit to accept more standalone transactions while maintaining the current limit for clusters with more than one element. Thoughts?

I was able to reproduce same error on master with the steps from @tdb3 53f4607cc8c67366662f49cb312d2e4ff8b6523a

......

abubakarismail@Abubakars-MacBook-Pro bitcoin %  ./src/bitcoin-cli -regtest -named send  outputs="{\"$ADDRESS\": 1}" fee_rate=10

error code: -1
error message:
map::at:  key not found                                                                                          

A naive approach of simply preventing accessing returned value of calculateIndividualBumpFees when its empty solves the issue.

<details> <summary>diff</summary>

diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 5d23ebd35a..12d320861f 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -296,7 +296,9 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const
 
         /* Set some defaults for depth, spendable, solvable, safe, time, and from_me as these don't matter for preset inputs since no selection is being done. */
         COutput output(outpoint, txout, /*depth=*/ 0, input_bytes, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false, coin_selection_params.m_effective_feerate);
-        output.ApplyBumpFee(map_of_bump_fees.at(output.outpoint));
+        if (!map_of_bump_fees.empty()) {
+            output.ApplyBumpFee(map_of_bump_fees.at(output.outpoint));
+        }
         result.Insert(output, coin_selection_params.m_subtract_fee_outputs);
     }
     return result;
@@ -455,10 +457,11 @@ CoinsResult AvailableCoins(const CWallet& wallet,
 
     if (feerate.has_value()) {
         std::map<COutPoint, CAmount> map_of_bump_fees = wallet.chain().calculateIndividualBumpF
ees(outpoints, feerate.value());
-
-        for (auto& [_, outputs] : result.coins) {
-            for (auto& output : outputs) {
-                output.ApplyBumpFee(map_of_bump_fees.at(output.outpoint));
+        if (!map_of_bump_fees.empty()) {
+            for (auto& [_, outputs] : result.coins) {
+                for (auto& output : outputs) {
+                    output.ApplyBumpFee(map_of_bump_fees.at(output.outpoint));
+                }
             }
         }
     }

</details>

I can create more transactions

abubakarismail@Abubakars-MacBook-Pro bitcoin % for i in $(seq 1 4); do ./src/bitcoin-cli -regtest -named send  outputs="{\"$ADDRESS\": 1}" fee_rate=10; done 
{
  "txid": "87c5c6564bf5c810be070c2fd5ee7b9e75cd93a6b0f3d0ea85163c5da088e857",
  "complete": true
}
{
  "txid": "fdc207674a7fa25ae16651ec69f5b7eb8f96b09c3b8c050648e6eb32db7e2532",
  "complete": true
}
{
  "txid": "375ad1485ad7f02221e9c79a81ca17ecbcaaf781165807ad23ef8198585e6fc9",
  "complete": true
}
{
  "txid": "30284d52762e9c2b4255a31fa851b8b745abd371bfa086e2c2f9edb00a707054",
  "complete": true
}

The error message just needs to be better. The error is happening when the result of CalculateIndividualBumpFees is an empty map

The empty map could also be because bump fees have already been calculated, mini miner error message should also be more verbose for each case?

A naive approach of simply preventing accessing returned value of calculateIndividualBumpFees when its empty solves the issue.

That would fallback to the pre ancestors aware funding status. Which isn't desirable.

Oh, thanks @furszy! I can update the return type of AvailableCoins to util::Result<CoinsResult> with a more descriptive error message for when the cluster limit is hit.

<details> <summary>diff</summary>

diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 5d23ebd35a..646d93bcd7 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -296,13 +296,16 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const
 
         /* Set some defaults for depth, spendable, solvable, safe, time, and from_me as these don't matter for preset inputs since no selection is being done. */
         COutput output(outpoint, txout, /*depth=*/ 0, input_bytes, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false, coin_selection_params.m_effective_feerate);
+        if (map_of_bump_fees.empty()) {
+            return util::Error{strprintf(_("Too many connected unconfirmed transactions, maximum cluster limit exceeded"))};
+        }
         output.ApplyBumpFee(map_of_bump_fees.at(output.outpoint));
         result.Insert(output, coin_selection_params.m_subtract_fee_outputs);
     }
     return result;
 }
 
-CoinsResult AvailableCoins(const CWallet& wallet,
+util::Result<CoinsResult> AvailableCoins(const CWallet& wallet,
                            const CCoinControl* coinControl,
                            std::optional<CFeeRate> feerate,
                            const CoinFilterParams& params)
@@ -456,9 +459,15 @@ CoinsResult AvailableCoins(const CWallet& wallet,
     if (feerate.has_value()) {
         std::map<COutPoint, CAmount> map_of_bump_fees = wallet.chain().calculateIndividualBumpF
ees(outpoints, feerate.value());
 
-        for (auto& [_, outputs] : result.coins) {
-            for (auto& output : outputs) {
-                output.ApplyBumpFee(map_of_bump_fees.at(output.outpoint));
+        if (map_of_bump_fees.empty()) {
+            if  (!outpoints.empty()) {
+                return util::Error{strprintf(_("Too many connected unconfirmed transactions, maximum cluster limit exceeded"))};
+            }
+        } else {
+            for (auto& [_, outputs] : result.coins) {
+                for (auto& output : outputs) {
+                    output.ApplyBumpFee(map_of_bump_fees.at(output.outpoint));
+                }
             }
         }
     }
diff --git a/src/wallet/spend.h b/src/wallet/spend.h
index 62a7b4e4c8..c387cb69bb 100644
--- a/src/wallet/spend.h
+++ b/src/wallet/spend.h
@@ -83,7 +83,7 @@ struct CoinFilterParams {
 /**
  * Populate the CoinsResult struct with vectors of available COutputs, organized by OutputType.
  */
-CoinsResult AvailableCoins(const CWallet& wallet,
+util::Result<CoinsResult> AvailableCoins(const CWallet& wallet,
                            const CCoinControl* coinControl = nullptr,
                            std::optional<CFeeRate> feerate = std::nullopt,
                            const CoinFilterParams& params = {}) EXCLUSIVE_LOCKS_REQUIRED(wallet
.cs_wallet);
(END)

</details>

Would it be better to instead modify Mini Miner to return the error message?

Would it be better to instead modify Mini Miner to return the error message?

Yes. It will be better to bubble up the error message from MiniMiner. But, before getting into the implementation details, let's first try to finish discussing the issue. I left a comment above (https://github.com/bitcoin/bitcoin/issues/29711#issuecomment-2018862521) that would be nice to follow it up.

I'm unsure whether a wallet containing a group of 500 independent unconfirmed transactions should block transaction creation until those transactions are confirmed. Would be good to obtain some usage metrics. Because this shouldn't present a DoS vector, and we could expand the limit to accept more standalone transactions while maintaining the current limit for clusters with more than one element. Thoughts?

This seems reasonable to me, but I'm unaware of the history of the 500 limit and the rationale for its implementation (or tradeoffs). The existing "steps to reproduce" relies on the wallet's coin selection (so these transactions might not be independent depending on the selection algorithm). I ran another test that explicitly spends only the mature coinbase outputs independently. The error wasn't received. All transactions were allowed, so it appears that in the existing implementation, independent unconfirmed transactions are not affected by the limit?

bcli createwallet test
export ADDRESS=$(bcli -rpcwallet=test getnewaddress)
bcli -rpcwallet=test -generate 701
bcli -rpcwallet=test listunspent | grep txid | awk '{print $2}' | sed 's/"//g' | sed 's/,//g' | while read -r line; do bcli -rpcwallet=test -named send outputs="{\"$ADDRESS\": 1}" fee_rate=10 inputs=[{\"txid\":\ \"$line\"\,\ \"vout\":\ 0\,\"sequence\":\ \"4294967295\"}]; done
bcli -rpcwallet=test getmempoolinfo
{
  "loaded": true,
  "size": 601,
  "bytes": 84738,
  "usage": 714096,
  "total_fee": 0.00847410,
  "maxmempool": 300000000,
  "mempoolminfee": 0.00001000,
  "minrelaytxfee": 0.00001000,
  "incrementalrelayfee": 0.00001000,
  "unbroadcastcount": 601,
  "fullrbf": false
}

I ran another test that explicitly spends only the mature coinbase outputs independently. The error wasn't received. All transactions were allowed, so it appears that in the existing implementation, independent unconfirmed transactions are not affected by the limit?

It isn't failing because your script skips the unconfirmed outputs. The send() RPC command provides 'minconf=1' by default. This functional test checks the case of independent unconfirmed transactions and triggers the failure consistently:

diff --git a/test/functional/wallet_spend_unconfirmed.py b/test/functional/wallet_spend_unconfirmed.py
--- a/test/functional/wallet_spend_unconfirmed.py	(revision 8ee4a2600371062ddda0ae623bc0b934c308d594)
+++ b/test/functional/wallet_spend_unconfirmed.py	(date 1711418421497)
@@ -465,6 +465,25 @@
 
         wallet.unloadwallet()
 
+    def test_independent_unconfirmed_txs_limit(self):
+        self.nodes[0].createwallet("independent_txs")
+        wallet = self.nodes[0].get_wallet_rpc("independent_txs")
+        # Generate spendable independent txs
+        self.generatetoaddress(self.nodes[0], 700, wallet.getnewaddress())
+
+        # Consume 500 txs
+        inputs = wallet.listunspent()
+        assert len(inputs) > 500
+        address = wallet.getnewaddress()
+        for i in range(501):
+            res = wallet.send(outputs=[{address: 1}],
+                              options={"minconf": 1, "fee_rate": 10, "add_inputs": False, "inputs": [inputs[i]]})
+            assert 'txid' in res
+
+        assert len(wallet.listunspent(minconf=0)) > 500
+        assert_equal(wallet.getmempoolinfo()['size'], 501)
+        # Trigger failure
+        assert 'txid' in wallet.send(outputs=[{address: 5}], options={"minconf": 0, "maxconf": 0, "fee_rate": 10})
 
     def run_test(self):
         self.log.info("Starting UnconfirmedInputTest!")
@@ -504,5 +523,7 @@
 
         self.test_external_input_unconfirmed_low()
 
+        self.test_independent_unconfirmed_txs_limit()
+
 if __name__ == '__main__':
     UnconfirmedInputTest().main()

We should definitely have a more descriptive error, and I expect that it would also feel like a bug to any user encountering it live. We probably do want to keep the limit, so there might be two avenues to mitigate the impact a bit:

If we encounter the limit:

• instead of outright failing, we could fall back to try building a transaction only with confirmed inputs
• we could fall back to processing UTXOs in smaller batches to stay under the limit, although that ensue a noticeable slow down of transaction building if there are a lot of unconfirmed transactions in the wallet

Currently, if you have more than 500 independent unconfirmed transactions, the wallet prevent users from creating a transaction with an unconfirmed input to prevent some DOS vector.

The limit is set by the CTxMempool::GatherClusters method, which was added with the introduction of the mini miner and package-aware funding to prevent the calculation of clusters containing 500 or more unconfirmed transactions in #27021.

I’ve bisected, and this issue does not occur on ab42b2ebdbf61225e636e4c00068fd29b2790d41 prior to introduction package aware funding.

It’s unclear to me why 500 is selected and which DOS limit we are preventing here, especially in this case where all 500 transactions are independent and unconfirmed.

I see from previous comments that the chosen number was arbitrary. (Source: discussion)

It’s suggested here (from review comment) that if a wallet deliberately wants to calculate the bump fees of maybe more than 500 unconfirmed transactions, it should do so in batches, which I believe is similar to what @murchandamus suggested?

If the batching is going be performed in mini miner, by batching GatherCluster calls I think it should not have some significant performance issues if the > 500 transactions are all independent unconfirmed transactions. Also I don't think it will be desirable to batch the calculateIndividualBumpFees calss from the wallet each there will be lots of redundant work

I’ve seen a comment from @furszy on refactoring the mini miner to improve performance, which I don't fully understand. (Source: discussion) Maybe you could clarify your review comment and whether this will benefit solving this issue.

Alternatively, within GatherCluster, we could check if the transactions ancestors and descendants we processed are 500. This way, we are indeed preventing some expensive work.

diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index 226dd9f353..1be14ce843 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -1207,13 +1207,15 @@ std::vector<CTxMemPool::txiter> CTxMemPool::GatherClusters(const std::vector<uin
     for (const auto& it : clustered_txs) {
         visited(it);
     }
+    size_t calculated_cluster_count{0};
     // i = index of where the list of entries to process starts
     for (size_t i{0}; i < clustered_txs.size(); ++i) {
         // DoS protection: if there are 500 or more entries to process, just quit.
-        if (clustered_txs.size() > 500) return {};
+        if (calculated_cluster_count > 500) return {};
         const txiter& tx_iter = clustered_txs.at(i);
         for (const auto& entries : {tx_iter->GetMemPoolParentsConst(), tx_iter->GetMemPoolChildrenConst()}) {
             for (const CTxMemPoolEntry& entry : entries) {
+                calculated_cluster_count++;
                 const auto entry_it = mapTx.iterator_to(entry);
                 if (!visited(entry_it)) {
                     clustered_txs.push_back(entry_it);
(END)

And then when modify mini miner to return more descriptive error to the wallet, i.e

1. When DOS limit is hit
2. When bump fees have already been calculated.

When DOS limit is hit I agree with @murchandamus that we could fall back to try building a transaction only with confirmed inputs.

Because currently in cases where most of the transactions are independent without any ancestor or descendant, I don't think the 500 limit is preventing an expensive computation because it's not like the whole mempool is going to be in users wallet. Thoughts?

Not sure what the status of this is, so removed it from any milestone for now.