-
Notifications
You must be signed in to change notification settings - Fork 238
docs: Simplify and improve payment header docs #1820
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: litt3 <[email protected]>
The latest Buf updates on your PR. Results from workflow Buf Proto / buf (pull_request).
|
Signed-off-by: litt3 <[email protected]>
Signed-off-by: litt3 <[email protected]>
I thought protoc runs in docker so it isn't needed? Are you using the correct make target? I don't think anyone uses the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, very clean. Have a bunch of small nits
api/proto/common/v2/common_v2.proto
Outdated
// payments are deducted as they make dispersal requests. This system is more flexible but has | ||
// more restrictions on which quorums can be used (currently limited to quorums 0 and 1). | ||
// The "size" of a dispersal, with respect to payments, is defined as the number of symbols being dispersed, rounded up | ||
// to the nearest multiple of the minNumSymbols defined in the PaymentVault contract. This size is relevant for both |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh really? So that means we can only pay for 128KiB, 256KiB, 384KiB, etc?
Thought blobs had to be a multiple of 2? Aka 384KiB is not possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this statement is technically correct. The cost is rounded up to the next multiple of 128KB
... it's just that the only case where a blob size isn't already a multiple of 128KB
(due to the power of 2 requirement) is for blobs that are smaller than 128KB
.
So I think an equally correct, but simpler, way to express this would be to say that blobs smaller than 128KB
are charged for 128KB
.
Please do check my reasoning here, but I'm gonna go ahead and make this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. So are we actually rounding up to the next multiple of 128KiB in the code somewhere? Might be simpler and require less reasoning to only enforce the min constraint in the code, so that people don't have to make this reasoning jump that you made (which I agree is correct)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
api/proto/common/v2/common_v2.proto
Outdated
// This is a pay-as-you-go model where users deposit funds into the PaymentVault contract and | ||
// payments are deducted as they make dispersal requests. This system is more flexible but has | ||
// more restrictions on which quorums can be used (currently limited to quorums 0 and 1). | ||
// The "size" of a dispersal, with respect to payments, is defined as the number of symbols being dispersed, rounded up |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// The "size" of a dispersal, with respect to payments, is defined as the number of symbols being dispersed, rounded up | |
// The "size" of a dispersal, with respect to payments, is defined as the number of symbols (aka bn254 field elements) being dispersed, rounded up |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reworded this sentence for a previous comment, so this comment isn't directly applicable anymore.
Still, I'm curious if you you think we should be defining "symbols" here. IMO it seems like symbols
are fundamental enough to our system that we shouldn't feel obligated to give the "aka" everywhere we talk about them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asked the question on slack: https://eigenlabs.slack.com/archives/C08SR37JZ7X/p1754503676460419
I wish we only used a single term everywhere. I agree it doesn't make a lot of sense to say "aka" everywhere though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 would be really nice to use FE everywhere, and ditch "symbols"
// IMPORTANT: With the current implementation, the cumulative payment of dispersals must be strictly increasing from | ||
// the perspective of the entity doing the accounting. If a given cumulative payment X is <= the cumulative payment | ||
// of a previous dispersal, then X is considered to be invalid. The implication is that a user must not behave in any | ||
// way that could result in payments being processed out of order, or risk dispersals failing without refund. In | ||
// practice, that means waiting for confirmation from the disperser that a blob has been received before submitting | ||
// the next blob. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugh thanks for finally writing this clearly! This explains the mutex we have on the client then. This is so bad...
I'm not sure how to protect from this. Even if we have the singleton controller where payment is serialized, the requests could still arrive out of order... we need to make sure causal order is maintained. Meaning either we serialize on the client, or else the server is forced to buffer somewhere (either in tcp if we force grpc to use a single tcp connection with single http/2 stream, or on app-side, as you mention below).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like I mention in the TODO below, I don't think it would actually be that hard to permit some level of concurrency. If you keep track of some metadata for recent payments, it's ok if they arrive a bit out of order
@samlaf I wasn't aware of what the docker based protoc was. I talked this over with @cody-littley, and we agreed that with I propose the following:
|
Signed-off-by: litt3 <[email protected]>
Big fan of this |
Signed-off-by: litt3 <[email protected]>
Signed-off-by: litt3 <[email protected]>
Signed-off-by: litt3 <[email protected]>
Signed-off-by: litt3 <[email protected]>
Signed-off-by: litt3 <[email protected]>
Signed-off-by: litt3 <[email protected]>
Signed-off-by: litt3 <[email protected]>
Signed-off-by: litt3 <[email protected]>
Signed-off-by: litt3 <[email protected]>
@claude /nitpick this PR, using the claude slash command found at Once obtaining the output from the nitpick command:
|
Claude finished @litt3's task —— View job Nitpick Command Completed
Note: I don't have access to the Nitpick Report1. api/proto/common/v2/common_v2.proto:37The phrase "Reserved-bandwidth dispersal do support" contains a grammatical error - should use "dispersals" (plural) @@ -37,1 +37,1 @@
-// Reserved-bandwidth dispersal do support custom quorums, as long as they are reserved onchain ahead of time. The quorum_numbers specified here must be a subset of the ones allowed by the on-chain reservation.
+// Reserved-bandwidth dispersals do support custom quorums, as long as they are reserved onchain ahead of time. The quorum_numbers specified here must be a subset of the ones allowed by the on-chain reservation. 2. api/proto/common/v2/common_v2.proto:96Inconsistent terminology - should use "symbols" instead of "minNumSymbols" when referring to the minimum size for clarity @@ -96,1 +96,1 @@
-// are billed as `minNumSymbols`.
+// are billed as if they were `minNumSymbols` symbols. 3. api/proto/common/v2/common_v2.proto:125TODO comment references "fixed window algorithm" but earlier review comments suggested using "bin algorithm" - terminology should be consistent throughout documentation @@ -125,1 +125,1 @@
-// TODO(litt3): there is an ongoing effort to use a leaky bucket algorithm instead of a fixed window algorithm to
+// TODO(litt3): there is an ongoing effort to use a leaky bucket algorithm instead of a bin algorithm to 4. api/proto/common/v2/common_v2.proto:134The field description uses "variable-sized big endian unsigned integer" but should specify the exact encoding format as suggested in review comments @@ -134,1 +134,1 @@
- // The cumulative_payment field is a variable-sized big endian unsigned integer, representing the total wei paid by
+ // The cumulative_payment field is a big endian serialized uint256 representing the total wei paid by |
@claude In a previous invocation on this PR, claude attempted to run the /nitpick command but couldn't create individual PR review comments because the Please list ALL tools you currently have access to (the complete list shown at initialization). |
Claude finished @litt3's task —— View job Current Tool Access List
Here are ALL the tools I currently have access to: Core Development Tools
Planning & Organization Tools
GitHub Integration Tools
What's MissingYou're correct that I do NOT have access to:
ImplicationsThis means I can:
But I cannot:
The previous Claude invocation was correct - I can only post the nitpick results in this single comment rather than creating individual review comments on specific lines. |
closes EGDA-1671
protoc
tools tomise
, since it's necessary whenever updating protobuf docs