From cf793846978a8783c23b66ba6b4f3f30e83ff3eb Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 23 Nov 2022 10:22:50 -0300 Subject: [PATCH] test: Coin Selection, duplicated preset inputs selection This exercises the bug inside CoinsResult::Erase that ends up on (1) a wallet crash or (2) a created and broadcasted tx that contains a reduced recipient's amount. This is covered by making the wallet selects the preset inputs twice during the coin selection process. Making the wallet think that the selection process result covers the entire tx target when it does not. It's actually creating a tx that sends more coins than what inputs are covering for. Which, combined with the SFFO option, makes the wallet incorrectly reduce the recipient's amount by the difference between the original target and the wrongly counted inputs. Which means, a created and relayed tx sending less coins to the destination than what the user inputted. --- src/wallet/test/spend_tests.cpp | 45 +++++++++++++++++++++++ test/functional/rpc_fundrawtransaction.py | 45 +++++++++++++++++++++++ 2 files changed, 90 insertions(+) diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp index a75b0148701..81a8883f855 100644 --- a/src/wallet/test/spend_tests.cpp +++ b/src/wallet/test/spend_tests.cpp @@ -112,5 +112,50 @@ BOOST_FIXTURE_TEST_CASE(FillInputToWeightTest, BasicTestingSetup) // Note: We don't test the next boundary because of memory allocation constraints. } +BOOST_FIXTURE_TEST_CASE(wallet_duplicated_preset_inputs_test, TestChain100Setup) +{ + // Verify that the wallet's Coin Selection process does not include pre-selected inputs twice in a transaction. + + // Add 4 spendable UTXO, 50 BTC each, to the wallet (total balance 200 BTC) + for (int i = 0; i < 4; i++) CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); + auto wallet = CreateSyncedWallet(*m_node.chain, WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return m_node.chainman->ActiveChain()), m_args, coinbaseKey); + + LOCK(wallet->cs_wallet); + auto available_coins = AvailableCoins(*wallet); + std::vector coins = available_coins.All(); + // Preselect the first 3 UTXO (150 BTC total) + std::set preset_inputs = {coins[0].outpoint, coins[1].outpoint, coins[2].outpoint}; + + // Try to create a tx that spends more than what preset inputs + wallet selected inputs are covering for. + // The wallet can cover up to 200 BTC, and the tx target is 299 BTC. + std::vector recipients = {{GetScriptForDestination(*Assert(wallet->GetNewDestination(OutputType::BECH32, "dummy"))), + /*nAmount=*/299 * COIN, /*fSubtractFeeFromAmount=*/true}}; + CCoinControl coin_control; + coin_control.m_allow_other_inputs = true; + for (const auto& outpoint : preset_inputs) { + coin_control.Select(outpoint); + } + + // Attempt to send 299 BTC from a wallet that only has 200 BTC. The wallet should exclude + // the preset inputs from the pool of available coins, realize that there is not enough + // money to fund the 299 BTC payment, and fail with "Insufficient funds". + // + // Even with SFFO, the wallet can only afford to send 200 BTC. + // If the wallet does not properly exclude preset inputs from the pool of available coins + // prior to coin selection, it may create a transaction that does not fund the full payment + // amount or, through SFFO, incorrectly reduce the recipient's amount by the difference + // between the original target and the wrongly counted inputs (in this case 99 BTC) + // so that the recipient's amount is no longer equal to the user's selected target of 299 BTC. + + // First case, use 'subtract_fee_from_outputs=true' + util::Result res_tx = CreateTransaction(*wallet, recipients, /*change_pos*/-1, coin_control); + BOOST_CHECK(!res_tx.has_value()); + + // Second case, don't use 'subtract_fee_from_outputs'. + recipients[0].fSubtractFeeFromAmount = false; + res_tx = CreateTransaction(*wallet, recipients, /*change_pos*/-1, coin_control); + BOOST_CHECK(!res_tx.has_value()); +} + BOOST_AUTO_TEST_SUITE_END() } // namespace wallet diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index 1152995ac9a..6028230ee28 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -107,6 +107,7 @@ class RawTransactionsTest(BitcoinTestFramework): self.generate(self.nodes[0], 121) self.test_add_inputs_default_value() + self.test_preset_inputs_selection() self.test_weight_calculation() self.test_change_position() self.test_simple() @@ -1200,6 +1201,50 @@ class RawTransactionsTest(BitcoinTestFramework): self.nodes[2].unloadwallet("test_preset_inputs") + def test_preset_inputs_selection(self): + self.log.info('Test wallet preset inputs are not double-counted or reused in coin selection') + + # Create and fund the wallet with 4 UTXO of 5 BTC each (20 BTC total) + self.nodes[2].createwallet("test_preset_inputs_selection") + wallet = self.nodes[2].get_wallet_rpc("test_preset_inputs_selection") + outputs = {} + for _ in range(4): + outputs[wallet.getnewaddress(address_type="bech32")] = 5 + self.nodes[0].sendmany("", outputs) + self.generate(self.nodes[0], 1) + + # Select the preset inputs + coins = wallet.listunspent() + preset_inputs = [coins[0], coins[1], coins[2]] + + # Now let's create the tx creation options + options = { + "inputs": preset_inputs, + "add_inputs": True, # automatically add coins from the wallet to fulfill the target + "subtract_fee_from_outputs": [0], # deduct fee from first output + "add_to_wallet": False + } + + # Attempt to send 29 BTC from a wallet that only has 20 BTC. The wallet should exclude + # the preset inputs from the pool of available coins, realize that there is not enough + # money to fund the 29 BTC payment, and fail with "Insufficient funds". + # + # Even with SFFO, the wallet can only afford to send 20 BTC. + # If the wallet does not properly exclude preset inputs from the pool of available coins + # prior to coin selection, it may create a transaction that does not fund the full payment + # amount or, through SFFO, incorrectly reduce the recipient's amount by the difference + # between the original target and the wrongly counted inputs (in this case 9 BTC) + # so that the recipient's amount is no longer equal to the user's selected target of 29 BTC. + + # First case, use 'subtract_fee_from_outputs = true' + assert_raises_rpc_error(-4, "Insufficient funds", wallet.send, outputs=[{wallet.getnewaddress(address_type="bech32"): 29}], options=options) + + # Second case, don't use 'subtract_fee_from_outputs' + del options["subtract_fee_from_outputs"] + assert_raises_rpc_error(-4, "Insufficient funds", wallet.send, outputs=[{wallet.getnewaddress(address_type="bech32"): 29}], options=options) + + self.nodes[2].unloadwallet("test_preset_inputs_selection") + def test_weight_calculation(self): self.log.info("Test weight calculation with external inputs")