Closes: #19363
Renaming this error improves clarity around the returned error both internally and externally when a transactions’ outputs are already found in the utxo set (TransactionError::ALREADY_IN_CHAIN -> TransactionError::ALREADY_IN_UTXO_SET)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | tdb3, ismaelsadeeq, ryanofsky |
| Stale ACK | theStack |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
This addresses #19363 ’s concern:
If a transaction is already in the block chain, and all of its outputs are spent, the BroadcastTransaction() fails to detect the correct transaction status. It returns TransactionError::MISSING_INPUTS instead of TransactionError::ALREADY_IN_CHAIN.
…by now returning TransactionError::INPUTS_MISSING_OR_SPENT. The translation for this error message already indicated this error may mean the inputs were missing or spent.
Concept and code-review ACK aa422bcb898b7e3aa8dca912d92016bd22cf243d
I agree that the proposed names are more to the point and less confusing.
21@@ -22,7 +22,7 @@ static TransactionError HandleATMPError(const TxValidationState& state, std::str
22 err_string_out = state.ToString();
23 if (state.IsInvalid()) {
24 if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) {
25- return TransactionError::MISSING_INPUTS;
Nice, I think we should also rename the one in TxValidationResult enum.
g/TX_MISSING_INPUTS/TX_MISSING_INPUT_OR_SPENT/g
and update comments appropriately.
I did consider this, but it felt much less exposed to the user, only via testmempoolaccept AFAIK:
0 result_inner.pushKV("reject-reason", "missing-inputs");
Therefore as an “internal error”, TX_MISSING_INPUTS (for any reason) it felt more accurate to me to leave as is? Very happy to be persuaded away from this viewpoint though :)
Therefore as an “internal error”, TX_MISSING_INPUTS (for any reason) it felt more accurate to me to leave as is?
I was reviewing this based on the PR rationale in the OP. You stated that “a transaction’s inputs are missing from the UTXO set, which could also be due to all inputs having already been spent (MISSING_INPUTS -> INPUTS_MISSING_OR_SPENT).” That’s why I suggested that TxValidationResult one should be changed just for uniformity. But Internally, you’re right TX_MISSING_INPUTS inferred or spent.
Maybe just update the testmempoolaccept reject reason since it’s user-facing?
But if you agree, you may have to update some functional tests that use it and maybe a release note since users heavily use the RPC.
This is a non blocking comment.
This PR is an improvement and will close the issue
I also don’t understand why the first commit is needed. The error message is already correct (and unchanged in this pull request): Untranslated("Inputs missing or spent").
I’d say it is fine to drop the commit (or keep it if you want). Just a style nit, either is fine.
Rebased on master to fix merge conflict.
Added an extra commit to cover PSBTError::MISSING_INPUTS.
651@@ -652,7 +652,7 @@ std::optional<PSBTError> LegacyScriptPubKeyMan::FillPSBT(PartiallySignedTransact
652 // Check non_witness_utxo has specified prevout
653 if (input.non_witness_utxo) {
654 if (txin.prevout.n >= input.non_witness_utxo->vout.size()) {
655- return PSBTError::MISSING_INPUTS;
656+ return PSBTError::INPUTS_MISSING_OR_SPENT;
In commit “refactor: PSBTError::MISSING_INPUTS” (b2b4b932572c5bb1ffa3fc4f34e17130348fbc24)
Does the name INPUTS_MISSING_OR_SPENT actually make sense to describe these two errors from FillPSBT? I’m not sure, but it seems these errors would only happen if the PSBT data was not internally consistent, so maybe it would be more appropriate to call this error something like PSBTError::INPUTS_INVALID?
Yes I agree on re-reading this that this error does not quite correctly describe the specific check being failed here, which is that the outpoints vout is not higher than the number of vouts in the input, i.e. it is by definition invalid.
I have taken your suggestion on the wording in 77ef75d3ce4bff76a0db730b57896dcf7e8f3988
77@@ -78,8 +78,8 @@ bool FeeModeFromString(const std::string& mode_string, FeeEstimateMode& fee_esti
78 bilingual_str PSBTErrorString(PSBTError err)
79 {
80 switch (err) {
81- case PSBTError::MISSING_INPUTS:
82- return Untranslated("Inputs missing or spent");
83+ case PSBTError::INPUTS_INVALID:
84+ return Untranslated("Invalid inputs specified");
In commit “refactor: PSBTError::MISSING_INPUTS” (77ef75d3ce4bff76a0db730b57896dcf7e8f3988):
I think it would be a little better if this message said just “Invalid inputs” instead of “Invalid inputs specified” since the input indices are internal to the PSBT data structure, not really specified by the user.
Someone more familiar with PSBT code than me might be able to suggest a better error code and message than this to use, but at least with this change the error should be more accurate.
Note: a test currently fails because this string is changing:
0File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/rpc_psbt.py", line 707, in run_test
1 assert_raises_rpc_error(-25, 'Inputs missing or spent', self.nodes[0].walletprocesspsbt, 'cHNidP8BAJoCAAAAAkvEW8NnDtdNtDpsmze+Ht2LH35IJcKv00jKAlUs21RrAwAAAAD/////S8Rbw2cO1020OmybN74e3Ysffkglwq/TSMoCVSzbVGsBAAAAAP7///8CwLYClQAAAAAWABSNJKzjaUb3uOxixsvh1GGE3fW7zQD5ApUAAAAAFgAUKNw0x8HRctAgmvoevm4u1SbN7XIAAAAAAAEAnQIAAAACczMa321tVHuN4GKWKRncycI22aX3uXgwSFUKM2orjRsBAAAAAP7///9zMxrfbW1Ue43gYpYpGdzJwjbZpfe5eDBIVQozaiuNGwAAAAAA/v///wIA+QKVAAAAABl2qRT9zXUVA8Ls5iVqynLHe5/vSe1XyYisQM0ClQAAAAAWABRmWQUcjSjghQ8/uH4Bn/zkakwLtAAAAAAAAQEfQM0ClQAAAAAWABRmWQUcjSjghQ8/uH4Bn/zkakwLtAAAAA==')
Code review ACK 77ef75d3ce4bff76a0db730b57896dcf7e8f3988 if test failure is fixed (see comment below)
Note: I didn’t totally follow the original issue or how these changes resolve it, but all the changes here seem reasonable regardless.
Code review ACK a05f4ca241045e8a4b295760590805f961c2e5e7.
Just changed psbt error string and fixed a test since the last review.
436 for node in self.nodes:
437 testres = node.testmempoolaccept([tx['hex']])[0]
438 assert_equal(testres['allowed'], False)
439 assert_equal(testres['reject-reason'], 'txn-already-known')
440- assert_raises_rpc_error(-27, 'Transaction already in block chain', node.sendrawtransaction, tx['hex'])
441+ assert_raises_rpc_error(-27, 'Transaction outputs already in utxo set', node.sendrawtransaction, tx['hex'])
Although it’s only the error message (and the number stays the same), I still think its appropriate to add a release note for the change to the user.
https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#release-notes
Release notes should be written for any PR that: … makes any other visible change to the end-user experience.
Approach ACK Good work increasing clarity. In the past I’ve had a head-scratching moment with some of these before I learned the nuances, so this should help people.
Left a comment about release notes.
Tested:
sendrawtransaction with the newly confirmed transaction.Transaction outputs already in UTXO set as expected.sendall to spent all UTXOs.sendrawtransaction with the the spent transaction.bad-txns-inputs-missingorspent53
54 //! Aliases for backward compatibility
55 RPC_TRANSACTION_ERROR = RPC_VERIFY_ERROR,
56 RPC_TRANSACTION_REJECTED = RPC_VERIFY_REJECTED,
57- RPC_TRANSACTION_ALREADY_IN_CHAIN= RPC_VERIFY_ALREADY_IN_CHAIN,
58+ RPC_TRANSACTION_ALREADY_IN_CHAIN= RPC_VERIFY_ALREADY_IN_UTXO_SET,
RPC_VERIFY_ALREADY_IN_CHAIN from this header. Either this is fine, in which case this whole “backward compat” section can be removed, or it is not fine, in which case the symbol needs to be restored. C.f. d29a2917ff73f7e82b32bd94a87df3ee211a27c2
re: #30212 (review)
Good catch. It is inconsistent to have added backwards compatible aliases previously when renaming and not add them now when renaming again. But it looks like the backwards compatibility section was added in 2014 when it was might have less clear whether aliases were needed. I’d say now aliases are probably not needed and it would be better to remove this section.
77@@ -78,8 +78,8 @@ bool FeeModeFromString(const std::string& mode_string, FeeEstimateMode& fee_esti
78 bilingual_str PSBTErrorString(PSBTError err)
79 {
80 switch (err) {
81- case PSBTError::MISSING_INPUTS:
82- return Untranslated("Inputs missing or spent");
83+ case PSBTError::INPUTS_INVALID:
84+ return Untranslated("Invalid inputs");
703@@ -704,7 +704,7 @@ def test_psbt_input_keys(psbt_input, keys):
704 assert_equal(analysis['next'], 'creator')
705 assert_equal(analysis['error'], 'PSBT is not valid. Input 0 specifies invalid prevout')
706
707- assert_raises_rpc_error(-25, 'Inputs missing or spent', self.nodes[0].walletprocesspsbt, 'cHNidP8BAJoCAAAAAkvEW8NnDtdNtDpsmze+Ht2LH35IJcKv00jKAlUs21RrAwAAAAD/////S8Rbw2cO1020OmybN74e3Ysffkglwq/TSMoCVSzbVGsBAAAAAP7///8CwLYClQAAAAAWABSNJKzjaUb3uOxixsvh1GGE3fW7zQD5ApUAAAAAFgAUKNw0x8HRctAgmvoevm4u1SbN7XIAAAAAAAEAnQIAAAACczMa321tVHuN4GKWKRncycI22aX3uXgwSFUKM2orjRsBAAAAAP7///9zMxrfbW1Ue43gYpYpGdzJwjbZpfe5eDBIVQozaiuNGwAAAAAA/v///wIA+QKVAAAAABl2qRT9zXUVA8Ls5iVqynLHe5/vSe1XyYisQM0ClQAAAAAWABRmWQUcjSjghQ8/uH4Bn/zkakwLtAAAAAAAAQEfQM0ClQAAAAAWABRmWQUcjSjghQ8/uH4Bn/zkakwLtAAAAA==')
708+ assert_raises_rpc_error(-25, 'Invalid inputs', self.nodes[0].walletprocesspsbt, 'cHNidP8BAJoCAAAAAkvEW8NnDtdNtDpsmze+Ht2LH35IJcKv00jKAlUs21RrAwAAAAD/////S8Rbw2cO1020OmybN74e3Ysffkglwq/TSMoCVSzbVGsBAAAAAP7///8CwLYClQAAAAAWABSNJKzjaUb3uOxixsvh1GGE3fW7zQD5ApUAAAAAFgAUKNw0x8HRctAgmvoevm4u1SbN7XIAAAAAAAEAnQIAAAACczMa321tVHuN4GKWKRncycI22aX3uXgwSFUKM2orjRsBAAAAAP7///9zMxrfbW1Ue43gYpYpGdzJwjbZpfe5eDBIVQozaiuNGwAAAAAA/v///wIA+QKVAAAAABl2qRT9zXUVA8Ls5iVqynLHe5/vSe1XyYisQM0ClQAAAAAWABRmWQUcjSjghQ8/uH4Bn/zkakwLtAAAAAAAAQEfQM0ClQAAAAAWABRmWQUcjSjghQ8/uH4Bn/zkakwLtAAAAA==')
In commit “refactor: PSBTError::MISSING_INPUTS” (a05f4ca241045e8a4b295760590805f961c2e5e7)
I wonder if “invalid transaction inputs” might be a clearer message for a user than “invalid inputs.” (I had suggested “invalid inputs” but in this context it seems vague.)
Looking good to me 🥇
I will re-ACK after comments were addressed
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/28352468859
Make sure to run all tests locally, according to the documentation.
The failure may happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
When using `sendrawtransaction` the ALREADY_IN_CHAIN error help string
may be confusing.
Rename TransactionError::ALREADY_IN_CHAIN to
TransactionError::ALREADY_IN_UTXO_SET and update the rpc help string.
Remove backwards compatibility alias as no longer required.
PSBTError::MISSING_INPUTS any more, as we return early. #29855 also modified the only test we had for a MISSING_INPUTS error, and I can’t trivially see a codepath to trigger it any more. Will address this in a followup.42@@ -43,7 +43,7 @@ static RPCHelpMan sendrawtransaction()
43 "\nThe transaction will be sent unconditionally to all peers, so using sendrawtransaction\n"
44 "for manual rebroadcast may degrade privacy by leaking the transaction's origin, as\n"
45 "nodes will normally not rebroadcast non-wallet transactions already in their mempool.\n"
46- "\nA specific exception, RPC_TRANSACTION_ALREADY_IN_CHAIN, may throw if the transaction cannot be added to the mempool.\n"
47+ "\nA specific exception, RPC_TRANSACTION_ALREADY_IN_UTXO_SET, may throw if the transaction cannot be added to the mempool.\n"
In commit “rpc: clarify ALREADY_IN_CHAIN rpc errors” (87b188052528e97729a85d9701864eaff1ea5ec6)
Was already the case before this PR, but this documentation seems misleading because “already in utxo set” error is not the only error that can be triggered if transaction can’t be added to the mempool.
As #19363 reports BroadcastTransaction will start returning MISSING_INPUTS instead of ALREADY_IN_UTXO_SET after the outputs are spent. Also it looks like BroadcastTransaction can returns MEMPOOL_REJECTED and MEMPOOL_ERROR which get translated into RPC_TRANSACTION_REJECTED and RPC_TRANSACTION_ERROR. So 3 different error codes may be returned if the transaction can’t be added to the mempool, not just 1
Code review ACK e9de0a76b99fd4708295e1178f6c079db92e7f36.
Since last review, change has been simplified, no longer renaming PSBTError::MISSING_INPUTS to PSBTError::INPUTS_INVALID and no longer renaming TransactionError::MISSING_INPUTS to TransactionError::INPUTS_MISSING_OR_SPENT
I did think both of those renames were improvements, but it does simplify the PR to drop them. Also it makes it easier to see how this PR fixes #19363.
Also release notes were added
Does bitcoind consider this to be a breaking change? The new error message may be “more accurate”, but for clients that were matching on this error to figure out why a transaction was rejected, this breaks old behavior.
Alternatively, the error constant could be renamed, with the error text/string left in place.