Skip to content

feat(platform-wallet): instrument wallet_manager RwLock behind lock-stats feature #3557

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
QuantumExplorer wants to merge 1 commit into
base: v3.1-dev
Choose a base branch
from
Open

feat(platform-wallet): instrument wallet_manager RwLock behind lock-stats feature #3557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 15 additions & 0 deletions packages/rs-platform-wallet/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ image = { version = "0.25", default-features = false, features = ["png", "jpeg",
# Security
zeroize = "1"

# Sync primitives used by the optional `lock-stats` feature for the
# per-tag breakdown map. Plain `parking_lot::Mutex` (sync, fast) is the
# right shape here — the per-tag map is touched on the lock acquire /
# release path, never across an `.await`.
parking_lot = { version = "0.12", optional = true }

# Shielded pool (optional, behind `shielded` feature)
grovedb-commitment-tree = { git = "https://github.com/dashpay/grovedb", rev = "8f25b20d04bfc0e8bdfb3870676d647a0d74918b", optional = true }
zip32 = { version = "0.2.0", default-features = false, optional = true }
Expand All @@ -62,3 +68,12 @@ default = ["bls", "eddsa"]
bls = ["key-wallet/bls", "key-wallet-manager/bls"]
eddsa = ["key-wallet/eddsa", "key-wallet-manager/eddsa"]
shielded = ["dep:grovedb-commitment-tree", "dep:zip32", "dash-sdk/shielded", "dpp/shielded-client"]

# Off by default. When enabled, the per-wallet `wallet_manager` RwLock is
# wrapped with an `InstrumentedRwLock` that records acquisition counts,
# wait time, and hold time per call site (using `read_at("tag")` /
# `write_at("tag")`). With the feature off the wrapper is a transparent
# type alias for `tokio::sync::RwLock` and there is zero runtime cost.
Comment on lines +75 to +76
Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Nitpick: Cargo.toml comment misstates the off-mode shape as a 'transparent type alias'

The comment says: 'With the feature off the wrapper is a transparent type alias for tokio::sync::RwLock and there is zero runtime cost.' InstrumentedRwLock<T> is always a struct holding inner: Arc<TokioRwLock<T>>, even with the feature off — it's a thin newtype, not a type alias, and Arc<InstrumentedRwLock<T>> adds an extra Arc indirection vs. plain Arc<TokioRwLock<T>>. The PR description's 'byte-for-byte identical after inlining' framing is accurate; the Cargo.toml summary should mirror it (e.g. 'a thin newtype around tokio::sync::RwLock that compiles down to direct delegation').

source: ['claude']

# See `crate::diagnostics::instrumented_lock` for the API and
# `LockStats` for the snapshot shape.
lock-stats = ["dep:parking_lot"]
28 changes: 22 additions & 6 deletions packages/rs-platform-wallet/src/changeset/core_bridge.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,17 @@ use key_wallet::transaction_checking::TransactionContext;
use key_wallet::Utxo;
use key_wallet_manager::{WalletEvent, WalletId, WalletManager};
use tokio::sync::broadcast::error::RecvError;
use tokio::sync::RwLock;
use tokio::task::JoinHandle;
use tokio_util::sync::CancellationToken;

use crate::changeset::changeset::{CoreChangeSet, PlatformWalletChangeSet};
use crate::changeset::traits::PlatformWalletPersistence;
// `InstrumentedRwLockExt` is unused when `lock-stats` is on — the
// wrapper has the same methods inherent and method resolution prefers
// inherent over trait. Suppress the warning so the call sites can
// import the trait unconditionally.
#[allow(unused_imports)]
use crate::diagnostics::{InstrumentedRwLock, InstrumentedRwLockExt};
use crate::wallet::platform_wallet::PlatformWalletInfo;

/// Spawn the wallet-event subscriber task.
Expand All @@ -45,13 +50,17 @@ use crate::wallet::platform_wallet::PlatformWalletInfo;
/// [`PlatformWalletPersistence::store`]. Exits when `cancel` fires
/// or the upstream broadcast channel closes.
pub fn spawn_wallet_event_adapter(
wallet_manager: Arc<RwLock<WalletManager<PlatformWalletInfo>>>,
wallet_manager: Arc<InstrumentedRwLock<WalletManager<PlatformWalletInfo>>>,
persister: Arc<dyn PlatformWalletPersistence>,
cancel: CancellationToken,
) -> JoinHandle<()> {
tokio::spawn(async move {
let mut receiver = {
let guard = wallet_manager.read().await;
// Subscribe-time read; happens once at task start. Tagged
// so the `lock-stats` feature can confirm the one-shot
// nature of this acquisition vs. the per-event probe in
// `is_chain_locked`.
let guard = wallet_manager.read_at("event_adapter::subscribe").await;
guard.subscribe_events()
};
tracing::debug!("WalletEventAdapter task started");
Expand Down Expand Up @@ -108,7 +117,7 @@ pub fn spawn_wallet_event_adapter(
/// Project an upstream [`WalletEvent`] into a [`CoreChangeSet`] suitable
/// for atomic persistence.
async fn build_core_changeset(
wallet_manager: &Arc<RwLock<WalletManager<PlatformWalletInfo>>>,
wallet_manager: &Arc<InstrumentedRwLock<WalletManager<PlatformWalletInfo>>>,
event: &WalletEvent,
) -> CoreChangeSet {
match event {
Expand Down Expand Up @@ -172,11 +181,18 @@ async fn build_core_changeset(
/// Returns `true` when the wallet's stored record for `txid` is in a
/// chain-locked block. Used to gate IS-lock projection.
async fn is_chain_locked(
wallet_manager: &Arc<RwLock<WalletManager<PlatformWalletInfo>>>,
wallet_manager: &Arc<InstrumentedRwLock<WalletManager<PlatformWalletInfo>>>,
wallet_id: &WalletId,
txid: &dashcore::Txid,
) -> bool {
let guard = wallet_manager.read().await;
// Tagged so the `lock-stats` feature can attribute this site's
// contribution to wallet-manager contention. The event adapter
// touches this lock once per `TransactionInstantLocked` event;
// tagging lets a perf audit distinguish the IS-lock finality
// probe from generic identity / token / address-sync reads.
let guard = wallet_manager
.read_at("event_adapter::is_chain_locked")
.await;
let Some(info) = guard.get_wallet_info(wallet_id) else {
return false;
};
Expand Down
Loading
Loading