Repository: bitcoin/bitcoin
File: src/node/interfaces.cpp (C++)

Code before:
        // block templates during the final catch-up moments after IBD.
        if (!CooldownIfHeadersAhead(chainman(), notifications(), *maybe_tip, m_interrupt)) return {};

        BlockAssembler::Options assemble_options{options};
        ApplyArgsManOptions(*Assert(m_node.args), assemble_options);
        return std::make_unique<BlockTemplateImpl>(assemble_options, BlockAssembler{chainman().ActiveChainstate(), context()->mempool.get(), assemble_options}.CreateNewBlock(), m_node);
    }

    void interrupt() override
    {
        InterruptWait(notifications(), m_interrupt);
    }

    bool checkBlock(const CBlock& block, const node::BlockCheckOptions& options, std::string& reason, std::string& debug) override
    {
        LOCK(chainman().GetMutex());
        BlockValidationState state{TestBlockValidity(chainman().ActiveChainstate(), block, /*check_pow=*/options.check_pow, /*=check_merkle_root=*/options.check_merkle_root)};
        reason = state.GetRejectReason();
        debug = state.GetDebugMessage();
        return state.IsValid();
    }

    NodeContext* context() override { return &m_node; }
    ChainstateManager& chainman() { return *Assert(m_node.chainman); }
    KernelNotifications& notifications() { return *Assert(m_node.notifications); }
    bool m_interrupt{false};
    NodeContext& m_node;
};
} // namespace
} // namespace node

namespace interfaces {
std::unique_ptr<Node> MakeNode(node::NodeContext& context) { return std::make_unique<node::NodeImpl>(context); }
std::unique_ptr<Chain> MakeChain(node::NodeContext& context) { return std::make_unique<node::ChainImpl>(context); }
std::unique_ptr<Mining> MakeMining(node::NodeContext& context) { return std::make_unique<node::MinerImpl>(context); }
} // namespace interfaces


Review comment (bug):
In commit "mining: add interrupt()" (b57da688c21233e852c5afe5c5afafd3cae84e91)

Would be good to note that `notifications().m_tip_block_mutex` must be held while accessing this variable because it is accessed from multiple threads. Not sure if it would be possible to add a `GUARDED_BY` annotation but that could be nice.

More importantly, we may want to consider moving this variable somewhere else. A side-effect of adding the variable here is that it makes the MinerImpl class have state, which can be bad because now destroying the `Mining` object while `createNewBlock` or `waitNext` calls are in progress can cause segfaults. This could cause the node to crash with existing IPC clients if they destroy `Mining` objects early, or if they just disconnect during a `waitNext` call and the `Mining` object is garbage collected before it returns.

For these reasons I think it might be good to move the `MinerImpl::m_interrupt` variable somewhere else like `KernelNotifications::m_blocktemplate_request_interrupted`. This would be hackier than the current solution but I think worth it to avoid node crashes triggered by IPC.

(In the longer term a better solution would be to provide an `interfaces::Interrupt` type that could be passed to `waitNext` and `createBlockTemplate` methods as additional method argument. These methods could then attach a `std::function<void()>` lambda to run whatever code they need when an interrupt is triggered. Clients would then have the ability to cancel individual calls by triggering the `Interrupt` object. Internally, the cancellation could be implemented either by making a separate RPC call or by using Cap'n Proto's native `allowCancellation` mechanism.)




Code after:
            // forever if no block was mined in the past day.
            while (chainman().IsInitialBlockDownload()) {
                maybe_tip = waitTipChanged(maybe_tip->hash, MillisecondsDouble{1000});
                if (!maybe_tip || chainman().m_interrupt || WITH_LOCK(notifications().m_tip_block_mutex, return m_interrupt_mining)) return {};
            }

            // Also wait during the final catch-up moments after IBD.
            if (!CooldownIfHeadersAhead(chainman(), notifications(), *maybe_tip, m_interrupt_mining)) return {};
        }

        BlockAssembler::Options assemble_options{options};
        ApplyArgsManOptions(*Assert(m_node.args), assemble_options);
        return std::make_unique<BlockTemplateImpl>(assemble_options, BlockAssembler{chainman().ActiveChainstate(), context()->mempool.get(), assemble_options}.CreateNewBlock(), m_node);
    }

    void interrupt() override
    {
        InterruptWait(notifications(), m_interrupt_mining);
    }

    bool checkBlock(const CBlock& block, const node::BlockCheckOptions& options, std::string& reason, std::string& debug) override
    {
        LOCK(chainman().GetMutex());
        BlockValidationState state{TestBlockValidity(chainman().ActiveChainstate(), block, /*check_pow=*/options.check_pow, /*check_merkle_root=*/options.check_merkle_root)};
        reason = state.GetRejectReason();
        debug = state.GetDebugMessage();
        return state.IsValid();
    }

    NodeContext* context() override { return &m_node; }
    ChainstateManager& chainman() { return *Assert(m_node.chainman); }
    KernelNotifications& notifications() { return *Assert(m_node.notifications); }
    // Treat as if guarded by notifications().m_tip_block_mutex
    bool m_interrupt_mining{false};
    NodeContext& m_node;
};
} // namespace
} // namespace node

namespace interfaces {
std::unique_ptr<Node> MakeNode(node::NodeContext& context) { return std::make_unique<node::NodeImpl>(context); }
std::unique_ptr<Chain> MakeChain(node::NodeContext& context) { return std::make_unique<node::ChainImpl>(context); }
std::unique_ptr<Mining> MakeMining(node::NodeContext& context) { return std::make_unique<node::MinerImpl>(context); }
} // namespace interfaces