Add txids with non-standard inputs to reject filter

[sdaftuar]

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.

[sdaftuar]

I need to test this still myself, but I wanted to open this as a potential alternative to feeling the need to backport #18044 (as @jnewbery has proposed in #19606) in order to protect older software from wasting bandwidth in the event of taproot activation in the future.

I have a branch where I backported this patch to 0.20 here: https://github.com/sdaftuar/bitcoin/tree/2020-07-reject-unknown-wit-0.20.

Note that this PR should be independently useful to mitigate an issue @ariard brought up here: #18044 (comment)

[jnewbery]

Concept ACK. Looks like a good change.

I think we might independently want to backport wtxid relay. It's really not that difficult of a backport, and we eventually want wtxid to be the standard way to relay txs and getting it into v0.20 would help with that.

[JeremyRubin]

Concept ACK and lite code review ACK.

It seems to me that this change won't help that much in the future upgrade case because even though this fixes witness versions things like leaf versions in taproot (which may be upgraded more often) will require a witness dependent check, and can't be txid reject filtered, unless we made it a policy that transactions spending taproots that mix and match leaf versions are non-standard (but only detectable in the case where an previous attempted witness is nonstandard).

[sdaftuar]

It seems to me that this change won't help that much in the future upgrade case [...]

@JeremyRubin I agree, this is a bandaid to help this particular scenario. Going forward, wtxid-relay + a change to the p2p protocol for announcing/requesting unconfirmed ancestors of a transaction by wtxid seems like the right way to solve this type of problem more generally.

[sipa]

@sdaftuar @naumenkogs Random idea about this: post-Erlay it may make sense to have a message "i do not have have dependencies of txid txid", at which point both sides add all known direct and indirect unconfirmed parents of txid they know of to their to-be-broadcast set; if erlay is in effect, the overlap will get cancelled out automatically.

[sdaftuar]

Updated some comments and also used this same logic when processing orphans.

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• Tidy up ProcessOrphanTx #19498 (Tidy up ProcessOrphanTx by jnewbery)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[jnewbery]

Code changes look good to me. One suggested change to a comment inline.

I think a functional test would be useful here. This change depends on the ordering of checks in ATMP, so there could potentially be subtle interactions there. Proving this out with a test for txid-relay peers and orphan fetching would be great.

[sdaftuar]

Happy to include a functional test if anyone is able to put one together for this.

[JeremyRubin]

@instagibbs might?

[instagibbs]

I think this deserves a test and shouldn't be too bad. I'll take a crack at it this weekend.

[luke-jr]

utACK

[sipa]

Code review ACK acdd7f3. A test would be nice.

[instagibbs]

Yeah might be worth noting that's what it's for. Took me a bit of work.

…

On Fri, Jul 31, 2020, 5:16 PM Luke Dashjr ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/net_processing.cpp <#19620 (comment)>: > @@ -2053,6 +2060,17 @@ void static ProcessOrphanTx(CConnman& connman, CTxMemPool& mempool, std::set<uin // if we start doing this too early. assert(recentRejects); recentRejects->insert(orphanTx.GetWitnessHash()); + // If the transaction failed for TX_INPUTS_NOT_STANDARD, + // then we know that the witness was irrelevant to the policy + // failure, since this check depends only on the txid + // (the scriptPubKey being spent is covered by the txid). + // Add the txid to the reject filter to prevent repeated + // processing of this transaction in the event that child + // transactions are later received (resulting in + // parent-fetching by txid via the orphan-handling logic). + if (orphan_state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && orphanTx.GetWitnessHash() != orphanTx.GetHash()) { Yes, inserting the same hash twice degrades the filter IIRC — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19620 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABMAFUZZCX3AQ6GZCIXYOX3R6MYCNANCNFSM4PLXHWJQ> .

[instagibbs]

Turns out there's already a nice natural spot for the test, feel free to snipe: instagibbs@57f782d

[instagibbs]

ACK ada658f

[sdaftuar]

Thanks @instagibbs! I added your test change here.

[jnewbery]

Code review ACK 9f88ded

[ajtowns]

ACK 9f88ded - code review