-
Notifications
You must be signed in to change notification settings - Fork 161
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
Refactor ChainStore, tipset usage, cache loaded tipsets #851
Conversation
Ok(b) => { | ||
self.chain_store().set_tipset_tracker(b.header()).await?; | ||
Ok(_) => { | ||
// self.chain_store().set_tipset_tracker(b.header()).await?; |
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.
Remove or add a TODO
/// tip_index tracks tipsets by epoch/parentset for use by expected consensus. | ||
tip_index: RwLock<TipIndex>, | ||
tip_index: TipIndex, |
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.
The PR description says we're removing TipIndex? Why u no remove tipindex? Is this what you mean by half of #814 ?
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.
I just mean I gutted the TipIndex functionality and usages of it, I decided to keep for now because that will be built upon in next PR (why remove when it will just be re-added soon?)
@@ -303,12 +307,12 @@ where | |||
return Ok(None); | |||
} | |||
// TODO get tipset by height using cache instead of reloading tipsets |
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.
Is this comment still relevant? Seems like you implement that caching in tipset_from_keys now
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.
ah, nice catch
{ | ||
fn get_chain_randomness( | ||
&self, | ||
pers: DomainSeparationTag, | ||
round: ChainEpoch, | ||
entropy: &[u8], | ||
) -> Result<[u8; 32], Box<dyn Error>> { | ||
self.cs | ||
.get_chain_randomness(&self.blks, pers, round, entropy) | ||
task::block_on( |
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.
We dont want to just make this an async_trait? Is it because this is really only used in the VM?
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.
yeah, the vm execution is sync, and I don't want to introduce async primitives when there will never be any benefit to it.
Edit: this would also be a huge change within actors
Summary of changes
Changes introduced in this pull request:
Only half of #814, but made sense to split up and get this in asap
Reference issue to close (if applicable)
Closes
Other information and links