From 51e2cd322cfc7271af309e3a2243448a2ec0cad4 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 30 Nov 2020 16:13:12 -0500 Subject: [PATCH 1/3] Have CalculateMaximumSignedTxSize also compute tx weight --- src/wallet/feebumper.cpp | 2 +- src/wallet/wallet.cpp | 18 ++++++++++++------ src/wallet/wallet.h | 4 ++-- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index 6cbad14de85..0d4612a2a7f 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -190,7 +190,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo if (coin_control.m_feerate) { // The user provided a feeRate argument. // We calculate this here to avoid compiler warning on the cs_wallet lock - const int64_t maxTxSize = CalculateMaximumSignedTxSize(*wtx.tx, &wallet); + const int64_t maxTxSize = CalculateMaximumSignedTxSize(*wtx.tx, &wallet).first; Result res = CheckFeeRate(wallet, wtx, *new_coin_control.m_feerate, maxTxSize, errors); if (res != Result::OK) { return res; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 3019d53c6c6..141842d9213 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1594,14 +1594,15 @@ bool CWallet::ImportScriptPubKeys(const std::string& label, const std::set CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, bool use_max_sig) { std::vector txouts; for (const CTxIn& input : tx.vin) { const auto mi = wallet->mapWallet.find(input.prevout.hash); // Can not estimate size without knowing the input details if (mi == wallet->mapWallet.end()) { - return -1; + return std::make_pair(-1, -1); } assert(input.prevout.n < mi->second.tx->vout.size()); txouts.emplace_back(mi->second.tx->vout[input.prevout.n]); @@ -1610,13 +1611,16 @@ int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wall } // txouts needs to be in the order of tx.vin -int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, const std::vector& txouts, bool use_max_sig) +std::pair CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, const std::vector& txouts, bool use_max_sig) { CMutableTransaction txNew(tx); if (!wallet->DummySignTx(txNew, txouts, use_max_sig)) { - return -1; + return std::make_pair(-1, -1); } - return GetVirtualTransactionSize(CTransaction(txNew)); + CTransaction ctx(txNew); + int64_t vsize = GetVirtualTransactionSize(ctx); + int64_t weight = GetTransactionWeight(ctx); + return std::make_pair(vsize, weight); } int CalculateMaximumSignedInputSize(const CTxOut& txout, const CWallet* wallet, bool use_max_sig) @@ -2770,6 +2774,7 @@ bool CWallet::CreateTransactionInternal( CMutableTransaction txNew; FeeCalculation feeCalc; CAmount nFeeNeeded; + std::pair tx_sizes; int nBytes; { std::set setCoins; @@ -2952,7 +2957,8 @@ bool CWallet::CreateTransactionInternal( txNew.vin.push_back(CTxIn(coin.outpoint,CScript())); } - nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly); + tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly); + nBytes = tx_sizes.first; if (nBytes < 0) { error = _("Signing transaction failed"); return false; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 69cf6b66a4a..c491cb2084b 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1321,8 +1321,8 @@ class WalletRescanReserver // Use DummySignatureCreator, which inserts 71 byte signatures everywhere. // NOTE: this requires that all inputs must be in mapWallet (eg the tx should // be IsAllFromMe). -int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, bool use_max_sig = false) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet); -int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, const std::vector& txouts, bool use_max_sig = false); +std::pair CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, bool use_max_sig = false) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet); +std::pair CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, const std::vector& txouts, bool use_max_sig = false); //! Add wallet name to persistent configuration so it will be loaded on startup. bool AddWalletSetting(interfaces::Chain& chain, const std::string& wallet_name); From 3e69939b78d0143d514c5d9b6c6a9844c9bb901c Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 30 Nov 2020 16:13:36 -0500 Subject: [PATCH 2/3] Fail if maximum weight is too large Our max weight check in CreateTransaction only worked if the transaction was fully signed. However if we are funding a transaction, it is possible that the tx weight will be too large for a standard tx. In that case, we should also fail. So we use the tx weight returned by CalculateMaximumSignedTxSize and check against the limit for those transactions. --- src/wallet/wallet.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 141842d9213..3faa038df46 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3068,7 +3068,8 @@ bool CWallet::CreateTransactionInternal( tx = MakeTransactionRef(std::move(txNew)); // Limit size - if (GetTransactionWeight(*tx) > MAX_STANDARD_TX_WEIGHT) + if ((sign && GetTransactionWeight(*tx) > MAX_STANDARD_TX_WEIGHT) || + (!sign && tx_sizes.second > MAX_STANDARD_TX_WEIGHT)) { error = _("Transaction too large"); return false; From 48a0319babb409cf486a9eb7c776810f70b06cb2 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 30 Nov 2020 15:32:06 -0500 Subject: [PATCH 3/3] Add a test that selects too large if BnB is used If BnB is used, the test will fail because the transaction is too large. --- test/functional/rpc_fundrawtransaction.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index 8ee0ecab0ac..ac7b2148bd6 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -94,6 +94,7 @@ def run_test(self): self.test_address_reuse() self.test_option_subtract_fee_from_outputs() self.test_subtract_fee_with_presets() + self.test_transaction_too_large() def test_change_position(self): """Ensure setting changePosition in fundraw with an exact match is handled properly.""" @@ -907,5 +908,24 @@ def test_subtract_fee_with_presets(self): signedtx = self.nodes[0].signrawtransactionwithwallet(fundedtx['hex']) self.nodes[0].sendrawtransaction(signedtx['hex']) + def test_transaction_too_large(self): + self.log.info("Test fundrawtx where BnB solution would result in a too large transaction, but Knapsack would not") + + self.nodes[0].createwallet("large") + wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + recipient = self.nodes[0].get_wallet_rpc("large") + outputs = {} + rawtx = recipient.createrawtransaction([], {wallet.getnewaddress(): 147.99899260}) + + # Make 1500 0.1 BTC outputs + # The amount that we target for funding is in the BnB range when these outputs are used. + # However if these outputs are selected, the transaction will end up being too large, so it shouldn't use BnB and instead fallback to Knapsack + # but that behavior is not implemented yet. For now we just check that we get an error. + for i in range(0, 1500): + outputs[recipient.getnewaddress()] = 0.1 + wallet.sendmany("", outputs) + self.nodes[0].generate(10) + assert_raises_rpc_error(-4, "Transaction too large", recipient.fundrawtransaction, rawtx) + if __name__ == '__main__': RawTransactionsTest().main()