-
Notifications
You must be signed in to change notification settings - Fork 1.5k
avoid tx root derivation #2942
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
base: staging
Are you sure you want to change the base?
avoid tx root derivation #2942
Conversation
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.
Thank you for your contribution
| /// Reads the deployment from a buffer. | ||
| fn read_le<R: Read>(mut reader: R) -> IoResult<Self> { | ||
| // Read the id variant. | ||
| let id_variant = u8::read_le(&mut reader)?; |
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.
Can this be made backwards compatible?
| let mut deployment = | ||
| serializer.serialize_struct("Deployment", len + self.id.get().is_some() as usize)?; | ||
| if let Some(id) = self.id.get() { | ||
| deployment.serialize_field("id", id)?; |
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.
Not backwards compatible.
| mod string; | ||
|
|
||
| use crate::{Transaction, Transition}; | ||
| use std::sync::OnceLock; |
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.
please move down to line 26 so external dependencies are in the same place
| #[derive(Clone)] | ||
| pub struct Deployment<N: Network> { | ||
| /// The deployment id | ||
| id: OnceLock<DeploymentID<N>>, |
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.
Would a design be possible where this is just an Option<...> like we did for the previous similar caching specified in the issue?
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 would require mutable constructs to deployment all over the code base as when we call to_deployment_id it's probably best to cache the id if not computed yet once, this also implies a redundant recursive chain of callers that will have to take an &mut. We could probably do a RefCell though?
Motivation
closes #2706
Tx root derivation takes some time during block generation, but these can be cached when computed for the first time.
Test Plan
No critical or logic changes. CI should catch any regressions