fix: update `sync_headers_in_reverse` logic to match lotus #4291

hanabi1224 · 2024-05-07T13:01:10Z

Summary of changes

Changes introduced in this pull request:

Update sync_headers_in_reverse logic to match lotus
Avoid re-validating the current head when the local chain is not a fork
Avoid some unnecessary clones

Log(importing a snapshot whose head is not a fork, it no longer re-validates the head):

2024-05-07T12:55:50.070707Z  INFO forest_filecoin::chain_sync::chain_muxer: Local node is behind the network, starting BOOTSTRAP from LOCAL_HEAD = 1591906 -> NETWORK_HEAD = 1592005
2024-05-07T12:55:52.146710Z  INFO forest_filecoin::chain_sync::tipset_syncer: Validating tipset: EPOCH = 1591907, N blocks = 2

lotus code(func (syncer *Syncer) collectHeaders): https://github.com/filecoin-project/lotus/blob/master/chain/sync.go#L684
Some highlights:

...
// we want to sync all the blocks until the height above our
// best tipset so far
untilHeight := known.Height() + 1
...
loop:
	for blockSet[len(blockSet)-1].Height() > untilHeight {
             ...
        }
...
if base.IsChildOf(knownParent) {
		// common case: receiving a block that's potentially part of the same tipset as our best block
		return blockSet, nil
	}

	// We have now ascertained that this is *not* a 'fast forward'
	log.Warnf("(fork detected) synced header chain (%s - %d) does not link to our best block (%s - %d)", incoming.Cids(), incoming.Height(), known.Cids(), known.Height())
	fork, err := syncer.syncFork(ctx, base, known, ignoreCheckpoint)

Reference issue to close (if applicable)

Closes #4113

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.

ruseinov · 2024-05-07T13:46:58Z

src/blocks/tipset.rs

+        // FIXME: The height check might go beyond what is meant by
+		//  "parent", but many parts of the code rely on the tipset's
+		//  height for their processing logic at the moment to obviate it.
+        && self.epoch() > other.epoch()


q: what you want to say here is that there is just a sanity check, correct? Just an extra measure to make sure the relationship is somewhat correct?

This is completely copied from lotus. See the link above : )
https://github.com/filecoin-project/lotus/blob/01ec22974942fb7328a1e665704c6cfd75d93372/chain/types/tipset.go#L258

Do we have any code that depends on this check? I think we reject blocks that do not have sensible epochs at an earlier stage. I think we can just remove this extra check.

Do we have any code that depends on this check? I think we reject blocks that do not have sensible epochs at an earlier stage. I think we can just remove this extra check.

@lemmih No other code depends on this check. Removed.

ruseinov · 2024-05-07T13:55:42Z

src/chain_sync/tipset_syncer.rs

-    let oldest_tipset = parent_tipsets.last().clone();
-    // Determine if the local chain was a fork.
-    // If it was, then sync the fork tipset range by iteratively walking back
+    let oldest_tipset = parent_tipsets.last();


nit: perhaps oldest_parent is a better name, similar to https://github.com/ChainSafe/forest/pull/4291/files#diff-c2e102581e8811dd894188425099a003280f0c7e38b9c5a7ca75f2a0dd0bdaddR824

Good point, actually parent is quite confusing here, renamed parent_tipsets to pending_tipsets(_for_sync), and oldest_tipset to oldest_pending_tipset

…s-to-match-lotus

lemmih

Logic appears to be sound (though still hard to follow).

lemmih · 2024-05-08T09:08:36Z

src/blocks/tipset.rs

+        // FIXME: The height check might go beyond what is meant by
+		//  "parent", but many parts of the code rely on the tipset's
+		//  height for their processing logic at the moment to obviate it.
+        && self.epoch() > other.epoch()


Do we have any code that depends on this check? I think we reject blocks that do not have sensible epochs at an earlier stage. I think we can just remove this extra check.

…s-to-match-lotus

aatifsyed

AIUI this is transpilation of some go code.
If so, WIBNI you checked in a comment to the reference code, so readers can understand why it is the way it is.

aatifsyed · 2024-06-05T13:24:51Z

src/blocks/tipset.rs

+        // Note: the extra `&& self.epoch() > other.epoch()` check in lotus is dropped
+        // See <https://github.com/filecoin-project/lotus/blob/01ec22974942fb7328a1e665704c6cfd75d93372/chain/types/tipset.go#L258>


excellent comment

aatifsyed · 2024-06-05T13:26:41Z

src/chain_sync/tipset_syncer.rs

-                parent_tipsets.extend(fork_tipsets);
-                break;
-            }
+    info!("Fork detected, searching for a common ancestor between the local chain and the network chain");


nit: this log message is basically the same as the comment above, you can probably delete the comment

…s-to-match-lotus

hanabi1224 · 2024-06-05T13:42:29Z

WIBNI you checked in a comment to the reference code, so readers can understand why it is the way it is.

@aatifsyed link added!

fix: update sync_headers_in_reverse logic to match lotus

8111d4b

hanabi1224 force-pushed the hm/update-sync-headers-to-match-lotus branch from 994a715 to 2affcee Compare May 7, 2024 13:09

fix: update sync_headers_in_reverse logic to match lotus

23366b9

hanabi1224 force-pushed the hm/update-sync-headers-to-match-lotus branch from 2affcee to 23366b9 Compare May 7, 2024 13:27

hanabi1224 marked this pull request as ready for review May 7, 2024 13:30

hanabi1224 requested a review from a team as a code owner May 7, 2024 13:30

hanabi1224 requested review from lemmih and sudo-shashank and removed request for a team May 7, 2024 13:30

short circuit proposed_head == current_head case

2aff1ce

ruseinov reviewed May 7, 2024

View reviewed changes

fix lint test

aecdf51

ruseinov reviewed May 7, 2024

View reviewed changes

hanabi1224 added 2 commits May 8, 2024 13:43

naming

5d7551b

Merge remote-tracking branch 'origin/main' into hm/update-sync-header…

34be8ff

…s-to-match-lotus

lemmih approved these changes May 8, 2024

View reviewed changes

hanabi1224 added 5 commits May 8, 2024 19:00

Merge remote-tracking branch 'origin/main' into hm/update-sync-header…

1744436

…s-to-match-lotus

drop the extra check in is_child_of

ea5ff89

Merge branch 'main' into hm/update-sync-headers-to-match-lotus

9dd95fe

Merge branch 'main' into hm/update-sync-headers-to-match-lotus

743e234

Merge branch 'main' into hm/update-sync-headers-to-match-lotus

c37bf4e

sudo-shashank removed their request for review May 13, 2024 11:45

hanabi1224 added 4 commits May 15, 2024 06:29

Merge branch 'main' into hm/update-sync-headers-to-match-lotus

ac0fcc8

Merge branch 'main' into hm/update-sync-headers-to-match-lotus

be57769

Merge branch 'main' into hm/update-sync-headers-to-match-lotus

4c818dc

Merge branch 'main' into hm/update-sync-headers-to-match-lotus

e751206

aatifsyed approved these changes Jun 5, 2024

View reviewed changes

hanabi1224 added 3 commits June 5, 2024 21:40

refine comments

3c106ea

Merge remote-tracking branch 'origin/main' into hm/update-sync-header…

5d9583a

…s-to-match-lotus

fmt

cb93ba2

aatifsyed approved these changes Jun 5, 2024

View reviewed changes

hanabi1224 enabled auto-merge June 5, 2024 13:44

hanabi1224 added this pull request to the merge queue Jun 5, 2024

Merged via the queue into main with commit d7c9b6e Jun 5, 2024
27 checks passed

hanabi1224 deleted the hm/update-sync-headers-to-match-lotus branch June 5, 2024 14:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: update `sync_headers_in_reverse` logic to match lotus #4291

fix: update `sync_headers_in_reverse` logic to match lotus #4291

hanabi1224 commented May 7, 2024 •

edited

ruseinov May 7, 2024

hanabi1224 May 7, 2024 •

edited

lemmih May 8, 2024

hanabi1224 May 8, 2024

ruseinov May 7, 2024 •

edited

hanabi1224 May 8, 2024

lemmih left a comment

lemmih May 8, 2024

aatifsyed left a comment

aatifsyed Jun 5, 2024

aatifsyed Jun 5, 2024

hanabi1224 Jun 5, 2024

hanabi1224 commented Jun 5, 2024

		// Note: the extra `&& self.epoch() > other.epoch()` check in lotus is dropped
		// See <https://github.com/filecoin-project/lotus/blob/01ec22974942fb7328a1e665704c6cfd75d93372/chain/types/tipset.go#L258>

fix: update sync_headers_in_reverse logic to match lotus #4291

fix: update sync_headers_in_reverse logic to match lotus #4291

Conversation

hanabi1224 commented May 7, 2024 • edited

Summary of changes

Reference issue to close (if applicable)

Other information and links

Change checklist

Choose a reason for hiding this comment

hanabi1224 May 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruseinov May 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemmih left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aatifsyed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanabi1224 commented Jun 5, 2024

fix: update `sync_headers_in_reverse` logic to match lotus #4291

fix: update `sync_headers_in_reverse` logic to match lotus #4291

hanabi1224 commented May 7, 2024 •

edited

hanabi1224 May 7, 2024 •

edited

ruseinov May 7, 2024 •

edited