Review questions and observations (diverse)

[Ben-PH]

https://github.com/rifflabs/neverust/blob/ff8ecdbf9a6a5c7458954a3e7556a521dbe7d702/neverust-core/src/archivist_tree.rs#L201-L205

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.

https://github.com/rifflabs/neverust/blob/ff8ecdbf9a6a5c7458954a3e7556a521dbe7d702/neverust-core/src/archivist_tree.rs#L285

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.

https://github.com/rifflabs/neverust/blob/ff8ecdbf9a6a5c7458954a3e7556a521dbe7d702/neverust-core/src/archivist_tree.rs#L312

change to with_capacity (though optimiser probable handles it anyway)

https://github.com/rifflabs/neverust/blob/ff8ecdbf9a6a5c7458954a3e7556a521dbe7d702/neverust-core/src/blockexc.rs#L92-L95

Could mode be an enum, with price per byte being an associated value to marketplace? UPDATE: #7

https://github.com/rifflabs/neverust/blob/ff8ecdbf9a6a5c7458954a3e7556a521dbe7d702/neverust-core/src/blockexc.rs#L195

super long function, with deep nesting. probably needs some attention

this file also probably deserves to be broken up into sub-modules

https://github.com/rifflabs/neverust/blob/ff8ecdbf9a6a5c7458954a3e7556a521dbe7d702/neverust-core/src/blockexc.rs#L775

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

https://github.com/rifflabs/neverust/blob/ff8ecdbf9a6a5c7458954a3e7556a521dbe7d702/neverust-core/src/botg.rs#L87

an RNG value in a default implimentation hits a bit weird. Not sure what the solution is without it creating its own problems, though.

https://github.com/rifflabs/neverust/blob/ff8ecdbf9a6a5c7458954a3e7556a521dbe7d702/neverust-core/src/botg.rs#L94-L113

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.

https://github.com/rifflabs/neverust/blob/ff8ecdbf9a6a5c7458954a3e7556a521dbe7d702/neverust-core/src/botg.rs#L94

derive default here

https://github.com/rifflabs/neverust/blob/ff8ecdbf9a6a5c7458954a3e7556a521dbe7d702/neverust-core/src/botg.rs#L570-L582

this should be up the top, or part of a submodule

https://github.com/rifflabs/neverust/blob/ff8ecdbf9a6a5c7458954a3e7556a521dbe7d702/neverust-core/src/chunker.rs#L8

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)

https://github.com/rifflabs/neverust/blob/ff8ecdbf9a6a5c7458954a3e7556a521dbe7d702/neverust-core/src/cid_blake3.rs#L37-L41

name and implementation appear to diverge. not entirely sure if this function is worth keeping

https://github.com/rifflabs/neverust/blob/ff8ecdbf9a6a5c7458954a3e7556a521dbe7d702/neverust-core/src/cid_blake3.rs#L57-L61

could make the hasher a constrained generic that defaults to sha256

https://github.com/rifflabs/neverust/blob/ff8ecdbf9a6a5c7458954a3e7556a521dbe7d702/neverust-core/src/cid_blake3.rs#L65-L71

equivilent to derived Default

Got through to discovery.rs

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