Repository: bitcoin/bitcoin
File: src/headerssync.h (C/C++)

Code before:
    State GetState() const { return m_download_state; }

    /** Return the height reached during the PRESYNC phase */
    int64_t GetPresyncHeight() const { return m_current_height; }

    /** Return the block timestamp of the last header received during the PRESYNC phase. */
    uint32_t GetPresyncTime() const { return m_last_header_received.nTime; }

    /** Return the amount of work in the chain received during the PRESYNC phase. */
    arith_uint256 GetPresyncWork() const { return m_current_chain_work; }

    /** Construct a HeadersSyncState object representing a headers sync via this
     *  download-twice mechanism).
     *
     * id: node id (for logging)
     * consensus_params: parameters needed for difficulty adjustment validation
     * chain_start: best known fork point that the peer's headers branch from
     * minimum_required_work: amount of chain work required to accept the chain
     */
    HeadersSyncState(NodeId id, const Consensus::Params& consensus_params,
            const CBlockIndex* chain_start, const arith_uint256& minimum_required_work);

    /** Result data structure for ProcessNextHeaders. */
    struct ProcessingResult {
        bool success{false};
        bool request_more{false};
    };

    /** Process a batch of headers, once a sync via this mechanism has started
     *
     * headers: headers that were received over the network for processing.
     *                   Assumes the caller has already verified the headers
     *                   are continuous, and has checked that each header
     *                   satisfies the proof-of-work target included in the
     *                   header (but not necessarily verified that the
     *                   proof-of-work target is correct and passes consensus
     *                   rules).
     *                   On output, this will be replaced with headers the caller
     *                   can fully process and validate now (because these
     *                   returned headers are on a chain with sufficient work).
     * full_headers_message: true if the message was at max capacity,
     *                       indicating more headers may be available
     * ProcessingResult.success: set to false if an error is detected and the sync is
     *                       aborted; true otherwise.
     * ProcessingResult.request_more: if true, the caller is suggested to call
     *                       NextHeadersRequestLocator and send a getheaders message using it.
     */
    ProcessingResult ProcessNextHeaders(std::vector<CBlockHeader>& headers, bool full_headers_message);

    /** Issue the next GETHEADERS message to our peer.
     *

Review comment (suggestion):
Not thrilled by increasing the number of in/out parameters - it pushes readers towards having to lean on comments more, rather than having the code somewhat self-documenting. It also makes the unit tests more cumbersome. What makes the change somewhat acceptable is that it conforms to the style of *net_processing.cpp*.

If we keep this, I would also suggest further simplification of `ProcessingResult` through converting it to an enum: f2a617955e65682b0765c7909c06aea19ca43eae

Code after:
    /** Return the current state of our download */
    State GetState() const { return m_download_state; }

    /** Return the height reached during the PRESYNC phase */
    int64_t GetPresyncHeight() const { return m_current_height; }

    /** Return the block timestamp of the last header received during the PRESYNC phase. */
    uint32_t GetPresyncTime() const { return m_last_header_received.nTime; }

    /** Return the amount of work in the chain received during the PRESYNC phase. */
    arith_uint256 GetPresyncWork() const { return m_current_chain_work; }

    /** Construct a HeadersSyncState object representing a headers sync via this
     *  download-twice mechanism).
     *
     * id: node id (for logging)
     * consensus_params: parameters needed for difficulty adjustment validation
     * chain_start: best known fork point that the peer's headers branch from
     * minimum_required_work: amount of chain work required to accept the chain
     */
    HeadersSyncState(NodeId id, const Consensus::Params& consensus_params,
                     const HeadersSyncParams& params, const CBlockIndex& chain_start,
                     const arith_uint256& minimum_required_work);

    /** Result data structure for ProcessNextHeaders. */
    struct ProcessingResult {
        std::vector<CBlockHeader> pow_validated_headers;
        bool success{false};
        bool request_more{false};
    };

    /** Process a batch of headers, once a sync via this mechanism has started
     *
     * received_headers: headers that were received over the network for processing.
     *                   Assumes the caller has already verified the headers
     *                   are continuous, and has checked that each header
     *                   satisfies the proof-of-work target included in the
     *                   header (but not necessarily verified that the
     *                   proof-of-work target is correct and passes consensus
     *                   rules).
     * full_headers_message: true if the message was at max capacity,
     *                       indicating more headers may be available
     * ProcessingResult.pow_validated_headers: will be filled in with any
     *                       headers that the caller can fully process and
     *                       validate now (because these returned headers are
     *                       on a chain with sufficient work)
     * ProcessingResult.success: set to false if an error is detected and the sync is
     *                       aborted; true otherwise.
     * ProcessingResult.request_more: if true, the caller is suggested to call
     *                       NextHeadersRequestLocator and send a getheaders message using it.
     */