fix: header downloading get stuck

[hanabi1224]

Summary of changes

Root cause: when the local head is followed by a null Tipset, sync_headers_in_reverse runs into a dead loop
e.g. When the local head is at epoch 1742466 which is followed by a null tipset at epoch 1742467, the last queried tipsets in network_tipsets are at epoch [1742466, ..] which are all filtered out, and lead the loop into a dead one.

        for tipset in network_tipsets
            .into_iter()
            .take_while(|ts| ts.epoch() >= until_epoch)
        {
                ...
        }

This is a regression introduced in #4291
In lotus, it breaks the outer loop directly with the label

loop:
	for blockSet[len(blockSet)-1].Height() > untilHeight {
            ...
            for _, b := range blks {
			if b.Height() < untilHeight {
				break loop
			}
            ...
            }
        }

Changes introduced in this pull request:

• break the loop when the tipset is a null one at the until_epoch

Reference issue to close (if applicable)

Closes #4441

Other information and links

Change checklist

• I have performed a self-review of my own code,
• I have made corresponding changes to the documentation,
• I have added tests that prove my fix is effective or that my feature works (if possible),
• I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

[LesnyRumcajs]

Fantastic, thank you!

[hanabi1224]

When the until_epoch is null, it relies on break 'outer; to break this loop

[LesnyRumcajs]

Could we avoid the inner/outer loop complexity by refactoring this to smaller functions that could potentially be unit tested? At least the block for tipset in network_tipsets {

[hanabi1224]

To avoid the 'outer label, how about this

        // breaks the loop when `until_epoch` has been added to `pending_tipsets`
        // or when `until_epoch` is null. Note that in the latter case, the outer
        // while condition is always true, and it relies on this flag to break
        // the loop
        let should_terminate = 'append_tipsets: {
            for tipset in network_tipsets {
                // This could happen when the `until_epoch` is a null epoch
                if tipset.epoch() < until_epoch {
                    break 'append_tipsets true;
                }
                validate_tipset_against_cache(bad_block_cache, tipset.key(), &accepted_blocks)?;
                accepted_blocks.extend(tipset.cids());
                tracker.write().set_epoch(tipset.epoch());
                pending_tipsets.push(tipset);
            }
            false
        };
        if should_terminate {
            break;
        }

[LesnyRumcajs]

It's up to you, but I'd still recommend extracting the loop to a separate function. This makes it possible to test and is subjectively easier for the reader of the parent function to grasp.

[hanabi1224]

How about this then? It still highly relies on the comments to explain this piece of code though

// tipsets is sorted by epoch in descending order
// returns true when `until_epoch_inclusive` is overreached
fn for_each_tipset_until_epoch_overreached(
    tipsets: impl IntoIterator<Item = Arc<Tipset>>,
    until_epoch_inclusive: ChainEpoch,
    mut callback: impl FnMut(Arc<Tipset>) -> Result<(), TipsetRangeSyncerError>,
) -> Result<bool, TipsetRangeSyncerError> {
    for tipset in tipsets {
        if tipset.epoch() < until_epoch_inclusive {
            return Ok(true);
        }
        callback(tipset)?;
    }
    Ok(false)
}

        let callback = |tipset: Arc<Tipset>| {
            validate_tipset_against_cache(bad_block_cache, tipset.key(), &accepted_blocks)?;
            accepted_blocks.extend(tipset.cids());
            tracker.write().set_epoch(tipset.epoch());
            pending_tipsets.push(tipset);
            Ok(())
        };
        // Breaks the loop when `until_epoch` is overreached, which happens
        // when there are null tipsets in the queried range.
        // Note that when the `until_epoch` is null, the outer while condition
        // is always true, and it relies on this flag to break the loop
        if for_each_tipset_until_epoch_overreached(network_tipsets, until_epoch, callback)? {
            // Breaks when the `until_epoch` is overreached.
            break;
        }

[LesnyRumcajs]

LGTM

[hanabi1224]

Since this has been queued, I will make this refactor in a separate PR and let's discuss there

[elmattic]

Nice!