-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add CachingShardStore
for batching writes to a backend ShardStore
#85
Conversation
`CachingShardStore` relies on this method being a no-op when the checkpoint doesn't exist.
@@ -36,4 +36,5 @@ test-dependencies = ["proptest", "assert_matches"] | |||
|
|||
[target.'cfg(unix)'.dev-dependencies] | |||
pprof = { version = "0.9", features = ["criterion", "flamegraph"] } # MSRV 1.56 | |||
dashmap = ">=5, <5.5.0" |
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.
What was the issue with 5.5.0?
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.
As I documented in the commit message, this was to keep MSRV. dashmap 5.5.0
bumped dependencies in a way that breaks the MSRV of this crate, but because we only use it as a dev-dependency we can just pin in this way. We've had this problem a lot with the criterion
/ pprof
stack, and it's the standard way we solve it.
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 ok. I knew that zcashd has an MSRV of 1.69, the librustzcash crates' MSRVs are 1.65, and dashmap 5.5.0's MSRV is 1.64. I guess it's xacrimon/dashmap@d36311e that causes the problem? Is there a reason why we can't/shouldn't update the incrementalmerkletree repo's crates to match librustzcash?
In any case,
- shardtree does not appear to declare its MSRV;
- there isn't a comment in this file saying what the MSRV of dashmap is, unlike for pprof and inferno.
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.
You're correct that shardtree
doesn't yet declare its MSRV; we should add that. The rest of the crates in this repo have MSRV 1.60, which is what CI is enforcing.
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.
Post-hoc ACK with a question about the dashmap version constraint.
No description provided.