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

kouloumos · 2022-09-06T18:02:30Z

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.

bitcoin/src/wallet/spend.cpp

Lines 1013 to 1018 in 447f50e

    
           // Limit size 
        
           if ((sign && GetTransactionWeight(*tx) > MAX_STANDARD_TX_WEIGHT) || 
        
               (!sign && tx_sizes.weight > MAX_STANDARD_TX_WEIGHT)) 
        
           { 
        
               return util::Error{_("Transaction too large")}; 
        
           }

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.

ishaanam

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

src/wallet/rpc/spend.cpp

kouloumos · 2022-09-09T08:44:22Z

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.

ishaanam · 2022-09-12T22:22:27Z

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.

achow101 · 2022-09-13T23:15:19Z

ACK 8d822033bcdf6888b900d5a65ff21f3d8b9d30d5

DrahtBot · 2022-09-14T09:50:34Z

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

Conflicts

No conflicts as of last run.

glozow

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.

glozow · 2022-09-14T10:48:41Z

test/functional/wallet_sendall.py

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

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

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.

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.

src/wallet/rpc/spend.cpp

kouloumos · 2022-09-14T18:47:22Z

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.

achow101 · 2022-09-14T20:27:45Z

ACK 2b4fc04916e3ed43fcf7297ef6d7c9d71148c844

ishaanam · 2022-09-14T21:46:53Z

src/wallet/rpc/spend.cpp

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

Suggested change

throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Transaction too large.");

throw JSONRPCError(RPC_WALLET_ERROR, "Transaction too large.");

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

bitcoin/src/wallet/rpc/spend.cpp

Lines 159 to 162 in 718304d

auto res = CreateTransaction(wallet, recipients, RANDOM_CHANGE_POSITION, coin_control, true);

if (!res) {

throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, util::ErrorString(res).original);

}

RPC_WALLET_ERROR < FundTransaction < CreateTransaction < CreateTransactionInternal

bitcoin/src/wallet/rpc/spend.cpp

Lines 702 to 704 in 718304d

if (!FundTransaction(wallet, tx, fee_out, change_position, error, lockUnspents, setSubtractFeeFromOutputs, coinControl)) {

throw JSONRPCError(RPC_WALLET_ERROR, error.original);

}

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

bitcoin/test/functional/wallet_fallbackfee.py

Lines 27 to 29 in 718304d

assert_raises_rpc_error(-6, "Fee estimation failed", lambda: self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1))

assert_raises_rpc_error(-4, "Fee estimation failed", lambda: self.nodes[0].fundrawtransaction(self.nodes[0].createrawtransaction([], {self.nodes[0].getnewaddress(): 1})))

assert_raises_rpc_error(-6, "Fee estimation failed", lambda: self.nodes[0].sendmany("", {self.nodes[0].getnewaddress(): 1}))

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

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.

w0xlt

ACK 2b4fc04

w0xlt

reACK cc434cb

kouloumos · 2022-09-15T10:48:48Z

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.

glozow · 2022-09-15T11:01:54Z

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

achow101 · 2022-09-15T17:22:21Z

ACK cc434cb

…e check cc434cb wallet: fix sendall creates tx that fails tx-size check (kouloumos) Pull request description: Fixes bitcoin#26011 The `sendall` RPC doesn't use `CreateTransactionInternal` as the rest of the wallet RPCs. [This has already been discussed in the original PR](bitcoin#24118 (comment)). 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._ ACKs for top commit: glozow: re ACK cc434cb via range-diff. Changes were addressing bitcoin#26024 (comment) and bitcoin#26024 (comment). achow101: ACK cc434cb w0xlt: reACK bitcoin@cc434cb Tree-SHA512: 64a1d8f2c737b39f3ccee90689eda1dd9c1b65f11b2c7bc0ec8bfe72f0202ce90ab4033bb0ecfc6080af8c947575059588a00938fe48e7fd553f7fb5ee03b3cc

murchandamus · 2022-09-15T20:17:32Z

post-merge ACK cc434cb

Thanks for catching this.

DrahtBot added the Wallet label Sep 6, 2022

ishaanam reviewed Sep 8, 2022

View reviewed changes

src/wallet/rpc/spend.cpp Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Sep 13, 2022

DrahtBot removed the Needs rebase label Sep 14, 2022

DrahtBot mentioned this pull request Sep 14, 2022

sendall: check if the maxtxfee has been exceeded #26084

Merged

fanquake added this to the 24.0 milestone Sep 14, 2022

fanquake requested a review from glozow September 14, 2022 09:46

glozow reviewed Sep 14, 2022

View reviewed changes

kouloumos force-pushed the wallet-fix-sendall-tx-size branch from 8d82203 to 2b4fc04 Compare September 14, 2022 18:45

achow101 mentioned this pull request Sep 14, 2022

sendall creates tx that fails tx-size mempool check #26011

Closed

ishaanam reviewed Sep 14, 2022

View reviewed changes

DrahtBot added the Needs rebase label Sep 15, 2022

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

cc434cb

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.

w0xlt approved these changes Sep 15, 2022

View reviewed changes

kouloumos force-pushed the wallet-fix-sendall-tx-size branch from 2b4fc04 to cc434cb Compare September 15, 2022 10:40

w0xlt approved these changes Sep 15, 2022

View reviewed changes

DrahtBot removed the Needs rebase label Sep 15, 2022

achow101 merged commit a56876e into bitcoin:master Sep 15, 2022

bitcoin locked and limited conversation to collaborators Sep 15, 2023

	// Limit size
	if ((sign && GetTransactionWeight(*tx) > MAX_STANDARD_TX_WEIGHT) \|\|
	(!sign && tx_sizes.weight > MAX_STANDARD_TX_WEIGHT))
	{
	return util::Error{_("Transaction too large")};
	}

	throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Transaction too large.");
	throw JSONRPCError(RPC_WALLET_ERROR, "Transaction too large.");

	auto res = CreateTransaction(wallet, recipients, RANDOM_CHANGE_POSITION, coin_control, true);
	if (!res) {
	throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, util::ErrorString(res).original);
	}

	if (!FundTransaction(wallet, tx, fee_out, change_position, error, lockUnspents, setSubtractFeeFromOutputs, coinControl)) {
	throw JSONRPCError(RPC_WALLET_ERROR, error.original);
	}

	assert_raises_rpc_error(-6, "Fee estimation failed", lambda: self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1))
	assert_raises_rpc_error(-4, "Fee estimation failed", lambda: self.nodes[0].fundrawtransaction(self.nodes[0].createrawtransaction([], {self.nodes[0].getnewaddress(): 1})))
	assert_raises_rpc_error(-6, "Fee estimation failed", lambda: self.nodes[0].sendmany("", {self.nodes[0].getnewaddress(): 1}))

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

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

Uh oh!

Conversation

kouloumos commented Sep 6, 2022

Uh oh!

ishaanam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kouloumos commented Sep 9, 2022

Uh oh!

ishaanam commented Sep 12, 2022

Uh oh!

achow101 commented Sep 13, 2022

Uh oh!

DrahtBot commented Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

glozow Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

kouloumos Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

glozow Sep 15, 2022

Choose a reason for hiding this comment

Uh oh!

kouloumos Sep 15, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kouloumos commented Sep 14, 2022

Uh oh!

achow101 commented Sep 14, 2022

Uh oh!

ishaanam Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

kouloumos Sep 15, 2022

Choose a reason for hiding this comment

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

kouloumos commented Sep 15, 2022

Uh oh!

glozow commented Sep 15, 2022

Uh oh!

achow101 commented Sep 15, 2022

Uh oh!

murchandamus commented Sep 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

DrahtBot commented Sep 14, 2022 •

edited

Loading