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

core/eth/state.go

retriever/cmd/main.go

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

api/clients/v2/disperser_client.go

api/clients/retrieval_client_test.go

core/eth/state.go

ian-shim

I feel like integrating the operator socket into opInfo is taking a bigger footprint and may be introducing more risk given that this information isn't needed in the existing ChainState use cases. This is primarily used to replace IndexedOperatorInfo in the api clients. Can we consider decoupling this client that fetches operator socket from the ChainState?

ian-shim · 2025-02-21T17:42:19Z

core/eth/reader.go

@@ -500,6 +500,12 @@ func (t *Reader) SocketRegistry(ctx context.Context) (gethcommon.Address, error)
 	})
 }

+func (t *Reader) RegistryCoordinator(ctx context.Context) (gethcommon.Address, error) {


Don't we already have t.bindings.RegCoordinatorAddr?

Yes, this function was added for ChainState to filter socket update event logs. Are you suggesting to just return t.bindings.RegCoordinatorAddr for this function? I thought this way is more standard looking at other address getter functions

Unless the RegistryCoordinator address is updated, we don't need an extra RPC call, right?

ian-shim · 2025-02-21T17:52:13Z

core/eth/state.go

+	prematureBreak := false
+	for _, log := range logs {
+		cs.socketPrevBlockNumber.Store(uint32(log.BlockNumber - 1))
+		transaction, err := cs.getTransaction(ctx, log.TxHash)


do we need to fetch the transaction again? Don't we get both operator ID and socket from the logs?

ian-shim · 2025-02-21T18:01:46Z

core/eth/state.go

+			cs.socketMu.Unlock()
+
+			if !ok {
+				socket, err := cs.Reader.GetOperatorSocket(ctx, operator.OperatorID)


Making individual calls to get each operator socket seems brittle since it would be hundreds of calls in short bursts, and this method returns error if a single call fails
Have you considered using a batch call instead?

Could we add an issue of this and improve later?

If we decouple this operator socket retrieval logic from the ChainState as suggested here and only use this in api clients, I'd feel more comfortable improving this later vs. now

ian-shim · 2025-02-21T18:38:31Z

core/eth/state.go

+
+// indexSocketMap filters event logs from the previously checked block number to the current block,
+// to identify all socket update events in that block range, and update the socket map accordingly
+func (cs *ChainState) indexSocketMap(ctx context.Context) error {


Can we add tests to these methods? Methods that simply make onchain calls are fine, but some involve non trivial logic

core/eth/state.go

litt3 · 2025-02-21T20:32:10Z

core/eth/state.go

+	// Mutex to access socket map
+	socketMu sync.Mutex
+	// The previous block number the socket map was updated at, inclusive
+	socketPrevBlockNumber atomic.Uint32


Doesn't appear to need to be atomic any more, since access is fully contained within socketMu

ian-shim · 2025-02-21T21:03:54Z

core/eth/state.go

+
+// parseOperatorIDFromEventLog parses the operator ID from a log and returns the operator ID.
+func (cs *ChainState) parseOperatorIDFromEventLog(log *types.Log) (core.OperatorID, error) {
+	if len(log.Topics) < 2 {


should len(log.Topics) be exactly 2?

ian-shim · 2025-02-21T21:07:38Z

core/eth/state.go

+			cs.socketMu.Unlock()
+
+			if !ok {
+				socket, err := cs.Reader.GetOperatorSocket(ctx, operator.OperatorID)


If we decouple this operator socket retrieval logic from the ChainState as suggested here and only use this in api clients, I'd feel more comfortable improving this later vs. now

ian-shim · 2025-02-21T21:09:30Z

core/eth/reader.go

@@ -500,6 +500,12 @@ func (t *Reader) SocketRegistry(ctx context.Context) (gethcommon.Address, error)
 	})
 }

+func (t *Reader) RegistryCoordinator(ctx context.Context) (gethcommon.Address, error) {


Unless the RegistryCoordinator address is updated, we don't need an extra RPC call, right?

hopeyen · 2025-02-24T05:10:57Z

I feel like integrating the operator socket into opInfo is taking a bigger footprint and may be introducing more risk given that this information isn't needed in the existing ChainState use cases. This is primarily used to replace IndexedOperatorInfo in the api clients. Can we consider decoupling this client that fetches operator socket from the ChainState?

Agree that this is taking a bigger footprint than what we originally intended. Stepping back to the original goal of this PR, is to utilize the on-chain storage of operator socket, and remove dependency of TheGraph indexer for retrieving the socket.

Recalling the first iteration of the PR, there's no caching or indexing, and simply calls the contract for the socket. We spoke about the concern then, was too many unnecessary RPC calls since GetOperatorState is called for many components other than the retrieval client, thus we started writing the cache/indexing. Some of the code looks more and more like our existing indexer component. I think we got caught up reinventing the indexer wheel, maybe?

What would you think if I remove all the later changes and go back to the first iteration, and instead add a separate function GetOperatorSockets, and only have retrieval client calls it? Preferably, let's not worry about the indexing in this PR, and try to optimize if RPC calls for the sockets turned out to be a problem for the retrieval client?

ian-shim · 2025-02-24T15:52:19Z

I think the caching here is definitely useful even on the client side. Why don't we just factor out the operator socket related methods to a separate component including all methods you implemented and leave the ChainState unchanged? This way the existing functionality of ChainState remain unchanged but only API clients do. Any thoughts?

anupsv · 2025-02-24T18:39:28Z

core/eth/state.go

 			}
 			totalStake.Add(totalStake, op.Stake)
 		}

 		totals[quorumID] = &core.OperatorInfo{
 			Stake: totalStake,
 			Index: core.OperatorIndex(len(quorum)),
+			// no socket for the total
+			Socket: "",


Is this even a case ? Can someone register as operator without a socket ?

anupsv · 2025-02-24T18:47:41Z

core/eth/state.go

+		return "", fmt.Errorf("unexpected number of values in event data")
+	}
+
+	socket, ok := values[0].(string)


We would need to check if the socket is mapped with host:port and not something like host:port:port or some bad string unless this is enforced on contract side. The other is, we need to check if the socket ip is some private ip like in 172.x or 192.x or localhost, we should reject. Should also be handled contract side as well.

litt3 · 2025-02-24T21:08:49Z

@hopeyen I'm looking into log filtering for a different task, and my exploration led me to a question about your implementation here:

Is there a reason you are manually implementing the log filtering logic, instead of using the autogenerated binding method here? It seems that should do what you've manually implemented here, and also allow you to avoid adding the huge RegistryCoordinator.json file. Am I missing something?

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

litt3 reviewed Feb 18, 2025

View reviewed changes

api/clients/v2/disperser_client.go Outdated Show resolved Hide resolved

chore: rm duplicate import

c23fd54

litt3 reviewed Feb 18, 2025

View reviewed changes

litt3 requested a review from anupsv February 19, 2025 18:07

hopeyen added 4 commits February 19, 2025 10:10

refactor: chainState comments, socket map checks, rename, loggers

c80f85e

feat: chainState logger arg

ce594bf

refactor: separate log fetching and processing

23b3efd

fix: lint

d939826

litt3 reviewed Feb 19, 2025

View reviewed changes

core/eth/state.go Outdated Show resolved Hide resolved

core/eth/state.go Outdated Show resolved Hide resolved

core/eth/state.go Outdated Show resolved Hide resolved

core/eth/state.go Outdated Show resolved Hide resolved

core/eth/state.go Outdated Show resolved Hide resolved

refactor: edge cases + helpers

663b8fa

litt3 reviewed Feb 20, 2025

View reviewed changes

core/eth/state.go Outdated Show resolved Hide resolved

core/eth/state.go Outdated Show resolved Hide resolved

core/eth/state.go Outdated Show resolved Hide resolved

core/eth/state.go Show resolved Hide resolved

refactor: names and logs, atomic int update

b463cfa

litt3 reviewed Feb 21, 2025

View reviewed changes

core/eth/state.go Outdated Show resolved Hide resolved

ian-shim reviewed Feb 21, 2025

View reviewed changes

hopeyen added 7 commits February 21, 2025 10:57

fix: logs

5a02ef7

refactor: reduce tx call

001ee66

refactor: clean up locking

ca4848d

fix: lint and registryCoordinator addr

d386c9d

fix: fmt error format

7e1d5a9

fix: function sig

90023b5

fix: typo

94636ec

litt3 reviewed Feb 21, 2025

View reviewed changes

ian-shim reviewed Feb 21, 2025

View reviewed changes

anupsv reviewed Feb 24, 2025

View reviewed changes

hopeyen added 3 commits February 24, 2025 23:49

feat: use generated binding filter

ad98ac4

fix: rm an accident file

021a776

fix: update mock writer

a010cf0

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 commented Feb 11, 2025

ian-shim left a comment

ian-shim Feb 21, 2025

hopeyen Feb 21, 2025

ian-shim Feb 21, 2025

ian-shim Feb 21, 2025

ian-shim Feb 21, 2025

hopeyen Feb 21, 2025

ian-shim Feb 21, 2025

ian-shim Feb 21, 2025

anupsv Feb 24, 2025

litt3 Feb 21, 2025

ian-shim Feb 21, 2025

ian-shim Feb 21, 2025

ian-shim Feb 21, 2025

hopeyen commented Feb 24, 2025

ian-shim commented Feb 24, 2025

anupsv Feb 24, 2025

anupsv Feb 24, 2025

litt3 commented Feb 24, 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

litt3 commented Feb 11, 2025

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

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

hopeyen commented Feb 24, 2025

ian-shim commented Feb 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

litt3 commented Feb 24, 2025

ian-shim Dec 20, 2024 •

edited

Loading

hopeyen Dec 20, 2024 •

edited

Loading