Tapscript new opcodes by sanket1729 · Pull Request #1020 · ElementsProject/elements

sanket1729 · 2021-07-12T16:28:05Z

Please refer to the new spec here. #1019

src/crypto/sha256.cpp

src/script/interpreter.cpp

apoelstra · 2021-08-02T17:46:53Z

src/script/interpreter.cpp

+                                stack.push_back(vchAssetBlindingNonce);
+                            } else { // No issuance
+                                stack.push_back(vchFalse);
+                            }


It occurs to me that there is no way to get the actual asset ID (though maybe you can gin it up using the streaming hash opcodes..). Worth thinking about in a future softfork, I don't want to extend the scope of this one too much.

I think we can compute the asset ID with our CAT and SHA256. If not, we can add new opcodes at a later date.

apoelstra · 2021-08-02T17:51:31Z

src/script/interpreter.cpp

+                        case 0: // Version
+                        {
+                            // This does an inplicit conversion from version(int32_t) to (uint32_t)
+                            push4_le(stack, (uint32_t) checker.GetTxVersion());


spelling: implicit.

Also it's not implicit, because you have an explicit cast :)

I believe static_casts are preferred in C++.

src/script/interpreter.cpp

test/functional/feature_tapscript_opcodes.py

src/script/interpreter.cpp

src/script/script.cpp

src/script/interpreter.cpp

src/script/script_error.cpp

src/script/interpreter.cpp

sanket1729

Removed the WIP from title. Ready for another review round.

src/script/interpreter.cpp

roconnor-blockstream · 2021-08-27T12:45:12Z

src/script/interpreter.cpp

 }

+// Check the script has sufficient sigops budget for checksig(crypto) operation
+inline bool udpate_validation_weight(ScriptExecutionData& execdata, ScriptError* serror)


s/udpate/update/

roconnor-blockstream · 2021-08-27T12:48:36Z

src/script/interpreter.cpp

+                    if (!pk.IsCompressed() || !res.IsCompressed())
+                        return set_error(serror, SCRIPT_ERR_PUBKEYTYPE);
+
+                    if(!udpate_validation_weight(execdata, serror)) return false; // serror is set


s/if(/if (/

And elsewhere.

roconnor-blockstream · 2021-08-27T12:56:49Z

src/pubkey.cpp

+    std::pair<XOnlyPubKey, bool> ret;
+    secp256k1_xonly_pubkey out_xonly;
+    if (!secp256k1_xonly_pubkey_from_pubkey(secp256k1_context_verify, &out_xonly, &parity, &out)) return nullopt;
+    secp256k1_xonly_pubkey_serialize(secp256k1_context_verify, ret.first.begin(), &out_xonly);


Prefer ret.first.data() over ret.first.begin() for grabbing a value that is morally a pointer rather than morally an iterator.

Unfortunately, upstream only has

const unsigned char* data() const { return m_keydata.begin(); }

and not

unsigned char* data() { return m_keydata.begin(); }

Adding the function in this commit

roconnor-blockstream · 2021-08-27T12:58:34Z

src/pubkey.h

 #include <uint256.h>

 #include <stdexcept>
+#include <cstring>


What is cstring used for?

bitcoin/bitcoin#22051 (1/9)

That results in a much safer interface (making the tweak commit to the key implicitly using a fixed tag means it can't be used for unrelated tweaking). bitcoin/bitcoin#22051 (5/9) We actually preserve the "unrelated tweaking" method so we can use it in OP_TWEAKVERIFY

bitcoin/bitcoin#22051 (6/9) Modified to use the old-style Optional rather than std::optional

roconnor-blockstream · 2021-08-27T21:31:12Z

src/pubkey.cpp

@@ -188,6 +188,18 @@ bool XOnlyPubKey::CheckPayToContract(const XOnlyPubKey& base, const uint256& has
    return secp256k1_xonly_pubkey_tweak_add_check(secp256k1_context_verify, m_keydata.begin(), parity, &base_point, hash.begin());


shakes my fist at upstream

roconnor-blockstream · 2021-08-27T21:39:32Z

I can no longer find anything worth complaining about.

apoelstra · 2021-08-30T15:56:52Z

ACK 1577ed4

Messy merge conflicts because this PR backported some ad-hoc stuff from upstream while keeping a few things that upstream deleted. Hopefully reviewing is easier than doing this in the first place, since ultimately all I did was delete code from one side or another of the conflicts. (Ok, I also changed some boost optional stuff to std::optional, and had to patch up a test file for test framework changes.) When reviewing the detailed crypto, bear in mind that taptweaks, like all hashes are the kind of crypto that cannot be subtly wrong -- it will either fail very hard or be correct. And we have independent implementations in C++ and Python that cross-check each other, si it's unlikely to be the former.

sanket1729 force-pushed the opcodes branch from 5732816 to 36528b2 Compare July 16, 2021 14:13

sanket1729 force-pushed the opcodes branch 2 times, most recently from fc24a94 to 3e389b8 Compare August 2, 2021 16:22