Add OP_TXHASH and OP_CHECKTXHASHVERIFY opcodes#29050
Add OP_TXHASH and OP_CHECKTXHASHVERIFY opcodes#29050stevenroose wants to merge 12 commits intobitcoin:masterfrom
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
I added a commit that has a few question marks about which contexts should have their own TxHashCache. I'm not entirely understanding which contexts have the Left some todos with questions where I was unsure. |
src/script/interpreter.cpp
Outdated
There was a problem hiding this comment.
nit: Consider adding curly braces here for clarity (since it's so close to the break)
src/script/interpreter.cpp
Outdated
There was a problem hiding this comment.
Leaving it on the stack seems inconsistent with the semantics of the other 'VERIFY' opcodes
src/script/interpreter.cpp
Outdated
There was a problem hiding this comment.
I guess there's nothing wrong with pushing pre-concatenated arguments on the stack to save bytes, but this also seems inconsistent with other opcodes. Not the most important thing in the world but I'm curious what others think
src/script/txhash.h
Outdated
src/script/bitcoinconsensus.cpp
Outdated
There was a problem hiding this comment.
A working build and green CI may help attract review. The last commit c368d32 doesn't build for me without updating the related fuzz tests.
sample diff
diff --git a/src/test/fuzz/script_assets_test_minimizer.cpp b/src/test/fuzz/script_assets_test_minimizer.cpp
index 511b581f606..7478897e170 100644
--- a/src/test/fuzz/script_assets_test_minimizer.cpp
+++ b/src/test/fuzz/script_assets_test_minimizer.cpp
@@ -7,6 +7,7 @@
#include <primitives/transaction.h>
#include <pubkey.h>
#include <script/interpreter.h>
+#include <script/txhash.h>
#include <serialize.h>
#include <streams.h>
#include <univalue.h>
@@ -159,7 +160,8 @@ void Test(const std::string& str)
tx.vin[idx].scriptWitness = ScriptWitnessFromJSON(test["success"]["witness"]);
PrecomputedTransactionData txdata;
txdata.Init(tx, std::vector<CTxOut>(prevouts));
- MutableTransactionSignatureChecker txcheck(&tx, idx, prevouts[idx].nValue, txdata, MissingDataBehavior::ASSERT_FAIL);
+ TxHashCache txhash_cache;
+ MutableTransactionSignatureChecker txcheck(&tx, idx, prevouts[idx].nValue, txdata, &txhash_cache, MissingDataBehavior::ASSERT_FAIL);
for (const auto flags : ALL_FLAGS) {
// "final": true tests are valid for all flags. Others are only valid with flags that are
// a subset of test_flags.
@@ -174,7 +176,8 @@ void Test(const std::string& str)
tx.vin[idx].scriptWitness = ScriptWitnessFromJSON(test["failure"]["witness"]);
PrecomputedTransactionData txdata;
txdata.Init(tx, std::vector<CTxOut>(prevouts));
- MutableTransactionSignatureChecker txcheck(&tx, idx, prevouts[idx].nValue, txdata, MissingDataBehavior::ASSERT_FAIL);
+ TxHashCache txhash_cache;
+ MutableTransactionSignatureChecker txcheck(&tx, idx, prevouts[idx].nValue, txdata, &txhash_cache, MissingDataBehavior::ASSERT_FAIL);
for (const auto flags : ALL_FLAGS) {
// If a test is supposed to fail with test_flags, it should also fail with any superset thereof.
if ((flags & test_flags) == test_flags) {
diff --git a/src/test/fuzz/script_flags.cpp b/src/test/fuzz/script_flags.cpp
index accb32f1cc4..d9001fd81a0 100644
--- a/src/test/fuzz/script_flags.cpp
+++ b/src/test/fuzz/script_flags.cpp
@@ -5,6 +5,7 @@
#include <consensus/amount.h>
#include <primitives/transaction.h>
#include <script/interpreter.h>
+#include <script/txhash.h>
#include <serialize.h>
#include <streams.h>
#include <test/fuzz/fuzz.h>
@@ -42,10 +43,11 @@ FUZZ_TARGET(script_flags)
}
PrecomputedTransactionData txdata;
txdata.Init(tx, std::move(spent_outputs));
+ TxHashCache txhash_cache;
for (unsigned i = 0; i < tx.vin.size(); ++i) {
const CTxOut& prevout = txdata.m_spent_outputs.at(i);
- const TransactionSignatureChecker checker{&tx, i, prevout.nValue, txdata, MissingDataBehavior::ASSERT_FAIL};
+ const TransactionSignatureChecker checker{&tx, i, prevout.nValue, txdata, &txhash_cache, MissingDataBehavior::ASSERT_FAIL};
ScriptError serror;
const bool ret = VerifyScript(tx.vin.at(i).scriptSig, prevout.scriptPubKey, &tx.vin.at(i).scriptWitness, verify_flags, checker, &serror);
diff --git a/src/test/fuzz/script_sigcache.cpp b/src/test/fuzz/script_sigcache.cpp
index 5fdbc9e1069..9cc233fed19 100644
--- a/src/test/fuzz/script_sigcache.cpp
+++ b/src/test/fuzz/script_sigcache.cpp
@@ -6,6 +6,7 @@
#include <key.h>
#include <pubkey.h>
#include <script/sigcache.h>
+#include <script/txhash.h>
#include <test/fuzz/FuzzedDataProvider.h>
#include <test/fuzz/fuzz.h>
#include <test/fuzz/util.h>
@@ -36,7 +37,8 @@ FUZZ_TARGET(script_sigcache, .init = initialize_script_sigcache)
const CAmount amount = ConsumeMoney(fuzzed_data_provider);
const bool store = fuzzed_data_provider.ConsumeBool();
PrecomputedTransactionData tx_data;
- CachingTransactionSignatureChecker caching_transaction_signature_checker{mutable_transaction ? &tx : nullptr, n_in, amount, store, tx_data};
+ TxHashCache txhash_cache;
+ CachingTransactionSignatureChecker caching_transaction_signature_checker{mutable_transaction ? &tx : nullptr, n_in, amount, store, tx_data, &txhash_cache};
if (fuzzed_data_provider.ConsumeBool()) {
const auto random_bytes = fuzzed_data_provider.ConsumeBytes<unsigned char>(64);
const XOnlyPubKey pub_key(ConsumeUInt256(fuzzed_data_provider));
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
Seems like this should be a draft |
|
⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
2 similar comments
|
⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
Closing this for now. Overall I think it'll be better open a new PR, after further discussion in bitcoin/bips#1500. |
8 participants
Implementation of OP_TXHASH and OP_CHECKTXHASHVERIFY, as per the draft BIP.
This MR includes a test using the test vectors generated from the reference implementation in the BIP.
The implementation utilizes a caching strategy that hopefully alleviates concerns around resource usage like quadratic hashing.
This MR on purpose does not make any consensus changes yet. Activation of the proposed opcodes will be coordinated in an independent MR, probably combined with other opcodes (like OP_CAT and/or OP_CHECKSIGFROMSTACK).
(I'm not very familiar with checks and lints on here, so I'll try to address problems as they come. I have personally been able to compile and run the tests, but that usually is only the tip of the iceberg.)