wallet: fix sendall creates tx that fails tx-size check #26024

pull kouloumos wants to merge 1 commits into bitcoin:master from kouloumos:wallet-fix-sendall-tx-size changing 2 files +22 −0
  1. kouloumos commented at 6:02 PM on September 6, 2022: contributor

    Fixes #26011

    The sendall RPC doesn't use CreateTransactionInternal as the rest of the wallet RPCs. This has already been discussed in the original PR. By not going through that path, it never checks the transaction's weight against the maximum tx weight for transactions we're willing to relay. https://github.com/bitcoin/bitcoin/blob/447f50e4aed9a8b1d80e1891cda85801aeb80b4e/src/wallet/spend.cpp#L1013-L1018 This PR adds a check for tx-size as well as test coverage for that case.

    Note: It seems that the test takes a bit of time on slower machines, I'm not sure if dropping it might be for the better.

  2. DrahtBot added the label Wallet on Sep 6, 2022
  3. in src/wallet/rpc/spend.cpp:1420 in 8d822033bc outdated 
    1405 | @@ -1406,6 +1406,11 @@ RPCHelpMan sendall()
1406 |                  }
1407 |              }
1408 |  
1409 | +            // Limit size
1410 | +            if (tx_size.weight > MAX_STANDARD_TX_WEIGHT) {
1411 | +                throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Transaction too large.");
1412 | +            }
    ishaanam commented at 11:08 PM on September 8, 2022:

    Might this check make more sense if it were placed in FinishTransaction (which is called by send and sendall) within the section that is only performed if the transaction is complete? The advantages of this would be that A) this check is now added for both sendall and send, and B) this check isn't performed if the transaction doesn't contain a complete set of signatures.

    kouloumos commented at 8:14 AM on September 9, 2022:

    send is using FundTransaction which then calls the wallet::FundTransaction which uses CreateTransaction therefore performing those checks.

    ishaanam commented at 12:09 AM on September 12, 2022:

    Ok, I hadn't noticed that.

  4. ishaanam commented at 11:20 PM on September 8, 2022: contributor

    Concept ACK It may be out of the scope of this PR to fix this, but I wanted to note that it seems like there are a few other checks that CreateTransactionInternal performs that aren't implemented in sendall but that I think would still be relevant. Namely:

    • whether the feerate exceeds the default maximum feerate set by the user
    • whether the tx will pass the mempool's chain limits
  5. kouloumos commented at 8:44 AM on September 9, 2022: contributor
    • whether the feerate exceeds the default maximum feerate set by the user

    There is already this check against the fee_rate passed by the user. I can't find any other relevant check at CreateTransactionInternal. Could you give more details on that?

    • whether the tx will pass the mempool's chain limits

    My understanding is that this is not a problem because this RPC only uses confirmed UTXOs.

  6. ishaanam commented at 10:22 PM on September 12, 2022: contributor

    I can't find any other relevant check at CreateTransactionInternal. Could you give more details on that?

    I meant this check, which checks that the total fee is not greater than the maximum fee (set using -maxtxfee). Earlier I should have said fee instead of feerate.

  7. DrahtBot added the label Needs rebase on Sep 13, 2022
  8. achow101 commented at 11:15 PM on September 13, 2022: member

    ACK 8d822033bcdf6888b900d5a65ff21f3d8b9d30d5

  9. DrahtBot removed the label Needs rebase on Sep 14, 2022
  10. fanquake added this to the milestone 24.0 on Sep 14, 2022
  11. fanquake requested review from glozow on Sep 14, 2022
  12. DrahtBot commented at 9:50 AM on September 14, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  13. in test/functional/wallet_sendall.py:279 in 8d822033bc outdated 
     263 | @@ -264,6 +264,20 @@ def sendall_fails_on_specific_inputs_with_send_max(self):
 264 |              recipients=[self.remainder_target],
 265 |              options={"inputs": [utxo], "send_max": True})
 266 |  
 267 | +    # This tests needs to be the last one otherwise @cleanup will fail with "Transaction too large" error
    glozow commented at 10:48 AM on September 14, 2022:

    Sendall being able to create multiple transactions would also automatically resolve this problem.

    kouloumos commented at 6:46 PM on September 14, 2022:

    Do you see this multiple transactions capability to be helpful on any other scenario except of when the wallet has many UTXOs?

    glozow commented at 10:05 AM on September 15, 2022:

    No. But it seems quite limiting to just not be able to sendall when the wallet gets bigger. I might be wrong but isn't consolidation one of its use cases? And it's a bit ugly to not be able to use cleanup after this test. Anyway I don't want to hold up this PR, so feel free to resolve.

    kouloumos commented at 10:24 AM on September 15, 2022:

    It was more of a curiosity question. Your argument seems valid, as per the current functional test, you need ~1600 inputs to reach the limit.

  14. in src/wallet/rpc/spend.cpp:1409 in 8d822033bc outdated 
    1405 | @@ -1406,6 +1406,11 @@ RPCHelpMan sendall()
1406 |                  }
1407 |              }
1408 |  
1409 | +            // Limit size
    glozow commented at 10:54 AM on September 14, 2022: 
                // If this transaction is too large, e.g. because the wallet has many UTXOs, it will be rejected by the node's mempool.
    kouloumos commented at 6:46 PM on September 14, 2022:

    Done, definitely adds more context.

  15. glozow commented at 11:09 AM on September 14, 2022: member

    utACK 8d822033bcdf6888b900d5a65ff21f3d8b9d30d5

    Agree we should stop sendall from creating a nonstandard tx. I think a more long term solution would be to still enable sendall to do what it's supposed to do, but create multiple transactions.

  16. kouloumos force-pushed on Sep 14, 2022
  17. kouloumos commented at 6:47 PM on September 14, 2022: contributor

    which checks that the total fee is not greater than the maximum fee (set using -maxtxfee).

    Good catch, I see that this change is now #26084.

    Force pushed only to add more context to the added check.

  18. achow101 commented at 8:27 PM on September 14, 2022: member

    ACK 2b4fc04916e3ed43fcf7297ef6d7c9d71148c844

  19. in src/wallet/rpc/spend.cpp:1411 in 2b4fc04916 outdated 
    1405 | @@ -1406,6 +1406,11 @@ RPCHelpMan sendall()
1406 |                  }
1407 |              }
1408 |  
1409 | +            // If this transaction is too large, e.g. because the wallet has many UTXOs, it will be rejected by the node's mempool.
1410 | +            if (tx_size.weight > MAX_STANDARD_TX_WEIGHT) {
1411 | +                throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Transaction too large.");
    ishaanam commented at 9:46 PM on September 14, 2022:

    I think that the error code should not indicate that the wallet has insufficient funds.

                    throw JSONRPCError(RPC_WALLET_ERROR, "Transaction too large.");
    kouloumos commented at 9:37 AM on September 15, 2022:

    I initially tracked what error code the same check was returning from CreateTransactionInternal.

    I now see that the 2 paths to call CreateTransaction return a different error code:

    RPC_WALLET_INSUFFICIENT_FUNDS < SendMoney < CreateTransaction < CreateTransactionInternal https://github.com/bitcoin/bitcoin/blob/718304d222671f98d2335cd9b90a3022f62d7b21/src/wallet/rpc/spend.cpp#L159-L162

    RPC_WALLET_ERROR < FundTransaction < CreateTransaction < CreateTransactionInternal https://github.com/bitcoin/bitcoin/blob/718304d222671f98d2335cd9b90a3022f62d7b21/src/wallet/rpc/spend.cpp#L702-L704

    I am not sure if I am missing something but it seems that there is ambiguity. Probably because CreateTransactionInternal includes a variety of checks that in the end need to be represented with a single error code.

    For example, a failed fee estimation is returned differently based on the RPC https://github.com/bitcoin/bitcoin/blob/718304d222671f98d2335cd9b90a3022f62d7b21/test/functional/wallet_fallbackfee.py#L27-L29

    Nevertheless, I agree with you that in this case RPC_WALLET_ERROR is more appropriate. I will change it together with the needed rebase.

  20. DrahtBot added the label Needs rebase on Sep 15, 2022
  21. wallet: fix sendall creates tx that fails tx-size check
    The `sendall` RPC doesn't use `CreateTransactionInternal`as the rest of
the wallet RPCs and it never checks against the tx-size mempool limit.
Add a check for tx-size as well as test coverage for that case.
    cc434cbf58
  22. w0xlt approved
  24. kouloumos force-pushed on Sep 15, 2022
  25. w0xlt approved
  27. kouloumos commented at 10:48 AM on September 15, 2022: contributor

    Thank you all for the reviews!

    A rebase was needed after the merge of #26084. Also force pushed to change the error code from RPC_WALLET_INSUFFICIENT_FUNDS to RPC_WALLET_ERROR based on @ishaanam's comment.

  28. DrahtBot removed the label Needs rebase on Sep 15, 2022
  29. glozow commented at 11:01 AM on September 15, 2022: member

    re ACK cc434cb via range-diff. Changes were addressing #26024 (review) and #26024 (review).

  30. achow101 commented at 5:22 PM on September 15, 2022: member

    ACK cc434cbf583ec8d1b0f3aa504417231fdc81f33a

  31. achow101 merged this on Sep 15, 2022
  32. achow101 closed this on Sep 15, 2022
  33. sidhujag referenced this in commit e91fcc5403 on Sep 15, 2022
  34. murchandamus commented at 8:17 PM on September 15, 2022: contributor

    post-merge ACK cc434cbf583ec8d1b0f3aa504417231fdc81f33a

    Thanks for catching this.

  35. bitcoin locked this on Sep 15, 2023
Contributors
kouloumosishaanamachow101DrahtBotglozoww0xltmurchandamus
Labels
Wallet

Milestone
24.0

Linked (view graph)
#24118 Add 'sendall' RPC née sweep#26011 sendall creates tx that fails tx-size mempool check
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-17 06:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me