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

vasco-santos · 2023-07-17T14:45:40Z

Renames size to height for piece/aggregate based on described in storacha/data-segment#13

Note that:

took the opportunity to update this to finally rely on data-segment lib as it is now ready to be used (both in tests, and to validate aggregate)
given piece and aggregate are both perfect binary trees, leaf count of can be derived as 2 ** height, and the size of the tree can be derived by leafCount * Node.Size (32).

…together with client and api

vasco-santos · 2023-07-17T14:47:29Z

packages/aggregate-api/src/aggregate/offer.js

 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)
-export const MAX_SIZE = 127 * (1 << 28)
+// export const MAX_SIZE = 127 * (1 << 28)
+export const MAX_SIZE = AggregateBuilder.DEFAULT_DEAL_SIZE


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

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

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.

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

vasco-santos · 2023-07-17T14:53:36Z

packages/capabilities/src/aggregate.js

-          code: SHA2_256_TRUNC254_PADDED,
-        },
-      }),
+      link: /** @type {import('./types').PieceLinkSchema} */ (


@Gozala we can't add first unknown in template config https://github.com/web3-storage/ucanto/blob/main/packages/core/src/schema/link.js#L89 so that we can specify Link<MerkleTreeNode, 61697, 4114, 1> instead of Link<unknown, 61697, 4114, 1>

Should we add a config param in ucanto?

You can not because there is no way to verify it, so I was expecting that ppl would write manually type cast like Schema<Link<T, ...>> that way you you imply to know what you're doing.

I'm not sure what config param in ucanto would look like. That said I have in-progress PR that would allow us to infer types like Schema.struct({ ... }).link({ codec, hasher, version }) which than would be able to infer the type from the schema.

ok, will keep as is for the time being

Gozala

Looks good. Let's just make sure that MIN_SIZE and MAX_SIZE are what we expect them to be. I also proposed few minor changes to avoid repeating functionality provided by data-segment lib.

Gozala · 2023-07-17T23:27:20Z

packages/aggregate-api/src/aggregate/offer.js

 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)
-export const MAX_SIZE = 127 * (1 << 28)
+// export const MAX_SIZE = 127 * (1 << 28)
+export const MAX_SIZE = AggregateBuilder.DEFAULT_DEAL_SIZE


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

Gozala · 2023-07-17T23:29:03Z

packages/aggregate-api/src/aggregate/offer.js

 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)
-export const MAX_SIZE = 127 * (1 << 28)
+// export const MAX_SIZE = 127 * (1 << 28)
+export const MAX_SIZE = AggregateBuilder.DEFAULT_DEAL_SIZE


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.

Gozala · 2023-07-17T23:47:32Z

packages/aggregate-api/src/aggregate/offer.js

@@ -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'
 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)


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

Gozala · 2023-07-17T23:50:40Z

packages/aggregate-api/src/aggregate/offer.js

+    pieces: offers.map((o) => ({
+      link: o.link,
+      size: 2n ** BigInt(o.height) * BigInt(Node.Size),
+    })),


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),

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.

Gozala · 2023-07-17T23:51:20Z

packages/aggregate-api/src/aggregate/offer.js

@@ -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'


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.

Gozala · 2023-07-18T00:00:16Z

packages/aggregate-api/test/aggregate.js

+    // Generate Pieces for offer
+    const { pieces, aggregate } = await randomAggregate(100, 128)
+    const badHeight = 31
+    const size = 2n ** BigInt(badHeight) * BigInt(Node.Size)


We already have PaddedSize.toHeight we probably should just add PaddedSize.fromHeight so we don't have to do this math all over the place.

storacha/data-segment#18

Gozala · 2023-07-18T00:06:26Z

packages/aggregate-client/src/types.ts

-  link: PieceCID
-  size: number
+  link: PieceLink
+  height: number
 }


Looks like we need another type along with Piece and PieceInfo that we can re-export here.

storacha/data-segment#17

Gozala · 2023-07-18T00:13:43Z

packages/capabilities/src/aggregate.js

-          code: SHA2_256_TRUNC254_PADDED,
-        },
-      }),
+      link: /** @type {import('./types').PieceLinkSchema} */ (


You can not because there is no way to verify it, so I was expecting that ppl would write manually type cast like Schema<Link<T, ...>> that way you you imply to know what you're doing.

I'm not sure what config param in ucanto would look like. That said I have in-progress PR that would allow us to infer types like Schema.struct({ ... }).link({ codec, hasher, version }) which than would be able to infer the type from the schema.

🤖 I have created a release *beep* *boop* --- ## [8.0.0](capabilities-v7.0.0...capabilities-v8.0.0) (2023-08-09) ### ⚠ BREAKING CHANGES * update aggregation capabilitites to use height instead of size together with client and api ([#831](#831)) ### Features * w3filecoin new client and api ([#848](#848)) ([7a58fbe](7a58fbe)) ### Bug Fixes * update aggregation capabilitites to use height instead of size together with client and api ([#831](#831)) ([31730f0](31730f0)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

fix!: update aggregation capabilitites to use height instead of size …

3250e85

…together with client and api

vasco-santos commented Jul 17, 2023

View reviewed changes

vasco-santos requested a review from Gozala July 17, 2023 14:50

vasco-santos commented Jul 17, 2023

View reviewed changes

Gozala mentioned this pull request Jul 18, 2023

Add PaddedSize.fromHeight function storacha/data-segment#16

Closed

Gozala approved these changes Jul 18, 2023

View reviewed changes

vasco-santos added 3 commits July 18, 2023 12:36

chore: address review comments

95bfaa1

chore: use new data segment lib

3736b2d

chore: use new data segment lib piece view type

11e68f0

vasco-santos force-pushed the fix/aggregation-spec-to-use-height-instead-of-size branch from 8103681 to 11e68f0 Compare July 25, 2023 16:45

vasco-santos merged commit 31730f0 into main Jul 25, 2023
14 checks passed

vasco-santos deleted the fix/aggregation-spec-to-use-height-instead-of-size branch July 25, 2023 17:35

This was referenced Jul 25, 2023

chore(main): release aggregate-client 2.0.0 #843

Closed

chore(main): release aggregate-api 2.0.0 #842

Closed

chore(main): release capabilities 8.0.0 #844

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vasco-santos commented Jul 17, 2023 •

edited

Loading

vasco-santos Jul 17, 2023

Gozala Jul 17, 2023

Gozala Jul 17, 2023

vasco-santos Jul 18, 2023

vasco-santos Jul 17, 2023

Gozala Jul 18, 2023

vasco-santos Jul 18, 2023

Gozala left a comment •

edited

Loading

Gozala Jul 17, 2023

Gozala Jul 17, 2023

Gozala Jul 17, 2023

Gozala Jul 17, 2023

Gozala Jul 17, 2023

Gozala Jul 17, 2023

Gozala Jul 18, 2023

vasco-santos Jul 18, 2023

Gozala Jul 18, 2023

vasco-santos Jul 18, 2023

Gozala Jul 18, 2023

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

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

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

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

Conversation

vasco-santos commented Jul 17, 2023 • edited Loading

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

Gozala left a comment • edited Loading

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

vasco-santos commented Jul 17, 2023 •

edited

Loading

Gozala left a comment •

edited

Loading