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

Code before:

        // Alternate waiting for a new tip and checking if fees have risen.
        // The latter check is expensive so we only run it once per second.
        auto now{NodeClock::now()};
        const auto deadline = now + options.timeout;
        const MillisecondsDouble tick{1000};
        const bool allow_min_difficulty{chainman().GetParams().GetConsensus().fPowAllowMinDifficultyBlocks};

        do {
            bool tip_changed{false};
            {
                WAIT_LOCK(notifications().m_tip_block_mutex, lock);
                // Note that wait_until() checks the predicate before waiting
                notifications().m_tip_block_cv.wait_until(lock, std::min(now + tick, deadline), [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
                    AssertLockHeld(notifications().m_tip_block_mutex);
                    const auto tip_block{notifications().TipBlock()};
                    // We assume tip_block is set, because this is an instance
                    // method on BlockTemplate and no template could have been
                    // generated before a tip exists.
                    tip_changed = Assume(tip_block) && tip_block != m_block_template->block.hashPrevBlock;
                    return tip_changed || chainman().m_interrupt;
                });
            }

            if (chainman().m_interrupt) return nullptr;
            // At this point the tip changed, a full tick went by or we reached
            // the deadline.

            // Must release m_tip_block_mutex before locking cs_main, to avoid deadlocks.
            LOCK(::cs_main);

            // On test networks return a minimum difficulty block after 20 minutes
            if (!tip_changed && allow_min_difficulty) {
                const NodeClock::time_point tip_time{std::chrono::seconds{chainman().ActiveChain().Tip()->GetBlockTime()}};
                if (now > tip_time + 20min) {
                    tip_changed = true;
                }
            }

            /**
             * We determine if fees increased compared to the previous template by generating
             * a fresh template. There may be more efficient ways to determine how much
             * (approximate) fees for the next block increased, perhaps more so after
             * Cluster Mempool.
             *
             * We'll also create a new template if the tip changed during this iteration.
             */
            if (options.fee_threshold < MAX_MONEY || tip_changed) {
                auto tmpl{std::make_unique<BlockTemplateImpl>(m_assemble_options,
                                                              BlockAssembler{
                                                                  chainman().ActiveChainstate(),

Review comment (bug):
In 31e3808df9c59d36a07cad07df89ae1bf9e63000 "interfaces: refactor: move `waitTipChanged` implementation to miner": this neatly solves the deadlock issue, but I think we should preserve that comment.

Additionally we should explain why `getTip()` is called here, ignoring the block hash returned by `WaitTipChanged`. Perhaps the latter should just return a boolean?

Code after:
    explicit MinerImpl(NodeContext& node) : m_node(node) {}

    bool isTestChain() override
    {
        return chainman().GetParams().IsTestChain();
    }

    bool isInitialBlockDownload() override
    {
        return chainman().IsInitialBlockDownload();
    }

    std::optional<BlockRef> getTip() override
    {
        return GetTip(chainman());
    }

    std::optional<BlockRef> waitTipChanged(uint256 current_tip, MillisecondsDouble timeout) override
    {
        return WaitTipChanged(chainman(), notifications(), current_tip, timeout);
    }

    std::unique_ptr<BlockTemplate> createNewBlock(const BlockCreateOptions& options) override
    {
        // Ensure m_tip_block is set so consumers of BlockTemplate can rely on that.
        if (!waitTipChanged(uint256::ZERO, MillisecondsDouble::max())) 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);
    }

    NodeContext* context() override { return &m_node; }
    ChainstateManager& chainman() { return *Assert(m_node.chainman); }
    KernelNotifications& notifications() { return *Assert(m_node.notifications); }
    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