It's no secret that I'm strongly opposed to this consensus change (trying to punish everyone just to send a signal to those we disagree with doesn't resonate with me).
But regardless of my views on it, I think it's important to disclose a serious consensus-critical bug in the current BIP-110 implementation, since it could harm users who choose to run this software even more.
The new restrictions here don't apply to UTXOs created before the activation height. This is tracked via per-input flags that relax the mandatory verify flags for old UTXOs.
The tx-wide script-execution cache handling was modified in this PR, but the cache key is still computed from the strict global flags only, and the result is cached under that strict key. This means that a tx that only passes with relaxed flags gets cached as if it passed strict validation. This introduces a cache poisoning consensus bug that can be triggered via a reorg across the activation boundary in the following way:
- A funding tx is confirmed before the activation height. After activation, it is spent with a rule-violating data push;
- Since the funding UTXO is exempt, the spend passes validation and its script result is cached;
- A single-block reorg at the activation boundary moves the funding tx from the last pre-activation block to a post-activation block (note that the spending tx is after the activation height in both cases);
- See https://github.com/bitcoin-data/stale-blocks/blob/master/stale-blocks.csv for real-world examples of such stale blocks;
- The same spend is now invalid (the funding UTXO is no longer exempt), but the cache still says it passed under strict flags;
- Since the spend tx script validity was accidentally cached before, the block is deemed valid and accepted;
- Other nodes that didn't experience the reorg would reject the block under reduced_data rules.
This behavior directly contradicts the warning comment (https://github.com/bitcoinknots/bitcoin/pull/238/changes/6c19d6703616a706bb2135d7afc575fe61b82b22#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R2409-R2412):
* WARNING: flags_per_input deviations from flags must be handled with care. Under no
* circumstances should they allow a script to pass that might not pass with the same
* `flags` parameter (which is used for the cache).The problem stems from the fact that the relaxed flags only apply to validation but not to caching (https://github.com/bitcoinknots/bitcoin/pull/238/changes/6c19d6703616a706bb2135d7afc575fe61b82b22#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R2446)
hasher.Write(UCharCast(tx.GetWitnessHash().begin()), 32).Write((unsigned char*)&flags, sizeof(flags)).Finalize(hashCacheEntry.begin());before the flags_per_input values are even considered (https://github.com/bitcoinknots/bitcoin/pull/238/changes/6c19d6703616a706bb2135d7afc575fe61b82b22#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R2468):
if (!flags_per_input.empty()) flags = flags_per_input[i];and the cache ends up being written with the strict global flag key (https://github.com/bitcoinknots/bitcoin/pull/238/changes/6c19d6703616a706bb2135d7afc575fe61b82b22#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R2495-R2499)
if (cacheFullScriptStore && !pvChecks) {
// We executed all of the provided scripts, and were told to
// cache the result. Do so now.
validation_cache.m_script_execution_cache.insert(hashCacheEntry);
}The bug can be reproduced by a simple unit test:
BOOST_FIXTURE_TEST_CASE(checkinputs_flags_per_input_cache_safety, Dersig100Setup)
{
// BIP110 cache-safety reproducer:
// A 300-byte witness push passes only when SCRIPT_VERIFY_REDUCED_DATA is relaxed.
const auto& coinbase_script{m_coinbase_txns[0]->vout[0].scriptPubKey};
const unsigned int strict_flags{SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_REDUCED_DATA};
const unsigned int relaxed_flags{SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS};
const CScript witness_script = CScript() << OP_DROP << OP_TRUE;
const std::vector<unsigned char> big_witness_elem(300, 0x42);
const CScript p2wsh_script = GetScriptForDestination(WitnessV0ScriptHash(witness_script));
const auto mine_funding_tx{[&]
{
CMutableTransaction tx;
tx.vin = {CTxIn{m_coinbase_txns[0]->GetHash(), 0}};
tx.vout = {CTxOut{11 * CENT, p2wsh_script}};
std::vector<unsigned char> vchSig;
const uint256 hash = SignatureHash(coinbase_script, tx, 0, SIGHASH_ALL, 0, SigVersion::BASE);
BOOST_CHECK(coinbaseKey.Sign(hash, vchSig));
vchSig.push_back(SIGHASH_ALL);
tx.vin[0].scriptSig << vchSig;
const CBlock block = CreateAndProcessBlock({tx}, coinbase_script);
LOCK(cs_main);
BOOST_CHECK(m_node.chainman->ActiveChain().Tip()->GetBlockHash() == block.GetHash());
return CTransaction{tx};
}};
const CTransaction funding_tx{mine_funding_tx()};
// Build spending transaction with witness stack [big_witness_elem, witness_script].
CMutableTransaction spend_tx;
spend_tx.vin = {CTxIn{funding_tx.GetHash(), 0}};
spend_tx.vout = {CTxOut{10 * CENT, GetScriptForDestination(PKHash(coinbaseKey.GetPubKey()))}};
spend_tx.vin[0].scriptWitness.stack = {big_witness_elem, {witness_script.begin(), witness_script.end()}};
const CTransaction spend{spend_tx};
BOOST_CHECK_EQUAL(spend.vin[0].scriptWitness.stack[0].size(), 300U);
LOCK(cs_main);
auto& coins_tip = m_node.chainman->ActiveChainstate().CoinsTip();
// Use a fresh cache to avoid unrelated pre-population or (very unlikely) false positives.
ValidationCache validation_cache{/*script_execution_cache_bytes=*/1 << 20, /*signature_cache_bytes=*/1 << 20};
const auto run_check{[&](const std::vector<unsigned int>& flags_per_input) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
TxValidationState state;
PrecomputedTransactionData txdata;
return CheckInputScripts(spend, state, &coins_tip, strict_flags, /*cacheSigStore=*/true, /*cacheFullScriptStore=*/true, txdata, validation_cache, /*pvChecks=*/nullptr, flags_per_input);
}};
// Step 1: strict validation (BIP110 active) should fail.
BOOST_CHECK(!run_check({}));
// Step 2: relaxed per-input flags (no REDUCED_DATA) should pass.
BOOST_CHECK(run_check({relaxed_flags}));
// Step 3: strict validation must still fail.
// Before the cache fix, step 2 could poison the strict cache key and make this pass.
BOOST_CHECK(!run_check({}));
}which fails with:
test/txvalidationcache_tests.cpp:447: error: in "txvalidationcache_tests/checkinputs_flags_per_input_cache_safety": check !run_check({}) has failed
indicating that the cache was poisoned and the tx passed strict validation while it clearly failed at the same height before.
We can also reproduce it with a higher-level functional test:
# ======================================================================
# Test 8: cache state must not survive activation-boundary reorg
# ======================================================================
self.log.info("Test 8: script-execution cache must not survive boundary-context flip")
def rewind_to(height):
# Height-based loop: invalidating one tip can switch to an alternate branch at same height.
while node.getblockcount() > height:
node.invalidateblock(node.getbestblockhash())
assert_equal(node.getblockcount(), height)
branch_point = ACTIVATION_HEIGHT - 2 # 430
rewind_to(branch_point)
# spend_tx has a 300-byte witness element: valid only with pre-activation exemption.
funding_tx, spend_tx = self.create_p2wsh_funding_and_spending_tx(wallet, node, VIOLATION_SIZE)
# Branch A: funding at 431 (exempt).
block = self.create_test_block([funding_tx], signal=False)
assert_equal(node.submitblock(block.serialize().hex()), None)
assert_equal(node.getblockcount(), ACTIVATION_HEIGHT - 1)
self.restart_node(0, extra_args=['-vbparams=reduced_data:0:999999999999:288:2147483647:2147483647', '-par=1']) # Use single-threaded validation to maximize chance of hitting cache-related issues.
# Validate-only block at height 432. This calls TestBlockValidity(fJustCheck=true),
# which populates the tx-wide script-execution cache under STRICT flags, even though
# the spend is only valid here due to the per-input "pre-activation UTXO" exemption.
self.generateblock(node, output=wallet.get_address(), transactions=[spend_tx.serialize().hex()], submit=False, sync_fun=self.no_op)
assert_equal(node.getblockcount(), ACTIVATION_HEIGHT - 1)
# Reorg to branch point; cache state is intentionally retained across reorg.
rewind_to(branch_point)
# Branch B: funding at 432 (non-exempt).
# Make this empty block unique to avoid duplicate-invalid when rebuilding branch B.
block = self.create_test_block([], signal=False)
block.nTime += 1
block.solve()
assert_equal(node.submitblock(block.serialize().hex()), None) # 431
block = self.create_test_block([funding_tx], signal=False)
assert_equal(node.submitblock(block.serialize().hex()), None) # 432
# Same spend is now non-exempt and must be rejected.
attack_block = self.create_test_block([spend_tx], signal=False) # 433
result = node.submitblock(attack_block.serialize().hex())
assert result is not None and 'Push value size limit exceeded' in result, \
f"Expected rejection after boundary-crossing reorg, got: {result}"which fails with:
AssertionError: Expected rejection after boundary-crossing reorg, got: None
And for those still in disbelief, the bug can be reproduced with a manual cli reorg test locally with regtest:
#!/usr/bin/env bash
killall bitcoind >/dev/null 2>&1 || true
set -euo pipefail
# BIP110 reduced_data script-exec cache poisoning demo (PR #238):
#
# Rule: Inputs spending UTXOs created before activation (h<432) are exempt from
# the new reduced_data limits. Implementation uses per-input script flags to
# relax checks for those inputs.
#
# Bug: Script-exec cache entries are keyed by tx-wide (witness hash + STRICT
# flags), but the tx may have only passed because some inputs were checked with
# RELAXED per-input flags. This "harmless lie" becomes harmful if a reorg moves
# the *funding tx* across the activation boundary:
# * Chain A: fund at h=431 (<432, exempt) -> spend after activation is valid
# * Reorg: fund at h=432 (>=432, strict) -> same spend becomes invalid
# If the cache says "already validated", script checks can be skipped and the
# now-invalid spend can be accepted.
#
# Demo output: the same spend at the same height (433) is REJECTED (control),
# then after validate-only + reorg it's ACCEPTED (BUG).
# Comment out step [5/6] to see step [6/6] reject again.
BITCOIND=${BITCOIND:-build/bin/bitcoind}; CLI=${CLI:-build/bin/bitcoin-cli}; TX=${TX:-build/bin/bitcoin-tx}
DATADIR="$(mktemp -d "${TMPDIR:-/tmp}/bip110-cache.XXXXXX")"; WALLET=w
cli() { "$CLI" -regtest -datadir="$DATADIR" -rpcwait "$@"; }
w() { cli -rpcwallet="$WALLET" "$@"; }
step() { printf '%s\n' "$*"; }
log() { printf ' [h=%s] %s\n' "$(cli getblockcount 2>/dev/null || echo '?')" "$*"; }
logh() { local h="$1"; shift; printf ' [h=%s] %s\n' "$h" "$*"; }
RST=$'\033[0m'
tid() { # deterministically colorize 4-hex prefixes so different txs stand out
local p="$1" b c; b=$((16#${p:0:2}))
local colors=(31 32 33 34 35 36 91 92 93 94 95 96); c="${colors[$((b % ${#colors[@]}))]}"
printf '\033[%sm%s%s' "$c" "$p" "$RST"
}
ACC="ACCEPTED"; REJ="REJECTED"; OK="👍"; BUG="👎"
j() { sed -nE "s/.*\"$1\"[[:space:]]*:[[:space:]]*\"?([^\",}]*)\"?.*/\\1/p" | head -n1; }
revhex() { echo "$1" | sed -E 's/(..)/\1 /g' | awk '{for (i=NF;i>=1;i--) printf $i; print ""}'; }
le() { local w="$1" v="$2"; revhex "$(printf "%0${w}x" "$v")"; }
rej_reason() { tr '\n' ' ' | sed -E 's/.*TestBlockValidity failed: ([^,"]*).*/\1/'; }
cleanup() { cli stop >/dev/null 2>&1 || true; rm -rf "$DATADIR"; }
trap cleanup EXIT
step "[1/6] start bitcoind"
# -par=1 => no script-check worker threads (enables script-exec cache write in TestBlockValidity)
"$BITCOIND" -regtest -daemon -datadir="$DATADIR" -fallbackfee=0.0001 -par=1 \
-vbparams=reduced_data:0:999999999999:288:2147483647:2147483647 >/dev/null # reduced_data BIP9 params; activates at h=432 on regtest (144-block periods)
cli getblockcount >/dev/null
cli createwallet "$WALLET" >/dev/null; ADDR="$(w getnewaddress)"
MT=""; gb() { [[ -n "${MT:-}" ]] && MT=$((MT + 1)) && cli setmocktime "$MT" >/dev/null; cli generateblock "$ADDR" "$@"; }
step "[2/6] mine to height 430 (activation happens at 432)"
cli generatetoaddress 430 "$ADDR" >/dev/null
MT="$(cli getblockheader "$(cli getbestblockhash)" | j time)"; cli setmocktime "$MT" >/dev/null
REDEEM=7551; P2SH="$(cli decodescript "$REDEEM" | j p2sh)"
step "[3/6] create funding tx + violating spend tx"
FUND_TXID="$(w sendtoaddress "$P2SH" 1.0)"; FUND_TAG="${FUND_TXID:0:4}"
cli getmempoolentry "$FUND_TXID" >/dev/null 2>&1 \
&& log "funding tx $(tid "$FUND_TAG"): ${ACC} ${OK} (mempool)" \
|| log "funding tx $(tid "$FUND_TAG"): ${REJ} ${BUG} (mempool)"
TXINFO="$(w gettransaction "$FUND_TXID")"; FUND_HEX="$(printf '%s\n' "$TXINFO" | j hex)"
FUND_VOUT=0; cli gettxout "$FUND_TXID" 0 true | j address | grep -q "$P2SH" || FUND_VOUT=1
TXID_LE="$(revhex "$FUND_TXID")"; VOUT_LE="$(le 8 "$FUND_VOUT")"; OUT_LE="$(le 16 99990000)" # fee=10k sats
PUSH300="$(printf '42%.0s' {1..300})"; SCRIPTSIG="4d2c01${PUSH300}02${REDEEM}" # 300-byte push + redeemScript; invalid if reduced_data enforced
SPEND_HEX="0200000001${TXID_LE}${VOUT_LE}fd3201${SCRIPTSIG}ffffffff01${OUT_LE}015100000000"
SPEND_TAG="$("$TX" -txid "$SPEND_HEX")"; SPEND_TAG="${SPEND_TAG:0:4}"
log "spend tx $(tid "$SPEND_TAG"): crafted (will be put in blocks directly, never submitted to mempool)"
step "[4/6] control: fund at h=432 (post-activation, NOT exempt) -> spend at h=433 must be REJECTED"
gb "[]" >/dev/null; gb "[\"$FUND_HEX\"]" >/dev/null; log "funding tx $(tid "$FUND_TAG"): ${ACC} ${OK} (confirmed at h=432)"
TIP="$(cli getblockcount)"; TRY_H=$((TIP + 1))
if out="$(gb "[\"$SPEND_HEX\"]" 2>&1)"; then
logh "$TRY_H" "spend tx $(tid "$SPEND_TAG"): ${ACC} ${BUG} (BUG: should be rejected)"
else
logh "$TRY_H" "spend tx $(tid "$SPEND_TAG"): ${REJ} ${OK} :: $(printf '%s' "$out" | rej_reason)"
fi
step "[5/6] poison: reorg so fund is at h=431 (<432, exempt) + validate-only spend in block h=432 (cache insert happens here)"
for _ in 1 2; do cli invalidateblock "$(cli getbestblockhash)" >/dev/null; done
gb "[\"$FUND_HEX\"]" >/dev/null; log "funding tx $(tid "$FUND_TAG"): ${ACC} ${OK} (confirmed at h=431)"
TIP="$(cli getblockcount)"; TRY_H=$((TIP + 1))
# submit=false => TestBlockValidity-only (cache write happens here), no block connection
gb "[\"$SPEND_HEX\"]" false >/dev/null 2>&1 \
&& logh "$TRY_H" "spend tx $(tid "$SPEND_TAG"): ${ACC} ${OK} (validate-only; exempt)" \
|| logh "$TRY_H" "spend tx $(tid "$SPEND_TAG"): ${REJ} ${BUG} (unexpected; should be accepted/exempt here)"
step "[6/6] trigger: reorg back so fund is at h=432 (>=432, NOT exempt) -> spend at h=433 must be REJECTED (ACCEPTED means poisoned cache hit)"
cli invalidateblock "$(cli getbestblockhash)" >/dev/null # if you comment step [5/6], this step rejects (no poisoned cache entry)
gb "[]" >/dev/null; gb "[\"$FUND_HEX\"]" >/dev/null; log "funding tx $(tid "$FUND_TAG"): ${ACC} ${OK} (confirmed at h=432)"
TIP="$(cli getblockcount)"; TRY_H=$((TIP + 1))
if out="$(gb "[\"$SPEND_HEX\"]" 2>&1)"; then
logh "$TRY_H" "spend tx $(tid "$SPEND_TAG"): ${ACC} ${BUG} (BUG: cache poisoning; this same tx at this same height was REJECTED above)"
else
logh "$TRY_H" "spend tx $(tid "$SPEND_TAG"): ${REJ} ${OK} :: $(printf '%s' "$out" | rej_reason)"
fiwhich demonstrated the flow as:
[1/6] start bitcoind
[2/6] mine to height 430 (activation happens at 432)
[3/6] create funding tx + violating spend tx
[h=430] funding tx 3990: ACCEPTED 👍 (mempool)
[h=430] spend tx 002a: crafted (will be put in blocks directly, never submitted to mempool)
[4/6] control: fund at h=432 (post-activation, NOT exempt) -> spend at h=433 must be REJECTED
[h=432] funding tx 3990: ACCEPTED 👍 (confirmed at h=432)
[h=433] spend tx 002a: REJECTED 👍 :: mempool-script-verify-flag-failed (Push value size limit exceeded)
[5/6] poison: reorg so fund is at h=431 (<432, exempt) + validate-only spend in block h=432 (cache insert happens here)
[h=431] funding tx 3990: ACCEPTED 👍 (confirmed at h=431)
[h=432] spend tx 002a: ACCEPTED 👍 (validate-only; exempt)
[6/6] trigger: reorg back so fund is at h=432 (>=432, NOT exempt) -> spend at h=433 must be REJECTED (ACCEPTED means poisoned cache hit)
[h=432] funding tx 3990: ACCEPTED 👍 (confirmed at h=432)
[h=433] spend tx 002a: ACCEPTED 👎 (BUG: cache poisoning; this same tx at this same height was REJECTED above)
The bug requires a reorg that moves a funding tx across the activation boundary - which is unlikely to occur just by chance. It is, however, a real consensus bug that can be triggered deliberately by miners aware of it, and can cause real harm to users who choose to run this software. I'm disclosing it ASAP to minimize potential harm.
Note that while reorg behavior was lightly tested in https://github.com/bitcoinknots/bitcoin/pull/238/changes/4c99d3b2630ef38ae8865be90850918cb3423f24#diff-2a47a7847c78024eff4f7e6ee245aa1faa366720070ba0184436bb6858bee06dR187-R210, the mined blocks didn't contain any txs, and the reorgs weren't done across the activation height. Most blocks on mainnet do contain transactions so it wasn't really testing anything useful.
Signed commits cloned to: https://github.com/l0rinc/bitcoin/commits/detached484
nit: the latest rebase done a few hours ago removed @luke-jr from the seeds in https://github.com/bitcoinknots/bitcoin/compare/f4045f37b6ba9780cb1f5d40295d5aa12192a29f..1d3cdac50c5616d7de5c433f3ff76ed7513f3a5a#diff-9468810859a6881caa4f5c4d3c806f494e8e078c4a4c9c53d8ed74a6d96d4973L20
It also changed the Knots release notes to Core release notes https://github.com/bitcoinknots/bitcoin/compare/f4045f37b6ba9780cb1f5d40295d5aa12192a29f..1d3cdac50c5616d7de5c433f3ff76ed7513f3a5a#diff-474e3093b86659f3d23995cb2fbe8e84bcacf0e3b019442b26c729445c7f2a8eL2
Were these done intentionally or is it a rebase oversight?