[payments] disperser server metering #792

hopeyen · 2024-10-08T03:42:31Z

Why are these changes needed?

Disperser server integrated with payments metering

Checks

I've made sure the lint is passing in this PR.
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 · 2024-10-23T21:51:17Z

core/auth/payment_signer.go

+
+func (s *PaymentSigner) SignBlobPayment(header *commonpb.PaymentHeader) ([]byte, error) {
+	// Set the account id to the hex encoded public key of the signer
+	header.AccountId = hex.EncodeToString(crypto.FromECDSAPub(&s.PrivateKey.PublicKey))


Is AccountID the public key or the eth account address?
I think there has been some confusion with how we authenticate today. Let's make sure it's clear what we're using (preferably eth account address)

It was intended to be the public key, but I've updated it to use the Eth account address

ian-shim · 2024-10-23T22:01:51Z

core/data.go

+	return crypto.Keccak256Hash(data)
+}
+
+// Hash returns the Keccak256 hash of the PaymentMetadata


ConvertPaymentHeader converts...

thanks 😅 updated

ian-shim · 2024-10-23T22:05:29Z

core/meterer/meterer.go

@@ -138,9 +130,10 @@ func (m *Meterer) ValidateQuorum(headerQuorums []uint8, allowedQuorums []uint8)
 // ValidateBinIndex checks if the provided bin index is valid
 func (m *Meterer) ValidateBinIndex(header core.PaymentMetadata, reservation *core.ActiveReservation) bool {
 	now := uint64(time.Now().Unix())
-	currentBinIndex := GetBinIndex(now, m.ReservationWindow)
+	reservationWindow := m.ChainState.GetReservationWindow()


does it make onchain query every time? should we cache these?

I don't expect this to make onchain query every time. ChainState tracks a PaymentVaultParams struct that caches all the contract constants so this should query directly from the cache (function) and I expect onchain reads only when 1) initializing ChainState struct and 2) refreshed periodically along with registered payments

Is there some kind of retry on failure mechanism?

nada, although I don't think a regular read from cached struct will have any failure. There's periodic polling against the on-chain contract to refresh the state, so if one poll fails, the next poll will be the next try

lol thought ChainState was this ChainState. Should we just call this OnchainPaymentState or sth?

yep we could do that! updated so it is more clear:)

ian-shim · 2024-10-23T22:16:12Z

disperser/apiserver/server.go

+}
+
+// dispersePaidBlob checks for payment metering, otherwise does the same thing as disperseBlob
+func (s *DispersalServer) dispersePaidBlob(ctx context.Context, blob *core.Blob, quorumNumbers []uint32, binIndex uint32, cumulativePayment *big.Int, signature []byte, authenticatedAddress string, apiMethodName string) (*pb.DisperseBlobReply, error) {


looks like its mostly copied from disperseBlob except for the s.meterer != nil block. Why don't we integrate it into the existing method?

I didn't dare mess around disperseBlob 😬 and wanted to keep the two flows cleanly separated from each other. Additionally, dispersePaidBlob has these extra params quorumNumbers []uint32, binIndex uint32, cumulativePayment *big.Int, signature []byte.

To make the code cleaner, I could add a PaymentMetadata param to disperseBlob and pass in an empty one when calling from DisperseBlob and construct a real one in DispersePaidBlob. What do you think?

To make the code cleaner, I could add a PaymentMetadata param to disperseBlob and pass in an empty one when calling from DisperseBlob and construct a real one in DispersePaidBlob. What do you think?

I'd be in favor of something like this to reduce code duplication

updated! removed dispersePaidBlob and added PaymentMetadata to disperseBlob

ian-shim · 2024-10-23T22:17:42Z

disperser/apiserver/server.go

+	s.logger.Debug("received a new paid blob dispersal request", "authenticatedAddress", authenticatedAddress, "origin", origin, "blobSizeBytes", blobSize, "securityParams", strings.Join(securityParamsStrings, ", "))
+
+	// payments before ratelimits
+	if s.meterer != nil {


How does it interact with rate limiter? Would there ever be a case where request is limited by both meterer and rate limiter? i.e. is it possible that there's been sufficient amount paid, but is above the rate limit?

There won't ever be a case where request is limited by both meterer and rate limiter. If meterer is configured, rate limiter should not be used.
Do we want a different type of behavior? Like, using meterer if there's PaymentMetadata, rate limiter if not?

What about something simple like this?

If the payment metadata is included, we use the meterer. If the meterer is unavailable, we fail.

If the payment metadata is not included, we use the rate limiter. If rate limiter is unavailable, pass (current behavior)

The DisperseBlobPaid method will always pass the payment metadata.

great, now disperseBlob will check for payment metadata to use metering, and then ratelimiter if it exists, otherwise continues as usual

ian-shim · 2024-10-23T22:20:02Z

disperser/apiserver/server.go

+	seenQuorums := make(map[uint8]struct{})
+
+	// TODO: validate payment signature against payment metadata
+	if !auth.VerifyPaymentSignature(req.GetPaymentHeader(), req.GetPaymentSignature()) {


Same here. The method looks exactly same as validateRequestAndGetBlob except this check. Could we extend the existing method?

The problem here was the two request types are different and we expect different quorum requirements (reservation doesn't need to be using required quorums, on-demand is automatically assumed to be using required quorums). I was thinking of making the checks in the existing method more modular but didn't want to mess around much... I would propose we can keep this new method and refactor the existing checks. What do you think?

sg, lets add a TODO here

mooselumph · 2024-10-24T22:43:02Z

disperser/cmd/apiserver/main.go

+
+		offchainStore, err := mt.NewOffchainStore(
+			config.AwsClientConfig,
+			"reservations",


Are these switched to constants in a future PR by chance?

nope, I updated to use config flag (optional with defaults)

should these be read from config?

mooselumph · 2024-10-24T22:43:58Z

disperser/apiserver/server_test.go

+		PricePerSymbol:         1,
+		MinNumSymbols:          1,
+		GlobalSymbolsPerSecond: 1000,
+		ReservationWindow:      60,


yep, rebased just now

mooselumph · 2024-10-24T22:49:41Z

core/meterer/meterer.go

@@ -138,9 +130,10 @@ func (m *Meterer) ValidateQuorum(headerQuorums []uint8, allowedQuorums []uint8)
 // ValidateBinIndex checks if the provided bin index is valid
 func (m *Meterer) ValidateBinIndex(header core.PaymentMetadata, reservation *core.ActiveReservation) bool {
 	now := uint64(time.Now().Unix())
-	currentBinIndex := GetBinIndex(now, m.ReservationWindow)
+	reservationWindow := m.ChainState.GetReservationWindow()


Is there some kind of retry on failure mechanism?

mooselumph · 2024-10-25T21:38:38Z

disperser/apiserver/server.go

@@ -209,7 +214,7 @@ func (s *DispersalServer) DisperseBlobAuthenticated(stream pb.Disperser_Disperse
 	}

 	// Disperse the blob
-	reply, err := s.disperseBlob(ctx, blob, authenticatedAddress, "DisperseBlobAuthenticated")
+	reply, err := s.disperseBlob(ctx, blob, authenticatedAddress, "DisperseBlobAuthenticated", &core.PaymentMetadata{})


Just pass in nil here?

sounds good, updated!

mooselumph · 2024-10-25T21:39:39Z

disperser/apiserver/server.go

@@ -282,7 +283,13 @@ func (s *DispersalServer) disperseBlob(ctx context.Context, blob *core.Blob, aut

 	s.logger.Debug("received a new blob dispersal request", "authenticatedAddress", authenticatedAddress, "origin", origin, "blobSizeBytes", blobSize, "securityParams", strings.Join(securityParamsStrings, ", "))

-	if s.ratelimiter != nil {
+	// If paymentHeader is not empty, we use the meterer, otherwise we use the ratelimiter if the ratelimiter is available
+	if paymentHeader != nil && *paymentHeader != (core.PaymentMetadata{}) {


Probably just check for nil and pass in nil for the non-paid endpoint.

mooselumph

Looks good!

mooselumph · 2024-10-27T00:35:11Z

disperser/apiserver/server.go

-	if s.ratelimiter != nil {
+	// If paymentHeader is not empty, we use the meterer, otherwise we use the ratelimiter if the ratelimiter is available
+	if paymentHeader != nil {
+		err := s.meterer.MeterRequest(ctx, *blob, *paymentHeader)


The blob length is passed in in the next PR

yep, all the changes regarding to our v1/v2 payment discussions were made in the next PR along with the integration test that initiated that discussion. I could repeat the changes here again, thinking it might make intuitive sense in the review process

This was mostly just a note for myself. I think it's fine as is!

mooselumph · 2024-10-27T00:42:10Z

disperser/apiserver/server.go

@@ -311,6 +318,38 @@ func (s *DispersalServer) disperseBlob(ctx context.Context, blob *core.Blob, aut
 	}, nil
 }

+func (s *DispersalServer) DispersePaidBlob(ctx context.Context, req *pb.DispersePaidBlobRequest) (*pb.DisperseBlobReply, error) {


Should we put this code behind a feature flag? Right now, if we were to release from this code, we'd be running code here which is still incomplete. (The previous PR hits the unimplemented fallback)

🤔 like if paymentFeatureEnabled do this, otherwise hit unimplemented fallback? the main incomplete part is the contract, could we wait til code complete to make a release?

Could a feature flag be useful for rollout plan? I'd imagine we might want to test this on preprod and testnet before mainnet? cc @ian-shim

Yes! This should be behind a feature flag. Maybe we can use EnablePaymentMeterer?

sounds good to me:) I added a check with if meterer is nil then the function fallback. This is because meterer is only set when EnablePaymentMeterer is true, and wanted to avoid adding additional config to DisperserServer

ian-shim · 2024-10-29T17:51:41Z

core/meterer/meterer.go

 	go func() {
-		ticker := time.NewTicker(1 * time.Hour)
+		ticker := time.NewTicker(updateInterval)


nit: would updateInterval be ever different from m.UpdateInterval? Should we just use m.UpdateInterval instead of passing it in as a param?

thanks for pointing it out! updated

hopeyen changed the base branch from master to hope/meterer-logic October 8, 2024 03:48

hopeyen force-pushed the hope/disperser-server-metering branch from 9755e83 to 0758b5d Compare October 8, 2024 03:49

hopeyen changed the title ~~disperser server metering~~ [payments] disperser server metering Oct 8, 2024

hopeyen force-pushed the hope/meterer-logic branch 4 times, most recently from 1bd1029 to d402849 Compare October 11, 2024 16:52

hopeyen force-pushed the hope/disperser-server-metering branch from 0758b5d to 6972c65 Compare October 11, 2024 17:29

hopeyen force-pushed the hope/meterer-logic branch 5 times, most recently from b72877e to b085195 Compare October 17, 2024 18:17

hopeyen force-pushed the hope/disperser-server-metering branch from 24cc85d to 256cd70 Compare October 21, 2024 19:20

hopeyen changed the base branch from hope/meterer-logic to hope/api-paid-blob-dispersal October 21, 2024 20:17

hopeyen force-pushed the hope/api-paid-blob-dispersal branch 3 times, most recently from eb7a1a3 to 80ec710 Compare October 22, 2024 16:25

hopeyen force-pushed the hope/disperser-server-metering branch from c81aad0 to 5c33742 Compare October 22, 2024 21:56

hopeyen requested review from mooselumph and ian-shim October 23, 2024 20:41

ian-shim reviewed Oct 23, 2024

View reviewed changes

hopeyen force-pushed the hope/api-paid-blob-dispersal branch 2 times, most recently from 6f8db25 to 9ed0ed8 Compare October 24, 2024 22:27

Base automatically changed from hope/api-paid-blob-dispersal to master October 24, 2024 22:38

hopeyen force-pushed the hope/disperser-server-metering branch from c5006a4 to 4f66eea Compare October 24, 2024 22:49

mooselumph reviewed Oct 24, 2024

View reviewed changes

hopeyen force-pushed the hope/disperser-server-metering branch 3 times, most recently from d4dc32b to e5c113a Compare October 24, 2024 23:57

mooselumph reviewed Oct 25, 2024

View reviewed changes

hopeyen force-pushed the hope/disperser-server-metering branch 2 times, most recently from 2ec7396 to d603716 Compare October 26, 2024 04:32

hopeyen requested a review from mooselumph October 26, 2024 17:02

mooselumph reviewed Oct 27, 2024

View reviewed changes

hopeyen added 17 commits October 29, 2024 09:30

feat: disperser server payment metering API

d48f090

refactor: rebase to master

45be754

refactor: DRY and config flags

c127da4

fix: outdated test

29bf135

fix: lint

dfad7c8

chore: test fix and startup query

693a752

refactor: update chainPaymentState

df5962d

refactor: server payment state init

3801be9

refactor: logger and flags

7f47dcc

revert: contract binding bin back to master

ff2abea

refactor: add and rm comments

860a59e

refactor: frequent on-chain refresh and payment error code

ac737a7

refactor: QuorumNumbers protobuf def

498f1dc

refactor: minor updates

8390a52

fix: integration test for ratelimit

cad97fe

feat: dispersePaidBlob disabled when unset

252e1f5

refactor: rebase to master

928ab5e

hopeyen force-pushed the hope/disperser-server-metering branch from c317303 to 928ab5e Compare October 29, 2024 16:33

ian-shim approved these changes Oct 29, 2024

View reviewed changes

refactor: use configsnot params

1b507c1

hopeyen requested a review from mooselumph October 30, 2024 04:08

mooselumph approved these changes Oct 30, 2024

View reviewed changes

hopeyen merged commit 43d8c54 into master Oct 30, 2024
9 checks passed

[payments] disperser server metering #792

[payments] disperser server metering #792

Conversation

hopeyen commented Oct 8, 2024 • edited 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

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

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

mooselumph 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

hopeyen commented Oct 8, 2024 •

edited

Loading