net: don’t send feefilter messages before the version handshake is complete #9117

1. 
   theuni commented at 6:19 pm on November 9, 2016: member

   Caused by changes in #8708. Same fix as #8862.

   This is a quick fix, I’ll PR a small cleanup of the disconnect logic so that these cases don’t continue to crop up.

   Edit: Forgot to mention, this currently asserts due to #8708, which was intended to point out issues like this one. So this should be considered a crash-fix.

2. net: don't send feefilter messages before the version handshake is complete 46625538d6
3. 
   MarcoFalke commented at 6:21 pm on November 9, 2016: member

   utACK 46625538d674a5aaf1bcfa7c8be8a49e5a23fa9e
4. MarcoFalke added the label P2P on Nov 9, 2016
5. 
   sipa commented at 7:19 pm on November 9, 2016: member

   What does !pto->fDisconnect have to do with the version handshake?
6. 
   theuni commented at 7:41 pm on November 9, 2016: member

   @sipa in this case, the handshake started, but stopped halfway-through and set fDisconnected because the node didn’t have the services expected. I suppose I should’ve ignored that and just called this “don’t send feefilter messages if fDisconnect has been set”.

   For a more correct/complete (but untested) change, see here: https://github.com/theuni/bitcoin/commits/feefilter-assert2 .

7. 
   jonasschnelli commented at 7:39 am on November 10, 2016: contributor

   utACK 46625538d674a5aaf1bcfa7c8be8a49e5a23fa9e
8. 
   rebroad commented at 6:29 am on November 11, 2016: contributor

   I still don’t understand why we don’t simply put the fDisconnect check along with the check for nVersion != 0 then all of these little issues would not exist. The only messages we need to send once fDisconnect is set are messages directly relating to the disconnection, e.g. the “bye” message @laanwj proposed a while back.
9. 
   theuni commented at 6:37 am on November 11, 2016: member

   @rebroad #9128 contains the complete fix to this, similar to what you’re suggesting.

   I’ll leave this open for now in case the quick fix is desired to avoid the crash while the bigger PR is in review.

10. 
    rebroad commented at 6:51 am on November 11, 2016: contributor

    @theuni oh! you got there before me! Well, #9129 contains just the fix we’re talking about, so might need less review…
11. 
    theuni commented at 6:55 am on November 11, 2016: member

    @rebroad That only stops processing if fDisconnect is already set when SendMessages is called. But with your changes, if it’s set part-way through SendMessages (as it is a few times), messages further down will now be allowed out!
12. laanwj merged this on Nov 21, 2016
13. laanwj closed this on Nov 21, 2016

    ---

14. laanwj referenced this in commit c3906403c8 on Nov 21, 2016
15. theuni added the label Needs backport on Nov 23, 2016
16. luke-jr referenced this in commit 6fe3981ac8 on Dec 2, 2016
17. laanwj removed the label Needs backport on Sep 5, 2017
18. codablock referenced this in commit afa99c41fc on Jan 15, 2018
19. lateminer referenced this in commit 793b8f55b1 on Oct 16, 2018
20. andvgal referenced this in commit b2dfdb006a on Jan 6, 2019
21. CryptoCentric referenced this in commit 23e9aea302 on Feb 24, 2019
22. MarcoFalke locked this on Sep 8, 2021

Contributors
theuni MarcoFalke sipa jonasschnelli rebroad

Labels
P2P