RGB KVStore Integration

[dcorral]

This PR adds database persistence for RGB-specific data via the KVStoreSync trait, replacing direct filesystem operations with KVStore calls through dependency injection.

Changes

New KVStore Functions (rgb_utils/mod.rs)

Added namespace constants and persistence functions:

pub const RGB_PRIMARY_NS: &str = "rgb";
pub const RGB_CHANNEL_INFO_NS: &str = "channel_info";
pub const RGB_CHANNEL_INFO_PENDING_NS: &str = "channel_info_pending";
pub const RGB_PAYMENT_INFO_INBOUND_NS: &str = "payment_info_inbound";
pub const RGB_PAYMENT_INFO_OUTBOUND_NS: &str = "payment_info_outbound";
pub const RGB_TRANSFER_INFO_NS: &str = "transfer_info";
pub const RGB_CONSIGNMENT_NS: &str = "consignment";

New functions:

• read_rgb_transfer_info / write_rgb_transfer_info
• read_rgb_channel_info / write_rgb_channel_info / remove_rgb_channel_info
• read_rgb_payment_info / write_rgb_payment_info
• read_rgb_consignment / write_rgb_consignment / remove_rgb_consignment
• get_rgb_channel_info_pending
• is_payment_rgb
• filter_first_hops

Dependency Injection

KVStore is now passed through struct fields as Arc<dyn KVStoreSync + Send + Sync>:

• ChannelManager.rgb_kv_store
• ChannelManagerReadArgs.rgb_kv_store
• ChannelContext.rgb_kv_store
• OutboundPayments.rgb_kv_store
• KeysManager.rgb_kv_store
• InMemorySigner.rgb_kv_store

Updated RGB Functions

All RGB functions now receive KVStore directly instead of filesystem paths:

• color_commitment - Reads pending channel info and payment info from KVStore, writes transfer info
• color_htlc - Reads transfer info from KVStore, writes updated transfer info
• color_closing - Reads pending channel info from KVStore, writes transfer info
• is_channel_rgb - Checks KVStore for channel RGB data
• get_rgb_channel_info - Reads from KVStore
• rename_rgb_files - Renames channel info and consignment entries in KVStore
• handle_funding - Writes consignment and channel info to KVStore
• update_rgb_channel_amount - Reads/writes channel info via KVStore

Proxy Key Pattern for HTLC Payments

Payment info uses a proxy key scheme for HTLC tracking:

• Main key: payment_hash (hex)
• Proxy key: {channel_id}_{payment_hash}
• Pending key: {payment_hash}_pending

Design

1. KVStore via dependency injection - Flows through all main structures rather than using a global registry
2. Namespaced storage - All RGB data organized under rgb primary namespace with typed secondary namespaces
3. Structured serialization - RgbInfo, RgbPaymentInfo, and TransferInfo serialized to KVStore

Breaking Changes

The following public functions now require a kv_store parameter instead of filesystem paths:

Function	New Parameter
is_channel_rgb	kv_store: &K where K: KVStoreSync
update_rgb_channel_amount	kv_store: &K where K: KVStoreSync
write_rgb_payment_info_file	kv_store: &K where K: KVStoreSync (also removed ldk_data_dir)
color_htlc	kv_store: &dyn KVStoreSync
color_closing	kv_store: &dyn KVStoreSync
handle_funding	kv_store: &K where K: KVStoreSync

Struct constructors (ChannelManager, KeysManager, InMemorySigner, OutboundPayments) now require an rgb_kv_store parameter.

[zoedberg]

@dcorral could you please rebase this PR?

[dcorral]

@zoedberg done!

[zoedberg]

Thanks! I left some change requests. Also, instead of serializing structs to JSON strings and then convert the strings to bytes I would use bincode to serialize the structs directly in bytes.

[zoedberg]

Why did you choose to use KVStoreSync instead of KVStore (its async version)? I would use the latter if possible, since RLN runs in an async environment

[dcorral]

I used KVStoreSync because all the call sites are in synchronous code paths: color_commitment, color_htlc, color_closing, update_rgb_channel_id, handle_funding, etc. are all sync functions inside channel.rs and channelmanager.rs, which are entirely sync.

Using the async KVStore would require making all of these async, which would mean changing a big chunk of LDK's internals. do you think it's worth doing that, or is KVStoreSync ok for now?

[zoedberg]

What about doing like ldk-node does? Which is implementing an inner store and then implementing for the wrapper store both KVStore and KVStoreSync. That way we could use the KVStoreSync in LDK (where we don't have an async environment) and KVStore in RLN (where we do have an async environment)

[dcorral]

Alright, sounds like a plan.

[dcorral]

Looking into it, KVStoreSyncWrapper already exists and it wraps any KVStoreSync into KVStore by boxing the sync result into a future. So the rust-lightning side can stay as-is with KVStoreSync and on the RLN side we just wrap the sea-orm store with KVStoreSyncWrapper to expose the async KVStore trait where needed.

[dcorral]

All three done:

• generic KV: KVStoreSync + Send + Sync + 'static in e67779b, only where the Arc fields actually lived; no dyn KVStoreSync left
• SeaOrmKvStore implements both KVStore (native async) and KVStoreSync
• RlnDatabase dropped

One deviation though, the KVStoreSync impl doesn't use block_in_place + Handle::current().block_on(), that deadlocks. The sync calls run on the node's runtime, so when all workers are parked waiting on DB results, nobody is left to poll the DB futures they're waiting on. Verified with a 30s timeout on the the sync→async adapter in kv_store.rs: the queued DB tasks only ran once the timeout unwound the blocked caller. Instead block_on spawns the future on a small dedicated runtime and waits on a channel inside block_in_place. Unlike the old DB_RUNTIME: multi-threaded, no per-call thread spawn. Async path unaffected.

[zoedberg]

Verified all three on the current head, looks good, thank you.

On the deviation:

the KVStoreSync impl doesn't use block_in_place + Handle::current().block_on(), that deadlocks. The sync calls run on the node's runtime, so when all workers are parked waiting on DB results, nobody is left to poll the DB futures they're waiting on

From my understanding this isn't how block_in_place behaves: it hands the worker's core off to another thread precisely so blocked callers can't starve the runtime, and Handle::block_on inside it polls the future on the calling thread. What would break is the nested-executor case: the coloring paths call the KVStore from inside futures::executor::block_on (see _get_rgb_wallet), and Handle::current().block_on() from within that nested context is not allowed. Can you confirm whether your 30s-timeout repro was on one of those paths? If that's the case, then the current design is the right call, just update the comment on top of fn db_runtime() to mention the nested futures::executor::block_on as the reason. If the deadlock reproduced on a plain sync path with a correctly-placed block_in_place, I'd like to see it, because then something else is off.

[dcorral]

Confirmed. The deadlock was on a nested path: the thread dump shows a peer-handler task inside futures::executor::block_on(_get_rgb_wallet) calling the store, so Handle::current().block_on() was illegal there, as you said. I don't have a deadlock to show on a plain sync path with a correctly-placed block_in_place.

On keeping the dedicated runtime: the sync store calls execute while holding LDK locks on runtime workers, e.g. internal_commitment_signed takes peer_state_mutex.lock() and the commitment path calls color_commitment, which hits the store. Waiting on the caller's runtime from inside those critical sections ties lock hold times to that runtime's scheduling; the dedicated runtime makes DB completion independent of it.

Updated the fn db_runtime() comment to mention the nested futures::executor::block_on as the reason.

[zoedberg]

Thanks for confirming the nested-executor cause.

On the lock-hold-time argument: I don't think it holds up as a reason to keep the dedicated runtime. The caller thread blocks for the full DB round-trip while holding peer_state either way, the dedicated runtime doesn't shorten the critical section, it only decouples the DB future's wakeups from the main runtime's I/O driver. With a correctly-placed block_in_place the runtime backfills the worker, so that dependency only matters under severe saturation. Meanwhile the dedicated runtime costs a cross-runtime spawn plus a channel wait on every sync DB call.

So the only hard blocker for block_in_place + Handle::current().block_on() is the nested futures::executor::block_on context, and I'd rather remove that than design around it. Looking at why it exists: _get_rgb_wallet and _accept_transfer are async only to spawn_blocking the wallet construction, but every caller is sync and immediately does futures::executor::block_on(...). That parks the calling thread for the full duration anyway, so the spawn_blocking buys nothing, it just adds a thread handoff and creates the illegal nested context.
For these reasons, I think we should:

1. make _get_rgb_wallet/_accept_transfer plain sync functions, wrapping the blocking wallet work in tokio::task::block_in_place instead of spawn_blocking
2. remove all futures::executor::block_on call sites in the coloring paths (and the same block_on(spawn_blocking(...)) pattern in RLN's output sweeper), so the invariant becomes "no nested executors anywhere"
3. then switch the KVStoreSync impl to block_in_place + Handle::current().block_on(), documenting the "no nested executors" invariant assumption, and drop db_runtime() entirely

As a follow-up after this PR lands: the deeper fix is to stop constructing a fresh rgb-lib wallet on every coloring call and instead inject RLN's long-lived Arc<Mutex<RgbLibWallet>> through the same plumbing this PR built for rgb_kv_store (possibly bundled with the kv_store and the consignment dir into a single injected context struct). That removes wallet construction and go_online from the critical sections entirely and makes the coloring paths plain sync method calls. We'll tackle that in a dedicated PR after this one, so no need to address it here.

[dcorral]

I implemented all three points. 1 and 2 are done and kept: _get_rgb_wallet/_accept_transfer are plain sync fns using block_in_place, and no futures::executor::block_on call sites remain anywhere (coloring paths and RLN's output spender included).

Point 3 deadlocks.

With the store on block_in_place + Handle::block_on and db_runtime() dropped, swap_roundtrip_sell hangs reproducibly. I traced it to sqlx itself: when a PoolConnection is dropped, sqlx returns it to the pool by spawning a task on the current runtime. So driving the future on the caller's thread makes the connection-return land on the caller's runtime. Once that runtime is saturated (which LDK sync code can cause on its own by blocking on std mutexes on workers without block_in_place), returns stop running, the pool drains, and the next acquire waits forever. A minimal unit test shows it deterministically: a sync writer holding a std mutex while another task pins the only worker completes write 0 and hangs on write 1, waiting for a connection whose return task is queued behind the pinned worker. This isn't fixable with better placement of block_in_place sqlx structurally needs a live executor to give connections back.

One idea to actually remove the dedicated runtime would be taking sqlx out of the store's data path: do the kv operations with rusqlite (plain blocking SQLite) under block_in_place, with the async KVStore impl delegating via spawn_blocking. There'd be no pool, no spawned return tasks, no timers — nothing that can be starved, on either path. sea-orm could stay for migrations, the entities and the typed config/auth tables, with only the hot kv path going direct. Not sure it's the right trade-off since it walks part of the sea-orm integration back, so I'd like your take before going that way. If it sounds reasonable I'd draft it in a separate commit

[zoedberg]

Having now the DB I think we can just make this an update, am I missing something?

[dcorral]

the KVStoreSync trait doesn't have a rename/update-key method though, so we'd need to either add one to the trait or handle it at the DB level in the RLN KVStore implementation. What's your suggestion?

[zoedberg]

Right. Then let's keep the read, write and remove, but let's put them inside an update_rgb_channel_info method. Also, have you considered a way to avoid write concurrency? It would be nice to be able to use DB transactions but I'm not sure that's easy to achieve

[dcorral]

to get real atomicity we'd need to add a transaction() method to KVStoreSync that runs all operations within a single DB transaction. This requires refactoring SeaOrmKvStore to be generic over sea-orm's ConnectionTrait (since DatabaseTransaction and DatabaseConnection both implement it but are different types) and the fs based FilesystemStore in lightning-persister would also need to implement it somehow, which is non-trivial.

The main risk here is crash recovery rather than write concurrency since LDK already holds exclusive access per channel during these calls and this rename only happens once per channel at funding time.

[zoedberg]

I'm not sure what do you intend here. Why do you say "requires refactoring SeaOrmKvStore to be generic over sea-orm's ConnectionTrait"? I don't see this requirement but maybe I'm over-simplifying this.

About write concurrency, my concern is broader, I was not talking about this method in particular.

[zoedberg]

Thanks for coming back to this.

Concurrency

I don't see the per-key lock map / version counter in the current heads, did you forget to push?

Atomicity

since RgbKvStoreExt is new in this PR, I can add a provided method that implementations may override with a real transaction

Since RgbKvStoreExt is a blanket impl over all K: KVStoreSync, no concrete type can override it, as you noted in April. Instead KVStoreSync is implemented concretely per store, so a default method there can be overridden. I think you could do:

pub enum KvOp {
    Write { secondary_namespace: String, key: String, value: Vec<u8> },
    Remove { secondary_namespace: String, key: String },
}

// on KVStoreSync, with a provided default body:
fn execute_batch(&self, primary_namespace: &str, ops: Vec<KvOp>) -> Result<(), io::Error> {
    // default: apply ops sequentially (today's behavior)
}

SeaOrmKvStore overrides it with connection.begin(), FilesystemStore keeps the default, and RgbKvStoreExt keeps its blanket impl. update_rgb_channel_id / update_rgb_channel_info become a single execute_batch call. Also note there are more multi-step sequences than the two you listed (see color_commitment for example), so with one primitive they can all migrate.

Implementation note: make sure the batch composes with the per-key lock map: acquire the per-key locks in sorted key order (or all upfront) before starting the transaction, to avoid deadlocks between concurrent batches.

rgb_payment fix

Nice catch! Since it was a pre-existing bug it would be great if you could split it into its own PR. And if it's not too difficult please also add an anti-regression test that shows the bug before the fix.

[dcorral]

Concurrency: right, that hadn't been pushed. It's on the current heads now: per-key lock map plus a version counter assigned at call time, stale writes skipped ( RGB-Tools/rgb-lightning-node@6f06e82, with deterministic tests for the write/remove ordering).

Atomicity: implemented now KvOp + execute_batch as a provided method on KVStoreSync with a sequential default, SeaOrmKvStore overriding it with connection.begin(). Migrated the multi-step sequences: both pairs in color_commitment (creation and pending→proxy promotion), write_rgb_payment_info, update_rgb_channel_info / update_rgb_channel_id (single 4-op batch), handle_funding, and the openchannel pair on the RLN side.
On composition: the batch takes the per-key locks for all touched keys upfront in sorted order and uses a single version, so concurrent batches and single writes can't deadlock.
One gap worth deciding on: read-modify-write sequences (e.g. update_rgb_channel_amount: read info -> adjust amounts -> write back) are still not atomic, write ordering can't help when the value was computed from a stale read. Today that's safe because all callers are already serialized (LDK's under peer_state, RLN's in the event handler, which processes events one at a time), same as with the filesystem. If we want it closed at the store level, I'd add an update method to KVStoreSync following the same pattern as execute_batch: provided default doing read -> closure -> write (today's behavior), SeaOrmKvStore overriding it to hold the per-key lock across the whole sequence.

rgb_payment fix: will do, splitting it into its own PR against rgb branch. On the anti-regression test, since we are not compiling LDK's tests, should I create an RLN test restarting a node with an HTLC in flight?

[zoedberg]

Verified both commits, thank you. The sorted upfront lock acquisition in execute_batch and the version handling on singles look correct, and the deadlock analysis holds (singles never hold one key lock while waiting on another, batches acquire in sorted order).

One semantic issue in the batch override though: the stale-write check is applied per-op (if *guards[idx] > version { continue; }), so if one key of a batch was concurrently written with a newer version, that op is skipped while the rest of the batch commits. That's a partial application of what we introduced batches to keep atomic (e.g. for update_rgb_channel_info it could remove the old key while skipping the write of the new one, losing the channel info). Since all ops in a batch share one version and represent one logical write, I think staleness should be all-or-nothing too: if any touched key has last_applied > version, skip the entire batch. Unreachable today given callers are serialized, but the whole point of doing this at the store level is to not depend on that.

Two minor notes, no action needed unless easy: the key_locks map grows unboundedly (one entry per key ever touched), fine at our scale, maybe worth a comment; and since the batch override goes through the same block_on helper as everything else, dropping the dedicated runtime (other thread) will carry it along automatically, no coordination needed between the two changes.

On the read-modify-write gap: agreed it exists and agreed it's currently closed by caller serialization (peer_state on the LDK side, the single-threaded event handler on the RLN side). Your proposed update method follows the same shape as execute_batch, which tells me introducing it later won't change anything structural, so let's defer it and not grow this PR further. What I'd like now instead is to make the invariant explicit: a short comment on update_rgb_channel_amount and write_rgb_payment_info stating that callers must be externally serialized per key, so whoever adds a new caller knows what they're signing up for.

About the test, I think you can add a restart to the close_force_pending_htlc test that you recently added in RGB-Tools/rgb-lightning-node#133

[dcorral]

All done:

• batch staleness is now all-or-nothing: after taking all the key locks, if any touched key has a newer applied version the whole batch is skipped; the per-op skips are gone. Added a unit test where one stale key voids the whole batch.
• key_locks now documents that entries are intentionally never pruned: the stored version is the key's staleness watermark, dropping an entry would let a delayed older write reapply.
• added the serialization comments on update_rgb_channel_amount and write_rgb_payment_info.
• close_force_pending_htlc now restarts the payer while the colored HTLC is pending and waits for the channel to re-establish before the force close.

Agree on the batch picking up the runtime change automatically however, see the other thread first, since removing the dedicated runtime hit an sqlx-level problem.

[zoedberg]

close_force_pending_htlc now restarts the payer while the colored HTLC is pending and waits for the channel to re-establish before the force close.

@dcorral we discussed that test and related fix should be in a separate PR (and into 2 separate commits) since this is a pre-existing bug

[zoedberg]

@dcorral please check the comments in the threads and rebase on top of the latest rgb tip. I think we are almost done, in next iteration if something is not perfect I will just add a commit on top if that's ok for you