feat(tee): clean TEE state when concluding a resharing #942
Conversation
netrome
left a comment
There was a problem hiding this comment.
It seems to me that we're doing the cleanup in the wrong place. Moreover, while there are good unit tests for the cleanup functions I don't see a test for the vote_pk logic. I.e. if the contract is in resharing, and vote_pk is called with the final vote, it cleans up the stale attestations and enters the running state.
DSharifi
left a comment
There was a problem hiding this comment.
I'd like to see an integration test that tests the whole flow E2E. I.e. in the itntegration test you start a resharing, and make sure that the old participants that are not in the new running state are not in the TEE state.
…te-when-concluding-a-resharing
Address code review comment: The vote_pk method was returning `InvalidState::ProtocolStateNotResharing` when the protocol state was not `Initializing`, which is misleading. This method expects the protocol to be in `Initializing` state (as documented in the function comment), so it should return `ProtocolStateNotInitializing` error instead.
…te-when-concluding-a-resharing
There was a problem hiding this comment.
Pull Request Overview
This PR implements TEE (Trusted Execution Environment) state cleanup functionality when concluding a resharing operation. The changes ensure that only current participants retain their TEE attestations after resharing completes, preventing stale participant data from accumulating.
- Adds automatic TEE cleanup when
vote_resharedtransitions back to running state - Implements a private
clean_tee_statusmethod that removes TEE data for non-participants - Adds comprehensive test coverage for both unit and integration scenarios
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
libs/chain-signatures/contract/src/lib.rs |
Adds TEE cleanup promise after resharing and implements the private cleanup method |
libs/chain-signatures/contract/src/tee/tee_state.rs |
Implements core TEE cleanup logic and helper methods with unit tests |
libs/chain-signatures/contract/src/state/mod.rs |
Fixes error type for vote_pk method validation |
libs/chain-signatures/contract/tests/common.rs |
Adds shared helper functions for TEE testing |
libs/chain-signatures/contract/tests/tee.rs |
Adds unit tests for TEE cleanup access control and functionality |
libs/chain-signatures/contract/tests/integration_tee_cleanup_after_resharing.rs |
Adds comprehensive integration test for the complete resharing flow |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
libs/chain-signatures/contract/tests/integration_tee_cleanup_after_resharing.rs
Show resolved
Hide resolved
netrome
left a comment
There was a problem hiding this comment.
Thank you for updating 🙏
| /// Private endpoint to clean up TEE information for non-participants after resharing. | ||
| /// This can only be called by the contract itself via a promise. | ||
| #[private] | ||
| pub fn clean_tee_status(&mut self) { | ||
| match self { | ||
| Self::V2(contract) => contract.clean_tee_status(), | ||
| _ => env::panic_str("expected V2"), | ||
| } | ||
| } |
There was a problem hiding this comment.
I know this is what was specified in the issue, but I'm a bit curious why we have a separate endpoint for this and spawn a promise rather than just calling this inline. What are the benefits we get from this approach?
There was a problem hiding this comment.
We avoid a failing vote_reshared due to insufficient gas.
There was a problem hiding this comment.
Makes sense. So what happens if we run out of gas during the cleanup? We'll just leave the information lying around until the next resharing?
There was a problem hiding this comment.
So what happens if we run out of gas during the cleanup? We'll just leave the information lying around until the next resharing?
Yes. The only reason for the call to fail would be if we attached insufficient gas. Since that is a hard-coded constant, any subsequent resharing won't succeed with high probability. But we would have time to push a patch with adjusted gas and it's not the end of the world.
In any case, I would hope we get much better at correctly estimating and tracking gas costs. But it's not trivial and AFAIK, we would first need to build some sort of bench-marking framework to do so effectively.
There was a problem hiding this comment.
We avoid a failing
vote_reshareddue to insufficient gas.
I'm also wondering how high of a risk it is that the contract call fails due to insufficient gas because of the cleanup, and if it's worth the complexity. Is this a recommended pattern for smart contracts on NEAR?
kevindeforth
left a comment
There was a problem hiding this comment.
Good work!
Just a few remarks:
- method naming
- please add the new api endpoints to the contract readme
- can you share some gas usage observations for the cleanup method? How did we arrive at 5Tgas?
- move the promise creation to the higher-level
VersionedMpcContract - lets not make the result of
vote_reshareddepend on the cleanup outcome.
Co-authored-by: kevindeforth <32777623+kevindeforth@users.noreply.github.com>
…te-when-concluding-a-resharing
Taking the liberty to merge since Daniel is OoO today. I’ve addressed his review comments and already got 2 extra approvals.
Fixes #844