rename TransactionError:ALREADY_IN_CHAIN #30212

willcl-ark · 2024-05-31T14:03:56Z

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)

DrahtBot · 2024-05-31T14:03:59Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

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.

Conflicts

No conflicts as of last run.

willcl-ark · 2024-05-31T14:07:45Z

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.

theStack

Concept and code-review ACK aa422bc

I agree that the proposed names are more to the point and less confusing.

ismaelsadeeq

Code Review ACK aa422bc

ismaelsadeeq · 2024-06-06T21:40:23Z

src/node/transaction.cpp

@@ -22,7 +22,7 @@ static TransactionError HandleATMPError(const TxValidationState& state, std::str
    err_string_out = state.ToString();
    if (state.IsInvalid()) {
        if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) {
-            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:

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.

willcl-ark · 2024-06-14T13:16:14Z

Rebased on master to fix merge conflict.

Added an extra commit to cover PSBTError::MISSING_INPUTS.

ismaelsadeeq

re-ACK b2b4b93

theStack

re-ACK b2b4b93

src/wallet/scriptpubkeyman.cpp

ryanofsky

Code review ACK 77ef75d 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.

src/common/messages.cpp

ryanofsky · 2024-07-01T18:08:16Z

Code review ACK a05f4ca.

Just changed psbt error string and fixed a test since the last review.

tdb3

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:

Ran unit/functional tests locally (passed)
Manually exercised the following scenario (regtest):
- Created a transaction (and broadcasted).
- Generated one block to confirm.
- Attempted to call sendrawtransaction with the newly confirmed transaction.
- Received Transaction outputs already in UTXO set as expected.
- Performed sendall to spent all UTXOs.
- Re-attempted to call sendrawtransaction with the the spent transaction.
- Received bad-txns-inputs-missingorspent

tdb3 · 2024-07-03T02:02:33Z

test/functional/rpc_rawtransaction.py

        self.generate(self.nodes[2], 1)
        for node in self.nodes:
            testres = node.testmempoolaccept([tx['hex']])[0]
            assert_equal(testres['allowed'], False)
            assert_equal(testres['reject-reason'], 'txn-already-known')
-            assert_raises_rpc_error(-27, 'Transaction already in block chain', node.sendrawtransaction, tx['hex'])
+            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.

maflcko

Left a question. Also, I don't think this is a "refactor"

maflcko · 2024-07-03T10:05:49Z

src/node/transaction.cpp

@@ -22,7 +22,7 @@ static TransactionError HandleATMPError(const TxValidationState& state, std::str
    err_string_out = state.ToString();
    if (state.IsInvalid()) {
        if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) {
-            return TransactionError::MISSING_INPUTS;


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.

maflcko · 2024-07-03T10:11:30Z

src/rpc/protocol.h

    RPC_IN_WARMUP                   = -28, //!< Client still warming up
    RPC_METHOD_DEPRECATED           = -32, //!< RPC method is deprecated

    //! Aliases for backward compatibility
    RPC_TRANSACTION_ERROR           = RPC_VERIFY_ERROR,
    RPC_TRANSACTION_REJECTED        = RPC_VERIFY_REJECTED,
-    RPC_TRANSACTION_ALREADY_IN_CHAIN= RPC_VERIFY_ALREADY_IN_CHAIN,
+    RPC_TRANSACTION_ALREADY_IN_CHAIN= RPC_VERIFY_ALREADY_IN_UTXO_SET,


95b09ed: The second commit is not a refactor, but claims to be one. However, it changes the rpc error messages. Also, it is removing the 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. d29a291

re: #30212 (comment)

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.

maflcko · 2024-07-03T10:12:57Z

src/common/messages.cpp

-        case PSBTError::MISSING_INPUTS:
-            return Untranslated("Inputs missing or spent");
+        case PSBTError::INPUTS_INVALID:
+            return Untranslated("Invalid inputs");


a05f4ca: This commit is not a refactor either.

ryanofsky

Agree on not calling the commits which change error messages refactoring. I do like first commit making error constant more consistent with error message, but agree it is not essential.

ryanofsky · 2024-07-03T10:52:21Z

src/rpc/protocol.h

    RPC_IN_WARMUP                   = -28, //!< Client still warming up
    RPC_METHOD_DEPRECATED           = -32, //!< RPC method is deprecated

    //! Aliases for backward compatibility
    RPC_TRANSACTION_ERROR           = RPC_VERIFY_ERROR,
    RPC_TRANSACTION_REJECTED        = RPC_VERIFY_REJECTED,
-    RPC_TRANSACTION_ALREADY_IN_CHAIN= RPC_VERIFY_ALREADY_IN_CHAIN,
+    RPC_TRANSACTION_ALREADY_IN_CHAIN= RPC_VERIFY_ALREADY_IN_UTXO_SET,


re: #30212 (comment)

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.

ryanofsky · 2024-07-03T10:58:21Z

test/functional/rpc_psbt.py

@@ -704,7 +704,7 @@ def test_psbt_input_keys(psbt_input, keys):
        assert_equal(analysis['next'], 'creator')
        assert_equal(analysis['error'], 'PSBT is not valid. Input 0 specifies invalid prevout')

-        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==')
+        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" (a05f4ca)

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

ismaelsadeeq

Looking good to me 🥇

I will re-ACK after comments were addressed

DrahtBot · 2024-08-05T14:18:51Z

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/28352468859}

Hints

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.

tdb3

ACK e9de0a7
Re-ran tests in #30212 (review) (successful)

ismaelsadeeq

ACK e9de0a7

willcl-ark · 2024-08-06T13:19:42Z

Dropped the first commit based on feedback recieved.
Dropped the final commit, as following merge of psbt: Check non witness utxo outpoint early #29855, I don't think that we can actually hit a PSBTError::MISSING_INPUTS any more, as we return early. psbt: Check non witness utxo outpoint 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.

ryanofsky

Code review ACK e9de0a7.

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

ryanofsky · 2024-08-06T14:33:32Z

src/rpc/mempool.cpp

@@ -43,7 +43,7 @@ static RPCHelpMan sendrawtransaction()
        "\nThe transaction will be sent unconditionally to all peers, so using sendrawtransaction\n"
        "for manual rebroadcast may degrade privacy by leaking the transaction's origin, as\n"
        "nodes will normally not rebroadcast non-wallet transactions already in their mempool.\n"
-        "\nA specific exception, RPC_TRANSACTION_ALREADY_IN_CHAIN, may throw if the transaction cannot be added to the mempool.\n"
+        "\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" (87b1880)

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

Roasbeef · 2024-09-12T22:48:19Z

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.

This was referenced Jun 1, 2024

Broadcast own transactions only via short-lived Tor or I2P connections #29415

Open

kernel: Streamline util library #29015

Merged

Implement BIP 370 PSBTv2 #21283

Open

theStack approved these changes Jun 3, 2024

View reviewed changes

ismaelsadeeq approved these changes Jun 6, 2024

View reviewed changes

DrahtBot mentioned this pull request Jun 8, 2024

Remove the legacy wallet and BDB dependency #28710

Draft

DrahtBot added the Needs rebase label Jun 12, 2024

willcl-ark force-pushed the clarify-missing-inputs-err branch from aa422bc to b2b4b93 Compare June 13, 2024 19:32

DrahtBot removed the Needs rebase label Jun 13, 2024

ismaelsadeeq approved these changes Jun 14, 2024

View reviewed changes

DrahtBot requested a review from theStack June 14, 2024 15:45

theStack approved these changes Jun 18, 2024

View reviewed changes

ryanofsky reviewed Jun 18, 2024

View reviewed changes

src/wallet/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

willcl-ark force-pushed the clarify-missing-inputs-err branch from b2b4b93 to 77ef75d Compare June 21, 2024 11:04

DrahtBot added the CI failed label Jun 21, 2024

ryanofsky approved these changes Jun 25, 2024

View reviewed changes

src/common/messages.cpp Outdated Show resolved Hide resolved

DrahtBot requested review from theStack and ismaelsadeeq June 25, 2024 01:27

willcl-ark force-pushed the clarify-missing-inputs-err branch from 77ef75d to a05f4ca Compare June 27, 2024 13:53

DrahtBot removed the CI failed label Jun 27, 2024

DrahtBot mentioned this pull request Jun 28, 2024

psbt: Check non witness utxo outpoint early #29855

Merged

tdb3 reviewed Jul 3, 2024

View reviewed changes

maflcko reviewed Jul 3, 2024

View reviewed changes

ryanofsky approved these changes Jul 3, 2024

View reviewed changes

DrahtBot added the Needs rebase label Jul 8, 2024

ismaelsadeeq reviewed Aug 2, 2024

View reviewed changes

willcl-ark force-pushed the clarify-missing-inputs-err branch from a05f4ca to 76eb6f3 Compare August 5, 2024 13:01

DrahtBot removed the Needs rebase label Aug 5, 2024

DrahtBot added the CI failed label Aug 5, 2024

willcl-ark added 2 commits August 5, 2024 15:45

doc: release note for 30212

e9de0a7

willcl-ark force-pushed the clarify-missing-inputs-err branch from 76eb6f3 to e9de0a7 Compare August 5, 2024 14:46

willcl-ark changed the title ~~rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN~~ rename TransactionError:ALREADY_IN_CHAIN Aug 5, 2024

DrahtBot removed the CI failed label Aug 5, 2024

tdb3 approved these changes Aug 5, 2024

View reviewed changes

DrahtBot requested review from ryanofsky and ismaelsadeeq August 5, 2024 22:43

ismaelsadeeq reviewed Aug 6, 2024

View reviewed changes

ryanofsky mentioned this pull request Aug 6, 2024

TransactionError::ALREADY_IN_CHAIN inconsistency #19363

Closed

ryanofsky approved these changes Aug 6, 2024

View reviewed changes

ryanofsky merged commit 870447f into bitcoin:master Aug 6, 2024
16 checks passed

guggero mentioned this pull request Sep 3, 2024

chain: match on new bitcoind v28.0 errors btcsuite/btcwallet#948

Merged

tdb3 mentioned this pull request Sep 14, 2024

doc: add guidance for RPC to developer notes #30142

Open

This was referenced Oct 31, 2024

Policy: Report reason inputs are non standard #29060

Open

test: cover edge case for lockunspent rpc #31209

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rename TransactionError:ALREADY_IN_CHAIN #30212

rename TransactionError:ALREADY_IN_CHAIN #30212

willcl-ark commented May 31, 2024 •

edited

Loading

DrahtBot commented May 31, 2024 •

edited

Loading

willcl-ark commented May 31, 2024

theStack left a comment

ismaelsadeeq left a comment

ismaelsadeeq Jun 6, 2024 •

edited

Loading

willcl-ark Jun 12, 2024

ismaelsadeeq Jun 13, 2024

maflcko Jul 3, 2024 •

edited

Loading

willcl-ark commented Jun 14, 2024

ismaelsadeeq left a comment

theStack left a comment

ryanofsky left a comment •

edited

Loading

ryanofsky commented Jul 1, 2024

tdb3 left a comment

tdb3 Jul 3, 2024 •

edited

Loading

maflcko left a comment

maflcko Jul 3, 2024 •

edited

Loading

maflcko Jul 3, 2024

ryanofsky Jul 3, 2024

maflcko Jul 3, 2024

ryanofsky left a comment

ryanofsky Jul 3, 2024

ryanofsky Jul 3, 2024

ismaelsadeeq left a comment

DrahtBot commented Aug 5, 2024

tdb3 left a comment

ismaelsadeeq left a comment

willcl-ark commented Aug 6, 2024

ryanofsky left a comment

ryanofsky Aug 6, 2024

Roasbeef commented Sep 12, 2024

rename TransactionError:ALREADY_IN_CHAIN #30212

rename TransactionError:ALREADY_IN_CHAIN #30212

Conversation

willcl-ark commented May 31, 2024 • edited Loading

DrahtBot commented May 31, 2024 • edited Loading

Code Coverage

Reviews

Conflicts

willcl-ark commented May 31, 2024

theStack left a comment

Choose a reason for hiding this comment

ismaelsadeeq left a comment

Choose a reason for hiding this comment

ismaelsadeeq Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

willcl-ark commented Jun 14, 2024

ismaelsadeeq left a comment

Choose a reason for hiding this comment

theStack left a comment

Choose a reason for hiding this comment

ryanofsky left a comment • edited Loading

Choose a reason for hiding this comment

ryanofsky commented Jul 1, 2024

tdb3 left a comment

Choose a reason for hiding this comment

tdb3 Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

maflcko left a comment

Choose a reason for hiding this comment

maflcko Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ismaelsadeeq left a comment

Choose a reason for hiding this comment

DrahtBot commented Aug 5, 2024

tdb3 left a comment

Choose a reason for hiding this comment

ismaelsadeeq left a comment

Choose a reason for hiding this comment

willcl-ark commented Aug 6, 2024

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef commented Sep 12, 2024

willcl-ark commented May 31, 2024 •

edited

Loading

DrahtBot commented May 31, 2024 •

edited

Loading

ismaelsadeeq Jun 6, 2024 •

edited

Loading

maflcko Jul 3, 2024 •

edited

Loading

ryanofsky left a comment •

edited

Loading

tdb3 Jul 3, 2024 •

edited

Loading

maflcko Jul 3, 2024 •

edited

Loading