wallet: Remove return value from CommitTransaction #17154

jnewbery · 2019-10-15T18:53:51Z

CommitTransaction() returns a bool to indicate success, but since commit
b3a7410 (#9302) it only returns true, even if the transaction was not
successfully broadcast. This commit changes CommitTransaction() to return
void.

All dead code in if (!CommitTransaction()) branches has been removed.

Two additional commits fix up the idiosyncratic whitespace in CommitTransaction and add a doxygen comment for the function.

maflcko · 2019-10-15T19:25:20Z

Slightly tend to NACK. What is this needed for?

Effectively, a gui user will send the transaction twice (spending different coins), losing all their funds.

jnewbery · 2019-10-15T19:47:11Z

@MarcoFalke

Slightly tend to NACK. What is this needed for?

Currently the function returns a bool, which is always true, but the callers have logic that depends on the return value.

CommitTransaction() did previously return true/false based on success. That was removed in #9302 so that the send___ RPC methods would return the txid even if the transaction failed to broadcast. That's useful because the transaction will still exist in the wallet, so the RPC user would want to know about it. The fix should have been to have the call sites ignore the return value of CommitTransaction(), but instead the PR changed the function itself to not return success/failure, meaning that as a side effect WalletModel::sendCoins() is no longer informed if transaction broadcast fails. From reading the PR, it looks to me like that change was unintentional.

I could change this PR to maintain the current behaviour (not return success to WalletModel::sendCoins()). Do you think that's preferable?

maflcko · 2019-10-15T20:45:00Z

From reading the PR, it looks to me like that change was unintentional.

I am pretty sure it was intentional. At least that was my understanding, see #9302 (comment)

To sum up, the wallet is only allowed to do one of the following:

Create a tx, sign it and add it to the wallet, so that it can be rebroadcast immediately or later, then return true
Create a tx, sign it and throw it away for some reason, then return false

A wallet is not allowed to create a tx, sign it and add it to the wallet, so that it will be rebroadcast later, but return false. <-- This is what you pull request is doing.

DrahtBot · 2019-10-15T20:47:37Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16966 (ui: make send a wizard by Sjors)
#16411 (Signet support by kallewoof)
#15931 (Remove GetDepthInMainChain dependency on locked chain interface by ariard)
#15921 (validation: Tidy up ValidationState interface by jnewbery)
#14582 (wallet: try -avoidpartialspends mode and use its result if fees do not change by kallewoof)
#9381 (Remove CWalletTx merging logic from AddToWallet by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jnewbery · 2019-10-15T20:59:29Z

In that case, CommitTransaction should not return anything, and all the dead code in if (!CommitTransaction()) blocks should be removed.

maflcko · 2019-10-15T22:04:33Z

Fine with me

Reviewer hint: use --ignore-all-space git diff option for review.

jnewbery · 2019-10-16T15:19:48Z

This PR now just changes CommitTransaction to return void, and deletes all dead code.

Also rebased on master.

src/wallet/wallet.h

achow101

Code Review ACK 5021b8a

src/wallet/wallet.h

jnewbery · 2019-10-16T20:48:11Z

Addressed @achow101 's review comment (thanks!)

achow101 · 2019-10-16T21:19:18Z

ACK 1f490c9

promag · 2019-10-17T13:12:08Z

Concept ACK.

src/wallet/wallet.h

CommitTransaction returns a bool to indicate success, but since commit b3a7410 it only returns true, even if the transaction was not successfully broadcast. This commit changes CommitTransaction() to return void. All dead code in `if (!CommitTransaction())` branches has been removed.

The `state` return argument has not been set since commit 611291c. Remove it (and the one place that it's used in a calling function).

maflcko · 2019-10-18T14:03:29Z

Concept ACK

laanwj · 2019-10-23T11:37:22Z

ACK 9e95931

9e95931 [wallet] Remove `state` argument from CWallet::CommitTransaction (John Newbery) d1734f9 [wallet] Remove return value from CommitTransaction() (John Newbery) b6f486a [wallet] Add doxygen comment to CWallet::CommitTransaction() (John Newbery) 8bba91b [wallet] Fix whitespace in CWallet::CommitTransaction() (John Newbery) Pull request description: `CommitTransaction()` returns a bool to indicate success, but since commit b3a7410 (#9302) it only returns true, even if the transaction was not successfully broadcast. This commit changes CommitTransaction() to return void. All dead code in `if (!CommitTransaction())` branches has been removed. Two additional commits fix up the idiosyncratic whitespace in `CommitTransaction` and add a doxygen comment for the function. ACKs for top commit: laanwj: ACK 9e95931 Tree-SHA512: a55a2c20369a45222fc0e02d0891495655a926e71c4f52cb72624768dd7b9c1dca716ea67d38420afb90f40c6e0fd448caa60c18fd693bb10ecb110b641820e6

JeremyRubin · 2019-11-24T06:17:21Z

Hmmm -- sorry for a posthumous comment here, but this patch confuses me a little.

It seems the better change would be to continue to have CommitTransaction return a success value/reject reason.

What's the recommended pattern following this change for checking the status of a committed transaction? What if a transaction is rejected for consensus reasons, and won't be in the wallet? As a wallet rpc implementer, I want to be able to detect a failed committransaction and then undo some prior steps...

jnewbery · 2019-11-24T15:46:13Z

This PR didn't change any behaviour. It removed a return value that was not used by any calling code.

See #9302 for why the RPC code was changed.

maflcko · 2019-11-24T16:21:28Z

And the TODO that this should, is still in the code:

     // TODO: if we expect the failure to be long term or permanent, instead delete wtx from the wallet and return failure.

JeremyRubin · 2019-11-24T19:58:50Z

Yeah I read those; my point is that it seems that the patched behavior of the function is buggy since #9302, and then in #17154 we just enforce the buggy behavior rather than addressing the TODO and returning an error code, which users of the function may prefer to have...

jnewbery · 2019-11-24T21:49:58Z

Contributions are welcome. If you want to suggest a PR to return failure information from CommitTransaction(), please go ahead.

maflcko · 2019-11-24T23:17:07Z

Instead of creating a transaction that is consensus-invalid and then returning a failure and removing the tx again, I'd prefer if the consensus-invalid transaction was not created by the wallet in the first place. Or if that is not possible, the wallet should have checked consensus-validity before storing it in the wallet.dat

JeremyRubin · 2019-11-25T06:01:05Z

@MarcoFalke well I don't disagree it's better to never be wrong in the first place! But the committransaction also seems to be able to fail for mere policy reasons too, right?

@jnewbery My original question was, in light of this patch, if there is actually a recommended handling for transactions which may err, what's the "proper handling" to recover the relevant information here?

I'd be happy to make a PR which makes the changes but I don't really fully understand what we want functionality wise from CommitTransaction.

maflcko · 2019-11-25T18:03:47Z

also seems to be able to fail for mere policy reasons too, right

Yes.
If the policy fail is expected to be short term, the wallet should continue as usual and commit the tx to the wallet.dat. If the policy fail is expected to be long term or permanent, the wallet shouldn't commit the tx in the first place.

I think you are arguing that the functionality to check the tx should happen in CommitTransaction and that this pull request made it harder to do so. I think we can only know best where to put this check after someone has implemented it. It might be better as a standalone function PreCheckCommitTransaction or similar, idk.

a recommended handling for transactions which may err

I think we don't have a recommendation to recover from such errors. The best we can do right now is manual fixups: abandontransaction (maybe in combination with disabled wallet rebroadcast).

jnewbery · 2019-11-25T18:47:59Z

I think the most critical thing in CommitTransaction() is that the transaction is added to the wallet and old coins are marked spent before the transaction is broadcast. It would be a problem if the transaction was broadcast and then failed to be added to the wallet.

I think adding a call to ATMP with test_accept before calling AddToWallet() and then returning any error could be an improvement.

…ansaction() Summary: bitcoin/bitcoin@b6f486a note: the first commit (bitcoin/bitcoin@8bba91b) didn't really apply to us. --- Depends on D7111 Partial backport of Core [[bitcoin/bitcoin#17154 | PR17154]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7112

…on() Summary: CommitTransaction returns a bool to indicate success, but since commit b3a7410 it only returns true, even if the transaction was not successfully broadcast. This commit changes CommitTransaction() to return void. All dead code in `if (!CommitTransaction())` branches has been removed. bitcoin/bitcoin@d1734f9 --- Depends on D7112 Partial backport of Core [[bitcoin/bitcoin#17154 | PR17154]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7113

…mmitTransaction Summary: The `state` return argument has not been set since commit 611291c198. Remove it (and the one place that it's used in a calling function). bitcoin/bitcoin@9e95931 --- Depends on D7113 Concludes backport of Core [[bitcoin/bitcoin#17154 | PR17154]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7114

jnewbery force-pushed the 2019-04-CommitTransaction branch from 77d8b78 to 25edffb Compare October 15, 2019 18:57

fanquake added the Wallet label Oct 15, 2019

fanquake changed the title ~~[wallet] Actually return success in CommitTransaction~~ wallet: Actually return success in CommitTransaction Oct 15, 2019

jnewbery force-pushed the 2019-04-CommitTransaction branch from 25edffb to 13c2cc4 Compare October 16, 2019 14:56

jnewbery changed the title ~~wallet: Actually return success in CommitTransaction~~ wallet: Remove return value from in CommitTransaction Oct 16, 2019

[wallet] Fix whitespace in CWallet::CommitTransaction()

8bba91b

Reviewer hint: use --ignore-all-space git diff option for review.

jnewbery force-pushed the 2019-04-CommitTransaction branch from 13c2cc4 to d3e6e19 Compare October 16, 2019 15:19

maflcko reviewed Oct 16, 2019

View reviewed changes