Update MANDATORY_SCRIPT_VERIFY_FLAGS #26291
Conversation
|
#15169 proposed having net_processing not ban nodes for txs that didn't meet soft fork rules, which then allowed dropping the Other previous discussion of the topic: #23536 (review) #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) |
02482a0 to
02356fb
Compare
|
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 |
|
Approach ACK The 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. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
|
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? |
I've added logging for |
|
Concept ACK to at least not reporting consensus script failures as "non-mandatory." wrt how the |
See #15141 where |
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: |
|
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... |
|
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 |
02356fb to
1b09cc5
Compare
|
Code review ACK 1b09cc5 Running some logging as well. |
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. |
|
Should we add exception(s) to |
|
It's not really about |
|
Isn't |
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.
The error for an invalid spend like that will be overridden as DERSIG-invalid txs could presumably be relayed by nodes running versions prior to 0.8.0 (2013-02-19, #2008) though, 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'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,
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. |
|
ACK 1b09cc5 The segwit version and the witness program are pushed as a single item on the scriptSig (for instance All your other comments make sense to me. |
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: $ wget https://bitcoincore.org/bin/bitcoin-core-0.10.3/bitcoin-0.10.3-linux64.tar.gz
$ tar xzvf bitcoin-0.10.3-linux64.tar.gz
$ cd bitcoin-0.10.3/bin
$ wget http://azure.erisian.com.au/~aj/tmp/bitcoin-testcase-26291.tgz
$ echo 'a8019a747e93c2bb35787a70400087361f90459e9f267a0103a22df1021f62f2 bitcoin-testcase-26291.tgz' | sha256sum -c
bitcoin-testcase-26291.tgz: OK
$ tar xzvf bitcoin-testcase-26291.tgz
$ mkdir datadir
$ echo 'rpcpassword=pass' > datadir/bitcoin.conf
$ ./bitcoind -regtest -datadir=`pwd`/datadir -daemon
$ cat blocks-no-witness.txt | while read blk; do ./bitcoin-cli -regtest -datadir=`pwd`/datadir submitblock $blk; done
$ ./bitcoin-cli -regtest -datadir=`pwd`/datadir getblockchaininfo | jq .blocks
102
$ ./bitcoin-cli -regtest -datadir=`pwd`/datadir sendrawtransaction $(cat tx-no-witness)
134dabc7cb0f9b16583a3cff5d48695aebbc0b8d24fe4a57983f1156494a6e55With 0.11.1 instead, you get: (The "unknown error" was fixed in #11418) EDIT: err, "unknown error" is from 0.12.0 which says: |
There was a problem hiding this comment.
Concept and code-review ACK 1b09cc5
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:
Lines 530 to 538 in 92d25e4
|
Nothing interesting in the logs for 3 days now. |
|
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: achow101@d473052 It makes and uses
From 0.11.0 onwards, all of the above transactions are rejected by 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. |
|
@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. |
Note that versions 0.9.x and 0.10.x checked tx standardness rules ( Lines 806 to 808 in 92d25e4 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). |
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. |
|
ACK 1b09cc5 |
1b09cc5 Make post-p2sh consensus rules mandatory for tx relay (Anthony Towns) 69c31bc doc, policy: Clarify comment on STANDARD_SCRIPT_VERIFY_FLAGS (Anthony Towns) Pull request description: The `MANDATORY_SCRIPT_VERIFY_FLAGS` constant was introduced in bitcoin#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`. ACKs for top commit: Sjors: Code review ACK 1b09cc5 darosior: ACK 1b09cc5 achow101: ACK 1b09cc5 theStack: Concept and code-review ACK 1b09cc5 Tree-SHA512: d3e5868e8cece478f2e934956ba0c231d8bb9c2daefd0df1f817774e292049902cfc1d0cd76dbd2e7722627a93eab2d7046ff678199aac70a2b01642e69349f1
The
MANDATORY_SCRIPT_VERIFY_FLAGSconstant 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 asTX_CONSENSUSandmandatory-script-verify-flag-failedvsTX_NOT_STANDARDandnon-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 reportTX_CONSENSUSfailures for txs that fail dersig/csv/cltv/nulldummy/witness/taproot checks, instead ofTX_NOT_STANDARD, which in turn addsMisbehaving(100)viaMaybePunishNodeForTxinnet_processing.