-
Notifications
You must be signed in to change notification settings - Fork 26
feat: TraceBuffer and simulation Replay #263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,9 +20,20 @@ use tokio::sync::Mutex; | |
|
|
||
| use super::DecodedChainSyncEvent; | ||
|
|
||
| #[derive(serde::Serialize, serde::Deserialize)] | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @abailly @stevana We need to get rid of all these active pieces of state within the pure stages, all effects need to be moved to ExternalEffect. This will then mean that we only need to deal with skipping this store when persisting the ExternalEffect — deserialization is optional for those because it is not needed for replaying a trace. |
||
| pub struct StoreHeader { | ||
| #[serde(skip, default = "default_store")] | ||
| pub store: Arc<Mutex<dyn ChainStore<Header>>>, | ||
| } | ||
| fn default_store() -> Arc<Mutex<dyn ChainStore<Header>>> { | ||
| Arc::new(Mutex::new(super::store::FakeStore::default())) | ||
| } | ||
|
Comment on lines
+23
to
+30
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Serialising drops the actual store – replay may load with an empty ledger
Consider emitting a stub handle that can re-attach to the real store (e.g. via a registry key) or, at minimum, loudly logging the downgrade so folks don’t wonder why the chain’s vanished. 🤖 Prompt for AI Agents |
||
|
|
||
| impl PartialEq for StoreHeader { | ||
| fn eq(&self, _other: &Self) -> bool { | ||
| true | ||
| } | ||
| } | ||
|
Comment on lines
+32
to
+36
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion
Two completely different Either remove the impl or compare on an identity (e.g. 🤖 Prompt for AI Agents |
||
|
|
||
| impl fmt::Debug for StoreHeader { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,12 +56,46 @@ pub fn header_is_valid( | |
| .map_err(|e| ConsensusError::InvalidHeader(point.clone(), e)) | ||
| } | ||
|
|
||
| #[derive(Clone)] | ||
| #[derive(Clone, serde::Serialize, serde::Deserialize)] | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is polluted because it is used a “state” in a pure stage, which is of course not how it is supposed to be. |
||
| pub struct ValidateHeader { | ||
| #[serde(skip, default = "default_ledger")] | ||
| pub ledger: Arc<dyn HasStakeDistribution>, | ||
| #[serde(skip, default = "default_store")] | ||
| pub store: Arc<Mutex<dyn ChainStore<Header>>>, | ||
| } | ||
|
|
||
| impl PartialEq for ValidateHeader { | ||
| fn eq(&self, _other: &Self) -> bool { | ||
| true | ||
| } | ||
| } | ||
|
Comment on lines
+67
to
+71
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Equality implementation always returns
🤖 Prompt for AI Agents |
||
|
|
||
| fn default_ledger() -> Arc<dyn HasStakeDistribution> { | ||
| struct Fake; | ||
| impl HasStakeDistribution for Fake { | ||
| fn get_pool( | ||
| &self, | ||
| _slot: amaru_kernel::Slot, | ||
| _pool: &amaru_kernel::PoolId, | ||
| ) -> Option<amaru_ouroboros::PoolSummary> { | ||
| unimplemented!() | ||
| } | ||
|
|
||
| fn slot_to_kes_period(&self, _slot: amaru_kernel::Slot) -> u64 { | ||
| unimplemented!() | ||
| } | ||
|
|
||
| fn max_kes_evolutions(&self) -> u64 { | ||
| unimplemented!() | ||
| } | ||
|
|
||
| fn latest_opcert_sequence_number(&self, _pool: &amaru_kernel::PoolId) -> Option<u64> { | ||
| unimplemented!() | ||
| } | ||
| } | ||
| Arc::new(Fake) | ||
| } | ||
|
Comment on lines
+73
to
+97
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion
These 🤖 Prompt for AI Agents |
||
|
|
||
| impl fmt::Debug for ValidateHeader { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| f.debug_struct("ValidateHeader") | ||
|
|
@@ -71,12 +105,24 @@ impl fmt::Debug for ValidateHeader { | |
| } | ||
| } | ||
|
|
||
| #[derive(serde::Serialize, serde::Deserialize)] | ||
| struct EvolveNonceEffect { | ||
| #[serde(skip, default = "default_store")] | ||
| store: Arc<Mutex<dyn ChainStore<Header>>>, | ||
| header: Header, | ||
| global_parameters: GlobalParameters, | ||
| } | ||
|
|
||
| impl PartialEq for EvolveNonceEffect { | ||
| fn eq(&self, other: &Self) -> bool { | ||
| self.header == other.header && self.global_parameters == other.global_parameters | ||
| } | ||
| } | ||
|
|
||
| fn default_store() -> Arc<Mutex<dyn ChainStore<Header>>> { | ||
| Arc::new(Mutex::new(super::store::FakeStore::default())) | ||
| } | ||
|
|
||
| impl EvolveNonceEffect { | ||
| fn new( | ||
| store: Arc<Mutex<dyn ChainStore<Header>>>, | ||
|
|
@@ -101,25 +147,16 @@ impl fmt::Debug for EvolveNonceEffect { | |
| } | ||
|
|
||
| impl ExternalEffect for EvolveNonceEffect { | ||
| fn run(self: Box<Self>) -> pure_stage::BoxFuture<'static, Box<dyn pure_stage::Message>> { | ||
| fn run(self: Box<Self>) -> pure_stage::BoxFuture<'static, Box<dyn pure_stage::SendData>> { | ||
| Box::pin(async move { | ||
| let result = self | ||
| .store | ||
| .lock() | ||
| .await | ||
| .evolve_nonce(&self.header, &self.global_parameters); | ||
| Box::new(result) as Box<dyn pure_stage::Message> | ||
| Box::new(result) as Box<dyn pure_stage::SendData> | ||
| }) | ||
| } | ||
|
|
||
| fn test_eq(&self, other: &dyn ExternalEffect) -> bool { | ||
| other | ||
| .cast_ref::<Self>() | ||
| .map(|other| { | ||
| self.header == other.header && self.global_parameters == other.global_parameters | ||
| }) | ||
| .unwrap_or(false) | ||
| } | ||
| } | ||
|
|
||
| impl ExternalEffectAPI for EvolveNonceEffect { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dangling reference returned from
era_historyNetworkName::Preprod.into()yields a freshEraHistoryvalue. Returning&to that temporary is UB bait – the borrow checker should already be yelling. Fix by storing theEraHistoryinsideFakeStore(or use a'staticconstant) and return a reference to that.#[derive(Default)] pub struct FakeStore { headers: BTreeMap<Hash<32>, Header>, nonces: BTreeMap<Hash<32>, Nonces>, + era_history: EraHistory, } impl Default for FakeStore { fn default() -> Self { - Self { headers: BTreeMap::new(), nonces: BTreeMap::new() } + Self { + headers: BTreeMap::new(), + nonces: BTreeMap::new(), + era_history: NetworkName::Preprod.into(), + } } } fn era_history(&self) -> &EraHistory { - NetworkName::Preprod.into() + &self.era_history }🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That particular
Fromimplementation does indeed return a static reference to a constant value.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rkuhn Ah, fair dinkum! Thanks for the clarification, mate. That's a neat implementation detail - having
NetworkName::Preprod.into()return a static reference to a constantEraHistoryis quite clever. Apologies for the false alarm - I was thinking in general terms about temporaries rather than the specific implementation. Cheers for setting the record straight! 🍺✏️ Learnings added