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.

Sign up for GitHub

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: role.fromIndex should be +ve integer #161

Merged
merged 2 commits into from
Feb 19, 2024
Merged

fix: role.fromIndex should be +ve integer #161

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3242afa
fix: role.fromIndex should be +ve integer
gmaclennan Nov 20, 2023
089a81c
use uint in role.fromIndex proto
gmaclennan Feb 15, 2024
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
3 changes: 2 additions & 1 deletion schema/role/v1.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@
"description": "Unique identifier for role assigned to device with auth core ID equal to `docId` of this record"
},
"fromIndex": {
"type": "number",
"type": "integer",
"minimum": 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should update the proto to use uint32 instead...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, makes sense. It increases the max supported number of records in a hypercore. Is 2^32 records per hypercore per device enough? since this currently refers to the index of the auth cores, that seems enough, but I've also been thinking of adding a fromIndex for other cores, so that peers could choose not to index a particular peer beyond fromIndex. I can see it's feasible that a blob core could have > 2^32 records, because blobs are chunked into 64kb. So maybe just make them all uint64?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know enough to make a decision here.

If each chunk is exactly 64 kilobytes, then I think uint32 is fine. 64 bytes × 1000 × 232 = 274877906944000 bytes = = ~275 terabytes.1

If chunks could be 1 byte, then that only allows about 4.3 gigabytes, which feels a little restrictive, and we might want to use uint64.

We should keep in mind that 64-bit integers don't fit in JavaScript numbers. It looks like ts-proto uses a custom Long class by default, but we can make it use bigints with --ts_proto_opt=forceLong=bigint. Not sure if that affects anything else, such as JSON serialization.

In any case, I think this is best done in a followup.

Footnotes

  1. This assumes a kilobyte is 1000 bytes instead of 1024, which is more conservative in this case. ↩

Copy link
Member Author

Choose a reason for hiding this comment

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

ok also sanity-checking number of records, OSM currently has about 8 billion nodes and 1 billion ways. With a uint32 we would be limited to about 4 billion records in a single hypercore, so it wouldn't be possible to import the entire OSM db into a single hypercore, but that would probably be a bad idea anyway. Anyway this is just a limit for evaluating capabilities, only really relevant with self-created data, so I think the limits are fine.

"description": "This is the index of the auth core that this role applies to (identified by the `docId`) to apply this role from. E.g. documents in the auth core assigned this role from this index will be evaluated according to this role."
}
},
Expand Down