| --- a/src/script/interpreter.cpp | |
| +++ b/src/script/interpreter.cpp | |
| @@ -504,6 +504,14 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript& | |
| return set_error(serror, SCRIPT_ERR_MINIMALDATA); | |
| } | |
| stack.push_back(vchPushValue); | |
| + if ((flags & SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS) && opcode == OP_FALSE) { | |
| + auto pc_tmp = pc; | |
| + opcodetype next_opcode; | |
| + valtype dummy_data; | |
| + if (script.GetOp(pc_tmp, next_opcode, dummy_data) && next_opcode == OP_IF) { | |
| + return set_error(serror, SCRIPT_ERR_DISCOURAGE_UPGRADABLE_NOPS); | |
| + } | |
| + } | |
| } else if (fExec || (OP_IF <= opcode && opcode <= OP_ENDIF)) | |
| switch (opcode) | |
| { |
I applied your patch to latest bitcoin core (no conflicts), and it did detect one transaction. But I don't know if it's an instance of the intended kind of transaction. I knew your code detected it because I set a breakpoint. Here's the stack backtrace (in case this is at all helpful):
#0 EvalScript (stack=std::vector of length 2, capacity 2 = {...}, script=..., flags=2097119, checker=..., sigversion=SigVersion::TAPSCRIPT, execdata=..., serror=0x7fff0ebd6f9c) at script/interpreter.cpp:488
#1 0x000055555611973e in ExecuteWitnessScript (stack_span=..., exec_script=..., flags=2097119, sigversion=SigVersion::TAPSCRIPT, checker=..., execdata=..., serror=0x7fff0ebd6f9c) at script/interpreter.cpp:1830
#2 0x000055555611a700 in VerifyWitnessProgram (witness=..., witversion=1, program=std::vector of length 32, capacity 32 = {...}, flags=2097119, checker=..., serror=0x7fff0ebd6f9c, is_p2sh=false) at script/interpreter.cpp:1944
#3 0x000055555611aae8 in VerifyScript (scriptSig=..., scriptPubKey=..., witness=0x7fff05467b30, flags=2097119, checker=..., serror=0x7fff0ebd6f9c) at script/interpreter.cpp:2001
#4 0x0000555555a90cd7 in CScriptCheck::operator() (this=0x7fff0ebd6f60) at ./src/validation.cpp:1645
#5 0x0000555555a914a3 in CheckInputScripts (tx=..., state=..., inputs=..., flags=2097119, cacheSigStore=true, cacheFullScriptStore=false, txdata=..., pvChecks=0x0) at ./src/validation.cpp:1740
#6 0x0000555555a8ab1f in (anonymous namespace)::MemPoolAccept::PolicyScriptChecks (this=0x7fff0ebd7760, args=..., ws=...) at ./src/validation.cpp:992
#7 0x0000555555a8c7d6 in (anonymous namespace)::MemPoolAccept::AcceptSingleTransaction (this=0x7fff0ebd7760, ptx=std::shared_ptr<const CTransaction> (use count 2, weak count 0) = {...}, args=...) at ./src/validation.cpp:1175
#8 0x0000555555a8ed9d in AcceptToMemoryPool (active_chainstate=..., tx=std::shared_ptr<const CTransaction> (use count 2, weak count 0) = {...}, accept_time=1675367849, bypass_limits=false, test_accept=false) at ./src/validation.cpp:1417
#9 0x0000555555aa24f0 in ChainstateManager::ProcessTransaction (this=0x5555568aad40, tx=std::shared_ptr<const CTransaction> (use count 2, weak count 0) = {...}, test_accept=false) at ./src/validation.cpp:3933
#10 0x0000555555694c6c in (anonymous namespace)::PeerManagerImpl::ProcessMessage (this=0x555563f49ea0, pfrom=..., msg_type=\"tx\", vRecv=..., time_received=..., interruptMsgProc=std::atomic<bool> = { false }) at ./src/net_processing.cpp:4045
#11 0x000055555569d9e1 in (anonymous namespace)::PeerManagerImpl::ProcessMessages (this=0x555563f49ea0, pfrom=0x7ffef0047700, interruptMsgProc=std::atomic<bool> = { false }) at ./src/net_processing.cpp:4926
#12 0x0000555555622b69 in CConnman::ThreadMessageHandler (this=0x555556892630) at ./src/net.cpp:2015
#13 0x00005555556259c4 in operator() (__closure=0x7fff0ebdba80) at ./src/net.cpp:2357
#14 0x0000555555635060 in std::__invoke_impl<void, CConnman::Start(CScheduler&, const CConnman::Options&)::<lambda()>&>(std::__invoke_other, struct {...} &) (__f=...) at /usr/include/c++/11/bits/invoke.h:61
#15 0x00005555556344e5 in std::__invoke_r<void, CConnman::Start(CScheduler&, const CConnman::Options&)::<lambda()>&>(struct {...} &) (__fn=...) at /usr/include/c++/11/bits/invoke.h:111
#16 0x0000555555633a98 in std::_Function_handler<void(), CConnman::Start(CScheduler&, const CConnman::Options&)::<lambda()> >::_M_invoke(const std::_Any_data &) (__functor=...) at /usr/include/c++/11/bits/std_function.h:290
#17 0x00005555557c6d39 in std::function<void ()>::operator()() const (this=0x7fff0ebdba80) at /usr/include/c++/11/bits/std_function.h:590
#18 0x00005555560a7b10 in util::TraceThread(std::basic_string_view<char, std::char_traits<char> >, std::function<void ()>) (thread_name=\"msghand\", thread_func=...) at util/thread.cpp:21
#19 0x00005555556328d1 in std::__invoke_impl<void, void (*)(std::basic_string_view<char>, std::function<void()>), char const*, CConnman::Start(CScheduler&, const CConnman::Options&)::<lambda()> >(std::__invoke_other, void (*&&)(std::basic_string_view<char, std::char_traits<char> >, std::function<void()>)) (__f=@0x5555612cfcf8: 0x5555560a7998 <util::TraceThread(std::basic_string_view<char, std::char_traits<char> >, std::function<void ()>)>) at /usr/include/c++/11/bits/invoke.h:61
#20 0x0000555555631ff8 in std::__invoke<void (*)(std::basic_string_view<char>, std::function<void()>), char const*, CConnman::Start(CScheduler&, const CConnman::Options&)::<lambda()> >(void (*&&)(std::basic_string_view<char, std::char_traits<char> >, std::function<void()>)) (__fn=@0x5555612cfcf8: 0x5555560a7998 <util::TraceThread(std::basic_string_view<char, std::char_traits<char> >, std::function<void ()>)>) at /usr/include/c++/11/bits/invoke.h:96
#21 0x0000555555631ac8 in std::thread::_Invoker<std::tuple<void (*)(std::basic_string_view<char, std::char_traits<char> >, std::function<void()>), char const*, CConnman::Start(CScheduler&, const CConnman::Options&)::<lambda()> > >::_M_invoke<0, 1, 2>(std::_Index_tuple<0, 1, 2>) (this=0x5555612cfce8) at /usr/include/c++/11/bits/std_thread.h:253
#22 0x00005555556318bf in std::thread::_Invoker<std::tuple<void (*)(std::basic_string_view<char, std::char_traits<char> >, std::function<void()>), char const*, CConnman::Start(CScheduler&, const CConnman::Options&)::<lambda()> > >::operator()(void) (this=0x5555612cfce8) at /usr/include/c++/11/bits/std_thread.h:260
#23 0x000055555563172b in std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(std::basic_string_view<char, std::char_traits<char> >, std::function<void()>), char const*, CConnman::Start(CScheduler&, const CConnman::Options&)::<lambda()> > > >::_M_run(void) (this=0x5555612cfce0) at /usr/include/c++/11/bits/std_thread.h:211
#24 0x00007ffff7c192b3 in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6
#25 0x00007ffff78a0b43 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:442
#26 0x00007ffff7932a00 in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
With your patch running, the node did sync through block 774628, which contains one of these (it's the record-largest transaction) without hitting the breakpoint. So that's good, it didn't reject the block.
If you up to the AcceptToMemoryPool frame, I think you can print tx->GetHash().ToString().c_str() or something to see the txid involved; then later, see if your node accepted or rejected it (debug.log or getrawtransaction). Another possibility for testing would be to find a tx and try feeding it into sendrawtransaction.
Thanks, I did the print you suggested, and here some transactions that your patch detected (I could get more if needed):
https://mempool.space/tx/a489ac6c63341f79e748a07d32a7abbbff902440b8e26eafabbdcd1619887a7a
https://mempool.space/tx/1991330ee1c127e1c6c1c24cadc2b47604a4e7d9d67c6606f5eecd767e3be815
https://mempool.space/tx/86539902e20780b1ed39d9db96f42b076260a8e4294505b5ec667a7eb7577413
getrawtransaction doesn't find any of these.
sendrawtransaction on txid a489... errors with "Argument list too long". (They're all pretty large, so I don't think the others will work either). The hex is about 1.8 MB. Seems like there should be a way to pass the hex on stdin, maybe I'll make a PR.
Maybe use the Python console w/ a JSON-RPC lib to make the requests?
Good idea, that worked, generated the following error:
bitcoinrpc.authproxy.JSONRPCException: -26: non-mandatory-script-verify-flag (NOPx reserved for soft-fork upgrades)
Here's the program I used:
#!/usr/bin/env python3
from bitcoinrpc.authproxy import AuthServiceProxy
import json
api = AuthServiceProxy("http://lmr:lmr@127.0.0.1:8332")
with open('rawtx', 'r') as file:
tx = file.read()
# remove the trailing newline
tx = tx[:-1]
api.sendrawtransaction(tx)
(the file rawtx contains the hex of the ordinals transaction, which I grabbed from the mempool.space explorer)
Nothing appears in debug.log when I do this, even with all logging categories enabled.
FWIW, here's another version that works for me, potentially allowing to specify more precise prefixes without having to nest GetOp calls:
diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index 03b157a847..aaabcd759f 100644
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -479,6 +479,11 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
return set_error(serror, SCRIPT_ERR_MINIMALDATA);
}
stack.push_back(vchPushValue);
+ const valtype ord_prefix{OP_FALSE, OP_IF, 0x03, 'o', 'r', 'd'};
+ if ((flags & SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS) && opcode == ord_prefix[0] &&
+ std::mismatch(ord_prefix.begin()+1, ord_prefix.end(), pc, pend).first == ord_prefix.end()) {
+ return set_error(serror, SCRIPT_ERR_DISCOURAGE_UPGRADABLE_NOPS);
+ }
} else if (fExec || (OP_IF <= opcode && opcode <= OP_ENDIF))
switch (opcode)
{
f9a3766eb22a2f84d5bd67572f2e9e4dc541d7ca016242f36f02b38aad09cf5f
These filters are an excercise in futility.
The inscription/ordinal convention can be changed faster than any Bitcoin release cadence.
Which leaves only a small subset of node runners willing to enforce new inscription/ordinal convention filters - probably having to compile in these filters on a daily basis.
Alternately - trying to make the filters more general to catch all possible patterns is beyond dangerous - it is absurd.
The inscription/ordinal convention can be changed faster than any Bitcoin release cadence.
But much slower than spam filters can be updated. And this is just a hypothetical issue; spammers typically give up quickly.
Which leaves only a small subset of node runners willing to enforce new inscription/ordinal convention filters
That doesn't follow either.
Anyone want to offer a noob advice on how to add this to my umbrel node?
Ning makinabang kareni?
What about OP_TRUE OP_NOTIF? What about a lot of other ways to push data?
1b9058c84af7c6b05a76cc495e502d424e7eb41e45ec4c6209fff8cb2f2dbed1
@garlonicon This patch is just a quick hack thrown together to address the active attack in the safest and simplest way possible. It isn't meant to be a magic solution to all possible future problems.
ff875a4a6d237aad12e6ade232ece459cf8d80067702ffcdb6b73c3f58e60db2
From e8c444d5f8891da6c87cb964b9a9a447c97d96f7 Mon Sep 17 00:00:00 2001
From: [Your Name]
Date: [Date]
Subject: [PATCH] Discourage the use of upgradable NOPs in scripts
This patch adds a new check to the EvalScript function to discourage the use of upgradable NOPs (i.e. OP_NOP, OP_NOP1, OP_NOP2, OP_NOP3, OP_NOP4, and OP_NOP5) in scripts when the SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS flag is set. Specifically, if the script contains an OP_FALSE opcode that is immediately followed by an OP_IF opcode, the function will return a SCRIPT_ERR_DISCOURAGE_UPGRADABLE_NOPS error.
The purpose of this check is to encourage script authors to use OP_CHECKLOCKTIMEVERIFY (OP_CLTV) and OP_CHECKSEQUENCEVERIFY (OP_CSV) opcodes instead of upgradable NOPs in order to ensure the security and compatibility of Bitcoin scripts.
Signed-off-by: [Your Name]
src/script/interpreter.cpp | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index 92d8f5e..f0e7b07 100644
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -504,6 +504,14 @@ bool EvalScript(std::vector<std::vector >& stack, const CScript&
return set_error(serror, SCRIPT_ERR_MINIMALDATA);
}
stack.push_back(vchPushValue);
-
if ((flags & SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS) && opcode == OP_FALSE) { -
auto pc_tmp = pc; -
opcodetype next_opcode; -
valtype dummy_data; -
if (script.GetOp(pc_tmp, next_opcode, dummy_data) && next_opcode == OP_IF) { -
return set_error(serror, SCRIPT_ERR_DISCOURAGE_UPGRADABLE_NOPS); -
} -
} } else if (fExec || (OP_IF <= opcode && opcode <= OP_ENDIF)) switch (opcode) {
--
2.17.1
Hey Im kindof a noob to the core codebase, but I'm having somewhat of a logical conflict understanding how this patch functions.
So from what I understand the inscriptions leverage a vulnerability in taproot that allows them to inscribe extra bytes. But at the top of interpreter.cpp we have // sigversion cannot be TAPROOT here, as it admits no script execution. assert(sigversion == SigVersion::BASE || sigversion == SigVersion::WITNESS_V0 || sigversion == SigVersion::TAPSCRIPT);
which confuses me, does this assertion not pretty much imply that whatever follows is not a taproot script? If so how is the patch checking for such inscriptions?
@dzyphr
SigVersion TAPROOT doesn't have a script; see the comment here and on the line following https://github.com/bitcoin/bitcoin/blob/master/src/script/interpreter.h#L192
enum class SigVersion
{
BASE = 0, //!< Bare scripts and BIP16 P2SH-wrapped redeemscripts
WITNESS_V0 = 1, //!< Witness v0 (P2WPKH and P2WSH); see BIP 141
TAPROOT = 2, //!< Witness v1 with 32-byte program, not BIP16 P2SH-wrapped, key path spending; see BIP 341
TAPSCRIPT = 3, //!< Witness v1 with 32-byte program, not BIP16 P2SH-wrapped, script path spending, leaf version 0xc0; see BIP 342
};
If you want to really dig into this, see BIP341.
Oh I see, thank you.
@luke-jr @LarryRuane I want to direct your attention to a particular ordinal transaction that looks like it may have eluded the filter.
A few days ago me and @ChrisMartl started running our nodes with debug=cmpctblock to get some visibility on the time it takes to validate the blocks. Two particular blocks (793604 and 793614) gave an interesting log. Both our nodes printed 0 txn requested, suggesting that all the required transactions to reconstruct the compact block were already in our ordinal-free mempools.
...
2023-06-09T18:34:45Z Successfully reconstructed block 00000000000000000002d8a686cb1d8e576727f03196997e052b794d85853e7e with 1 txn prefilled, 628 txn from mempool (incl at least 8 from extra pool) and 0 txn requested
2023-06-09T18:34:45Z UpdateTip: new best=00000000000000000002d8a686cb1d8e576727f03196997e052b794d85853e7e height=793604 version=0x2000c000 log2_work=94.231860 tx=849983377 date='2023-06-09T18:34:23Z' progress=1.000000 cache=6.6MiB(42573txo)
...
2023-06-09T20:23:03Z Successfully reconstructed block 000000000000000000013d4c03325c521fc9bb00efa658706df7d2e6f717624c with 1 txn prefilled, 1060 txn from mempool (incl at least 18 from extra pool) and 0 txn requested
2023-06-09T20:23:03Z UpdateTip: new best=000000000000000000013d4c03325c521fc9bb00efa658706df7d2e6f717624c height=793614 version=0x20800000 log2_work=94.231996 tx=850007009 date='2023-06-09T20:22:24Z' progress=1.000000 cache=6.0MiB(37153txo)
...
These blocks aren't empty, so on a cursory look and given the state of the average mempool this suggests that the miners who mined them were running this patch. Besides these two plus a couple of empty blocks, all other block validations requested at least a few transactions to the peers.
On closer inspect though, we found at least one transaction in block 793604 that is clearly an ordinal encoding a BRC-20 token. Therefore this suggests that the current version of the patch might be buggy on some edge case and didn't identify it correctly. Hope you find this useful.
WARNING: This has NOT been tested.Use at your own risk. If you test it, please share your results.Edit: Thanks to @LarryRuane for testing, this seems to work!