Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OP_CHECKSIGADD got left behind! #151

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BlobMaster41
Copy link

@BlobMaster41 BlobMaster41 commented Jun 16, 2024

OP_CHECKSIGADD got left behind!

We can not parse a transaction containing OP_CHECKSIGADD opcodes due to the following check:

if (!GetOp(it, opcode, item) || opcode > MAX_OPCODE || item.size() > MAX_SCRIPT_ELEMENT_SIZE) {
    return false;
}

@instagibbs
Copy link

Is this function ever used by something that invokes it in a tapscript context? If so, could you add a regression test?

@BlobMaster41
Copy link
Author

BlobMaster41 commented Jun 16, 2024

Is this function ever used by something that invokes it in a tapscript context? If so, could you add a regression test?

Hey, yeah so if you try the following transaction, btcdeb wont be able to decode the script itself even if the script lead to an invalid transaction (on purpose, the script itself will be reverted after stack execution):

 ./btcdeb --verbose --debug=sighash,signing,segwit,taproot --txin=020000000001018743fd257bca71380c09e09359ba37cea9e760c974866ca8982a739cff16d3020000000000fdffffff02a086010000000000160014dc0d46077a225cbc9a844750f9bacb9ee5eed4f9c027090000000000225120647e7db279052199f234014f25608084aa3c4fe032e9a149f3d5bfc822af005a0614dc0d46077a225cbc9a844750f9bacb9ee5eed4f9200373626d317ae8788ce3280b491068610d840c23ecb64c14075bbb9f670af52c40c255ea59f8d453931d5efc4ce46b98a02e9c4c34ebbe13267b5151edf036d108289820ce4d961e391e2829600d011e65667dfe8e317e81fa5dff6897e6172b0e403ee42545b1b12cec59981724289b6265c7b1e02a1e798e21bf5d9c5e4015746b1752b8360ac56c1558bf5b514ba90c6c99ea0a715d290d70473f77dae64164e7fd6901200373626d317ae8788ce3280b491068610d840c23ecb64c14075bbb9f670af52cad20f6ff14388fd7cdcd229e8194d3ec1fe2ff54e18231464fc9f6e959c16a273f3aada9146be111472cff4df4c2052061c6c5929fbded8ebd88a91457d3bd43a330a6b08397e767de16fd355a882ef98874519c6303627369004c840203e68ee236fcc34933d493b305a6f0c7561259471d13ecd10dfe0e37633a7cbe02d38ea5b0ec9e22dbb36406baa3c5bbb02708e2bd94d49297ea6f3ebd26d18c77038244c130704f1e5068b0853914e8c36ed442f90b416660ba1cfa37e77c15ccc803b64a3280d026571035bcc88a4084e4c3e2142f3546f1c74de4cc471dbf6c588e510202004f4c5e1f8b080000000000020abb932fbe3b29b9a8c4b020d5a03827db283ba7b822393b23cdd220a3acd422ddc0ccc2a2a8a2c4b2b0a4d8ac24a3b422dbb8d4a432b7223535abdcd438bdd8a0222b27238f011fe0543f0000369e5a5b6400000067516841c10373626d317ae8788ce3280b491068610d840c23ecb64c14075bbb9f670af52ce7e4d593fcb72926eedbe0d1e311f41acd6f6ef161dcba081a75168ec4dcd37900000000 --tx=0200000000010151faed5859edb61095bb0a703d806ca6d5b346ec9cd7e62b55123dbe418e4fd20100000000ffffffff01801a060000000000225120647e7db279052199f234014f25608084aa3c4fe032e9a149f3d5bfc822af005a0440bdcd6f90e0c8041cc74fad9e6e1712c2a613b658570a09e874ba47973aac8edbd698644f39d26970e064f31258510d5cee122a874ddd27750aaf47aed67093f04043f695e4512b3842c6e9d494a6354dc454a398b54a3956d05774a2e09c340680d57717df4b495b7ba74035939bf6efa4c146f9e723aa91d9cb639fb10c5828558f002003e68ee236fcc34933d493b305a6f0c7561259471d13ecd10dfe0e37633a7cbe7cba20d38ea5b0ec9e22dbb36406baa3c5bbb02708e2bd94d49297ea6f3ebd26d18c777cba208244c130704f1e5068b0853914e8c36ed442f90b416660ba1cfa37e77c15ccc87cba20b64a3280d026571035bcc88a4084e4c3e2142f3546f1c74de4cc471dbf6c588e7cba529c41c150929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac025fc3ba6e23970f193823c72924d629199c63998b05d5fb3d43d8d5553068a1c00000000

After the change, the transaction can be executed without any problems.

Here is a valid version of the transaction. This transaction includes empty witnesses (so it works with OP_CHECKSIGADD). There is an other bug in btcdeb code. You can reproduce it via the following command (I did not fix it yet.):

 ./btcdeb --verbose --debug=sighash,signing,segwit,taproot --txin=02000000000101778dd43a3dd2d24f826e9366ceb417c200488f838324fa616d81b80dcd0b38750000000000fdffffff02a086010000000000160014dc0d46077a225cbc9a844750f9bacb9ee5eed4f9c027090000000000225120b33aceb14b2d01dda0ea096d257e8c1a90af113a6ead04d3ff6f8838bfa35b960614dc0d46077a225cbc9a844750f9bacb9ee5eed4f9200373626d317ae8788ce3280b491068610d840c23ecb64c14075bbb9f670af52c407519a5740b3288119c2db2e9c4cb6f87b04719fd1ac3585d6f8a5f2657a194d2f621b60a9670fb4acb5269904b03cb2f81d4018ba8b93652255fb668d3304a3b404e13ff7eed2a1a61b2c685e81ec2b8a11cb602747df1a23fdb37fd81feadb3fd36e6ceb3e0cd7a0b7649d937e6dc9827754d370ae1fd26a78bcc72a1a9ac9b76fd6901200373626d317ae8788ce3280b491068610d840c23ecb64c14075bbb9f670af52cad201fe9c25d67a53456f5699baeee6c36d4cc179c17d339ad5532571fd0a7ee5707ada9146be111472cff4df4c2052061c6c5929fbded8ebd88a91457d3bd43a330a6b08397e767de16fd355a882ef98874519c6303627369004c840203e68ee236fcc34933d493b305a6f0c7561259471d13ecd10dfe0e37633a7cbe02d38ea5b0ec9e22dbb36406baa3c5bbb02708e2bd94d49297ea6f3ebd26d18c77038244c130704f1e5068b0853914e8c36ed442f90b416660ba1cfa37e77c15ccc803b64a3280d026571035bcc88a4084e4c3e2142f3546f1c74de4cc471dbf6c588e510202004f4c5e1f8b080000000000020abb932fbe3b29b9a8c4b020d5a03827db283ba7b822393b23cdd220a3acd422ddc0ccc2a2a8a2c4b2b0a4d8ac24a3b422dbb8d4a432b7223535abdcd438bdd8a0222b27238f011fe0543f0000369e5a5b6400000067516841c00373626d317ae8788ce3280b491068610d840c23ecb64c14075bbb9f670af52ce7e4d593fcb72926eedbe0d1e311f41acd6f6ef161dcba081a75168ec4dcd37900000000 --tx=020000000001028c056958c0c00d26f5f4488482ea3615f1a92c50acfcdedb63de48a0ccb777760000000000ffffffff8b8fd26d7a23027064a143897e4aafd043000bb72bee0a1f550de409c7660cfe0100000000ffffffff031027000000000000160014dc0d46077a225cbc9a844750f9bacb9ee5eed4f9801a060000000000225120b33aceb14b2d01dda0ea096d257e8c1a90af113a6ead04d3ff6f8838bfa35b96400d030000000000225120cbe1fb2adf81b16ba4afbb38743f4738ccb28170d2efc35a3ca9366ce64ea4510614dc0d46077a225cbc9a844750f9bacb9ee5eed4f9200373626d317ae8788ce3280b491068610d840c23ecb64c14075bbb9f670af52c4082e4d4fdcd1523eb51b14685bd97f7aec94c5f77644332ae8948bda8a77b28afb66d3afe3ace4899457255571ea65e43da0bd69b68717edcfb0a2d8c2c916f2d40353e14c6d0bd042c618c5fade578e890bd7557bd196ece296be8b6b8fb0c195aa23f5fbeb4557b90c0d75319197be027cbd5a71b34dc512daa811ba0223cbc569d200373626d317ae8788ce3280b491068610d840c23ecb64c14075bbb9f670af52cad2003366d50eab1c272c84347fc5a70d2d01130439a8c188a8603be86c9857457b8ada9146be111472cff4df4c2052061c6c5929fbded8ebd88a91457d3bd43a330a6b08397e767de16fd355a882ef98874519c63036273694f1e1f8b080000000000020a6b9dbfe628033ec0cceb00004ff406612400000067516841c050929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac0e7e4d593fcb72926eedbe0d1e311f41acd6f6ef161dcba081a75168ec4dcd3790640fd78ae71afe6b2fefa20f3fef4292c3f8f59e19f4e74d72faa4010e46c37d526337eff825a6a941c978fa33f82a823bba5fb0afca340f955f261980622c75163000040730a2a4486d962f6ab45599e441e4b0a38a9746398d717f3b0f6ebaef2ad7a1419d71b9474dba4ebb187a20a0f57883ccafae5767c749523d8da5626266ead708b002003e68ee236fcc34933d493b305a6f0c7561259471d13ecd10dfe0e37633a7cbeba20d38ea5b0ec9e22dbb36406baa3c5bbb02708e2bd94d49297ea6f3ebd26d18c77ba208244c130704f1e5068b0853914e8c36ed442f90b416660ba1cfa37e77c15ccc8ba20b64a3280d026571035bcc88a4084e4c3e2142f3546f1c74de4cc471dbf6c588eba529c41c050929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac025fc3ba6e23970f193823c72924d629199c63998b05d5fb3d43d8d5553068a1c00000000

Currently this valid transaction error with the following:
invalid script (witness stack last element)

The error is due to an invalid opcode being decoded: 32765. I would have to debug the code futher to find the cause of that bug.

As mentioned in BIP 342, this should be possible: https://en.bitcoin.it/wiki/BIP_0342

Potentially, we should add a check to verify that the transaction is taproot, if its not taproot, this opcode is not valid. This opcode is only usable for tapscript transactions.

Here the transaction on my local testnet:
https://mempool.opnet.org/tx/94546b3f6c338c2ab4c23c42f93828dfe23f9a686359564c4ba23d0dfcea3784

@nghuyenthevinh2000
Copy link

thanks man, this works like a charm, so far so good when I am using OP_CHECKSIGADD with tap

@darosior
Copy link
Member

CHECKSIGADD is only a valid opcode in Tapscript, not legacy Bitcoin Script. This is on purpose as MAX_OPCODE is only ever used in parsing legacy Script. So either you are trying to use CHECKSIGADD in legacy Script context (in a scriptSig or a scriptPubKey) or btcdeb extended the usage of parsing functions making use of MAX_OPCODE beyond what Bitcoin Core uses them for.

@BlobMaster41
Copy link
Author

No. Try it in a tapscript like the example provided. Without this, it does NOT work inside tapscripts. Try it yourself. If this is not the intended behavior, you have a major issue somewhere.

@ajtowns
Copy link

ajtowns commented Dec 13, 2024

HasValidOps is for legacy bitcoin script (scriptPubKey, scriptSig, witness version 0), it's not correct for tapscript and shouldn't be used for it. Something along the lines of this might be better:

--- a/btcdeb.cpp
+++ b/btcdeb.cpp
@@ -242,9 +242,9 @@ int main(int argc, char* const* argv)
         }
     }

-    CScript script;
     if (script_str) {
-        if (instance.parse_script(script_str)) {
+        int witprogver = ((flags & SCRIPT_VERIFY_TAPROOT) != 0 ? 1 : 0);
+        if (instance.parse_script(witprogver, script_str)) {
             if (verbose) btc_logf("valid script\n");
         } else {
             fprintf(stderr, "invalid script\n");

--- a/instance.cpp
+++ b/instance.cpp
@@ -93,7 +93,7 @@ bool Instance::parse_input_transaction(const char* txdata, int select_index) {
     return true;
 }

-bool Instance::parse_script(const char* script_str) {
+bool Instance::parse_script(int witprogver, const char* script_str) {
     std::vector<unsigned char> scriptData = Value(script_str).data_value();
     script = CScript(scriptData.begin(), scriptData.end());
     // for (const auto& keymap : COMPILER_CTX.keymap) {
@@ -110,12 +110,12 @@ bool Instance::parse_script(const char* script_str) {
     //     printf("miniscript failed to parse script; miniscript support disabled\n");
     //     msenv = nullptr;
     // }
-    return script.HasValidOps();
+    return witprogver != 0 || script.HasValidOps();
 }

-bool Instance::parse_script(const std::vector<uint8_t>& script_data) {
+bool Instance::parse_script(int witprogver, const std::vector<uint8_t>& script_data) {
     script = CScript(script_data.begin(), script_data.end());
-    return script.HasValidOps();
+    return witprogver != 0 || script.HasValidOps();
 }

 bool Instance::parse_pretend_valid_expr(const char* expr) {
@@ -545,7 +545,7 @@ bool Instance::configure_tx_txin() {
             }
         } else assert(!"should never get here; was a new witprogver added?");

-        if (parse_script(std::vector<uint8_t>(validation.begin(), validation.end()))) {
+        if (parse_script(witprogver, std::vector<uint8_t>(validation.begin(), validation.end()))) {
             btc_logf("valid script\n");
         } else {
             fprintf(stderr, "invalid script (witness stack last element)\n");

--- a/instance.h
+++ b/instance.h
@@ -68,8 +68,8 @@ public:
     bool parse_transaction(const char* txdata, bool parse_amounts = false);
     bool parse_input_transaction(const char* txdata, int select_index = -1);

-    bool parse_script(const char* script_str);
-    bool parse_script(const std::vector<uint8_t>& script_data);
+    bool parse_script(int witprogver, const char* script_str);
+    bool parse_script(int witprogver, const std::vector<uint8_t>& script_data);

     void parse_stack_args(size_t argc, char* const* argv, size_t starting_index);
     void parse_stack_args(const std::vector<const char*> args);
diff --git a/tap.cpp b/tap.cpp
index 991571c..4e01d8b 100644
--- a/tap.cpp
+++ b/tap.cpp
@@ -265,7 +265,7 @@ int main(int argc, char* const* argv)
     for (size_t i = 0; i < script_count; ++i) {
         Item scriptData = Value(ca.l[2 + i]).data_value();
         CScript script = CScript(scriptData.begin(), scriptData.end());
-        if (!script.HasValidOps()) {
+        if (!is_tapscript && !script.HasValidOps()) {
             abort("invalid script #%zu: %s", i, HEXC(scriptData));
         }
         if (!quiet) {

(test/signing.cpp also needs to be parse_script(0, SCRIPT); instead of parse_script(SCRIPT) in various places)

See also bitcoin/bitcoin#22338

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants