Skip to content
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

fix!: update aggregation capabilitites to use height instead of size together with client and api #831

Merged
merged 4 commits into from
Jul 25, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/aggregate-api/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@
"@ucanto/server": "^8.0.0",
"@ucanto/transport": "^8.0.0",
"@web3-storage/capabilities": "workspace:^",
"@web3-storage/data-segment": "^1.0.1"
"@web3-storage/data-segment": "^2.1.0"
},
"devDependencies": {
"@ipld/car": "^5.1.1",
Expand Down
37 changes: 27 additions & 10 deletions packages/aggregate-api/src/aggregate/offer.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import * as Server from '@ucanto/server'
import { CBOR } from '@ucanto/core'
import { Node, Aggregate as AggregateBuilder } from '@web3-storage/data-segment'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import { Node, Aggregate as AggregateBuilder } from '@web3-storage/data-segment'
import { Node, Piece, Aggregate as AggregateBuilder } from '@web3-storage/data-segment'

You'll need this to use Piece.fromJSON directly.

import * as Aggregate from '@web3-storage/capabilities/aggregate'
import * as Offer from '@web3-storage/capabilities/offer'
import * as API from '../types.js'

export const MIN_SIZE = 1 + 127 * (1 << 27)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what this number is, and I wonder if it was mistaken for

Suggested change
export const MIN_SIZE = 1 + 127 * (1 << 27)
export const MIN_SIZE = 127 * (1 << 27)

Which would be unpadded size corresponding to 16GiB deal, that is after padding we'd get 16GiB which is probably what was the intention was.

SIZE = 127 * (1 << 27) // 17045651456
DEAL_SIZE = SIZE + SIZE / 127 // 17179869184 => 16GiB

export const MAX_SIZE = 127 * (1 << 28)
// export const MAX_SIZE = 127 * (1 << 28)
export const MAX_SIZE = AggregateBuilder.DEFAULT_DEAL_SIZE
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Gozala interesting that these values are different than ones previously obtained with riba. Clues?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The former value is (unpadded) payload size, if you pad it you'll get a deal (aggregate) size which is the new value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I don't know if that 32GiB is the max deal size for spade, so I'm not sure we do need to impose it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am playing with values that Riba provided us back in the days. Maybe not relevant anymore, but I will confirm and iterate


/**
* @param {API.AggregateServiceContext} context
Expand Down Expand Up @@ -39,29 +41,44 @@ export const claim = async (
}

// Validate offer content
const size = offers.reduce((accum, offer) => accum + offer.size, 0)
if (size < MIN_SIZE) {
const aggregateLeafs = 2n ** BigInt(piece.height)
const aggregateSize = aggregateLeafs * BigInt(Node.Size)

if (aggregateSize < MIN_SIZE) {
return {
error: new AggregateOfferInvalidSizeError(
`offer under size, offered: ${size}, minimum: ${MIN_SIZE}`
`offer under size, offered: ${aggregateSize}, minimum: ${MIN_SIZE}`
),
}
} else if (size > MAX_SIZE) {
} else if (aggregateSize > MAX_SIZE) {
return {
error: new AggregateOfferInvalidSizeError(
`offer over size, offered: ${size}, maximum: ${MAX_SIZE}`
`offer over size, offered: ${aggregateSize}, maximum: ${MAX_SIZE}`
),
}
} else if (size !== piece.size) {
}

// Validate commP of commPs
const aggregateBuild = AggregateBuilder.build({
pieces: offers.map((o) => ({
link: o.link,
size: 2n ** BigInt(o.height) * BigInt(Node.Size),
})),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should pass a size as it it may be different from default, note that builder is not deriving size from pieces it just uses 32GiB default.

You also do not need to do the math there, there is Piece.fromJSON that you could use instead

Suggested change
pieces: offers.map((o) => ({
link: o.link,
size: 2n ** BigInt(o.height) * BigInt(Node.Size),
})),
size: aggregateSize
pieces: offers.map(Piece.fromJSON),

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we expect pieces to be sorted, we should make note of that in the spec, if that is not the case we probably should sort them here.

})
if (!aggregateBuild.link.equals(piece.link)) {
return {
error: new AggregateOfferInvalidSizeError(
`offer size mismatch, specified: ${piece.size}, actual: ${size}`
`aggregate piece CID mismatch, specified: ${piece.link}, computed: ${aggregateBuild.link}`
),
}
} else if (aggregateBuild.height !== piece.height) {
return {
error: new AggregateOfferInvalidSizeError(
`aggregate height mismatch, specified: ${piece.height}, computed: ${aggregateBuild.height}`
),
}
}

// TODO: Validate commP of commPs

// Create effect for receipt
const fx = await Offer.arrange
.invoke({
Expand Down
9 changes: 3 additions & 6 deletions packages/aggregate-api/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import type {
} from '@ucanto/interface'
import type { ProviderInput } from '@ucanto/server'

import type { PieceLink } from '@web3-storage/data-segment'
import type { Piece } from '@web3-storage/aggregate-client/types'
export * from '@web3-storage/aggregate-client/types'

Expand All @@ -30,9 +31,7 @@ export interface ServiceContext
OfferServiceContext {}

export interface ArrangedOfferStore {
get: (
pieceLink: Link<unknown, number, number, 0 | 1>
) => Promise<string | undefined>
get: (pieceLink: PieceLink) => Promise<string | undefined>
}

export interface OfferStore {
Expand All @@ -45,9 +44,7 @@ export interface OfferToQueue {
}

export interface AggregateStore {
get: (
pieceLink: Link<unknown, number, number, 0 | 1>
) => Promise<unknown[] | undefined>
get: (pieceLink: PieceLink) => Promise<unknown[] | undefined>
}

export interface UcantoServerContext extends ServiceContext {
Expand Down
Loading