[CHIA-1569] Fix fee behavior with chia wallet coins combine (#18679)

* Fix coin fees wrt coin combining

* Add override behavior

Author: Quexington

chia/_tests/cmds/wallet/test_coins.py

@@ -67,14 +67,15 @@ async def combine_coins(
 
     inst_rpc_client = CoinsCombineRpcClient()  # pylint: disable=no-value-for-parameter
     test_rpc_clients.wallet_rpc_client = inst_rpc_client
+    assert sum(coin.amount for coin in STD_TX.removals) < 500_000_000_000
     command_args = [
         "wallet",
         "coins",
         "combine",
         FINGERPRINT_ARG,
         "-i1",
         "--largest-first",
-        "-m0.001",
+        "-m0.5",
         "--min-amount",
         "0.1",
         "--max-amount",
@@ -91,11 +92,13 @@ async def combine_coins(
         "150",
     ]
     # these are various things that should be in the output
+    assert_list = ["Fee is >= the amount of coins selected. To continue, please use --override flag."]
+    run_cli_command_and_assert(capsys, root_dir, command_args, assert_list)
     assert_list = [
         "Transactions would combine up to 500 coins",
         f"To get status, use command: chia wallet get_transaction -f {FINGERPRINT} -tx 0x{STD_TX.name.hex()}",
     ]
-    run_cli_command_and_assert(capsys, root_dir, command_args, assert_list)
+    run_cli_command_and_assert(capsys, root_dir, command_args + ["--override"], assert_list)
     expected_tx_config = TXConfig(
         min_coin_amount=uint64(100_000_000_000),
         max_coin_amount=uint64(200_000_000_000),
@@ -109,13 +112,18 @@ async def combine_coins(
         largest_first=True,
         target_coin_ids=[bytes32([0] * 32)],
         target_coin_amount=uint64(1_000_000_000_000),
-        fee=uint64(1_000_000_000),
+        fee=uint64(500_000_000_000),
         push=False,
     )
     expected_calls: logType = {
-        "get_wallets": [(None,)],
-        "get_synced": [()],
+        "get_wallets": [(None,)] * 2,
+        "get_synced": [()] * 2,
         "combine_coins": [
+            (
+                expected_request,
+                expected_tx_config,
+                test_condition_valid_times,
+            ),
             (
                 expected_request,
                 expected_tx_config,

chia/_tests/wallet/rpc/test_wallet_rpc.py

@@ -3022,3 +3022,85 @@ async def test_combine_coins(wallet_environments: WalletTestFramework) -> None:
             )
         ]
     )
+
+
+@pytest.mark.parametrize(
+    "wallet_environments",
+    [
+        {
+            "num_environments": 1,
+            "blocks_needed": [2],
+            "trusted": True,  # irrelevant
+            "reuse_puzhash": True,  # irrelevant
+        }
+    ],
+    indirect=True,
+)
+@pytest.mark.limit_consensus_modes(reason="irrelevant")
+@pytest.mark.anyio
+async def test_fee_bigger_than_selection_coin_combining(wallet_environments: WalletTestFramework) -> None:
+    """
+    This tests the case where the coins we would otherwise select are not enough to pay the fee.
+    """
+
+    env = wallet_environments.environments[0]
+    env.wallet_aliases = {
+        "xch": 1,
+        "cat": 2,
+    }
+
+    # Should have 4 coins, two 1.75 XCH, two 0.25 XCH
+
+    # Grab one of the 0.25 ones to specify
+    async with env.wallet_state_manager.new_action_scope(wallet_environments.tx_config) as action_scope:
+        target_coin = list(await env.xch_wallet.select_coins(uint64(250_000_000_000), action_scope))[0]
+        assert target_coin.amount == 250_000_000_000
+
+    fee = uint64(1_750_000_000_000)
+    # Under standard circumstances we would select the small coins, but this is not enough to pay the fee
+    # Instead, we will grab the big coin first and combine it with one of the smaller coins
+    xch_combine_request = CombineCoins(
+        wallet_id=uint32(1),
+        number_of_coins=uint16(2),
+        fee=fee,
+        largest_first=False,
+        push=True,
+    )
+
+    # First test an error where fee selection causes too many coins to be selected
+    with pytest.raises(ResponseFailureError, match="without selecting more coins than specified: 3"):
+        await env.rpc_client.combine_coins(
+            dataclasses.replace(xch_combine_request, fee=uint64(2_250_000_000_000)),
+            wallet_environments.tx_config,
+        )
+
+    await env.rpc_client.combine_coins(
+        xch_combine_request,
+        wallet_environments.tx_config,
+    )
+
+    await wallet_environments.process_pending_states(
+        [
+            WalletStateTransition(
+                pre_block_balance_updates={
+                    "xch": {
+                        "unconfirmed_wallet_balance": -fee,
+                        "spendable_balance": -2_000_000_000_000,
+                        "pending_change": 250_000_000_000,
+                        "max_send_amount": -2_000_000_000_000,
+                        "pending_coin_removal_count": 2,
+                    }
+                },
+                post_block_balance_updates={
+                    "xch": {
+                        "confirmed_wallet_balance": -fee,
+                        "spendable_balance": 250_000_000_000,
+                        "pending_change": -250_000_000_000,
+                        "max_send_amount": 250_000_000_000,
+                        "pending_coin_removal_count": -2,
+                        "unspent_coin_count": -1,  # combine 2 into 1
+                    }
+                },
+            )
+        ]
+    )

chia/cmds/coin_funcs.py

@@ -128,6 +128,7 @@ async def async_combine(
     largest_first: bool,
     push: bool,
     condition_valid_times: ConditionValidTimes,
+    override: bool,
 ) -> List[TransactionRecord]:
     async with get_wallet_client(wallet_rpc_port, fingerprint) as (wallet_client, fingerprint, config):
         try:
@@ -167,6 +168,14 @@ async def async_combine(
             timelock_info=condition_valid_times,
         )
 
+        if (
+            not override
+            and wallet_id == 1
+            and fee >= sum(coin.amount for tx in resp.transactions for coin in tx.removals)
+        ):
+            print("Fee is >= the amount of coins selected. To continue, please use --override flag.")
+            return []
+
         print(f"Transactions would combine up to {number_of_coins} coins.")
         if push:
             cli_confirm("Would you like to Continue? (y/n): ")

chia/cmds/coins.py

@@ -108,6 +108,7 @@ def list_cmd(
     default=False,
     help="Sort coins from largest to smallest or smallest to largest.",
 )
+@click.option("--override", help="Submits transaction without checking for unusual values", is_flag=True, default=False)
 @tx_out_cmd()
 def combine_cmd(
     wallet_rpc_port: Optional[int],
@@ -125,6 +126,7 @@ def combine_cmd(
     reuse: bool,
     push: bool,
     condition_valid_times: ConditionValidTimes,
+    override: bool,
 ) -> List[TransactionRecord]:
     from .coin_funcs import async_combine
 
@@ -145,6 +147,7 @@ def combine_cmd(
             largest_first=largest_first,
             push=push,
             condition_valid_times=condition_valid_times,
+            override=override,
         )
     )
 

chia/rpc/wallet_rpc_api.py

@@ -1204,18 +1204,22 @@ async def combine_coins(
         async with action_scope.use() as interface:
             interface.side_effects.selected_coins.extend(coins)
 
-        # Next let's select enough coins to meet the target if there is one
-        if request.target_coin_amount is not None:
-            fungible_amount_needed = request.target_coin_amount
-            if isinstance(wallet, Wallet):
-                fungible_amount_needed = uint64(request.target_coin_amount + request.fee)
-            amount_selected = sum(c.amount for c in coins)
-            if amount_selected < fungible_amount_needed:
-                coins.extend(
-                    await wallet.select_coins(
-                        amount=uint64(fungible_amount_needed - amount_selected), action_scope=action_scope
-                    )
+        # Next let's select enough coins to meet the target + fee if there is one
+        fungible_amount_needed = uint64(0) if request.target_coin_amount is None else request.target_coin_amount
+        if isinstance(wallet, Wallet):
+            fungible_amount_needed = uint64(fungible_amount_needed + request.fee)
+        amount_selected = sum(c.amount for c in coins)
+        if amount_selected < fungible_amount_needed:  # implicit fungible_amount_needed > 0 here
+            coins.extend(
+                await wallet.select_coins(
+                    amount=uint64(fungible_amount_needed - amount_selected), action_scope=action_scope
                 )
+            )
+
+        if len(coins) > request.number_of_coins:
+            raise ValueError(
+                f"Options specified cannot be met without selecting more coins than specified: {len(coins)}"
+            )
 
         # Now let's select enough coins to get to the target number to combine
         if len(coins) < request.number_of_coins:
@@ -1243,6 +1247,7 @@ async def combine_coins(
             uint64(sum(c.amount for c in coins)) if request.target_coin_amount is None else request.target_coin_amount
         )
         if isinstance(wallet, Wallet):
+            primary_output_amount = uint64(primary_output_amount - request.fee)
             await wallet.generate_signed_transaction(
                 primary_output_amount,
                 await wallet.get_puzzle_hash(new=action_scope.config.tx_config.reuse_puzhash),