deprecate `roles_logic_sv2`

[plebhash]

after #1928, this crate is only being consumed in SRI for its custom Mutex

this primitive is not a fundamental requirement for building Sv2 apps, and it doesn't make sense for SRI to maintain it

[GitGab19]

Adding this as the final sub-issue of #1268.

[lucasbalieiro]

Guys, I was cleaning up the custom mutex in the roles (just testing the waters), and while talking with @plebhash, we ran into the question: what should replace it?

• std::sync::Mutex
• tokio::sync::Mutex
• tokio::sync::RwLock

We’re leaning toward tokio::sync::RwLock to reduce contention, since the data is read far more often than written.

[Shourya742]

Guys, I was cleaning up the custom mutex in the roles (just testing the waters), and while talking with @plebhash, we ran into the question: what should replace it?

• std::sync::Mutex
• tokio::sync::Mutex
• tokio::sync::RwLock

We’re leaning toward tokio::sync::RwLock to reduce contention, since the data is read far more often than written.

tokio locks are way slower. And I also dont understand the motivation to remove the custom mutex what we use, rather than making it better.

[plebhash]

And I also dont understand the motivation to remove the custom mutex what we use, rather than making it better.

It simply wraps a closure and enforces a drop after its execution

what's being achieved there that proper usage of a regular lock wouldn't achieve? if we want to enforce dropping the lock, what's the difference between:

custom_mutex.safe_lock(|data| data.do_something())?

and

{
    let mut guard = mutex.lock()?;
    guard.do_something()
}

why maintain this logic (that has absolutely nothing to do with mining) on our codebase?

anyways, if we're going to keep it around, roles_logic_sv2 should not exist with the sole purpose of giving it a place to exist... so we should maybe add a module on the new stratum-apps crate to accomodate for it

and we should definitely do something about super_safe_lock because it's absolute nonsense

edit: I get the historical reasons of how super_safe_lock was developed, but the main part of it was commented out (apparently for windows compatibility?), leaving it as an actual footgun rather than something useful

[Shourya742]

And I also dont understand the motivation to remove the custom mutex what we use, rather than making it better.

It simply wraps a closure and enforces a drop after its execution

what's being achieved there that proper usage of a regular lock wouldn't achieve? if we want to enforce dropping the lock, what's the difference between:

custom_mutex.safe_lock(|data| data.do_something())?

and

{
    let mut guard = mutex.lock()?;
    guard.do_something()
}

why maintain this logic (that has absolutely nothing to do with mining) on our codebase?

anyways, if we're going to keep it around, roles_logic_sv2 should not exist with the sole purpose of giving it a place to exist... so we should maybe add a module on the new stratum-apps crate to accomodate for it

and we should definitely get rid of super_safe_lock because it's absolute nonsense

edit: I get the historical reasons of how super_safe_lock was developed, but the main part of it was commented out (apparently for windows compatibility?), leaving it as an actual footgun rather than something useful

Yeah, so the difference is the closure-based safe_lock actually enforces some safety guarantees that a regular lock() pattern doesn’t. With the closure version, the lifetime of the guard is strictly bound to the closure scope. You literally can’t accidentally hold the lock longer than intended or, worse, hit an await while it’s still active. That kind of structured locking makes it a lot harder to shoot yourself in the foot, especially when the code evolves and async sneaks in later. In contrast, the regular let guard = lock()? pattern looks fine until someone refactors the block to include an await or pass the guard around and that’s when subtle lock poisoning or deadlocks start showing up. It might look like a bit of extra boilerplate with safe_locks but, we get compile-time enforcement of lock lifetimes and a clearly bounded critical section. I’m all for relocating it into a dedicated module away from roles-logic, but the abstraction itself is valuable. It prevents exactly the kind of async locking issues that are painful.

[lucasbalieiro]

If we agree NOT to drop super_safe_lock, moving it to stratum-apps (or similar) seems like the best move in my perspective.

I tried removing the custom mutex from the apps, but there are too many places to refactor SAFELY(probably skill-issue). Using a raw Mutex | RwLock could work, I did something similar in pleblottery, but there it was manageable because it was just me and @plebhash and a smaller project.

in a bigger codebase like SRI apps, subtle bugs around guard lifetimes or deadlocks are easier to introduce, so the closure wrapper still adds real value, agreeing with @Shourya742.

also, without the wrapper, any reviewer or future contributor has to manually ensure that every lock is dropped at the right time.

I agree super_safe_lock needs improvement, the name is misleading, and the implementation could be better. The idea of handling locks automatically in a closure is good, I think.

after studying a bit, I’ve put together a proposal for a new version that’s safe and backward-compatible so we dont have to change anything on the roles. Integration tests pass locally.

use std::{
    panic::{self, AssertUnwindSafe},
    sync::{Mutex as Mutex_, MutexGuard, PoisonError},
};
#[derive(Debug)]
pub struct Mutex<T: ?Sized>(Mutex_<T>);

impl<T> Mutex<T> {
    pub fn new(v: T) -> Self {
        Mutex(Mutex_::new(v))
    }
    pub fn safe_lock<F, Ret>(&self, thunk: F) -> Result<Ret, PoisonError<MutexGuard<'_, T>>>
    where
        F: FnOnce(&mut T) -> Ret,
    {
        let mut lock = self.0.lock()?;
        let return_value = thunk(&mut *lock);
        drop(lock);
        Ok(return_value)
    }
    pub fn super_safe_lock<F, Ret>(&self, thunk: F) -> Ret
    where
        F: FnOnce(&mut T) -> Ret,
    {
        // Try locking; unwrap poison errors, since we handle panic safety ourselves
        let mut lock = self
            .0
            .lock()
            .unwrap_or_else(|poisoned| poisoned.into_inner());

        // Run closure in a catch_unwind so we can safely drop before resuming panic
        let result = panic::catch_unwind(AssertUnwindSafe(|| thunk(&mut *lock)));

        // Drop lock before propagating panic
        drop(lock);

        match result {
            Ok(val) => val,
            Err(payload) => panic::resume_unwind(payload),
        }
    }

    /// Provides direct access to the underlying [`Mutex`].
    pub fn to_remove(&self) -> Result<MutexGuard<'_, T>, PoisonError<MutexGuard<'_, T>>> {
        self.0.lock()
    }
}

[plebhash]

AFAIU the current implementation of super_safe_lock, despite being disabled, aims to reproduce code from no-panic crate so that super_safe_lock would fail compilation for any closure that has room for panic.

the proposal above is going in a different direction, more like "handle panics gracefully" at runtime.

I'm not claiming one is better than the other, but I think it's important to highlight that we would be changing the original intention of this function, and if we do it, it should be done intentionally and with a solid rationale, because enforcing the so called "super-safety" at compile time is different than enforcing it at runtime.

if we wanted to be more aligned with the current approach, an alternative could be trying to use the no-panic crate as a dependency (assuming stratum-apps won't have the same strict policy around dependencies), and investigate:

• windows compilation to understand the motivations for a39836a
• CI failures that apparently motivated disable_nopanic

and as pointed out on #495 (comment) we could also investigate whether this could be adapted into a custom RwLock rather than Mutex

but all these discussions feel like a brand new task, which is starting to get out of scope with this current one

if we're going to keep this custom Mutex around, that should be done by moving it as-is to stratum-apps... then cleanup and improvements can be done on a follow-up with a properly defined scope

[GitGab19]

For now I would simply move it to stratum-apps without changing anything about it.

Then we can open a specific issue about the custom Mutex and discuss approaches there.