Skip to content
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

rewind refactoring in the low-level backend storage #3028

Closed
garyyu opened this issue Sep 7, 2019 · 1 comment
Closed

rewind refactoring in the low-level backend storage #3028

garyyu opened this issue Sep 7, 2019 · 1 comment
Assignees

Comments

@garyyu
Copy link
Contributor

garyyu commented Sep 7, 2019

To track an improvement item here, the original discussion: #3004 (comment)

garyyu 2 days ago Member
I have a very old question here, why the txhashset::extending_readonly always need a txhashset write lock instead of a read lock?

antiochp 2 days ago Author Member
Good question - its to do with how we implement rewind in the low-level backend storage - it needs to be a mutable ref as we update internal state last_pos.

Its an awkward hold over from how the backend was originally implemented. We'll get this fixed at some point.

		let mut header_pmmr = self.header_pmmr.write();
		let mut txhashset = self.txhashset.write();

		// Now create an extension from the txhashset and validate against the
		// latest block header. Rewind the extension to the specified header to
		// ensure the view is consistent.
		txhashset::extending_readonly(&mut header_pmmr, &mut txhashset, |mut ext| {
			pipe::rewind_and_apply_fork(&header, &mut ext)?;
		... ...

The biggest meaning could be for the get_merkle_proof, which is used by OutputPrintable.

For the moment, there's no much impact for this write lock. But I believe it's a big improvement in the future when we have more merkle proof use cases, which need call for example this get_merkle_proof.

The read lock will allow multiple parallel read access.

@antiochp
Copy link
Member

Going to close this for now - not immediately actionable.
We know this code needs revisiting at some point, so we don't gain much value tracking it as a github issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants