feat(sync): Implement a subjectiveHead atomic pointer on the node to use #108

renaynay · 2023-09-19T10:01:54Z

Resolves #99

codecov-commenter · 2023-09-20T13:54:09Z

Codecov Report

Merging #108 (91d975a) into main (e7d24b8) will increase coverage by 0.15%.
The diff coverage is 74.11%.

@@            Coverage Diff             @@
##             main     #108      +/-   ##
==========================================
+ Coverage   69.09%   69.25%   +0.15%     
==========================================
  Files          37       37              
  Lines        2938     2979      +41     
==========================================
+ Hits         2030     2063      +33     
- Misses        758      768      +10     
+ Partials      150      148       -2

Files	Coverage Δ
sync/sync_store.go	`83.33% <100.00%> (+2.08%)`	⬆️
sync/sync_head.go	`71.12% <94.11%> (+1.89%)`	⬆️
headertest/dummy_suite.go	`87.50% <28.57%> (-12.50%)`	⬇️
headertest/dummy_header.go	`72.58% <40.00%> (-0.28%)`	⬇️
sync/sync.go	`73.52% <79.16%> (+0.38%)`	⬆️

... and 1 file with indirect coverage changes

liamsi · 2023-09-21T09:00:32Z

sync/sync_head.go

@@ -145,8 +161,12 @@ func (s *Syncer[H]) incomingNetworkHead(ctx context.Context, head H) error {
 	}

 	// TODO(@Wondertan):
-	//  Implement setSyncTarget and use it for soft failures
-	s.setSubjectiveHead(ctx, head)
+	//  Implement setSyncTarget and use it for soft failures // TODO @renaynay <-can remove this comment?


TODO @liamsi ask rene and hlib if these two todos can be removed now? 😅

liamsi · 2023-09-21T09:02:03Z

sync/fork_test/fork.go

+	// this sleep is necessary to allow the syncer to trigger a job
+	// as calling SyncWait prematurely may falsely return without error
+	// as the syncer has not yet registered a sync job.
+	//time.Sleep(time.Millisecond * 100)


can this be removed?

sync/fork_test/fork.go

distractedm1nd · 2023-09-22T16:49:46Z

headertest/dummy_header.go

+
+func (edv ErrDummyVerify) Error() string {
+	return edv.Reason
+}

 type DummyHeader struct {
 	Chainid      string
 	PreviousHash header.Hash
 	HeightI      uint64
 	Timestamp    time.Time



Suggested change

distractedm1nd · 2023-09-22T16:53:49Z

sync/sync.go

+	// if there's no subjective head stored, syncer has already applied the
+	// subjective head to the store and the syncer has passed it
+	if sbjHead != nil {
+		sh := *sbjHead


why do you need to deref here? Can't you do it on sbjHead def or cast? either way, please do it on line 333 if possible

distractedm1nd · 2023-09-22T16:59:52Z

sync/sync.go

+	if len(headers) == 0 {
+		s.metrics.recordTotalSynced(totalHeaders)
+		return nil
+	}


Is this necessary? I don't know why we can't just leave it how it was

cmwaters

Looks good. Still trying to wrap my head around how the mechanism works

cmwaters · 2023-09-27T10:53:23Z

sync/fork_test/fork.go

+// headers between its storeHead --> subjectiveHead.
+type eclipsedExchange struct {
+	trustedPeer      header.Store[*headertest.DummyHeader]
+	eclipsedExchange header.Store[*headertest.DummyHeader]


nit: keep it consistent with trustedPeer so we understand there are two different peers

Suggested change

eclipsedExchange header.Store[*headertest.DummyHeader]

eclipsedPeer header.Store[*headertest.DummyHeader]

cmwaters · 2023-09-27T10:54:31Z

sync/sync_fork_test.go

+	cmd.Stdout = os.Stderr
+	cmd.Stderr = os.Stderr
+	err = cmd.Run()
+	require.Error(t, err)


Do you want to make a specific assertion to the error that is generated

the assertion is below

cmwaters · 2023-09-27T11:06:10Z

sync/sync_fork_test.go

+	require.NoError(t, err)
+
+	cmd := exec.Command("go", "run", path)
+	cmd.Stdout = os.Stderr


Is it important to catch stdout?

nice to be able to catch the output, but not important

cmwaters · 2023-09-27T11:09:43Z

sync/sync.go

@@ -322,13 +331,44 @@ func (s *Syncer[H]) requestHeaders(

 // storeHeaders updates store with new headers and updates current syncStore's Head.
 func (s *Syncer[H]) storeHeaders(ctx context.Context, headers ...H) error {


Why is this check done in storeHeaders? I would have thought this would happen during verification (that we check if it's the height of the subjective head and compare the hashed)

Wondertan · 2023-09-29T16:07:30Z

sync/sync_fork_test.go

+func TestForkFollowingPrevention(t *testing.T) {
+	path, err := filepath.Abs("./fork_test/fork.go")
+	require.NoError(t, err)
+
+	cmd := exec.Command("go", "run", path)
+	cmd.Stdout = os.Stderr


[complement]
This is smart, and I like it.

Wondertan · 2023-09-29T16:59:11Z

sync/sync.go

+		headers = headers[segmentSize:]
+
+		err := s.store.Append(ctx, mustApplySegment...)
+		if err != nil {


[blocking]
This must be a verification error only.

I agree and also would prefer if we could also assess whether it failed specifically on the height sbjHead-1 against sbjHead

Wondertan · 2023-10-01T08:03:22Z

sync/sync_head.go

@@ -145,8 +161,12 @@ func (s *Syncer[H]) incomingNetworkHead(ctx context.Context, head H) error {
 	}

 	// TODO(@Wondertan):
-	//  Implement setSyncTarget and use it for soft failures
-	s.setSubjectiveHead(ctx, head)
+	//  Implement setSyncTarget and use it for soft failures // TODO @renaynay <-can remove this comment?


[discussion]
We can keep it until we make setSyncTarget method

Wondertan · 2023-10-01T08:10:15Z

sync/sync.go

+	var sbjHeadHeight uint64
+	sbjHead := *s.sbjHead.Load()
+	// if there's no subjective head stored, syncer has already applied the
+	// subjective head to the store and the syncer has passed it
+	if !sbjHead.IsZero() {
+		sbjHeadHeight = sbjHead.Height()
+		// sanity check the height to check whether it's within the range
+		if sbjHeadHeight < headers[0].Height() || sbjHeadHeight > headers[len(headers)-1].Height() {
+			sbjHeadHeight = 0
+		}
+	}
+
+	// if the header is within range, create a segment that *must* be applied
+	// such that the syncer fails if the subjective head cannot be applied
+	if sbjHeadHeight != 0 {
+		segmentSize := sbjHeadHeight - headers[0].Height() + 1
+		mustApplySegment := make([]H, segmentSize)
+		copy(mustApplySegment, headers[:segmentSize])
+		headers = headers[segmentSize:]
+
+		err := s.store.Append(ctx, mustApplySegment...)
+		if err != nil {
+			log.Fatal(fmt.Errorf("failed to apply trusted head at height %d to synced chain segment, "+
+				"potentially following fork: %w", sbjHeadHeight, err))
+		}
+		// if success, then clear subjective head as it's already applied as the store head
+		s.sbjHead.Store(new(H))
+	}


[suggestion]
Separate this code block with { and } and add a TODO pointing to backwards sync issue

renaynay · 2023-10-02T15:22:27Z

Something is borked with sync.

as fork-following prevention

… the syncer crashes if it detects that it has been following a fork

renaynay · 2023-10-05T09:24:57Z

Nvm bug was in main

renaynay force-pushed the fork-following-prevention branch from c9352b5 to bcc80f8 Compare September 20, 2023 13:50

renaynay marked this pull request as ready for review September 20, 2023 14:16

renaynay requested review from tzdybal, Wondertan, vgonkivs and gupadhyaya as code owners September 20, 2023 14:16

liamsi reviewed Sep 21, 2023

View reviewed changes

sync/fork_test/fork.go Outdated Show resolved Hide resolved

liamsi reviewed Sep 21, 2023

View reviewed changes

sync/fork_test/fork.go Outdated Show resolved Hide resolved

distractedm1nd requested changes Sep 22, 2023

View reviewed changes

renaynay requested review from distractedm1nd and liamsi September 25, 2023 12:45

cmwaters reviewed Sep 27, 2023

View reviewed changes

renaynay mentioned this pull request Sep 29, 2023

sync: Implement backwards sync #112

Open

Wondertan reviewed Oct 1, 2023

View reviewed changes

distractedm1nd assigned renaynay Oct 2, 2023

renaynay force-pushed the fork-following-prevention branch from 303940d to a343118 Compare October 2, 2023 15:14

renaynay requested review from Wondertan and cmwaters October 2, 2023 15:18

renaynay marked this pull request as draft October 2, 2023 15:26

renaynay added 7 commits October 5, 2023 11:23

feat(sync): Implement a subjectiveHead atomic pointer on the node to use

1d56342

as fork-following prevention

doc(sync): add doc to explain sbjHead

7907db7

test(sync/fork): Implement a roundabout fork following test to ensure…

b124d06

… the syncer crashes if it detects that it has been following a fork

better assert

5f2140a

test(sync/fork_test): Fork chain at height 50 instead of height 1

9200ba0

chore(sync): implement ryan suggestions

b267d6e

chore(sync): store zero H value on sbjHead pointer

0c1bf29

renaynay added 3 commits October 5, 2023 11:23

fix(sync): ensure only fatal on verification error

2742b9e

chore(sync/fork_test): rename to eclipsedPeer for clarity

6bc2e87

rename err in dummy header

91d975a

renaynay force-pushed the fork-following-prevention branch from a6716a0 to 91d975a Compare October 5, 2023 09:24

renaynay marked this pull request as ready for review October 5, 2023 09:25

renaynay marked this pull request as draft October 11, 2023 14:00

Wondertan mentioned this pull request Oct 19, 2023

feat(proof/service)!: check that proof height is less than available threshold celestiaorg/go-fraud#36

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sync): Implement a subjectiveHead atomic pointer on the node to use #108

feat(sync): Implement a subjectiveHead atomic pointer on the node to use #108

renaynay commented Sep 19, 2023 •

edited

Loading

codecov-commenter commented Sep 20, 2023 •

edited

Loading

liamsi Sep 21, 2023

liamsi Sep 21, 2023

distractedm1nd Sep 22, 2023

distractedm1nd Sep 22, 2023

distractedm1nd Sep 22, 2023

cmwaters left a comment

cmwaters Sep 27, 2023

cmwaters Sep 27, 2023

renaynay Oct 2, 2023

cmwaters Sep 27, 2023

renaynay Oct 2, 2023

cmwaters Sep 27, 2023

Wondertan Sep 29, 2023

Wondertan Sep 29, 2023

renaynay Oct 2, 2023

Wondertan Oct 1, 2023

Wondertan Oct 1, 2023

renaynay commented Oct 2, 2023

renaynay commented Oct 5, 2023

	eclipsedExchange header.Store[*headertest.DummyHeader]
	eclipsedPeer header.Store[*headertest.DummyHeader]

		@@ -322,13 +331,44 @@ func (s *Syncer[H]) requestHeaders(

		// storeHeaders updates store with new headers and updates current syncStore's Head.
		func (s *Syncer[H]) storeHeaders(ctx context.Context, headers ...H) error {

feat(sync): Implement a subjectiveHead atomic pointer on the node to use #108

Are you sure you want to change the base?

feat(sync): Implement a subjectiveHead atomic pointer on the node to use #108

Conversation

renaynay commented Sep 19, 2023 • edited Loading

codecov-commenter commented Sep 20, 2023 • edited Loading

Codecov Report

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

cmwaters 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

renaynay commented Oct 2, 2023

renaynay commented Oct 5, 2023

renaynay commented Sep 19, 2023 •

edited

Loading

codecov-commenter commented Sep 20, 2023 •

edited

Loading