feat: EigenDA failover #46

epociask · 2024-11-02T08:35:48Z

TLDR: Added failover from ETH DA to EigenDA in the event of detected 503 or service unavailable errors.

arbnode/batch_poster.go

samlaf · 2024-11-03T17:57:12Z

arbnode/batch_poster.go

@@ -125,6 +125,7 @@ type BatchPoster struct {
 	batchReverted        atomic.Bool // indicates whether data poster batch was reverted
 	nextRevertCheckBlock int64       // the last parent block scanned for reverting batches
 	postedFirstBatch     bool        // indicates if batch poster has posted the first batch
+	failoverETHDA        bool        // indicates if batch poster should failover to ETHDA


I would add more context here, like "when a failover signal is received from proxy". Or "indicates whether batch posted is allowed to failover" or something

samlaf · 2024-11-03T17:58:23Z

arbnode/batch_poster.go

@@ -148,6 +149,7 @@ type BatchPosterDangerousConfig struct {
 type BatchPosterConfig struct {
 	Enable                             bool `koanf:"enable"`
 	DisableDapFallbackStoreDataOnChain bool `koanf:"disable-dap-fallback-store-data-on-chain" reload:"hot"`
+	EnableEigenDAFailover              bool `koanf:"eigenda-failover-to-anytrust" reload:"hot"`


Oh they use koanf, cool! Been meaning to give it a try. Do you like it?

Also check the lint issue, the flag name and tag are different.

samlaf · 2024-11-03T18:00:51Z

arbnode/batch_poster.go

@@ -1241,7 +1253,7 @@ func (b *BatchPoster) maybePostSequencerBatch(ctx context.Context) (bool, error)
 		}

 		var useEigenDA bool
-		if b.eigenDAWriter != nil {
+		if b.eigenDAWriter != nil && !b.failoverETHDA {


add comment explaining the logic here. Why are you setting useEigenDA to true only when we don't failover?

Don't we still want to use eigenda until we failover?

arbnode/node.go

samlaf · 2024-11-03T18:21:01Z

arbnode/node.go

+		return nil, errors.New("eigenDA and anytrust cannot both be enabled without EnableEigenDAFailover=true in batch poster config")
+	}
+
+	if config.EigenDA.Enable { // anytrust is enabled as an EigenDA failover


how can I know that anytrust is enabled here? if just seems to check that eigenda is enabled

samlaf · 2024-11-03T18:25:01Z

arbnode/batch_poster.go

+			// to update an internal field and retrigger the poster's event loop. Since this update should be down across batch posters on a distributed cluster, we
+			// must also


typo: down -> done

also comment unfinished

arbnode/batch_poster.go

samlaf · 2024-11-03T18:27:35Z

system_tests/eigenda_test.go

+	defer func() {
+		cancel()
+	}()


Suggested change

defer func() {

cancel()

}()

defer cancel()

…script logic

samlaf · 2024-11-20T10:21:36Z

@epociask my baaaddddd..... :( :(
Was doing work on a bunch of proxy PRs and merging them, and merged this one as well by mistake.

samlaf

some nits

samlaf · 2024-11-20T10:57:09Z

arbnode/batch_poster.go

+	batchReverted          atomic.Bool // indicates whether data poster batch was reverted
+	nextRevertCheckBlock   int64       // the last parent block scanned for reverting batches
+	postedFirstBatch       bool        // indicates if batch poster has posted the first batch
+	eigenDAFailoverToETHDA bool        // indicates if batch poster should failover to ETHDA


eigenDAFailoverToETHDA sounds like a config variable and not a state variable. Consider renaming to failedOverToETHDA or something more "stateful". Right now it sounds like a config flag that just failover is enabled.

samlaf · 2024-11-20T11:00:11Z

arbnode/batch_poster.go

+		if err != nil && errors.Is(err, eigenda.SvcUnavailableErr) && b.config().EnableEigenDAFailover && b.dapWriter != nil { // Failover to anytrust commitee if enabled
+			log.Error("EigenDA service is unavailable, failing over to any trust mode")
+			b.building.useEigenDA = false
+			failOver = true
+		}
+
+		if err != nil && errors.Is(err, eigenda.SvcUnavailableErr) && b.config().EnableEigenDAFailover && b.dapWriter == nil { // Failover to ETH DA if enabled
+			// when failing over to ETHDA (i.e 4844, calldata), we may need to re-encode the batch. To do this in compliance with the existing code, it's easiest


maybe refactor to have an outer if with the 3 same conditions and then an internal if/else only for the dapWriter. Took me a while to parse the difference, and to realized that dapWriter was probably a writer to anytrust.

samlaf · 2024-11-20T11:01:59Z

arbnode/batch_poster.go

+
+		if err != nil && errors.Is(err, eigenda.SvcUnavailableErr) && b.config().EnableEigenDAFailover && b.dapWriter == nil { // Failover to ETH DA if enabled
+			// when failing over to ETHDA (i.e 4844, calldata), we may need to re-encode the batch. To do this in compliance with the existing code, it's easiest
+			// to update an internal field and retrigger the poster's event loop. Since the batch poster can be distributed across mulitple nodes, there could be


nodes sounds a bit weird to me in this context. Is this standard arbitrum terminology? If not I would consider renaming to "multiple batch_posters". Node reminds me of too many different things like eth node, like k8s node, etc, but typically it means "a computer/physical server", and not a process (k8s pod)

samlaf · 2024-11-20T11:02:39Z

arbnode/batch_poster.go

+			// degraded temporary performance as each batch poster will re-encode the batch on another event loop tick using the coordination lock which could worst case
+			// could require every batcher instance to fail dispersal to EigenDA.


typo: could worst case could

samlaf · 2024-11-20T11:03:38Z

arbnode/batch_poster.go

-	if b.dapWriter != nil {
+	var eigenDaBlobInfo *eigenda.EigenDABlobInfo
+	eigenDADispersed := false
+	failOver := false


how is this failOver variable different from eigenDAFailoverToETHDA?

samlaf · 2024-11-20T11:04:13Z

arbnode/batch_poster.go

+			log.Error("EigenDA service is unavailable and anytrust is disabled, failing over to ETH DA")
+			b.eigenDAFailoverToETHDA = true
+
+			// // if the batch's size exceeds the native DA max size limit, we must re-encode the batch to accomodate the AnyTrust, calldata, and 4844 size limits


samlaf · 2024-11-20T11:05:54Z

arbnode/batch_poster.go

+			b.eigenDAFailoverToETHDA = true
+
+			// // if the batch's size exceeds the native DA max size limit, we must re-encode the batch to accomodate the AnyTrust, calldata, and 4844 size limits
+			if (len(sequencerMsg) > b.config().MaxSize && !b.building.use4844) || (len(sequencerMsg) > b.config().Max4844BatchSize && b.building.use4844) {


nit: consider changing order to make it cleaner:

if (b.building.use4844 && len(sequencerMsg) > b.config().Max4844BatchSize) || (!b.building.use4844 && len(sequencerMsg) > b.config().MaxSize)

samlaf · 2024-11-20T11:06:58Z

arbnode/batch_poster.go


-		batchPosterDASuccessCounter.Inc(1)
-		batchPosterDALastSuccessfulActionGauge.Update(time.Now().Unix())
+		} else if failOver {


can you use eigenDAFailoverToETHDA instead? Or are those variables different?

epociask added 2 commits November 2, 2024 04:35

feat: EigenDA failover

f877b48

feat: EigenDA failover - go fmt

92a13f0

samlaf reviewed Nov 3, 2024

View reviewed changes

samlaf marked this pull request as draft November 4, 2024 12:20

epociask added 6 commits November 4, 2024 14:06

feat: EigenDA failover - updated terminology and fixed bug in replay …

c940fdc

…script logic

feat: EigenDA failover - fix lint

3e7e707

feat: EigenDA failover - rename feature flag

dfa809a

feat: EigenDA failover - fix AnyTrust failover bug

9bb2e70

feat: EigenDA failover - update cfg name and add test comment

a49db8f

feat: EigenDA failover - go fmt

3aac169

epociask marked this pull request as ready for review November 7, 2024 20:31

epociask requested a review from samlaf November 9, 2024 04:18

samlaf merged commit 48843f8 into eigenda-v3.2.1 Nov 20, 2024
8 checks passed

samlaf reviewed Nov 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: EigenDA failover #46

feat: EigenDA failover #46

epociask commented Nov 2, 2024 •

edited

Loading

samlaf Nov 3, 2024

samlaf Nov 3, 2024

samlaf Nov 3, 2024

samlaf Nov 3, 2024

samlaf Nov 3, 2024

samlaf Nov 3, 2024

samlaf Nov 3, 2024

samlaf commented Nov 20, 2024

samlaf left a comment

samlaf Nov 20, 2024

samlaf Nov 20, 2024

samlaf Nov 20, 2024

samlaf Nov 20, 2024

samlaf Nov 20, 2024

samlaf Nov 20, 2024

samlaf Nov 20, 2024

samlaf Nov 20, 2024

		// to update an internal field and retrigger the poster's event loop. Since this update should be down across batch posters on a distributed cluster, we
		// must also

		// degraded temporary performance as each batch poster will re-encode the batch on another event loop tick using the coordination lock which could worst case
		// could require every batcher instance to fail dispersal to EigenDA.

feat: EigenDA failover #46

feat: EigenDA failover #46

Conversation

epociask commented Nov 2, 2024 • edited Loading

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

samlaf commented Nov 20, 2024

samlaf 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

epociask commented Nov 2, 2024 •

edited

Loading