Switch total_consistency_lock to a Send RwLock variant

[tnull]

When trying to switch to async background processing/event handling in LDK Node, I stumbled across the fact that tokio won't let me spawn a task for the process_events_async, as the we currently try to hold the total_consistency_lock over .await boundaries:

rust-lightning/lightning/src/ln/channelmanager.rs

Lines 5242 to 5259 in e1208bf

	let _read_guard = self.total_consistency_lock.read().unwrap();
	let mut result = NotifyOption::SkipPersist;
	// TODO: This behavior should be documented. It's unintuitive that we query
	// ChannelMonitors when clearing other events.
	if self.process_pending_monitor_events() {
	result = NotifyOption::DoPersist;
	}
	let pending_events = mem::replace(&mut *self.pending_events.lock().unwrap(), vec![]);
	if !pending_events.is_empty() {
	result = NotifyOption::DoPersist;
	}
	for event in pending_events {
	handler(event).await;
	}

This is an issue since RwLockReadGuard is not Send:

     = help: within `impl futures::Future<Output = ()>`, the trait `std::marker::Send` is not implemented for `std::sync::RwLockReadGuard<'_, ()>`
note: future is not `Send` as this value is used across an await
    --> /Users/ero/.cargo/git/checkouts/rust-lightning-da94c153801dff32/17eafed/lightning/src/ln/channelmanager.rs:5258:18
     |
5242 |         let _read_guard = self.total_consistency_lock.read().unwrap();
     |             ----------- has type `std::sync::RwLockReadGuard<'_, ()>` which is not `Send`
...
5258 |             handler(event).await;
     |                           ^^^^^^ await occurs here, with `_read_guard` maybe used later
...
5264 |     }
     |     - `_read_guard` is later dropped here
note: required by a bound in `tokio::runtime::Runtime::spawn`
    --> /Users/ero/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.23.0/src/runtime/runtime.rs:192:21
     |
192  |         F: Future + Send + 'static,
     |                     ^^^^ required by this bound in `tokio::runtime::Runtime::spawn`

In order to fix this, we need to explore options for replacing the RwLock of total_consistency_lock. One option might be taking a suitable dependency such as parking_lot, another approach would be to implement our own RwLock variant that meets the requirements.

[TheBlueMatt]

Yea, so this is really gonna be a pain. We're gonna have to really bend over backwards for this, but I think it'll be pretty doable. Basically just implementing our own RwLock using atomics and CVs is CS201 kinda stuff, and will get us Send/Sync, at least insofar as the "inner type" on the mutex is () (ie we dont have to actually send anything anywhere).

[tnull]

Yea, so this is really gonna be a pain. We're gonna have to really bend over backwards for this, but I think it'll be pretty doable. Basically just implementing our own RwLock using atomics and CVs is CS201 kinda stuff, and will get us Send/Sync, at least insofar as the "inner type" on the mutex is () (ie we dont have to actually send anything anywhere).

Sounds good, I intend to pick this up after the 0.0.114 release.

[TheBlueMatt]

When you do this, can you look into ripping out all the Send + Sync bounds on the process_events_async fn? For folks doing wasm work they want to use the no-std/futures-based BP but can't actually have any threading. Thus, the Send + Sync bound is unnecessary, and problematic because its perfectly reasonable to use references/refcells or such there.

[TonyGiorgio]

+1 for removing Sync + Send for the background processor. The other area where we have copy-pasted a file from LDK to remove it is in the lightning-transaction-sync::esplora file: e9fa6ec (I can open up a seperate issue on that if it's preferred.)

Since we're coming from wasm, that's fine for us for now. I believe at one point I tried to do a bunch of unsafe impl Sync/Send but was in trait/alias hell so I gave up on that for now.

[tnull]

+1 for removing Sync + Send for the background processor. The other area where we have copy-pasted a file from LDK to remove it is in the lightning-transaction-sync::esplora file: e9fa6ec (I can open up a seperate issue on that if it's preferred.)

Since we're coming from wasm, that's fine for us for now. I believe at one point I tried to do a bunch of unsafe impl Sync/Send but was in trait/alias hell so I gave up on that for now.

I'm not sure about the background processor (as the async version doesn't work anyways currently), but I'm pretty sure we can't remove Sync + Send markers in lightning-transaction-sync, as otherwise we can't call sync from a tokio task (which is why I put them there in the first place).

[TheBlueMatt]

Yea, its actually really annoying Rust doesn't let us make some kind of optionally-Sync bound. We'd need a duplicate method that avoids the bounds or a feature to make them optional, if we could do that with relatively low LoC we should, though.