feat(wallet): always emit ChainLockProcessed on chainlock advance#769
Open
shumkov wants to merge 6 commits into
Open
feat(wallet): always emit ChainLockProcessed on chainlock advance#769shumkov wants to merge 6 commits into
shumkov wants to merge 6 commits into
Conversation
Replace the BTreeMap return of `WalletInfoInterface::apply_chain_lock`
with an explicit `ApplyChainLockOutcome { per_account, metadata_advanced }`
so the wallet-manager-level emitter can fire one event per effect:
`TransactionsChainlocked` when records were promoted, and a separate
event whenever `last_applied_chain_lock` advanced (added in the next
commit).
The two effects fire independently — a quiescent wallet that sees a
chainlock above its history still advances the finality boundary even
though no record is promoted. Durable consumers that persist
`last_applied_chain_lock` (e.g. the platform-wallet bridge that uses
it to build a `ChainAssetLockProof` for `InBlock` asset-lock TXs on
restart) need a signal on every boundary advance, not just on the
promotion path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ance
Add a new `WalletEvent::ChainLockApplied { wallet_id, chain_lock }`
variant that fires whenever a wallet's `last_applied_chain_lock`
metadata advances forward (or moves from `None` to `Some`), and route
it through the FFI dispatcher.
`apply_chain_lock` now reads `ApplyChainLockOutcome` from the wallet
info and emits up to two events per wallet, in this order:
1. `ChainLockApplied` when the metadata advanced, so persisters that
need to mirror `last_applied_chain_lock` to durable storage have a
single hook regardless of whether any record was promoted. This is
the gap the platform-wallet bridge had to live with: a chainlock
that advanced the boundary without promoting anything was
completely invisible, and the persisted `last_applied_chain_lock`
went stale, which broke restart-time `ChainAssetLockProof`
construction for `InBlock` asset-lock TXs.
2. `TransactionsChainlocked` when at least one record was promoted
from `InBlock` to `InChainLockedBlock` (unchanged contract).
Ordering matters: `ChainLockApplied` fires FIRST so a persister
listening to both events can write durable metadata before the
promotion record.
FFI: adds `OnWalletChainLockAppliedCallback` and an
`on_chain_lock_applied` field appended to `FFIWalletEventCallbacks`
(before `user_data`) so existing offsets stay stable for C consumers.
Event-tests updated to reflect the new two-event contract on chainlock
promotion, the metadata-only advance path, and the higher-replay path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR introduces a new wallet event ChangesChainLock Event Separation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dash-spv-ffi/src/callbacks.rs (1)
1203-1218: ⚡ Quick winAdd a direct unit test for
ChainLockAppliedcallback dispatch.This new dispatch branch should have an explicit callback assertion (wallet id + height/hash/signature wiring) to prevent silent FFI regressions.
As per coding guidelines, "Write unit tests for new functionality".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dash-spv-ffi/src/callbacks.rs` around lines 1203 - 1218, Add a unit test that exercises the WalletEvent::ChainLockApplied branch and asserts the FFI callback receives the exact wallet id, block_height, block_hash and signature values: create a test that sets up a struct with on_chain_lock_applied pointing to a mock/tracing callback (capturing the C string pointer, height, hash bytes, sig bytes and user_data), construct a WalletEvent::ChainLockApplied with a known wallet_id and a ChainLock containing a known block_height, a 32-byte block_hash and a 96-byte signature, invoke the same event dispatcher that contains the match on WalletEvent::ChainLockApplied (the code that references self.on_chain_lock_applied), and assert the captured values match the originals (decode the C string pointer back to bytes/string and compare raw arrays for hash/signature and the height and wallet_id), failing the test if any value is miswired to catch regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dash-spv-ffi/src/callbacks.rs`:
- Around line 886-889: The new field on_chain_lock_applied was inserted before
user_data which changes the ABI layout despite the comment promise; move
on_chain_lock_applied to after user_data to preserve the previous ABI offset for
user_data, update the struct in callbacks.rs accordingly (referencing the field
names on_chain_lock_applied and user_data), and ensure the dispatch path in
dispatch() correctly invokes the callback for WalletEvent::ChainLockApplied;
also add a unit test mirroring the existing test_blocks_needed_dispatch_* tests
to cover ChainLockApplied dispatch so behavior is verified.
---
Nitpick comments:
In `@dash-spv-ffi/src/callbacks.rs`:
- Around line 1203-1218: Add a unit test that exercises the
WalletEvent::ChainLockApplied branch and asserts the FFI callback receives the
exact wallet id, block_height, block_hash and signature values: create a test
that sets up a struct with on_chain_lock_applied pointing to a mock/tracing
callback (capturing the C string pointer, height, hash bytes, sig bytes and
user_data), construct a WalletEvent::ChainLockApplied with a known wallet_id and
a ChainLock containing a known block_height, a 32-byte block_hash and a 96-byte
signature, invoke the same event dispatcher that contains the match on
WalletEvent::ChainLockApplied (the code that references
self.on_chain_lock_applied), and assert the captured values match the originals
(decode the C string pointer back to bytes/string and compare raw arrays for
hash/signature and the height and wallet_id), failing the test if any value is
miswired to catch regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b06f31c1-731f-482c-b22a-d7be36ebcfd6
📒 Files selected for processing (8)
dash-spv-ffi/src/bin/ffi_cli.rsdash-spv-ffi/src/callbacks.rskey-wallet-manager/src/event_tests.rskey-wallet-manager/src/events.rskey-wallet-manager/src/process_block.rskey-wallet-manager/src/wallet_interface.rskey-wallet/src/tests/keep_finalized_transactions_tests.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
shumkov
changed the title
…ied dispatch CodeRabbit caught that the new `on_chain_lock_applied` field was inserted before `user_data` in `FFIWalletEventCallbacks`, which shifted `user_data`'s byte offset and contradicted the layout-stability claim in the doc comment — C-side consumers hand-allocating this struct from older headers (i.e. without regenerating via cbindgen) rely on `user_data` staying where it was. Move the new field strictly to the end so every prior field — including `user_data` — keeps its previous offset, and rewrite the field's doc comment to reflect the new placement and the actual ABI guarantee. Also adds the `ChainLockApplied` dispatch unit test CodeRabbit suggested as a nice-to-have, sitting alongside the existing dispatch tests at the bottom of `callbacks.rs`. The test registers an `extern "C"` callback, fires a `WalletEvent::ChainLockApplied` carrying a synthetic `ChainLock::dummy(777)`, and asserts the callback received `cl_height == 777` — pinning the dispatch path against silent regressions. Refs: #769 (comment) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…xture Appending `on_chain_lock_applied` to `FFIWalletEventCallbacks` broke the integration-test fixture in `dashd_sync/callbacks.rs`, which constructs the struct with all fields named explicitly (no `..Default::default()` spread). CI under `cargo test -p dash-spv-ffi --all-features` failed with `error[E0063]: missing field on_chain_lock_applied`, which cascaded into every job that builds the FFI test suite (ffi matrix, pre-commit, address-sanitizer). The new wallet-event tests don't need this callback, so wire it as `None` — same shape as the freshly-added `on_transactions_chainlocked` slot right above it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #769 +/- ##
=============================================
+ Coverage 72.62% 72.68% +0.05%
=============================================
Files 320 320
Lines 70254 70333 +79
=============================================
+ Hits 51022 51119 +97
+ Misses 19232 19214 -18
|
xdustinface
requested changes
May 15, 2026
Collaborator
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
I think we should keep it atomic here too and just modify and reuse TransactionsChainlocked chainlock event. Like:
- Rename
TransactionsChainlocked->ChainLockProcessed - Rename
per_account->locked_transactions - Just always emit when
metadata_advancedwith empty transactoins if none were updated.
Two `assert_eq!` calls added earlier on this branch (commits 3a6e452 / de28922) exceed the workspace rustfmt width and now fail the `cargo fmt` pre-commit hook on macOS/Ubuntu/Windows. Straight `cargo fmt --all` output — no behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…cessed Per @xdustinface's review on #769: keep chainlock fan-out atomic by collapsing the two-event split (`ChainLockApplied` + `TransactionsChainlocked`) into a single `WalletEvent::ChainLockProcessed`, fired whenever the wallet's `last_applied_chain_lock` advances. Net-new per-account promotions ride along in `locked_transactions` — possibly empty when the chainlock advanced the metadata boundary without promoting any record (durable consumers that persist the chainlock proof still observe those empty-promotion events). Surface changes: * `WalletEvent::TransactionsChainlocked` → `WalletEvent::ChainLockProcessed` * `ApplyChainLockOutcome.per_account` → `locked_transactions` (matching the event field name; outcome and event speak the same vocabulary) * `WalletEvent::ChainLockApplied` removed entirely * `OnWalletTransactionsChainlockedCallback` → `OnWalletChainLockProcessedCallback`, signature unchanged * `FFIWalletEventCallbacks.on_transactions_chainlocked` → `on_chain_lock_processed`; the separate `on_chain_lock_applied` slot is gone, so `FFIWalletEventCallbacks` shrinks by one callback pointer (a hard ABI break for the unreleased addition, fine since v0.42 hasn't shipped) * `ffi_cli`'s `on_wallet_transactions_chainlocked` printer renamed `on_wallet_chain_lock_processed`; emits "[Wallet] ChainLock processed" Emission semantics: `process_block.rs` now sends one event per wallet per chainlock, gated on `outcome.metadata_advanced`. Replays of the same chainlock height are silent. Tests in `key-wallet-manager/src/event_tests.rs` and `key-wallet/src/tests/keep_finalized_transactions_tests.rs` updated to the merged shape, including the empty-`locked_transactions` case for chainlocks that advance the boundary without promoting any record. Dispatch test coverage strengthened per CodeRabbit's nitpick on #769#discussion (1203-1218): replaces the height-only assertion with two tests at the bottom of `dash-spv-ffi/src/callbacks.rs`: * `test_chain_lock_processed_dispatch_round_trips_every_field` — registers an `extern "C"` callback, captures every wired argument into a typed `Captured` struct, fires a `ChainLockProcessed` with two distinct accounts (one txid each), and asserts wallet_id hex-encoding, height, 32-byte block hash, 96-byte signature, and `finalized_count == 2` (counts (account, txid) pairs, not accounts). * `test_chain_lock_processed_dispatch_fires_with_empty_promotions` — empty `locked_transactions` must still fire the callback with `finalized_count == 0`; pins the contract for durable consumers that persist the chainlock proof even when no record was promoted. Closes the design pushback on #769; addresses the dispatch-test nitpick in the same review. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Applied as you proposed, in 73fb21c:
FFI side renamed in lockstep: callback is now |
xdustinface
reviewed
May 15, 2026
| /// fire the callback (durable consumers persist the chainlock proof | ||
| /// even when no record was promoted) with `finalized_count == 0`. | ||
| #[test] | ||
| fn test_chain_lock_processed_dispatch_fires_with_empty_promotions() { |
Collaborator
There was a problem hiding this comment.
This test doesn't really make sense i think? Its testing the dispatch function?
shumkov
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Make wallet chainlock events atomic and emit one on every metadata advance, even when no transaction record was promoted.
Concretely:
WalletEvent::TransactionsChainlocked→WalletEvent::ChainLockProcessed.per_accountfield →locked_transactions(and the underlyingApplyChainLockOutcome.per_accountinkey-wallet, so outcome and event share one name).ChainLockProcessedper wallet whenever the wallet'slast_applied_chain_lockadvances — promotions ride along inlocked_transactions, which is empty when the chainlock advanced the boundary without flipping any record.Before this PR, the event only fired when at least one record flipped
InBlock→InChainLockedBlock. A chainlock that landed on a quiescent wallet (noInBlockrecords at heights<= cl_height) advanced the in-memorymetadata.last_applied_chain_lockbut produced no event, so durable consumers had no signal to flush the new height to disk.Why this matters downstream
dashpay/platform'srs-platform-walletships a fast-path on app restart: if a persisted asset-lock transaction is stillInBlockand the persistedlast_applied_chain_lockhas a height>= record.height, build aChainAssetLockProofdirectly from the persisted chainlock signature instead of waiting for SPV to re-deliver one.That fast-path is reliable only if the persisted chainlock is current. Pre-this-PR, sessions that received chainlocks without promoting records left the persister stale, so the fast-path missed on cold restart. See dashpay/platform#3634 for context.
Design choice
An earlier iteration added a second variant (
ChainLockApplied) alongside the existingTransactionsChainlocked, with an explicit before-after ordering between the two. @xdustinface pushed back: keep the chainlock fan-out atomic and reuse one event. This PR now reflects that — a singleChainLockProcessedper advance, withlocked_transactionspossibly empty.Changes
key-walletApplyChainLockOutcomeretains bothmetadata_advanced: boolandlocked_transactions: BTreeMap<AccountType, Vec<Txid>>(renamed fromper_account).WalletInfoInterface::apply_chain_lockstill returns it; the default impl returnsApplyChainLockOutcome::default();ManagedWalletInfopopulates both fields.key-wallet-managerWalletEvent::TransactionsChainlocked→WalletEvent::ChainLockProcessed { wallet_id, chain_lock, locked_transactions }.process_block::apply_chain_lockemits one event per wallet gated onoutcome.metadata_advanced; replays at the same height are silent.Display+wallet_id()arms updated; trait doc onapply_chain_lockrewritten.dash-spv-ffiOnWalletTransactionsChainlockedCallback→OnWalletChainLockProcessedCallback(same signature: wallet_id, height, hash, signature, finalized array, count, user_data).FFIWalletEventCallbacks.on_transactions_chainlocked→on_chain_lock_processed. The struct shrinks by one fn pointer (the abandonedon_chain_lock_appliedslot from the earlier two-event design is removed).on_wallet_chain_lock_processed) and emits ‟[Wallet] ChainLock processed”.Backwards compatibility
Breaking at the public surface — variant renamed, field renamed, callback type & field renamed. This is acceptable here because v0.42 is unreleased; downstream consumers regenerated against this branch pick up the rename via cbindgen and the compiler.
Test plan
cargo check --workspace --all-targets --all-featurescargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --checkcargo test -p key-wallet --lib(476 passed)cargo test -p key-wallet-manager --lib(45 passed — chainlock event tests updated to the merged shape, including the empty-locked_transactionscase for advances that promote nothing)cargo test -p dash-spv-ffi --lib(46 passed — two new dispatch tests pin every wired field plus the empty-promotion path)🤖 Generated with Claude Code