chore: modify verifier to not require eth archive node #241

samlaf · 2025-01-14T00:51:01Z

Fixes Issue

Fixes #230

Changes proposed

Contains 2 main changes needed to remove archive node requirements:

4d85d3b: force ethConfirmationDepth to be <64 (so we read at most 64 blocks in the past, while normal nodes store the latest 128 states): not strictly necessary as this was a flag which could have been set by teh validators anyways, but its a nice to have to make sure that the code works even without archival node dependency.
c4e6c90: read the quorum parameters (required quorums and adversary thresholds) from the EigenDAServiceManager only once, at startup.

Also updated the README with this information in cc3538f

Copying the comment on the getQuorumParametersAtLatestBlock function here to highlight the solution proposed in this PR:

// Note: this strategy (fetching once and caching) only works because these parameters are immutable.
// They might be different in different environments (for eg on a devnet or testnet), but they are fixed on a given network.
// We used to allow these parameters to change (via a setter function on the contract), but that then forced us here in the proxy
// to query for these parameters on every request, at the batch's reference block number (RBN).
// This in turn required rollup validators running this proxy to have an archive node, in case the RBN was >128 blocks in the past,
// which was not ideal. So we decided to make these parameters immutable, and cache them here.

Screenshots (Optional)

Note to reviewers

We panic in the flag's action, as well as in the verifier's constructor when this condition is not respected. This will make sure that an archival node is not required.

…ization This removes the need for running with an eth archive node.

README.md

verify/cert.go

…chive-node

Co-authored-by: Ethen <[email protected]>

verify/cert.go

… = 64

…chive-node

EthenNotEthan · 2025-01-23T16:52:32Z

flags/eigendaflags/cli.go

@@ -118,6 +118,7 @@ func CLIFlags(envPrefix, category string) []cli.Flag {
 			Category: category,
 		},
 		&cli.BoolFlag{
+			// This flag is DEPRECATED. Use ConfirmationDepthFlagName, which accept "finalization" or a number <64.


knit - this comment is redundant with the error action message below

Yes but I prefer to keep it there. An action typically is not meant for deprecation, so on a first reading without the comment one would read through this entire flag and it would take some brain processing to figure out its deprecated. Typically some redundancy is always good in programming. Also helps catch errors.

flags/eigendaflags/cli.go

verify/cert.go

common/consts/consts.go

flags/eigendaflags/cli.go

EthenNotEthan · 2025-01-23T17:09:53Z

verify/cert.go

+// from the EigenDAServiceManager contract at the latest block.
+// We then cache these parameters and use them in the Verifier to verify the certificates.
+//
+// Note: this strategy (fetching once and caching) only works because these parameters are immutable.


immutable per the implementation contract right? IIUC we could technically still upgrade the source proxy contract with a new implementation with alternative bytecode defining different immutable values

Yes, but you're the one asking me to reduce comment cognitive overload :P
Do you think I should add that explanation?

nah not necessary for an explanation - more just calling out a potential risk. i.e if the service manager were to be upgraded arbitrarily to use a different quorum then proxy would have no conception? maybe lets callout in a small issue

Yep agree. Thanks for raising this. Will go ahead and merge as is, but let's keep in mind the entire space of interaction with the smart contracts as we go forward, especially for V2 as we try to make the SecurityThresholds upgradeable again.

verify/cert.go

Co-authored-by: EthenNotEthan <[email protected]>

EthenNotEthan

LGTM

bxue-l2 · 2025-01-23T18:00:45Z

verify/cert.go

+	// ethConfirmationDepth is using to verify that a blob's batch has been bridged to the EigenDAServiceManager contract at least
+	// this many blocks in the past. To do so we make an eth_call to the contract at the current block_number - ethConfirmationDepth.
+	// Hence in order to not require an archive node, this value should be kept low. We force it to be < 64.
+	// waitForFinalization should be used instead of ethConfirmationDepth if the user wants to wait for finality (typically 64 blocks in happy case).
 	ethConfirmationDepth uint64


Looks to me this variable is the upper bound for the punctuality gap.

In v2 we don't have to worry about it.

Tangential question, how to ensure all proxy set the same parameters, and upgrades.

bxue-l2 · 2025-01-23T18:13:15Z

flags/eigendaflags/cli.go

@@ -210,7 +210,12 @@ func validateConfirmationFlag(val string) error {
 	}

 	if depth >= 64 {
-		log.Printf("Warning: confirmation depth set to %d, which is > 2 epochs (64). Consider using 'finalized' instead.\n", depth)
+		// We keep this low (<128) to avoid requiring an archive node (see how this is used in CertVerifier).
+		// Note: assuming here that no sane person would ever need to set this to a number to something >64.


why this is insane to set > 64. Is it reasonable assumption

Forgot this one in #241

samlaf added 2 commits January 13, 2025 18:52

chore: force verifier's EthConfirmationDepth to be <64

4d85d3b

We panic in the flag's action, as well as in the verifier's constructor when this condition is not respected. This will make sure that an archival node is not required.

chore: modify verifier to load quorum parameters only once at initial…

c4e6c90

…ization This removes the need for running with an eth archive node.

samlaf marked this pull request as draft January 14, 2025 00:51

samlaf requested a review from EthenNotEthan January 14, 2025 00:51

samlaf added 2 commits January 13, 2025 19:54

style: fix minor lint issue

1d6cff9

docs: update README to mention that archival node is no longer needed

cc3538f

samlaf changed the title ~~Chore modify verifier to not require eth archive node~~ chore: modify verifier to not require eth archive node Jan 14, 2025

EthenNotEthan reviewed Jan 14, 2025

View reviewed changes

README.md Outdated Show resolved Hide resolved

verify/cert.go Show resolved Hide resolved

EthenNotEthan reviewed Jan 14, 2025

View reviewed changes

verify/cert.go Outdated Show resolved Hide resolved

verify/cert.go Show resolved Hide resolved

samlaf and others added 3 commits January 21, 2025 17:02

Merge branch 'main' into chore--modify-verifier-to-not-require-eth-ar…

d11a364

…chive-node

docs: clean-up README archival node requirement explanation

dc0b205

docs: fix verify/cert.go comment typo

9614a4f

Co-authored-by: Ethen <[email protected]>

samlaf marked this pull request as ready for review January 22, 2025 01:45

samlaf requested review from EthenNotEthan, litt3 and bxue-l2 January 22, 2025 01:45

litt3 reviewed Jan 22, 2025

View reviewed changes

samlaf mentioned this pull request Jan 22, 2025

bug: requiredQuorums glitch on holesky #252

Closed

gastonponti mentioned this pull request Jan 22, 2025

Required quorums glitch #253

Closed

samlaf added 3 commits January 23, 2025 10:54

docs: for eg -> e.g.

ec8d910

style(cert): remove unecessary bound checks from inside loop

4a86753

style: create consts package with EthHappyPathFinalizationDepthBlocks…

275acd6

… = 64

samlaf requested a review from litt3 January 23, 2025 16:25

Merge branch 'main' into chore--modify-verifier-to-not-require-eth-ar…

e2d09e5

…chive-node

litt3 approved these changes Jan 23, 2025

View reviewed changes

EthenNotEthan reviewed Jan 23, 2025

View reviewed changes

samlaf and others added 4 commits January 23, 2025 12:41

style: change panic into error return

0739f3b

docs: change op reference for eth reference

f255bc8

docs: make flag comment simpler

9c43884

Update verify/cert.go

5674628

Co-authored-by: EthenNotEthan <[email protected]>

samlaf requested a review from EthenNotEthan January 23, 2025 17:44

EthenNotEthan approved these changes Jan 23, 2025

View reviewed changes

samlaf merged commit deac7be into main Jan 23, 2025
9 checks passed

samlaf deleted the chore--modify-verifier-to-not-require-eth-archive-node branch January 23, 2025 18:17

bxue-l2 reviewed Jan 23, 2025

View reviewed changes

samlaf mentioned this pull request Jan 24, 2025

remove eth_call that requires archive node #258

Closed

samlaf added a commit that referenced this pull request Jan 24, 2025

fix: remove last eth_call that required archive node

9f5b1d4

Forgot this one in #241

samlaf mentioned this pull request Jan 24, 2025

fix: remove last eth_call that required archive node #259

Merged

samlaf added a commit that referenced this pull request Jan 24, 2025

fix: remove last eth_call that required archive node (#259)

5530ac4

Forgot this one in #241

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: modify verifier to not require eth archive node #241

chore: modify verifier to not require eth archive node #241

samlaf commented Jan 14, 2025 •

edited

Loading

EthenNotEthan Jan 23, 2025

samlaf Jan 23, 2025

EthenNotEthan Jan 23, 2025

samlaf Jan 23, 2025

EthenNotEthan Jan 23, 2025

samlaf Jan 23, 2025

EthenNotEthan left a comment

bxue-l2 Jan 23, 2025

bxue-l2 Jan 23, 2025

chore: modify verifier to not require eth archive node #241

chore: modify verifier to not require eth archive node #241

Conversation

samlaf commented Jan 14, 2025 • edited Loading

Fixes Issue

Changes proposed

Screenshots (Optional)

Note to reviewers

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

EthenNotEthan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samlaf commented Jan 14, 2025 •

edited

Loading