feat: validator authenticate on-demand request by disperser key #1539

hopeyen · 2025-05-08T00:40:34Z

Why are these changes needed?

Validators should only accept ondemand requests from the EigenLabs disperser.
The key has already been authenticated at the step of the check.
If batch request contains any blob with ondemand payment but the batch didn't come from EigenLabs disperser, then validator rejects.

this check will be removed when we support decentralized ondemand usage

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

anupsv · 2025-05-12T16:44:22Z

node/grpc/server_v2.go

+					return nil, api.NewErrorInvalidArg("on-demand payments are only allowed for EigenLabsDisperser")
+				}
+			}
+		}


Before this, do we check that the ondemand blobs we're from EigenLabsDisperser by checking the signature or something ?

yep! in AuthenticateStoreChunksRequest on L131 we authenticate the signature first to make sure it is a registered disperser

Arguably, we can move this check in the AuthenticateStoreChunksRequest right ? Cause it has the registered disperser and signature check.

jianoaix · 2025-05-13T21:22:26Z

core/meterer/meterer.go

@@ -277,3 +277,7 @@ func (m *Meterer) IncrementGlobalBinUsage(ctx context.Context, symbolsCharged ui
 func (m *Meterer) GetReservationBinLimit(reservation *core.ReservedPayment, reservationWindow uint64) uint64 {
 	return reservation.SymbolsPerSecond * reservationWindow
 }
+
+func IsOnDemandPayment(paymentMetadata *core.PaymentMetadata) bool {
+	return paymentMetadata.CumulativePayment.Cmp(big.NewInt(0)) > 0


The reservation_period should be unset as well?

I don't think we need this check with similar reason in #1536 comment

So on demand payment will not have cumulative payment ?

jianoaix · 2025-05-13T21:31:42Z

node/grpc/server_v2.go

+			if meterer.IsOnDemandPayment(&blob.BlobHeader.PaymentMetadata) {
+				// Batch contains on-demand payments, so the chunk must be from EigenLabsDisperser
+				if in.DisperserID != api.EigenLabsDisperserID {
+					return nil, api.NewErrorInvalidArg("on-demand payments are only allowed for EigenLabsDisperser")


would be helpful to pinpoint the blob that's on-demand but via a non-eigen disperser in error log

yes! would be. I will add

ian-shim · 2025-05-21T22:16:50Z

disperser/apiserver/server_v2.go

@@ -170,6 +170,7 @@ func (s *DispersalServerV2) Start(ctx context.Context) error {
 		for {
 			select {
 			case <-ticker.C:
+				s.logger.Debug("Refreshing onchain quorum state", "onchainState", s.onchainState.Load())


nit: Is this intentionally logging the state prior to the refresh? A bit confusing to me as I'd interpret this as the state after the refresh if this showed up in the logs

Ah, makes sense. moved down

ian-shim · 2025-05-21T22:20:13Z

node/grpc/server_v2.go

@@ -145,6 +143,15 @@ func (s *ServerV2) StoreChunks(ctx context.Context, in *pb.StoreChunksRequest) (
 		if err != nil {
 			return nil, api.NewErrorInvalidArg(fmt.Sprintf("failed to verify request: %v", err))
 		}
+		// TODO: move to blob authenticator later
+		for _, blob := range batch.BlobCertificates {


Should metering logic be gated behind the permissionless disperser feature flag?

There's no metering logic, just a filter for whether the node should serve the ondemand request from a certain disperser. If the disperser is not EigenLabs, it will get rejected regardless of whether there's permissionless dispersers or not, so I didn't think this block should be gated by other flags

ian-shim · 2025-05-21T22:20:46Z

Need to resolve CI

dmanc · 2025-06-10T01:28:46Z

node/grpc/server_v2.go

+			if meterer.IsOnDemandPayment(&blob.BlobHeader.PaymentMetadata) {
+				// Batch contains on-demand payments, so the chunk must be from EigenLabsDisperser
+				if in.DisperserID != api.EigenLabsDisperserID {
+					return nil, api.NewErrorInvalidArg(fmt.Sprintf("on-demand payments are only allowed for EigenLabsDisperser; receiving disperser ID: %d", in.DisperserID))


Should this be a forbidden error?

PermissionDenied (Code 7) → HTTP 403: "the caller does not have permission to execute the specified operation"

makes sense, updated!

…ey (#1539)" This reverts commit f545b16.

* Revert "refactor: account ledger interface incorporates debit slip (#1732)" This reverts commit b4cf3d9. * Revert "fix: period record circular wrapping refresh (#1731)" This reverts commit 1091f46. * Revert "fix: disperser_client v2 nil ptr bug (#1710)" This reverts commit a5acd93. * Revert "fix: disperser_client v2 should use default onDemandQuorums (#1700)" This reverts commit 005e0a6. * Revert "refactor: unify accountant debit slip (#1695)" This reverts commit a9c0868. * Revert "feat: AccountantLedger interface for next step unifications (#1694)" This reverts commit 354e42f. * Revert "feat: extract generic payment logic (#1693)" This reverts commit 1003b98. * Revert "fix: make disperser client backwards compatible (#1686)" This reverts commit 26c612f. * Revert "fix: payment state api doesn't fail at zero valued reservations/ondemand (#1682)" This reverts commit 2031c1d. * Revert "refactor: quorum specific metrics for metered bytes (#1668)" This reverts commit 7ac688d. * Revert "feat: GetPaymentStateForAllQuorums api impl (#1664)" This reverts commit ed89588. * Revert "refactor: core meterer consolidation (#1663)" This reverts commit 319a865. * Revert "refactor: onchain state interface (#1662)" This reverts commit d795dfa. * Revert "refactor: core meterer period record module (#1661)" This reverts commit afbef90. * Revert "refactor: payment common functions (#1653)" This reverts commit 36a3d33. * Revert "fix: period index calculation (#1641)" This reverts commit c229748. * Revert "refactor: use generic column name (#1626)" This reverts commit 4d1994e. * Revert "feat: payment onchain state interface (#1625)" This reverts commit 61e46ac. * Revert "feat: offchain quorum period record getter (#1620)" This reverts commit a309139. * Revert "feat: protobuf payment state api quorum specific variant (#1613)" This reverts commit 58acdfd. * Revert "fix: nil assignment to new account (#1612)" This reverts commit bcbf9f5. * Revert "feat: offchain batch writes (#1580)" This reverts commit ce4d2f5. * Revert "feat: validator authenticate on-demand request by disperser key (#1539)" This reverts commit f545b16. * fix: issues after series of reverts (scary) * chore: add back ErrZeroSymbols error This error was likely removed by mistake during a revert conflict fix from one of the previous set of reverted PRs. * style: fix lint * test: fix accountant_test hardcoded error string * Revert "Revert "fix: period index calculation (#1641)"" This reverts commit b8767ae.

feat: on-demand check

8928e15

hopeyen force-pushed the hope/on-demand-check branch from 1e0b820 to 8928e15 Compare May 8, 2025 16:02

hopeyen marked this pull request as ready for review May 8, 2025 18:34

hopeyen requested review from jianoaix and anupsv May 8, 2025 19:24

anupsv reviewed May 12, 2025

View reviewed changes

anupsv previously approved these changes May 12, 2025

View reviewed changes

jianoaix reviewed May 13, 2025

View reviewed changes

hopeyen mentioned this pull request May 14, 2025

feat: reserved-only disperser apiserver #1536

Merged

5 tasks

hopeyen requested a review from ian-shim May 16, 2025 18:59

chore: add disperser id logging when reject ondemand

b540d38

hopeyen dismissed anupsv’s stale review via b540d38 May 21, 2025 13:01

Merge branch 'master' into hope/on-demand-check

638a931

ian-shim reviewed May 21, 2025

View reviewed changes

hopeyen added 2 commits May 22, 2025 13:17

fix: lint

ceab008

fix: lint

cf5be5f

hopeyen requested a review from ian-shim May 22, 2025 21:32

ahope/on-demand-checkMerge branch 'hope/on-demand-check'

901ab25

hopeyen requested a review from anupsv May 27, 2025 15:27

Merge branch 'master' into hope/on-demand-check

06b7b03

dmanc reviewed Jun 10, 2025

View reviewed changes

feat: node return 403 for on-demand from non-EigenLabs dispersers

d77c666

anupsv approved these changes Jun 12, 2025

View reviewed changes

dmanc approved these changes Jun 13, 2025

View reviewed changes

hopeyen merged commit f545b16 into master Jun 13, 2025
15 checks passed

hopeyen deleted the hope/on-demand-check branch June 13, 2025 19:14

samlaf added a commit that referenced this pull request Jul 11, 2025

Revert "feat: validator authenticate on-demand request by disperser k…

96a56ca

…ey (#1539)" This reverts commit f545b16.

feat: validator authenticate on-demand request by disperser key #1539

feat: validator authenticate on-demand request by disperser key #1539

Uh oh!

Conversation

hopeyen commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are these changes needed?

Checks

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ian-shim commented May 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hopeyen commented May 8, 2025 •

edited

Loading