Return useful error message on ATMP failure #9016

pull instagibbs wants to merge 1 commits into bitcoin:master from instagibbs:atmperror changing 4 files +22 −13

instagibbs commented at 6:33 PM on October 25, 2016: member

Currently the only message reported during RPC usage and tests is the unhelpful

JSONRPC error: Error: The transaction was rejected! This might happen if some of the coins in your wallet were already spent, such as if you used a copy of the wallet and coins were spent in the copy but not marked as spent here.
MarcoFalke added the label Wallet on Oct 25, 2016
MarcoFalke commented at 6:38 PM on October 25, 2016: member

Thanks!

utACK 3df625f49911d0af512dc53d3b9b4a71f3dcd754
in src/wallet/wallet.cpp:None in 3df625f499 outdated
```
2501 | @@ -2501,10 +2502,11 @@ bool CWallet::CommitTransaction(CWalletTx& wtxNew, CReserveKey& reservekey, CCon
2502 |  
2503 |          if (fBroadcastTransactions)
2504 |          {
2505 | +            CValidationState state;
2506 |              // Broadcast
2507 | -            if (!wtxNew.AcceptToMemoryPool(maxTxFee)) {
2508 | +            if (!wtxNew.AcceptToMemoryPool(maxTxFee, state)) {
2509 |                  // This must not fail. The transaction has already been signed and recorded.
```
MarcoFalke commented at 6:39 PM on October 25, 2016:

Nit: I think you can just remove this comment, as it is outdated since at least 0.12.0

instagibbs commented at 6:42 PM on October 25, 2016:

Could you explain how so?

MarcoFalke commented at 6:50 PM on October 25, 2016:

The 0.12.0 release notes says "[...] limits [...] the length and size of unconfirmed transaction chains that are allowed in the mempool (generally limiting the length of unconfirmed chains to 25 transactions, with a total size of 101 KB)."

So claiming this "must not fail" is a somewhat strong claim.

instagibbs commented at 6:53 PM on October 25, 2016:

Oh, is "must not" being used in the descriptive sense rather than prescriptive? (I read it as "oh it better not or there is trouble)If so, we should definitely re-write this.

Also, I've encountered that specific error before so I'd agree with you.

MarcoFalke commented at 7:03 PM on October 25, 2016:

IIRC the trouble means that you end up with transactions in your wallet that may never reach the mempool or a block.

But consider my nit a µnit, it can be done in a later pull.
MarcoFalke added the label Docs and Output on Oct 25, 2016
instagibbs commented at 7:20 PM on October 25, 2016: member

On second thought, I might want to pass the error up through CommitTransaction. Right now it's only logged in debug.log in all the codepaths. Or I could change the error message to direct the user to check the debug log.
laanwj commented at 8:09 AM on October 26, 2016: member

Or I could change the error message to direct the user to check the debug log.

Does this require enabling any debug flags?
laanwj commented at 8:09 AM on October 26, 2016: member

Concept ACK, though I'd really prefer reporting the specific problem inline instead of directing the user at the debug log.
instagibbs force-pushed on Oct 26, 2016
instagibbs commented at 6:52 PM on October 26, 2016: member

@laanwj Done. This should show up during rpc test failures as well,
Return useful error message on ATMP failure 169bdabe14
instagibbs force-pushed on Oct 26, 2016
jonasschnelli commented at 9:06 AM on October 28, 2016: contributor

Nice. utACK 169bdabe14ef5988ff92a7370114edc85b070b27 Possible GUI extension (in another PR) would be to pass the state.GetRejectReason() down to the error display logic and allow the user to get the rejection details in the dialog. But meh.
laanwj commented at 11:49 AM on October 28, 2016: member

utACK 169bdab
laanwj merged this on Oct 28, 2016
laanwj closed this on Oct 28, 2016
laanwj referenced this in commit 0dcb888266 on Oct 28, 2016
MarcoFalke commented at 1:21 PM on October 28, 2016: member

post merge re-utACK 169bdabe14ef5988ff92a7370114edc85b070b27
codablock referenced this in commit a5728f673a on Sep 19, 2017
codablock referenced this in commit 848f3389b4 on Jan 13, 2018
andvgal referenced this in commit 9ecb512f8c on Jan 6, 2019
CryptoCentric referenced this in commit 3237e0952e on Feb 15, 2019
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#9043 [qt] Return useful error message on ATMP failure