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

Strictly speaking, do we want to move this check above any other check that could return TX_NOT_STANDARD, so transactions with non-standard inputs are guaranteed to be put in the recentrejects filter? I don't think it matters, since upgraded peers won't relay transactions that fail other standardness checks, unless we loosen those checks for some other reason.

Yeah I agree. It doesn't seem worth the effort to figure out how to make it so that a tx that would fail in AreInputsStandard() will always have its txid go into the rejects filter -- there are a lot of checks that happen before that (including checks that don't depend on looking up the inputs first), which we'd have to move around somehow and complicate the logic. But, if this proves to be insufficient for some reason we can always try to revisit.

nit: since we're touching the line could stand to put brackets on the if statement block

Would it make sense to call this TX_NOWIT_NON_STANDARD instead or so, so that perhaps later it could be extended to cover other forms of nonstandardness that we know are not due to witness data?

Well I didn't expect that we'd ever go to the trouble of even doing this step (wtxid-relay was always the right solution, rather than trying to scatter detection of txid-is-definitely-bad failures through transaction validation), but if you think it's plausible that we would repeat this type of thing in the future, then I agree this would make sense.

We could also just as easily change this enum at that point though...

Right, of course. The name can be changed later if it becomes more expansive.

Should be 'via txid'

Thanks, fixed.

why does orphanTx.GetWitnessHash() != orphanTx.GetHash() matter?

Ok, because the wtxid is inserted above already, it would be a redundant entry for a non-witness-having tx?

Yes, inserting the same hash twice degrades the filter IIRC

Not quite. Adding the same entry is a no-op on the underlying bloom filter (since the same entry hashes to the same values). It does however increment the internal nEntriesThisGeneration counter, which means we'll be able to add fewer entries before wiping a previous generation. It effectively lowers the actual capacity of the rolling bloom filter.

Added a comment to explain this.

I think new comment add a bit of ambiguity with regards to name WITNESS_UNKNOWN. Solver() only check the witnessversion to qualify a spent output type as such. Maybe WITNESSVERSION_UNKNOWN ?

Not taking this change -- I think this is clear enough, and not worth changing the 15 places in the codebase that enum appears because of its usage in this context.

This new check seems to overrule the SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_WITNESS_PROGRAM one in VerifyWitnessProgram() ? AFAICT, we don't have test coverage for this standard script flag.

                                   AssertionError: [node 1] Expected messages "['non-mandatory-script-verify-flag (Witness version reserved for soft-fork upgrades)']" does not partially match log:
                                    - 2020-08-01T00:14:31.620579Z [msghand] received: tx (87 bytes) peer=1
                                    - 2020-08-01T00:14:31.620845Z [msghand] 52ba45c4ff1da3119475d9ac8201c4ea7018be75444de05aeda9dba22cde28d6 from peer=1 was not accepted: bad-txns-nonstandard-inputs
                                    - 2020-08-01T00:14:31.661127Z [msghand] received: ping (8 bytes) peer=1

See new test on both master/diff: https://github.com/ariard/bitcoin/commit/a97e4635ae592e582d186079911d943c58df0e54

I'm not sure I understand whether you're asking for a change, or pointing out a bug, or something else? Could you explain if anything needs to be done here?

Ah sorry, previously a segwit v1+ spend would have been rejected by this check : https://github.com/bitcoin/bitcoin/blob/34eb2362581d4d8f0bfd3baa12ba750afaf85c62/src/script/interpreter.cpp#L1552 in VerifyWitnessProgram

Now, a segwit v+1 spend is going to be rejected by this new check in AreInputsStandards. As linked test confirms if you run it on both master/diff, it's duplicating a policy rule enforcement.

Is this the first case where we duplicate policy rule checks ? I don't know, but we should avoid issues in case of future changes making them inconsistent (like verifying first with bitcoinconsensus_verify_script then broadcasting through ATMP and obtaining divergent results).

If you think it's worthy to fix, we can a) remove L1552 in script/interpreter.cpp or b) add a check against a common `MAX_STANDARD_SEGWIT_VERSION const in L1552 and L133 of script/standard.cpp or c) just add a comment around both about duplication ?

Is this the first case where we duplicate policy rule checks ?

First, it's not an identical check, because the AreInputsStandard() check is only enabled on mainnet and not testnet, via the fRequireStandard flag also needing to be set. Whereas the script flag is enabled on all networks.

Second, I think I'd be pretty surprised if anyone has a mental model of validation where they were certain that policy rule enforcement is not potentially duplicated: the validation rules are so complex and the checks so verbose that we generally tend to take a belt-and-suspenders approach, rather than trying to strictly ensure that any given check only happens once. If anyone has an understanding that any given transaction policy failure can only happen once in our code, I'd be curious to learn more about how we can be certain of this!

So this doesn't strike me as problematic; moreover I think trying to change validation code in order to ensure there is no duplication is risky and has no benefit. So I've instead added a comment pointing out that the check might be duplicative.

If I remove this reason parameter, then this test passes on master (i.e. I don't think the line below actually tests any functional change).

Good catch. Increasing the wait to 5 seconds seems to fix it, probably because of INV delay logic? 2 seconds isn't enough time for the INV to get sent out by the node.

I wonder if having a negative log line is better for each negative test case so we don't have to guess what the time required is.

You could use setmocktime to move time forward and force the node to send out getdata requests.

Ok, I'd suggest just doing 5 seconds for now for both instances of announce_tx_and_wait_for_getdata success=False in this test.

Changed this to 5 seconds.

For my understanding, is there any reason to not use the existing const orphanHash local variable, e.g. (without the assert)

+                assert(orphanHash == orphanTx.GetHash());
-                if (orphan_state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && orphanTx.GetWitnessHash() != orphanTx.GetHash()) {
+                if (orphan_state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && orphanTx.GetWitnessHash() != orphanHash) {
                     // We only add the txid if it differs from the wtxid, to
                     // avoid wasting entries in the rolling bloom filter.
-                    recentRejects->insert(orphanTx.GetHash());
+                    recentRejects->insert(orphanHash);

Similarly, any reason to not use the existing local variables txid and wtxid that are set next to tx; all three are const. Tested running this with asserts:

-                if (state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && tx.GetWitnessHash() != tx.GetHash()) {
-                    recentRejects->insert(tx.GetHash());
+                if (state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && wtxid != txid) {
+                    recentRejects->insert(txid);

I don't like that this is testing transaction acceptance by relying on our transaction download logic. It means that changes to transaction download (eg #19184) may cause unrelated tests to fail, or (perhaps worse) make it such that tests are no longer actually testing what we want them to test.

Unfortunately, we have no way to directly query a node's recent rejects filter, so we do need to test this indirectly somehow. Here's an alternative that uses the debug log to determine why the transaction has been rejected. It's not ideal, but I think it's better than testing via a separate subcomponent (tx download):

diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py
index 519fb0438f..e0c671f55d 100755
--- a/test/functional/p2p_segwit.py
+++ b/test/functional/p2p_segwit.py
@@ -123,13 +123,14 @@ def get_virtual_size(witness_block):
     vsize = int((3 * base_size + total_size + 3) / 4)
     return vsize
 
-def test_transaction_acceptance(node, p2p, tx, with_witness, accepted, reason=None):
+def test_transaction_acceptance(node, p2p, tx, with_witness, accepted, reason=None, non_reason=None):
     """Send a transaction to the node and check that it's accepted to the mempool
 
     - Submit the transaction over the p2p interface
     - use the getrawmempool rpc to check for acceptance."""
     reason = [reason] if reason else []
-    with node.assert_debug_log(expected_msgs=reason):
+    non_reason = [non_reason] if non_reason else []
+    with node.assert_debug_log(expected_msgs=reason, unexpected_msgs=non_reason):
         p2p.send_and_ping(msg_tx(tx) if with_witness else msg_no_witness_tx(tx))
         assert_equal(tx.hash in node.getrawmempool(), accepted)
 
@@ -1418,7 +1419,7 @@ class SegWitTest(BitcoinTestFramework):
         temp_utxo.pop()  # last entry in temp_utxo was the output we just spent
         temp_utxo.append(UTXO(tx2.sha256, 0, tx2.vout[0].nValue))
 
-        # Spend everything in temp_utxo into an segwit v1 output.
+        # Spend everything in temp_utxo into a segwit v1 output.
         tx3 = CTransaction()
         total_value = 0
         for i in temp_utxo:
@@ -1433,8 +1434,9 @@ class SegWitTest(BitcoinTestFramework):
         # making sure the txid is added to the reject filter
         self.std_node.announce_tx_and_wait_for_getdata(tx3)
         test_transaction_acceptance(self.nodes[1], self.std_node, tx3, with_witness=True, accepted=False, reason="bad-txns-nonstandard-inputs")
-        # Now the node will no longer ask for getdata of this transaction when advertised by same txid
-        self.std_node.announce_tx_and_wait_for_getdata(tx3, timeout=5, success=False)
+        # If we send the transaction again, it'll be rejected by the recent
+        # rejects filter before we try to accept it to mempool
+        test_transaction_acceptance(self.nodes[1], self.std_node, tx3, with_witness=True, accepted=False, non_reason="bad-txns-nonstandard-inputs")
 
         # Spending a higher version witness output is not allowed by policy,
         # even with fRequireStandard=false.

Unfortunately, we have no way to directly query a node's recent rejects filter

Would it be better to have a ~unit~ test that calls ATMP and then checks that the txid made it into the reject filter?

I'm not sure if that counts as a unit test? ATMP and the reject filter are in different components.

Not sure it's "better", I still think the best thing would be to add log lines in case of rejection due to recentRejects and scan for that line in logs.

I still think the best thing would be to add log lines in case of rejection due to recentRejects and scan for that line in logs.

This seems reasonable, and very easy to implement.

After some noodling/test writing I actually think a simple timeout average_wait_for_typical_getdata<<n<<GETDATA_TX_INTERVAL is best, or a "unit" test.

I think 5 seconds is ok for now, someone can replace later with something better.

I actually think a simple timeout

Can you explain why you think that? (I'm not disagreeing, just trying to understand why you think relying on tx download is a better test for this tx acceptance change).

There seems to be some confusion around the concept of unit tests here. This is higher-level functionality spread across two different components (validation and net_processing). The only testing framework that we have that encompasses those two components is the functional test framework. As far as I'm aware, we have cpp tests for net_processing (eg denialofservice_tests.cpp) and validation (eg validation_block_tests.cpp), but not tests that span both.

Generally, the concept of unit tests is to test units of code in isolation (eg here we could test that validation returns TX_INPUTS_NOT_STANDARD when the inputs are non-standard). Functional test test higher-level system functionality (eg we don't redownload or revalidate transactions which have non-standard inputs).

I was mostly annoyed that the various failure cases have to be handled on a case by case basis. One case is AlreadyHave, the other is not but the getdata is not sent because another peer has already had it requested. I'd rather write something that covers both properly.