[payments] meterer core logic #790

hopeyen · 2024-10-08T01:48:12Z

Why are these changes needed?

Core logic of 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 :(

mooselumph · 2024-10-08T17:49:17Z

core/meterer/meterer.go

+	if err := m.ValidateQuorum(blobHeader, reservation.QuorumNumbers); err != nil {
+		return fmt.Errorf("invalid quorum for reservation: %w", err)
+	}
+	if !m.ValidateBinIndex(blobHeader, reservation) {


What do you think of the idea of adding a ReceiptTime field to the context when we receive the message and then using these field when we validate the bin index?

I think this idea is similar to having disperser set the bin index of a message. The main trade-off was client cannot easily predict the ReceiptTime and the behavior of the message in the local accounting. I think this is a good idea with tolerance, something like MaxDelayReceiveTime?

mooselumph · 2024-10-08T17:53:42Z

core/meterer/meterer.go

+	return nil
+}
+
+func (m *Meterer) ValidateQuorum(blobHeader BlobHeader, allowedQuoroms []uint8) error {


Add comment: ValidateQuorums ensures that the quorums listed in the blobHeader are present within allowedQuorums

mooselumph · 2024-10-08T18:07:52Z

core/meterer/meterer.go

+type Config struct {
+	GlobalBytesPerSecond uint64 // 2^64 bytes ~= 18 exabytes per second; if we use uint32, that's ~4GB/s
+	PricePerChargeable   uint32 // 2^64 gwei ~= 18M Eth; uint32 => ~4ETH
+	MinChargeableSize    uint32


Note: In current implementation, this should be in units of Symbols since DataLength is also in symbols. As we've discussed, this parametrization is subject to change.

mooselumph · 2024-10-08T19:30:29Z

core/meterer/meterer_test.go

+	assert.Error(t, err, "invalid bin index for reservation")
+
+	// test bin usage
+	accountID := crypto.PubkeyToAddress(privateKey2.PublicKey).Hex()


Shouldn't this bin index pass?

core/meterer/meterer_test.go

mooselumph · 2024-10-08T20:38:07Z

core/meterer/meterer_test.go

+	header, err = meterer.ConstructPaymentMetadata(signer, binIndex, 0, 50, quorumNumbers, privateKey2)
+	assert.NoError(t, err)
+	err = mt.MeterRequest(ctx, *header)
+	assert.Error(t, err, "cannot insert cumulative payment in out of order")


I don't completely understand how this is different from the previous check...

mooselumph

looks good! A few more comments

mooselumph · 2024-10-15T16:41:02Z

core/meterer/meterer.go

+	return nil
+}
+
+// ValidateQuorums ensures that the quorums listed in the blobHeader are present within allowedQuorums


Note: A reservation that does not utilize all of the allowed quorums will be accepted. However, it will still charge against all of the allowed quorums. We should make sure this is the desired UX.

mooselumph · 2024-10-15T16:55:47Z

core/meterer/meterer_test.go

+		assert.Equal(t, strconv.Itoa(int((i+1)*dataLength)), item["BinUsage"].(*types.AttributeValueMemberN).Value)
+
+	}
+	// frist over flow is allowed


mooselumph · 2024-10-15T21:44:43Z

core/meterer/meterer_test.go

+	assert.ErrorContains(t, err, "bin has already been filled")
+
+	// overwhelming bin overflow for empty bins (assuming all previous requests happened within 1 reservation window)
+	header, err = auth.ConstructPaymentMetadata(&signer, binIndex-1, 0, 1000, quoromNumbers, privateKey2)


Shoud we also have a passing test for binIndex-1?

yep! added it and ready for review

mooselumph · 2024-10-15T21:52:47Z

core/meterer/meterer_test.go

-		GlobalBytesPerSecond: 1000,
-		ReservationWindow:    60,
-		ChainReadTimeout:     3 * time.Second,
+		PricePerSymbol:         1,


Would it be worth using some unique prime values here for different parameters (e.g. 2,3,5,7) to give better confidence that the calcualtions are being done correctly?

mooselumph · 2024-10-16T20:45:53Z

core/data.go

@@ -490,22 +490,24 @@ type PaymentMetadata struct {
 	BinIndex uint32
 	// TODO: we are thinking the contract can use uint128 for cumulative payment,
 	// but the definition on v2 uses uint64. Double check with team.
-	CumulativePayment uint64
+	CumulativePayment big.Int


I think it's common practice to use a pointer for big.Int, i.e. CumulativePayment *big.Int, as most methods consume the pointer.

sg, updated

ian-shim · 2024-10-21T21:33:05Z

core/meterer/onchain_state.go

 	}
+	pcs.ActiveReservations = activeReservations


I think we should lock this method unless there is a strong reason not to, as i don't think it will cause any performance issues but there is a chance ActiveReservations and OnDemandPayments are accessed in parallel?

ian-shim

just one comment around locking

ian-shim · 2024-10-22T03:59:05Z

core/meterer/onchain_state.go

+	pcs.OnDemandPayments = onDemandPayments
+	pcs.OnDemandLocks.Unlock()
+
+	return nil
 }

 func (pcs *OnchainPaymentState) GetActiveReservations(ctx context.Context, blockNumber uint) (map[string]core.ActiveReservation, error) {
 	return pcs.ActiveReservations, nil


I think it needs to get RLock here

ian-shim · 2024-10-22T03:59:24Z

core/meterer/onchain_state.go

+	pcs.ReservationsLock.Lock()
+	pcs.ActiveReservations[accountID] = res
+	pcs.ReservationsLock.Unlock()
+	return res, nil
 }

 func (pcs *OnchainPaymentState) GetOnDemandPayments(ctx context.Context, blockNumber uint) (map[string]core.OnDemandPayment, error) {
 	return pcs.OnDemandPayments, nil


hopeyen changed the title ~~feat: meterer core logic~~ meterer core logic Oct 8, 2024

hopeyen changed the title ~~meterer core logic~~ [payments] meterer core logic Oct 8, 2024

hopeyen force-pushed the hope/meterer-helpers branch 5 times, most recently from f553c37 to 6ae70ef Compare October 8, 2024 18:02

hopeyen force-pushed the hope/meterer-logic branch 2 times, most recently from 6092848 to 2211a82 Compare October 8, 2024 19:30

mooselumph reviewed Oct 8, 2024

View reviewed changes

hopeyen force-pushed the hope/meterer-logic branch from 60d7524 to 0323624 Compare October 8, 2024 21:05

hopeyen force-pushed the hope/meterer-helpers branch 3 times, most recently from 31932db to f43e6cf Compare October 9, 2024 21:04

hopeyen force-pushed the hope/meterer-logic branch from ec28469 to 50c7f63 Compare October 9, 2024 22:49

hopeyen force-pushed the hope/meterer-helpers branch 4 times, most recently from ac0c882 to fab84e3 Compare October 11, 2024 00:50

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

mooselumph reviewed Oct 15, 2024

View reviewed changes

hopeyen force-pushed the hope/meterer-helpers branch from aae5395 to 4a50bf6 Compare October 15, 2024 22:21

hopeyen force-pushed the hope/meterer-logic branch from d402849 to d86aab9 Compare October 16, 2024 00:04

hopeyen force-pushed the hope/meterer-helpers branch 2 times, most recently from 0d0dabf to 9d5663a Compare October 16, 2024 17:31

hopeyen force-pushed the hope/meterer-logic branch from ea5bc3b to a1e4589 Compare October 16, 2024 17:40

mooselumph reviewed Oct 16, 2024

View reviewed changes

hopeyen force-pushed the hope/meterer-logic branch from 2f44b2f to b956ae0 Compare October 16, 2024 21:43

mooselumph approved these changes Oct 16, 2024

View reviewed changes

hopeyen added 18 commits October 17, 2024 11:17

refactor: PaymentMetadata to core/data, signing to core/auth

2729527

chore: use Mock for on-chain payments in tests

0a8588a

fix: db creation in tests, mock filename

427b64d

fix: lints and db/signer tests

0c800f0

feat: numeric incremental updates, query with ordering and lim

27fa23c

feat: IncrementBy update fn

fdc1bc4

feat: meterer core logic

19f4a62

refactor: unit of price and datalength based on symbols

f0abee9

refactor: test mocks and size/price calculation

1cc4376

chore: update tests for new payment structs

8566767

chore: use prime for meterer test config

fab86bf

refactor: update TokenAmount type in payment metadata and dynamoDB

ffcc329

feat: ping contract when missing local payment account

3de9147

feat: meterer periodic on-chain state refresh

861b004

refactor: use pointer for big int

4bc0780

fix: lint

ddcd899

fix: db QueryWithInput test

20214fb

refactor: meterer Start() routine for refreshing on-chain

b085195

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

refactor: rebase with type changes

0aaa646

hopeyen force-pushed the hope/meterer-logic branch from 462f216 to 0aaa646 Compare October 17, 2024 18:35

hopeyen requested a review from ian-shim October 18, 2024 21:39

ian-shim reviewed Oct 21, 2024

View reviewed changes

ian-shim approved these changes Oct 21, 2024

View reviewed changes

hopeyen force-pushed the hope/meterer-logic branch from cc53fab to 05f9379 Compare October 22, 2024 01:44

feat: add on-chain write lock

3521295

hopeyen force-pushed the hope/meterer-logic branch from 05f9379 to 3521295 Compare October 22, 2024 01:45

hopeyen merged commit febcd95 into master Oct 22, 2024
10 checks passed

hopeyen deleted the hope/meterer-logic branch October 22, 2024 02:48

ian-shim reviewed Oct 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[payments] meterer core logic #790

[payments] meterer core logic #790

hopeyen commented Oct 8, 2024

mooselumph Oct 8, 2024

hopeyen Oct 8, 2024

mooselumph Oct 8, 2024

mooselumph Oct 8, 2024

mooselumph Oct 8, 2024

mooselumph Oct 8, 2024

mooselumph left a comment

mooselumph Oct 15, 2024

mooselumph Oct 15, 2024

mooselumph Oct 15, 2024

hopeyen Oct 16, 2024

mooselumph Oct 15, 2024

mooselumph Oct 16, 2024

hopeyen Oct 16, 2024

ian-shim Oct 21, 2024

ian-shim left a comment

ian-shim Oct 22, 2024

ian-shim Oct 22, 2024

[payments] meterer core logic #790

[payments] meterer core logic #790

Conversation

hopeyen commented Oct 8, 2024

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

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

ian-shim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment