p2p: Clarify control flow in ProcessMessage #13946

1. 
   MarcoFalke commented at 11:59 am on August 12, 2018: member

   ProcessMessage is effectively a massive switch case construct. In the past there were attempts to clarify the control flow in ProcessMessage() by moving each case into a separate static function (see #9608). It was closed because it wasn’t clear if moving each case into a function was the right approach. Though, we can quasi treat each case as a function by adding a return statement to each case. (Can be seen as a continuation of bugfix #13162)

   This patch does exactly that.

   Also note that this patch is a subset of previous approaches such as #9608 and #10145.

   Review suggestion: git diff HEAD~ --function-context

2. MarcoFalke added the label Refactoring on Aug 12, 2018
3. MarcoFalke added the label P2P on Aug 12, 2018
4. MarcoFalke force-pushed on Aug 12, 2018
5. 
   practicalswift commented at 2:45 pm on August 12, 2018: contributor

   utACK fa37c5b662f1aeeaa53e60377979156e84d550b4

   Clarification is good

6. 
   domob1812 commented at 3:52 pm on August 12, 2018: contributor

   utACK fa37c5b662f1aeeaa53e60377979156e84d550b4

   Seems like a reasonable clarification with minimal changes.

7. 
   DrahtBot commented at 5:03 pm on August 12, 2018: member

   • #14032 (Add p2p layer encryption with ECDH/ChaCha20Poly1305 by jonasschnelli)
   • #13947 (Dandelion transaction relay (BIP 156) by MarcoFalke)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

8. 
   sipa commented at 5:26 pm on August 12, 2018: member

   Any reason not to drop all the elses in between the different options?
9. p2p: Clarify control flow in ProcessMessage() fa6c3dea42
10. MarcoFalke force-pushed on Aug 12, 2018
11. 
    MarcoFalke commented at 7:13 pm on August 12, 2018: member

    Made everything early-return as requested by @sipa
12. 
    Empact commented at 1:25 am on August 13, 2018: member

    utACK fa6c3de

    nit: This block would be a bit easier to parse if the returns were one level up: https://github.com/bitcoin/bitcoin/blob/0df7a6c13ec484f9f31cfe3ed3039098a9bef124/src/net_processing.cpp#L1585-L1594

    nit: Not necessarily to be fixed in this PR, but I noticed that due to !fImporting && !fReindex, CMPCTBLOCK, BLOCKTXN, HEADERS, and BLOCK messages will be treated with LogPrint(BCLog::NET, "Unknown command \"%s\" from peer=%d\n" during import or reindex. The log message could be more informative.

13. 
    practicalswift commented at 7:17 am on August 13, 2018: contributor

    utACK fa6c3dea420b6c50c164ccc34f4e9e8a7d9a8022
14. in src/net_processing.cpp:2380 in fa6c3dea42

    2373@@ -2384,10 +2374,10 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
    2374                 Misbehaving(pfrom->GetId(), nDoS);
    2375             }
    2376         }
    2377+        return true;
    2378     }
    2379 
    2380-
    2381-    else if (strCommand == NetMsgType::CMPCTBLOCK && !fImporting && !fReindex) // Ignore blocks received while importing
    2382+    if (strCommand == NetMsgType::CMPCTBLOCK && !fImporting && !fReindex) // Ignore blocks received while importing
    

    ---

    ---

    

    jnewbery commented at 5:26 pm on August 24, 2018:

    May be out of scope for this PR, but I think it’d be better for these conditionals to be nested:

    0if (strCommand == NetMsgType::CMPCTBLOCK) {
    1    if (!fImporting && !fReindex) { // Ignore blocks received while importing
    2        ...
    3    }
    4    // could optionally log in an else branch here
    5    return true;
    6}
    

    for a couple of reasons:

    • it prevents compact blocks from being logged as “Unknown command” if received during import/reindex
    • it maintains your new control flow of each message type returning after its own code block.

    Same for BLOCKTXN, HEADERS and BLOCK message code blocks below.

    ---

    

    MarcoFalke commented at 5:30 pm on August 24, 2018:

    Indeed out of scope, as this is not refactoring, but bugfixing-like territory.

    ---

    

    jnewbery commented at 5:58 pm on August 24, 2018:

    ok
15. 
    jnewbery commented at 5:27 pm on August 24, 2018: member

    utACK fa6c3dea420b6c50c164ccc34f4e9e8a7d9a8022

    one comment inline.

16. 
    laanwj commented at 4:18 pm on August 25, 2018: member

    I’m not sure this is much better with the early returns. though, this does untangle the case and makes it easier to split up the function and move to dispatching based on say, a hash table, in the future, so it’s a step in the right direction.

    utACK fa6c3dea420b6c50c164ccc34f4e9e8a7d9a8022

17. laanwj merged this on Aug 25, 2018
18. laanwj closed this on Aug 25, 2018

    ---

19. laanwj referenced this in commit 021dce935a on Aug 25, 2018
20. MarcoFalke deleted the branch on Aug 25, 2018
21. 
    JeremyRubin commented at 6:46 pm on September 16, 2018: contributor

    post-merge utack, good incremental step.
22. PastaPastaPasta referenced this in commit c261173669 on Jan 25, 2020
23. UdjinM6 referenced this in commit 31066138c0 on Jan 31, 2020
24. UdjinM6 referenced this in commit e5e3572e9d on Feb 3, 2020
25. MIPPL referenced this in commit 3311e33aa0 on May 15, 2020
26. FornaxA referenced this in commit fa74145cee on Jul 6, 2020
27. ckti referenced this in commit 77002d0909 on Mar 28, 2021
28. MarcoFalke locked this on Sep 8, 2021

Contributors
MarcoFalke practicalswift domob1812 DrahtBot sipa Empact jnewbery laanwj JeremyRubin

Labels
Refactoring P2P