test(kt-devnet): add batcher failover kurtosis test #33
Conversation
samlaf
force-pushed
the
feat--op-batcher-altda-failover-to-ethda
branch
from
c548b50 to
2a78e16
Compare
samlaf
force-pushed
the
test--add-batcher-failover-kurtosis-test
branch
from
360b1f6 to
c52d9f2
Compare
| } | ||
|
|
||
| // Fetches all the batch-inbox posted commitments from blockNum (inclusive) to current block. | ||
| func fetchBatcherTxsSinceBlock(gethL1Endpoint string, batchInbox string, blockNum uint64) ([]BatcherTx, error) { |
There was a problem hiding this comment.
are there no constructions that are already exist in state derivation pipeline that can be leveraged for reading batcher txs instead?
| // 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. |
There was a problem hiding this comment.
should we add a comment describing the parallelism limitations?
| // 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) { |
There was a problem hiding this comment.
will we eventually wanna add tests ensuring failover to?
- keccak commitment mode
- 4844
There was a problem hiding this comment.
keccak yes probably, I'll add a comment. 4844 I don't know... it's hard to implement and not a priority for me.
| // 1. Check that the original commitments are EigenDA | ||
| harness.requireBatcherTxsToBeFromLayer(t, sinceBlock, DALayerEigenDA) | ||
|
|
||
| // 2. Failover and check that the commitments are now EthDA |
There was a problem hiding this comment.
EthDA is just calldata here?
| }) | ||
|
|
||
| // 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") |
There was a problem hiding this comment.
magic number 10 is used in a lot of places - consider using a variable
| // 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 |
There was a problem hiding this comment.
not currently used. is it planned for future use, or just there as a template for future tests that might need it?
| if batcherTx.daLayer != expectedLayer { | ||
| wrongCommitmentsToDiscard++ | ||
| } | ||
| // as soon as we see 3 ethDA commitments, or an EigenDA commitment, we stop |
There was a problem hiding this comment.
doc seems to describe a previous version of the code which wasn't generalized to expectedLayer
| // Get the kurtosis tag | ||
| tag := field.Tag.Get("kurtosis") | ||
| if tag == "" { | ||
| continue // Skip fields without tags |
There was a problem hiding this comment.
What would be the reason for fields to exist without the tag? Is the idea that you'd add kurtosis fields to production structs?
| // NewServiceEndpoint string `kurtosis:"new-service-name,port-name"` | ||
| } | ||
|
|
||
| func getEndpointsFromKurtosis(enclaveCtx *enclaves.EnclaveContext) (*EnclaveServiceEndpoints, error) { |
There was a problem hiding this comment.
A doc here would be helpful. It looks like this method replaces the service name in the EnclaveServiceEndpoints struct with a valid localhost endpoint?
| // 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. |
There was a problem hiding this comment.
nit: weird formatting here, with newline and tab mid-sentence
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.