BIP 118 (ANYPREVOUT)

[ajtowns]

Implementation of BIP 118

[naumenkogs]

I am not 100% this repo is a good place for code comments, let me know if it's not.

Maybe a dumb question, but why 0x40 is not here?

[instagibbs]

IIUC, 0x40 would be ANYPREVOUT|DEFAULT, so to speak. Instead it has to be ANYPREVOUT|ALL, which is 0x41

[ajtowns]

Yeah -- also, the same reason that 0x80 (ANYONECANPAY|DEFAULT) isn't there.

[stevenroose]

Doesn't this also make it ANYAMOUNT?

[instagibbs]

yes, SIGHASH_ANYPREVOUTANYSCRIPT omits the script and the amount: https://github.com/bitcoin/bips/blob/master/bip-0118.mediawiki#signature-message

[ajtowns]

Perhaps instead of ANYPREVOUT and ANYPREVOUTANYSCRIPT, it would be better to name them ANYPREVOUTLIMITED and ANYPREVOUT? The latter verifies a particular pubkey is signing the output(s); the fomer is more limited, in that it also verifies that the prevout's scriptPubKey and value, and the particular tapscript that's being used.

[darosior]

Just had a look at the first commit for now.

• What do you think of making the random tests into fuzz targets?
• What do you think of having a feature_anyprevout.py functional with a very basic implementation of usecases? (For instance a trivial covenant, and a very basic eltoo channel where an outdated update is broadcast and the new one can be rebound.)
  Also, a basic functional test that would sanity check a valid and invalid signature would fix BIP 118 (ANYPREVOUT) #4 (review).

Happy to help, of course. Not asking you to do it. I'm looking at a fuzz target currently that would assert the result of the "old" SignatureHashSchnorr is always the same as the result of the "new" SignatureHashSchnorr with KeyVersion::TAPROOT.

[darosior]

All tests pass if i change this to always treat any ANYPREVOUT signature as valid.

diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index 8f497fdc92..bad34be83b 100644
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -399,7 +399,7 @@ static bool EvalChecksigTapscript(const valtype& sig, const valtype& pubkey, Scr
     } else if ((pubkey.size() == 1 || pubkey.size() == 33) && pubkey[0] == 0x01) {
         if ((flags & SCRIPT_VERIFY_DISCOURAGE_ANYPREVOUT) != 0) {
             return set_error(serror, SCRIPT_ERR_DISCOURAGE_UPGRADABLE_PUBKEYTYPE);
-        } else if ((flags & SCRIPT_VERIFY_ANYPREVOUT) == 0) {
+        } else if ((flags & SCRIPT_VERIFY_ANYPREVOUT) == 1) {
             return true;
         } else if (pubkey.size() == 1) {
             assert(execdata.m_internal_key);

[darosior]

Changing the semantic to the opposite will not fail any unit test.

diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index 8f497fdc92..e4ed144c7d 100644
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -1752,7 +1754,7 @@ bool SignatureHashSchnorr(uint256& hash_out, ScriptExecutionData& execdata, cons
     // Additional data for BIP 342 signatures
     if (sigversion == SigVersion::TAPSCRIPT) {
         assert(execdata.m_tapleaf_hash_init);
-        if (input_type != SIGHASH_ANYPREVOUTANYSCRIPT) {
+        if (input_type == SIGHASH_ANYPREVOUTANYSCRIPT) {
             ss << execdata.m_tapleaf_hash;
         }
         ss << uint8_t(keyversion);

[ajtowns]

I believe that if you use test/functional/feature_taproot.py --dumptests and make the data available for script_assets_test in test/script_tests.cpp you'll get unit test failures for these sorts of changes. See test/fuzz/script_assets_test_minimizer.cpp for some hints, though you don't need to actually run the fuzzer -- that just wastes less time.

[darosior]

nit: maybe a bit clearer:

diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index 227f2ab385..b6791033bf 100644
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -1657,6 +1657,18 @@ static bool HandleMissingData(MissingDataBehavior mdb)
     assert(!"Unknown MissingDataBehavior value");
 }
 
+bool IsHashTypeValid(uint8_t hash_type, KeyVersion key_version) {
+    switch (hash_type) {
+        case 0: case 1: case 2: case 3:
+        case 0x81: case 0x82: case 0x83:
+            return true;
+        case 0x41: case 0x42: case 0x43:
+        case 0xc1: case 0xc2: case 0xc3:
+            return key_version == KeyVersion::ANYPREVOUT;
+    }
+    return false;
+}
+
 template<typename T>
 bool SignatureHashSchnorr(uint256& hash_out, ScriptExecutionData& execdata, const T& tx_to, uint32_t in_pos, uint8_t hash_type, SigVersion sigversion, KeyVersion keyversion, const PrecomputedTransactionData& cache, MissingDataBehavior mdb)
 {
@@ -1689,20 +1701,7 @@ bool SignatureHashSchnorr(uint256& hash_out, ScriptExecutionData& execdata, cons
     const uint8_t output_type = (hash_type == SIGHASH_DEFAULT) ? SIGHASH_ALL : (hash_type & SIGHASH_OUTPUT_MASK); // Default (no sighash byte) is equivalent to SIGHASH_ALL
     const uint8_t input_type = hash_type & SIGHASH_INPUT_MASK;
 
-    switch(hash_type) {
-        case 0: case 1: case 2: case 3:
-        case 0x81: case 0x82: case 0x83:
-            break;
-        case 0x41: case 0x42: case 0x43:
-        case 0xc1: case 0xc2: case 0xc3:
-            if (keyversion == KeyVersion::ANYPREVOUT) {
-                break;
-            } else {
-                return false;
-            }
-        default:
-            return false;
-    }
+    if (!IsHashTypeValid(hash_type, keyversion)) return false;
     ss << hash_type;
 
     // Transaction level data

[darosior]

I'm looking at a fuzz target currently that would [...]

Done at https://github.com/darosior/bitcoin/tree/anyprevout. I've left it run for a couple days and didn't have a failure. I've collected a small initial corpus. It presents a decent coverage of SignatureHashSchnorr.

[ajtowns]

I'm looking at a fuzz target currently that would [...]

Done at https://github.com/darosior/bitcoin/tree/anyprevout. I've left it run for a couple days and didn't have a failure. I've collected a small initial corpus. It presents a decent coverage of SignatureHashSchnorr.

Added {} for the else assert and removed the unused hinted_hash_type. Though I wonder if this isn't a hint that it would be better to structure the code so that non-APO pubkeys don't come anywhere near an APO code path (along the lines of bitcoin/bips#1367).

[darosior]

Added {} for the else assert and removed the unused hinted_hash_type

Oops.. It was supposed to be used along the purely pseudo-random hashtype at the end, in an attempt to make it pass the hash type check at least once per iteration:

diff --git a/src/test/fuzz/anyprevout.cpp b/src/test/fuzz/anyprevout.cpp
index 856f27312a..eeed75e69b 100644
--- a/src/test/fuzz/anyprevout.cpp
+++ b/src/test/fuzz/anyprevout.cpp
@@ -178,9 +178,9 @@ FUZZ_TARGET(anyprevout_old_new_sighash)
                                                      cache, MissingDataBehavior::ASSERT_FAIL);
         assert(new_res == old_res);
         assert(new_sighash == old_sighash);
-        const auto new_res_h = SignatureHashSchnorr(new_sighash, exec_data, tx_to, in_pos, hash_type, sig_version,
+        const auto new_res_h = SignatureHashSchnorr(new_sighash, exec_data, tx_to, in_pos, hinted_hash_type, sig_version,
                                                     KeyVersion::TAPROOT, cache, MissingDataBehavior::ASSERT_FAIL);
-        const auto old_res_h = OldSignatureHashSchnorr(old_sighash, exec_data, tx_to, in_pos, hash_type, sig_version,
+        const auto old_res_h = OldSignatureHashSchnorr(old_sighash, exec_data, tx_to, in_pos, hinted_hash_type, sig_version,
                                                        cache, MissingDataBehavior::ASSERT_FAIL);
         assert(new_res_h == old_res_h);
         assert(new_sighash == old_sighash);

But it's not helping much actually. Which makes sense since it's only a single byte for the fuzzer to guess... I've removed the duplication of the SignatureHashSchnorr entirely (and added curly braces to the else assert in another push.

structure the code so that non-APO pubkeys don't come anywhere near an APO code path

How would you do that without duplicating the whole logic?

[ajtowns]

structure the code so that non-APO pubkeys don't come anywhere near an APO code path

How would you do that without duplicating the whole logic?

I was thinking something like this: ajtowns@af2f34d#diff-a0337ffd7259e8c7c9a7786d6dbd420c80abfa1afdb34ebae3261109d9ae3c19R1661

There's not all that much logic to actually duplicate if you remove the non-APO cases.

[darosior]

I was thinking something like this: ajtowns@af2f34d#diff-a0337ffd7259e8c7c9a7786d6dbd420c80abfa1afdb34ebae3261109d9ae3c19R1661

I think i personally prefer the current approach. But no strong preference. Others: what do you think?

[instagibbs]

weak concept ACK for... but can be persuaded either way. I like the idea of the implementation being "obviously" matching the BIP even if somehow the 118 digest diverges from older sighashes in some unexpected way.

[ajtowns]

Going to merge this as-is; note that it can be re-reviewed when a PR is opened to rebase it on top of 24.0 (or followup PRs can be proposed for this 23.0 branch too, of course)