test: check duplicate wtxidrelay is counted once #36505

Sending wtxidrelay twice before verack must not increment m_wtxid_relay_peers twice, otherwise the consistency check in FinalizeNode fails. This PR adds a test case for it as well as it kills the following mutant (https://mutanthub.space/mutants/3756):

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index beec44f..a050f08 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -4200,5 +4200,5 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
         if (pfrom.GetCommonVersion() >= WTXID_RELAY_VERSION) {
+            m_wtxid_relay_peers++;
             if (!peer.m_wtxid_relay) {
                 peer.m_wtxid_relay = true;
-                m_wtxid_relay_peers++;
             } else {

This test case basically connects an inbound peer and sends its version message, but not verack yet. Sends wtxidrelay twice. The second one should be ignored as a duplicate, and the test checks the peer isn't disconnected. And then, calls FinalizeNode. When the last peer is removed, PeerManagerImpl checks that m_wtxid_relay_peers == 0.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

External sites

• Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36505
• Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36505.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36505#author-ralph

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline and AI policy for information on the review process. A summary of reviews will appear here.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

The patch looks a bit odd and doesn't apply?

git apply  mutant-3756.patch 
mutant-3756.patch:16: trailing whitespace.
         
error: patch failed: src/net_processing.cpp:4200
error: src/net_processing.cpp: patch does not apply

The correct patch would be:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index beec44f..a050f08 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -4200,5 +4200,5 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
         if (pfrom.GetCommonVersion() >= WTXID_RELAY_VERSION) {
+            m_wtxid_relay_peers++;
             if (!peer.m_wtxid_relay) {
                 peer.m_wtxid_relay = true;
-                m_wtxid_relay_peers++;
             } else {

The patch looks a bit odd and doesn't apply?

git apply  mutant-3756.patch 
mutant-3756.patch:16: trailing whitespace.
         
error: patch failed: src/net_processing.cpp:4200
error: src/net_processing.cpp: patch does not apply

The correct patch would be:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index beec44f..a050f08 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -4200,5 +4200,5 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
         if (pfrom.GetCommonVersion() >= WTXID_RELAY_VERSION) {
+            m_wtxid_relay_peers++;
             if (!peer.m_wtxid_relay) {
                 peer.m_wtxid_relay = true;
-                m_wtxid_relay_peers++;
             } else {

Yes, sorry! Just updated the description.

Yes, sorry! Just updated the description.

I was also thinking about the mutant website, which ships the broken patch.

Also, the mutant is already killed by the tests:

$ FUZZ=process_messages ./bld-cmake/bin/fuzz ../b-c-qa-assets/fuzz_corpora/process_messages/199f4539d8e99f3d42833667373c71425656f490 
fuzz: net_processing.cpp:1830: virtual void (anonymous namespace)::PeerManagerImpl::FinalizeNode(const CNode &): Assertion `m_wtxid_relay_peers == 0' failed.
Error processing input "../b-c-qa-assets/fuzz_corpora/process_messages/199f4539d8e99f3d42833667373c71425656f490"

Of course, no objection. I just wonder generally, considering that modern LLM bots can be used to write basically any test (even if they are redundant), which tests we should be aiming for and which tests the review effort is well spent on.

I'd say exact script behavior like #34919 is probably a unit test. Whereas this can be a fuzz test? But again, no objection. Seems fine to add this.

I was also thinking about the mutant website, which ships the broken patch.

Yes, let me check if that was a bug when generating the patch (should be easy to fix) or my fault when uploading the mutant to the platform.

Of course, no objection. I just wonder generally, considering that modern LLM bots can be used to write basically any test (even if they are redundant), which tests we should be aiming for and which tests the review effort is well spent on.

I'd say exact script behavior like #34919 is probably a unit test. Whereas this can be a fuzz test? But again, no objection. Seems fine to add this.

I probably missed this fuzz target when testing the mutant, sorry! In my opinion, this PR can be closed, we don't need to add this unit test, since the mutant is killed by an input from our public corpora and running the target only with the corpora is good and fast enough for our analysis.