From 30b6821647f006ef10148c7488ebd6b0626403a1 Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Thu, 21 Sep 2023 00:10:04 +0200 Subject: [PATCH 1/3] soroban-rpc: preflight: Exclude temporary expired entries from SnapshotSource --- .../lib/preflight/src/ledger_storage.rs | 47 +++++++++++++------ 1 file changed, 32 insertions(+), 15 deletions(-) diff --git a/cmd/soroban-rpc/lib/preflight/src/ledger_storage.rs b/cmd/soroban-rpc/lib/preflight/src/ledger_storage.rs index 2ac3862b8..80c07bc5d 100644 --- a/cmd/soroban-rpc/lib/preflight/src/ledger_storage.rs +++ b/cmd/soroban-rpc/lib/preflight/src/ledger_storage.rs @@ -1,6 +1,6 @@ use sha2::Digest; use soroban_env_host::storage::SnapshotSource; -use soroban_env_host::xdr::ContractDataDurability::Persistent; +use soroban_env_host::xdr::ContractDataDurability::{Persistent, Temporary}; use soroban_env_host::xdr::{ ConfigSettingEntry, ConfigSettingId, Error as XdrError, ExpirationEntry, Hash, LedgerEntry, LedgerEntryData, LedgerKey, LedgerKeyConfigSetting, LedgerKeyExpiration, ReadXdr, ScError, @@ -189,6 +189,24 @@ impl LedgerStorage { Ok((entry, expiration_seq)) } + fn get_including_persistent_expired( + &self, + key: &LedgerKey, + ) -> Result<(LedgerEntry, Option), Error> { + let entry_and_expiration = self.get(key, true)?; + // Explicitly discard temporary expired entries + if let Ok(expirable_entry) = + TryInto::>::try_into(&entry_and_expiration) + { + if expirable_entry.durability() == Temporary + && expirable_entry.has_expired(self.current_ledger_sequence) + { + return Err(Error::NotFound); + } + } + Ok(entry_and_expiration) + } + pub(crate) fn get_xdr(&self, key: &LedgerKey, include_expired: bool) -> Result, Error> { // TODO: this can be optimized since for entry types other than ContractCode/ContractData, // they don't need to be deserialized and serialized again @@ -227,28 +245,27 @@ impl LedgerStorage { impl SnapshotSource for LedgerStorage { fn get(&self, key: &Rc) -> Result<(Rc, Option), HostError> { - let mut entry_and_expiration = - ::get(self, key, self.restore_tracker.is_some())?; if let Some(ref tracker) = self.restore_tracker { + let mut entry_and_expiration = self + .get_including_persistent_expired(key) + .map_err(HostError::from)?; // If the entry expired, we modify the expiration to make it seem like it was restored entry_and_expiration.1 = tracker.track_and_restore(self.current_ledger_sequence, key, &entry_and_expiration); + return Ok((entry_and_expiration.0.into(), entry_and_expiration.1)); } - Ok((entry_and_expiration.0.into(), entry_and_expiration.1)) + let entry_and_expiration = + ::get(self, key, false).map_err(HostError::from)?; + return Ok((entry_and_expiration.0.into(), entry_and_expiration.1)); } fn has(&self, key: &Rc) -> Result { - let entry_and_expiration = - match ::get(self, key, self.restore_tracker.is_some()) { - Err(e) => match e { - Error::NotFound => return Ok(false), - _ => return Err(HostError::from(e)), - }, - Ok(le) => le, - }; - if let Some(ref tracker) = self.restore_tracker { - _ = tracker.track_and_restore(self.current_ledger_sequence, key, &entry_and_expiration); + let result = ::get(self, key); + if let Err(ref host_error) = result { + if host_error.error == HostError::from(Error::NotFound).error { + return Ok(false); + } } - Ok(true) + return result.map(|_| true); } } From 6efed2016e43f37bad481757fec38b148080b2ae Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Thu, 21 Sep 2023 01:35:35 +0200 Subject: [PATCH 2/3] Factor in get_including_persistent_expired --- .../lib/preflight/src/ledger_storage.rs | 32 +++++++------------ 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/cmd/soroban-rpc/lib/preflight/src/ledger_storage.rs b/cmd/soroban-rpc/lib/preflight/src/ledger_storage.rs index 80c07bc5d..51f37ee17 100644 --- a/cmd/soroban-rpc/lib/preflight/src/ledger_storage.rs +++ b/cmd/soroban-rpc/lib/preflight/src/ledger_storage.rs @@ -189,24 +189,6 @@ impl LedgerStorage { Ok((entry, expiration_seq)) } - fn get_including_persistent_expired( - &self, - key: &LedgerKey, - ) -> Result<(LedgerEntry, Option), Error> { - let entry_and_expiration = self.get(key, true)?; - // Explicitly discard temporary expired entries - if let Ok(expirable_entry) = - TryInto::>::try_into(&entry_and_expiration) - { - if expirable_entry.durability() == Temporary - && expirable_entry.has_expired(self.current_ledger_sequence) - { - return Err(Error::NotFound); - } - } - Ok(entry_and_expiration) - } - pub(crate) fn get_xdr(&self, key: &LedgerKey, include_expired: bool) -> Result, Error> { // TODO: this can be optimized since for entry types other than ContractCode/ContractData, // they don't need to be deserialized and serialized again @@ -246,9 +228,17 @@ impl LedgerStorage { impl SnapshotSource for LedgerStorage { fn get(&self, key: &Rc) -> Result<(Rc, Option), HostError> { if let Some(ref tracker) = self.restore_tracker { - let mut entry_and_expiration = self - .get_including_persistent_expired(key) - .map_err(HostError::from)?; + let mut entry_and_expiration = self.get(key, true)?; + // Explicitly discard temporary expired entries + if let Ok(expirable_entry) = + TryInto::>::try_into(&entry_and_expiration) + { + if expirable_entry.durability() == Temporary + && expirable_entry.has_expired(self.current_ledger_sequence) + { + return Err(HostError::from(Error::NotFound)); + } + } // If the entry expired, we modify the expiration to make it seem like it was restored entry_and_expiration.1 = tracker.track_and_restore(self.current_ledger_sequence, key, &entry_and_expiration); From 2d8c9a711444593603cd312b528fb76fef700850 Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Thu, 21 Sep 2023 01:47:35 +0200 Subject: [PATCH 3/3] Add specific error for expired entries and improve error check --- cmd/soroban-rpc/lib/preflight/src/ledger_storage.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/cmd/soroban-rpc/lib/preflight/src/ledger_storage.rs b/cmd/soroban-rpc/lib/preflight/src/ledger_storage.rs index 51f37ee17..b000994c9 100644 --- a/cmd/soroban-rpc/lib/preflight/src/ledger_storage.rs +++ b/cmd/soroban-rpc/lib/preflight/src/ledger_storage.rs @@ -28,6 +28,8 @@ extern "C" { pub(crate) enum Error { #[error("not found")] NotFound, + #[error("entry expired")] + EntryExpired, #[error("xdr processing error: {0}")] Xdr(#[from] XdrError), #[error("nul error: {0}")] @@ -43,7 +45,9 @@ pub(crate) enum Error { impl From for HostError { fn from(value: Error) -> Self { match value { - Error::NotFound => ScError::Storage(ScErrorCode::MissingValue).into(), + Error::NotFound | Error::EntryExpired => { + ScError::Storage(ScErrorCode::MissingValue).into() + } Error::Xdr(_) => ScError::Value(ScErrorCode::InvalidInput).into(), _ => ScError::Context(ScErrorCode::InternalError).into(), } @@ -182,7 +186,7 @@ impl LedgerStorage { && expiration_seq.is_some() && has_expired(expiration_seq.unwrap(), self.current_ledger_sequence) { - return Err(Error::NotFound); + return Err(Error::EntryExpired); } let entry = LedgerEntry::from_xdr(xdr)?; @@ -236,7 +240,7 @@ impl SnapshotSource for LedgerStorage { if expirable_entry.durability() == Temporary && expirable_entry.has_expired(self.current_ledger_sequence) { - return Err(HostError::from(Error::NotFound)); + return Err(HostError::from(Error::EntryExpired)); } } // If the entry expired, we modify the expiration to make it seem like it was restored @@ -252,7 +256,7 @@ impl SnapshotSource for LedgerStorage { fn has(&self, key: &Rc) -> Result { let result = ::get(self, key); if let Err(ref host_error) = result { - if host_error.error == HostError::from(Error::NotFound).error { + if host_error.error.is_code(ScErrorCode::MissingValue) { return Ok(false); } }