bitcoin-tx: reject in and outscript arguments with too many separators #36368

Bruce039 commented at 2:44 PM on September 28, 2026: none

outscript=VALUE:SCRIPT[:FLAGS] only reads the flags when there are exactly three fields (bitcoin-tx.cpp:482). With a trailing or extra : the flags are silently dropped:

$ bitcoin-tx -create outscript=0:OP_DROP:S nversion=1
010000000001000000000000000017a91471ed53322d470bb96657deb786b94f97dd46fb158700000000  # P2SH, as requested
$ bitcoin-tx -create outscript=0:OP_DROP:S: nversion=1
0100000000010000000000000000017500000000  # bare OP_DROP output, exit code 0

So a typo gives a transaction paying to the raw script instead of the P2SH/P2WSH wrapper, with no error. outpubkey and outaddr already reject extra fields with "TX output missing or too many separators".

This rejects outscript arguments with more than three fields, and in=TXID:VOUT[:SEQUENCE_NUMBER] arguments with more than three fields (which were accepted the same way), both with a general "Invalid format, expected ..." error. bitcoin-util-test.json gets a case for each; they fail on master (exit code 0) and pass with the fix, and tool_utils.py passes.

AI tools were used to help find this issue and to prepare the patch and test.

DrahtBot commented at 2:44 PM on September 28, 2026: contributor

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

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36368
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36368.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36368#author-ralph

Reviews

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

Type	Reviewers
ACK	maflcko, cyb3ralbert

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

in src/bitcoin-tx.cpp:472 in ee6d4d3fe3

 467 | @@ -468,6 +468,8 @@ static void MutateTxAddOutScript(CMutableTransaction& tx, const std::string& str
 468 |      std::vector<std::string> vStrInputParts = SplitString(strInput, ':');
 469 |      if (vStrInputParts.size() < 2)
 470 |          throw std::runtime_error("TX output missing separator");
 471 | +    if (vStrInputParts.size() > 3)
 472 | +        throw std::runtime_error("TX output missing or too many separators");

maflcko commented at 3:15 PM on September 28, 2026:

Why would the TX output be missing? Also, new code should use {} for the if body

Bruce039 force-pushed on Sep 28, 2026

Bruce039 commented at 3:57 PM on September 28, 2026: none

Thanks, fixed both: the error for extra fields is now "TX output has too many separators" instead of the copied "missing or too many" message, and the if body uses braces. The util test is updated to match. (Changes prepared with AI tools.)

in src/bitcoin-tx.cpp:472 in 7d5af256b0

 467 | @@ -468,6 +468,9 @@ static void MutateTxAddOutScript(CMutableTransaction& tx, const std::string& str
 468 |      std::vector<std::string> vStrInputParts = SplitString(strInput, ':');
 469 |      if (vStrInputParts.size() < 2)
 470 |          throw std::runtime_error("TX output missing separator");
 471 | +    if (vStrInputParts.size() > 3) {
 472 | +        throw std::runtime_error("TX output has too many separators");

maflcko commented at 8:28 AM on September 29, 2026:

not sure how much hand-holding a user needs here. Seems fine to just give an exact and general error message.

Also, the other cases (in) should be covered as well:

diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp
index 017e8e1..82812f5 100644
--- a/src/bitcoin-tx.cpp
+++ b/src/bitcoin-tx.cpp
@@ -58,3 +58,3 @@ static void SetupBitcoinTxArgs(ArgsManager &argsman)
     argsman.AddArg("delout=N", "Delete output N from TX", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS);
-    argsman.AddArg("in=TXID:VOUT(:SEQUENCE_NUMBER)", "Add input to TX", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS);
+    argsman.AddArg("in=TXID:VOUT[:SEQUENCE_NUMBER]", "Add input to TX", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS);
     argsman.AddArg("locktime=N", "Set TX lock time to N", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS);
@@ -263,5 +263,5 @@ static void MutateTxAddInput(CMutableTransaction& tx, const std::string& strInpu
 
-    // separate TXID:VOUT in string
-    if (vStrInputParts.size()<2)
-        throw std::runtime_error("TX input missing separator");
+    if (vStrInputParts.size() < 2 || vStrInputParts.size() > 3) {
+        throw std::runtime_error("Invalid format, expected in=TXID:VOUT[:SEQUENCE_NUMBER]");
+    }
 
@@ -466,6 +466,6 @@ static void MutateTxAddOutScript(CMutableTransaction& tx, const std::string& str
 {
-    // separate VALUE:SCRIPT[:FLAGS]
     std::vector<std::string> vStrInputParts = SplitString(strInput, ':');
-    if (vStrInputParts.size() < 2)
-        throw std::runtime_error("TX output missing separator");
+    if (vStrInputParts.size() < 2 || vStrInputParts.size() > 3) {
+        throw std::runtime_error("Invalid format, expected outscript=VALUE:SCRIPT[:FLAGS]");
+    }

Bruce039 commented at 1:43 PM on September 29, 2026:

Done in fea4a9b832: both now fail with the general "Invalid format, expected ..." message, in= rejects extra fields too, and the util test has a case for each. (Changes prepared with AI tools.)

maflcko commented at 1:52 PM on September 29, 2026:

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

maflcko approved

maflcko commented at 8:29 AM on September 29, 2026: member

lgtm. Not sure how many users are using this tool, but seems fine to harden this

Bruce039 renamed this:
~~bitcoin-tx: reject outscript with too many separators~~
bitcoin-tx: reject in and outscript arguments with too many separators
on Sep 29, 2026

bitcoin-tx: reject in and outscript arguments with too many separators

outscript=VALUE:SCRIPT[:FLAGS] and in=TXID:VOUT[:SEQUENCE_NUMBER] only
checked for too few fields and silently ignored extra ones, so a typo like
outscript=0:OP_DROP:S: dropped the P2SH flag without an error. Reject both
with a general "Invalid format" error.

2fef1db4f8

Bruce039 force-pushed on Sep 29, 2026

Bruce039 commented at 2:18 PM on September 29, 2026: none

Squashed into one commit (2fef1db4f8), no code change. (Changes prepared with AI tools.)

maflcko commented at 3:14 PM on September 29, 2026: member

lgtm ACK 2fef1db4f88a02efd666da41bc3641345940669d

cyb3ralbert commented at 3:13 PM on October 8, 2026: contributor

tested ACK 2fef1db4f88a02efd666da41bc3641345940669d

Checked by running bitcoin-tx before and after the change: with an extra trailing : (outscript=0:OP_DROP:S:) the S flag was silently lost — a bare OP_DROP script was created with exit code 0. With this PR the same argument fails with Invalid format, expected outscript=VALUE:SCRIPT[:FLAGS].

Same for in=: in=<TXID>:0:1:1 was accepted with exit code 0 and the fourth field ignored; now it fails with Invalid format, expected in=TXID:VOUT[:SEQUENCE_NUMBER].

tool_utils.py with the new test cases fails on master and passes on the PR head and on the head merged into master. Built and tested on Ubuntu 24.04 (x86_64), gcc 13.3.0, Release.

DrahtBot added the label CI failed on Oct 9, 2026

DrahtBot removed the label CI failed on Oct 9, 2026