refactor(key-wallet-manager): track wallet heights per wallet#689
Merged
xdustinface merged 2 commits intov0.42-devfrom Apr 27, 2026
Merged
refactor(key-wallet-manager): track wallet heights per wallet#689xdustinface merged 2 commits intov0.42-devfrom
xdustinface merged 2 commits intov0.42-devfrom
Conversation
Contributor
📝 WalkthroughWalkthroughWalletManager removed its own block-height fields and computes heights from per-wallet infos; WalletInfo gained persistent Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant WalletManager
participant WalletInfoA
participant WalletInfoB
Caller->>WalletManager: update_last_processed_height(h)
WalletManager->>WalletInfoA: update_last_processed_height(h)
WalletManager->>WalletInfoB: update_last_processed_height(h)
Note right of WalletInfoA: metadata.last_processed_height updated
Note right of WalletInfoB: metadata.last_processed_height updated
Caller->>WalletManager: update_synced_height(s)
WalletManager->>WalletInfoA: update_synced_height(s)
WalletManager->>WalletInfoB: update_synced_height(s)
Note right of WalletInfoA: metadata.synced_height updated
Note right of WalletInfoB: metadata.synced_height updated
Caller->>WalletManager: last_processed_height()
WalletManager->>WalletInfoA: last_processed_height()
WalletManager->>WalletInfoB: last_processed_height()
WalletInfoA-->>WalletManager: hA
WalletInfoB-->>WalletManager: hB
WalletManager-->>Caller: max(hA, hB) (0 if no wallets)
Caller->>WalletManager: synced_height()
WalletManager->>WalletInfoA: synced_height()
WalletManager->>WalletInfoB: synced_height()
WalletInfoA-->>WalletManager: sA
WalletInfoB-->>WalletManager: sB
WalletManager-->>Caller: min(sA, sB) (0 if no wallets)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
xdustinface
force-pushed
the
refactor/heights-per-wallet
branch
from
97abf06 to
032d2af
Compare
![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: 3
🧹 Nitpick comments (7)
key-wallet-manager/src/accessors.rs (1)
14-23: Duplicated aggregation logic.These inherent methods duplicate the bodies of the
WalletInterfaceimpl inkey-wallet-manager/src/process_block.rs(Lines 100-102 and 114-116). Two copies of the same aggregation semantics risk drift if one is changed without the other. Consider having the inherent methods delegate to the trait impl (or vice versa) so there’s a single source of truth.🔧 Example refactor
- /// Return the highest last-processed height across all managed wallets. - pub fn last_processed_height(&self) -> CoreBlockHeight { - self.wallet_infos.values().map(|info| info.last_processed_height()).max().unwrap_or(0) - } - - /// Return the lowest durable sync checkpoint height across all managed wallets. - /// This is the conservative floor used as the filter-sync resume point. - pub fn synced_height(&self) -> CoreBlockHeight { - self.wallet_infos.values().map(|info| info.synced_height()).min().unwrap_or(0) - } + /// Return the highest last-processed height across all managed wallets. + pub fn last_processed_height(&self) -> CoreBlockHeight { + <Self as WalletInterface>::last_processed_height(self) + } + + /// Return the lowest durable sync checkpoint height across all managed wallets. + /// This is the conservative floor used as the filter-sync resume point. + pub fn synced_height(&self) -> CoreBlockHeight { + <Self as WalletInterface>::synced_height(self) + }(Requires
use crate::WalletInterface;— and works because the trait impl inprocess_block.rsalready applies toWalletManager<T>with the same bounds.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/accessors.rs` around lines 14 - 23, The two inherent methods last_processed_height and synced_height duplicate aggregation logic that's already implemented in the WalletInterface impl; change them to delegate to the trait implementation to keep a single source of truth (or alternatively move the trait impl to call these inherent methods, but prefer delegation). Import the trait with use crate::WalletInterface and have the inherent methods call the trait methods on self (i.e., use the WalletInterface::last_processed_height and WalletInterface::synced_height implementations for WalletManager<T>) so only the trait impl holds the aggregation semantics.key-wallet-manager/tests/integration_test.rs (1)
161-246: LGTM — thorough coverage of the new semantics.The test validates both directions of the aggregation (max for last-processed, propagation for synced) and the pre-wallet no-op case. One optional addition: after Line 245, you could directly verify the min semantics of
synced_height()by diverging the wallets individually (e.g.,wallet_info1.update_synced_height(50000)) and then assertingmanager.synced_height() == 40000. That pins the aggregation choice in the test so future changes to min-vs-max get caught.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/tests/integration_test.rs` around lines 161 - 246, Add an assertion that verifies manager.synced_height() aggregates as the minimum across wallets: after the final manager.update_synced_height(40000) call, get a mutable reference to one wallet (get_wallet_info_mut(&wallet_id1)) and call update_synced_height(50000) on it, then assert that manager.synced_height() still equals 40000 while wallet_info1.synced_height() == 50000 and wallet_info2.synced_height() == 40000; this pins the min-aggregation semantic for synced_height().key-wallet-manager/examples/wallet_creation.rs (1)
142-149: Optional: demonstratesynced_heightalongside last-processed height.The example now clearly distinguishes “last-processed height,” but since this PR introduces a separate per-wallet synced-height accessor/updater, showing both in the same section would give consumers a clearer mental model of the new API split.
🔧 Proposed addition
println!(" Current last-processed height (Testnet): {:?}", manager.last_processed_height()); // Update last-processed height across all managed wallets manager.update_last_processed_height(850_000); println!(" Updated last-processed height to: {:?}", manager.last_processed_height()); + + // Synced (durable filter-scan) checkpoint is tracked independently + manager.update_synced_height(849_000); + println!(" Updated synced height to: {:?}", manager.synced_height());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/examples/wallet_creation.rs` around lines 142 - 149, Show the new per-wallet synced-height alongside the global last-processed height: after printing manager.last_processed_height() also iterate the managed wallet IDs and print manager.synced_height(wallet_id) for each; then call manager.update_synced_height(wallet_id, new_value) for one or more wallets (e.g. the same 850_000) and re-print both manager.last_processed_height() and each manager.synced_height(wallet_id) to demonstrate the API split. Reference the existing methods manager.last_processed_height() and manager.update_last_processed_height(…) and the new per-wallet methods manager.synced_height(wallet_id) and manager.update_synced_height(wallet_id, value).key-wallet-manager/src/process_block.rs (2)
215-228: Test coverage regression for populated manager.The old version of this test presumably validated that
update_last_processed_heightactually propagates to wallet state. Now every assertion is0because there are no wallets to update. The non-trivial path is only exercised inkey-wallet-manager/tests/integration_test.rs::test_block_height_tracking, which is fine, but consider either renaming this totest_last_processed_height_empty_managerfor accuracy, or re-adding a wallet to make it a true round-trip test (as in the integration test).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/process_block.rs` around lines 215 - 228, The test test_last_processed_height currently exercises only an empty manager so last_processed_height stays 0; fix by making it a true round-trip: instantiate a ManagedWalletInfo (or a test wallet) and add it to the WalletManager before calling update_last_processed_height so that WalletManager::update_last_processed_height actually propagates to wallets and changes manager.last_processed_height(); update assertions to expect the propagated heights (e.g., after update_last_processed_height(1000) assert non-zero/new height), or alternatively rename the test to test_last_processed_height_empty_manager if you prefer to keep the empty-manager behavior.
118-122: Consider emitting wallet events on synced-height advance (and enforcing monotonicity).
update_last_processed_heightcarefully snapshots/emits balance changes;update_synced_heightjust forwards blindly. Two things worth considering:
- Because
ManagedWalletInfo::update_synced_height(inwallet_info_interface.rs) overwrites unconditionally, a caller passing a lower value here will silently pull the per-wallet checkpoints backwards — and the manager-level min will follow. Guarding at this entry point would prevent the whole wallet set from regressing due to one bad call.- No event is emitted when the sync checkpoint advances. If downstream consumers care about filter-sync progress (e.g., for UI or persistence), consider adding a dedicated event akin to
BalanceUpdated. Not required for this PR, but worth a follow-up.🔧 Suggested guard
fn update_synced_height(&mut self, height: CoreBlockHeight) { for (_wallet_id, info) in self.wallet_infos.iter_mut() { - info.update_synced_height(height); + if height > info.synced_height() { + info.update_synced_height(height); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/process_block.rs` around lines 118 - 122, The manager's update_synced_height currently forwards the new CoreBlockHeight to each ManagedWalletInfo::update_synced_height unconditionally — change update_synced_height (in process_block.rs) to first compare the incoming height against each wallet's current synced height and only call ManagedWalletInfo::update_synced_height when the new height is greater (enforce monotonicity to avoid regressing per-wallet checkpoints and the manager-level minimum); additionally, consider emitting a sync-progress event (similar to BalanceUpdated) when a wallet's synced height is advanced so downstream consumers can observe checkpoint changes (this event emission can mirror the snapshot/emit pattern used in update_last_processed_height).key-wallet-ffi/src/wallet_manager_tests.rs (1)
434-464: Test no longer exercises the non-trivial path.After the refactor this test only asserts the no-op-when-empty behavior, which duplicates coverage already in
key-wallet-manager/tests/integration_test.rs::test_block_height_tracking. Given the FFI layer is the one consumers actually call, consider adding a wallet before theupdate_last_processed_heightcall so thatwallet_manager_current_heightactually reflects propagation through the FFI boundary. As-is, a regression in the FFI height wiring could pass this test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/wallet_manager_tests.rs` around lines 434 - 464, The test test_wallet_manager_synced_height currently only verifies the no-op path; before calling manager_ref.manager.write().await -> update_last_processed_height(new_height) add a wallet through the FFI layer so the update exercises the non-trivial propagation path: create/add a wallet using the existing FFI add-wallet function (call it on the same manager returned by wallet_manager_create), assert the wallet was added (non-null/ok), then call update_last_processed_height(new_height) and assert wallet_manager_current_height(manager, error) == new_height; finally clean up with wallet_manager_free(manager). Ensure you reference the same manager variable and error pointer used with wallet_manager_create/current_height/free so the FFI wiring is exercised.key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (1)
92-100: UseCoreBlockHeighttype for consistency in parameter and return signatures.
synced_height()returnsCoreBlockHeightbutupdate_synced_height()accepts a bareu32. For consistency with the type aliasing used elsewhere and to matchupdate_last_processed_height's convention, type both parameters asCoreBlockHeight.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs` around lines 92 - 100, The interface is inconsistent: synced_height() returns CoreBlockHeight but update_last_processed_height() and update_synced_height() accept u32; change both method parameter types to CoreBlockHeight so all signatures consistently use the CoreBlockHeight alias (update_last_processed_height(&mut self, current_height: CoreBlockHeight) and update_synced_height(&mut self, current_height: CoreBlockHeight)) while leaving synced_height() as CoreBlockHeight.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet-manager/src/lib.rs`:
- Around line 301-302: Newly added wallets are being left with synced_height=0
which regresses WalletManager::synced_height() to 0; for the wallet
creation/import paths create_wallet_with_random_mnemonic,
import_wallet_from_extended_priv_key, import_wallet_from_xpub, and
import_wallet_from_bytes, set the wallet's synced_height to the manager's
last_processed_height() (the same value used for managed_info.set_birth_height)
instead of leaving it at 0 by calling
managed_info.set_synced_height(self.last_processed_height()) right after setting
birth_height and first_loaded_at; keep the existing behavior for
create_wallet_from_mnemonic where an explicit birth_height is provided (leave
synced_height=0) and document that difference to avoid accidental regressions.
In `@key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs`:
- Around line 99-100: The update_synced_height implementation in
ManagedWalletInfo currently unconditionally overwrites metadata.synced_height
and can regress the checkpoint; change it to enforce monotonicity by only
updating metadata.synced_height when current_height > metadata.synced_height (or
document that callers must always pass non-decreasing heights). Update the
ManagedWalletInfo::update_synced_height method to compare the incoming
current_height with the stored metadata.synced_height and only assign and
persist when it advances, so accessors.rs::synced_height() never observes a
backward jump.
In `@key-wallet/src/wallet/metadata.rs`:
- Around line 21-22: The review points out inconsistent use of serde defaults on
WalletMetadata: either apply #[serde(default)] consistently to all fields or
remove it from synced_height; locate the WalletMetadata struct and either add
#[derive(Deserialize, Serialize)] with #[serde(default)] on the struct or add
#[serde(default)] to each field (first_loaded_at, birth_height,
last_processed_height, last_synced, total_transactions, version, custom,
synced_height) so deserialization is uniform; update or add a short comment on
the struct indicating the chosen versioning strategy if you intentionally want
only synced_height to be tolerant to missing data.
---
Nitpick comments:
In `@key-wallet-ffi/src/wallet_manager_tests.rs`:
- Around line 434-464: The test test_wallet_manager_synced_height currently only
verifies the no-op path; before calling manager_ref.manager.write().await ->
update_last_processed_height(new_height) add a wallet through the FFI layer so
the update exercises the non-trivial propagation path: create/add a wallet using
the existing FFI add-wallet function (call it on the same manager returned by
wallet_manager_create), assert the wallet was added (non-null/ok), then call
update_last_processed_height(new_height) and assert
wallet_manager_current_height(manager, error) == new_height; finally clean up
with wallet_manager_free(manager). Ensure you reference the same manager
variable and error pointer used with wallet_manager_create/current_height/free
so the FFI wiring is exercised.
In `@key-wallet-manager/examples/wallet_creation.rs`:
- Around line 142-149: Show the new per-wallet synced-height alongside the
global last-processed height: after printing manager.last_processed_height()
also iterate the managed wallet IDs and print manager.synced_height(wallet_id)
for each; then call manager.update_synced_height(wallet_id, new_value) for one
or more wallets (e.g. the same 850_000) and re-print both
manager.last_processed_height() and each manager.synced_height(wallet_id) to
demonstrate the API split. Reference the existing methods
manager.last_processed_height() and manager.update_last_processed_height(…) and
the new per-wallet methods manager.synced_height(wallet_id) and
manager.update_synced_height(wallet_id, value).
In `@key-wallet-manager/src/accessors.rs`:
- Around line 14-23: The two inherent methods last_processed_height and
synced_height duplicate aggregation logic that's already implemented in the
WalletInterface impl; change them to delegate to the trait implementation to
keep a single source of truth (or alternatively move the trait impl to call
these inherent methods, but prefer delegation). Import the trait with use
crate::WalletInterface and have the inherent methods call the trait methods on
self (i.e., use the WalletInterface::last_processed_height and
WalletInterface::synced_height implementations for WalletManager<T>) so only the
trait impl holds the aggregation semantics.
In `@key-wallet-manager/src/process_block.rs`:
- Around line 215-228: The test test_last_processed_height currently exercises
only an empty manager so last_processed_height stays 0; fix by making it a true
round-trip: instantiate a ManagedWalletInfo (or a test wallet) and add it to the
WalletManager before calling update_last_processed_height so that
WalletManager::update_last_processed_height actually propagates to wallets and
changes manager.last_processed_height(); update assertions to expect the
propagated heights (e.g., after update_last_processed_height(1000) assert
non-zero/new height), or alternatively rename the test to
test_last_processed_height_empty_manager if you prefer to keep the empty-manager
behavior.
- Around line 118-122: The manager's update_synced_height currently forwards the
new CoreBlockHeight to each ManagedWalletInfo::update_synced_height
unconditionally — change update_synced_height (in process_block.rs) to first
compare the incoming height against each wallet's current synced height and only
call ManagedWalletInfo::update_synced_height when the new height is greater
(enforce monotonicity to avoid regressing per-wallet checkpoints and the
manager-level minimum); additionally, consider emitting a sync-progress event
(similar to BalanceUpdated) when a wallet's synced height is advanced so
downstream consumers can observe checkpoint changes (this event emission can
mirror the snapshot/emit pattern used in update_last_processed_height).
In `@key-wallet-manager/tests/integration_test.rs`:
- Around line 161-246: Add an assertion that verifies manager.synced_height()
aggregates as the minimum across wallets: after the final
manager.update_synced_height(40000) call, get a mutable reference to one wallet
(get_wallet_info_mut(&wallet_id1)) and call update_synced_height(50000) on it,
then assert that manager.synced_height() still equals 40000 while
wallet_info1.synced_height() == 50000 and wallet_info2.synced_height() == 40000;
this pins the min-aggregation semantic for synced_height().
In `@key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs`:
- Around line 92-100: The interface is inconsistent: synced_height() returns
CoreBlockHeight but update_last_processed_height() and update_synced_height()
accept u32; change both method parameter types to CoreBlockHeight so all
signatures consistently use the CoreBlockHeight alias
(update_last_processed_height(&mut self, current_height: CoreBlockHeight) and
update_synced_height(&mut self, current_height: CoreBlockHeight)) while leaving
synced_height() as CoreBlockHeight.
🪄 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: 2cc5b8c1-21cb-41e3-97ab-bcf124e95a24
📒 Files selected for processing (8)
key-wallet-ffi/src/wallet_manager_tests.rskey-wallet-manager/examples/wallet_creation.rskey-wallet-manager/src/accessors.rskey-wallet-manager/src/lib.rskey-wallet-manager/src/process_block.rskey-wallet-manager/tests/integration_test.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rskey-wallet/src/wallet/metadata.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #689 +/- ##
=============================================
+ Coverage 70.03% 70.45% +0.41%
=============================================
Files 319 319
Lines 66611 66613 +2
=============================================
+ Hits 46654 46931 +277
+ Misses 19957 19682 -275
|
ZocoLini
requested changes
Apr 24, 2026
| pub fn synced_height(&self) -> CoreBlockHeight { | ||
| self.wallet_infos.values().map(|info| info.synced_height()).min().unwrap_or(0) | ||
| } | ||
|
|
Collaborator
There was a problem hiding this comment.
Aren't this methods already defined and implemented in the impl WalletInterface block?? I am confused
Collaborator
Author
There was a problem hiding this comment.
They are but they were not accessible in few places because of missing trait bounds. See 055dfc9.
This was referenced Apr 25, 2026
Move committed sync height into wallet metadata and expose it through `WalletInfoInterface` so `WalletManager` no longer relies on global cached heights. Compute manager-level synced_height and tip_height as the minimum across wallet infos, propagate committed sync updates per wallet, and keep tip_height as per-wallet applied chain state. Update tests and example messaging to match the new aggregation semantics.
xdustinface
force-pushed
the
refactor/heights-per-wallet
branch
from
055dfc9 to
4a4547d
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
key-wallet-manager/src/process_block.rs (1)
215-228: Test only exercises the empty-manager path; consider covering the multi-wallet aggregation.
test_last_processed_heightnow only validates that calls on an empty manager are a no-op. The new behavior of interest —maxover per-walletlast_processed_height()and the per-wallet forwarding done byupdate_last_processed_height— is uncovered. A small extension that adds two wallets with different heights and checks the aggregated result would lock in the new semantics and protect against future regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/process_block.rs` around lines 215 - 228, Add a test case to exercise the non-empty manager aggregation: after the existing empty-manager assertions, construct two ManagedWalletInfo instances (or use existing helpers) with differing last_processed_height() values, insert them into the WalletManager via the same API used elsewhere (e.g., WalletManager::insert_wallet or however wallets are added in tests), call manager.update_last_processed_height(...) and assert that manager.last_processed_height() returns the max of the per-wallet heights; also verify that update_last_processed_height forwards the new height to each wallet by checking each wallet's stored last_processed_height() if test helpers expose it. Use the WalletManager::last_processed_height and WalletManager::update_last_processed_height symbols to locate the code to extend.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@key-wallet-manager/src/process_block.rs`:
- Around line 215-228: Add a test case to exercise the non-empty manager
aggregation: after the existing empty-manager assertions, construct two
ManagedWalletInfo instances (or use existing helpers) with differing
last_processed_height() values, insert them into the WalletManager via the same
API used elsewhere (e.g., WalletManager::insert_wallet or however wallets are
added in tests), call manager.update_last_processed_height(...) and assert that
manager.last_processed_height() returns the max of the per-wallet heights; also
verify that update_last_processed_height forwards the new height to each wallet
by checking each wallet's stored last_processed_height() if test helpers expose
it. Use the WalletManager::last_processed_height and
WalletManager::update_last_processed_height symbols to locate the code to
extend.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 10eead33-06c1-4cfc-927d-74c52fbcd8f1
📒 Files selected for processing (8)
key-wallet-ffi/src/wallet_manager_tests.rskey-wallet-manager/examples/wallet_creation.rskey-wallet-manager/src/accessors.rskey-wallet-manager/src/lib.rskey-wallet-manager/src/process_block.rskey-wallet-manager/tests/integration_test.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rskey-wallet/src/wallet/metadata.rs
✅ Files skipped from review due to trivial changes (2)
- key-wallet-ffi/src/wallet_manager_tests.rs
- key-wallet-manager/examples/wallet_creation.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- key-wallet/src/wallet/metadata.rs
- key-wallet-manager/src/accessors.rs
- key-wallet-manager/tests/integration_test.rs
xdustinface
added a commit
that referenced
this pull request
Apr 27, 2026
Filter matching and block processing now operate per wallet, so a wallet added at runtime catches up without forcing the already-synced wallets to reprocess anything. - `WalletInterface` restructured around per-wallet ops: `process_block_for_wallets`, `wallets_behind`, `monitored_addresses_for`, `wallet_synced_height`, and monotonic per-wallet height updates. Aggregate heights are derived (min of `synced_height`, max of `last_processed_height`) rather than stored. - `FiltersManager::scan_batch` matches each behind wallet's addresses only against filter heights it hasn't yet covered; already-synced wallets are skipped entirely. Matched blocks flow through `BlocksNeeded` carrying the per-block wallet set so `BlocksManager` processes each block only against the wallets whose filters matched. `FiltersBatch` records the scanned-wallet set so commit advances only their `synced_height`. - `FiltersManager::tick` detects when a wallet's `synced_height` sits below the current `committed_height` (a runtime add behind scan progress), clears in-flight pipeline state, lowers `committed_height` to the new aggregate floor, and re-enters `start_download` on the next 100ms tick. Runs in `Syncing`, `Synced`, and `WaitForEvents`. Based on: - #689
ZocoLini
approved these changes
Apr 27, 2026
Collaborator
ZocoLini
left a comment
ZocoLini
left a comment
There was a problem hiding this comment.
I no longer see the duplicated methods so I guess you managed
xdustinface
added a commit
that referenced
this pull request
Apr 27, 2026
Filter matching and block processing now operate per wallet, so a wallet added at runtime catches up without forcing the already-synced wallets to reprocess anything. - `WalletInterface` restructured around per-wallet ops: `process_block_for_wallets`, `wallets_behind`, `monitored_addresses_for`, `wallet_synced_height`, and monotonic per-wallet height updates. Aggregate heights are derived (min of `synced_height`, max of `last_processed_height`) rather than stored. - `FiltersManager::scan_batch` matches each behind wallet's addresses only against filter heights it hasn't yet covered; already-synced wallets are skipped entirely. Matched blocks flow through `BlocksNeeded` carrying the per-block wallet set so `BlocksManager` processes each block only against the wallets whose filters matched. `FiltersBatch` records the scanned-wallet set so commit advances only their `synced_height`. - `FiltersManager::tick` detects when a wallet's `synced_height` sits below the current `committed_height` (a runtime add behind scan progress), clears in-flight pipeline state, lowers `committed_height` to the new aggregate floor, and re-enters `start_download` on the next 100ms tick. Runs in `Syncing`, `Synced`, and `WaitForEvents`. Based on: - #689
xdustinface
added a commit
that referenced
this pull request
Apr 27, 2026
Filter matching and block processing now operate per wallet, so a wallet added at runtime catches up without forcing the already-synced wallets to reprocess anything. - `WalletInterface` restructured around per-wallet ops: `process_block_for_wallets`, `wallets_behind`, `monitored_addresses_for`, `wallet_synced_height`, and monotonic per-wallet height updates. Aggregate heights are derived (min of `synced_height`, max of `last_processed_height`) rather than stored. - `FiltersManager::scan_batch` matches each behind wallet's addresses only against filter heights it hasn't yet covered; already-synced wallets are skipped entirely. Matched blocks flow through `BlocksNeeded` carrying the per-block wallet set so `BlocksManager` processes each block only against the wallets whose filters matched. `FiltersBatch` records the scanned-wallet set so commit advances only their `synced_height`. - `FiltersManager::tick` detects when a wallet's `synced_height` sits below the current `committed_height` (a runtime add behind scan progress), clears in-flight pipeline state, lowers `committed_height` to the new aggregate floor, and re-enters `start_download` on the next 100ms tick. Runs in `Syncing`, `Synced`, and `WaitForEvents`. Based on: - #689
QuantumExplorer
added a commit
that referenced
this pull request
Apr 28, 2026
* feat: per-wallet filter scan and runtime wallet catch-up Filter matching and block processing now operate per wallet, so a wallet added at runtime catches up without forcing the already-synced wallets to reprocess anything. - `WalletInterface` restructured around per-wallet ops: `process_block_for_wallets`, `wallets_behind`, `monitored_addresses_for`, `wallet_synced_height`, and monotonic per-wallet height updates. Aggregate heights are derived (min of `synced_height`, max of `last_processed_height`) rather than stored. - `FiltersManager::scan_batch` matches each behind wallet's addresses only against filter heights it hasn't yet covered; already-synced wallets are skipped entirely. Matched blocks flow through `BlocksNeeded` carrying the per-block wallet set so `BlocksManager` processes each block only against the wallets whose filters matched. `FiltersBatch` records the scanned-wallet set so commit advances only their `synced_height`. - `FiltersManager::tick` detects when a wallet's `synced_height` sits below the current `committed_height` (a runtime add behind scan progress), clears in-flight pipeline state, lowers `committed_height` to the new aggregate floor, and re-enters `start_download` on the next 100ms tick. Runs in `Syncing`, `Synced`, and `WaitForEvents`. Based on: - #689 * chore: merge v0.42-dev into feat/per-wallet-filter-scan (#698) * chore(dash-spv): refresh masternode seed files (#695) Co-authored-by: QuantumExplorer <11468583+QuantumExplorer@users.noreply.github.com> * feat: make wallet events atomic (#696) * feat: make wallet events atomic Reshape `WalletEvent` so each variant carries the records or context needed to persist a wallet update atomically off a single event, alongside the post-change balance. The variant set is now: - `TransactionReceived { wallet_id, record, balance }`. Fires when the wallet first sees an off-chain transaction. - `TransactionStatusChanged { wallet_id, txid, context, balance }`. Fires when a known off-chain transaction has its state change. Currently fires only for InstantSend locks. - `BlockUpdate { wallet_id, height, inserted, updated, matured, balance }`. Carries records bucketed by what happened to them in the block, plus the post-block balance. - `SyncHeightUpdate { wallet_id, height }`. Marks a filter-batch checkpoint. `TransactionRecord` carries `account_type` directly, identifying the owning account. `WalletInfoInterface` gains a `matured_coinbase_records` method that enumerates coinbase records crossing the maturity threshold during a height advance, populating `BlockUpdate.matured`. The FFI groups the flattened account-discriminator fields into an `FFIAccountType` struct and renames the prior discriminant enum to `FFIAccountKind`. * fix: record balance before bumping `block_processed_wallet_count` Tests wait on `block_processed_wallet_count` and then read `last_confirmed`/`last_unconfirmed`. Bumping the counter before storing the balance snapshot left those reads racey. Reorder so the balance is recorded first. Addresses CodeRabbit review comment on PR #696 #696 (comment) * fix: place `IdentityTopUp.registration_index` in `index_secondary` The `FFIAccountType` doc states `index_secondary` carries `registration_index` for `IdentityTopUp` and `index = 0` for variants without a meaningful primary index, matching the parallel encoding in `FFIAccountKind::from_account_type`. The `From<&AccountType>` impl wrote `registration_index` into `index` instead, breaking the documented FFI contract. Addresses CodeRabbit review comment on PR #696 #696 (comment) * fix: route confirmation backfills to `new_records` `is_new` is wallet-wide (set on the first matching account, then breaks), so the per-account `else` branch can run for an account that did not previously hold the record. `confirm_transaction` backfills via `record_transaction` in that case, but the post-call record was always pushed onto `updated_records`, breaking the atomic `inserted`/`updated` contract consumed by `WalletEvent::BlockUpdate`. Capture `existed_before` per account and route to `new_records` when the record was just created. Addresses CodeRabbit review comment on PR #696 #696 (comment) * refactor(key-wallet-manager): extract `finalize_block_advance` helper `process_block` and `update_last_processed_height` duplicated the entire balance-snapshot, prior-heights collection, matured-coinbase window, height advance, and per-wallet `BlockUpdate` emission. Extract the shared tail into a private `WalletManager::finalize_block_advance` helper that takes the inserted/updated maps. `update_last_processed_height` becomes a one-line call with empty maps; `process_block` keeps only its txdata loop before delegating. * refactor: rename wallet events for clearer semantics Rename `WalletEvent` variants and the matching FFI callbacks to past-participle names that say what happened, replacing vague "Update" suffixes: - `TransactionReceived` -> `TransactionDetected`. "Received" implied incoming funds, but the event fires for any first-time off-chain sighting (incoming or outgoing). - `TransactionStatusChanged` -> `TransactionInstantLocked`. The event only ever fires for an InstantSend lock applied to a known mempool tx, so name it for what it actually is. Drop the `status: TransactionContext` field and carry the `InstantLock` directly. - `BlockUpdate` -> `BlockProcessed`. Mirrors `process_block` and matches the past-participle pattern. - `SyncHeightUpdate` -> `SyncHeightAdvanced`. Conveys monotonic forward motion. FFI rename mirrors the Rust side: the IS callback now takes `islock_data: *const u8` + `islock_len: usize` instead of an `FFITransactionContext`, removing a discriminant that was always `InstantSend`. The wallet-side `OnBlockProcessedCallback` becomes `OnWalletBlockProcessedCallback` to disambiguate from the existing sync-event type with the same name. * fix: record balance before bumping IS-locked counter in test callback Addresses CodeRabbit review comment on PR #696 #696 (review) The instant_locked callback bumped `transaction_instant_send_locked_count` before calling `record_balance`. Tests that wait on the counter and then read `last_confirmed`/`last_unconfirmed` could observe the previous balance snapshot. Match the ordering used by the other callbacks: store the balance first, then bump the counter. * fix: backfill missing transaction record in InstantSend path Addresses CodeRabbit review comment on PR #696 #696 (review) The IS-lock branch in `WalletTransactionChecker::check_core_transaction` only updated accounts that already held a `TransactionRecord` for the txid. When wallet-level `is_new` was `false` (because at least one account had the record) but another matched account did not, the latter was silently skipped: no record was created and `mark_utxos_instant_send` ran against an empty UTXO set on that account. Mirror the confirmation path: when the affected account lacks the record, call `record_transaction` to register the record and its UTXOs, then mark them IS-locked. This ordering ensures the freshly registered UTXOs receive the IS-lock flag too. The backfilled record is pushed into `new_records` to match the existing convention from commit 659a6d5. Add `test_instantsend_backfills_missing_record_in_other_account` covering the multi-account scenario. --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: QuantumExplorer <11468583+QuantumExplorer@users.noreply.github.com> Co-authored-by: Kevin Rombach <35775977+xdustinface@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: QuantumExplorer <quantum@dash.org> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: QuantumExplorer <11468583+QuantumExplorer@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Move committed sync height into wallet metadata and expose it through
WalletInfoInterfacesoWalletManagerno longer relies on global cached heights.Compute manager-level synced_height and tip_height as the minimum across wallet infos, propagate committed sync updates per wallet, and keep tip_height as per-wallet applied chain state.
Update tests and example messaging to match the new aggregation semantics.
Based on:
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Tests