[payments] disperser client local metering #793

hopeyen · 2024-10-08T03:52:37Z

Why are these changes needed?

Disperser client integrated with payments metering with inabox integration tests

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-28T18:57:18Z

api/clients/accountant.go

+	// binUsage := a.binUsages[0] + dataLength
+	a.usageLock.Lock()
+	defer a.usageLock.Unlock()
+	a.binUsages[0] += dataLength


The way that the accountant is doing bin accounting looks to me like it might be incorrect; in particular, it might not line up with the server, since the transitions are happening at different points on the client than on the server.

I have another idea for how we could possibly do this. Will follow up!

updated to use the wrapped bin array

mooselumph

Looks really good! I'll continue reviewing after the discussed changes to the accountant!

mooselumph · 2024-10-28T23:12:22Z

disperser/apiserver/server.go

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

 	// 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{}) {
-		err := s.meterer.MeterRequest(ctx, *blob, *paymentHeader)
+		err := s.meterer.MeterRequest(ctx, *paymentHeader, uint(blobSize), blob.GetQuorumNumbers())


I think this needs to be converted from byte length to symbol length (

eigenda/encoding/utils.go

Line 10 in db3da32

func GetBlobLength(blobSize uint) uint {

)

thank for the link, I've updated to use the conversion

mooselumph · 2024-10-28T23:17:58Z

disperser/apiserver/payment_test.go

+	pk := "0x0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdeb"
+	signer := auth.NewPaymentSigner(pk)
+
+	dataLength := len(data)


Similarly to the server.go comment, I believe we should be using the number of symbols, here, correct?

ian-shim · 2024-10-30T16:38:32Z

api/clients/accountant.go

+	return &a
+}
+
+// accountant calculates and records payment information


nit: BlobPaymentInfo calculates...

can we also document what's being returned?

updated the comments

ian-shim · 2024-10-30T16:38:58Z

api/clients/accountant.go

+	binRecords        []BinRecord
+	usageLock         sync.Mutex
+	cumulativePayment *big.Int
+	stopRotation      chan struct{}


How is this used?

outdated concept, removed:)

ian-shim · 2024-10-30T16:41:24Z

api/clients/disperser_client.go

 }

 var _ DisperserClient = &disperserClient{}

-// DisperserClient maintains a single underlying grpc connection to the disperser server,


Why don't we keep this doc?

Sorry for taking it out, I'm not sure what happened, putting it back in

ian-shim · 2024-10-30T16:42:05Z

api/clients/disperser_client.go

 func (c *disperserClient) DispersePaidBlob(ctx context.Context, data []byte, quorums []uint8) (*disperser.BlobStatus, []byte, error) {
-	return nil, nil, api.NewErrorInternal("not implemented")


Keep returning this error if accountant is not configured?

sounds good, added it

ian-shim · 2024-10-30T16:42:12Z

api/clients/disperser_client.go

+		return nil, nil, fmt.Errorf("encountered an error to convert a 32-bytes into a valid field element, please use the correct format where every 32bytes(big-endian) is less than 21888242871839275222246405745257275088548364400416034343698204186575808495617 %w", err)
+	}
+
+	header, signature, err := c.accountant.AccountBlob(ctx, uint64(len(data)), quorums)


Check nil pointer?

the inner level SignBlobPayment(protoPaymentHeader) should error out if the header is nil so I don't expect this to happen, but added it for additional safety

ian-shim · 2024-10-30T16:46:28Z

api/clients/accountant.go

+	Usage uint64
+}
+
+func NewAccountant(reservation core.ActiveReservation, onDemand core.OnDemandPayment, reservationWindow uint32, pricePerSymbol uint32, minNumSymbols uint32, paymentSigner core.PaymentSigner) *Accountant {


any validations needed here?
Also, should we be passing in pointers to the structs?

I think a lot of what's going on here will be replaced by #835 . binRecords and cumulativePayment should change more frequently compared to the global parameters, and no one else needs access to the structs. would you help me understand what we will get by using pointers here?

Passing by values will make copies of the structs. Unless you're modifying the input structs, it's more efficient to pass by pointer

New accountant would modify the input structs in later functions. Since there's no other use for the input params after being passed in, I thought it would be good for the accountant to take ownership of the params and imagined a garbage collector. Now I realize I'm thinking in rust 😅 gonna update this to be more efficient

ian-shim · 2024-10-30T16:50:57Z

api/clients/accountant.go

+	"github.com/Layr-Labs/eigenda/core/meterer"
+)
+
+type IAccountant interface {


should we call the interface Accountant and the struct accountant and only export the interface?

is https://github.com/Layr-Labs/eigenda/pull/847/files a fix to this? I think it looks good

Nah that one does something similar to PaymentSigner, not to Accountant

made similar changes to accountant

Can we remove the I prefix now?

ian-shim · 2024-10-30T16:53:51Z

api/clients/eigenda_client.go

@@ -298,6 +304,125 @@ func (m *EigenDAClient) putBlob(ctx context.Context, rawData []byte, resultChan
 	}
 }

+// PaidPutBlob behaves like PutBlob but with authenticated payment.


Looks like this set of methods are copy paste of the existing set, except calling DispersePaidBlob instead of DisperseBlobAuthenticated. Could we integrate this to the existing method?

yep, did a similar refactoring that we did on the server side; if there's a setting on "PaymentSignerPrivateKeyHex", then we call out to DispersePaidBlob, otherwise DisperseBlobAuthenticated. lmk if this is okay?

mooselumph

Nice!

mooselumph · 2024-10-30T22:06:42Z

api/clients/accountant.go

+// then on-demand if the reservation is not available. The returned values are
+// bin index for reservation payments and cumulative payment for on-demand payments,
+// and both fields are used to create the payment header and signature
+func (a *Accountant) BlobPaymentInfo(ctx context.Context, numSymbols uint64, quorumNumbers []uint8) (uint32, *big.Int, error) {


The logic in this function is pretty coupled with the core/meterer library. It might be worth considering whether there are ways to encapsulate the core logic and reuse it across these contexts. Maybe an improvement for later.

mooselumph · 2024-10-30T22:08:19Z

api/clients/accountant.go

+	incrementRequired := big.NewInt(int64(a.PaymentCharged(uint(numSymbols))))
+	a.cumulativePayment.Add(a.cumulativePayment, incrementRequired)
+	if a.cumulativePayment.Cmp(a.onDemand.CumulativePayment) <= 0 {
+		if err := QuorumCheck(quorumNumbers, []uint8{0, 1}); err != nil {


Guessing these parameters will be read from the disperser endpoint in the next PR?

I didn't intend on reading this from disperser endpoint. Everything else made sense to, but the quorum numbers were not included in the next PR because I was expecting them to stay as 0 and 1. alternatively, I could mark them as a hard coded constant and/or add to accountant struct.

I think either of these is fine. In the event that this set increased, it seems like it wouldn't be a breaking change for clients still using {0,1}. Maybe just a hard-coded constant for legibility would be good!

mooselumph · 2024-10-30T22:13:46Z

api/clients/accountant.go

+	}
+	protoPaymentHeader := pm.ConvertToProtoPaymentHeader()
+
+	signature, err := a.paymentSigner.SignBlobPayment(protoPaymentHeader)


I know this would have been a previous PR, but why does SignBlobPayment take the proto struct instead of the core golang struct here, out of curiosity?

uhhhhh I think it was because DispersePaidBlobRequest gave me the proto struct. This is a good question, I thought about it a bit and added a conversion function (proto->golang) so we are now using the local struct

mooselumph · 2024-10-30T22:16:16Z

api/clients/accountant.go

+}
+
+func (a *Accountant) GetRelativeBinRecord(index uint32) *BinRecord {
+	relativeIndex := index % 3


I think I'm convinced that 3 should always work here. But is there any reason to make this configurable? Maybe a hard coded constant would be better?

sure 🤔 maybe a client wants more insights into the past and use a bigger number? I added a config to EigenDA proxy client and the accountant will take the maximum between the setting and the minimum number of bins (hardcoded to 3)

mooselumph · 2024-10-30T22:55:56Z

disperser/apiserver/server_test.go

@@ -683,6 +685,22 @@ func newTestServer(transactor core.Writer, testName string) *apiserver.Dispersal
 		panic("failed to make initial query to the on-chain state")
 	}

+	mockState.On("GetPricePerSymbol").Return(uint32(encoding.BYTES_PER_SYMBOL), nil)


I'm a little confused about why you decided to use encoding.BYTES_PER_SYMBOL for this. 😅

ha uhhh this is because the previous tests was made on the basis of charging per byte instead of per symbol, and I was lazy to redo the calculation after the blob size conversion, so I just scaled the price 😅

ian-shim · 2024-11-01T16:28:36Z

api/clients/accountant.go

+	return 0, big.NewInt(0), fmt.Errorf("neither reservation nor on-demand payment is available")
+}
+
+// accountant provides and records payment information


nit: AccountBlob provides...

ian-shim · 2024-11-01T16:30:43Z

api/clients/disperser_client.go

@@ -78,6 +78,8 @@ type disperserClient struct {
 	//       instead of a real network connection for eg.
 	conn   *grpc.ClientConn
 	client disperser_rpc.DisperserClient
+
+	accountant Accountant


should this be removed?

I thought it would be okay to leave as is, but removing for clarity

hopeyen changed the base branch from master to hope/disperser-server-metering October 8, 2024 03:52

hopeyen changed the title ~~disperser client metering~~ disperser client local metering Oct 8, 2024

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

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

hopeyen force-pushed the hope/disperser-client-metering branch from 887827c to e3a3eda Compare October 11, 2024 18:42

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

hopeyen force-pushed the hope/disperser-client-metering branch from e3a3eda to cf39a09 Compare October 22, 2024 02:32

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

hopeyen force-pushed the hope/disperser-client-metering branch 2 times, most recently from 32e61dd to 072e007 Compare October 23, 2024 20:39

hopeyen force-pushed the hope/disperser-server-metering branch 5 times, most recently from 45a72b2 to 4218aba Compare October 25, 2024 00:17

hopeyen force-pushed the hope/disperser-client-metering branch 3 times, most recently from 835f1be to 76e1aad Compare October 25, 2024 21:35

mooselumph reviewed Oct 28, 2024

View reviewed changes

hopeyen force-pushed the hope/disperser-client-metering branch from 313a2cf to e20e9b6 Compare October 29, 2024 01:48

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

hopeyen requested review from mooselumph and ian-shim October 30, 2024 04:08

hopeyen force-pushed the hope/disperser-client-metering branch from 6797360 to c446db2 Compare October 30, 2024 08:50

hopeyen changed the base branch from hope/disperser-server-metering to master October 30, 2024 08:51

ian-shim reviewed Oct 30, 2024

View reviewed changes

mooselumph reviewed Oct 30, 2024

View reviewed changes

mooselumph approved these changes Oct 31, 2024

View reviewed changes

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

hopeyen added 10 commits October 31, 2024 20:53

feat: client payment api integration

a8e78a1

feat: blob length converted to symbol

5c46052

fix: bin record tracking with modularized bin index

143086d

refactor: symbolLength consistency

8952272

refactor: update tests and add quorum check

150733d

refactor: comments and minor updates

752b57c

refactor: use pointers

3bbec23

refactor: type conversions

0e5b034

refactor: hardcode required quorum constant

5b0d1c6

refactor: rm comment and update interface input

61a4c3b

hopeyen force-pushed the hope/disperser-client-metering branch 13 times, most recently from 3267b20 to e0369f3 Compare November 1, 2024 13:46

fix: proto, contracts, tests

150582d

hopeyen force-pushed the hope/disperser-client-metering branch from e0369f3 to 150582d Compare November 1, 2024 14:27

fix: try to fix inabox integration test

d065607

ian-shim approved these changes Nov 1, 2024

View reviewed changes

refactor: address last feedbacks

54f2300

hopeyen merged commit 7af65ea into master Nov 2, 2024
9 checks passed

hopeyen deleted the hope/disperser-client-metering branch November 2, 2024 12:18

		func (c disperserClient) DispersePaidBlob(ctx context.Context, data []byte, quorums []uint8) (disperser.BlobStatus, []byte, error) {
		return nil, nil, api.NewErrorInternal("not implemented")

[payments] disperser client local metering #793

[payments] disperser client local metering #793

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

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

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

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