fix: seed sync checkpoints from birth_height in ManagedWalletInfo ctors#692
Open
xdustinface wants to merge 4 commits intov0.42-devfrom
Open
fix: seed sync checkpoints from birth_height in ManagedWalletInfo ctors#692xdustinface wants to merge 4 commits intov0.42-devfrom
birth_height in ManagedWalletInfo ctors#692xdustinface wants to merge 4 commits intov0.42-devfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR requires callers to supply a wallet birth height when constructing ManagedWalletInfo. It updates WalletInfoInterface signatures, ManagedWalletInfo implementations, WalletManager creation/import paths, FFI call sites, and numerous tests to pass an explicit Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #692 +/- ##
=============================================
+ Coverage 70.59% 70.62% +0.03%
=============================================
Files 320 320
Lines 68028 68023 -5
=============================================
+ Hits 48021 48043 +22
+ Misses 20007 19980 -27
|
xdustinface
force-pushed
the
refactor/heights-per-wallet
branch
from
055dfc9 to
4a4547d
Compare
Contributor
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
github-actions
Bot
added
the
merge-conflict
xdustinface
force-pushed
the
refactor/birth-height-construction
branch
from
973115f to
5ee6537
Compare
github-actions
Bot
removed
the
merge-conflict
Contributor
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
github-actions
Bot
added
the
merge-conflict
…dWalletInfo` ctors `WalletInfoInterface::from_wallet` and `from_wallet_with_name` (plus the inherent `ManagedWalletInfo` constructors) now take `birth_height: CoreBlockHeight` and seed both checkpoint heights (`synced_height`, `last_processed_height`) to `birth_height.saturating_sub(1)` so the next block to scan is always `birth_height`. Without this, every wallet-add path had to remember `set_birth_height` after construction, and even when called it only set `birth_height`, leaving the checkpoints at `0`. With the per-wallet sync introduced in #694, that dragged `WalletManager::synced_height` (a min across wallets) back to genesis on every add, forcing a re-scan from `0`. Moving the seed into construction makes the invariant a property of the type. `ManagedWalletInfo::with_birth_height` is removed. It set only `birth_height` and had no callers.
xdustinface
force-pushed
the
refactor/birth-height-construction
branch
from
5ee6537 to
0a49ef4
Compare
github-actions
Bot
removed
the
merge-conflict
xdustinface
changed the title
ManagedWalletInfo ctorsbirth_height in ManagedWalletInfo ctors
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-wallet-ffi/src/transaction_checking.rs (1)
77-83:⚠️ Potential issue | 🟠 MajorFFI still hardcodes genesis as the wallet birth height.
Line 82 always constructs
ManagedWalletInfowith0, so restored wallets coming through this API still cannot preserve their actual birth height. That keeps the full-rescan behavior for FFI consumers even though the Rust API now makesbirth_heightexplicit. This entry point needs abirth_heightparameter, or it needs to build from persisted metadata instead of forcing genesis.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/transaction_checking.rs` around lines 77 - 83, The FFI function wallet_create_managed_wallet currently forces genesis by calling ManagedWalletInfo::from_wallet(wallet.inner(), 0); modify this entry point to accept a birth_height parameter (or retrieve the wallet's persisted birth height from its metadata) and pass that value into ManagedWalletInfo::from_wallet instead of 0 so restored wallets preserve their actual birth_height; update the extern "C" signature, any FFI callers, and the construction via FFIManagedWalletInfo::new accordingly.
🧹 Nitpick comments (2)
key-wallet/src/wallet/mod.rs (1)
416-429: Please pin the new off-by-one checkpoint behavior in this test.This now exercises the new constructor signature, but it still would not fail if
birth_heightstopped seedingsynced_height/last_processed_heighttobirth_height - 1. A small non-zero birth-height assertion here would lock down the contract this PR is introducing.Proposed test tweak
- let mut managed_info = ManagedWalletInfo::from_wallet(&wallet, 0); + let mut managed_info = ManagedWalletInfo::from_wallet(&wallet, 100); managed_info.set_name("Test Wallet".to_string()); managed_info.set_description(Some("A test wallet".to_string())); // Test initial managed info assert_eq!(managed_info.wallet_id, wallet.wallet_id); assert_eq!(managed_info.name.as_ref().unwrap(), "Test Wallet"); assert_eq!(managed_info.description.as_ref().unwrap(), "A test wallet"); + assert_eq!(managed_info.metadata.birth_height, 100); + assert_eq!(managed_info.metadata.synced_height, 99); + assert_eq!(managed_info.metadata.last_processed_height, 99); assert_eq!(managed_info.metadata.first_loaded_at, 0); // Default value assert!(managed_info.metadata.last_synced.is_none());As per coding guidelines, "Write unit tests for new functionality".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/wallet/mod.rs` around lines 416 - 429, The test creates ManagedWalletInfo via ManagedWalletInfo::from_wallet but doesn't assert the new off-by-one contract for birth_height seeding synced_height/last_processed_height; update the test after constructing managed_info to (1) ensure the seed birth height is non-zero (e.g. assert managed_info.metadata.birth_height > 0 or set wallet.birth_height to a small non-zero value before calling ManagedWalletInfo::from_wallet) and (2) assert that metadata.synced_height == Some(birth_height - 1) and metadata.last_processed_height == Some(birth_height - 1) to lock down the new behavior introduced by the constructor/signature change.key-wallet-ffi/src/transaction.rs (1)
309-309: Avoid hardcodingbirth_height = 0in FFI transaction checks.This works today, but it sidesteps the new constructor invariant and can reintroduce checkpoint drift if this path later persists managed state. Prefer threading an explicit birth/checkpoint height from caller or wallet context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/transaction.rs` at line 309, ManagedWalletInfo::from_wallet is being called with a hardcoded birth height of 0 (let mut managed_info = ManagedWalletInfo::from_wallet(wallet.inner(), 0)), which violates the constructor invariant and risks checkpoint drift; instead thread an explicit birth/checkpoint height into this code path by either (A) obtaining the height from the wallet context (e.g., call a wallet method like wallet.birth_height() or wallet.checkpoint_height() and pass that value to ManagedWalletInfo::from_wallet) or (B) add a birth_height parameter to the surrounding function and use that parameter when calling ManagedWalletInfo::from_wallet(wallet.inner(), birth_height); update call sites accordingly so no hardcoded 0 remains.
🤖 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 440-442: The comment incorrectly states “current chain tip” though
the code passes self.last_processed_height() to T::from_wallet, which is
manager-derived and not necessarily the chain tip; update the comment above the
T::from_wallet(&wallet, self.last_processed_height()) call to describe that it
uses the manager's last processed height (or aggregate tip) as the birth height,
not the node's chain tip, so the wording accurately reflects
self.last_processed_height()’s semantics.
In `@key-wallet/src/wallet/managed_wallet_info/mod.rs`:
- Around line 85-100: The public API still allows creating inconsistent
checkpoints because ManagedWalletInfo::new, ManagedWalletInfo::with_name and
WalletInfoInterface::set_birth_height do not maintain the invariant that
WalletMetadata.{birth_height, synced_height, last_processed_height} are aligned;
update the API so either (A) all public constructors require a birth_height
argument and remove the mutable setter (delete
WalletInfoInterface::set_birth_height and change new/with_name to accept
birth_height) or (B) keep the setter but make
WalletInfoInterface::set_birth_height (and any constructors that accept a birth
height) update all three fields atomically (set synced_height =
last_processed_height = birth_height.saturating_sub(1)) to match
ManagedWalletInfo::from_wallet, referencing ManagedWalletInfo::from_wallet,
WalletMetadata::birth_height, WalletMetadata::synced_height and
WalletMetadata::last_processed_height when making the change.
---
Outside diff comments:
In `@key-wallet-ffi/src/transaction_checking.rs`:
- Around line 77-83: The FFI function wallet_create_managed_wallet currently
forces genesis by calling ManagedWalletInfo::from_wallet(wallet.inner(), 0);
modify this entry point to accept a birth_height parameter (or retrieve the
wallet's persisted birth height from its metadata) and pass that value into
ManagedWalletInfo::from_wallet instead of 0 so restored wallets preserve their
actual birth_height; update the extern "C" signature, any FFI callers, and the
construction via FFIManagedWalletInfo::new accordingly.
---
Nitpick comments:
In `@key-wallet-ffi/src/transaction.rs`:
- Line 309: ManagedWalletInfo::from_wallet is being called with a hardcoded
birth height of 0 (let mut managed_info =
ManagedWalletInfo::from_wallet(wallet.inner(), 0)), which violates the
constructor invariant and risks checkpoint drift; instead thread an explicit
birth/checkpoint height into this code path by either (A) obtaining the height
from the wallet context (e.g., call a wallet method like wallet.birth_height()
or wallet.checkpoint_height() and pass that value to
ManagedWalletInfo::from_wallet) or (B) add a birth_height parameter to the
surrounding function and use that parameter when calling
ManagedWalletInfo::from_wallet(wallet.inner(), birth_height); update call sites
accordingly so no hardcoded 0 remains.
In `@key-wallet/src/wallet/mod.rs`:
- Around line 416-429: The test creates ManagedWalletInfo via
ManagedWalletInfo::from_wallet but doesn't assert the new off-by-one contract
for birth_height seeding synced_height/last_processed_height; update the test
after constructing managed_info to (1) ensure the seed birth height is non-zero
(e.g. assert managed_info.metadata.birth_height > 0 or set wallet.birth_height
to a small non-zero value before calling ManagedWalletInfo::from_wallet) and (2)
assert that metadata.synced_height == Some(birth_height - 1) and
metadata.last_processed_height == Some(birth_height - 1) to lock down the new
behavior introduced by the constructor/signature change.
🪄 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: 885618de-3dc3-415f-a7d1-5b3b7d241b42
📒 Files selected for processing (16)
key-wallet-ffi/src/managed_wallet.rskey-wallet-ffi/src/transaction.rskey-wallet-ffi/src/transaction_checking.rskey-wallet-manager/src/accessors.rskey-wallet-manager/src/lib.rskey-wallet-manager/tests/integration_test.rskey-wallet-manager/tests/test_serialized_wallets.rskey-wallet/src/test_utils/wallet.rskey-wallet/src/transaction_checking/transaction_router/tests/identity_transactions.rskey-wallet/src/transaction_checking/transaction_router/tests/provider.rskey-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rskey-wallet/src/wallet/managed_wallet_info/mod.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rskey-wallet/src/wallet/mod.rs
…allet import Addresses CodeRabbit review comment on PR #692 #692 (comment)
The setter only updated `birth_height`, leaving `synced_height` and `last_processed_height` at their previous values, so a caller using it after construction would silently bypass the seeding invariant established by `from_wallet`. With the per-wallet height tracking in #694 that aggregate to a min/max across wallets, this could drag the manager's `synced_height` back to genesis. After the constructor change, the setter has zero callers in-tree, so removing it is the cleanest way to enforce the invariant: `birth_height` is fixed at construction via `from_wallet`. Addresses CodeRabbit review comment on PR #692 #692 (comment)
…anaged_info` Use a non-zero `birth_height` and assert that `metadata.synced_height` and `metadata.last_processed_height` land at `birth_height - 1`, locking the seeding invariant in place. Addresses CodeRabbit review comment on PR #692 #692 (review)
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
1 participant
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.
WalletInfoInterface::from_walletandfrom_wallet_with_name(plus the inherentManagedWalletInfoconstructors) now takebirth_height: CoreBlockHeightand seed both checkpoint heights (synced_height,last_processed_height) tobirth_height.saturating_sub(1)so the next block to scan is alwaysbirth_height.Without this, every wallet-add path had to remember
set_birth_heightafter construction. Forgetting it left the checkpoints at0, draggingWalletManager::synced_height(a min across wallets) back to genesis on every add. Moving the seed into construction makes the invariant a property of the type.ManagedWalletInfo::with_birth_heightis removed. It set onlybirth_heightand had no callers.Based on:
Summary by CodeRabbit