diff --git a/src/policy/policy.h b/src/policy/policy.h --- a/src/policy/policy.h +++ b/src/policy/policy.h @@ -86,7 +86,8 @@ SCRIPT_VERIFY_NULLDUMMY | SCRIPT_VERIFY_SIGPUSHONLY | SCRIPT_VERIFY_MINIMALDATA | SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS | SCRIPT_VERIFY_CLEANSTACK | SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY | - SCRIPT_VERIFY_CHECKSEQUENCEVERIFY | SCRIPT_VERIFY_CHECKDATASIG_SIGOPS; + SCRIPT_VERIFY_CHECKSEQUENCEVERIFY | SCRIPT_VERIFY_CHECKDATASIG_SIGOPS | + SCRIPT_DISALLOW_SEGWIT_RECOVERY; /** * For convenience, standard but not mandatory verify flags. diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -1635,9 +1635,10 @@ CScript pubKey2(pubKeySerialized.begin(), pubKeySerialized.end()); popstack(stack); - // Bail out early if ALLOW_SEGWIT_RECOVERY is set, the redeem script is - // a p2sh segwit program and it was the only item pushed into the stack - if ((flags & SCRIPT_ALLOW_SEGWIT_RECOVERY) != 0 && stack.empty() && + // Bail out early if SCRIPT_DISALLOW_SEGWIT_RECOVERY is not set, the + // redeem script is a p2sh segwit program, and it was the only item + // pushed onto the stack. + if ((flags & SCRIPT_DISALLOW_SEGWIT_RECOVERY) == 0 && stack.empty() && pubKey2.IsWitnessProgram()) { return set_success(serror); } diff --git a/src/script/script_flags.h b/src/script/script_flags.h --- a/src/script/script_flags.h +++ b/src/script/script_flags.h @@ -12,6 +12,7 @@ SCRIPT_VERIFY_NONE = 0, // Evaluate P2SH subscripts (softfork safe, BIP16). + // Note: The Segwit Recovery feature is an exception to P2SH SCRIPT_VERIFY_P2SH = (1U << 0), // Passing a non-strict-DER signature or one with undefined hashtype to a @@ -63,6 +64,7 @@ // be true". // (softfork safe, BIP62 rule 6) // Note: CLEANSTACK should never be used without P2SH or WITNESS. + // Note: The Segwit Recovery feature is an exception to CLEANSTACK SCRIPT_VERIFY_CLEANSTACK = (1U << 8), // Verify CHECKLOCKTIMEVERIFY @@ -106,8 +108,9 @@ // SCRIPT_ENABLE_SCHNORR = (1U << 19), - // Allows the recovery of coins sent to p2sh segwit addresses - SCRIPT_ALLOW_SEGWIT_RECOVERY = (1U << 20), + // The exception to CLEANSTACK and P2SH for the recovery of coins sent + // to p2sh segwit addresses is not allowed. + SCRIPT_DISALLOW_SEGWIT_RECOVERY = (1U << 20), }; #endif // BITCOIN_SCRIPT_SCRIPTFLAGS_H diff --git a/src/script/sigcache.cpp b/src/script/sigcache.cpp --- a/src/script/sigcache.cpp +++ b/src/script/sigcache.cpp @@ -29,7 +29,7 @@ SCRIPT_VERIFY_CHECKSEQUENCEVERIFY | SCRIPT_VERIFY_MINIMALIF | SCRIPT_VERIFY_NULLFAIL | SCRIPT_VERIFY_COMPRESSED_PUBKEYTYPE | SCRIPT_ENABLE_SIGHASH_FORKID | SCRIPT_ENABLE_REPLAY_PROTECTION | - SCRIPT_VERIFY_CHECKDATASIG_SIGOPS | SCRIPT_ALLOW_SEGWIT_RECOVERY; + SCRIPT_VERIFY_CHECKDATASIG_SIGOPS | SCRIPT_DISALLOW_SEGWIT_RECOVERY; /** * Valid signature cache, to avoid doing expensive ECDSA signature checking diff --git a/src/test/data/script_tests.json b/src/test/data/script_tests.json --- a/src/test/data/script_tests.json +++ b/src/test/data/script_tests.json @@ -3262,149 +3262,163 @@ [ "0x16 0x001491b24bf9f5288532960ac687abb035127b1d28a5", "HASH160 0x14 0x17743beb429c55c942d2ec703b98c4d57c2df5c6 EQUAL", - "CLEANSTACK,P2SH", + "CLEANSTACK,DISALLOW_SEGWIT_RECOVERY,P2SH", "CLEANSTACK", - "v0 P2SH-P2WPKH but no SCRIPT_ALLOW_SEGWIT_RECOVERY" + "v0 P2SH-P2WPKH with SCRIPT_DISALLOW_SEGWIT_RECOVERY" ], [ "0x16 0x001491b24bf9f5288532960ac687abb035127b1d28a5", "HASH160 0x14 0x17743beb429c55c942d2ec703b98c4d57c2df5c6 EQUAL", - "ALLOW_SEGWIT_RECOVERY,CLEANSTACK,P2SH", + "CLEANSTACK,P2SH", "OK", - "v0 P2SH-P2WPKH with SCRIPT_ALLOW_SEGWIT_RECOVERY" + "Valid Segwit Recovery with v0 P2SH-P2WPKH" ], [ "0 0x16 0x001491b24bf9f5288532960ac687abb035127b1d28a5", "HASH160 0x14 0x17743beb429c55c942d2ec703b98c4d57c2df5c6 EQUAL", - "ALLOW_SEGWIT_RECOVERY,CLEANSTACK,P2SH", + "CLEANSTACK,P2SH", "CLEANSTACK", - "v0 P2SH-P2WPKH with extra stack item and SCRIPT_ALLOW_SEGWIT_RECOVERY" + "v0 P2SH-P2WPKH Segwit Recovery with extra stack item" ], [ "0x22 0x00205a0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f", "HASH160 0x14 0x17a6be2f8fe8e94f033e53d17beefda0f3ac4409 EQUAL", - "CLEANSTACK,P2SH", + "CLEANSTACK,DISALLOW_SEGWIT_RECOVERY,P2SH", "CLEANSTACK", - "v0 P2SH-P2WSH but no SCRIPT_ALLOW_SEGWIT_RECOVERY" + "v0 P2SH-P2WSH with SCRIPT_DISALLOW_SEGWIT_RECOVERY" ], [ "0x22 0x00205a0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f", "HASH160 0x14 0x17a6be2f8fe8e94f033e53d17beefda0f3ac4409 EQUAL", - "ALLOW_SEGWIT_RECOVERY,CLEANSTACK,P2SH", + "CLEANSTACK,P2SH", "OK", - "v0 P2SH-P2WSH with SCRIPT_ALLOW_SEGWIT_RECOVERY" + "Valid Segwit Recovery with v0 P2SH-P2WSH" ], [ "0 0x22 0x00205a0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f", "HASH160 0x14 0x17a6be2f8fe8e94f033e53d17beefda0f3ac4409 EQUAL", - "ALLOW_SEGWIT_RECOVERY,CLEANSTACK,P2SH", + "CLEANSTACK,P2SH", "CLEANSTACK", - "v0 P2SH-P2WSH with extra stack item and SCRIPT_ALLOW_SEGWIT_RECOVERY" + "v0 P2SH-P2WSH Segwit Recovery with extra stack item" ], [ "0x03 0x00015a", "HASH160 0x14 0x40b6941895022d458de8f4bbfe27f3aaa4fb9a74 EQUAL", - "ALLOW_SEGWIT_RECOVERY,CLEANSTACK,P2SH", + "CLEANSTACK,P2SH", "CLEANSTACK", - "Invalid witness program (too short) with SCRIPT_ALLOW_SEGWIT_RECOVERY" + "Segwit Recovery with invalid witness program (too short)" ], [ "0x04 0x00025a01", "HASH160 0x14 0x86123d8e050333a605e434ecf73128d83815b36f EQUAL", - "ALLOW_SEGWIT_RECOVERY,CLEANSTACK,P2SH", + "CLEANSTACK,P2SH", "OK", - "Valid witness program (min allowed length) with SCRIPT_ALLOW_SEGWIT_RECOVERY" + "Segwit Recovery with valid witness program (min allowed length)" ], [ "0x2a 0x00285a0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f2021222324252627", "HASH160 0x14 0xdf7b93f88e83471b479fb219ae90e5b633d6b750 EQUAL", - "ALLOW_SEGWIT_RECOVERY,CLEANSTACK,P2SH", + "CLEANSTACK,P2SH", "OK", - "Valid witness program (max allowed length) with SCRIPT_ALLOW_SEGWIT_RECOVERY" + "Segwit Recovery with valid witness program (max allowed length)" ], [ "0x2b 0x00295a0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728", "HASH160 0x14 0x13aa4fcfd630508e0794dca320cac172c5790aea EQUAL", - "ALLOW_SEGWIT_RECOVERY,CLEANSTACK,P2SH", + "CLEANSTACK,P2SH", "CLEANSTACK", - "Invalid witness program (too long) with SCRIPT_ALLOW_SEGWIT_RECOVERY" + "Segwit Recovery with invalid witness program (too long)" ], [ "0x22 0x60205a0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f", "HASH160 0x14 0x9b0c7017004d3818b7c833ddb3cb5547a22034d0 EQUAL", - "ALLOW_SEGWIT_RECOVERY,CLEANSTACK,P2SH", + "CLEANSTACK,P2SH", "OK", - "Valid witness program (max allowed version) with SCRIPT_ALLOW_SEGWIT_RECOVERY" + "Segwit Recovery with valid witness program (max allowed version)" ], [ "0x22 0x4f205a0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f", "HASH160 0x14 0x97aa1e96e49ca6d744d7344f649dd9f94bcc35eb EQUAL", - "ALLOW_SEGWIT_RECOVERY,CLEANSTACK,P2SH", + "CLEANSTACK,P2SH", "CLEANSTACK", - "Invalid witness program (invalid version -1) with SCRIPT_ALLOW_SEGWIT_RECOVERY" + "Segwit Recovery with invalid witness program (invalid version -1)" ], [ "0x23 0x0111205a0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f", "HASH160 0x14 0x4b5321beb1c09f593ff3c02be4af21c7f949e101 EQUAL", - "ALLOW_SEGWIT_RECOVERY,CLEANSTACK,P2SH", + "CLEANSTACK,P2SH", "CLEANSTACK", - "Invalid witness program (invalid version 17) with SCRIPT_ALLOW_SEGWIT_RECOVERY" + "Segwit Recovery with invalid witness program (invalid version 17)" ], [ "0x23 0x00205a0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f51", "HASH160 0x14 0x8eb812176c9e71732584123dd06d3246e659b199 EQUAL", - "ALLOW_SEGWIT_RECOVERY,CLEANSTACK,P2SH", + "CLEANSTACK,P2SH", "CLEANSTACK", - "Invalid witness program (more than 2 stack items) with SCRIPT_ALLOW_SEGWIT_RECOVERY" + "Segwit Recovery with invalid witness program (more than 2 stack items)" ], [ "0x04 0x00020000", "HASH160 0x14 0x0e01bcfe7c6f3fd2fd8f81092299369744684733 EQUAL", - "ALLOW_SEGWIT_RECOVERY,CLEANSTACK,P2SH", + "CLEANSTACK,P2SH", "OK", "Valid segwit recovery, in spite of false value being left on stack (0)" ], [ "0x04 0x00020080", "HASH160 0x14 0x10ddc638cb26615f867dad80efacced9e73766bc EQUAL", - "ALLOW_SEGWIT_RECOVERY,CLEANSTACK,P2SH", + "CLEANSTACK,P2SH", "OK", "Valid segwit recovery, in spite of false value being left on stack (minus 0)" ], +[ + "0x04 0x00020000", + "HASH160 0x14 0x0e01bcfe7c6f3fd2fd8f81092299369744684733 EQUAL", + "CLEANSTACK,DISALLOW_SEGWIT_RECOVERY,P2SH", + "EVAL_FALSE", + "Otherwise valid segwit recovery, in spite of false value being left on stack (0), but with SCRIPT_DISALLOW_SEGWIT_RECOVERY" +], +[ + "0x04 0x00020080", + "HASH160 0x14 0x10ddc638cb26615f867dad80efacced9e73766bc EQUAL", + "CLEANSTACK,DISALLOW_SEGWIT_RECOVERY,P2SH", + "EVAL_FALSE", + "Otherwise valid segwit recovery, in spite of false value being left on stack (minus 0), but with SCRIPT_DISALLOW_SEGWIT_RECOVERY" +], [ "0x22 0x50205a0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f", "HASH160 0x14 0xbe02794ceede051da41b420e88a86fff2802af06 EQUAL", - "ALLOW_SEGWIT_RECOVERY,CLEANSTACK,P2SH", + "CLEANSTACK,P2SH", "BAD_OPCODE", - "Invalid witness program (OP_RESERVED in version field) with SCRIPT_ALLOW_SEGWIT_RECOVERY" + "Segwit Recovery with invalid witness program (OP_RESERVED in version field)" ], [ "0x17 0x01001491b24bf9f5288532960ac687abb035127b1d28a5", "HASH160 0x14 0x0718743e67c1ef4911e0421f206c5ff81755718e EQUAL", - "ALLOW_SEGWIT_RECOVERY,CLEANSTACK,P2SH", + "CLEANSTACK,P2SH", "CLEANSTACK", - "Invalid witness program (non-minimal push in version field) with SCRIPT_ALLOW_SEGWIT_RECOVERY" + "Segwit Recovery with invalid witness program (non-minimal push in version field)" ], [ "0x05 0x004c0245aa", "HASH160 0x14 0xd3ec673296c7fd7e1a9e53bfc36f414de303e905 EQUAL", - "ALLOW_SEGWIT_RECOVERY,CLEANSTACK,P2SH", + "CLEANSTACK,P2SH", "CLEANSTACK", - "Invalid witness program (non-minimal push in program field) with SCRIPT_ALLOW_SEGWIT_RECOVERY" + "Segwit Recovery with invalid witness program (non-minimal push in program field)" ], [ "0x16 0x001491b24bf9f5288532960ac687abb035127b1d28a5", "HASH160 0x14 0x17a6be2f8fe8e94f033e53d17beefda0f3ac4409 EQUAL", - "ALLOW_SEGWIT_RECOVERY,CLEANSTACK,P2SH", + "CLEANSTACK,P2SH", "EVAL_FALSE", - "v0 P2SH-P2WPKH whose redeem script hash does not match P2SH output and SCRIPT_ALLOW_SEGWIT_RECOVERY" + "v0 P2SH-P2WPKH Segwit Recovery whose redeem script hash does not match P2SH output" ], [ "0x16 0x001491b24bf9f5288532960ac687abb035127b1d28a5", "1", - "ALLOW_SEGWIT_RECOVERY,CLEANSTACK,P2SH", + "CLEANSTACK,P2SH", "CLEANSTACK", - "v0 P2SH-P2WPKH spending a non-P2SH output and SCRIPT_ALLOW_SEGWIT_RECOVERY" + "v0 P2SH-P2WPKH Segwit Recovery spending a non-P2SH output" ], ["CHECKSEQUENCEVERIFY tests"], diff --git a/src/test/script_tests.cpp b/src/test/script_tests.cpp --- a/src/test/script_tests.cpp +++ b/src/test/script_tests.cpp @@ -2080,25 +2080,25 @@ .EditPush(64, "01", "41") .ScriptError(SCRIPT_ERR_EVAL_FALSE)); - // Tests SCRIPT_ALLOW_SEGWIT_RECOVERY - const uint32_t allowSegwitRecoveryFlags = SCRIPT_VERIFY_CLEANSTACK | - SCRIPT_VERIFY_P2SH | - SCRIPT_ALLOW_SEGWIT_RECOVERY; + // Tests Segwit Recovery transactions and SCRIPT_DISALLOW_SEGWIT_RECOVERY + const uint32_t allowSegwitRecoveryFlags = + SCRIPT_VERIFY_CLEANSTACK | SCRIPT_VERIFY_P2SH; tests.push_back( TestBuilder(CScript() << OP_0 << ToByteVector(keys.pubkey0.GetID()), - "v0 P2SH-P2WPKH but no SCRIPT_ALLOW_SEGWIT_RECOVERY", - SCRIPT_VERIFY_CLEANSTACK | SCRIPT_VERIFY_P2SH, true) + "v0 P2SH-P2WPKH with SCRIPT_DISALLOW_SEGWIT_RECOVERY", + SCRIPT_VERIFY_CLEANSTACK | SCRIPT_VERIFY_P2SH | + SCRIPT_DISALLOW_SEGWIT_RECOVERY, + true) .PushRedeem() .ScriptError(SCRIPT_ERR_CLEANSTACK)); tests.push_back( TestBuilder(CScript() << OP_0 << ToByteVector(keys.pubkey0.GetID()), - "v0 P2SH-P2WPKH with SCRIPT_ALLOW_SEGWIT_RECOVERY", + "Valid Segwit Recovery with v0 P2SH-P2WPKH", allowSegwitRecoveryFlags, true) .PushRedeem()); tests.push_back( TestBuilder(CScript() << OP_0 << ToByteVector(keys.pubkey0.GetID()), - "v0 P2SH-P2WPKH with extra stack item and " - "SCRIPT_ALLOW_SEGWIT_RECOVERY", + "v0 P2SH-P2WPKH Segwit Recovery with extra stack item", allowSegwitRecoveryFlags, true) .Num(0) .PushRedeem() @@ -2108,82 +2108,82 @@ 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31})); tests.push_back( TestBuilder(CScript() << OP_0 << ToByteVector(dummy256), - "v0 P2SH-P2WSH but no SCRIPT_ALLOW_SEGWIT_RECOVERY", - SCRIPT_VERIFY_CLEANSTACK | SCRIPT_VERIFY_P2SH, true) + "v0 P2SH-P2WSH with SCRIPT_DISALLOW_SEGWIT_RECOVERY", + SCRIPT_VERIFY_CLEANSTACK | SCRIPT_VERIFY_P2SH | + SCRIPT_DISALLOW_SEGWIT_RECOVERY, + true) .PushRedeem() .ScriptError(SCRIPT_ERR_CLEANSTACK)); + tests.push_back(TestBuilder(CScript() << OP_0 << ToByteVector(dummy256), + "Valid Segwit Recovery with v0 P2SH-P2WSH", + allowSegwitRecoveryFlags, true) + .PushRedeem()); tests.push_back( TestBuilder(CScript() << OP_0 << ToByteVector(dummy256), - "v0 P2SH-P2WSH with SCRIPT_ALLOW_SEGWIT_RECOVERY", + "v0 P2SH-P2WSH Segwit Recovery with extra stack item", allowSegwitRecoveryFlags, true) - .PushRedeem()); - tests.push_back(TestBuilder(CScript() << OP_0 << ToByteVector(dummy256), - "v0 P2SH-P2WSH with extra stack item and " - "SCRIPT_ALLOW_SEGWIT_RECOVERY", - allowSegwitRecoveryFlags, true) - .Num(0) - .PushRedeem() - .ScriptError(SCRIPT_ERR_CLEANSTACK)); - // Tests the limits of IsWitnessProgram along with - // SCRIPT_ALLOW_SEGWIT_RECOVERY + .Num(0) + .PushRedeem() + .ScriptError(SCRIPT_ERR_CLEANSTACK)); + // Tests the limits of IsWitnessProgram without + // SCRIPT_DISALLOW_SEGWIT_RECOVERY. std::vector shortprogram({90, 1}); tests.push_back( TestBuilder(CScript() << OP_0 << std::vector(shortprogram.begin(), shortprogram.end() - 1), - "Invalid witness program (too short) with " - "SCRIPT_ALLOW_SEGWIT_RECOVERY", + "Segwit Recovery with invalid witness program (too short)", allowSegwitRecoveryFlags, true) .PushRedeem() .ScriptError(SCRIPT_ERR_CLEANSTACK)); tests.push_back( - TestBuilder(CScript() << OP_0 << shortprogram, - "Valid witness program (min allowed length) with " - "SCRIPT_ALLOW_SEGWIT_RECOVERY", - allowSegwitRecoveryFlags, true) + TestBuilder( + CScript() << OP_0 << shortprogram, + "Segwit Recovery with valid witness program (min allowed length)", + allowSegwitRecoveryFlags, true) .PushRedeem()); std::vector longprogram( {90, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40}); tests.push_back( - TestBuilder(CScript() << OP_0 - << std::vector(longprogram.begin(), - longprogram.end() - 1), - "Valid witness program (max allowed length) with " - "SCRIPT_ALLOW_SEGWIT_RECOVERY", - allowSegwitRecoveryFlags, true) + TestBuilder( + CScript() << OP_0 + << std::vector(longprogram.begin(), + longprogram.end() - 1), + "Segwit Recovery with valid witness program (max allowed length)", + allowSegwitRecoveryFlags, true) .PushRedeem()); - tests.push_back(TestBuilder(CScript() << OP_0 << longprogram, - "Invalid witness program (too long) with " - "SCRIPT_ALLOW_SEGWIT_RECOVERY", - allowSegwitRecoveryFlags, true) - .PushRedeem() - .ScriptError(SCRIPT_ERR_CLEANSTACK)); tests.push_back( - TestBuilder(CScript() << OP_16 << ToByteVector(dummy256), - "Valid witness program (max allowed version) with " - "SCRIPT_ALLOW_SEGWIT_RECOVERY", + TestBuilder(CScript() << OP_0 << longprogram, + "Segwit Recovery with invalid witness program (too long)", allowSegwitRecoveryFlags, true) + .PushRedeem() + .ScriptError(SCRIPT_ERR_CLEANSTACK)); + tests.push_back( + TestBuilder( + CScript() << OP_16 << ToByteVector(dummy256), + "Segwit Recovery with valid witness program (max allowed version)", + allowSegwitRecoveryFlags, true) .PushRedeem()); tests.push_back( - TestBuilder(CScript() << OP_1NEGATE << ToByteVector(dummy256), - "Invalid witness program (invalid version -1) with " - "SCRIPT_ALLOW_SEGWIT_RECOVERY", - allowSegwitRecoveryFlags, true) + TestBuilder( + CScript() << OP_1NEGATE << ToByteVector(dummy256), + "Segwit Recovery with invalid witness program (invalid version -1)", + allowSegwitRecoveryFlags, true) .PushRedeem() .ScriptError(SCRIPT_ERR_CLEANSTACK)); tests.push_back( - TestBuilder(CScript() << 17 << ToByteVector(dummy256), - "Invalid witness program (invalid version 17) with " - "SCRIPT_ALLOW_SEGWIT_RECOVERY", - allowSegwitRecoveryFlags, true) + TestBuilder( + CScript() << 17 << ToByteVector(dummy256), + "Segwit Recovery with invalid witness program (invalid version 17)", + allowSegwitRecoveryFlags, true) .PushRedeem() .ScriptError(SCRIPT_ERR_CLEANSTACK)); tests.push_back( TestBuilder(CScript() << OP_0 << ToByteVector(dummy256) << OP_1, - "Invalid witness program (more than 2 stack " - "items) with SCRIPT_ALLOW_SEGWIT_RECOVERY", + "Segwit Recovery with invalid witness program (more than 2 " + "stack items)", allowSegwitRecoveryFlags, true) .PushRedeem() .ScriptError(SCRIPT_ERR_CLEANSTACK)); @@ -2199,12 +2199,29 @@ "on stack (minus 0)", allowSegwitRecoveryFlags, true) .PushRedeem()); + tests.push_back( + TestBuilder(CScript() << OP_0 << std::vector({0, 0}), + "Otherwise valid segwit recovery, in spite of false value " + "being left " + "on stack (0), but with SCRIPT_DISALLOW_SEGWIT_RECOVERY", + allowSegwitRecoveryFlags | SCRIPT_DISALLOW_SEGWIT_RECOVERY, + true) + .PushRedeem() + .ScriptError(SCRIPT_ERR_EVAL_FALSE)); tests.push_back( TestBuilder( - CScript() << OP_RESERVED << ToByteVector(dummy256), - "Invalid witness program (OP_RESERVED in version field) with " - "SCRIPT_ALLOW_SEGWIT_RECOVERY", - allowSegwitRecoveryFlags, true) + CScript() << OP_0 << std::vector({0, 0x80}), + "Otherwise valid segwit recovery, in spite of false value being " + "left " + "on stack (minus 0), but with SCRIPT_DISALLOW_SEGWIT_RECOVERY", + allowSegwitRecoveryFlags | SCRIPT_DISALLOW_SEGWIT_RECOVERY, true) + .PushRedeem() + .ScriptError(SCRIPT_ERR_EVAL_FALSE)); + tests.push_back( + TestBuilder(CScript() << OP_RESERVED << ToByteVector(dummy256), + "Segwit Recovery with invalid witness program (OP_RESERVED " + "in version field)", + allowSegwitRecoveryFlags, true) .PushRedeem() .ScriptError(SCRIPT_ERR_BAD_OPCODE)); const uint8_t nonmin_push_00[] = {1, 0}; @@ -2212,8 +2229,8 @@ TestBuilder( CScript(&nonmin_push_00[0], &nonmin_push_00[sizeof(nonmin_push_00)]) << ToByteVector(keys.pubkey0.GetID()), - "Invalid witness program (non-minimal push in version field) with " - "SCRIPT_ALLOW_SEGWIT_RECOVERY", + "Segwit Recovery with invalid witness program (non-minimal push in " + "version field)", allowSegwitRecoveryFlags, true) .PushRedeem() .ScriptError(SCRIPT_ERR_CLEANSTACK)); @@ -2222,22 +2239,22 @@ TestBuilder((CScript() << OP_0) + CScript(&nonmin_push_45aa[0], &nonmin_push_45aa[sizeof(nonmin_push_45aa)]), - "Invalid witness program (non-minimal push in program " - "field) with SCRIPT_ALLOW_SEGWIT_RECOVERY", + "Segwit Recovery with invalid witness program (non-minimal " + "push in program field)", allowSegwitRecoveryFlags, true) .PushRedeem() .ScriptError(SCRIPT_ERR_CLEANSTACK)); tests.push_back( TestBuilder(CScript() << OP_0 << ToByteVector(dummy256), - "v0 P2SH-P2WPKH whose redeem script hash does not match " - "P2SH output and SCRIPT_ALLOW_SEGWIT_RECOVERY", + "v0 P2SH-P2WPKH Segwit Recovery whose redeem script hash " + "does not match " + "P2SH output", allowSegwitRecoveryFlags, true) .Push(CScript() << OP_0 << ToByteVector(keys.pubkey0.GetID())) .ScriptError(SCRIPT_ERR_EVAL_FALSE)); tests.push_back( TestBuilder(CScript() << OP_1, - "v0 P2SH-P2WPKH spending a non-P2SH output and " - "SCRIPT_ALLOW_SEGWIT_RECOVERY", + "v0 P2SH-P2WPKH Segwit Recovery spending a non-P2SH output", allowSegwitRecoveryFlags) .Push(CScript() << OP_0 << ToByteVector(keys.pubkey0.GetID())) .ScriptError(SCRIPT_ERR_CLEANSTACK)); diff --git a/src/test/scriptflags.cpp b/src/test/scriptflags.cpp --- a/src/test/scriptflags.cpp +++ b/src/test/scriptflags.cpp @@ -33,7 +33,7 @@ {"REPLAY_PROTECTION", SCRIPT_ENABLE_REPLAY_PROTECTION}, {"CHECKDATASIG", SCRIPT_VERIFY_CHECKDATASIG_SIGOPS}, {"SCHNORR", SCRIPT_ENABLE_SCHNORR}, - {"ALLOW_SEGWIT_RECOVERY", SCRIPT_ALLOW_SEGWIT_RECOVERY}, + {"DISALLOW_SEGWIT_RECOVERY", SCRIPT_DISALLOW_SEGWIT_RECOVERY}, }; uint32_t ParseScriptFlags(std::string strFlags) { diff --git a/src/test/sigcache_tests.cpp b/src/test/sigcache_tests.cpp --- a/src/test/sigcache_tests.cpp +++ b/src/test/sigcache_tests.cpp @@ -38,7 +38,7 @@ SCRIPT_VERIFY_CHECKSEQUENCEVERIFY | SCRIPT_VERIFY_MINIMALIF | SCRIPT_VERIFY_NULLFAIL | SCRIPT_VERIFY_COMPRESSED_PUBKEYTYPE | SCRIPT_ENABLE_SIGHASH_FORKID | SCRIPT_ENABLE_REPLAY_PROTECTION | - SCRIPT_VERIFY_CHECKDATASIG_SIGOPS | SCRIPT_ALLOW_SEGWIT_RECOVERY; + SCRIPT_VERIFY_CHECKDATASIG_SIGOPS | SCRIPT_DISALLOW_SEGWIT_RECOVERY; /* We will be testing that these flags DO affect the cache entry. The expected * behaviour is that flags which are not explicitly listed as invariant in * script/sigcache.cpp will affect the cache entry. Here we will thus enforce diff --git a/src/validation.cpp b/src/validation.cpp --- a/src/validation.cpp +++ b/src/validation.cpp @@ -719,9 +719,6 @@ } if (IsGreatWallEnabledForCurrentBlock(config)) { - if (!fRequireStandard) { - extraFlags |= SCRIPT_ALLOW_SEGWIT_RECOVERY; - } extraFlags |= SCRIPT_ENABLE_SCHNORR; } @@ -1238,14 +1235,8 @@ // This differs from MANDATORY_SCRIPT_VERIFY_FLAGS as it contains // additional upgrade flags (see AcceptToMemoryPoolWorker variable // extraFlags). - // Even though it is not a mandatory flag, - // SCRIPT_ALLOW_SEGWIT_RECOVERY is strictly more permissive than the - // set of standard flags. It therefore needs to be added in order to - // check if we need to penalize the peer that sent us the - // transaction or not. uint32_t mandatoryFlags = - (flags & ~STANDARD_NOT_MANDATORY_VERIFY_FLAGS) | - SCRIPT_ALLOW_SEGWIT_RECOVERY; + flags & ~STANDARD_NOT_MANDATORY_VERIFY_FLAGS; if (flags != mandatoryFlags) { // Check whether the failure was caused by a non-mandatory // script verification check. If so, don't trigger DoS @@ -1620,13 +1611,11 @@ flags |= SCRIPT_VERIFY_CLEANSTACK; } - // If the Great Wall fork is enabled, we start accepting transactions - // recovering coins sent to segwit addresses. We also start accepting + // If the Great Wall fork is enabled, we start accepting // 65/64-byte Schnorr signatures in CHECKSIG and CHECKDATASIG respectively, // and their verify variants. We also stop accepting 65 byte signatures in // CHECKMULTISIG and its verify variant. if (IsGreatWallEnabled(config, pChainTip)) { - flags |= SCRIPT_ALLOW_SEGWIT_RECOVERY; flags |= SCRIPT_ENABLE_SCHNORR; } diff --git a/test/functional/abc-segwit-recovery-activation.py b/test/functional/abc-segwit-recovery.py rename from test/functional/abc-segwit-recovery-activation.py rename to test/functional/abc-segwit-recovery.py --- a/test/functional/abc-segwit-recovery-activation.py +++ b/test/functional/abc-segwit-recovery.py @@ -4,15 +4,19 @@ # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. """ -This test checks activation of the SCRIPT_ALLOW_SEGWIT_RECOVERY flag +This test checks that blocks containing segwit recovery transactions will be accepted, +that segwit recovery transactions are rejected from mempool acceptance (even with +-acceptnonstdtxn=1), and that segwit recovery transactions don't result in bans. """ +import time + from test_framework.blocktools import ( create_block, create_coinbase, make_conform_to_ctor, ) -from test_framework.comptool import RejectResult, TestInstance, TestManager +from test_framework.comptool import TestInstance, TestManager from test_framework.messages import ( COIN, COutPoint, @@ -36,16 +40,11 @@ ) from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( - assert_equal, assert_raises_rpc_error, sync_blocks, ) -# far into the future -GREAT_WALL_START_TIME = 2000000000 - -# First blocks (initial coinbases, pre-fork test blocks) happen 1 day before. -FIRST_BLOCK_TIME = GREAT_WALL_START_TIME - 86400 +TEST_TIME = int(time.time()) # Error due to non clean stack CLEANSTACK_ERROR = b'non-mandatory-script-verify-flag (Script did not clean its stack)' @@ -63,7 +62,7 @@ self.n = n -class SegwitRecoveryActivationTest(BitcoinTestFramework): +class SegwitRecoveryTest(BitcoinTestFramework): def set_test_params(self): self.num_nodes = 2 @@ -72,35 +71,28 @@ self.tip = None self.blocks = {} # We have 2 nodes: - # 1) node_nonstd (nodes[0]) accepts non-standard txns. It's used to - # test the activation itself via TestManager. + # 1) node_nonstd (nodes[0]) accepts non-standard txns. It does not + # accept Segwit recovery transactions, since it is included in + # standard flags, and transactions that violate these flags are + # never accepted into the mempool. # 2) node_std (nodes[1]) doesn't accept non-standard txns and # doesn't have us whitelisted. It's used to test for bans, as we # connect directly to it via mininode and send a segwit spending - # txn. This transaction is non-standard and, before activation, - # also invalid. We check, before and after activation, that - # sending this transaction doesn't result in a ban. + # txn. This transaction is non-standard. We check that sending + # this transaction doesn't result in a ban. # Nodes are connected to each other, so node_std receives blocks and # transactions that node_nonstd has accepted. Since we are checking # that segwit spending txn are not resulting in bans, node_nonstd # doesn't get banned when forwarding this kind of transactions to # node_std. self.extra_args = [['-whitelist=127.0.0.1', - "-acceptnonstdtxn", - "-greatwallactivationtime={}".format( - GREAT_WALL_START_TIME), - "-replayprotectionactivationtime={}".format( - 2 * GREAT_WALL_START_TIME)], - ["-acceptnonstdtxn=0", - "-greatwallactivationtime={}".format( - GREAT_WALL_START_TIME), - "-replayprotectionactivationtime={}".format( - 2 * GREAT_WALL_START_TIME)]] + "-acceptnonstdtxn"], + ["-acceptnonstdtxn=0"]] def run_test(self): # Move the mocktime up to activation for node in self.nodes: - node.setmocktime(GREAT_WALL_START_TIME) + node.setmocktime(TEST_TIME) test = TestManager(self, self.options.tmpdir) # TestManager only connects to node_nonstd (nodes[0]) test.add_all_connections([self.nodes[0]]) @@ -112,7 +104,7 @@ def next_block(self, number): if self.tip == None: base_block_hash = self.genesis_hash - block_time = FIRST_BLOCK_TIME + block_time = TEST_TIME else: base_block_hash = self.tip.sha256 block_time = self.tip.nTime + 1 @@ -189,8 +181,6 @@ # 2) txspend: spends outputs from segwit addresses def create_segwit_fund_and_spend_tx(spend, case0=False): if not case0: - # To make sure we'll be able to recover coins sent to segwit addresses, - # we test using historical recoveries from btc.com: # Spending from a P2SH-P2WPKH coin, # txhash:a45698363249312f8d3d93676aa714be59b0bd758e62fa054fb1ea6218480691 redeem_script0 = bytearray.fromhex( @@ -281,99 +271,38 @@ txfund_case0, txspend_case0 = create_segwit_fund_and_spend_tx( out[1], True) - # Create blocks to get closer to activate the fork. # Mine txfund, as it can't go into node_std mempool because it's # nonstandard. b = block(5555) - b.nTime = GREAT_WALL_START_TIME - 1 update_block(5555, [txfund, txfund_case0]) yield accepted() - for i in range(5): - block(5100 + i) - test.blocks_and_transactions.append([self.tip, True]) - yield test - # Since the TestManager is not connected to node_std, we must check # both nodes are synchronized before continuing. sync_blocks(self.nodes) - # Check we are just before the activation time - assert_equal(node_nonstd.getblockheader( - node_nonstd.getbestblockhash())['mediantime'], GREAT_WALL_START_TIME - 1) - assert_equal(node_std.getblockheader( - node_std.getbestblockhash())['mediantime'], GREAT_WALL_START_TIME - 1) - - # Before the fork, segwit spending txns are rejected. - assert_raises_rpc_error(-26, RPC_CLEANSTACK_ERROR, - node_nonstd.sendrawtransaction, ToHex(txspend)) - assert_raises_rpc_error(-26, RPC_CLEANSTACK_ERROR, - node_std.sendrawtransaction, ToHex(txspend)) - assert_raises_rpc_error(-26, RPC_EVAL_FALSE_ERROR, - node_nonstd.sendrawtransaction, ToHex(txspend_case0)) - assert_raises_rpc_error(-26, RPC_EVAL_FALSE_ERROR, - node_std.sendrawtransaction, ToHex(txspend_case0)) - - # Blocks containing segwit spending txns are rejected as well. - block(2) - update_block(2, [txspend, txspend_case0]) - yield rejected(RejectResult(16, b'blk-bad-inputs')) - - # Rewind bad block - tip(5104) - - # Check that non-upgraded nodes checking for standardness are not - # banning nodes sending segwit spending txns. - check_for_no_ban_on_rejected_tx(txspend, 64, CLEANSTACK_ERROR) - check_for_no_ban_on_rejected_tx(txspend_case0, 64, EVAL_FALSE_ERROR) - - # Activate the fork in both nodes! - forkblock = block(5556) - yield accepted() - sync_blocks(self.nodes) - - # Check we just activated the fork - assert_equal(node_nonstd.getblockheader( - node_nonstd.getbestblockhash())['mediantime'], GREAT_WALL_START_TIME) - assert_equal(node_std.getblockheader( - node_std.getbestblockhash())['mediantime'], GREAT_WALL_START_TIME) - # Check that upgraded nodes checking for standardness are not banning # nodes sending segwit spending txns. check_for_no_ban_on_rejected_tx(txspend, 64, CLEANSTACK_ERROR) check_for_no_ban_on_rejected_tx(txspend_case0, 64, EVAL_FALSE_ERROR) - # Segwit spending txns are accepted in the mempool of nodes not checking - # for standardness, but rejected in nodes that check. - node_nonstd.sendrawtransaction(ToHex(txspend)) - node_nonstd.sendrawtransaction(ToHex(txspend_case0)) - check_mempool_equal(node_nonstd, [txspend, txspend_case0]) + # Segwit recovery txns are never accepted into the mempool, + # as they are included in standard flags. + assert_raises_rpc_error(-26, RPC_CLEANSTACK_ERROR, + node_nonstd.sendrawtransaction, ToHex(txspend)) + assert_raises_rpc_error(-26, RPC_EVAL_FALSE_ERROR, + node_nonstd.sendrawtransaction, ToHex(txspend_case0)) assert_raises_rpc_error(-26, RPC_CLEANSTACK_ERROR, node_std.sendrawtransaction, ToHex(txspend)) assert_raises_rpc_error(-26, RPC_EVAL_FALSE_ERROR, node_std.sendrawtransaction, ToHex(txspend_case0)) - # Blocks containing segwit spending txns are now accepted in both - # nodes. + # Blocks containing segwit spending txns are accepted in both nodes. block(5) postforkblock = update_block(5, [txspend, txspend_case0]) yield accepted() sync_blocks(self.nodes) - # Ok, now we check if a reorg work properly accross the activation. - node_nonstd.invalidateblock(postforkblock.hash) - check_mempool_equal(node_nonstd, [txspend, txspend_case0]) - - # Also check that nodes checking for standardness don't return a segwit - # spending txn into the mempool when disconnecting a block. - node_std.invalidateblock(postforkblock.hash) - assert(len(node_std.getrawmempool()) == 0) - - # Deactivate the fork. The spending tx has been evicted from the - # mempool - node_nonstd.invalidateblock(forkblock.hash) - assert(len(node_nonstd.getrawmempool()) == 0) - if __name__ == '__main__': - SegwitRecoveryActivationTest().main() + SegwitRecoveryTest().main()