Implement cert verifier address provider #1368

litt3 · 2025-03-06T16:40:52Z

Closes EGDA-979

Why are these changes needed?

As described in this issue, a single cert verifier address isn't enough for payload dispersal: the active address may change after the disperser chooses an RBN, once dynamic cert verifier changes are possible
To future proof the client APIs for when dynamic cert verifier addresses are supported, this PR makes necessary API changes, so that a CertVerifierAddressProvider is used throughout the clients, instead of manually passing in an address for each call
- This change represents a nice simplification of the APIs, in addition to the benefit of supporting future efforts
This PR also does a lot of work organizing code into packages. These changes were necessary, so that the new structures could be put into reasonable packages without introducing circular dependencies. IMO the changes in this PR represent a substantial improvement to the package structure of the clients.

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 :(

Signed-off-by: litt3 <[email protected]>

api/clients/v2/payloaddispersal/payload_disperser_config.go

api/clients/v2/verification/cert_verifier.go

Signed-off-by: litt3 <[email protected]>

anupsv · 2025-03-06T18:26:45Z

api/clients/v2/payloaddispersal/payload_disperser.go

 	blob, err := payload.ToBlob(pd.config.PayloadPolynomialForm)
 	if err != nil {
 		return nil, fmt.Errorf("convert payload to blob: %w", err)
 	}

 	timeoutCtx, cancel := context.WithTimeout(ctx, pd.config.ContractCallTimeout)
 	defer cancel()
-	requiredQuorums, err := pd.requiredQuorumsStore.GetQuorumNumbersRequired(timeoutCtx, certVerifierAddress)
+	requiredQuorums, err := pd.certVerifier.GetQuorumNumbersRequired(timeoutCtx)


This utilizes the cache righT?

Correct. The cache is now internal to the cert verifier, rather than being managed by the payload disperser.

anupsv · 2025-03-06T18:38:52Z

api/clients/v2/verification/block_number_provider.go

+// This method is synchronized in a way that, if called by multiple goroutines, only a single goroutine will actually
+// poll the internal eth client for the most recent block number. The goroutine responsible for polling at a given time
+// updates an atomic integer, so that all goroutines may check the most recent block without duplicating work.
+func (bnp *BlockNumberProvider) MaybeWaitForBlockNumber(ctx context.Context, targetBlockNumber uint64) error {


The maybe is quite confusing until i went through the logic twice. Idk if we can rename it or soemthing.

Renamed to WaitForBlockNumber ceee94b9

samlaf

Looks like very nice cleanup. Have a bunch of nits but honestly would be happy merging in this state.

api/clients/v2/cert_verifier_address_provider.go

api/clients/v2/payloaddispersal/payload_disperser_config.go

api/clients/v2/payloadretrieval/relay_payload_retriever_test.go

samlaf · 2025-03-06T18:28:21Z

test/v2/correctness/correctness_test.go

+// checkAndSetCertVerifierAddress checks whether the input address string is empty, and skips the test if it is
+//
+// If the input address is not empty, this method configures the test client to use the input address as the cert
+// verifier address.
+func checkAndSetCertVerifierAddress(t *testing.T, c *client.TestClient, certVerifierAddress string) {
+	if certVerifierAddress == "" {
+		t.Skip("Requested cert verifier address is not configured")


first time I see this kind of pattern of skipping a test inside some function. Is this something you've used before? Curious if there would be another way to skip the test earlier and inside the test itself.

Is this something you've used before?

Nope, I just did this since there were two things that needed to happen, each time a test handles a cert verifier address. Don't feel too strongly about it, would you prefer doing each of things manually on a per-test basis?

As we've discussed before, I'm just scared of dynamic stuff. Can this skip be done midway through the test after some processing has been done? Or is it purely a construct used at the beginning of tests? If the latter then fine with me.

we talked offline. Would prefer in the future that we don't have this kind of dynamic skipping of tests. But let's move on for now, more tests is always better than no tests!

api/clients/v2/verification/cert_verifier.go

api/clients/v2/verification/block_number_provider.go

Signed-off-by: litt3 <[email protected]>

cody-littley · 2025-03-07T15:22:20Z

api/clients/v2/verification/block_number_monitor.go

-type BlockNumberProvider struct {
+// BlockNumberMonitor is a utility for waiting for a certain ethereum block number
+//
+// TODO: this utility is not currently in use, but DO NOT delete it. It will be necessary for the upcoming


Suggested change

// TODO: this utility is not currently in use, but DO NOT delete it. It will be necessary for the upcoming

// TODO(litt): this utility is not currently in use, but DO NOT delete it. It will be necessary for the upcoming

which litt? ;)

Litt specified, and disambiguated

Signed-off-by: litt3 <[email protected]>

Draft cert verifier address provider feature

e9890d3

Signed-off-by: litt3 <[email protected]>

litt3 self-assigned this Mar 6, 2025

litt3 added 2 commits March 6, 2025 11:48

Merge branch 'master' into cert-verifier-address-provider

2f3be38

Clean up

fb0d95a

Signed-off-by: litt3 <[email protected]>

litt3 marked this pull request as ready for review March 6, 2025 17:28

litt3 requested review from anupsv, samlaf and cody-littley March 6, 2025 17:28

cody-littley approved these changes Mar 6, 2025

View reviewed changes

api/clients/v2/payloaddispersal/payload_disperser_config.go Outdated Show resolved Hide resolved

api/clients/v2/verification/cert_verifier.go Outdated Show resolved Hide resolved

litt3 added 2 commits March 6, 2025 12:47

Simplify checkAndSetDefaults docs

6ca805b

Signed-off-by: litt3 <[email protected]>

Fix wording

235570f

Signed-off-by: litt3 <[email protected]>

cody-littley approved these changes Mar 6, 2025

View reviewed changes

anupsv reviewed Mar 6, 2025

View reviewed changes

samlaf approved these changes Mar 6, 2025

View reviewed changes

litt3 added 9 commits March 7, 2025 08:20

Rename to WaitForBlockNumber

ceee94b

Signed-off-by: litt3 <[email protected]>

Use common.Address instead of strings

a01d8fe

Signed-off-by: litt3 <[email protected]>

Make default config getters private

f48dea3

Signed-off-by: litt3 <[email protected]>

Make map docs consistent

947a1b7

Signed-off-by: litt3 <[email protected]>

Improve block number vs RBN verbiage

a8e5cb0

Signed-off-by: litt3 <[email protected]>

Tighten up the cert verifier

7c5ce3e

Signed-off-by: litt3 <[email protected]>

Fix test var name

b34f52b

Signed-off-by: litt3 <[email protected]>

Merge branch 'master' into cert-verifier-address-provider

5d0d929

Tighten up block number monitor

9db13f7

Signed-off-by: litt3 <[email protected]>

litt3 requested review from cody-littley, samlaf and anupsv March 7, 2025 15:19

cody-littley approved these changes Mar 7, 2025

View reviewed changes

litt3 added 2 commits March 7, 2025 10:58

Improve test

37c48c5

Signed-off-by: litt3 <[email protected]>

Add detail to TODO

a057fb8

Signed-off-by: litt3 <[email protected]>

samlaf approved these changes Mar 7, 2025

View reviewed changes

litt3 merged commit 8f43a4a into master Mar 7, 2025
10 of 12 checks passed

litt3 deleted the cert-verifier-address-provider branch March 7, 2025 17:24

litt3 mentioned this pull request Mar 7, 2025

chore: Update to latest core version, to include cert verifier address provider Layr-Labs/eigenda-proxy#333

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement cert verifier address provider #1368

Implement cert verifier address provider #1368

litt3 commented Mar 6, 2025 •

edited by samlaf

Loading

anupsv Mar 6, 2025

litt3 Mar 7, 2025

anupsv Mar 6, 2025

litt3 Mar 7, 2025

samlaf left a comment

samlaf Mar 6, 2025

litt3 Mar 7, 2025

samlaf Mar 7, 2025

samlaf Mar 7, 2025

cody-littley Mar 7, 2025

samlaf Mar 7, 2025

litt3 Mar 7, 2025

	// TODO: this utility is not currently in use, but DO NOT delete it. It will be necessary for the upcoming
	// TODO(litt): this utility is not currently in use, but DO NOT delete it. It will be necessary for the upcoming

Implement cert verifier address provider #1368

Implement cert verifier address provider #1368

Conversation

litt3 commented Mar 6, 2025 • edited by samlaf Loading

Why are these changes needed?

Checks

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

litt3 commented Mar 6, 2025 •

edited by samlaf

Loading