From ea25292271c92d57a5392d1a64b91437a8afb20d Mon Sep 17 00:00:00 2001 From: Kiyoshi Nakao Date: Fri, 12 Jun 2026 15:57:43 +0900 Subject: [PATCH] enclave-api: bind speculative base by state_id instead of byte equality The canonical store's Any encoding is not stable across writers: create_client stores the submitter's encoding verbatim while update_client stores the enclave light client's re-encoded form (e.g. JSON-embedded config fields re-serialized). A base rebuilt by the relayer therefore cannot reproduce the stored bytes in general, so the stored/supplied byte comparisons deterministically rejected every batch whose base was not a verbatim round-trip of the store (first hit at the second batch of an initial sync, where the base is rebuilt rather than queried). Replace the client_state/consensus_state byte checks with the encoding-independent binding that already closes the chain: the enclave-observed prev_state_id must match the state_id recorded at prev_height. Keep the stale-base protection the latest-client_state byte check provided by tracking a host-managed speculative-commit high-water mark and rejecting bases below it. --- modules/enclave-api/src/enclave.rs | 161 +++++++++++------- modules/service/src/speculative/service.rs | 156 ++++++++++------- modules/service/src/speculative/validation.rs | 21 ++- modules/types/src/store_key.rs | 11 ++ 4 files changed, 221 insertions(+), 128 deletions(-) diff --git a/modules/enclave-api/src/enclave.rs b/modules/enclave-api/src/enclave.rs index f4c42eee..0cc7064b 100644 --- a/modules/enclave-api/src/enclave.rs +++ b/modules/enclave-api/src/enclave.rs @@ -1,6 +1,6 @@ use crate::errors::{Error, Result}; use keymanager::EnclaveKeyManager; -use lcp_types::{store_key, Any, EnclaveMetadata, Height}; +use lcp_types::{store_key, EnclaveMetadata, Height}; use sgx_types::{sgx_enclave_id_t, SgxResult}; use sgx_urts::SgxEnclave; use std::path::PathBuf; @@ -128,24 +128,34 @@ pub trait HostStoreTxManager: CommitStoreAccessor { } /// `apply_write_set_with_expected_base` applies a speculative write set only if the - /// store already contains the explicit base state that seeded the batch at - /// `prev_height`. + /// canonical chain recorded at `prev_height` matches the explicit base state that + /// seeded the batch. /// /// The check and apply run under the same serialized update transaction keyed by /// `update_key`, so the accepted base cannot change between verification and commit. - /// The explicit base client state must match the latest canonical - /// client_state. This prevents an old, historically valid base state from - /// overwriting a newer latest-only client_state. The caller-supplied - /// `prev_state_id` (observed in-enclave by the first speculative unit) - /// must also match the height-indexed state ID previously stored by a - /// successful create/serial/speculative update. + /// + /// The base is bound by content, not by bytes. The canonical store's Any + /// encoding is not stable across writers: create_client stores the + /// submitter's encoding verbatim, while update_client stores the enclave + /// light client's re-encoded form (which may, for example, re-serialize + /// JSON-embedded config fields). A base rebuilt by the relayer therefore + /// cannot reproduce the stored bytes in general, so byte equality is not a + /// usable invariant here. Instead, the caller-supplied `prev_state_id` + /// (observed in-enclave by the first speculative unit over the supplied + /// base) must match the height-indexed state ID previously written by a + /// successful create/serial/speculative update; state IDs are computed + /// in-enclave over the decoded states, so they are encoding-independent. + /// + /// To keep an old, historically valid base from silently rewinding the + /// host-store cache, `prev_height` must also be at or above the + /// speculative-commit high-water mark recorded by previous calls; the mark + /// advances to `post_height` on every successful apply. fn apply_write_set_with_expected_base( &self, update_key: UpdateKey, prev_height: Height, - client_state: &Any, - consensus_state: &Any, prev_state_id: Option<&[u8]>, + post_height: Height, write_set: WriteSet, ) -> Result<()> where @@ -153,14 +163,9 @@ pub trait HostStoreTxManager: CommitStoreAccessor { { let tx = self.begin_tx(Some(update_key.clone()))?; let tx_id = tx.get_id(); - if let Err(e) = self.verify_expected_base_state_in_tx( - tx_id, - &update_key, - &prev_height, - client_state, - consensus_state, - prev_state_id, - ) { + if let Err(e) = + self.verify_expected_base_state_in_tx(tx_id, &update_key, &prev_height, prev_state_id) + { self.rollback_tx(tx); return Err(e); } @@ -168,6 +173,12 @@ pub trait HostStoreTxManager: CommitStoreAccessor { self.rollback_tx(tx); return Err(e); } + if let Err(e) = + self.advance_speculative_commit_height_in_tx(tx_id, &update_key, post_height) + { + self.rollback_tx(tx); + return Err(e); + } self.commit_tx(tx) } @@ -189,41 +200,27 @@ pub trait HostStoreTxManager: CommitStoreAccessor { tx_id: store::TxId, client_id: &str, prev_height: &Height, - client_state: &Any, - consensus_state: &Any, prev_state_id: Option<&[u8]>, ) -> Result<()> where S: TxAccessor, { - let client_state_key = store_key::client_state_bytes(client_id); - let client_state_value = - bincode::serde::encode_to_vec(client_state, bincode::config::standard()) - .map_err(Error::bincode_encode)?; - let canonical_client_state = - self.use_mut_store(|store| store.tx_get(tx_id, &client_state_key))?; - if canonical_client_state.as_deref() != Some(client_state_value.as_slice()) { - return Err(Error::invalid_argument(format!( - "stored speculative base client_state mismatch: client_id={} height={}-{}", - client_id, - prev_height.revision_number(), - prev_height.revision_height() - ))); - } - - let consensus_state_key = store_key::consensus_state_bytes(client_id, prev_height); - let consensus_state_value = - bincode::serde::encode_to_vec(consensus_state, bincode::config::standard()) - .map_err(Error::bincode_encode)?; - let canonical_consensus_state = - self.use_mut_store(|store| store.tx_get(tx_id, &consensus_state_key))?; - if canonical_consensus_state.as_deref() != Some(consensus_state_value.as_slice()) { - return Err(Error::invalid_argument(format!( - "stored speculative base consensus_state mismatch: client_id={} height={}-{}", - client_id, - prev_height.revision_number(), - prev_height.revision_height() - ))); + // Reject bases older than the speculative-commit high-water mark so a + // stale batch cannot rewind state committed by a previous batch. The + // mark only tracks speculative commits: a serial update_client that + // advances the client further does not move it, so a base taken from + // the post-serial canonical state still passes this check. + if let Some(commit_height) = self.get_speculative_commit_height_in_tx(tx_id, client_id)? { + if *prev_height < commit_height { + return Err(Error::invalid_argument(format!( + "stale speculative base height: client_id={} height={}-{} last_committed={}-{}", + client_id, + prev_height.revision_number(), + prev_height.revision_height(), + commit_height.revision_number(), + commit_height.revision_height() + ))); + } } let prev_state_id = prev_state_id.ok_or_else(|| { @@ -234,14 +231,17 @@ pub trait HostStoreTxManager: CommitStoreAccessor { prev_height.revision_height() )) })?; - // Do not recompute the state ID from the supplied raw Anys here: light - // clients derive state IDs from a canonicalized client state (e.g. - // latest_height/frozen reset), and that canonicalization is - // ELC-specific and only available inside the enclave. The supplied - // base bytes are already pinned to the canonical store by the two - // checks above, and the stored state_id below was written by the - // in-enclave light client for exactly those bytes, so comparing the - // observed prev_state_id against the stored state_id closes the chain. + // Do not recompute the state ID from the supplied raw Anys, and do not + // compare the supplied bytes against the stored client/consensus + // states: light clients derive state IDs from decoded, canonicalized + // states (canonicalization is ELC-specific and only available inside + // the enclave), and the stored Any bytes are not a canonical encoding + // (create_client stores the submitter's encoding, update_client the + // enclave's re-encoded form). The stored state_id below was written by + // the in-enclave light client for the canonical state at prev_height, + // and the observed prev_state_id was computed in-enclave over the + // supplied base, so comparing the two binds the supplied base content + // to the canonical chain independently of byte encodings. let state_id_key = store_key::state_id_bytes(client_id, prev_height); let stored_state_id = self.use_mut_store(|store| store.tx_get(tx_id, &state_id_key))?; // Clients created before state_id tracking have no stored entry at @@ -266,6 +266,53 @@ pub trait HostStoreTxManager: CommitStoreAccessor { Ok(()) } + /// `get_speculative_commit_height_in_tx` reads the speculative-commit + /// high-water mark for `client_id`, i.e. the highest `post_height` applied + /// through `apply_write_set_with_expected_base` so far. The record is + /// host-managed and host-encoded (bincode), so it is symmetric for both + /// the writer and the reader. + fn get_speculative_commit_height_in_tx( + &self, + tx_id: store::TxId, + client_id: &str, + ) -> Result> + where + S: TxAccessor, + { + let key = store_key::speculative_commit_height_bytes(client_id); + let value = self.use_mut_store(|store| store.tx_get(tx_id, &key))?; + value + .map(|v| { + bincode::serde::decode_from_slice::(&v, bincode::config::standard()) + .map(|(height, _)| height) + .map_err(Error::bincode_decode) + }) + .transpose() + } + + /// `advance_speculative_commit_height_in_tx` moves the speculative-commit + /// high-water mark forward to `post_height`; it never rewinds the mark. + fn advance_speculative_commit_height_in_tx( + &self, + tx_id: store::TxId, + client_id: &str, + post_height: Height, + ) -> Result<()> + where + S: TxAccessor, + { + if let Some(commit_height) = self.get_speculative_commit_height_in_tx(tx_id, client_id)? { + if post_height <= commit_height { + return Ok(()); + } + } + let key = store_key::speculative_commit_height_bytes(client_id); + let value = bincode::serde::encode_to_vec(post_height, bincode::config::standard()) + .map_err(Error::bincode_encode)?; + self.use_mut_store(|store| store.tx_set(tx_id, key, value))?; + Ok(()) + } + /// `rollback_tx` rollbacks the changes in the transaction fn rollback_tx(&self, tx: ::PreparedTx) { self.use_mut_store(|store| store.rollback(tx)); diff --git a/modules/service/src/speculative/service.rs b/modules/service/src/speculative/service.rs index a40279a5..acfefafd 100644 --- a/modules/service/src/speculative/service.rs +++ b/modules/service/src/speculative/service.rs @@ -164,13 +164,20 @@ impl SpeculativeService { observed_transition: result.observed_transition, }); } + let last_post_height = units + .last() + .map(|unit| unit.observed_transition.post_height) + .ok_or_else(|| SpeculativeBatchFailure { + kind: SpeculativeBatchFailureKind::BatchSizeMismatch, + unit_id: None, + detail: "speculative batch must contain at least one unit".to_string(), + })?; app.enclave .apply_write_set_with_expected_base( batch.client_id.clone(), first_base.prev_height, - &first_base.client_state, - &first_base.consensus_state, first_prev_state_id.as_deref(), + last_post_height, merged_write_set, ) .map_err(|e| SpeculativeBatchFailure { @@ -670,62 +677,6 @@ mod tests { assert_eq!(err.unit_id.as_deref(), Some("unit-0001")); } - #[test] - fn stitch_rejects_first_base_state_that_is_not_in_store() { - let client_id = "07-tendermint-0"; - let enclave = FakeEnclave::new(Duration::from_millis(1)); - let app = AppService::::new("test-home", enclave, 1); - let service = SpeculativeService::new(1); - let req = with_explicit_base_state_payload(mk_req( - "unit-0000", - client_id, - Some(Height::new(0, 10)), - None, - )); - set_canonical_client_state( - &app, - client_id, - req.base_state - .client_state - .as_ref() - .expect("test base client_state"), - ); - let result = SpeculativeUpdateClientResult { - response: MsgUpdateClientResponse::default(), - write_set: WriteSet::default(), - base_state: req.base_state.clone(), - observed_transition: ObservedStateTransition { - prev_height: Some(Height::new(0, 10)), - prev_state_id: None, - post_height: Height::new(0, 11), - post_state_id: vec![1; 32], - }, - }; - - let err = service - .stitch_speculative_update_client_batch( - &app, - SpeculativeUpdateClientBatch { - client_id: client_id.to_string(), - units: vec![req], - }, - SpeculativeUpdateClientBatchResult { - client_id: client_id.to_string(), - units: vec![result], - }, - ) - .expect_err("unknown first base consensus state must be rejected"); - - assert_eq!(err.kind, SpeculativeBatchFailureKind::BaseStateMismatch); - assert_eq!(err.unit_id.as_deref(), Some("unit-0000")); - assert!( - err.detail - .contains("stored speculative base consensus_state mismatch"), - "unexpected error detail: {}", - err.detail - ); - } - #[test] fn stitch_accepts_first_base_state_when_stored_consensus_and_state_id_match() { let client_id = "07-tendermint-0"; @@ -1078,8 +1029,15 @@ mod tests { ); } + // Regression test for the deterministic first-stitch failure on real ELC + // paths: the canonical store's Any encoding changes across writers + // (create_client stores the relayer's encoding verbatim; update_client + // stores the enclave light client's re-encoded form, e.g. with + // JSON-embedded config re-serialized), so a relayer-rebuilt base cannot + // reproduce the stored bytes. The stitch must bind the base via the + // stored state_id, not via byte equality with the stored Anys. #[test] - fn stitch_rejects_first_base_state_when_canonical_client_state_advanced() { + fn stitch_accepts_first_base_state_when_stored_bytes_use_different_encoding() { let client_id = "07-tendermint-0"; let enclave = FakeEnclave::new(Duration::from_millis(1)); let app = AppService::::new("test-home", enclave, 1); @@ -1093,7 +1051,9 @@ mod tests { let prev_height = req.base_state.prev_height.expect("test base prev_height"); let prev_state_id = state_id_for_base_state(&req.base_state); req.base_state.prev_state_id = Some(prev_state_id.clone()); - seed_canonical_base_state(&app, client_id, &req.base_state); + // The canonical store holds a different encoding of the base states + // than the supplied bytes; only the state_id recorded at prev_height + // matches the supplied base content. set_canonical_client_state( &app, client_id, @@ -1102,6 +1062,77 @@ mod tests { value: vec![42], }, ); + app.enclave.use_mut_store(|store| { + store.set( + lcp_types::store_key::state_id_bytes(client_id, &prev_height), + prev_state_id.clone(), + ); + }); + let post_height = Height::new(0, 11); + let result = SpeculativeUpdateClientResult { + response: MsgUpdateClientResponse::default(), + write_set: WriteSet::default(), + base_state: req.base_state.clone(), + observed_transition: ObservedStateTransition { + prev_height: Some(prev_height), + prev_state_id: Some(prev_state_id), + post_height, + post_state_id: vec![1; 32], + }, + }; + + service + .stitch_speculative_update_client_batch( + &app, + SpeculativeUpdateClientBatch { + client_id: client_id.to_string(), + units: vec![req], + }, + SpeculativeUpdateClientBatchResult { + client_id: client_id.to_string(), + units: vec![result], + }, + ) + .expect("base bytes differing from the stored encoding must be accepted when the state_id matches"); + + let recorded = app.enclave.use_mut_store(|store| { + store.get(&lcp_types::store_key::speculative_commit_height_bytes( + client_id, + )) + }); + let recorded = recorded.expect("speculative commit height must be recorded"); + let (recorded_height, _): (Height, usize) = + bincode::serde::decode_from_slice(&recorded, bincode::config::standard()) + .expect("decode speculative commit height"); + assert_eq!(recorded_height, post_height); + } + + #[test] + fn stitch_rejects_stale_first_base_below_speculative_commit_height() { + let client_id = "07-tendermint-0"; + let enclave = FakeEnclave::new(Duration::from_millis(1)); + let app = AppService::::new("test-home", enclave, 1); + let service = SpeculativeService::new(1); + let mut req = with_explicit_base_state_payload(mk_req( + "unit-0000", + client_id, + Some(Height::new(0, 10)), + None, + )); + let prev_height = req.base_state.prev_height.expect("test base prev_height"); + let prev_state_id = state_id_for_base_state(&req.base_state); + req.base_state.prev_state_id = Some(prev_state_id.clone()); + seed_canonical_base_state(&app, client_id, &req.base_state); + // A previous speculative batch already committed up to height 12, so + // a base at height 10 — even with a valid historical state_id — must + // not rewind the canonical store. + app.enclave.use_mut_store(|store| { + store.set( + lcp_types::store_key::speculative_commit_height_bytes(client_id), + bincode::serde::encode_to_vec(Height::new(0, 12), bincode::config::standard()) + .expect("encode speculative commit height"), + ); + }); let result = SpeculativeUpdateClientResult { response: MsgUpdateClientResponse::default(), write_set: WriteSet::default(), @@ -1126,13 +1157,12 @@ mod tests { units: vec![result], }, ) - .expect_err("stale base client_state should be rejected"); + .expect_err("stale base below the speculative commit height should be rejected"); assert_eq!(err.kind, SpeculativeBatchFailureKind::BaseStateMismatch); assert_eq!(err.unit_id.as_deref(), Some("unit-0000")); assert!( - err.detail - .contains("stored speculative base client_state mismatch"), + err.detail.contains("stale speculative base height"), "unexpected error detail: {}", err.detail ); diff --git a/modules/service/src/speculative/validation.rs b/modules/service/src/speculative/validation.rs index ff870541..e27ef746 100644 --- a/modules/service/src/speculative/validation.rs +++ b/modules/service/src/speculative/validation.rs @@ -103,14 +103,19 @@ pub(crate) fn validate_linear_transitions( // must report the previous unit's post state as its own base state before the // batch can be stitched into one canonical write set. // -// Binding scope: only the first unit's base bytes are pinned byte-for-byte -// against the canonical store (`verify_expected_base_state_in_tx`). Later -// units are bound to their predecessor solely through the canonicalized -// state_id chain, so base fields erased by ELC canonicalization (for example -// `latest_height`) are not byte-compared. A divergent intermediate base from -// the authenticated relayer cannot affect the on-chain proof chain; at worst -// it corrupts this client's stitched host-store cache, which a subsequent -// serial update_client rewrites. +// Binding scope: every unit (including the first) is bound by the +// canonicalized state_id chain, not by byte comparison of the supplied base +// Anys: the canonical store's Any encoding is not stable across writers +// (create_client stores the submitter's encoding, update_client the enclave's +// re-encoded form), so a relayer-rebuilt base cannot reproduce the stored +// bytes in general. The first unit's prev_state_id is checked against the +// height-indexed state_id recorded in the canonical store, and its prev_height +// against the speculative-commit high-water mark +// (`verify_expected_base_state_in_tx`); base fields erased by ELC +// canonicalization (for example `latest_height`) are therefore not compared. +// A divergent base from the authenticated relayer cannot affect the on-chain +// proof chain; at worst it corrupts this client's stitched host-store cache, +// which a subsequent serial update_client rewrites. fn validate_observed_transition_follows( unit_id: &str, previous: Option<&ObservedStateTransition>, diff --git a/modules/types/src/store_key.rs b/modules/types/src/store_key.rs index b6183deb..4a08a955 100644 --- a/modules/types/src/store_key.rs +++ b/modules/types/src/store_key.rs @@ -17,6 +17,13 @@ pub fn state_id(client_id: &str, height: &Height) -> String { ) } +/// Host-managed record of the highest `post_height` committed through the +/// speculative stitch path. Not written by the enclave; see `enclave-api`'s +/// `apply_write_set_with_expected_base`. +pub fn speculative_commit_height(client_id: &str) -> String { + format!("clients/{client_id}/speculativeCommitHeight") +} + pub fn consensus_state(client_id: &str, height: &Height) -> String { format!( "clients/{}/consensusStates/{}-{}", @@ -42,6 +49,10 @@ pub fn consensus_state_bytes(client_id: &str, height: &Height) -> Vec { consensus_state(client_id, height).into_bytes() } +pub fn speculative_commit_height_bytes(client_id: &str) -> Vec { + speculative_commit_height(client_id).into_bytes() +} + #[cfg(test)] mod tests { use super::*;