kernel: remove mempool_persist #30344

pull theuni wants to merge 2 commits into bitcoin:master from theuni:kernel-no-mempool-persist changing 6 files +20 −21

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.

kernel: remove mempool_persist.cpp

DumpMempool/LoadMempool are not necessary for the kernel

6d242ff1e9

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, glozow

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.
DrahtBot added the label Validation on Jun 26, 2024
theuni commented at 7:04 pm on June 26, 2024: member

ping @TheCharlatan
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.

Type	Reviewers
ACK	TheCharlatan, stickies-v, glozow

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.

TheCharlatan approved
TheCharlatan commented at 7:33 pm on June 26, 2024: contributor

ACK 4d2d9ab66db29a575cf2fab709c2d15044174c7e
mempool: move LoadMempool/DumpMempool to node f1478c0545
theuni force-pushed on Jun 27, 2024
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.
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 :)
stickies-v approved
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.
glozow commented at 7:54 am on July 1, 2024: member

lgtm, iiuc you’d want #30141 merged before this one?
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.
glozow commented at 2:55 pm on July 1, 2024: member

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.

It seems they have a couple acks and conflict with each other? Was taking a guess based on #30344 (comment)
theuni commented at 3:06 pm on July 1, 2024: member

@glozow We both noticed this independently. @TheCharlatan Had it queued up as part of his next batch of changes (not yet PR’d), but I didn’t know that when I PR’d this one. He’s since agreed that it makes sense to merge this on its own.

#30141 is unrelated, he’s just waiting for that to be merged before PR’ing his next batch of kernel changes.
glozow commented at 3:11 pm on July 1, 2024: member

#30141 is unrelated, he’s just waiting for that to be merged before PR’ing his next batch of kernel changes.

I can see that they aren’t related, just trying to clarify which one to merge first because the PRs conflict. I figured this would be an ok place to ask that since it’s the same people. Apologies, I wasn’t trying to imply that they’re related at all, my previous comment was to provide an explanation for why I was asking which one to prioritize.
glozow commented at 9:00 am on July 2, 2024: member

ACK f1478c0545
glozow merged this on Jul 2, 2024
glozow closed this on Jul 2, 2024
maflcko added the label Needs CMake port on Jul 2, 2024
hebasto commented at 6:57 am on July 14, 2024: member

Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/264.
hebasto removed the label Needs CMake port on Jul 14, 2024

Contributors
theuni DrahtBot TheCharlatan stickies-v glozow hebasto

Labels
Validation

kernel: remove mempool_persist #30344

Code Coverage

Reviews

Conflicts