disperser meterer for payments #779

hopeyen · 2024-09-30T22:58:33Z

Why are these changes needed?

PoC inside disperser for handling payments

summary of changes

Create a meterer package inside the disperser that can be used to replace ratelimiter that validate the requests before storing them
Add mocked interfaces for contracts
Add dynamo DB for managing offchain state
Using simple data types (first u64 before worrying about u128)
Unit tests
Add flag for payments and take precedence over ratelimit when disperser serve requests
- ineffective as blob request header must be updated first

unit tests

reservations
- test for invalid signatures
- test for invalid bin index
- test for non existent accounts
- if blob size > bin, request is rejected
- blob size <= bin, given total throughput is still under reservation bin limit, requests is okay (need to add handling and tests for less than chargeable size)
- first overflow is okay, check that bin+2 gets charged
- following overflows for the same bin is rejected
- --> to add periodic bin cleanup/removal
on demand
- test for invalid signatures
- test for invalid bin index
- test for non existent accounts
- --> to add chargeable and ensure that the chargeable unit is recorded when blob is smaller
- if blobSize > cumulative payments, request is rejected
- if blob size > global rate limit, request is rejected
- for everything in between, if globally okay, things should work
- reject requests exceeding the cumulative payment limit

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

disperser/meterer/meterer.go

mooselumph · 2024-10-01T22:37:31Z

disperser/meterer/meterer.go

+func (m *Meterer) ValidateBinIndex(blobHeader BlobHeader, reservation *ActiveReservation) bool {
+	currentBinIndex := GetCurrentBinIndex()
+	// Valid bin indexes are either the current bin or the previous bin
+	if (blobHeader.BinIndex != currentBinIndex && blobHeader.BinIndex != (currentBinIndex-1)) || (reservation.StartEpoch > blobHeader.BinIndex || blobHeader.BinIndex > reservation.EndEpoch) {


Does it make sense for Epochs to use the same units as Bins?

I'm open to change this for easier usage. What about using start and end timestamps instead of epochs?

mooselumph · 2024-10-01T22:38:01Z

disperser/meterer/meterer.go

+// GetCurrentBinIndex returns the current bin index based on time
+func GetCurrentBinIndex() uint64 {
+	currentTime := time.Now().Unix()
+	binInterval := 375 // Interval that allows 3 32MB blobs at minimal throughput


Should this be configuraable? What are the units?

yes! changed this to use reservationWindow config and added an indication in the help string that this is in seconds

mooselumph · 2024-10-01T22:40:39Z

disperser/meterer/meterer.go

+		return fmt.Errorf("failed to get active reservation by account: %w", err)
+	}
+	if newUsage <= 2*binLimit && blobHeader.BinIndex+2 <= reservation.EndEpoch {
+		m.OffchainStore.UpdateReservationBin(ctx, blobHeader.AccountID, uint64(blobHeader.BinIndex+2), newUsage-binLimit)


Note: this is not a race condition because previous logic ensures this can only happen in a single thread.

mooselumph · 2024-10-01T22:54:19Z

disperser/meterer/offchain_store.go

+	result, err := s.dynamoClient.QueryIndex(ctx, s.onDemandTableName, "AccountIDIndex", "AccountID = :account", commondynamodb.ExpresseionValues{
+		":account": &types.AttributeValueMemberS{
+			Value: accountID,
+		}})


I think we should be able to configure a sort key associated with the CumulativePayment field and directly retrieve the records with cumulative payment above and below the provided payment.

mooselumph · 2024-10-01T22:59:04Z

disperser/meterer/meterer.go

+	currentBinIndex := uint64(time.Now().Unix())
+
+	// Valid bin indexes are either the current bin or the previous bin (allow this second or prev sec)
+	if blobHeader.BinIndex != currentBinIndex && blobHeader.BinIndex != (currentBinIndex-1) {


For global rate limit, I think we can argue that we don't need to include the BinIndex within the header, but just use the receipt time directly.

makes sense, in this case we don't need to validate the bin index, maybe not even receipt time, but the time when global rate gets checked (since global rate limit check happens after payment validations, we avoid failing on cases where payment validation ever gets to more than 2 seconds)

mooselumph

Some more comments!

mooselumph · 2024-10-02T16:22:16Z

common/aws/dynamodb/client.go

+		if _, ok := key[itemKey]; ok {
+			// Cannot update the key
+			continue
+		}


I realize this is using the template above, but it's unclear to me why this is failing silently.

Apparently dynamoDB doesn't let user update the primary key or sort key of an existing item, so if the current itemKey is part of the item's key, we simply move on to the next attribute in the item map. I think this is failing silently because we want to operate on other fields anyway.

some 3 alternative ways that still ensure consistency:

a loop to check the keys (and return err) before the updating loop,

do the same as now, but revert the changes later on if we cannot update the key

do the same as now, and return a list of keys that couldn't get updated

mooselumph · 2024-10-02T16:23:05Z

common/aws/dynamodb/client.go

+			// update = update.Add(expression.Name(itemKey), expression.Value(n.Value))
+			// update = update.Add(expression.Name(itemKey), expression.Value(n.Value))


mooselumph · 2024-10-02T16:27:57Z

disperser/meterer/offchain_store.go

+	return binUsageValue, nil
+}
+
+func (s *OffchainStore) FindReservationBin(ctx context.Context, accountID string, binIndex uint64) (*ReservationBin, error) {


Is this used?

Nope atm, it was intended for an API, which i now realized I've yet to add

mooselumph · 2024-10-02T16:28:09Z

disperser/meterer/offchain_store.go

+}
+
+// Find all reservation bins for a given account
+func (s *OffchainStore) FindReservationBins(ctx context.Context, accountID string) ([]ReservationBin, error) {


Is this used?

mooselumph · 2024-10-02T17:09:22Z

disperser/meterer/onchain_state.go

+
+// OperatorInfo contains information about an operator which is stored on the blockchain state,
+// corresponding to a particular quorum
+type ActiveReservation struct {


Note: neeed to add QuorumNumbers as discussed

mooselumph · 2024-10-02T17:17:20Z

disperser/meterer/types.go

+	// Existing fields
+	Commitment       core.G1Point
+	DataLength       uint32
+	BlobQuorumParams []BlobQuorumParam


This can just be a list of QuorumNumbers, per the new header spec.

mooselumph · 2024-10-02T17:18:28Z

disperser/meterer/types.go

+	BinIndex  uint64
+
+	Signature []byte
+	BlobSize  uint32


This is redundant with DataLength. We should decide on canonical naming.

went with DataLength

mooselumph · 2024-10-02T17:23:21Z

disperser/meterer/types.go

+}
+
+// Protocol defines parameters: FixedFeePerByte; fine to leave global rate-limit offchain atm
+type OnDemand struct {


Is this used?

mooselumph · 2024-10-02T17:32:17Z

disperser/meterer/types.go

+	Version   uint32
+	AccountID string
+	Nonce     uint32 // use nonce to prevent duplicate payments in the same reservation window
+	BinIndex  uint64


It seems like uint32 would be sufficient for this?

updated to be more memory efficient; I'm assuming we don't need to worry about an index in the unit of seconds (previously global bin index is in seconds so 2^32 in unix let this go up to Jan 19, 2038, but with that being removed, uint32 is fine for reservation bin indices

I also updated start & end epochs to use uint32, as well as PricePerChargeable (2^64 gwei ~= 18M Eth; uint32 => ~4ETH), MinChargeableSize (in bytes, this is ~4GB/s), and ReservationWindow (136years but uint16 is only 18hours if we are counting in seconds; we could instead use hour as window unit and allow for 7years).

Left GlobalBytesPerSecond with uint64 for the aspiration of supporting rate-limit greater than 4GB/s network-wide ✨

hopeyen added 5 commits September 30, 2024 08:59

feat: reservation with mocked interfaces

435e46d

feat: on-demand with mocked interfaces

c5858cf

refactor: creatTable's in util

57c83d7

refactor: clean up dead code

8eaa2a8

feat: add meterer to disperser api server

41fd0c8

hopeyen marked this pull request as draft September 30, 2024 22:59

mooselumph reviewed Oct 1, 2024

View reviewed changes

feat: chargeable size calc and some config flags

b14bf86

hopeyen force-pushed the hope/disperser-meterer branch from 36c729b to b14bf86 Compare October 1, 2024 23:30

hopeyen added 3 commits October 1, 2024 16:39

refactor: remove some explicit mocked payment state

8548415

feat: reservation bin limit config

1b65acf

refactor: rm global index validation check for on-demand

671df9b

mooselumph reviewed Oct 2, 2024

View reviewed changes

hopeyen added 7 commits October 2, 2024 11:31

fix: check cumulative payments with on-chain state upperbd

533e6d3

refactor: sorted key, query fn with ordering, payment num type

13f8fde

refactor: rm dead code, add quoromNumber in ActiveReservations

4afd390

feat: reservation quorom check and tests

1af7037

feat: ondemand quorom check and tests

d93c91c

refactor: more memory efficient integer types

e8386ab

refactor: rm dup fields BlobSize -> DataLength

e1d454b

hopeyen force-pushed the hope/disperser-meterer branch from 4bd5a2e to e1d454b Compare October 2, 2024 22:44

hopeyen added 4 commits October 3, 2024 13:32

feat: eigenda client local accountant

18ddb8a

refactor: rm unnecessary fields: nonce, version

24977c1

refactor: accountant sign over blob header fields

ba7bb66

fix: no accounting in normal dispersal blob

dd2ab52

chore: inabox payments e2e test

500ecc4

hopeyen force-pushed the hope/disperser-meterer branch from 9295cbd to 500ecc4 Compare October 4, 2024 18:33

hopeyen and others added 4 commits October 4, 2024 21:03

feat: grpc paid version impl

75acda3

chore: add dummy state for testing (later add dummy contract)

243a2f9

chore: passing payment e2e inabox test with some hardcoded values

219acf5

fix: implement PaidDisperseBlob for mockDisperserClient

9239918

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

disperser meterer for payments #779

disperser meterer for payments #779

hopeyen commented Sep 30, 2024 •

edited

Loading

mooselumph Oct 1, 2024

hopeyen Oct 2, 2024

mooselumph Oct 1, 2024

hopeyen Oct 2, 2024

mooselumph Oct 1, 2024

mooselumph Oct 1, 2024

hopeyen Oct 2, 2024

mooselumph Oct 1, 2024

hopeyen Oct 2, 2024

mooselumph left a comment

mooselumph Oct 2, 2024

hopeyen Oct 2, 2024

mooselumph Oct 2, 2024

mooselumph Oct 2, 2024

hopeyen Oct 2, 2024

mooselumph Oct 2, 2024

mooselumph Oct 2, 2024

mooselumph Oct 2, 2024

mooselumph Oct 2, 2024

hopeyen Oct 2, 2024

mooselumph Oct 2, 2024

mooselumph Oct 2, 2024

hopeyen Oct 2, 2024

		// update = update.Add(expression.Name(itemKey), expression.Value(n.Value))
		// update = update.Add(expression.Name(itemKey), expression.Value(n.Value))

disperser meterer for payments #779

Are you sure you want to change the base?

disperser meterer for payments #779

Conversation

hopeyen commented Sep 30, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hopeyen commented Sep 30, 2024 •

edited

Loading