validation: Check if mempool exists before size check in ActivateSnapshot #30388

1. 
   TheCharlatan commented at 6:50 am on July 4, 2024: contributor

   The mempool is an optional component of the chainstate manager, so don’t assume its presence and instead check if it is there first.
2. 
   DrahtBot commented at 6:51 am on July 4, 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	fjahr, maflcko

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

3. TheCharlatan force-pushed on Jul 4, 2024
4. TheCharlatan renamed this:
   bugfix: Check if mempool exists before asserting in ActivateSnapshot
   bugfix: Check if mempool exists before check in ActivateSnapshot
   on Jul 4, 2024
5. TheCharlatan renamed this:
   bugfix: Check if mempool exists before check in ActivateSnapshot
   bugfix: Check if mempool exists before size check in ActivateSnapshot
   on Jul 4, 2024
6. 
   maflcko commented at 7:21 am on July 4, 2024: member

   It can only be nullptr in unit tests, not in bitcoind. However, it seems fine to add this check for consistency with the other code.

   lgtm ACK 39ec860eb8b867cce4ed437049abc0e031fd3c95

7. 
   mzumsande commented at 7:32 am on July 4, 2024: contributor

   I wouldn’t call this a “bug” because I think we can assume that the active chainstate (which is checked here, not any chainstate) must have a mempool attached - this is not optional. But if it helps the tests, why not?
8. TheCharlatan renamed this:
   bugfix: Check if mempool exists before size check in ActivateSnapshot
   validation: Check if mempool exists before size check in ActivateSnapshot
   on Jul 4, 2024
9. DrahtBot added the label Validation on Jul 4, 2024
10. validation: Check if mempool exists before asserting in ActivateSnapshot 33c48c106c
11. TheCharlatan force-pushed on Jul 4, 2024
12. 
    TheCharlatan commented at 8:05 am on July 4, 2024: contributor

    Changed description to drop the bugfix label.

    Re #30388 (comment)

    I wouldn’t call this a “bug” because I think we can assume that the active chainstate (which is checked here, not any chainstate) must have a mempool attached - this is not optional.

    We do the same check on the active chainstate here: https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L4587. So as @maflcko points out, it would be good for consistency’s sake alone.

13. in src/validation.cpp:5684 in 33c48c106c

    5680@@ -5681,7 +5681,8 @@ util::Result<void> ChainstateManager::ActivateSnapshot(
    5681             return util::Error{strprintf(_("The base block header (%s) is part of an invalid chain."), base_blockhash.ToString())};
    5682         }
    5683 
    5684-        if (Assert(m_active_chainstate->GetMempool())->size() > 0) {
    5685+        auto mempool{m_active_chainstate->GetMempool()};
    

    ---

    ---

    

    fjahr commented at 12:11 pm on July 4, 2024:

    nit: The type doesn’t seem so complex that it has to be auto

    0CTxMemPool* mempool{m_active_chainstate->GetMempool()};
    

14. 
    fjahr commented at 12:14 pm on July 4, 2024: contributor

    ACK 33c48c106cf03ff62994ff5777a775982d90eab9

    While this isn’t something that can be hit by users I agree it makes sense conceptually. I am not sure this will help the performance of the tests much as I have laid out in my comment here: #30329 (comment)

15. DrahtBot requested review from maflcko on Jul 4, 2024
16. 
    maflcko commented at 12:32 pm on July 4, 2024: member

    re-ACK 33c48c106cf03ff62994ff5777a775982d90eab9
17. glozow merged this on Jul 4, 2024
18. glozow closed this on Jul 4, 2024

    ---

19. Theghost256 approved

Contributors
TheCharlatan DrahtBot maflcko mzumsande fjahr Theghost256

Review Requested
maflcko

Labels
Validation