rpc: clarify sendrawtransaction decode error #36250

The existing sendrawtransaction decode error suggests that the transaction may have no inputs. The same error is returned for every DecodeHexTx() failure, including invalid hex and incomplete or malformed serialization.

Mention encoding and serialization requirements while retaining the existing input hint. Transaction processing and error codes are unchanged.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36250.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline and AI policy for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Thanks for working on this. The current message still places too much emphasis on the transaction having an input.

DecodeHexTx() does not explicitly require a nonempty input vector. An empty-input transaction that successfully decodes is rejected later by CheckTransaction() as bad-txns-vin-empty. I think the decode error should describe the general serialization failure without retaining the input hint.

Could you also add focused coverage to RawTransactionsTest.sendrawtransaction_tests() in test/functional/rpc_rawtransaction.py for representative invalid encodings and incomplete serializations? For example:

@@
     def sendrawtransaction_tests(self):
+        self.log.info("Test sendrawtransaction with malformed transaction data")
+        decode_error = (
+            "TX decode failed. Make sure the transaction is complete and correctly serialized "
+            "as hexadecimal data."
+        )
+        assert_raises_rpc_error(-22, decode_error, self.nodes[2].sendrawtransaction, "not-hex")
+
         self.log.info("Test sendrawtransaction with missing input")
         inputs = [{'txid': TXID, 'vout': 1}]  # won't exist
         address = getnewdestination()[2]
         outputs = {address: 4.998}
         rawtx = self.nodes[2].createrawtransaction(inputs, outputs)
+
+        assert_raises_rpc_error(-22, decode_error, self.nodes[2].sendrawtransaction, rawtx[:-2])
+        assert_raises_rpc_error(-22, decode_error, self.nodes[2].sendrawtransaction, rawtx + "00")
         assert_raises_rpc_error(-25, "bad-txns-inputs-missingorspent", self.nodes[2].sendrawtransaction, rawtx)

This leaves the existing missing-input assertion and log message in place while adding direct coverage for invalid hex, truncation, and trailing bytes.

ACK 663d1fe2

Built locally and ran build-pr36250/test/functional/test_runner.py rpc_rawtransaction.py rpc_echo_payload.py. Both tests passed.

Edit: Also ran test_bitcoin, all clear.

Commits reworked to satisfy the "test ancestor commits" CI job — no change to the overall diff.

Previously the first commit changed the error message in src/rpc/mempool.cpp while the matching rpc_echo_payload.py expectation update lived in the second commit, so the job failed when testing the first commit on its own (AssertionError: Unexpected msg: TX decode failed. ...).

The split is now:

1. c8dd876ec — message change + the rpc_echo_payload.py expectation update it requires (self-contained, passes on its own)
2. fec99e442 — the new malformed-payload assertions in rpc_rawtransaction.py only

Concept ACK The same error covers invalid hex, incomplete serialization, and trailing bytes, so broadening the message makes sense. The new tests cover these distinct failures while preserving the existing missing-input validation check.

The input hint was deliberately added in #19836 (https://github.com/bitcoin/bitcoin/pull/19836#issuecomment-685872256) to explain decoding failures involving zero-input transactions. I’d retain that troubleshooting guidance alongside the general requirements, with serialization and hex encoding described separately.

The PR description already says the input hint is retained, which would match that suggestion. “Transaction processing and error codes are unchanged” would be more precise than “RPC behavior and error codes are unchanged.” Or just remove it. LLMs are eager to add these but I don't think they're very useful.

ACK 94a616d7000371a147112b4364db49372c74bad4

To simplify it for the other reviewers, please go through the review comments that are already addressed and resolved them manually.

re-ACK 94a616d

Built and tested each commit locally. All unit tests passed, and applicable functional tests passed.

The input hint was deliberately added in #19836 (#19836 (comment)) to explain decoding failures involving zero-input transactions.

Yeah, this was done intentionally, when the heuristic fails. So there are two error cases handled in the same error message:

• TX decode failed.
• Someone provided a "valid" zero-input tx, which also failed witness deser.

I understand the error message can be a bit confusing, but I don't understand this pull request. What is so special about sendrawtransaction?

Nothing special about sendrawtransaction itself. The problem is the message is attached to every DecodeHexTx() failure, not only the zero-input / witness-heuristic case from #19836.

DecodeHexTx() returns false for invalid hex, truncated hex, extra trailing bytes, and the ambiguous empty-vin / witness-marker case. All of those currently say “Make sure the tx has at least one input.” A truncated or non-hex payload does not have an input problem; bad-txns-vin-empty is the later CheckTransaction() path when an empty-vin tx actually deserializes.

The #19836 hint is still there. This only names the other DecodeHexTx() failure modes so the same string is not a false lead for the common cases.

Happy to drop the input clause again if reviewers prefer a general decode message and leave the empty-vin case to CheckTransaction().

Well, invalid hex or truncated hex is not a valid tx, so the tx doesn't have at least one input, so the current error msg is perfectly fine :trollface:

In any case, either this doesn't matter, in which case this special-casing here should be closed, or it does matter, in which case it should be fixed in all places.

That's the plan — #36263 applies the same message to the remaining DecodeHexTx() error paths (testmempoolaccept, submitpackage, generateblock, combinerawtransaction, signrawtransactionwithkey, signrawtransactionwithwallet, importprunedfunds). It's stacked on this PR and marked draft until this one merges, per l0rinc's request. This PR is the first, smallest step so the wording gets agreed once before it's replicated.

Please do not use LLM generated comments, per https://github.com/bitcoin/bitcoin/blob/master/doc/AI_POLICY.md

In any case, I'll close this for now. A single pull request for this should be enough.

If you are worried about duplication, the string can be extracted into a single place (e.g. inline constexpr string_view in a header).