feat: relax indexedChainState to ChainState for retrieval #943

hopeyen · 2024-12-02T17:11:31Z

Why are these changes needed?

Enable retrieval client to run without indexing

Checks

I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
I've checked the new test coverage and the coverage percentage didn't drop.
Testing Strategy
- Unit tests
- Integration tests
- This PR is not tested :(

ian-shim

lgtm

ian-shim · 2024-12-20T00:43:28Z

core/eth/state.go

@@ -28,7 +28,14 @@ func (cs *ChainState) GetOperatorStateByOperator(ctx context.Context, blockNumbe
 		return nil, err


can we move these methods to eth reader and remove this struct?
Not sure why we need a separate struct for this

If we're caching the operator sockets, maybe we should keep this struct separate

ian-shim · 2024-12-20T00:45:34Z

core/eth/state.go

+// buildSocketMap returns a map from operatorID to socket address for the operators in the operatorsByQuorum
+func (cs *ChainState) buildSocketMap(ctx context.Context, operatorsByQuorum core.OperatorStakes) (map[core.OperatorID]string, error) {
+	socketMap := make(map[core.OperatorID]string)
+	for _, quorum := range operatorsByQuorum {


isn't the key the quorum and the value map[OperatorIndex]OperatorStake here?

yes exactly. This loop is to get all the operator ID and their socket from all quorums

one way to address this would be to preload cache initially by querying every single operator in the current block. Then for the following requests, query the current set of operators, query the logs by filtering on SocketUpdate event, find any socket updates for existing operators, and refresh sockets for all operators.
Then the subsequent requests will only cost 2 eth calls

ian-shim · 2024-12-20T00:58:58Z

core/eth/state.go

@@ -38,7 +45,12 @@ func (cs *ChainState) GetOperatorState(ctx context.Context, blockNumber uint, qu
 		return nil, err
 	}

-	return getOperatorState(operatorsByQuorum, uint32(blockNumber))
+	socketMap, err := cs.buildSocketMap(ctx, operatorsByQuorum)


does this mean whenever we call this method, it will query the chain # operators times?
this seems very inefficient, should we cache this?

yes I expect this too. I can add socket map to the ChainState cache and only query if the operator is missing from the socket map. Although, that means we might need to add a refresh or a failover somewhere so that we don't use the old socket addresses after operators make an updater. Lmk what you think

ian-shim · 2024-12-20T00:59:23Z

core/eth/state.go

@@ -28,7 +28,14 @@ func (cs *ChainState) GetOperatorStateByOperator(ctx context.Context, blockNumbe
 		return nil, err


If we're caching the operator sockets, maybe we should keep this struct separate

litt3 · 2025-02-11T16:52:24Z

core/eth/state.go

+	Tx                    core.Reader
+	SocketMap             map[core.OperatorID]*string
+	socketMu              sync.Mutex
+	socketPrevBlockNumber uint32


These struct members could benefit from some docs. E.g. it's not immediately clear from the name socketPrevBlockNumber what this exactly is

litt3 · 2025-02-11T16:52:40Z

core/eth/state.go

 }

 func NewChainState(tx core.Reader, client common.EthClient) *ChainState {
+	currentBlockNumber, err := client.BlockByNumber(context.Background(), nil)
+	// TODO: consider changing function signature to return error


+1 I think this would be a good change to make

litt3 · 2025-02-11T16:54:41Z

core/eth/state.go

-	return getOperatorState(operatorsByQuorum, uint32(blockNumber))
+	err = cs.refreshSocketMap(ctx, operatorsByQuorum)
+	if err != nil {
+		return nil, err


The errors throughout this file would benefit from added context with fmt.Errorf, e.g. block number, operator ID, etc.

litt3 · 2025-02-11T17:21:02Z

core/eth/state.go

@@ -59,7 +85,131 @@ func (cs *ChainState) GetOperatorSocket(ctx context.Context, blockNumber uint, o
 	return socket, nil
 }

-func getOperatorState(operatorsByQuorum core.OperatorStakes, blockNumber uint32) (*core.OperatorState, error) {
+// buildSocketMap returns a map from operatorID to socket address for the operators in the operatorsByQuorum


This doc is out of date. It's actually updating a map internally, not returning a map as stated. Should this method be renamed?

litt3 · 2025-02-11T17:50:38Z

core/eth/state.go

+	missingOperatorIds := make([]core.OperatorID, 0)
+	for _, quorum := range operatorsByQuorum {
+		for _, operator := range quorum {
+			missingOperatorIds = append(missingOperatorIds, operator.OperatorID)


I'm a bit confused about the logic here.

In this loop, you're collecting ALL operator IDs into a collection, called missingOperatorIds

In buildSocketMap, you fetch the socket for every operator, with GetOperatorSocket

Then, in buildSocketMap, you update cs.SocketMap with all the new values

I thought the purpose of this new cache utility was that you don't need to fetch the socket for every operator every time, but it seems like that's what's happening. You have a comment above that seems to indicate an intention to only add operators to missingOperatorIds if the socket isn't already in the map. Is that logic missing?

really good catch, thanks!

litt3 · 2025-02-11T18:20:55Z

core/eth/state.go

+		} else if method.Name == "updateOperatorSocket" {
+			socket = inputs[0].(string)
+		} else {
+			return fmt.Errorf("unknown method filtered for socket update event: %s", method.Name)


This error is a problem, too. My interpretation is that, if a given event triggers this error, then this method will abort. But the very same event will be processed the next time this is tried, and it would error again. So a single event that triggers this error case would break the entire state object

We should never run into this case 🤔 I am going to let it print a log and skip this case; the block range will be updated and the log won't be processed again. I'm not sure if this is the best handle here

litt3 · 2025-02-11T18:23:41Z

core/eth/state.go

+		if err != nil {
+			return err
+		}
+		methodSig := calldata[:4]


For this and other direct index accessing, I think you need to add checks that the length is actually the expected length. Otherwise, we could have a panic at runtime

litt3 · 2025-02-11T18:26:07Z

core/eth/state.go

+		} else {
+			return fmt.Errorf("unknown method filtered for socket update event: %s", method.Name)
+		}
+		operatorID := core.OperatorID(log.Topics[1].Bytes())


Does the length of Bytes need to be checked? OperatorID is expecting 32 bytes

litt3 · 2025-02-11T18:30:55Z

core/eth/state.go

-				Index: core.OperatorIndex(ind),
+				Stake:  op.Stake,
+				Index:  core.OperatorIndex(ind),
+				Socket: *socketMap[op.OperatorID],


Isn't this an unprotected access of the cs.SocketMap?

litt3 · 2025-02-11T18:37:30Z

retriever/cmd/main.go

-		}
+		retrieverServiceServer := retriever.NewServer(config, logger, retrievalClient, cs, chainClient)
+		// This only start the metrics server; consider unwrapping the function
+		retrieverServiceServer.Start(context.Background())


+1 I think it makes sense to make this change

doesn't seem like I can access metrics with the pointer to retriever service server

litt3 · 2025-02-11T18:39:35Z

This PR re-adds an old file, contracts/bindings/EigenDABlobVerifier/binding.go. Presumably this was a merge conflict- this file can be deleted

hopeyen requested review from ian-shim and mooselumph December 2, 2024 18:18

ian-shim reviewed Dec 20, 2024

View reviewed changes

ian-shim mentioned this pull request Dec 20, 2024

[v2] Retriever server #1048

Merged

5 tasks

ian-shim reviewed Dec 20, 2024

View reviewed changes

hopeyen force-pushed the hope/distributed-retrieval branch from b8566be to 59b7ff7 Compare December 20, 2024 03:09

hopeyen force-pushed the hope/distributed-retrieval branch 2 times, most recently from d393a7e to 36b9280 Compare January 3, 2025 06:40

hopeyen force-pushed the hope/distributed-retrieval branch from f473ed5 to ac07b0c Compare February 11, 2025 15:43

litt3 reviewed Feb 11, 2025

View reviewed changes

hopeyen and others added 13 commits February 12, 2025 16:03

feat: relax indexedChainState to ChainState for retrieval

9201e3c

feat: index operator socket registry with filter logs

8f88c8e

fix: lint for mocks

f009aaf

fix: unit tests for retrieval client

33c33f2

chore: contract binding update

0438b5d

refactor: initialize socket map with direct reads

2ddee5e

fix: properly read filtered logs for params

76898c0

fix: make sure to query socket for other operators in the quorum

6099a2f

refactor: clean up

f67ce0f

chore: regenerate contracts

05760b5

rfr: remove old code and update new client usage

a6e0b89

fix: test v2 client update

56ead50

rfr: variable comments and error msgs

f0508b7

hopeyen force-pushed the hope/distributed-retrieval branch from 71ad17b to f0508b7 Compare February 13, 2025 00:05

hopeyen added 4 commits February 13, 2025 08:55

refactor: chainState constructor returns error

adad91d

refactor: remove old file, socketUpdate indexing per log

1cd5ce6

refactor: check length for direct indexing access

f2d88f3

fix: protect socket update lock, refactor getOperatorState helper

44ac427

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: relax indexedChainState to ChainState for retrieval #943

feat: relax indexedChainState to ChainState for retrieval #943

hopeyen commented Dec 2, 2024

ian-shim left a comment

ian-shim Dec 20, 2024

ian-shim Dec 20, 2024

ian-shim Dec 20, 2024

hopeyen Dec 20, 2024

ian-shim Dec 20, 2024 •

edited

Loading

ian-shim Dec 20, 2024

hopeyen Dec 20, 2024 •

edited

Loading

ian-shim Dec 20, 2024

litt3 Feb 11, 2025

litt3 Feb 11, 2025

litt3 Feb 11, 2025

litt3 Feb 11, 2025

litt3 Feb 11, 2025

hopeyen Feb 13, 2025

litt3 Feb 11, 2025

hopeyen Feb 13, 2025

litt3 Feb 11, 2025

litt3 Feb 11, 2025

litt3 Feb 11, 2025

litt3 Feb 11, 2025

hopeyen Feb 13, 2025

litt3 commented Feb 11, 2025

		@@ -28,7 +28,14 @@ func (cs *ChainState) GetOperatorStateByOperator(ctx context.Context, blockNumbe
		return nil, err

feat: relax indexedChainState to ChainState for retrieval #943

Are you sure you want to change the base?

feat: relax indexedChainState to ChainState for retrieval #943

Conversation

hopeyen commented Dec 2, 2024

Why are these changes needed?

Checks

ian-shim 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

ian-shim Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hopeyen Dec 20, 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

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

litt3 commented Feb 11, 2025

ian-shim Dec 20, 2024 •

edited

Loading

hopeyen Dec 20, 2024 •

edited

Loading