Update MANDATORY_SCRIPT_VERIFY

ajtowns commented at 5:13 am on October 11, 2022: contributor

The MANDATORY_SCRIPT_VERIFY_FLAGS constant was introduced in #3843 to distinguish between block consensus rules and relay standardness rules. However it was not actually used in the consensus code path: instead it only differentiates between the failure being reported as TX_CONSENSUS and mandatory-script-verify-flag-failed vs TX_NOT_STANDARD and non-mandatory-script-verify-flag.

This updates the list of mandatory flags to include the post-p2sh soft forks that are enforced as consensus rules via GetBlockScriptFlags(). The effect of this change is that validation.cpp will report TX_CONSENSUS failures for txs that fail dersig/csv/cltv/nulldummy/witness/taproot checks, instead of TX_NOT_STANDARD, which in turn adds Misbehaving(100) via MaybePunishNodeForTx in net_processing.

ajtowns added the label P2P on Oct 11, 2022

ajtowns added the label Validation on Oct 11, 2022

ajtowns added the label Needs Conceptual Review on Oct 11, 2022

ajtowns commented at 5:25 am on October 11, 2022: contributor

#15169 proposed having net_processing not ban nodes for txs that didn’t meet soft fork rules, which then allowed dropping the MANDATORY_SCRIPT_VERIFY_FLAGS constant entirely. Is that a better approach?

Other previous discussion of the topic: #23536#pullrequestreview-905402394 #5696

(EDIT: I was poking at this because I got an error about a taproot consensus rule that said it was “non-mandatory”, which seemed misleading)

ajtowns force-pushed on Oct 11, 2022

luke-jr commented at 3:15 pm on October 25, 2022: member

Seems like this could result in banning pre-Segwit and pre-Taproot nodes?

ajtowns commented at 4:59 pm on October 25, 2022: contributor

Seems like this could result in banning pre-Segwit and pre-Taproot nodes?

It would only impact nodes that produce/relay invalid segwit/taproot spends, ie ones that don’t implement standardness checks. It also only triggers discouragement not banning, so affected peers can still reconnect. If we don’t want to do that, we should probably drop MaybePunishNodeForTx entirely.

john-moffett commented at 7:56 pm on January 23, 2023: contributor

Approach ACK

The non-mandatory part is very misleading for actual consensus issues. Worse, the TxValidationResult is explicitly TX_NOT_STANDARD.

I got about halfway through a very similar change before I found this PR. I don’t think the risk of discouraging old nodes is all that high. As you said, it’s only those who eschew standardness checks. We probably wouldn’t want them to keep sending us incomplete or invalid data anyway.

DrahtBot commented at 7:56 pm on January 23, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Sjors, darosior, theStack, achow101
Concept ACK	instagibbs, glozow
Approach ACK	john-moffett

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

fanquake requested review from instagibbs on Mar 20, 2023

instagibbs commented at 3:44 pm on March 20, 2023: member

strong concept ACK on making it more clear on what flags are doing what(I find those names super unhelpful)

Completely unsure on increasing the scope of what we disconnect for. Are we seeing these messages in the wild?

ajtowns commented at 1:25 pm on March 21, 2023: contributor

Completely unsure on increasing the scope of what we disconnect for. Are we seeing these messages in the wild?

I’ve added logging for TX_NOT_STANDARD in MaybePunishNodeForTx. After 20 hours or so running that on a listening node, I’ve had a couple of instances of rejections for dust (via IsStandardTx for outputs matching IsDust), but so far that’s it.

glozow commented at 2:37 pm on March 22, 2023: member

Concept ACK to at least not reporting consensus script failures as “non-mandatory.”

wrt how the TxValidationResult types are used in MaybePunishNodeForTx, is something like this what TX_RECENT_CONSENSUS_CHANGE was intended for?

ajtowns commented at 3:23 pm on March 22, 2023: contributor

wrt how the TxValidationResult types are used in MaybePunishNodeForTx, is something like this what TX_RECENT_CONSENSUS_CHANGE was intended for?

See #15141 where RECENT_CONSENSUS_CHANGE was introduced, and #11639 where it was discussed more (under the name SOFT_FORK). (Note that at that time, it applied to both blocks and txs, the validation results weren’t separated until #15921 – so some of the discussion there focuses more on blocks that violate recent soft fork rules rather than txs)

ajtowns commented at 6:54 am on March 28, 2023: contributor

Completely unsure on increasing the scope of what we disconnect for. Are we seeing these messages in the wild?

I’ve added logging for TX_NOT_STANDARD in MaybePunishNodeForTx. After 20 hours or so running that on a listening node, I’ve had a couple of instances of rejections for dust (via IsStandardTx for outputs matching IsDust), but so far that’s it.

I’ve been running this on mainnet for a week now and have only caught non-standard tx failures for dust, so, no, I don’t think we’re seeing these in the wild. FWIW, my node discarded 505 non-standard txs, all due to the dust rule, and 493 of those txs were from a single node. Example discarded tx:

02023-03-28T06:35:05Z non-standard tx 95cfc86ba6e3141a991ad296fe6c3dba96879446bdac6a053529e08d97328f9a from peer=XXX: dust

DrahtBot added the label CI failed on May 29, 2023

DrahtBot removed the label CI failed on May 31, 2023

instagibbs commented at 2:50 pm on June 23, 2023: member

I think these changes would mean we’d get taproot “consensus” looking failure strings for a test network for taproot isn’t active? The rest of the flags being moved are under buried deployments which means block height N and beyond are enforcing it(including the next block definitionally). I haven’t looked at this in a long time.

edit: @ajtowns notes offline that while not buried, it’s actually enforced on all post-segwit blocks(with a couple of exceptions hardcoded): #23536

will test…

instagibbs commented at 5:29 pm on July 3, 2023: member

I’ve been running this for the last week, no transaction-based misbehavior reports in my logs. still unsure the juice is worth the squeeze, but appears to not be obviously harmful.

edit: one option is to merge this, and still consider removing disconnects entirely as done in #15169, but it seems to be a much larger delta with unresolved conversations

ajtowns removed the label Needs Conceptual Review on Jul 27, 2023

in src/policy/policy.h:87 in 461259c4ec outdated

84 /**
85  * Standard script verification flags that standard transactions will comply
86- * with. However scripts violating these flags may still be present in valid
87- * blocks and we must accept those blocks.
88+ * with. However we do not ban/disconnect nodes that forward txs violating
89+ * these rules, for better forwards and backwards compatability.

theStack commented at 10:33 pm on August 15, 2023:

Strictly speaking, wouldn’t this sentence belong to STANDARD_NOT_MANDATORY_VERIFY_FLAGS rather than to STANDARD_SCRIPT_VERIFY_FLAGS? The latter includes mandatory flags, for which a violation does lead to a ban/disconnect, as just stated in the same commit a few lines above (“may trigger a DoS ban”).

ajtowns commented at 1:29 am on August 17, 2023:

Sure. Presuming #28244 is merged, will tweak the wording when rebasing on that

ajtowns commented at 3:31 am on August 18, 2023:

Done

DrahtBot added the label Needs rebase on Aug 17, 2023

doc, policy: Clarify comment on STANDARD_SCRIPT_VERIFY_FLAGS 69c31bc748

Make post-p2sh consensus rules mandatory for tx relay 1b09cc5959

ajtowns force-pushed on Aug 17, 2023

DrahtBot removed the label Needs rebase on Aug 17, 2023

DrahtBot added the label CI failed on Aug 18, 2023

ajtowns commented at 3:40 am on August 18, 2023: contributor

First commit from this has been merged via #28244; rebased on top of that. See #28285 or #28289 for CI failure.

Sjors commented at 11:18 am on August 18, 2023: member

Code review ACK 1b09cc5959d4719ffad131b395f8185e9ab4b1a1

Running some logging as well.

fanquake removed review request from instagibbs on Aug 18, 2023

fanquake requested review from instagibbs on Aug 18, 2023

DrahtBot removed the label CI failed on Aug 18, 2023

darosior commented at 3:41 pm on August 18, 2023: member

Seems like this could result in banning pre-Segwit and pre-Taproot nodes?

It would only impact nodes that produce/relay invalid segwit/taproot spends, ie ones that don’t implement standardness checks. It also only triggers discouragement not banning, so affected peers can still reconnect. If we don’t want to do that, we should probably drop MaybePunishNodeForTx entirely.

An invalid p2sh-wrapped Segwit spend would be considered standard by a pre-segwit node, but consensus-invalid by a post-segwit node. So this change would make it possible for an attacker to broadcast an invalid spend of any p2sh-wrapped Segwit UTxO to all pre-0.13 nodes on the network thereby splitting them from the network.

They could still reconnect, but would be disconnected again until either a block is mined with a (valid) spend of the p2sh-wrapped segwit coin, or they manually purge the invalid transaction from their mempool. But it would be trivial for the attacker to send another invalid spend of an unspent p2sh-wrapped segwit coin.

Sjors commented at 3:48 pm on August 18, 2023: member

Should we add exception(s) to MaybePunishNodeForTx for where a soft-fork made a previously standard transaction consensus invalid? Is this the only time that happened?

darosior commented at 4:06 pm on August 18, 2023: member

It’s not really about MaybePunishNodeForTx (which already does not add a Misbehaving score for TX_RECENT_CONSENSUS_CHANGE), but rather to actually detect it’s due to a recent consensus change and not a pre-existing consensus rule and return TX_RECENT_CONSENSUS_CHANGE in this case.

Sjors commented at 6:12 pm on August 18, 2023: member

Isn’t TX_RECENT_CONSENSUS_CHANGE meant for more recent consensus changes?

ajtowns commented at 1:06 am on August 19, 2023: contributor

An invalid p2sh-wrapped Segwit spend would be considered standard by a pre-segwit node, but consensus-invalid by a post-segwit node.

An invalid p2sh-wrapped segwit spend will violate the cleanstack rule (#5143, released in v0.11.0 2015-07-12), by leaving both the segwit version and the witness program on the stack. For bitcoind versions prior to v10.0, I believe it would fail standardness checks due to only specific script templates being allowed, and segwit/p2sh not being one of them. So I think only bitcoind v10.x would relay this sort of tx.

So this change would make it possible for an attacker to broadcast an invalid spend of any p2sh-wrapped Segwit UTxO to all pre-0.13 nodes on the network thereby splitting them from the network.

The error for an invalid spend like that will be overridden as TX_WITNESS_STRIPPED which doesn’t result in a ban.

DERSIG-invalid txs could presumably be relayed by nodes running versions prior to 0.8.0 (2013-02-19, #2008) though, and NULLDUMMY-invalid txs were presumably relayable prior to 0.10.0 (2015-02-16 #3843). Both would result in a misbehaving score and possible disconnection/discouragement with this PR.

Note that a ban here wouldn’t split them from the network – they’d still be able to remain connected to nodes running bitcoind versions up to v0.25 nodes who can then connect to nodes running v0.26+. Also, running very old versions of bitcoind likely leaves your node vulnerable to long-fixed bugs that can have similar or worse results, eg https://en.bitcoin.it/wiki/CVE-2013-2293 or https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2016-10724 .

They could still reconnect, but would be disconnected again until either a block is mined with a (valid) spend of the p2sh-wrapped segwit coin,

They’d only be disconnected again if they rebroadcast the tx; I expect that would only occur if the tx was sent to their wallet address. But as you say,

[…] But it would be trivial for the attacker to send another invalid spend of an unspent p2sh-wrapped segwit coin.

In general, I think the idea is just: provided the tx has been non-standard for >5 years, and the tx is consensus invalid anyway, it’s fine to ban nodes that try to relay the tx. The only nodes this could potentially cause problems for are ones that are very old (and thus are likely buggy for other reasons), and it’s easily worked around (by running the old node in blocksonly mode, or hiding it behind a new blocksonly node, or hiding it behind a node that runs an intermediate version). I believe for all the cases here, the txs have been non-standard for >8 years.

darosior commented at 8:36 am on August 19, 2023: member

ACK 1b09cc5959d4719ffad131b395f8185e9ab4b1a1

The segwit version and the witness program are pushed as a single item on the scriptSig (for instance <0 <20-bytes pubkey hash>> for a P2SH-P2WPKH) so i don’t think it would violate the cleanstack rule of 0.11 and 0.12 nodes. Regarding the templates i believe you are right. So that’s only 0.10, 0.11 and 0.12 nodes that would relay an invalid p2sh-wrapped segwit spending transaction. However, as you say those shouldn’t be a concern since the error would be overridden as TX_WITNESS_STRIPPED.

All your other comments make sense to me.

ajtowns commented at 10:27 am on August 19, 2023: contributor

The segwit version and the witness program are pushed as a single item on the scriptSig (for instance <0 <20-bytes pubkey hash>> for a P2SH-P2WPKH) so i don’t think it would violate the cleanstack rule of 0.11 and 0.12 nodes.

A p2sh spend is evaluated in two parts, first by running the scriptSig to generate the stack of elements, then checking the top-most stack element matches the hash from the scriptPubKey (ie, running the scriptPubKey), then by interpreting the top-most stack element as a script and executing it with the remaining elements from the stack. It’s this second step that leaves two (or more) entries on the stack and violates the cleanstack rule if you’re not segwit aware. See https://github.com/bitcoin/bitcoin/blob/v0.11.0/src/script/interpreter.cpp#L1137-L1147

Here’s an example:

 0$ wget https://bitcoincore.org/bin/bitcoin-core-0.10.3/bitcoin-0.10.3-linux64.tar.gz
 1$ tar xzvf bitcoin-0.10.3-linux64.tar.gz
 2$ cd bitcoin-0.10.3/bin
 3$ wget http://azure.erisian.com.au/~aj/tmp/bitcoin-testcase-26291.tgz
 4$ echo 'a8019a747e93c2bb35787a70400087361f90459e9f267a0103a22df1021f62f2  bitcoin-testcase-26291.tgz' | sha256sum -c
 5bitcoin-testcase-26291.tgz: OK
 6$ tar xzvf bitcoin-testcase-26291.tgz
 7$ mkdir datadir
 8$ echo 'rpcpassword=pass' > datadir/bitcoin.conf
 9$ ./bitcoind -regtest -datadir=`pwd`/datadir -daemon
10$ cat blocks-no-witness.txt | while read blk; do ./bitcoin-cli -regtest -datadir=`pwd`/datadir submitblock $blk; done
11$ ./bitcoin-cli -regtest -datadir=`pwd`/datadir getblockchaininfo | jq .blocks
12102
13$ ./bitcoin-cli -regtest -datadir=`pwd`/datadir sendrawtransaction $(cat tx-no-witness)
14134dabc7cb0f9b16583a3cff5d48695aebbc0b8d24fe4a57983f1156494a6e55

With 0.11.1 instead, you get:

0error: {"code":-26,"message":"64: non-mandatory-script-verify-flag (No error)"}

(The “unknown error” was fixed in #11418)

EDIT: err, “unknown error” is from 0.12.0 which says:

0error code: -26
1error message:
264: non-mandatory-script-verify-flag (unknown error)

theStack approved

theStack commented at 2:46 am on August 20, 2023: contributor

Concept and code-review ACK 1b09cc5959d4719ffad131b395f8185e9ab4b1a1

While I doubt that the increased set of mandatory flags changes anything noticably in practice, it still seems the right approach from a philosophical perspective. Being more strict to peers (while still giving them the chance to reconnect at any time) that send tx which have been non-standard for +5 years and consensus-invalid just seems reasonable, and all doubts that this could lead to problems for peers running older versions have been convincingly refuted (nice analysis @ajtowns!).

For bitcoind versions prior to v10.0, I believe it would fail standardness checks due to only specific script templates being allowed, and segwit/p2sh not being one of them. So I think only bitcoind v10.x would relay this sort of tx.

Was curious about this, dug a bit into the past and can confirm this for v0.9.0. The AreInputsStandard function there calls Solver twice for spent P2SH outputs: once for the output script (obviously), and another time for the stack top element after evaluating the scriptSig pushes, i.e. the redeem script. For p2sh-segwit spends, the top element (OP_0 <20 or 32 bytes data>) wouldn’t match any of the back-then P2PK/P2PKH/multsig/nulldata templates (i.e. Solver returns false) and the tx would therefore be considered non-standard: https://github.com/bitcoin/bitcoin/blob/92d25e4eebbc20c4b056faeab688b2cef5790bac/src/main.cpp#L530-L538

Sjors commented at 9:21 am on August 21, 2023: member

Nothing interesting in the logs for 3 days now.

achow101 commented at 2:36 am on August 23, 2023: member

I wrote a test script which goes through all of our releases since 0.9.0 to check which things that violate the updated mandatory flags get relayed: https://github.com/achow101/bitcoin/commit/d4730529012a37539c54312302f77c1af414f0d6

It makes and uses sendrawtransaction on single input transactions with the following:

p2sh-p2wpkh with invalid signature
p2sh-p2wpkh without witness
p2wpkh with invalid signature
p2wpkh without witness
p2tr with invalid signature
p2tr with invalid witness
p2sh multisig with non-null dummy
simple csv script prior to relative lock time
simple cltv script prior to lock time
p2pkh with non-canonical signature

From 0.11.0 onwards, all of the above transactions are rejected by sendrawtransaction.

0.9.x and 0.10.x accept the p2sh-p2wpkh, p2wpkh, and p2tr witness stripped transactions. 0.9.x additionally allows the non-null dummy, csv, and cltv transactions.

All tested versions reject the non-canonical signature.

Sjors commented at 8:53 am on August 23, 2023: member

@achow101 very cool. Perhaps we can add this test (in another PR) with the caveat that none of the old releases run on our CI. It should have a brief instruction for how to fetch the old releases for those who do want to try.

theStack commented at 11:12 am on August 23, 2023: contributor

0.9.x and 0.10.x accept the p2sh-p2wpkh, p2wpkh, and p2tr witness stripped transactions.

Note that versions 0.9.x and 0.10.x checked tx standardness rules (IsStandardTx, AreInputsStandard) only on mainnet, and there was no way to enable them for regtest/testnet (the -acceptnonstdtxn parameter was introduced in v0.12.0). E.g. for 0.9.0:

https://github.com/bitcoin/bitcoin/blob/92d25e4eebbc20c4b056faeab688b2cef5790bac/src/main.cpp#L806-L808

So on mainnet, I think all of the segwit spend transactions listed above (p2sh-p2wpkh, p2wpkh, p2tr) are also rejected for 0.9.x, and 0.10.x only accepts the p2sh-segwit spend (0.10.x allows non-standard P2SH redeem-scripts, as long as they don’t exceed a certain sigop count).

achow101 commented at 8:10 pm on August 23, 2023: member

Note that versions 0.9.x and 0.10.x checked tx standardness rules (IsStandardTx, AreInputsStandard) only on mainnet

Oh, good catch.

I’ve built those versions with that patched out so that they always check the standardness rules. With this change, 0.9.x rejects all of the transactions, and 0.10.x accepts the p2sh-p2wpkh witness stripped.

achow101 commented at 8:11 pm on August 23, 2023: member

ACK 1b09cc5959d4719ffad131b395f8185e9ab4b1a1

achow101 merged this on Aug 23, 2023

achow101 closed this on Aug 23, 2023

Update MANDATORY_SCRIPT_VERIFY_FLAGS #26291

Reviews

Conflicts