stratum-mining / stratum

stratum
https://stratumprotocol.org
Other
207 stars 122 forks source link

Roles Technical Debt #1162

Closed jbesraa closed 2 days ago

jbesraa commented 1 week ago

While working on #1066 and the integration tests for the roles, I noticed a few technical debts that would need to be addressed in a future refactor:

Note that the goal of this issue is to document the different problems within the roles code, but we are not planning to resolve them until we are done with #845.**

The following points are related almost always to all of the roles:

  1. Problem: async-channel is used within tokio context which can result in an unexpected bugs Solution: use tokio channels https://tokio.rs/tokio/tutorial/channels

  2. Problem: Some struct(s) function(s) have weird signatures like self_: &Arc<Mutex<Self>> Solution: Wrap only the properties that need to be mutated in a Mutex

  3. Problem: An internal Mutex implementation is used Solution: use https://docs.rs/tokio/latest/tokio/sync/struct.Mutex.html instead

  4. Problem: Duplicate code between the different roles. Some roles need Downstream service, others need Upstream. Usually this is just a server. Solution: use a common definition for Upstream, Downstream across the roles where possible

  5. Problem: Config properties are not clear and not grouped in structs Solution: Group properties into structs with more clear names

  6. Problem: Roles have more responsibilities than needed. For example the pool role is required to connect to the TemplateProvider instead of receiving the TemplateProvider connection as an input. Solution: Minimize code inside role::start functions across the different roles to only do whats that role is responsible for.

Fi3 commented 1 week ago
  1. we should remove async-channel cause most of the time is not needed or we can slightly change the implementation, that would result in a dependency removed that is good. Said that I want to add that I'm pretty sure that using them withing tokio context is ok.
  2. This make sense, but sometimes is faster to have one single Mutex than a lot of small ones, so we should go case by case here.
  3. We use the internal Mutex cause it have safe_lock that IMO is an interface fair superior than lock, it make a lot of foot guns harder or not possible (lock between await, not releasing the lock ecc ecc). Also keep in mind that tokio Mutex is not the right choice, most of the time is better to use std Mutex, you can find more about it on the tokio documentation.