Review questions and observations (diverse) #5
Description
In this function, it creates a new hasher. Would it be prudent to make this a generic function (e.g. fn compress<H: Hasher(...). This could be handy for testing, e.g. a no-op hash would make it much easier to setup test hypothesis, it will eliminate the hashing cost, making performance analysis less noisy, etc.
this path vector is populated by cloning a vector in self.layers per entry.
Thinking about how this structure is used, does it make sense that this can be a reference instead of a clone? If so, it could be marked as a possible avenue when it comes to digging for marginal performance gains.
change to with_capacity (though optimiser probable handles it anyway)
Could mode be an enum, with price per byte being an associated value to marketplace? UPDATE: #7
super long function, with deep nesting. probably needs some attention
this file also probably deserves to be broken up into sub-modules
probably worth just deep copying the collection. the end user might want it in the original form anyway. if they want a vec, they can do the transformation themselves
an RNG value in a default implimentation hits a bit weird. Not sure what the solution is without it creating its own problems, though.
suggest providing context as to why they are optional. Assume those familiar with this can auto-understand, but this context will help smooth the onboarding for those learning.
derive default here
this should be up the top, or part of a submodule
Though this is constrained in the implementation, consider constraining the R generic.
also, some parts of the implementation do not need the constraints in their implementation (e.g. chunk size function)
name and implementation appear to diverge. not entirely sure if this function is worth keeping
could make the hasher a constrained generic that defaults to sha256
equivilent to derived Default
Many parts of the code base use tokio async as the concurrency model, but here we are using system threads.
doc comments explain here: https://github.com/rifflabs/neverust/blob/4c00ec71fad01c3307bd3c3386cc286b72fe7a36/neverust-core/src/pending_blocks.rs#L53-L56
at first glance this comes across as a potential smell. I'm sure a deeper understanding of the concurrency architecture will resolve this, but in the mean time, I would like to flag it so it has a chance to not slip between the cracks