Skip to content

Commit 8a4bab8

Browse files
committed
Merge bitcoin#35863: test: fix wrong transaction in GetP2SHSigOpCount assertion
756afe1 test: give each ValidateInputsStandardness case its own scope (JP) 5559fa4 test: fix wrong transaction in GetP2SHSigOpCount assertion (JP) Pull request description: While reading through `script_p2sh_tests.cpp` I noticed one of the assertions in `ValidateInputsStandardness` checks the wrong transaction. The test builds `txToNonStd2_no_scriptSig` (which spends a P2SH prevout with an empty scriptSig) and checks its standardness result ("input 0 P2SH redeemscript missing"), but the `GetP2SHSigOpCount` assertion right after it re-checks the previous transaction: line 433 is byte-identical to line 419. Looks like a copy-paste slip from 248c175, which added a `GetP2SHSigOpCount` check after each constructed transaction. This PR points the assertion at `txToNonStd2_no_scriptSig` and expects 0 sigops. With an empty scriptSig there's no redeemScript push, so `GetSigOpCount(scriptSig)` ends up counting an empty subscript and returns 0. This case wasn't asserted anywhere before. The line above covers the other side, where the same prevout spent with the actual redeemScript counts 20. To make sure the fix isn't vacuous I also ran the assertion expecting 20, and it fails with `[0 != 20]`. Tested with: ``` cmake --build build --target test_bitcoin build/bin/test_bitcoin --run_test=script_p2sh_tests ``` ACKs for top commit: l0rinc: ACK 756afe1 sedited: ACK 756afe1 Tree-SHA512: 463eda7bb8790fb55619b36a6bedcd437f5c3d753e8c0abaa57dde3154421cde8c36f04293eb712ddf2745d525b5e254e8a1ab898f4f68538520dd80d873dba3
2 parents 975a314 + 756afe1 commit 8a4bab8

1 file changed

Lines changed: 124 additions & 108 deletions

File tree

src/test/script_p2sh_tests.cpp

Lines changed: 124 additions & 108 deletions
Original file line numberDiff line numberDiff line change
@@ -354,125 +354,141 @@ BOOST_AUTO_TEST_CASE(ValidateInputsStandardness)
354354

355355
AddCoins(coins, CTransaction(txFrom), 0);
356356

357-
CMutableTransaction txTo;
358-
txTo.vout.resize(1);
359-
txTo.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key[1].GetPubKey()));
357+
{
358+
CMutableTransaction txTo;
359+
txTo.vout.resize(1);
360+
txTo.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key[1].GetPubKey()));
361+
362+
txTo.vin.resize(5);
363+
for (int i = 0; i < 5; i++)
364+
{
365+
txTo.vin[i].prevout.n = i;
366+
txTo.vin[i].prevout.hash = txFrom.GetHash();
367+
}
368+
SignatureData empty;
369+
BOOST_CHECK(SignSignature(keystore, CTransaction(txFrom), txTo, 0, SIGHASH_ALL, empty));
370+
SignatureData empty_b;
371+
BOOST_CHECK(SignSignature(keystore, CTransaction(txFrom), txTo, 1, SIGHASH_ALL, empty_b));
372+
SignatureData empty_c;
373+
BOOST_CHECK(SignSignature(keystore, CTransaction(txFrom), txTo, 2, SIGHASH_ALL, empty_c));
374+
// SignSignature doesn't know how to sign these. We're
375+
// not testing validating signatures, so just create
376+
// dummy signatures that DO include the correct P2SH scripts:
377+
txTo.vin[3].scriptSig << OP_11 << OP_11 << std::vector<unsigned char>(oneAndTwo.begin(), oneAndTwo.end());
378+
txTo.vin[4].scriptSig << std::vector<unsigned char>(fifteenSigops.begin(), fifteenSigops.end());
379+
380+
BOOST_CHECK(::ValidateInputsStandardness(CTransaction(txTo), coins).IsValid());
381+
// 22 P2SH sigops for all inputs (1 for vin[0], 6 for vin[3], 15 for vin[4]
382+
BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(txTo), coins), 22U);
383+
}
360384

361-
txTo.vin.resize(5);
362-
for (int i = 0; i < 5; i++)
363385
{
364-
txTo.vin[i].prevout.n = i;
365-
txTo.vin[i].prevout.hash = txFrom.GetHash();
386+
CMutableTransaction coinbase_tx_mut;
387+
coinbase_tx_mut.vin.resize(1);
388+
CTransaction coinbase_tx{coinbase_tx_mut};
389+
BOOST_CHECK(coinbase_tx.IsCoinBase());
390+
BOOST_CHECK_EQUAL(GetP2SHSigOpCount(coinbase_tx, coins), 0U);
366391
}
367-
SignatureData empty;
368-
BOOST_CHECK(SignSignature(keystore, CTransaction(txFrom), txTo, 0, SIGHASH_ALL, empty));
369-
SignatureData empty_b;
370-
BOOST_CHECK(SignSignature(keystore, CTransaction(txFrom), txTo, 1, SIGHASH_ALL, empty_b));
371-
SignatureData empty_c;
372-
BOOST_CHECK(SignSignature(keystore, CTransaction(txFrom), txTo, 2, SIGHASH_ALL, empty_c));
373-
// SignSignature doesn't know how to sign these. We're
374-
// not testing validating signatures, so just create
375-
// dummy signatures that DO include the correct P2SH scripts:
376-
txTo.vin[3].scriptSig << OP_11 << OP_11 << std::vector<unsigned char>(oneAndTwo.begin(), oneAndTwo.end());
377-
txTo.vin[4].scriptSig << std::vector<unsigned char>(fifteenSigops.begin(), fifteenSigops.end());
378-
379-
BOOST_CHECK(::ValidateInputsStandardness(CTransaction(txTo), coins).IsValid());
380-
// 22 P2SH sigops for all inputs (1 for vin[0], 6 for vin[3], 15 for vin[4]
381-
BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(txTo), coins), 22U);
382-
383-
CMutableTransaction coinbase_tx_mut;
384-
coinbase_tx_mut.vin.resize(1);
385-
CTransaction coinbase_tx{coinbase_tx_mut};
386-
BOOST_CHECK(coinbase_tx.IsCoinBase());
387-
BOOST_CHECK_EQUAL(GetP2SHSigOpCount(coinbase_tx, coins), 0U);
388392

389393
// TxoutType::SCRIPTHASH
390-
CMutableTransaction txToNonStd1;
391-
txToNonStd1.vout.resize(1);
392-
txToNonStd1.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key[1].GetPubKey()));
393-
txToNonStd1.vout[0].nValue = 1000;
394-
txToNonStd1.vin.resize(1);
395-
txToNonStd1.vin[0].prevout.n = 5;
396-
txToNonStd1.vin[0].prevout.hash = txFrom.GetHash();
397-
txToNonStd1.vin[0].scriptSig << std::vector<unsigned char>(sixteenSigops.begin(), sixteenSigops.end());
398-
399-
const auto txToNonStd1_res = ::ValidateInputsStandardness(CTransaction(txToNonStd1), coins);
400-
BOOST_CHECK(txToNonStd1_res.IsInvalid());
401-
BOOST_CHECK_EQUAL(txToNonStd1_res.GetRejectReason(), "bad-txns-nonstandard-inputs");
402-
BOOST_CHECK_EQUAL(txToNonStd1_res.GetDebugMessage(), "p2sh redeemscript sigops exceed limit (input 0: 16 > 15)");
403-
404-
BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(txToNonStd1), coins), 16U);
405-
406-
CMutableTransaction txToNonStd2;
407-
txToNonStd2.vout.resize(1);
408-
txToNonStd2.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key[1].GetPubKey()));
409-
txToNonStd2.vout[0].nValue = 1000;
410-
txToNonStd2.vin.resize(1);
411-
txToNonStd2.vin[0].prevout.n = 6;
412-
txToNonStd2.vin[0].prevout.hash = txFrom.GetHash();
413-
txToNonStd2.vin[0].scriptSig << std::vector<unsigned char>(twentySigops.begin(), twentySigops.end());
414-
415-
const auto txToNonStd2_res = ::ValidateInputsStandardness(CTransaction(txToNonStd2), coins);
416-
BOOST_CHECK(txToNonStd2_res.IsInvalid());
417-
BOOST_CHECK_EQUAL(txToNonStd2_res.GetRejectReason(), "bad-txns-nonstandard-inputs");
418-
BOOST_CHECK_EQUAL(txToNonStd2_res.GetDebugMessage(), "p2sh redeemscript sigops exceed limit (input 0: 20 > 15)");
419-
BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(txToNonStd2), coins), 20U);
420-
421-
CMutableTransaction txToNonStd2_no_scriptSig;
422-
txToNonStd2_no_scriptSig.vout.resize(1);
423-
txToNonStd2_no_scriptSig.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key[1].GetPubKey()));
424-
txToNonStd2_no_scriptSig.vout[0].nValue = 1000;
425-
txToNonStd2_no_scriptSig.vin.resize(1);
426-
txToNonStd2_no_scriptSig.vin[0].prevout.n = 6;
427-
txToNonStd2_no_scriptSig.vin[0].prevout.hash = txFrom.GetHash();
428-
429-
const auto txToNonStd2_no_scriptSig_res = ::ValidateInputsStandardness(CTransaction(txToNonStd2_no_scriptSig), coins);
430-
BOOST_CHECK(txToNonStd2_no_scriptSig_res.IsInvalid());
431-
BOOST_CHECK_EQUAL(txToNonStd2_no_scriptSig_res.GetRejectReason(), "bad-txns-nonstandard-inputs");
432-
BOOST_CHECK_EQUAL(txToNonStd2_no_scriptSig_res.GetDebugMessage(), "input 0 P2SH redeemscript missing");
433-
BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(txToNonStd2), coins), 20U);
394+
{
395+
CMutableTransaction txToNonStd1;
396+
txToNonStd1.vout.resize(1);
397+
txToNonStd1.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key[1].GetPubKey()));
398+
txToNonStd1.vout[0].nValue = 1000;
399+
txToNonStd1.vin.resize(1);
400+
txToNonStd1.vin[0].prevout.n = 5;
401+
txToNonStd1.vin[0].prevout.hash = txFrom.GetHash();
402+
txToNonStd1.vin[0].scriptSig << std::vector<unsigned char>(sixteenSigops.begin(), sixteenSigops.end());
403+
404+
const auto txToNonStd1_res = ::ValidateInputsStandardness(CTransaction(txToNonStd1), coins);
405+
BOOST_CHECK(txToNonStd1_res.IsInvalid());
406+
BOOST_CHECK_EQUAL(txToNonStd1_res.GetRejectReason(), "bad-txns-nonstandard-inputs");
407+
BOOST_CHECK_EQUAL(txToNonStd1_res.GetDebugMessage(), "p2sh redeemscript sigops exceed limit (input 0: 16 > 15)");
408+
409+
BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(txToNonStd1), coins), 16U);
410+
}
411+
412+
{
413+
CMutableTransaction txToNonStd2;
414+
txToNonStd2.vout.resize(1);
415+
txToNonStd2.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key[1].GetPubKey()));
416+
txToNonStd2.vout[0].nValue = 1000;
417+
txToNonStd2.vin.resize(1);
418+
txToNonStd2.vin[0].prevout.n = 6;
419+
txToNonStd2.vin[0].prevout.hash = txFrom.GetHash();
420+
txToNonStd2.vin[0].scriptSig << std::vector<unsigned char>(twentySigops.begin(), twentySigops.end());
421+
422+
const auto txToNonStd2_res = ::ValidateInputsStandardness(CTransaction(txToNonStd2), coins);
423+
BOOST_CHECK(txToNonStd2_res.IsInvalid());
424+
BOOST_CHECK_EQUAL(txToNonStd2_res.GetRejectReason(), "bad-txns-nonstandard-inputs");
425+
BOOST_CHECK_EQUAL(txToNonStd2_res.GetDebugMessage(), "p2sh redeemscript sigops exceed limit (input 0: 20 > 15)");
426+
BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(txToNonStd2), coins), 20U);
427+
}
428+
429+
{
430+
CMutableTransaction txToNonStd2_no_scriptSig;
431+
txToNonStd2_no_scriptSig.vout.resize(1);
432+
txToNonStd2_no_scriptSig.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key[1].GetPubKey()));
433+
txToNonStd2_no_scriptSig.vout[0].nValue = 1000;
434+
txToNonStd2_no_scriptSig.vin.resize(1);
435+
txToNonStd2_no_scriptSig.vin[0].prevout.n = 6;
436+
txToNonStd2_no_scriptSig.vin[0].prevout.hash = txFrom.GetHash();
437+
438+
const auto txToNonStd2_no_scriptSig_res = ::ValidateInputsStandardness(CTransaction(txToNonStd2_no_scriptSig), coins);
439+
BOOST_CHECK(txToNonStd2_no_scriptSig_res.IsInvalid());
440+
BOOST_CHECK_EQUAL(txToNonStd2_no_scriptSig_res.GetRejectReason(), "bad-txns-nonstandard-inputs");
441+
BOOST_CHECK_EQUAL(txToNonStd2_no_scriptSig_res.GetDebugMessage(), "input 0 P2SH redeemscript missing");
442+
BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(txToNonStd2_no_scriptSig), coins), 0U);
443+
}
434444

435445
// TxoutType::NONSTANDARD
436-
CMutableTransaction txToNonStd3;
437-
txToNonStd3.vout.resize(1);
438-
txToNonStd3.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key[1].GetPubKey()));
439-
txToNonStd3.vout[0].nValue = 1000;
440-
txToNonStd3.vin.resize(1);
441-
txToNonStd3.vin[0].prevout.n = 7;
442-
txToNonStd3.vin[0].prevout.hash = txFrom.GetHash();
443-
444-
const auto txToNonStd3_res = ::ValidateInputsStandardness(CTransaction(txToNonStd3), coins);
445-
BOOST_CHECK(txToNonStd3_res.IsInvalid());
446-
BOOST_CHECK_EQUAL(txToNonStd3_res.GetRejectReason(), "bad-txns-nonstandard-inputs");
447-
BOOST_CHECK_EQUAL(txToNonStd3_res.GetDebugMessage(), "input 0 script unknown");
446+
{
447+
CMutableTransaction txToNonStd3;
448+
txToNonStd3.vout.resize(1);
449+
txToNonStd3.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key[1].GetPubKey()));
450+
txToNonStd3.vout[0].nValue = 1000;
451+
txToNonStd3.vin.resize(1);
452+
txToNonStd3.vin[0].prevout.n = 7;
453+
txToNonStd3.vin[0].prevout.hash = txFrom.GetHash();
454+
455+
const auto txToNonStd3_res = ::ValidateInputsStandardness(CTransaction(txToNonStd3), coins);
456+
BOOST_CHECK(txToNonStd3_res.IsInvalid());
457+
BOOST_CHECK_EQUAL(txToNonStd3_res.GetRejectReason(), "bad-txns-nonstandard-inputs");
458+
BOOST_CHECK_EQUAL(txToNonStd3_res.GetDebugMessage(), "input 0 script unknown");
459+
}
448460

449461
// TxoutType::INCORRECT_SCRIPTSIG
450-
CMutableTransaction txToNonStd4;
451-
txToNonStd4.vout.resize(1);
452-
txToNonStd4.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key[1].GetPubKey()));
453-
txToNonStd4.vout[0].nValue = 1000;
454-
txToNonStd4.vin.resize(1);
455-
txToNonStd4.vin[0].prevout.n = 8;
456-
txToNonStd4.vin[0].prevout.hash = txFrom.GetHash();
457-
txToNonStd4.vin[0].scriptSig = op_return_script;
458-
459-
const auto txToNonStd4_res = ::ValidateInputsStandardness(CTransaction(txToNonStd4), coins);
460-
BOOST_CHECK(txToNonStd4_res.IsInvalid());
461-
BOOST_CHECK_EQUAL(txToNonStd4_res.GetRejectReason(), "bad-txns-nonstandard-inputs");
462-
BOOST_CHECK_EQUAL(txToNonStd4_res.GetDebugMessage(), "p2sh scriptsig malformed (input 0: OP_RETURN was encountered)");
462+
{
463+
CMutableTransaction txToNonStd4;
464+
txToNonStd4.vout.resize(1);
465+
txToNonStd4.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key[1].GetPubKey()));
466+
txToNonStd4.vout[0].nValue = 1000;
467+
txToNonStd4.vin.resize(1);
468+
txToNonStd4.vin[0].prevout.n = 8;
469+
txToNonStd4.vin[0].prevout.hash = txFrom.GetHash();
470+
txToNonStd4.vin[0].scriptSig = op_return_script;
471+
472+
const auto txToNonStd4_res = ::ValidateInputsStandardness(CTransaction(txToNonStd4), coins);
473+
BOOST_CHECK(txToNonStd4_res.IsInvalid());
474+
BOOST_CHECK_EQUAL(txToNonStd4_res.GetRejectReason(), "bad-txns-nonstandard-inputs");
475+
BOOST_CHECK_EQUAL(txToNonStd4_res.GetDebugMessage(), "p2sh scriptsig malformed (input 0: OP_RETURN was encountered)");
476+
}
463477

464478
// TxoutType::WITNESS_UNKNOWN
465-
CMutableTransaction txWitnessUnknown;
466-
txWitnessUnknown.vout.resize(1);
467-
txWitnessUnknown.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key[1].GetPubKey()));
468-
txWitnessUnknown.vout[0].nValue = 1000;
469-
txWitnessUnknown.vin.resize(1);
470-
txWitnessUnknown.vin[0].prevout.n = 9;
471-
txWitnessUnknown.vin[0].prevout.hash = txFrom.GetHash();
472-
const auto txWitnessUnknown_res = ::ValidateInputsStandardness(CTransaction(txWitnessUnknown), coins);
473-
BOOST_CHECK(txWitnessUnknown_res.IsInvalid());
474-
BOOST_CHECK_EQUAL(txWitnessUnknown_res.GetRejectReason(), "bad-txns-nonstandard-inputs");
475-
BOOST_CHECK_EQUAL(txWitnessUnknown_res.GetDebugMessage(), "input 0 witness program is undefined");
479+
{
480+
CMutableTransaction txWitnessUnknown;
481+
txWitnessUnknown.vout.resize(1);
482+
txWitnessUnknown.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key[1].GetPubKey()));
483+
txWitnessUnknown.vout[0].nValue = 1000;
484+
txWitnessUnknown.vin.resize(1);
485+
txWitnessUnknown.vin[0].prevout.n = 9;
486+
txWitnessUnknown.vin[0].prevout.hash = txFrom.GetHash();
487+
const auto txWitnessUnknown_res = ::ValidateInputsStandardness(CTransaction(txWitnessUnknown), coins);
488+
BOOST_CHECK(txWitnessUnknown_res.IsInvalid());
489+
BOOST_CHECK_EQUAL(txWitnessUnknown_res.GetRejectReason(), "bad-txns-nonstandard-inputs");
490+
BOOST_CHECK_EQUAL(txWitnessUnknown_res.GetDebugMessage(), "input 0 witness program is undefined");
491+
}
476492
}
477493

478494
BOOST_AUTO_TEST_SUITE_END()

0 commit comments

Comments
 (0)