test: maxuploadtarget: check for mempool msg disconnect if limit is reached, improve existing test coverage #28996

theStack commented at 6:13 PM on December 4, 2023: contributor

This PR improves existing and adds new test coverage for the -maxuploadtarget mechanism (feature_maxuploadtarget.py) in the following ways, one commit each:

verify the uploadtarget state via the getnettotals RPC (uploadtarget result field): https://github.com/bitcoin/bitcoin/blob/160d23677ad799cf9b493eaa923b2ac080c3fb8e/src/rpc/net.cpp#L581-L582 Note that reaching the total limit (target_reached == True) always implies that the historical blocks serving limits is also reached (serve_historical_blocks == False), i.e. it's impossible that both flags are set to True.
check for peer's specific disconnect reason (in this case, "historical block serving limit reached, disconnect peer"): https://github.com/bitcoin/bitcoin/blob/160d23677ad799cf9b493eaa923b2ac080c3fb8e/src/net_processing.cpp#L2272-L2280
add a test for a peer disconnect if the uploadtarget is reached and a mempool message is received (if bloom filters are enabled): https://github.com/bitcoin/bitcoin/blob/160d23677ad799cf9b493eaa923b2ac080c3fb8e/src/net_processing.cpp#L4755-L4763 Note that another reason for disconnect after receiving a MEMPOOL msg of a peer is if bloom filters are disabled on the node. This case is already covered in the functional test p2p_nobloomfilter_messages.py.

DrahtBot commented at 6:14 PM on December 4, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, sr-gi, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot added the label Tests on Dec 4, 2023

DrahtBot added the label CI failed on Jan 16, 2024

theStack force-pushed on Jan 22, 2024

DrahtBot removed the label CI failed on Jan 22, 2024

DrahtBot added the label CI failed on Jan 31, 2024

test: verify `-maxuploadtarget` limit state via `getnettotals` RPC result 73d7372115

test: check for specific disconnect reasons in feature_maxuploadtarget.py

This ensures that the disconnect happens for the expected reason and
also makes it easier to navigate between implementation and test code,
i.e. both the questions "do we have test coverage for this disconnect?"
(from an implementation reader's perspective) and "where is the code
handling this disconnect?" (from a test reader's perspective) can be
answered simply by grep-ping the corresponding debug message.

dd5cf38818

test: check that mempool msgs lead to disconnect if uploadtarget is reached

Note that another reason for disconnect after receiving a MEMPOOL msg of a peer
is if bloom filters are disabled on the node. This case is covered in the
functional test `p2p_nobloomfilter_messages.py`.

b58f009d95

theStack force-pushed on Feb 5, 2024

DrahtBot removed the label CI failed on Feb 5, 2024

maflcko commented at 9:22 AM on February 6, 2024: member

lgtm ACK b58f009d9585aab775998644de07e27e2a4a8045

glozow requested review from sr-gi on Feb 6, 2024

in test/functional/feature_maxuploadtarget.py:161 in b58f009d95

 160 | @@ -139,23 +161,32 @@ def run_test(self):
 161 |          p2p_conns[2].sync_with_ping()

sr-gi commented at 6:38 PM on February 6, 2024:

This is sort of unrelated, but it may be worth bringing it in given we are updating this test anyway. The line right above this mentions that the time is advanced by 24 hours and then the checks are performed again. However, the time is actually advance 48 hours: it was previously set to 48 hours in the past, and now it's set to the current time.

I guess it wouldn't hurt to also define SECONDS_PER_DAY = 60*60*24 for readability throughout the test.

in test/functional/feature_maxuploadtarget.py:181 in b58f009d95

 178 |          peer = self.nodes[0].add_p2p_connection(TestP2PConn())
 179 |  
 180 | +        # Sending mempool message shouldn't disconnect peer, as total limit isn't reached yet
 181 | +        peer.send_and_ping(msg_mempool())
 182 | +
 183 |          #retrieve 20 blocks which should be enough to break the 1MB limit

sr-gi commented at 6:57 PM on February 6, 2024:

nit: whitespace + caps here

PS: idk why this was pinned here, I meant L181

sr-gi commented at 7:01 PM on February 6, 2024: member

tACK b58f009

achow101 commented at 12:22 AM on February 9, 2024: member

ACK b58f009d9585aab775998644de07e27e2a4a8045

achow101 merged this on Feb 9, 2024

achow101 closed this on Feb 9, 2024

theStack deleted the branch on Feb 9, 2024

bitcoin locked this on Feb 8, 2025