Our policy checks for non-standard inputs depend only on the non-witness
portion of a transaction: we look up the scriptPubKey of the input being
spent from our UTXO set (which is covered by the input txid), and the p2sh
checks only rely on the scriptSig portion of the input.
Consequently it's safe to add txids of transactions that fail these checks to
the reject filter, as the witness is irrelevant to the failure. This is helpful
for any situation where we might request the transaction again via txid (either
from txid-relay peers, or if we might fetch the transaction via txid due to
parent-fetching of orphans).
Further, in preparation for future witness versions being deployed on the
network, ensure that WITNESS_UNKNOWN transactions are rejected in
AreInputsStandard(), so that transactions spending v1 (or greater) witness
outputs will fall into this category of having their txid added to the reject
filter.
0a4f1422cd Further improve comments around recentRejects (Suhas Daftuar)
0e20cfedb7 Disconnect peers sending wtxidrelay message after VERACK (Suhas Daftuar)
cacd85209e test: Use wtxid relay generally in functional tests (Fabian Jahr)
8d8099e97a test: Add tests for wtxid tx relay in segwit test (Fabian Jahr)
9a5392fdf6 test: Update test framework p2p protocol version to 70016 (Fabian Jahr)
dd78d1d641 Rename AddInventoryKnown() to AddKnownTx() (Suhas Daftuar)
4eb515574e Make TX_WITNESS_STRIPPED its own rejection reason (Suhas Daftuar)
97141ca442 Delay getdata requests from peers using txid-based relay (Suhas Daftuar)
46d78d47de Add p2p message "wtxidrelay" (Suhas Daftuar)
2d282e0cba ignore non-wtxidrelay compliant invs (Anthony Towns)
ac88e2eb61 Add support for tx-relay via wtxid (Suhas Daftuar)
8e68fc246d Add wtxids to recentRejects instead of txids (Suhas Daftuar)
144c385820 Add wtxids of confirmed transactions to bloom filter (Suhas Daftuar)
85c78d54af Add wtxid-index to orphan map (Suhas Daftuar)
08b39955ec Add a wtxid-index to mapRelay (Suhas Daftuar)
60f0acda71 Just pass a hash to AddInventoryKnown (Suhas Daftuar)
c7eb6b4f1f Add wtxid to mempool unbroadcast tracking (Amiti Uttarwar)
2b4b90aa8f Add a wtxid-index to the mempool (Suhas Daftuar)
Pull request description:
Using txids (a transaction's hash, without witness) for transaction relay is problematic, post-segwit -- if a peer gives us a segwit transaction that fails policy checks, it could be because the txid associated with the transaction is definitely unacceptable to our node (regardless of the witness), or it could be that the transaction was malleated and with a different witness, the txid could be accepted to our mempool.
We have a bloom filter of recently rejected transactions, whose purpose is to help us avoid redownloading and revalidating transactions that fail to be accepted, but because of this potential for witness malleability to interfere with relay of valid transactions, we do not use the filter for segwit transactions. This issue is discussed at some length in #8279. The effect of this is that whenever a segwit transaction that fails policy checks is relayed, a node would download that transaction from every peer announcing it, because it has no way presently to cache failure. Historically this hasn't been a big problem, but if/when policy for accepting segwit transactions were to change (eg taproot, or any other change), we could expect older nodes talking to newer nodes to be wasting bandwidth because of this.
As discussed in that issue, switching to wtxid-based relay solves this problem -- by using an identifier for a transaction that commits to all the data in our relay protocol, we can be certain if a transaction that a peer is announcing is one that we've already tried to process, or if it's something new. This PR introduces support for wtxid-based relay with peers that support it (and remains backwards compatible with peers that use txids for relay, of course).
Apart from code correctness, one issue to be aware of is that by downloading from old and new peers alike, we should expect there to be some bandwidth wasted, because sometimes we might download the same transaction via txid-relay as well as wtxid-relay. The last commit in this PR implements a heuristic I want to analyze, which is to just delay relay from txid-relay peers by 2 seconds, if we have at least 1 wtxid-based peer. I've just started running a couple nodes with this heuristic so I can measure how well it works, but I'm open to other ideas for minimizing that issue. In the long run, I think this will be essentially a non-issue, so I don't think it's too big a concern, we just need to bite the bullet and deal with it during upgrade.
Finally, this proposal would need a simple BIP describing the changes, which I haven't yet drafted. However, review and testing of this code in the interim would be welcome.
To do items:
- [x] Write BIP explaining the spec here (1 new p2p message for negotiating wtxid-based relay, along with a new INV type)
- [ ] Measure and evaluate a heuristic for minimizing how often a node downloads the same transaction twice, when connected to old and new nodes.
ACKs for top commit:
naumenkogs:
utACK 0a4f1422cd
laanwj:
utACK 0a4f1422cd
Tree-SHA512: d8eb8f0688cf0cbe9507bf738e143edab1f595551fdfeddc2b6734686ea26e7f156b6bfde38bad8bbbe8bec1857c7223e1687f8f018de7463dde8ecaa8f450df
Previously, TX_WITNESS_MUTATED could be returned during transaction validation
for either transactions that had a witness that was non-standard, or for
transactions that had no witness but were invalid due to segwit validation
rules.
However, for txid/wtxid-relay considerations, net_processing distinguishes the
witness stripped case separately, because it affects whether a wtxid should be
able to be added to the reject filter. It is safe to add the wtxid of a
witness-mutated transaction to the filter (as that wtxid shouldn't collide with
the txid, and hence it wouldn't interfere with transaction relay from
txid-relay peers), but it is not safe to add the wtxid (== txid) of a
witness-stripped transaction to the filter, because that would interfere with
relay of another transaction with the same txid (but different wtxid) when
relaying from txid-relay peers.
Also updates the comment explaining this logic, and explaining that we can get
rid of this complexity once there's a sufficient deployment of wtxid-relaying
peers on the network.
Change comment from `The reason is that if the number of hashes in the list at a given time
is odd`, to ` The reason is that if the number of hashes in the list at a given level
is odd` (to be a bit more precise)
ValidationState::Invalid() takes a parameter `ret` which is returned to
the caller. All call sites set this to false. Remove the `ret` parameter
and just return false always.
9075d13153 [docs] Add release notes for removal of REJECT reasons (John Newbery)
04a2f326ec [validation] Fix REJECT message comments (John Newbery)
e9d5a59e34 [validation] Remove REJECT code from CValidationState (John Newbery)
0053e16714 [logging] Don't log REJECT code when transaction is rejected (John Newbery)
a1a07cfe99 [validation] Fix peer punishment for bad blocks (John Newbery)
Pull request description:
We no longer send BIP 61 REJECT messages, so there's no need to set
a REJECT code in the CValidationState object.
Note that there is a minor bug fix in p2p behaviour here. Because the
call to `MaybePunishNode()` in `PeerLogicValidation::BlockChecked()` only
previously happened if the REJECT code was > 0 and < `REJECT_INTERNAL`,
then there are cases were `MaybePunishNode()` can get called where it
wasn't previously:
- when `AcceptBlockHeader()` fails with `CACHED_INVALID`.
- when `AcceptBlockHeader()` fails with `BLOCK_MISSING_PREV`.
Note that `BlockChecked()` cannot fail with an 'internal' reject code. The
only internal reject code was `REJECT_HIGHFEE`, which was only set in
ATMP.
This reverts a minor bug introduced in 5d08c9c579.
ACKs for top commit:
ariard:
ACK 9075d13, changes since last reviewed are splitting them in separate commits to ease understanding and fix nits
fjahr:
ACK 9075d13153, confirmed diff to last review was fixing nits in docs/comments.
ryanofsky:
Code review ACK 9075d13153. Only changes since last review are splitting the main commit and updating comments
Tree-SHA512: 58e8a1a4d4e6f156da5d29fb6ad6a62fc9c594bbfc6432b3252e962d0e9e10149bf3035185dc5320c46c09f3e49662bc2973ec759679c0f3412232087cb8a3a7
fdb3e8f8b2 Ignore old versionbit activations (Anthony Towns)
Pull request description:
PR 16060 removed the CSV and Segwit BIP9 softfork definitions and hard-coded ('buried') the activation heights. The versionbits code will warn users if an undefined softfork has been signalled in block header versions, and removing the CSV/Segwit definitions caused those warnings to be triggered.
Change the BIP 9 warning code to only check for unknown softforks after the segwit activation height.
ACKs for top commit:
MarcoFalke:
ACK fdb3e8f8b2
ajtowns:
ACK fdb3e8f8b2 for what it's worth
achow101:
ACK fdb3e8f8b2
Sjors:
ACK fdb3e8f8b2. It makes the bit 0 warning go away in mainnet and testnet QT when a new block arrives. I think the code is clear enough.
jonatack:
ACK fdb3e8f8b2
Tree-SHA512: e6fd34e8902f8c7affb28e8951803e47d542710d5f1229000746656a37ee59d754439fc33e36b7eef87544262e5aac374645db91b74cb507e73514003ca7a67f
0c62e3aa73 New regression testing for CVE-2018-17144, CVE-2012-2459, and CVE-2010-5137. (lucash-dev)
38bfca6bb2 Added comments referencing multiple CVEs in tests and production code. (lucash-dev)
Pull request description:
This functional test includes two scenarios that test for regressions of vulnerabilities, but they are only briefly described. There are freely available documents explaining in detail the issues, but without explicit mentions, the developer trying to maintain the code needs an additional step of digging in commit history and PR conversations to figure it out.
Added comments to explicitly mention CVE-2018-17144 and CVE-2012-2459, for more complete documentation.
This improves developer experience by making understanding the tests easier.
ACKs for top commit:
laanwj:
ACK 0c62e3aa73, checked the CVE numbers, thanks for adding documentation
Tree-SHA512: 3ee05351745193b8b959e4a25d50f25a693b2d24b0732ed53cf7d5882df40b5dd0f1877bd5c69cffb921d4a7acf9deb3cc1160b96dc730d9b5984151ad06b7c9
This commit adds comments referencing multiple CVEs both in production and test code.
CVEs covered in this commit:
CVE-2010-5137
CVE-2010-5139
CVE-2010-5141
CVE-2012-1909
CVE-2012-2459
CVE-2012-3789
CVE-2018-17144
This is a first step towards cleaning up our DoS interface - make
validation return *why* something is invalid, and let net_processing
figure out what that implies in terms of banning/disconnection/etc.
Behavior change: peers will now be banned for providing blocks
with premature coinbase spends.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
Suhas Daftuar <sdaftuar@gmail.com>
Compared with previous bans, the following changes are made:
* Txn with empty vin/vout or null prevouts move from 10 DoS
points to 100.
* Loose transactions with a dependency loop now result in a ban
instead of 10 DoS points.
* Many pre-segwit soft-fork errors now result in a ban.
Note: Transactions that violate soft-fork script flags since P2SH do not generally
result in a ban. Also, banning behavior for invalid blocks is dependent on
whether the node is validating with multiple script check threads, due to a long-
standing bug. That inconsistency is still present after this commit.
* Proof of work failure moves from 50 DoS points to a ban.
* Blocks with timestamps under MTP now result in a ban, blocks
too far in the future continue to *not* result in a ban.
* Inclusion of non-final transactions in a block now results in a
ban instead of 10 DoS points.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
Isolate the decision of whether to ban a peer to one place in the
code, rather than having it sprinkled throughout net_processing.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
Suhas Daftuar <sdaftuar@gmail.com>
John Newbery <john@johnnewbery.com>
CheckTransaction is a context-free function that does not require access
to the blockchain or mempool. Move it from src/consensus/tx_verify in
lib_server to a new unit src/consensus/tx_check in lib_consensus so that
it can be called by non-server libraries.
MarcoFalke