wallet: Remove return value from CommitTransaction #17154

jnewbery commented at 6:53 PM on October 15, 2019: member

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.

jnewbery force-pushed on Oct 15, 2019

fanquake added the label Wallet on Oct 15, 2019

MarcoFalke commented at 7:25 PM on October 15, 2019: member

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.

fanquake renamed this:
~~[wallet] Actually return success in CommitTransaction~~
wallet: Actually return success in CommitTransaction
on Oct 15, 2019

jnewbery commented at 7:47 PM on October 15, 2019: member

@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?

MarcoFalke commented at 8:45 PM on October 15, 2019: member

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 commented at 8:47 PM on October 15, 2019: member

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 commented at 8:59 PM on October 15, 2019: member

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

MarcoFalke commented at 10:04 PM on October 15, 2019: member

Fine with me

jnewbery force-pushed on Oct 16, 2019

jnewbery renamed this:
~~wallet: Actually return success in CommitTransaction~~
wallet: Remove return value from in CommitTransaction
on Oct 16, 2019

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

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

8bba91b22d

jnewbery force-pushed on Oct 16, 2019

jnewbery commented at 3:19 PM on October 16, 2019: member

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

Also rebased on master.

in src/wallet/wallet.h:1161 in d3e6e19d58 outdated

1157 | +     * @param mapValue[in] key-values to be set on the transaction.
1158 | +     * @param orderForm[in] BIP 70 / BIP 21 order form details to be set on the transaction.
1159 | +     * @param reserveKey[in,out] keypool key used for change
1160 | +     * @param state[in,out] CValidationState object returning information about whether the transaction was accepted
1161 | +     */
1162 | +void CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector<std::pair<std::string, std::string>> orderForm, CValidationState& state);

MarcoFalke commented at 3:36 PM on October 16, 2019:

    void CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector<std::pair<std::string, std::string>> orderForm, CValidationState& state);

:eyes:

jnewbery commented at 4:03 PM on October 16, 2019:

fixed

jnewbery force-pushed on Oct 16, 2019

in src/wallet/wallet.h:1158 in 20200a038c outdated

1153 | +     * broadcasting the transaction.
1154 | +     *
1155 | +     * @param tx[in,out] The transaction to be broadcast.
1156 | +     * @param mapValue[in] key-values to be set on the transaction.
1157 | +     * @param orderForm[in] BIP 70 / BIP 21 order form details to be set on the transaction.
1158 | +     * @param reserveKey[in,out] keypool key used for change

achow101 commented at 8:31 PM on October 16, 2019:

reserveKey is not an argument for this function.

jnewbery commented at 8:47 PM on October 16, 2019:

fixed

achow101 commented at 8:36 PM on October 16, 2019: member

Code Review ACK 5021b8a7fb4de518155146de6cac231e91e874f8

jnewbery force-pushed on Oct 16, 2019

jnewbery commented at 8:48 PM on October 16, 2019: member

Addressed @achow101 's review comment (thanks!)

achow101 commented at 9:19 PM on October 16, 2019: member

ACK 1f490c9a5f8767477f6ffe382b4a0578dbac5e3e

promag commented at 1:12 PM on October 17, 2019: member

Concept ACK.

in src/wallet/wallet.h:1155 in c7397e0749 outdated

1146 | @@ -1147,6 +1147,16 @@ class CWallet final : public FillableSigningProvider, private interfaces::Chain:
1147 |       */
1148 |      bool CreateTransaction(interfaces::Chain::Lock& locked_chain, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut,
1149 |                             std::string& strFailReason, const CCoinControl& coin_control, bool sign = true);
1150 | +    /**
1151 | +     * Submit the transaction to the node's mempool and then relay to peers.
1152 | +     * Should be called after CreateTransaction unless you want to abort
1153 | +     * broadcasting the transaction.
1154 | +     *
1155 | +     * @param tx[in,out] The transaction to be broadcast.

MarcoFalke commented at 6:55 PM on October 17, 2019:

in commit c7397e074996086578d9ac3db78d654fdbbf58a5:

this is not a return parameter. The tx is consumed (std::move)

jnewbery commented at 1:39 PM on October 18, 2019:

Thanks. Fixed.

in src/wallet/wallet.h:1158 in c7397e0749 outdated

1153 | +     * broadcasting the transaction.
1154 | +     *
1155 | +     * @param tx[in,out] The transaction to be broadcast.
1156 | +     * @param mapValue[in] key-values to be set on the transaction.
1157 | +     * @param orderForm[in] BIP 70 / BIP 21 order form details to be set on the transaction.
1158 | +     * @param state[in,out] CValidationState object returning information about whether the transaction was accepted

MarcoFalke commented at 6:56 PM on October 17, 2019:

I think this should just be [out]?

jnewbery commented at 1:39 PM on October 18, 2019:

This actually isn't set any more (since commit 611291c198). I've removed it in a new commit.

[wallet] Add doxygen comment to CWallet::CommitTransaction() b6f486a02b

[wallet] Remove return value from CommitTransaction()

CommitTransaction returns a bool to indicate success, but since commit
b3a74100b8 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.

d1734f9a3b

in src/wallet/wallet.h:1160 in 1f490c9a5f outdated

1156 | @@ -1157,7 +1157,7 @@ class CWallet final : public FillableSigningProvider, private interfaces::Chain:
1157 |       * @param orderForm[in] BIP 70 / BIP 21 order form details to be set on the transaction.
1158 |       * @param state[in,out] CValidationState object returning information about whether the transaction was accepted
1159 |       */
1160 | -    bool CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector<std::pair<std::string, std::string>> orderForm, CValidationState& state);
1161 | +    void CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector<std::pair<std::string, std::string>> orderForm, CValidationState& state);

MarcoFalke commented at 7:01 PM on October 17, 2019:

in commit 1f490c9a5f8767477f6ffe382b4a0578dbac5e3e you claim that the return value is removed, but you still return state, which is used in a single place (feebumper). Haven't thought too much about this, but it seems fine to use there because it can't lead to loss of funds?

Also, that code path shouldn't be hit unless there is a sudden massive fee spike or a logic error.

jnewbery commented at 1:39 PM on October 18, 2019:

I've removed state, since it's never actually set.

MarcoFalke commented at 2:02 PM on October 18, 2019:

ouch. Good catch :grimacing:

jnewbery force-pushed on Oct 18, 2019

[wallet] Remove `state` argument from CWallet::CommitTransaction

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).

9e95931865

jnewbery force-pushed on Oct 18, 2019

MarcoFalke commented at 2:03 PM on October 18, 2019: member

Concept ACK

laanwj renamed this:
~~wallet: Remove return value from in CommitTransaction~~
wallet: Remove return value from CommitTransaction
on Oct 23, 2019

laanwj commented at 11:37 AM on October 23, 2019: member

ACK 9e95931865186d7a9a6dc54b64bd96507e9fea4b

laanwj referenced this in commit 8a191148db on Oct 24, 2019

laanwj merged this on Oct 24, 2019

laanwj closed this on Oct 24, 2019

MarkLTZ referenced this in commit 0815981dfe on Nov 17, 2019

JeremyRubin commented at 6:17 AM on November 24, 2019: contributor

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 commented at 3:46 PM on November 24, 2019: member

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.

jnewbery deleted the branch on Nov 24, 2019

MarcoFalke commented at 4:21 PM on November 24, 2019: member

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 commented at 7:58 PM on November 24, 2019: contributor

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 commented at 9:49 PM on November 24, 2019: member

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

MarcoFalke commented at 11:17 PM on November 24, 2019: member

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 commented at 6:01 AM on November 25, 2019: contributor

@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.

MarcoFalke commented at 6:03 PM on November 25, 2019: member

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 commented at 6:47 PM on November 25, 2019: member

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.

jasonbcox referenced this in commit 973e07b9f4 on Aug 5, 2020

jasonbcox referenced this in commit 5e00d48938 on Aug 5, 2020

jasonbcox referenced this in commit a87b1b6c04 on Aug 5, 2020

MarcoFalke locked this on Dec 16, 2021