test(kt-devnet): add batcher failover kurtosis test by samlaf · Pull Request #33 · Layr-Labs/optimism

samlaf · 2025-02-26T18:12:21Z

Closes DAINT-303

This PR adds a golang failover test which uses the kurtosis devnet as backend.
This test is used to test the new batcher failover behavior from #34.
We will merge this test into that PR, and then that PR into eigenda-develop (our master branch).

Note to reviewers

Please be very harsh on the golang test, as this same framework will be reused to add new tests for future features.

The "proper" way to implement this would probably have been to reuse op's op-e2e framework, but create a way to populate this system (equivalent to our harness in this PR) from a kurtosis backend. This would have taken me a lot more time to figure out however, and I feel like its something that the OP team might create at some point, so prefer to let them put in the work there and figure out how to do it properly, and we can potentially move to using that at a future point.

…routes

ethenotethan · 2025-02-26T22:11:03Z

kurtosis-devnet/tests/eigenda/failover_test.go

+}
+
+// Fetches all the batch-inbox posted commitments from blockNum (inclusive) to current block.
+func fetchBatcherTxsSinceBlock(gethL1Endpoint string, batchInbox string, blockNum uint64) ([]BatcherTx, error) {


are there no constructions that are already exist in state derivation pipeline that can be leveraged for reading batcher txs instead?

Added note in 6111c52

Basically this feels easier for now. If we ever need to manage more complex state (like failing over to 4844 txs), then prob would be good to consider using L1Retriever instead.

ethenotethan · 2025-02-26T23:02:58Z

kurtosis-devnet/tests/eigenda/failover_test.go

+// We assume that this enclave is already running.
+const enclaveName = "eigenda-memstore-devnet"
+
+// TestFailover tests the failover behavior of the batcher, in response to the proxy returning 503 errors.


should we add a comment describing the parallelism limitations?

ethenotethan · 2025-02-26T23:06:42Z

kurtosis-devnet/tests/eigenda/failover_test.go

+// The test then toggles the failover back off and checks that the batcher starts submitting EigenDA batches again.
+// The batches inbox transactions are queried via geth's GraphQL API.
+// TODO: We will also need to test the failover behavior of the node, which currently doesn't finalize after failover (fixed in https://github.com/Layr-Labs/optimism/pull/23)
+func TestFailover(t *testing.T) {


will we eventually wanna add tests ensuring failover to?

keccak commitment mode

4844

keccak yes probably, I'll add a comment. 4844 I don't know... it's hard to implement and not a priority for me.

ethenotethan · 2025-02-26T23:07:04Z

kurtosis-devnet/tests/eigenda/failover_test.go

+	// 1. Check that the original commitments are EigenDA
+	harness.requireBatcherTxsToBeFromLayer(t, sinceBlock, DALayerEigenDA)
+
+	// 2. Failover and check that the commitments are now EthDA


EthDA is just calldata here?

litt3 · 2025-02-27T14:06:29Z

kurtosis-devnet/tests/eigenda/failover_test.go

+	})
+
+	// assume kurtosis is running and is at least at block 10 (just deploying the contracts takes more than 10 blocks)
+	require.GreaterOrEqual(t, harness.testStartL1BlockNum, uint64(10), "Test started too early in the chain")


magic number 10 is used in a lot of places - consider using a variable

Good callout. Hardcoded it and updated the graphql query functions to use both a FROM and a TO block. This then makes the semantics of the test much easier to understand, so I was able to document them more easily: 0dba0f2

litt3 · 2025-02-27T14:10:30Z

kurtosis-devnet/tests/eigenda/failover_test.go

+// Test Harness, which contains all the state needed to run the tests.
+// harness also defines some higher-level "require" methods that are used in the tests.
+type harness struct {
+	enclaveCtx          *enclaves.EnclaveContext


not currently used. is it planned for future use, or just there as a template for future tests that might need it?

Good call, added just in case, but agree its cleaner to remove. Done: 2277e3a

litt3 · 2025-02-27T14:19:13Z

kurtosis-devnet/tests/eigenda/failover_test.go

+		if batcherTx.daLayer != expectedLayer {
+			wrongCommitmentsToDiscard++
+		}
+		// as soon as we see 3 ethDA commitments, or an EigenDA commitment, we stop


doc seems to describe a previous version of the code which wasn't generalized to expectedLayer

litt3 · 2025-02-27T14:27:39Z

kurtosis-devnet/tests/eigenda/failover_test.go

+		// Get the kurtosis tag
+		tag := field.Tag.Get("kurtosis")
+		if tag == "" {
+			continue // Skip fields without tags


What would be the reason for fields to exist without the tag? Is the idea that you'd add kurtosis fields to production structs?

None, good catch. This was claude generated. I updated to return error instead: 80fcebf

litt3 · 2025-02-27T14:36:35Z

kurtosis-devnet/tests/eigenda/failover_test.go

+	// NewServiceEndpoint   string `kurtosis:"new-service-name,port-name"`
+}
+
+func getEndpointsFromKurtosis(enclaveCtx *enclaves.EnclaveContext) (*EnclaveServiceEndpoints, error) {


A doc here would be helpful. It looks like this method replaces the service name in the EnclaveServiceEndpoints struct with a valid localhost endpoint?

Super good callout thanks, I even had to figure out for myself why I was using the localhost endpoints instead of the private ip:port endpoints which are more efficient. Documented in 3065622

litt3 · 2025-02-27T14:48:58Z

kurtosis-devnet/tests/eigenda/failover_test.go

+// Update the proxy's memstore config to start returning 503 errors
+// Note: we have to GetConfig, update it and then UpdateConfig because the client doesn't implement a "patch" method,
+//
+//	even though the API does support it.


nit: weird formatting here, with newline and tab mid-sentence

Was not being used and I had put it there only "in case"

Define l1BlocksQueriedForBatcherTxs and better describe how tests use it Also refactored graphql query to use both FROM and TO blocks, not only FROM. This makes the semantics of the tests easier to understand.

…unction

…ServiceEndpoints field doesn't have kurtosis tag

…ndpointsFromKurtosis Also better document the function to explain what its doing

…etriever api

…run sequentially

This is done to reflect the fact that batcher currently doesn't support failing over to calldata.

ethenotethan

LGTM 🛳️

kurtosis-devnet/tests/eigenda/failover_test.go

* test(altda): add test for altda->ethda failover * feat(batcher): altda->ethda failover when altda is down * chore: fix typos * fix(fakeDAServer): handlePut was still handling put when in failover mode * Update op-batcher/batcher/driver.go Co-authored-by: Gaston Ponti <pontigaston@gmail.com> * chore: better logs in batcher * test(kt-devnet): add batcher failover kurtosis test (#33) * test(kt-devnet): add batcher failover test * chore(kt-devnet): use new proxy image v1.6.5 which has memstore rest routes * test(kt-devnet): fix TestFailover + add comments/logs * ci(kt-devnet): run new kurtosis failover test in ci * test(kt-devnet): remove enclaveCtx from test harness Was not being used and I had put it there only "in case" * test(kt-devnet): refactor failover test to not use hardcoded 10 constant Define l1BlocksQueriedForBatcherTxs and better describe how tests use it Also refactored graphql query to use both FROM and TO blocks, not only FROM. This makes the semantics of the tests easier to understand. * test(kt-devnet): better comments for requireBatcherTxsToBeFromLayer function * teest(kt-devnet): return err from getEndpointsFromKurtosis if EnclaveServiceEndpoints field doesn't have kurtosis tag * docs(kt-failover-test): rename getEndpointsFromKurtosis -> getPublicEndpointsFromKurtosis Also better document the function to explain what its doing * style(kt-devnet): reformat a weirdly tabbed comment * docs(kt-failover-test): describe why we use graphql api insead of l1retriever api * docs(kt-failover-test): add note mentioning that kt tests need to be run sequentially * docs(kt-failover-test): update test name to TestFailoverToEthDACalldata This is done to reflect the fact that batcher currently doesn't support failing over to calldata. * style: fix lint * docs(e2eutils): document returned values for WaitForBlockWithTxFromSender * docs(e2eutils): add doc comment for TransactionsBySender * docs(op-e2e): remove wrong comment in failover_test * docs(op-e2e): better test comment in failover_test * style(op-e2e): merged 2 if statements into one * style: fix comment typo --------- Co-authored-by: Gaston Ponti <pontigaston@gmail.com>

samlaf marked this pull request as draft February 26, 2025 18:12

samlaf force-pushed the feat--op-batcher-altda-failover-to-ethda branch from c548b50 to 2a78e16 Compare February 26, 2025 18:20

test(kt-devnet): add batcher failover test

c52d9f2

samlaf force-pushed the test--add-batcher-failover-kurtosis-test branch from 360b1f6 to c52d9f2 Compare February 26, 2025 18:24

samlaf added 3 commits February 26, 2025 14:55

chore(kt-devnet): use new proxy image v1.6.5 which has memstore rest …

8a1e0af

…routes

test(kt-devnet): fix TestFailover + add comments/logs

84cec1e

ci(kt-devnet): run new kurtosis failover test in ci

8a1e073

samlaf marked this pull request as ready for review February 26, 2025 19:58

samlaf requested review from bxue-l2, cody-littley, ethenotethan and litt3 February 26, 2025 20:01

ethenotethan reviewed Feb 26, 2025

View reviewed changes

litt3 reviewed Feb 27, 2025

View reviewed changes

samlaf added 6 commits March 3, 2025 11:12

test(kt-devnet): remove enclaveCtx from test harness

2277e3a

Was not being used and I had put it there only "in case"

test(kt-devnet): refactor failover test to not use hardcoded 10 constant

0dba0f2

Define l1BlocksQueriedForBatcherTxs and better describe how tests use it Also refactored graphql query to use both FROM and TO blocks, not only FROM. This makes the semantics of the tests easier to understand.

test(kt-devnet): better comments for requireBatcherTxsToBeFromLayer f…

c63c4d0

…unction

teest(kt-devnet): return err from getEndpointsFromKurtosis if Enclave…

80fcebf

…ServiceEndpoints field doesn't have kurtosis tag

docs(kt-failover-test): rename getEndpointsFromKurtosis -> getPublicE…

3065622

…ndpointsFromKurtosis Also better document the function to explain what its doing

style(kt-devnet): reformat a weirdly tabbed comment

85b5c12

samlaf requested a review from litt3 March 3, 2025 18:55

samlaf added 4 commits March 3, 2025 14:44

docs(kt-failover-test): describe why we use graphql api insead of l1r…

6111c52

…etriever api

docs(kt-failover-test): add note mentioning that kt tests need to be …

8047e72

…run sequentially

docs(kt-failover-test): update test name to TestFailoverToEthDACalldata

bedc45f

This is done to reflect the fact that batcher currently doesn't support failing over to calldata.

style: fix lint

703500e

samlaf requested a review from ethenotethan March 3, 2025 20:04

ethenotethan approved these changes Mar 4, 2025

View reviewed changes

litt3 approved these changes Mar 4, 2025

View reviewed changes

kurtosis-devnet/tests/eigenda/failover_test.go Show resolved Hide resolved

samlaf merged commit 7126821 into feat--op-batcher-altda-failover-to-ethda Mar 4, 2025
7 checks passed

samlaf deleted the test--add-batcher-failover-kurtosis-test branch March 4, 2025 15:51

Conversation

samlaf commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Note to reviewers

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!

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!

ethenotethan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

samlaf commented Feb 26, 2025 •

edited

Loading