-
Notifications
You must be signed in to change notification settings - Fork 28
feat: cap addPieces batches at 40, tighten piece metadata limits #814
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -91,6 +91,14 @@ export const SIZE_CONSTANTS = { | |
| */ | ||
| DEFAULT_UPLOAD_BATCH_SIZE: 32, | ||
|
|
||
| /** | ||
| * Maximum pieces per addPieces (or createDataSetAndAddPieces) call. | ||
| * | ||
| * On-chain limitations fail batch sizes above 41; we constrain to 40 here to | ||
| * catch those failures early and surface informative errors. | ||
| */ | ||
| MAX_ADD_PIECES_BATCH_SIZE: 40, | ||
|
Comment on lines
+94
to
+100
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. intentional
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is 1 a good margin? Would 2 be better? What if we use no safety margin?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not really intended as a margin, it's intended as a round number cause exposing 41 invites more questions .. but I guess the comment also invites questions. I was trying to be vague so as to not explain the details too much. We know >41 breaks the fvm event limit. We are relatively confident that fully-loaded metadata now shouldn't break ~40 with gas. I have a small amount of confidence that create+add with fully loaded metadata shouldn't break it but I need to wait till we have everything merged and set up before I can measure that—but even then, gas on mainnet is very different to devnet and different still from calibnet so we're doing a bit of stabbing in the dark without actually running it on mainnet.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it! I don't think 40 is better than 41 here, it's as magic a number. If we decide to keep 40, I suggest to add to the document that we're doing this to not raise suspicion |
||
|
|
||
| /** | ||
| * Bytes per leaf in the PDP merkle tree. | ||
| * The FWSS contract converts leaf counts to bytes via `totalBytes = leafCount * BYTES_PER_LEAF`. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import assert from 'assert' | ||
| import { AtLeastOnePieceRequiredError, TooManyPiecesError } from '../src/errors/warm-storage.ts' | ||
| import { validateAddPiecesBatch } from '../src/sp/add-pieces.ts' | ||
| import { SIZE_CONSTANTS } from '../src/utils/constants.ts' | ||
|
|
||
| describe('validateAddPiecesBatch', () => { | ||
| const max = SIZE_CONSTANTS.MAX_ADD_PIECES_BATCH_SIZE | ||
|
|
||
| it('should throw when empty', () => { | ||
| assert.throws(() => validateAddPiecesBatch(0), AtLeastOnePieceRequiredError) | ||
| }) | ||
|
|
||
| it('should throw for non-positive or non-integer counts', () => { | ||
| for (const bad of [-1, 1.5, Number.NaN, Number.POSITIVE_INFINITY]) { | ||
| assert.throws(() => validateAddPiecesBatch(bad), AtLeastOnePieceRequiredError) | ||
| } | ||
| }) | ||
|
|
||
| it('should accept a count at the maximum', () => { | ||
| assert.doesNotThrow(() => validateAddPiecesBatch(max)) | ||
| }) | ||
|
|
||
| it('should throw when above the maximum', () => { | ||
| assert.throws(() => validateAddPiecesBatch(max + 1), TooManyPiecesError) | ||
| }) | ||
| }) |
Uh oh!
There was an error while loading. Please reload this page.