kernel: remove mempool_persist #30344

1. 
   theuni commented at 7:03 pm on June 26, 2024: member

   DumpMempool/LoadMempool are not necessary for the kernel.

   Noticed while working on instantiated logging.

   I suppose these could have been left in on purpose, but I’m assuming it was probably just an oversight.

2. kernel: remove mempool_persist.cpp

   DumpMempool/LoadMempool are not necessary for the kernel

   6d242ff1e9
3. 
   DrahtBot commented at 7:03 pm on June 26, 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	TheCharlatan, stickies-v

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #30141 (kernel: De-globalize validation caches by TheCharlatan)
   • #26022 (Add util::ResultPtr class by ryanofsky)
   • #25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
   • #25665 (refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky)

   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.

4. DrahtBot added the label Validation on Jun 26, 2024
5. 
   theuni commented at 7:04 pm on June 26, 2024: member

   ping @TheCharlatan
6. 
   TheCharlatan commented at 7:12 pm on June 26, 2024: contributor

   I’ve got this on a branch that I’ll PR as soon as #30141 is merged: https://github.com/TheCharlatan/bitcoin/commits/kernelRmGlobals/ . I try to keep the open PRs limited to get more focused review.
7. in src/init.cpp:129 in 4d2d9ab66d outdated

   126 using node::ApplyArgsManOptions;
   127 using node::BlockManager;
   128 using node::CacheSizes;
   129 using node::CalculateCacheSizes;
   130+using node::DumpMempool;
   131+using node::LoadMempool;
   

   ---

   ---

   

   TheCharlatan commented at 7:22 pm on June 26, 2024:

   Nit: These should be below node::DEFAULT_STOPATHEIGHT.

   ---

   

   theuni commented at 2:24 pm on June 27, 2024:

   Done.
8. TheCharlatan approved
9. 
   TheCharlatan commented at 7:33 pm on June 26, 2024: contributor

   ACK 4d2d9ab66db29a575cf2fab709c2d15044174c7e
10. mempool: move LoadMempool/DumpMempool to node f1478c0545
11. theuni force-pushed on Jun 27, 2024
12. 
    theuni commented at 2:26 pm on June 27, 2024: member

    I’ve got this on a branch that I’ll PR as soon as #30141 is merged: https://github.com/TheCharlatan/bitcoin/commits/kernelRmGlobals/ . I try to keep the open PRs limited to get more focused review.

    Aha, I see.

    I think this one is simple and self-contained enough to go in on its own unless you’re opposed.

    I’ll ping you on these in the future to make sure you don’t already have them teed up.

    Also, concept ACK on your next branch. Will give #30141 a final review and ACK to keep things moving.

13. 
    TheCharlatan commented at 1:28 pm on June 28, 2024: contributor

    Re-ACK f1478c05458562a9bef5c2ba43959d758e7b4745

    I’ll ping you on these in the future to make sure you don’t already have them teed up.

    No worries, it’s nice to see contributions from more developers, and if there is some redundant work every now and then, it just means it was reviewed already :)

14. stickies-v approved
15. 
    stickies-v commented at 1:50 pm on June 28, 2024: contributor

    ACK f1478c05458562a9bef5c2ba43959d758e7b4745

    I think kernel should have an interface that (indirectly) allows users to dump/load mempool to/from disk (or elsewhere), but this mempool_persist implementation seems too node opinionated to be included in kernel.

16. 
    glozow commented at 7:54 am on July 1, 2024: member

    lgtm, iiuc you’d want #30141 merged before this one?
17. 
    TheCharlatan commented at 8:04 am on July 1, 2024: contributor

    Re #30344 (comment)

    lgtm, iiuc you’d want #30141 merged before this one?

    Mmh, I don’t think this was said anywhere? Looks like the two are not really related either.

Contributors
theuni DrahtBot TheCharlatan stickies-v glozow

Labels
Validation