feat(sync): maintain tail #265

Wondertan · 2025-03-31T21:13:10Z

Adds Tail method to Syncer that can initialize a node with a height estimated header or using the configured height(or hash) as the new Tail. Besides, it can gracefully handle reconfiguration of height/hash upon restarts.

This change deliberately leaves some tech debt behind. The new Tail method has quite a lot of branchiness and could be teared apart/rewritten reducing nesting and improving readability. Some more specific debts are marked with a TODO. All of this is not "clean" and simply achieves the goal. The future backward sync will anyway require us to refactor this logic, so there is minimum value in cleaning things now.

Nvm, I end up refactoring it for the sake of reviewers sanity and code maintainability. There are still TODOs left for things that are literally blocked on bsync or else, but everything else was uniformly integrated into the existing code.

Depends on #283 and #285

Closes #258 and #259

sync/sync_head.go

sync/sync_tail.go

Wondertan · 2025-05-11T01:26:13Z

Converting to draft until dependencies are merged

codecov-commenter · 2025-05-13T21:42:30Z

Codecov Report

Attention: Patch coverage is 66.66667% with 66 lines in your changes missing coverage. Please review.

Please upload report for BASE (pruning@9fce763). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
sync/sync_tail.go	69.56%	26 Missing and 9 partials ⚠️
sync/sync_head.go	51.02%	23 Missing and 1 partial ⚠️
sync/sync_store.go	72.22%	3 Missing and 2 partials ⚠️
headertest/store.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             pruning     #265   +/-   ##
==========================================
  Coverage           ?   54.52%           
==========================================
  Files              ?       39           
  Lines              ?     4169           
  Branches           ?        0           
==========================================
  Hits               ?     2273           
  Misses             ?     1733           
  Partials           ?      163

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

renaynay

I tried to review this as thoroughly as possible. I left a few comments - some of them are just for me and my understanding. Feel free to resolve or correct my understanding.

Open questions:

why do we allow SyncFromHeight? -- IMO providing two options for one purpose is not necessary (FOR NOW) (where SyncFromHash would likely be the more used option; and restricting optionality to just hash would help clean up unnecessary additional code toggling between these options. (e.g. SyncFromHash has higher priority than SyncFromHeight.) We've historically relied on TrustedHash instead of something like "TrustedHeight" (bc how can we trust height if we only pass a # and no hash so we can check that header we receive == header we query for). If we allow SyncFromHeight, we would also want to ask user to specify the hash they believe to be the header hash for the header at the given height, no? Otherwise we are JTMB?
Can we rename head_sync file in syncer at some point? Totally missed this new addition and it’ll be so confusing 😂 (separate but relevant)

renaynay · 2025-05-14T16:39:43Z

sync/sync_tail.go

+func (s *Syncer[H]) isTailHashOutdated(h H) (header.Hash, bool) {
+	wantHash := s.Params.SyncFromHash
+	outdated := wantHash != nil && (h.IsZero() || !bytes.Equal(wantHash, h.Hash()))
+	return wantHash, outdated
+}
+
+func (s *Syncer[H]) isTailHeightOutdated(h H) (uint64, bool) {


these two methods are confusing bc they not only check for outdated - they also return outdated == true if header is nil. Maybe just methods need renaming

I have a little refactoring for them going on. I had the same concern

Check how it looked like in the first iteration https://github.com/celestiaorg/go-header/pull/265/files/70f65dea032db43d3a499e9d5a995b076906240f
💀

sync/sync_tail.go

renaynay · 2025-05-14T17:01:56Z

sync/sync_tail.go

+	head H,
+	blockTime, trustingPeriod time.Duration,
+) (height uint64) {
+	headersToRetain := uint64(trustingPeriod / blockTime) //nolint:gosec


this can be extracted into var, no? retentionWindow

afaics can't change trustingPeriod or blockTime in runtime

I don't follow what do you mean by extracting here

doesn't matter just a nit

renaynay · 2025-05-14T17:06:36Z

sync/sync_tail.go

+	case from.Height() > to.Height():
+		log.Infof("move tail down from %d to %d, syncing the diff...", from.Height(), to.Height())
+
+		// TODO(@Wondertan): This works but it assumes this code is only run before syncing routine starts.


Yeah afaict we would never "grow" tail in runtime -- this would only happen on a restart for a user who wants to specify "lower" tail height

and this would be hit before a new subjHead would be set which is what would trigger a sync job in the forward direction.

not actionable

renaynay · 2025-05-14T17:08:12Z

sync/sync_tail.go

+		}
+	} else if tailHash == nil && tailHeight == 0 {
+		if tail.IsZero() {
+			// no previously known Tail available - estimate solely on Head


let's add a debug log for this case as well pls that includes estimated height

it's debugged after refactoring in a different place

renaynay · 2025-05-14T17:12:47Z

sync/sync.go

 	if err != nil {
 		return err
 	}
-	// gets the latest head and kicks off syncing if necessary


relic, will get it back

renaynay · 2025-05-14T17:16:20Z

sync/sync_head.go

+	case errors.Is(err, header.ErrEmptyStore):
+		log.Info("empty store, initializing...")
+		s.metrics.subjectiveInitialization(s.ctx)


Okay so this will make it so that we do not have to manually initialise the store via nodebuilder in celestia-node in order to kick off header syncing - syncer will do it for us here.

Yes, that's was the purpose behind doing #243

renaynay · 2025-05-14T17:26:13Z

sync/sync_head.go

 	defer s.incomingMu.Unlock()

-	err := s.verify(ctx, head)
+	// TODO(@Wondertan): We need to ensure Tail is available before verification for subj init case and this is fine here


can we perform a sanity check here on the incoming head somehow to ensure we do not get exorbitant height that could trigger a cut of the tail too significantly? (meaning one that is shorter than the pruning window)

renaynay · 2025-05-14T17:28:22Z

sync/sync_store.go

+	//	However, Syncer has yet to be refactored to not assume those invariants and until then
+	//	this method is a shim that allows using store with old assumptions.
+	//  To be reworked by bsync.
+	if headers[0].Height() >= head.Height() {


can we extract into separate func here ensureAppendAdjacency

Hmm, we could

Don't see much value from doing this tbh, so ignoring

Wondertan · 2025-05-14T17:43:35Z

Can we rename head_sync file in syncer at some point? Totally missed this new addition and it’ll be so confusing 😂 (separate but relevant)

Totally agree and had the same thought. There is a simple solution for that. Rename sync_*.go files to syncer_*.go to match the name of the component

Wondertan · 2025-05-14T17:51:02Z

why do we allow SyncFromHeight? -- IMO providing two options for one purpose is not necessary (FOR NOW) (where SyncFromHash would likely be the more used option; and restricting optionality to just hash would help clean up unnecessary additional code toggling between these options. (e.g. SyncFromHash has higher priority than SyncFromHeight.) We've historically relied on TrustedHash instead of something like "TrustedHeight" (bc how can we trust height if we only pass a # and no hash so we can check that header we receive == header we query for). If we allow SyncFromHeight, we would also want to ask user to specify the hash they believe to be the header hash for the header at the given height, no? Otherwise we are JTMB?

Height based sync was requested by @cmwaters and I generally agree that it is needed/simpler for users. There is ofc security aspect to it where syncing from a historical height is not LRA resistant. However, this is true currently for both hash and height based sync. The difference in trust model is that the header you start with is less trusted with hash, but beyond that, both are similarly insecure. Once bsync comes both of them will become secure, so it make sense for now to introduce an option that's easier for users and keep the old one for backwards compatibility.

renaynay

This PR LGTM - your choice re nits

sync/sync_tail.go

…g gossiped headers

renaynay

so on start-up, we no longer need to ensure that tail was set in store and we can do that after processing the first incoming header, is that correct? meaning incoming header will become new head and the append will make that it's tail and then subjectiveTail will be hit and if hash is set, it will override that tail and fetch the hash + trigger sync up to head?

Wondertan · 2025-05-21T17:13:17Z

during subjective init, tail is guaranteed to be written first, even if incoming head was there earlier. Other then that its correct

Adds `Tail` method to `Syncer` that can initialize a node with a height estimated header or using the configured height(or hash) as the new `Tail`. Besides, it can gracefully handle reconfiguration of height/hash upon restarts. ~This change deliberately leaves some tech debt behind. The new Tail method has quite a lot of branchiness and could be teared apart/rewritten reducing nesting and improving readability. Some more specific debts are marked with a TODO. All of this is not "clean" and simply achieves the goal. The future backward sync will anyway require us to refactor this logic, so there is minimum value in cleaning things now.~ Nvm, I end up refactoring it for the sake of reviewers sanity and code maintainability. There are still TODOs left for things that are literally blocked on bsync or else, but everything else was uniformly integrated into the existing code. Depends on #283 and #285 Closes #258 and #259

Wondertan added the kind:feat label Mar 31, 2025

Wondertan self-assigned this Mar 31, 2025

Wondertan force-pushed the sync-from-tail branch from bc832cf to 44a7fd9 Compare April 1, 2025 17:58

Wondertan force-pushed the sync-from-tail branch from 44a7fd9 to 5473b09 Compare May 6, 2025 21:04

Wondertan changed the base branch from main to pruning May 6, 2025 21:35

Wondertan marked this pull request as ready for review May 6, 2025 21:51

Wondertan requested review from tzdybal, renaynay, vgonkivs, gupadhyaya, walldiss and cristaloleg as code owners May 6, 2025 21:51

cristaloleg reviewed May 6, 2025

View reviewed changes

sync/sync_head.go Outdated Show resolved Hide resolved

Wondertan force-pushed the sync-from-tail branch from b569828 to 65a3e3a Compare May 7, 2025 23:34

cristaloleg reviewed May 8, 2025

View reviewed changes

sync/sync_head.go Outdated Show resolved Hide resolved

Wondertan force-pushed the sync-from-tail branch from 3b7e71f to 53307d0 Compare May 8, 2025 13:28

Wondertan changed the title ~~feat(sync): subjective tail~~ feat(sync): Tail May 8, 2025

Wondertan added the pruning label May 8, 2025

Wondertan mentioned this pull request May 8, 2025

store: DeleteTo doesn't remove Head/Tail #284

Closed

Wondertan commented May 11, 2025

View reviewed changes

sync/sync_tail.go Outdated Show resolved Hide resolved

Wondertan marked this pull request as draft May 11, 2025 01:25

Wondertan changed the title ~~feat(sync): Tail~~ feat(sync): maintain tail May 11, 2025

Wondertan added 7 commits May 13, 2025 03:14

feat(sync): subjectiveTail

115af45

fix head

c698d74

lint

f87e8b4

add diff handling and minor other improvements

f97fe87

lint + combine tests

9faaccd

refine logs/metrics

96b450b

tests for reinit

d37c42b

add pruningwindow

cb71de0

Wondertan added 2 commits May 13, 2025 23:48

lint

eb96e98

add option

88e6f69

Wondertan marked this pull request as ready for review May 13, 2025 21:50

Wondertan added 2 commits May 14, 2025 17:35

estimation test

a99d7ac

resolve potential rare but possible case that will annoy the user

630ffa3

Wondertan force-pushed the sync-from-tail branch from 01b277e to 630ffa3 Compare May 14, 2025 16:41

renaynay reviewed May 14, 2025

View reviewed changes

last refactor beautifcation

31d7e13

Wondertan force-pushed the sync-from-tail branch from d936810 to 31d7e13 Compare May 14, 2025 21:05

minor findings

6613081

renaynay approved these changes May 15, 2025

View reviewed changes

sync/sync_tail.go Outdated Show resolved Hide resolved

Wondertan commented May 15, 2025

View reviewed changes

sync/sync_tail.go Outdated Show resolved Hide resolved

Wondertan added 5 commits May 16, 2025 18:13

minor fixes

8dec451

add test and clarify docs

2931553

fix tail hash logic

0b91ae8

better file name

b413137

fix security concern by never calling subjective tail before verifyin…

86cd2fb

…g gossiped headers

renaynay approved these changes May 21, 2025

View reviewed changes

Wondertan merged commit 57411f0 into pruning May 21, 2025
2 checks passed

Wondertan deleted the sync-from-tail branch May 21, 2025 17:13

This was referenced May 28, 2025

sync: Subjective Tail #258

Closed

sync: Prune on new head #259

Closed

feat(sync): maintain tail #265

feat(sync): maintain tail #265

Uh oh!

Conversation

Wondertan commented Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Wondertan commented May 11, 2025

Uh oh!

codecov-commenter commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

renaynay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wondertan commented May 14, 2025

Uh oh!

Wondertan commented May 14, 2025

Uh oh!

renaynay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

renaynay left a comment

Choose a reason for hiding this comment

Uh oh!

Wondertan commented May 21, 2025

Uh oh!

Uh oh!

Uh oh!

Wondertan commented Mar 31, 2025 •

edited

Loading

codecov-commenter commented May 13, 2025 •

edited

Loading