Wallet: Nondescript error message for 502nd unconfirmed transaction

tdb3 commented at 5:58 pm on March 23, 2024: contributor

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.

0error code: -1
1error message:
2map::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:

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

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

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

Induce the error on the 502nd send:

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

Confirm transactions, then add a new transaction:

0bcli -rpcwallet=test -generate 1
1bcli -rpcwallet=test -named send outputs="{\"$ADDRESS\": 1}" fee_rate=10
2bcli -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

achow101 commented at 6:19 pm on March 23, 2024: member

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.

maflcko added this to the milestone 27.0 on Mar 24, 2024

maflcko added the label Bug on Mar 24, 2024

furszy commented at 8:31 pm on March 25, 2024: member

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?

ismaelsadeeq commented at 9:22 pm on March 25, 2024: member

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

0......
1
2abubakarismail@Abubakars-MacBook-Pro bitcoin %  ./src/bitcoin-cli -regtest -named send  outputs="{\"$ADDRESS\": 1}" fee_rate=10
3
4error code: -1
5error message:
6map::at:  key not found

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

 0diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
 1index 5d23ebd35a..12d320861f 100644
 2--- a/src/wallet/spend.cpp
 3+++ b/src/wallet/spend.cpp
 4@@ -296,7 +296,9 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const
 5 
 6         /* 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. */
 7         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);
 8-        output.ApplyBumpFee(map_of_bump_fees.at(output.outpoint));
 9+        if (!map_of_bump_fees.empty()) {
10+            output.ApplyBumpFee(map_of_bump_fees.at(output.outpoint));
11+        }
12         result.Insert(output, coin_selection_params.m_subtract_fee_outputs);
13     }
14     return result;
15@@ -455,10 +457,11 @@ CoinsResult AvailableCoins(const CWallet& wallet,
16 
17     if (feerate.has_value()) {
18         std::map<COutPoint, CAmount> map_of_bump_fees = wallet.chain().calculateIndividualBumpF
19ees(outpoints, feerate.value());
20-
21-        for (auto& [_, outputs] : result.coins) {
22-            for (auto& output : outputs) {
23-                output.ApplyBumpFee(map_of_bump_fees.at(output.outpoint));
24+        if (!map_of_bump_fees.empty()) {
25+            for (auto& [_, outputs] : result.coins) {
26+                for (auto& output : outputs) {
27+                    output.ApplyBumpFee(map_of_bump_fees.at(output.outpoint));
28+                }
29             }
30         }
31     }

I can create more transactions

 0abubakarismail@Abubakars-MacBook-Pro bitcoin % for i in $(seq 1 4); do ./src/bitcoin-cli -regtest -named send  outputs="{\"$ADDRESS\": 1}" fee_rate=10; done 
 1{
 2  "txid": "87c5c6564bf5c810be070c2fd5ee7b9e75cd93a6b0f3d0ea85163c5da088e857",
 3  "complete": true
 4}
 5{
 6  "txid": "fdc207674a7fa25ae16651ec69f5b7eb8f96b09c3b8c050648e6eb32db7e2532",
 7  "complete": true
 8}
 9{
10  "txid": "375ad1485ad7f02221e9c79a81ca17ecbcaaf781165807ad23ef8198585e6fc9",
11  "complete": true
12}
13{
14  "txid": "30284d52762e9c2b4255a31fa851b8b745abd371bfa086e2c2f9edb00a707054",
15  "complete": true
16}

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?

furszy commented at 9:25 pm on March 25, 2024: member

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.

ismaelsadeeq commented at 9:42 pm on March 25, 2024: member

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.

 0diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
 1index 5d23ebd35a..646d93bcd7 100644
 2--- a/src/wallet/spend.cpp
 3+++ b/src/wallet/spend.cpp
 4@@ -296,13 +296,16 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const
 5 
 6         /* 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. */
 7         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);
 8+        if (map_of_bump_fees.empty()) {
 9+            return util::Error{strprintf(_("Too many connected unconfirmed transactions, maximum cluster limit exceeded"))};
10+        }
11         output.ApplyBumpFee(map_of_bump_fees.at(output.outpoint));
12         result.Insert(output, coin_selection_params.m_subtract_fee_outputs);
13     }
14     return result;
15 }
16 
17-CoinsResult AvailableCoins(const CWallet& wallet,
18+util::Result<CoinsResult> AvailableCoins(const CWallet& wallet,
19                            const CCoinControl* coinControl,
20                            std::optional<CFeeRate> feerate,
21                            const CoinFilterParams& params)
22@@ -456,9 +459,15 @@ CoinsResult AvailableCoins(const CWallet& wallet,
23     if (feerate.has_value()) {
24         std::map<COutPoint, CAmount> map_of_bump_fees = wallet.chain().calculateIndividualBumpF
25ees(outpoints, feerate.value());
26 
27-        for (auto& [_, outputs] : result.coins) {
28-            for (auto& output : outputs) {
29-                output.ApplyBumpFee(map_of_bump_fees.at(output.outpoint));
30+        if (map_of_bump_fees.empty()) {
31+            if  (!outpoints.empty()) {
32+                return util::Error{strprintf(_("Too many connected unconfirmed transactions, maximum cluster limit exceeded"))};
33+            }
34+        } else {
35+            for (auto& [_, outputs] : result.coins) {
36+                for (auto& output : outputs) {
37+                    output.ApplyBumpFee(map_of_bump_fees.at(output.outpoint));
38+                }
39             }
40         }
41     }
42diff --git a/src/wallet/spend.h b/src/wallet/spend.h
43index 62a7b4e4c8..c387cb69bb 100644
44--- a/src/wallet/spend.h
45+++ b/src/wallet/spend.h
46@@ -83,7 +83,7 @@ struct CoinFilterParams {
47 /**
48  * Populate the CoinsResult struct with vectors of available COutputs, organized by OutputType.
49  */
50-CoinsResult AvailableCoins(const CWallet& wallet,
51+util::Result<CoinsResult> AvailableCoins(const CWallet& wallet,
52                            const CCoinControl* coinControl = nullptr,
53                            std::optional<CFeeRate> feerate = std::nullopt,
54                            const CoinFilterParams& params = {}) EXCLUSIVE_LOCKS_REQUIRED(wallet
55.cs_wallet);
56(END)

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

furszy commented at 9:55 pm on March 25, 2024: member

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.

tdb3 commented at 0:24 am on March 26, 2024: contributor

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?

 0bcli createwallet test
 1export ADDRESS=$(bcli -rpcwallet=test getnewaddress)
 2bcli -rpcwallet=test -generate 701
 3bcli -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
 4bcli -rpcwallet=test getmempoolinfo
 5{
 6  "loaded": true,
 7  "size": 601,
 8  "bytes": 84738,
 9  "usage": 714096,
10  "total_fee": 0.00847410,
11  "maxmempool": 300000000,
12  "mempoolminfee": 0.00001000,
13  "minrelaytxfee": 0.00001000,
14  "incrementalrelayfee": 0.00001000,
15  "unbroadcastcount": 601,
16  "fullrbf": false
17}

furszy commented at 2:10 am on March 26, 2024: member

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:

 0diff --git a/test/functional/wallet_spend_unconfirmed.py b/test/functional/wallet_spend_unconfirmed.py
 1--- a/test/functional/wallet_spend_unconfirmed.py	(revision 8ee4a2600371062ddda0ae623bc0b934c308d594)
 2+++ b/test/functional/wallet_spend_unconfirmed.py	(date 1711418421497)
 3@@ -465,6 +465,25 @@
 4 
 5         wallet.unloadwallet()
 6 
 7+    def test_independent_unconfirmed_txs_limit(self):
 8+        self.nodes[0].createwallet("independent_txs")
 9+        wallet = self.nodes[0].get_wallet_rpc("independent_txs")
10+        # Generate spendable independent txs
11+        self.generatetoaddress(self.nodes[0], 700, wallet.getnewaddress())
12+
13+        # Consume 500 txs
14+        inputs = wallet.listunspent()
15+        assert len(inputs) > 500
16+        address = wallet.getnewaddress()
17+        for i in range(501):
18+            res = wallet.send(outputs=[{address: 1}],
19+                              options={"minconf": 1, "fee_rate": 10, "add_inputs": False, "inputs": [inputs[i]]})
20+            assert 'txid' in res
21+
22+        assert len(wallet.listunspent(minconf=0)) > 500
23+        assert_equal(wallet.getmempoolinfo()['size'], 501)
24+        # Trigger failure
25+        assert 'txid' in wallet.send(outputs=[{address: 5}], options={"minconf": 0, "maxconf": 0, "fee_rate": 10})
26 
27     def run_test(self):
28         self.log.info("Starting UnconfirmedInputTest!")
29@@ -504,5 +523,7 @@
30 
31         self.test_external_input_unconfirmed_low()
32 
33+        self.test_independent_unconfirmed_txs_limit()
34+
35 if __name__ == '__main__':
36     UnconfirmedInputTest().main()

murchandamus commented at 3:11 pm on March 26, 2024: contributor

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

ismaelsadeeq commented at 3:01 pm on April 9, 2024: member

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.

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

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

When DOS limit is hit
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?

fanquake removed this from the milestone 27.0 on Apr 16, 2024

fanquake added this to the milestone 27.1 on Apr 16, 2024

fanquake removed this from the milestone 27.1 on Jun 19, 2024

fanquake commented at 9:12 am on June 19, 2024: member

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

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