init: avoid unsetting service bits from `nLocalServices` #25887

1. 
   theStack commented at 12:32 PM on August 20, 2022: contributor

   This PR is a late follow-up to the review club session about the PR "Default to NODE_WITNESS in nLocalServices" (#21090):

   17:32 <lightlike> hmm, if we are in pruned mode, we first set NODE_NETWORK and then unset it later in init.cpp. that seems a bit strange.
   ...
   17:33 <jnewbery> lightlike: ah yes, you're right. That does seem a bit messy.
   

   Rather than setting the service bit NODE_NETWORK first and then unset it (if in fPruneMode), start with the bare minimum flags that we always serve and only add NODE_NETWORK if we are running as a non-pruned node. This seems to be a more logical approach than currently on master.

2. in src/init.cpp:1522 in 03f23332b5 outdated

   1518 | @@ -1519,11 +1519,12 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
   1519 |  
   1520 |      // ********************************************************* Step 10: data directory maintenance
   1521 |  
   1522 | -    // if pruning, unset the service bit and perform the initial blockstore prune
   1523 | -    // after any wallet rescanning has taken place.
   1524 | -    if (fPruneMode) {
   1525 | -        LogPrintf("Unsetting NODE_NETWORK on prune mode\n");
   1526 | -        nLocalServices = ServiceFlags(nLocalServices & ~NODE_NETWORK);
   1527 | +    if (!fPruneMode) {
   

   ---

   ---

   

   MarcoFalke commented at 12:46 PM on August 20, 2022:

   nit: Generally it is recommended if the if has two branches to not invert the condition to avoid a double negative in the else branch

   ---

   

   theStack commented at 12:55 PM on August 20, 2022:

   Thanks, fixed that by keeping the original condition and setting the flag in the else-branch instead.

3. theStack force-pushed on Aug 20, 2022
4. in src/init.cpp:1524 in a681bb33d8 outdated

   1518 | @@ -1519,18 +1519,19 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
   1519 |  
   1520 |      // ********************************************************* Step 10: data directory maintenance
   1521 |  
   1522 | -    // if pruning, unset the service bit and perform the initial blockstore prune
   1523 | +    // if pruning, perform the initial blockstore prune
   1524 |      // after any wallet rescanning has taken place.
   1525 |      if (fPruneMode) {
   

   ---

   ---

   

   stickies-v commented at 5:59 PM on August 20, 2022:

   I think you can reduce the nesting now:

       if (fPruneMode && !fReindex) {
   

   ---

   

   theStack commented at 8:33 PM on August 20, 2022:

   Note that this change would change the logic considering the else-branch, i.e. NODE_NETWORK would wrongly be set if both fPruneMode and fReindex are set to true.

   ---

   

   stickies-v commented at 9:57 PM on August 20, 2022:

   Ah yes you're right, that wouldn't work.

5. 
   stickies-v commented at 6:02 PM on August 20, 2022: contributor

   Approach ACK a681bb33d8645acf9e7d4f007c4835f9a39aaae0

   init.cpp::nLocalServices doesn't get exposed until at the end of AppInitMain() where the flag is now set, so I don't see how this can lead to any behaviour change (except for the different logging)

6. in src/init.cpp:1525 in a681bb33d8 outdated

   1518 | @@ -1519,18 +1519,19 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
   1519 |  
   1520 |      // ********************************************************* Step 10: data directory maintenance
   1521 |  
   1522 | -    // if pruning, unset the service bit and perform the initial blockstore prune
   1523 | +    // if pruning, perform the initial blockstore prune
   1524 |      // after any wallet rescanning has taken place.
   1525 |      if (fPruneMode) {
   1526 | -        LogPrintf("Unsetting NODE_NETWORK on prune mode\n");
   

   ---

   ---

   

   MarcoFalke commented at 7:35 PM on August 20, 2022:

   Changing the logging isn't refactoring, so I think it should come with a rationale and without a "refactor" label

   ---

   

   theStack commented at 8:40 PM on August 20, 2022:

   Agree, removed the "refactor" tag from both the PR description and the commit. There is no deeper rationale than the one already given in the PR description (i.e. "it's the more obvious and less confusing way").

7. theStack renamed this:
   init: refactor: avoid unsetting service bits from `nLocalServices`
   init: avoid unsetting service bits from `nLocalServices`
   on Aug 20, 2022
8. init: avoid unsetting service bits from `nLocalServices`

   Rather than setting the service bit `NODE_NETWORK` first and then unset
   it, start out the bare minimum flags that every node serves and only add
   `NODE_NETWORK` if we are running as a non-pruned node.

   1b5bec78e9
9. theStack force-pushed on Aug 20, 2022
10. stickies-v approved
11. 
    stickies-v commented at 4:00 PM on August 21, 2022: contributor

    ACK 1b5bec78e942473b6ffac4004b1cf6d535fd2892

12. 
    naumenkogs commented at 7:44 AM on August 24, 2022: member

    ACK 1b5bec78e942473b6ffac4004b1cf6d535fd2892

13. fanquake requested review from dergoegge on Aug 24, 2022
14. fanquake requested review from mzumsande on Aug 24, 2022
15. LarryRuane approved
16. 
    LarryRuane commented at 8:59 PM on August 31, 2022: contributor

    ACK 1b5bec78e942473b6ffac4004b1cf6d535fd2892

17. MarcoFalke merged this on Sep 1, 2022
18. MarcoFalke closed this on Sep 1, 2022

    ---

19. theStack deleted the branch on Sep 1, 2022
20. sidhujag referenced this in commit 543cbccfcd on Sep 1, 2022
21. bitcoin locked this on Sep 1, 2023

Contributors

theStackMarcoFalkestickies-vnaumenkogsLarryRuane

Review Requested
dergoeggemzumsande