perf(contract): contract should not store full attestation submission #1663
Conversation
| /// The digest of the MPC image running. | ||
| pub launcher_compose_hash: [u8; 32], | ||
| /// Unix timestamp for when the attestation was created. | ||
| pub creation_time_stamp_seonds: u64, |
There was a problem hiding this comment.
where do we get the time from?
There was a problem hiding this comment.
The comment is s currently misleading. The timestamp is taken from "now" time argument during the initial verification, so it's the timestamp of when the contract first saw the attestation and not the creation date.
In a follow up PR we should refactor this internal implementation to use the timestamp embedded in the attestation instead.
barakeinav1
left a comment
barakeinav1
left a comment
There was a problem hiding this comment.
overall look good,
I requested some changes (mainly nits, typo name etc..) ,
I also think you should add another test: (I didn't see something like it)
#[test]
fn re_verify_fails_after_allowed_hash_is_cleaned_up() {
// 1. Add allowed MPC hash H1 at time T0
// 2. Add participant attested with H1
// 3. Add new MPC hash H2 (upgrade)
// 4. Advance time > tee_upgrade_deadline_duration
// 5. cleanup_expired_hashes removes H1
// 6. re_verify_tee_participant fails
}
barakeinav1
left a comment
barakeinav1
left a comment
There was a problem hiding this comment.
approving with some comments to be address
We already have integration tests in place. |
crates/contract/src/tee/tee_state.rs
Outdated
| participants_attestations: IterableMap::new(StorageKey::TeeParticipantAttestation), | ||
| } | ||
| } | ||
| pub(crate) participants_attestations: BTreeMap<near_sdk::PublicKey, NodeAttestation>, |
There was a problem hiding this comment.
Changed type to BtreeMap as upgrade tests were running out of gas (even with max_gas) when keeping it as an IterableMap
| "expiry_time_stamp_seconds": { | ||
| "description": "Unix time stamp for when this attestation expires.", | ||
| "expiry_timestamp_seconds": { | ||
| "description": "Unix time stamp for when this attestation will be expired.", |
There was a problem hiding this comment.
nit: the previous sense sounds more natural to me
| /// optimization (#1617) by avoiding full contract code deserialization; there’s likely still | ||
| /// room for further optimization. | ||
| pub const GAS_FOR_VOTE_UPDATE: Gas = Gas::from_tgas(232); | ||
| pub const GAS_FOR_VOTE_UPDATE: Gas = Gas::from_tgas(241); |
There was a problem hiding this comment.
do you remember why this bump was needed? We continue approaching the limit, although this cannot be completely solved until #1674 is solved
There was a problem hiding this comment.
Was needed due to the change of using BtreeMap isntead of IterableMap IIRC
netrome
left a comment
netrome
left a comment
There was a problem hiding this comment.
A few blockers. We should understand better what's costing so much gas. Beyond this, we need to figure out how to introduce this change in a compatible way.
| "VerifiedAttestation": { | ||
| "oneOf": [ | ||
| { | ||
| "type": "object", | ||
| "required": [ | ||
| "Dtack" | ||
| ], | ||
| "properties": { | ||
| "Dtack": { | ||
| "$ref": "#/definitions/VerifiedDstackAttestation" | ||
| } | ||
| }, | ||
| "additionalProperties": false | ||
| }, | ||
| { | ||
| "type": "object", | ||
| "required": [ | ||
| "Mock" | ||
| ], | ||
| "properties": { | ||
| "Mock": { | ||
| "$ref": "#/definitions/MockAttestation" | ||
| } | ||
| }, | ||
| "additionalProperties": false | ||
| } | ||
| ] | ||
| }, | ||
| "VerifiedDstackAttestation": { | ||
| "type": "object", | ||
| "required": [ | ||
| "expiry_timestamp_seconds", | ||
| "launcher_compose_hash", | ||
| "mpc_image_hash" | ||
| ], | ||
| "properties": { | ||
| "expiry_timestamp_seconds": { | ||
| "description": "Unix time stamp for when this attestation will be expired.", | ||
| "type": "integer", | ||
| "format": "uint64", | ||
| "minimum": 0.0 | ||
| }, | ||
| "launcher_compose_hash": { | ||
| "description": "The digest of the launcher compose file running.", | ||
| "type": "array", | ||
| "items": { | ||
| "type": "integer", | ||
| "format": "uint8", | ||
| "minimum": 0.0 | ||
| }, | ||
| "maxItems": 32, | ||
| "minItems": 32 | ||
| }, | ||
| "mpc_image_hash": { | ||
| "description": "The digest of the MPC image running.", | ||
| "type": "array", | ||
| "items": { | ||
| "type": "integer", | ||
| "format": "uint8", | ||
| "minimum": 0.0 | ||
| }, | ||
| "maxItems": 32, | ||
| "minItems": 32 | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Given that we change the interface here, I'm curious about compatibility guarantees. We've discussed this synchronously now and it seems like this would cause nodes to fail to observe attestations in previous contract versions.
We need to figure out how to make this change backwards compatible to ensure nodes can submit and observe attestations in the old contract.
There was a problem hiding this comment.
It's backwards compatible with current mock attestations, but not with dstack. Given that we don't store any dstack attestations, and don't intend to have node submit dstack attestations before this change it should be fine.
|
Carved out flaky test fix into #1695 |
DSharifi
force-pushed
the
1629-contract-should-not-store-full-attestation-submission
branch
from
55f3aec to
d84382b
Compare
DSharifi
force-pushed
the
1629-contract-should-not-store-full-attestation-submission
branch
from
669c61c to
ee42690
Compare
closes #1629