fix(store): track store's contiguous head #239

cristaloleg · 2025-01-08T11:26:57Z

Introduce a new unexported Store field which tracks the highest contiguous header observed.
Rework heightSub to work only with height and not headers (drastically simplified internals and API)
Load headKey on Store.Start
Store.Head is much simpler, again
batch will be reworked in the next PR.

Fixes #201

store/store.go

store/heightsub.go

store/store.go

store/store_test.go

p2p/server_test.go

store/batch.go

store/heightsub.go

sync/sync_test.go

Wondertan

We are getting there

Besides the comments, there are two more cases for head/subscription handling we need to support:

Getting headers above contigious head

Height == 100
Append 150
GetByHeight 150 -> no error

Subscribing for headers above contigious head

Height == 100
goA GetByHeight(150) -> block
Append(150) -> no error
goA -> unblock, no error

We can also do them in a follow up

store/heightsub.go

store/store.go

store/heightsub.go

store/store.go

Coming out as a result of reviewing #239

store/store.go

Wondertan · 2025-01-27T15:27:39Z

Let's add more more info on what has changed in the description

store/batch.go

renaynay · 2025-01-29T12:43:29Z

store/heightsub.go

+	for h := range hs.heightSubs {
+		if h < height {
+			hs.notify(h, true)
+		}


In which situation would there be active subscriptions on an un-initialised heightsub instance (which means an uninitialised header store)? Just curious

We have test TestBatch_GetByHeightBeforeInit which can invoke a method on non-inited store. To keep this behaviour (at least in this RP) we need to handle this case properly in heightSub.

Is it necessary that heightsub should be able to be re-initialised with pre-existing subscriptions still on it?

Its not see #243

Can be resolved?

I mean yes, it's just still unclear to me in which case there are waiters on a unintialised heightsub in current use of the lib

store/heightsub_test.go

store/store.go

renaynay · 2025-01-29T15:27:09Z

store/store.go

+
+	newHeight := s.advanceContiguousHead(ctx, h.Height())
+	if newHeight >= h.Height() {
+		s.contiguousHead.Store(&h)


wouldn't this already happen in updateContiguousHead?

It might not happen if we will not find a higher head due to if currHeight > prevHeight {.

That's why we do here >= and update.

Why can't this check be de-duplicated and left only to advanceContiguousHead where it can check for >= on #L541 ?

Well, can be. The only things is that we will do a bit more often headKey updates. I don't see a problem with that, done.

Ah, no. We need a real header (newHead) to do updateContiguousHead. Adding >= will complicate code for not a little reason. Reverted.

Can be resolved?

I still do not understand why we are doing this check, in different formats, in 2 different places. It's complicated.

After today's changes this can be simplified even more. Done.

store/store.go

Wondertan

Another pass with multiple simplifications

store/store.go

codecov-commenter · 2025-02-04T13:13:12Z

Codecov Report

Attention: Patch coverage is 90.29851% with 13 lines in your changes missing coverage. Please review.

Project coverage is 64.21%. Comparing base (88c5b8c) to head (ac051e4).
Report is 27 commits behind head on main.

Files with missing lines	Patch %	Lines
store/heightsub.go	86.66%	6 Missing and 2 partials ⚠️
store/store.go	92.75%	3 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #239      +/-   ##
==========================================
+ Coverage   62.80%   64.21%   +1.41%     
==========================================
  Files          39       38       -1     
  Lines        3589     3641      +52     
==========================================
+ Hits         2254     2338      +84     
+ Misses       1160     1133      -27     
+ Partials      175      170       -5

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

Wondertan

Massive

Lets make sure we keep PR description with necessary details and have next PR to fix batch scheduled

Wondertan

Approve with a bunch of nits

store/store.go

cristaloleg · 2025-02-05T09:47:23Z

store/store.go

+func (s *Store[H]) nextContiguousHead(ctx context.Context, height uint64) H {
+	var newHead H
+	for {
+		height++


Should we fetch just height first to see that the header exists at all and start looking for the next after?

No idea, just have realised that there is a window of opportunity that we might look for a contiguous headers when initial doesn't exist (which sounds like a disk datastore corruption or dunno).

renaynay · 2025-02-11T11:51:02Z

store/store.go

-	if err == nil {
-		return head, nil
+func (s *Store[H]) Head(_ context.Context, _ ...header.HeadOption[H]) (H, error) {
+	if head := s.contiguousHead.Load(); head != nil {


I'm pretty sure this will break the initialisation behaviour in celestia-node. Did you test this PR with celestia-node?

Celestia-node calls the Init function from inside the store package here in go-header, which relies on reading the Head from the store (s) before it calls s.Init (which is unsafe btw - as Init can technically take a header whose height is < actual head height on disk, initialises heightsub with it, stores it in contiguousHead field, and then flushes it to disk which replaces the actual head key).

This means that if the node is trying to use the store.Init function to check whether the node's header store is actually initialised or not (which happens before the store is actually started), then Head will always return ErrNoHead as contiguousHead is not set at that point, causing the node to re-fetch the genesis header and unsafe init the header store with it again.

There is a helper Init function in store pkg that we use instead of calling the Store.Init directly.

I added a comment to #243 (which stoles idea from 246)

@cristaloleg This still does not address the problem that this PR breaks previous behaviour of Head and is incomplete without addressing the issue of unsafe Init + ordering of initialisation vs. Start

The catch makes sense. Propose to return lazy loading in Head, yet keeping it on Start as well, or reworking Init helper to call Start instead of Head

renaynay · 2025-02-11T11:53:45Z

store/store.go

 	// closed s.writesDn means that store was stopped before, recreate chan.
 	select {
 	case <-s.writesDn:
 		s.writesDn = make(chan struct{})
 	default:
 	}

+	if err := s.loadContiguousHead(ctx); err != nil {
+		// we might start on an empty datastore, no key is okay.


is it necessary to change this behaviour in this PR?

Scope is large and this feels like an unnecessary change to group in.

Well, @Wondertan your word

It is, otherwise there is a bug explained in some earlier thread.

In short we need to have contiguousHead set on start, 'cause we need to update it without gaps. That's why this should happen here.

(Sorry, somehow this wasn't send but msg above was)

renaynay · 2025-02-13T10:54:34Z

store/store.go

-	if head := s.contiguousHead.Load(); head != nil {
-		return *head, nil
+func (s *Store[H]) Head(ctx context.Context, _ ...header.HeadOption[H]) (H, error) {
+	head, err := s.GetByHeight(ctx, s.heightSub.Height())


You can also try check loading contiguousHead here and if nil, then try readHead

Wondertan

Approve from my side :)

Massive work. Thank you @cristaloleg!

cristaloleg added the kind:fix label Jan 8, 2025

cristaloleg self-assigned this Jan 8, 2025

cristaloleg requested review from tzdybal, renaynay, Wondertan, vgonkivs, gupadhyaya and walldiss as code owners January 8, 2025 11:26

Wondertan reviewed Jan 8, 2025

View reviewed changes

store/store.go Outdated Show resolved Hide resolved

store/heightsub.go Outdated Show resolved Hide resolved

store/store.go Show resolved Hide resolved

store/store_test.go Show resolved Hide resolved

Wondertan reviewed Jan 9, 2025

View reviewed changes

store/store_test.go Outdated Show resolved Hide resolved

cristaloleg marked this pull request as draft January 10, 2025 13:09

Wondertan mentioned this pull request Jan 13, 2025

refactor(store): synchronize Store writes #241

Draft

cristaloleg marked this pull request as ready for review January 13, 2025 13:09

cristaloleg commented Jan 13, 2025

View reviewed changes

p2p/server_test.go Show resolved Hide resolved

cristaloleg commented Jan 13, 2025

View reviewed changes

store/batch.go Show resolved Hide resolved

cristaloleg commented Jan 13, 2025

View reviewed changes

store/heightsub.go Outdated Show resolved Hide resolved

cristaloleg commented Jan 13, 2025

View reviewed changes

sync/sync_test.go Outdated Show resolved Hide resolved

Wondertan mentioned this pull request Jan 13, 2025

test(store): add multi-subscription heightSub testcase #242

Merged

Wondertan reviewed Jan 13, 2025

View reviewed changes

cristaloleg pushed a commit that referenced this pull request Jan 14, 2025

test(store): add multi-subscription heightSub testcase (#242)

3cf61a8

Coming out as a result of reviewing #239

cristaloleg added 10 commits January 14, 2025 14:49

fix(store): track store's contiguous head

46f8fad

more fixes

cbcb738

fix test

4bc4aa0

fix sync test

9e19840

fix sync test

cc8894c

test heightSub

73b65f2

simplify test

b74e825

wip

2a317ad

cleanup

1903695

simplify heightSub

268afc3

Wondertan reviewed Jan 27, 2025

View reviewed changes

store/store.go Outdated Show resolved Hide resolved

drop param

459baad

Wondertan mentioned this pull request Jan 28, 2025

store: Synchronous API #244

Open

renaynay reviewed Jan 29, 2025

View reviewed changes

cristaloleg added 3 commits January 30, 2025 10:23

review suggestions

9f1c92b

fix

d554c55

revert

034e618

Wondertan reviewed Feb 3, 2025

View reviewed changes

store/store.go Outdated Show resolved Hide resolved

store/store.go Outdated Show resolved Hide resolved

store/store.go Outdated Show resolved Hide resolved

store/store.go Outdated Show resolved Hide resolved

store/store.go Outdated Show resolved Hide resolved

Wondertan reviewed Feb 3, 2025

View reviewed changes

store/store.go Outdated Show resolved Hide resolved

cristaloleg added 5 commits February 4, 2025 13:19

more review suggestions

aa97c39

simplify

f2d860c

simplify again

5518222

simplify

aef55c4

linter

ac051e4

Wondertan previously approved these changes Feb 4, 2025

View reviewed changes

think different

35856eb

cristaloleg dismissed Wondertan’s stale review via 35856eb February 4, 2025 16:47

Wondertan previously approved these changes Feb 4, 2025

View reviewed changes

more suggestions

73b5c89

cristaloleg dismissed Wondertan’s stale review via 73b5c89 February 5, 2025 09:44

cristaloleg commented Feb 5, 2025

View reviewed changes

upd cmnt

84a7090

renaynay reviewed Feb 11, 2025

View reviewed changes

revert head method

921de51

renaynay reviewed Feb 13, 2025

View reviewed changes

revert but a bit

dc6d5bf

Wondertan approved these changes Feb 13, 2025

View reviewed changes

cristaloleg changed the base branch from main to feat-adj February 14, 2025 11:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(store): track store's contiguous head #239

fix(store): track store's contiguous head #239

cristaloleg commented Jan 8, 2025 •

edited

Loading

Wondertan left a comment

Wondertan commented Jan 27, 2025

renaynay Jan 29, 2025

cristaloleg Jan 30, 2025

renaynay Jan 30, 2025

Wondertan Jan 31, 2025

cristaloleg Feb 4, 2025

renaynay Feb 4, 2025

renaynay Jan 29, 2025

cristaloleg Jan 30, 2025

renaynay Jan 30, 2025

cristaloleg Jan 31, 2025

cristaloleg Jan 31, 2025

cristaloleg Feb 4, 2025

renaynay Feb 4, 2025

cristaloleg Feb 4, 2025

Wondertan left a comment

codecov-commenter commented Feb 4, 2025

Wondertan left a comment •

edited

Loading

Wondertan left a comment

cristaloleg Feb 5, 2025

renaynay Feb 11, 2025

Wondertan Feb 11, 2025

cristaloleg Feb 11, 2025

renaynay Feb 11, 2025

Wondertan Feb 11, 2025

cristaloleg Feb 14, 2025

renaynay Feb 11, 2025

cristaloleg Feb 11, 2025

Wondertan Feb 11, 2025

cristaloleg Feb 11, 2025

renaynay Feb 13, 2025

cristaloleg Feb 13, 2025

Wondertan left a comment

fix(store): track store's contiguous head #239

Are you sure you want to change the base?

fix(store): track store's contiguous head #239

Conversation

cristaloleg commented Jan 8, 2025 • edited Loading

Wondertan left a comment

Choose a reason for hiding this comment

Wondertan commented Jan 27, 2025

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wondertan left a comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 4, 2025

Codecov Report

Wondertan left a comment • edited Loading

Choose a reason for hiding this comment

Wondertan 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wondertan left a comment

Choose a reason for hiding this comment

cristaloleg commented Jan 8, 2025 •

edited

Loading

Wondertan left a comment •

edited

Loading