net processing: Remove dropmessagestest #20747

pull jnewbery wants to merge 1 commits into bitcoin:master from jnewbery:2020-12-dropmessagestest changing 2 files +0 −6

jnewbery commented at 5:53 PM on December 22, 2020: member

-dropmessagestest is a command line option that causes 1 in n received messages to be dropped. The Bitcoin P2P protocol is stateful and in general cannot handle messages being dropped. Dropped version/verack/ping/pong messages will cause the connection to time out and be torn down. Other dropped messages may also cause the peer to believe that the peer has stalled and tear down the connection.

It seems difficult to uncover any actual issues with -dropmessagestest, and any coverage that could be generated would probably be easier to trigger with fuzz testing.

[net processing] Remove dropmessagestest

-dropmessagestest is a command line option that causes 1 in n received
messages to be dropped. The Bitcoin P2P protocol is stateful and in
general cannot handle messages being dropped. Dropped
version/verack/ping/pong messages will cause the connection to time out
and be torn down. Other dropped messages may also cause the peer to
believe that the peer has stalled and tear down the connection.

It seems difficult to uncover any actual issues with -dropmessagestest,
and any coverage that could be generated would probably be easier to
trigger with fuzz testing.

176325a5a4

jnewbery added the label P2P on Dec 22, 2020
MarcoFalke commented at 6:21 PM on December 22, 2020: member

cr ACK 176325a5a47befe32d480b3dc206dd0e64e04b21
MarcoFalke added the label Tests on Dec 22, 2020
practicalswift commented at 7:25 PM on December 22, 2020: contributor

cr ACK 176325a5a47befe32d480b3dc206dd0e64e04b21

Thanks for pruning options.

More generally: Concept ACK on removing obsolete/unused options.

Less is sometimes more when it comes to the number of knobs presented to end-users :)
amitiuttarwar approved
amitiuttarwar commented at 12:29 AM on December 23, 2020: contributor
ACK 176325a5a47befe32d480b3dc206dd0e64e04b21
- agree with the rationale in OP
- sanity checked that this arg isn't used anywhere
- did some code archeology because I was curious why this was introduced, looks like it can be traced back to satoshi code
dhruv commented at 7:00 PM on December 23, 2020: member

cr ACK 176325a

Checked that the only use of -dropmessagestest is also removed in this PR. I can't think of any use cases where this is useful as the non-determinism makes it hard to test resulting behavior.
fanquake merged this on Dec 24, 2020
fanquake closed this on Dec 24, 2020
sidhujag referenced this in commit cd3bb41ae0 on Dec 24, 2020
jnewbery deleted the branch on Dec 24, 2020
Fabcien referenced this in commit 317ba5df7a on Feb 1, 2022
DrahtBot locked this on Feb 15, 2022

Contributors

Labels