kernel: Remove batchpriority from kernel library #30083

1. 
   TheCharlatan commented at 8:43 pm on May 10, 2024: contributor

   The current usage of ScheduleBatchPriority is not transparent. Once the thread scheduling is changed, it remains unchanged for the remainder of the thread’s lifetime. So move the call from ImportBlocks to the init code where it is clearer that its effect lasts for the entire lifetime of the thread.

   Users of the kernel library might not expect ImportBlocks to have an influence on the thread it is called in. Particularly since it is only a compile time option and cannot be controlled at runtime. With this patch users of the kernel library can now freely choose their own scheduling policy.

   This PR is easier reviewed with git diff --color-moved-ws=ignore-all-space --color-moved=dimmed-zebra

   ---

   This PR is part of the libbitcoinkernel project.

2. 
   DrahtBot commented at 8:43 pm on May 10, 2024: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	maflcko, ryanofsky, hebasto

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

3. DrahtBot added the label Validation on May 10, 2024
4. 
   maflcko commented at 11:31 am on May 13, 2024: member

   utACK 04ffe4061da2d0135f206032e167703772b5da78 🎪

   Signature:

   0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
   1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
   2trusted comment: utACK 04ffe4061da2d0135f206032e167703772b5da78 🎪
   3+qesV40tSRVM79Z50q9pkaw6EHghp2f2kaOTAqmPSZrbiwPdZpTUZ9LRm10oH73eRU6lRi7LFaZGDceW1rZzDg==
   

5. in src/node/blockstorage.cpp:1240 in 04ffe4061d outdated

   1286+        BlockValidationState state;
   1287+        if (!chainstate->ActivateBestChain(state, nullptr)) {
   1288+            chainman.GetNotifications().fatalError(strprintf(_("Failed to connect best block (%s)."), state.ToString()));
   1289+            return;
   1290         }
   1291-    } // End scope of ImportingNow
   

   ---

   ---

   

   ryanofsky commented at 4:34 pm on May 13, 2024:

   In commit “kernel: Remove batchpriority from kernel library” (04ffe4061da2d0135f206032e167703772b5da78)

   I think it might be useful to keep the // End scope of ImportNow comment here. If someone is adding new code at this point, it’s possible they might want to add it with importing set to false, or importing set to true, so the comment would be a reminder that the value will change at this point.

   ---

   

   TheCharlatan commented at 5:23 pm on May 13, 2024:

   Do you want to preserve the comment or the scoping? I was on the fence about this, but decided to drop the scoping after the introduced RAII wrappers for the ECC context, which don’t scope either: https://github.com/bitcoin/bitcoin/pull/29252/files#diff-dbfadb3e0332664bff298a329b1d27065d2decbbe43fd391388af18f5861c114R17

   Besides, I don’t think it is likely that his function will grow. It is finally scoped in a reasonably limited way now. If a more functionality is added, it should probably go in another function and removing the scoping as done here encourages that.

   ---

   

   ryanofsky commented at 6:52 pm on May 13, 2024:

   re: #30083 (review)

   Whatever you prefer is good. I like getting rid of the nested scope, and the current PR is fine. I was just suggesting keeping the comment as a reminder that the import variable would be change to false at that point.

6. ryanofsky approved
7. 
   ryanofsky commented at 4:35 pm on May 13, 2024: contributor

   Code review ACK 04ffe4061da2d0135f206032e167703772b5da78, this is an obvious improvement
8. fanquake requested review from theuni on May 14, 2024
9. kernel: Remove batchpriority from kernel library

   The current usage of ScheduleBatchPriority is not transparent. Once the
   thread scheduling is changed, it remains unchanged for the remainder of
   the thread's lifetime. So move the call from `ImportBlocks` to the init
   code where it is clearer that its effect lasts for the entire lifetime
   of the thread.
   
   Users of the kernel library might not expect `ImportBlocks` to have an
   influence on the thread it is called in. Particularly since it is only a
   compile time option and cannot be controlled at runtime. With this patch
   users of the kernel library can now choose their own scheduling policy.

   d4b17c7d46
10. TheCharlatan force-pushed on May 14, 2024
11. 
    TheCharlatan commented at 8:30 am on May 14, 2024: contributor

    Thanks for the ACKs, should be trivial to re-ACK.

    Updated 04ffe4061da2d0135f206032e167703772b5da78 -> d4b17c7d46ad8e2833ade99d5b4c9741c913e84d (rmKernelBatchPriority_0 -> rmKernelBatchPriority_1, compare)

    • Addressed @ryanofsky’s comment, reverting removing the comment.
12. 
    maflcko commented at 11:19 am on May 14, 2024: member

    ACK d4b17c7d46ad8e2833ade99d5b4c9741c913e84d 📭

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: ACK d4b17c7d46ad8e2833ade99d5b4c9741c913e84d 📭
    3suRJUCQ4uzxiXNUieIrA4SeUCJIloFdgg+YvgCi0vdTfSIQEnnKZRVj3kRUjiqOz8RbQwh+se0rOwSIUgZeKAg==
    

13. DrahtBot requested review from ryanofsky on May 14, 2024
14. ryanofsky approved
15. 
    ryanofsky commented at 1:23 pm on May 14, 2024: contributor

    Code review ACK d4b17c7d46ad8e2833ade99d5b4c9741c913e84d, just added suggested comment since last review
16. 
    hebasto commented at 2:26 pm on May 14, 2024: member

    Concept ACK.
17. hebasto approved
18. 
    hebasto commented at 2:41 pm on May 14, 2024: member

    ACK d4b17c7d46ad8e2833ade99d5b4c9741c913e84d, I have reviewed the code and it looks OK.
19. ryanofsky merged this on May 14, 2024
20. ryanofsky closed this on May 14, 2024

    ---

21. 
    theuni commented at 4:59 pm on May 14, 2024: member

    Post-merge utACK d4b17c7d46ad8e2833ade99d5b4c9741c913e84d
22. hebasto added the label Needs CMake port on May 20, 2024
23. 
    hebasto commented at 11:02 pm on May 20, 2024: member

    Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/202.
24. hebasto removed the label Needs CMake port on May 20, 2024

Contributors
TheCharlatan DrahtBot maflcko ryanofsky hebasto theuni

Review Requested
theuni

Labels
Validation