Skip to content

Commit 677a67c

Browse files
authored
fix: seed sync checkpoints from birth_height in ManagedWalletInfo ctors (#692)
* fix(key-wallet): seed sync checkpoints from `birth_height` in `ManagedWalletInfo` 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. * docs(key-wallet-manager): clarify fallback `birth_height` source on wallet import Addresses CodeRabbit review comment on PR #692 #692 (comment) * refactor(key-wallet): drop `WalletInfoInterface::set_birth_height` 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) * test(key-wallet): pin off-by-one seed contract in `test_wallet_with_managed_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)
1 parent 8da22a8 commit 677a67c

16 files changed

Lines changed: 100 additions & 88 deletions

File tree

key-wallet-ffi/src/managed_wallet.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -501,7 +501,7 @@ mod tests {
501501

502502
// Create managed wallet info from the wallet heap-allocated like C would do
503503
let wallet_rust = unsafe { &(*wallet).wallet };
504-
let managed_info = ManagedWalletInfo::from_wallet(wallet_rust);
504+
let managed_info = ManagedWalletInfo::from_wallet(wallet_rust, 0);
505505
let ffi_managed = Box::into_raw(Box::new(FFIManagedWalletInfo::new(managed_info)));
506506

507507
// Test get_next_receive_address with valid pointers
@@ -581,7 +581,7 @@ mod tests {
581581

582582
// We need to work with the existing wallet structure
583583
// Create managed wallet info from the existing wallet
584-
let mut managed_info = ManagedWalletInfo::from_wallet(wallet_arc);
584+
let mut managed_info = ManagedWalletInfo::from_wallet(wallet_arc, 0);
585585

586586
let network = key_wallet::Network::Testnet;
587587

@@ -759,7 +759,7 @@ mod tests {
759759

760760
// Create managed wallet info
761761
let wallet_arc = unsafe { &(*wallet_ptr).wallet };
762-
let mut managed_info = ManagedWalletInfo::from_wallet(wallet_arc);
762+
let mut managed_info = ManagedWalletInfo::from_wallet(wallet_arc, 0);
763763

764764
// Set some test balance values
765765
managed_info.balance = WalletCoreBalance::new(1000000, 50000, 10000, 25000);

key-wallet-ffi/src/transaction.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -306,7 +306,7 @@ pub unsafe extern "C" fn wallet_check_transaction(
306306
use key_wallet::transaction_checking::WalletTransactionChecker;
307307
use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo;
308308

309-
let mut managed_info = ManagedWalletInfo::from_wallet(wallet.inner());
309+
let mut managed_info = ManagedWalletInfo::from_wallet(wallet.inner(), 0);
310310

311311
// Check the transaction - wallet is always required now
312312
let wallet_mut = unwrap_or_return!(wallet.inner_mut(), error);

key-wallet-ffi/src/transaction_checking.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ pub unsafe extern "C" fn wallet_create_managed_wallet(
7979
error: *mut FFIError,
8080
) -> *mut FFIManagedWalletInfo {
8181
let wallet = deref_ptr!(wallet, error);
82-
let managed_info = ManagedWalletInfo::from_wallet(wallet.inner());
82+
let managed_info = ManagedWalletInfo::from_wallet(wallet.inner(), 0);
8383
Box::into_raw(Box::new(FFIManagedWalletInfo::new(managed_info)))
8484
}
8585

key-wallet-manager/src/accessors.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -249,7 +249,7 @@ mod tests {
249249
fn insert_wallet_rejects_duplicate() {
250250
let mut manager: WalletManager<ManagedWalletInfo> = WalletManager::new(Network::Testnet);
251251
let wallet = build_wallet();
252-
let info = ManagedWalletInfo::from_wallet(&wallet);
252+
let info = ManagedWalletInfo::from_wallet(&wallet, 0);
253253

254254
let id =
255255
manager.insert_wallet(wallet.clone(), info.clone()).expect("first insert succeeds");

key-wallet-manager/src/lib.rs

Lines changed: 9 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -170,8 +170,7 @@ impl<T: WalletInfoInterface + Send + Sync + 'static> WalletManager<T> {
170170

171171
// Create managed wallet info from the wallet to properly initialize accounts
172172
// This ensures the ManagedAccountCollection is synchronized with the Wallet's accounts
173-
let mut managed_info = T::from_wallet(&wallet);
174-
managed_info.set_birth_height(birth_height);
173+
let mut managed_info = T::from_wallet(&wallet, birth_height);
175174
managed_info.set_first_loaded_at(current_timestamp());
176175

177176
// The wallet already has accounts created according to the provided options
@@ -276,8 +275,7 @@ impl<T: WalletInfoInterface + Send + Sync + 'static> WalletManager<T> {
276275
})?;
277276

278277
// Add the wallet to the manager
279-
let mut managed_info = T::from_wallet(&final_wallet);
280-
managed_info.set_birth_height(birth_height);
278+
let mut managed_info = T::from_wallet(&final_wallet, birth_height);
281279
managed_info.set_first_loaded_at(current_timestamp());
282280

283281
self.wallets.insert(wallet_id, final_wallet);
@@ -311,8 +309,7 @@ impl<T: WalletInfoInterface + Send + Sync + 'static> WalletManager<T> {
311309
}
312310

313311
// Create managed wallet info
314-
let mut managed_info = T::from_wallet(&wallet);
315-
managed_info.set_birth_height(self.last_processed_height());
312+
let mut managed_info = T::from_wallet(&wallet, self.last_processed_height());
316313
managed_info.set_first_loaded_at(current_timestamp());
317314

318315
self.wallets.insert(wallet_id, wallet);
@@ -352,8 +349,7 @@ impl<T: WalletInfoInterface + Send + Sync + 'static> WalletManager<T> {
352349
}
353350

354351
// Create managed wallet info
355-
let mut managed_info = T::from_wallet(&wallet);
356-
managed_info.set_birth_height(self.last_processed_height());
352+
let mut managed_info = T::from_wallet(&wallet, self.last_processed_height());
357353
managed_info.set_first_loaded_at(current_timestamp());
358354

359355
self.wallets.insert(wallet_id, wallet);
@@ -400,8 +396,7 @@ impl<T: WalletInfoInterface + Send + Sync + 'static> WalletManager<T> {
400396
}
401397

402398
// Create managed wallet info
403-
let mut managed_info = T::from_wallet(&wallet);
404-
managed_info.set_birth_height(self.last_processed_height());
399+
let mut managed_info = T::from_wallet(&wallet, self.last_processed_height());
405400
managed_info.set_first_loaded_at(current_timestamp());
406401

407402
self.wallets.insert(wallet_id, wallet);
@@ -442,11 +437,10 @@ impl<T: WalletInfoInterface + Send + Sync + 'static> WalletManager<T> {
442437
return Err(WalletError::WalletExists(wallet_id));
443438
}
444439

445-
// Create managed wallet info from the imported wallet
446-
let mut managed_info = T::from_wallet(&wallet);
447-
448-
// Use the current height as the birth height since we don't know when it was originally created
449-
managed_info.set_birth_height(self.last_processed_height());
440+
// Create managed wallet info from the imported wallet, using the manager's
441+
// current aggregated last-processed height as the fallback birth height
442+
// since the serialized form does not preserve it.
443+
let mut managed_info = T::from_wallet(&wallet, self.last_processed_height());
450444
managed_info.set_first_loaded_at(current_timestamp());
451445

452446
self.wallets.insert(wallet_id, wallet);

key-wallet-manager/tests/integration_test.rs

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -181,36 +181,41 @@ fn test_block_height_tracking() {
181181
.create_wallet_from_mnemonic(
182182
&mnemonic2.to_string(),
183183
"",
184-
0,
184+
5000,
185185
WalletAccountCreationOptions::Default,
186186
)
187187
.unwrap();
188188

189189
assert_eq!(manager.wallet_count(), 2);
190190

191-
// Both wallets initialized with `synced_height = birth_height - 1 = 0`,
192-
// so neither has been processed past genesis.
193-
for wallet_info in manager.get_all_wallet_infos().values() {
194-
assert_eq!(wallet_info.last_processed_height(), 0);
195-
assert_eq!(wallet_info.synced_height(), 0);
196-
}
191+
// wallet1 was created with birth_height = 0 so its sync checkpoint stays at 0;
192+
// wallet2 was created with birth_height = 5000 and is seeded to birth_height - 1.
193+
let wallet_info1 = manager.get_wallet_info(&wallet_id1).unwrap();
194+
let wallet_info2 = manager.get_wallet_info(&wallet_id2).unwrap();
195+
assert_eq!(wallet_info1.last_processed_height(), 0);
196+
assert_eq!(wallet_info1.synced_height(), 0);
197+
assert_eq!(wallet_info2.last_processed_height(), 4999);
198+
assert_eq!(wallet_info2.synced_height(), 4999);
199+
// Manager aggregates: synced_height is the min, last_processed_height is the max.
200+
assert_eq!(manager.synced_height(), 0);
201+
assert_eq!(manager.last_processed_height(), 4999);
197202

198203
// Per-wallet last-processed updates only touch the addressed wallet.
199204
manager.update_wallet_last_processed_height(&wallet_id1, 12345);
200205
assert_eq!(manager.last_processed_height(), 12345);
201206
let wallet_info1 = manager.get_wallet_info(&wallet_id1).unwrap();
202207
let wallet_info2 = manager.get_wallet_info(&wallet_id2).unwrap();
203208
assert_eq!(wallet_info1.last_processed_height(), 12345);
204-
assert_eq!(wallet_info2.last_processed_height(), 0);
209+
assert_eq!(wallet_info2.last_processed_height(), 4999);
205210

206211
// Per-wallet synced-height updates only touch the addressed wallet.
207212
manager.update_wallet_synced_height(&wallet_id1, 12000);
208213
let wallet_info1 = manager.get_wallet_info(&wallet_id1).unwrap();
209214
let wallet_info2 = manager.get_wallet_info(&wallet_id2).unwrap();
210215
assert_eq!(wallet_info1.synced_height(), 12000);
211-
assert_eq!(wallet_info2.synced_height(), 0);
212-
// Aggregate `synced_height()` is `min` across wallets, so wallet 2 holds it at 0.
213-
assert_eq!(manager.synced_height(), 0);
216+
assert_eq!(wallet_info2.synced_height(), 4999);
217+
// Aggregate `synced_height()` is `min` across wallets, so wallet 2 holds it at 4999.
218+
assert_eq!(manager.synced_height(), 4999);
214219

215220
// Advance wallet 2 too. Aggregate min jumps to wallet 2's new value.
216221
manager.update_wallet_synced_height(&wallet_id2, 11000);

key-wallet-manager/tests/test_serialized_wallets.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
#[cfg(test)]
33
mod tests {
44
use key_wallet::wallet::initialization::WalletAccountCreationOptions;
5+
use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface;
56
use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo;
67
use key_wallet::Network;
78
use key_wallet_manager::WalletManager;
@@ -26,6 +27,12 @@ mod tests {
2627
assert!(!bytes.is_empty());
2728
println!("Full wallet ID: {}", hex::encode(wallet_id));
2829

30+
// The wallet's sync checkpoint should be seeded to birth_height - 1.
31+
let info = manager.get_wallet_info(&wallet_id).unwrap();
32+
assert_eq!(info.birth_height(), 100_000);
33+
assert_eq!(info.synced_height(), 99_999);
34+
assert_eq!(info.last_processed_height(), 99_999);
35+
2936
// Test 2: Create watch-only wallet (no private keys)
3037
let mut manager2 = WalletManager::<ManagedWalletInfo>::new(Network::Testnet);
3138
let result = manager2.create_wallet_from_mnemonic_return_serialized_bytes(

key-wallet/src/test_utils/wallet.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ impl TestWalletContext {
3131
let wallet = Wallet::new_random(Network::Testnet, WalletAccountCreationOptions::Default)
3232
.expect("Should create wallet");
3333
let mut managed_wallet =
34-
ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string());
34+
ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string(), 0);
3535

3636
let xpub = wallet
3737
.accounts

key-wallet/src/transaction_checking/transaction_router/tests/identity_transactions.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ async fn test_identity_registration_account_routing() {
4444
wallet.add_account(account_type, None).expect("Failed to add account to wallet");
4545

4646
let mut managed_wallet_info =
47-
ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string());
47+
ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string(), 0);
4848

4949
// Get the identity registration account
5050
let account = wallet
@@ -155,7 +155,7 @@ async fn test_normal_payment_to_identity_address_not_detected() {
155155
let mut wallet = Wallet::new_random(network, WalletAccountCreationOptions::Default)
156156
.expect("Failed to create wallet with default options");
157157
let mut managed_wallet_info =
158-
ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string());
158+
ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string(), 0);
159159

160160
let account = wallet
161161
.accounts

key-wallet/src/transaction_checking/transaction_router/tests/provider.rs

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -136,10 +136,10 @@ async fn test_provider_registration_transaction_routing_check_owner_only() {
136136
.expect("Failed to create wallet with default options");
137137

138138
let mut other_managed_wallet_info =
139-
ManagedWalletInfo::from_wallet_with_name(&other_wallet, "Other".to_string());
139+
ManagedWalletInfo::from_wallet_with_name(&other_wallet, "Other".to_string(), 0);
140140

141141
let mut managed_wallet_info =
142-
ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string());
142+
ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string(), 0);
143143

144144
// Get addresses from provider accounts
145145
let managed_owner = managed_wallet_info
@@ -271,10 +271,10 @@ async fn test_provider_registration_transaction_routing_check_voting_only() {
271271
.expect("Failed to create wallet with default options");
272272

273273
let mut other_managed_wallet_info =
274-
ManagedWalletInfo::from_wallet_with_name(&other_wallet, "Other".to_string());
274+
ManagedWalletInfo::from_wallet_with_name(&other_wallet, "Other".to_string(), 0);
275275

276276
let mut managed_wallet_info =
277-
ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string());
277+
ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string(), 0);
278278

279279
// Get addresses from provider accounts
280280
let owner_address = other_managed_wallet_info
@@ -406,10 +406,10 @@ async fn test_provider_registration_transaction_routing_check_operator_only() {
406406
.expect("Failed to create wallet with default options");
407407

408408
let mut other_managed_wallet_info =
409-
ManagedWalletInfo::from_wallet_with_name(&other_wallet, "Other".to_string());
409+
ManagedWalletInfo::from_wallet_with_name(&other_wallet, "Other".to_string(), 0);
410410

411411
let mut managed_wallet_info =
412-
ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string());
412+
ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string(), 0);
413413

414414
// Get addresses from provider accounts
415415
let owner_address = other_managed_wallet_info
@@ -587,10 +587,10 @@ async fn test_provider_registration_transaction_routing_check_platform_only() {
587587
.expect("Failed to create wallet with default options");
588588

589589
let mut other_managed_wallet_info =
590-
ManagedWalletInfo::from_wallet_with_name(&other_wallet, "Other".to_string());
590+
ManagedWalletInfo::from_wallet_with_name(&other_wallet, "Other".to_string(), 0);
591591

592592
let mut managed_wallet_info =
593-
ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string());
593+
ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string(), 0);
594594

595595
// Get addresses from provider accounts
596596
let owner_address = other_managed_wallet_info
@@ -785,7 +785,7 @@ async fn test_provider_update_registrar_with_voting_and_operator() {
785785
.expect("Failed to create wallet with default options");
786786

787787
let mut managed_wallet_info =
788-
ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string());
788+
ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string(), 0);
789789

790790
// Get voting address
791791
let voting_address = managed_wallet_info
@@ -858,7 +858,7 @@ async fn test_provider_revocation_classification_and_routing() {
858858
.expect("Failed to create wallet with default options");
859859

860860
let mut managed_wallet_info =
861-
ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string());
861+
ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string(), 0);
862862

863863
// Get a standard address for collateral return
864864
let account = wallet

0 commit comments

Comments
 (0)