Fix a few cases where messages were sent after requested disconnect #8862

1. 
   theuni commented at 11:21 pm on October 1, 2016: member

   Noticed these in #8708, which turns them into crashes. PRing separately here as I believe it needs a 0.13.1 backport.
2. net: fix a few cases where messages were sent rather than dropped upon disconnection

   75ead758 turned these into crashes in the event of a handshake failure, most
   notably when a peer does not offer the expected services.
   
   There are likely other cases that these assertions will find as well.

   905bc68d05
3. fanquake added the label Needs backport on Oct 1, 2016
4. fanquake added this to the milestone 0.13.1 on Oct 1, 2016
5. MarcoFalke added the label P2P on Oct 2, 2016
6. 
   sipa commented at 0:27 am on October 3, 2016: member

   Looks harmless at worst. utACK.
7. 
   gmaxwell commented at 3:33 am on October 3, 2016: contributor

   utACK. Seems like it would be easy to introduce more of these issues in the future, this fix seems fine though we might want to consider something more comprehensive for later.
8. 
   theuni commented at 4:42 am on October 3, 2016: member

   @gmaxwell Agreed. As mentioned above, #8708 adds an assert to catch a specific form of this case (sending a post-handshake message before the handshake is complete), so a more comprehensive fix will be required for sure, and is coming as part of the disconnection logic cleanup. These two were just added because they’re low-hanging fruit and are easily backported, and because they were the only ones actually observed while testing.
9. 
   rebroad commented at 5:13 am on October 3, 2016: contributor

   Why not put this logic at the start of SendMessages - where it checks if Version is non zero?
10. 
    theuni commented at 5:31 am on October 3, 2016: member

    @rebroad I believe that may inhibit the send of some valid reject messages. Generally speaking, I don’t like the idea of relying on a stateful difference in SendMessages, as I think the goal should be generally async responses as opposed to the current model of “check all connections for all eligible broadcasts all the time”.

    It’s a good makeshift suggestion though, I’ll have a look.

11. 
    laanwj commented at 8:53 am on October 3, 2016: member

    utACK

    Noticed these in #8708, which turns them into crashes

    Ouch. I hope we can avoid this in a consistent way. To avoid DoSes, in network code it’s always preferable to continue and (if its state is messed up) just boot a peer to crashing. Even in case of bugs.

12. laanwj merged this on Oct 4, 2016
13. laanwj closed this on Oct 4, 2016

    ---

14. laanwj referenced this in commit d93f0c6184 on Oct 4, 2016
15. laanwj referenced this in commit 7ae6242960 on Oct 13, 2016
16. laanwj removed the label Needs backport on Oct 13, 2016
17. codablock referenced this in commit f6dfb8478d on Sep 19, 2017
18. codablock referenced this in commit f45adb5619 on Jan 12, 2018
19. lateminer referenced this in commit 771b74ab4e on Oct 24, 2018
20. andvgal referenced this in commit fca10d82cd on Jan 6, 2019
21. random-zebra referenced this in commit 5fcad0c139 on Aug 2, 2020
22. MarcoFalke locked this on Sep 8, 2021

Contributors
theuni sipa gmaxwell rebroad laanwj

Labels
P2P

Milestone
0.13.1