-
Notifications
You must be signed in to change notification settings - Fork 12
feat: support sharded canister ranges certificate structure #461
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
base: main
Are you sure you want to change the base?
Conversation
- Add support for new sharded canister ranges at /canister_ranges/<shard> - Maintain backwards compatibility with old structure at /subnet/<subnet_id>/canister_ranges - Update certificate builder to use new sharded structure for tests - Handle empty canister ranges gracefully for local development - Bump version to 3.0.4 This implementation follows the IC interface spec and ic-agent pattern for the new certificate structure introduced in newer IC versions, ensuring compatibility while maintaining support for existing certificates.
1689dad to
c3c317d
Compare
… compatibility @dfinity/agent 2.4.1 doesn't support the new sharded canister ranges structure yet. The Rust verification code already supports both old and new formats, so reverting the test certificates to use the old format allows all tests to pass while still supporting production certificates with the new format.
packages/ic-certificate-verification/src/certificate_verification.rs
Outdated
Show resolved
Hide resolved
packages/ic-certificate-verification/src/certificate_verification.rs
Outdated
Show resolved
Hide resolved
| ) -> CertificationTestResult<LabeledTree<Vec<u8>>> { | ||
| let canister_ranges = serialize_to_cbor(&canister_ranges.to_vec()); | ||
|
|
||
| // Use old structure for tests until @dfinity/agent supports the new sharded format |
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 don't understand this comment: the new agent supports the new sharded format. That's why we need to make this PR. What am I missing?
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.
This is not correct. The old format work with the ts integration test and new format doesn't. I'll still looking into it.
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.
Probably, we just need to update agent-js: https://github.com/dfinity/response-verification/blob/main/package.json#L17-L20
Update certificate verification to be strict about /canister_ranges root existence (Absent returns error) but lenient about Unknown shards in partial certificate views. This matches main branch behavior while supporting the new sharded structure from dfx 0.30.2. - Error on Absent /canister_ranges root (strict requirement) - Accept Unknown shards (for partial certificate views in WASM/JS) - Update test certificate builder to match dfx 0.30.2 structure - All unit and e2e tests pass
Look up /canister_ranges/<delegation.subnet_id> directly instead of iterating through all subnet IDs. This is more efficient and correct, matching the behavior of the old structure where we looked up a specific subnet's ranges. - Use delegation.subnet_id in the lookup path - Only iterate through range keys for that specific subnet - All integration tests pass
- Update @dfinity/agent, @dfinity/candid, @dfinity/principal to 3.4.3 - Certificate builder generates new format: /canister_ranges/<subnet_id>/<range_key> - Note: JS tests will fail until agent supports new sharded structure
- Replace compare with uint8Equals - Replace Certificate.lookup() with lookup_path() and lookupResultToBuffer() - Convert ArrayBuffer types to Uint8Array where needed - Update HashTree node type annotations - Build passes but tests fail (agent doesn't support new cert structure yet)
- Replace compare with uint8Equals - Use lookup_path() and lookupResultToBuffer() instead of direct lookup - Handle agent.rootKey null check - Update hashUInt32 to return Uint8Array instead of ArrayBuffer
Update @dfinity/pic from v0.12.0 to v0.18.0 for compatibility with @dfinity/agent, @dfinity/candid, and @dfinity/principal v3.4.3
Import IDL from @icp-sdk/core and cast idlFactory to the correct type IDL.InterfaceFactory that @dfinity/pic@0.18.0 expects, instead of using 'any'. This provides better type safety while maintaining compatibility between @dfinity/candid and @icp-sdk/core candid type definitions. Also revert package.json to use @dfinity/pic@0.18.0.
afd4a39 to
7fdd78f
Compare
@icp-sdk/core@5.0.0 is needed to import IDL.InterfaceFactory type for compatibility with @dfinity/pic@0.18.0
@dfinity/pic@0.18.0 uses @icp-sdk/core with incompatible candid type definitions. The 'as any' cast avoids TypeScript errors while maintaining runtime compatibility. This is necessary because: - Generated declarations use @dfinity/candid types - @dfinity/pic expects @icp-sdk/core types - The types are functionally equivalent at runtime - Modern module resolution would be needed to import from @icp-sdk/core
Add type casts to resolve Principal type incompatibilities between @dfinity/principal@3.4.3 and @icp-sdk/core@5.0.0 (bundled with pic). - Cast fixture.canisterId from @icp-sdk/core to @dfinity/principal - Cast canisterId back to any when passing to pic.getCyclesBalance() This affects test files in: - assets - custom-assets - skip-certification - json-api
Revert all TypeScript and WASM package updates to keep this PR focused on the Rust certificate verification changes only. This reverts: - @dfinity/agent, @dfinity/candid, @dfinity/principal updates - @dfinity/pic update - @icp-sdk/core dependency addition - certificate-verification-js API compatibility changes - certified-counter frontend API changes - All test file Principal type casting fixes The Rust changes for sharded canister ranges support are preserved.
Add support for both new and old certificate canister range structures: - Primary: /canister_ranges/<subnet_id>/<range_key> (new format) - Fallback: /subnet/<subnet_id>/canister_ranges (old format) This ensures compatibility with existing JS/WASM integration tests that use @dfinity/agent@1.0.1, which generates certificates in the old format. The new format is tried first, and if absent, falls back to the old format.
Add handling for LookupResult::Error case when looking up old certificate format. Treat Error the same as Absent - return CanisterRangesNotFound error since neither format could be found.
Change test certificate builder to generate certificates in the old format (/subnet/<subnet_id>/canister_ranges) for backward compatibility with @dfinity/agent@1.0.1 used in JS tests. The Rust code supports both old and new formats via fallback logic, but the old JS agent only supports the old format. This ensures all tests pass: - Rust tests: Pass (reads old format via fallback) - JS tests: Pass (old agent can read old format) - Real IC: Uses new format (Rust code reads via primary path)
Renamed functions to reflect that new format is the standard: - create_delegation_tree() now creates new format (was create_delegation_tree_new_format) - create_delegation_tree_old_format() creates old format (was create_delegation_tree) Updated CertificateBuilder: - New format is now the default (use_new_certificate_format: true) - Added with_old_certificate_format() method for explicit old format - Removed with_new_certificate_format() method (no longer needed) Test changes: - Rust tests use new format by default (no method call needed) - WASM builder explicitly uses old format for JS compatibility This ensures: - New format is the default everywhere (future-proof) - Old format is explicitly opt-in (backward compatibility) - Clear naming convention aligns with code strategy
Changed the boolean flag from use_new_certificate_format to use_old_certificate_format for better clarity: - Default is false (use new format) - Set to true to use old format - Inverted all logic accordingly This is more intuitive since: - New format is the standard/default - We track the exception (old format) not the rule - Aligns with method name: with_old_certificate_format()
Combined SubtreeLookupResult::Absent and ::Unknown cases to both fall back to the old certificate format. The separate Unknown handling was not needed because: - Our WASM/JS tests use old format (Absent case) - If new format is pruned (Unknown), we should also try old format - No evidence of pruned new format certificates in WASM/JS yet This simplifies the logic while maintaining the same behavior: - New format found -> use it - New format absent/unknown -> try old format - Both missing -> error All tests pass (Rust + WASM/JS integration tests).
Replace explicit enumeration of all non-Found cases with a wildcard pattern. This is cleaner and more maintainable: - Found -> parse and return - Anything else -> error Same behavior, simpler code.
| ); | ||
| let canister_id = Principal::from_slice(canister_id); | ||
|
|
||
| // Try new structure first: /canister_ranges/<subnet_id>/<range_key> |
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.
Let's add a comment here that we only fallback to the old structure because of JS and that once we bump agent-js, we can get rid of the old structure.
| LookupResult::Found(old_range_data) => parse_cbor_principals_array(old_range_data)?, | ||
| LookupResult::Absent | LookupResult::Error => { | ||
| // Neither format found - this is an error | ||
| return Err(CertificateVerificationError::CanisterRangesNotFound { |
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 see you introduced a new error: CanisterRangesNotFound. Previously, we used SubnetCanisterIdRangesNotFound. It doesn't really matter, but maybe we can remove one or the other such that we only have one.
| let canister_ranges: Vec<(Principal, Principal)> = match cert | ||
| .tree | ||
| .lookup_subtree(&canister_ranges_path) | ||
| { | ||
| SubtreeLookupResult::Found(subnet_tree) => { | ||
| // Collect all ranges from all range keys under this subnet | ||
| let mut ranges = Vec::new(); | ||
| let range_keys = subnet_tree.list_paths(); | ||
|
|
||
| for range_key_path in range_keys { | ||
| if !range_key_path.is_empty() { | ||
| if let LookupResult::Found(range_data) = | ||
| subnet_tree.lookup_path([range_key_path[0].as_bytes()]) | ||
| { | ||
| let subnet_ranges: Vec<(Principal, Principal)> = | ||
| parse_cbor_principals_array(range_data)?; | ||
| ranges.extend(subnet_ranges); | ||
| } | ||
| } | ||
| } | ||
| ranges | ||
| } | ||
| SubtreeLookupResult::Absent | SubtreeLookupResult::Unknown => { | ||
| // New structure not found, try old structure: /subnet/<subnet_id>/canister_ranges | ||
| let old_canister_ranges_path = [ | ||
| "subnet".as_bytes(), | ||
| delegation.subnet_id.as_ref(), | ||
| "canister_ranges".as_bytes(), | ||
| ]; | ||
| match cert.tree.lookup_path(&old_canister_ranges_path) { | ||
| LookupResult::Found(old_range_data) => parse_cbor_principals_array(old_range_data)?, | ||
| LookupResult::Absent | LookupResult::Error => { | ||
| // Neither format found - this is an error | ||
| return Err(CertificateVerificationError::CanisterRangesNotFound { | ||
| path: canister_ranges_path.iter().map(|p| p.to_vec()).collect(), | ||
| }); | ||
| } | ||
| LookupResult::Unknown => Vec::new(), | ||
| } | ||
| } |
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.
It would be nice to make use of functional iterators (more rust). something like:
let canister_ranges: Vec<(Principal, Principal)> = match cert.tree.lookup_subtree(&canister_ranges_path) {
SubtreeLookupResult::Found(subnet_tree) => {
// Use an iterator to transform paths into parsed data
subnet_tree
.list_paths()
.into_iter()
.filter_map(|path| {
let key = path.first()?;
if let LookupResult::Found(data) = subnet_tree.lookup_path([key.as_bytes()]) {
Some(parse_cbor_principals_array(data))
} else {
None
}
})
.collect::<Result<Vec<Vec<_>>, _>>()?
.into_iter()
.flatten()
.collect()
}
SubtreeLookupResult::Absent | SubtreeLookupResult::Unknown => {
let old_path = [
b"subnet".as_slice(),
delegation.subnet_id.as_ref(),
b"canister_ranges".as_slice(),
];
match cert.tree.lookup_path(&old_path) {
LookupResult::Found(data) => parse_cbor_principals_array(data)?,
LookupResult::Unknown => Vec::new(),
LookupResult::Absent | LookupResult::Error => {
return Err(CertificateVerificationError::CanisterRangesNotFound {
path: canister_ranges_path.iter().map(|p| p.to_vec()).collect(),
});
}
}
}
};
Summary
Adds support for the new sharded canister ranges certificate structure introduced in newer IC versions, while maintaining backwards compatibility with the old structure.
Changes
/canister_ranges/<subnet_id>/subnet/<subnet_id>/canister_rangesImplementation Details
This follows the IC interface spec and ic-agent pattern for certificate verification.
The implementation:
/canister_ranges/<subnet_id>/subnet/<subnet_id>/canister_rangesif the new structure doesn't yield any rangesTesting
Related Issues
Resolves compatibility issues with newer IC versions that use the sharded certificate structure.