Skip to content

Instantly share code, notes, and snippets.

@luke-jr
Created February 1, 2023 20:05
  • Star 17 You must be signed in to star a gist
  • Fork 7 You must be signed in to fork a gist
Star You must be signed in to star a gist
Save luke-jr/4c022839584020444915c84bdd825831 to your computer and use it in GitHub Desktop.
--- 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)
{
@LarryRuane
Copy link

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.

@luke-jr
Copy link
Author

luke-jr commented Feb 3, 2023

Maybe use the Python console w/ a JSON-RPC lib to make the requests?

@LarryRuane
Copy link

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.

@theStack
Copy link

theStack commented Feb 5, 2023

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)
             {

@peterzion45
Copy link

f9a3766eb22a2f84d5bd67572f2e9e4dc541d7ca016242f36f02b38aad09cf5f

@RandyMcMillan
Copy link

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.

@luke-jr
Copy link
Author

luke-jr commented Feb 8, 2023

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.

@VegArchie
Copy link

Anyone want to offer a noob advice on how to add this to my umbrel node?

@LarryRuane
Copy link

LarryRuane commented Feb 11, 2023

@peterzion45
Copy link

Ning makinabang kareni?

@garlonicon
Copy link

What about OP_TRUE OP_NOTIF? What about a lot of other ways to push data?

@peterzion45
Copy link

1b9058c84af7c6b05a76cc495e502d424e7eb41e45ec4c6209fff8cb2f2dbed1

@luke-jr
Copy link
Author

luke-jr commented Feb 17, 2023

@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.

@peterzion45
Copy link

ff875a4a6d237aad12e6ade232ece459cf8d80067702ffcdb6b73c3f58e60db2

@Yugocana
Copy link

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

@dzyphr
Copy link

dzyphr commented May 7, 2023

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?

@LarryRuane
Copy link

@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.

@dzyphr
Copy link

dzyphr commented May 8, 2023

Oh I see, thank you.

@1ma
Copy link

1ma commented Jun 14, 2023

@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.

@cculianu
Copy link

cculianu commented Nov 20, 2023

Hi guys -- I made a patch -- which is much larger but also adds -ordislow=1 , which delays relay of newly-arrived blocks if they contain known-ordinal txns. Basically the node refuses to relay the block -- doesn't even announce the cmpctblock to peers -- for a time (while it's the tip, basically) if it contains known-ordinal txns. The way it can decide this without actually evaluating the block is to rely on the fact that it contains txns it had previously rejected due to ordinals. It uses a rolling bloom filter to maintain a set of 1 million txids it has rejected in the past (with false positive probability 10^-7 , for total space use of ~12MB for the set).

In the process of still testing it, but it's here, as a patch against Core v26.0rc2.

PR (to my own repo, heh): cculianu/bitcoin#1
Actual branch: https://github.com/cculianu/bitcoin/tree/ordislow_26.0rc2

Patch in-action, with -debug=ordislow -debug=cmpctblock:
Screenshot 2023-11-20 at 11 10 43 AM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment