test(contract): sandbox coverage + stub verifier for async attestation - #3715
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
0e60532 to
47655a4
Compare
This comment was marked as outdated.
This comment was marked as outdated.
98a7a50 to
d3ecae2
Compare
|
Requirement carried over from review of #3714 (discussion_r3558779901): Add sandbox coverage for the TLS-key-ownership authorization guard in |
|
Note-to-self / tracking from #3714 (#3714 (comment)): the async-attestation gas constants in `crates/contract/src/config.rs` are not benchmarked — `DEFAULT_VERIFIER_TERA_GAS = 100` and `DEFAULT_RESOLVE_VERIFICATION_TERA_GAS = 60` are conservative estimates. I deferred measuring them to this PR since it adds the stub-verifier sandbox coverage. Proposal: in the `StubResponse::Verified` sandbox test, read the `resolve_verification` receipt's `total_gas_burnt` (per-receipt, via `ExecutionFinalResult::receipt_outcomes()`, same approach as `participants_gas.rs`) and size `resolve_verification_tera_gas` at ~2x the observed burn. Analytically it looks over-provisioned — the post-DCAP work is the same weight class as `clean_tee_status`/`clean_invalid_attestations` (10 Tgas), so ~20 is likely the right ceiling. Two caveats:
If it's not trivial here, fine to spin off a follow-up issue (relatedly, other unbenchmarked constants exist, e.g. `VOTE_FOREIGN_CHAIN_GAS` in the e2e tests — #3641 discussion_r3543560094). |
bc1f255 to
7134f67
Compare
|
PR title type suggestion: This PR modifies source code in Suggested title: |
|
JFYI, I created a follow-up issue #3787 for these two comments, since they’re both blocked on the same missing fixture: |
54d794f to
c3ab60b
Compare
6afae04 to
b21cc1e
Compare
|
@kevindeforth @gilcu3 @netrome, I’ve addressed all the nits. Feel free to take another look. Sorry for the force-pushes. The base PR, #3714, tends to run into conflicts with Hopefully, we can merge everything into |
|
|
||
| // Then | ||
| // assert_matches! requires Debug, which PromiseOrValue doesn't implement | ||
| assert!(matches!(result, PromiseOrValue::Promise(_))); |
There was a problem hiding this comment.
nit: we generally prefer assert_matches
| #[expect(clippy::large_enum_variant)] | ||
| #[derive(Debug, Default, Clone, Serialize, Deserialize, BorshDeserialize, BorshSerialize)] | ||
| #[derive( | ||
| Debug, Default, Clone, PartialEq, Eq, Serialize, Deserialize, BorshDeserialize, BorshSerialize, | ||
| )] | ||
| #[cfg_attr( |
There was a problem hiding this comment.
I saw this comment saying some traits were removed, but here I still see 2 included. Were this needed then?
barakeinav1
left a comment
There was a problem hiding this comment.
added a few comments
| let dtos::Attestation::Dstack(dstack) = &mut attestation else { | ||
| panic!("fixture must be a Dstack attestation"); | ||
| }; | ||
| dstack.quote = dtos::HexVec(vec![0u8; 16]); |
There was a problem hiding this comment.
This is where the real-DCAP gas cost slips through: the quote here is 16 zero bytes, which dcap_qvl rejects at parse, so verify_quote returns almost immediately and costs next to nothing. A valid quote runs the full verification (sig + cert chain + TCB), which is far heavier — and in an e2e run on a real TDX cluster that path blows past the verifier_tera_gas = 100 default (see #3714), so attestation never completes.
Since no test here feeds a valid quote, the gas budget isn't actually covered. Might be worth a case with a real quote fixture at production gas, asserting a stored attestation.
There was a problem hiding this comment.
@pbeza for reference, there's already a test that runs a real valid quote through verify_quote and gets Verified: crates/tee-verifier/tests/verify_quote.rs::verify_quote__should_return_verified_td10_report_for_valid_fixture. It uses the real test_utils::attestation::quote() + collateral() fixtures and pins the VM block timestamp to VALID_ATTESTATION_TIMESTAMP so the collateral is in its validity window.
That's the fixture setup this sandbox coverage could borrow to drive the full DCAP path (and its real gas cost) instead of the 16-byte quote that's rejected at parse. One catch is the clock: that test pins it via testing_env!; in a near-workspaces sandbox you'd need the block time inside the fixture's window (time fast-forward) for it to verify rather than expire.
barakeinav1
left a comment
There was a problem hiding this comment.
need to add a test that check the real gas used by the contract (since this just failed in an E2E test).
see #3715 (comment)
…o 3642-async-attestation-tests # Conflicts: # crates/tee-context/Cargo.toml
|
PR title type suggestion: This PR modifies source code files in addition to test files. When source code changes alongside test changes, the type should typically be something other than Suggested title: |
…o 3642-async-attestation-tests # Conflicts: # crates/tee-context/Cargo.toml
barakeinav1
left a comment
There was a problem hiding this comment.
I opened a follow-up issue.
#3897
|
Second half of the #3664 split, stacked on #3714 (async attestation). Tests only; no production-code changes.