Add txids with non-standard inputs to reject filter #19620

sdaftuar · 2020-07-29T15:37:55Z

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 · 2020-07-29T15:46:40Z

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.

src/validation.cpp

JeremyRubin · 2020-07-29T17:44:03Z

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 · 2020-07-29T19:48:33Z

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 · 2020-07-29T19:55:03Z

@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 · 2020-07-30T21:33:44Z

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

src/consensus/validation.h

DrahtBot · 2020-07-31T04:06:21Z

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.

src/net_processing.cpp

sdaftuar · 2020-07-31T18:37:48Z

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

JeremyRubin · 2020-07-31T18:47:01Z

@instagibbs might?

instagibbs · 2020-07-31T18:51:36Z

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 · 2020-07-31T20:30:36Z

Code review ACK acdd7f3. A test would be nice.

src/validation.cpp

src/net_processing.cpp

instagibbs · 2020-07-31T21:17:29Z

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> .

src/policy/policy.cpp

instagibbs · 2020-08-03T18:40:41Z

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

instagibbs · 2020-08-03T18:41:37Z

ACK ada658f

sdaftuar · 2020-08-03T19:07:11Z

Thanks @instagibbs! I added your test change here.

test/functional/p2p_segwit.py

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 · 2020-08-05T20:01:28Z

@jonatack Thanks for taking a look -- if I end up re-pushing I'll address the nits.

ariard · 2020-08-06T09:04:14Z

Code Review/Tested ACK 9f88ded

Thanks for the comment on duplicative checks under some flags combination, I agree that alternatives aren't worthy the burden review.

jnewbery · 2020-08-06T11:04:34Z

Code review ACK 9f88ded

ajtowns · 2020-08-06T18:31:37Z

ACK 9f88ded - code review

9f88ded test addition of unknown segwit spends to txid reject filter (Gregory Sanders) 7989901 Add txids with non-standard inputs to reject filter (Suhas Daftuar) Pull request description: 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. ACKs for top commit: ajtowns: ACK 9f88ded - code review jnewbery: Code review ACK 9f88ded ariard: Code Review/Tested ACK 9f88ded naumenkogs: utACK 9f88ded jonatack: ACK 9f88ded Tree-SHA512: 1e93c0a5b68cb432524780ffc0093db893911fdfed9e2ed17f888e59114cc75d2a07062aefad4e5ce2e87c9270886117a8abb3c78fb889c9b9f31967f1777148

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. Github-Pull: bitcoin#19620 Rebased-From: 7989901

Github-Pull: bitcoin#19620 Rebased-From: 9f88ded

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. Github-Pull: bitcoin#19620 Rebased-From: 7989901

Github-Pull: bitcoin#19620 Rebased-From: 9f88ded

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. Github-Pull: bitcoin#19620 Rebased-From: 7989901

Github-Pull: bitcoin#19620 Rebased-From: 9f88ded

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. Github-Pull: bitcoin#19620 Rebased-From: 7989901

Github-Pull: bitcoin#19620 Rebased-From: 9f88ded

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. Github-Pull: bitcoin#19620 Rebased-From: 7989901

Github-Pull: bitcoin#19620 Rebased-From: 9f88ded

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. Github-Pull: bitcoin#19620 Rebased-From: 7989901

Github-Pull: bitcoin#19620 Rebased-From: 9f88ded

52c3bec test addition of unknown segwit spends to txid reject filter (Gregory Sanders) 2ea826c Add txids with non-standard inputs to reject filter (Suhas Daftuar) Pull request description: Backport of #19620 to 0.19. ACKs for top commit: instagibbs: utACK 52c3bec jnewbery: utACK 52c3bec jonasschnelli: utACK 52c3bec Tree-SHA512: 76b52d3fb0f9d88674dd186dee611bf0a2349b17549ef7909b4b37ace5b64d4edce56d71410e7b743e7e7d18855b84ff4b555a5edac26f67786abb9a264fa256

107cf15 test addition of unknown segwit spends to txid reject filter (Gregory Sanders) 06f9c5c Add txids with non-standard inputs to reject filter (Suhas Daftuar) Pull request description: Backport of #19620 to 0.20. ACKs for top commit: instagibbs: utACK 107cf15 fjahr: utACK 107cf15 jnewbery: utACK 107cf15 Tree-SHA512: d89c75fefdb6b50f40adfcf3cfdb2a791122e4d6a5903cb553c664042963577beb97a73319f9d1cb8320d32846778233c95255812c37fb4c7b1b25da161a6595

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. Github-Pull: #19620 Rebased-From: 7989901

Github-Pull: #19620 Rebased-From: 9f88ded

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. Github-Pull: bitcoin#19620 Rebased-From: 7989901

Github-Pull: bitcoin#19620 Rebased-From: 9f88ded

Summary: Nothing in the PR's description is relevant to use, but having a special subcategory of TX_NOT_STANDARD for non-standard inputs seems reasonable. Our codebase already adds those txid to the recentRejectFilter, so most of the changes do not apply. This is a backport of [[bitcoin/bitcoin#19620 | core#19620]] bitcoin/bitcoin@7989901 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10061

jnewbery reviewed Jul 29, 2020

View reviewed changes

src/validation.cpp Show resolved Hide resolved

DrahtBot added P2P TX fees and policy Validation labels Jul 29, 2020

sdaftuar force-pushed the 2020-07-reject-unknown-wit branch from be23c2a to ba64567 Compare July 30, 2020 21:32

sipa reviewed Jul 30, 2020

View reviewed changes

src/consensus/validation.h Show resolved Hide resolved

DrahtBot mentioned this pull request Jul 31, 2020

Tidy up ProcessOrphanTx #19498

Merged

jnewbery reviewed Jul 31, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

sdaftuar force-pushed the 2020-07-reject-unknown-wit branch from ba64567 to acdd7f3 Compare July 31, 2020 18:31

luke-jr approved these changes Jul 31, 2020

View reviewed changes

instagibbs reviewed Jul 31, 2020

View reviewed changes

src/validation.cpp Show resolved Hide resolved

src/net_processing.cpp Show resolved Hide resolved

ariard reviewed Aug 1, 2020

View reviewed changes

src/policy/policy.cpp Show resolved Hide resolved

src/policy/policy.cpp Show resolved Hide resolved

sdaftuar force-pushed the 2020-07-reject-unknown-wit branch from acdd7f3 to ada658f Compare August 3, 2020 17:43

jnewbery reviewed Aug 4, 2020

View reviewed changes

test/functional/p2p_segwit.py Show resolved Hide resolved

sdaftuar and others added 2 commits August 4, 2020 13:29

test addition of unknown segwit spends to txid reject filter

9f88ded

fanquake merged commit 6d85435 into bitcoin:master Aug 6, 2020

This was referenced Aug 7, 2020

0.20: Add txids with non-standard inputs to reject filter #19680

Merged

0.19: Add txids with non-standard inputs to reject filter #19681

Merged

sdaftuar pushed a commit to sdaftuar/bitcoin that referenced this pull request Aug 7, 2020

test addition of unknown segwit spends to txid reject filter

a77c784

Github-Pull: bitcoin#19620 Rebased-From: 9f88ded

sdaftuar pushed a commit to sdaftuar/bitcoin that referenced this pull request Aug 7, 2020

test addition of unknown segwit spends to txid reject filter

107cf15

Github-Pull: bitcoin#19620 Rebased-From: 9f88ded

sdaftuar pushed a commit to sdaftuar/bitcoin that referenced this pull request Aug 7, 2020

test addition of unknown segwit spends to txid reject filter

127331e

Github-Pull: bitcoin#19620 Rebased-From: 9f88ded

sdaftuar pushed a commit to sdaftuar/bitcoin that referenced this pull request Aug 7, 2020

test addition of unknown segwit spends to txid reject filter

d670580

Github-Pull: bitcoin#19620 Rebased-From: 9f88ded

sipa mentioned this pull request Aug 7, 2020

Implement BIP 340-342 validation (Schnorr/taproot/tapscript) #17977

Closed

4 tasks

sdaftuar pushed a commit to sdaftuar/bitcoin that referenced this pull request Aug 10, 2020

test addition of unknown segwit spends to txid reject filter

568007c

Github-Pull: bitcoin#19620 Rebased-From: 9f88ded

sdaftuar pushed a commit to sdaftuar/bitcoin that referenced this pull request Aug 11, 2020

test addition of unknown segwit spends to txid reject filter

52c3bec

Github-Pull: bitcoin#19620 Rebased-From: 9f88ded

backpacker69 referenced this pull request in peercoin/peercoin Sep 8, 2020

test addition of unknown segwit spends to txid reject filter

cfc53bc

Github-Pull: #19620 Rebased-From: 9f88ded

Bushstar pushed a commit to Bushstar/omnicore that referenced this pull request Oct 21, 2020

test addition of unknown segwit spends to txid reject filter

08e3601

Github-Pull: bitcoin#19620 Rebased-From: 9f88ded

gmaxwell mentioned this pull request Nov 5, 2020

Backport wtxid orphan fetch to v0.20 #20317

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add txids with non-standard inputs to reject filter #19620

Add txids with non-standard inputs to reject filter #19620

sdaftuar commented Jul 29, 2020

sdaftuar commented Jul 29, 2020

jnewbery left a comment

JeremyRubin commented Jul 29, 2020

sdaftuar commented Jul 29, 2020 •

edited

Loading

sipa commented Jul 29, 2020 •

edited

Loading

sdaftuar commented Jul 30, 2020

DrahtBot commented Jul 31, 2020

jnewbery left a comment

sdaftuar commented Jul 31, 2020

JeremyRubin commented Jul 31, 2020

instagibbs commented Jul 31, 2020

luke-jr left a comment

sipa commented Jul 31, 2020

instagibbs commented Jul 31, 2020 via email

instagibbs commented Aug 3, 2020

instagibbs commented Aug 3, 2020

sdaftuar commented Aug 3, 2020

sdaftuar commented Aug 5, 2020

ariard commented Aug 6, 2020

jnewbery commented Aug 6, 2020

ajtowns commented Aug 6, 2020

Add txids with non-standard inputs to reject filter #19620

Add txids with non-standard inputs to reject filter #19620

Conversation

sdaftuar commented Jul 29, 2020

sdaftuar commented Jul 29, 2020

jnewbery left a comment

Choose a reason for hiding this comment

JeremyRubin commented Jul 29, 2020

sdaftuar commented Jul 29, 2020 • edited Loading

sipa commented Jul 29, 2020 • edited Loading

sdaftuar commented Jul 30, 2020

DrahtBot commented Jul 31, 2020

Conflicts

jnewbery left a comment

Choose a reason for hiding this comment

sdaftuar commented Jul 31, 2020

JeremyRubin commented Jul 31, 2020

instagibbs commented Jul 31, 2020

luke-jr left a comment

Choose a reason for hiding this comment

sipa commented Jul 31, 2020

instagibbs commented Jul 31, 2020 via email

instagibbs commented Aug 3, 2020

instagibbs commented Aug 3, 2020

sdaftuar commented Aug 3, 2020

sdaftuar commented Aug 5, 2020

ariard commented Aug 6, 2020

jnewbery commented Aug 6, 2020

ajtowns commented Aug 6, 2020

sdaftuar commented Jul 29, 2020 •

edited

Loading

sipa commented Jul 29, 2020 •

edited

Loading