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 aa422bcb898b7e3aa8dca912d92016bd22cf243d

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

ismaelsadeeq

Code Review ACK aa422bcb898b7e3aa8dca912d92016bd22cf243d

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

src/node/transaction.cpp

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 b2b4b932572c5bb1ffa3fc4f34e17130348fbc24

theStack

re-ACK b2b4b932572c5bb1ffa3fc4f34e17130348fbc24

src/wallet/scriptpubkeyman.cpp

ryanofsky

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.

src/common/messages.cpp

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

Code review ACK a05f4ca241045e8a4b295760590805f961c2e5e7.

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

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

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

95b09ed4741b9a15d7d285e248161d5a508f5586: 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

a05f4ca241045e8a4b295760590805f961c2e5e7: 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

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

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

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

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

c6eca6f doc: add guidance for RPC to developer notes (tdb3) Pull request description: Adds guidance statements to the RPC interface section of the developer notes with examples of when to implement `-deprecatedrpc=`. Wanted to increase awareness of preferred RPC implementation approaches for newer contributors. This implements some of what's discussed in #29912 (comment) Opinions may differ, so please don't be shy. We want to make RPC as solid/safe as possible. Examples of discussions where guidelines/context might have added value: #30212 (comment) #29845 (comment) #30381 (review) #29954 (comment) #30410 (review) #30713 #30381 #29060 (review) ACKs for top commit: l0rinc: ACK c6eca6f fjahr: ACK c6eca6f maflcko: lgtm ACK c6eca6f jonatack: ACK c6eca6f Tree-SHA512: 01a98a8dc0eb91762b225d3278cdb4a5e380ceb7486fd096b4ad9122bed859cea8584d8996d3dce51272fdb792f4a793a1bd1c5445efeb87f0a30f9b6e59a790

This was referenced Jun 1, 2024

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

Merged

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

Merged

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 ismaelsadeeq and theStack 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 ismaelsadeeq and ryanofsky 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

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

Merged

This was referenced Oct 31, 2024

Policy: Report debug message why inputs are non standard #29060

Open

test: cover edge case for lockunspent rpc #31209

Closed

xyephy mentioned this pull request Feb 26, 2025

Upgrade to bitcoin v28 whiveio/whive#12

Open

willcl-ark mentioned this pull request Aug 14, 2025

validation: rename block script verification error from "mandatory" to "block" #33183

Merged

bitcoin locked and limited conversation to collaborators Sep 12, 2025

rename TransactionError:ALREADY_IN_CHAIN #30212

rename TransactionError:ALREADY_IN_CHAIN #30212

Uh oh!

Conversation

willcl-ark commented May 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

willcl-ark commented May 31, 2024

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq left a comment

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willcl-ark commented Jun 14, 2024

Uh oh!

ismaelsadeeq left a comment

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky commented Jul 1, 2024

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

tdb3 Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Aug 5, 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