fix: resolve sync stalls #1438
fix: resolve sync stalls #1438
Conversation
WalkthroughDependency versions updated; contact-request DAO split into user-scoped queries; blockchain service gained lifecycle, timeout/reschedule and thread-priority logic; pre-block stages expanded and service-side definitions removed; platform sync uses separate last-contact timestamps; UI/ViewModel tweaks; wallet save/load performance tests and test helpers added; new blockchain config introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App (ProcessLifecycleOwner)
participant BService as BlockchainServiceImpl
participant LifecycleObs as appLifecycleObserver
participant ThreadPri as ThreadPriorityHelper
participant BlockStore as BlockStore
App->>BService: onCreate()
activate BService
BService->>LifecycleObs: register observer
BService->>BlockStore: initialize & verify (timed)
Note over BlockStore: detect file-lock vs corruption
deactivate BService
App->>LifecycleObs: onStart() [foreground]
LifecycleObs->>BService: isAppInBackground = false
LifecycleObs->>ThreadPri: adjustThreadPriorityForState(false)
App->>LifecycleObs: onStop() [background]
LifecycleObs->>BService: isAppInBackground = true
LifecycleObs->>ThreadPri: adjustThreadPriorityForState(true)
sequenceDiagram
participant PlatformSync as PlatformSyncService
participant DAO as DashPayContactRequestDao
participant Status as SyncStatusTracker
rect rgb(245, 250, 255)
Note over PlatformSync: contact fetches now use direction-specific times
PlatformSync->>DAO: fetch toContactDocuments(after = lastFromMe)
DAO-->>PlatformSync: toContactDocuments
PlatformSync->>Status: updateSyncStatus(GetInvites)
PlatformSync->>DAO: fetch fromContactDocuments(after = lastToMe)
DAO-->>PlatformSync: fromContactDocuments
PlatformSync->>Status: updateSyncStatus(TransactionMetadata)
PlatformSync->>DAO: fetch updatedProfiles(after = min(lastToMe, lastFromMe))
DAO-->>PlatformSync: updatedProfiles
PlatformSync->>Status: updateSyncStatus(Topups)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wallet/src/de/schildbach/wallet/ui/transactions/TransactionResultViewBinder.kt (1)
399-417: Remove the unusedoutputAddressesparameter.The
outputAddressesparameter is added to the method signature but is never referenced in the method body. This suggests incomplete refactoring.Apply this diff to remove the unused parameter:
- private fun setReturns(outputAddresses: List<Address>, outputOpReturns: List<String>, inflater: LayoutInflater, error: Boolean, completed: Boolean) { + private fun setReturns(outputOpReturns: List<String>, inflater: LayoutInflater, error: Boolean, completed: Boolean) { binding.outputsContainer.isVisible = (outputOpReturns.isNotEmpty() && outputOpReturns.contains("OP RETURN")) || outputAddresses.isNotEmpty() outputOpReturns.forEach { // ... } }Then update the call sites at lines 163 and 421 accordingly.
🧹 Nitpick comments (1)
wallet/src/de/schildbach/wallet/ui/transactions/TransactionResultViewBinder.kt (1)
386-397: Clarify the visibility condition logic.The visibility check
(outputOpReturns.isNotEmpty() && outputOpReturns.contains("OP RETURN")) || outputAddresses.isNotEmpty()appears on both lines 387 and 400. IfoutputOpReturnsonly ever contains the literal string"OP RETURN"(or is empty), thenisNotEmpty()alone would suffice. Additionally, this duplicated logic could be extracted to a helper method for better maintainability.Consider extracting the visibility logic:
+ private fun shouldShowOutputs(outputAddresses: List<Address>, outputOpReturns: List<String>): Boolean { + return outputOpReturns.contains("OP RETURN") || outputAddresses.isNotEmpty() + } + private fun setOutputs(outputAddresses: List<Address>, outputOpReturns: List<String>, inflater: LayoutInflater) { - binding.outputsContainer.isVisible = (outputOpReturns.isNotEmpty() && outputOpReturns.contains("OP RETURN")) || outputAddresses.isNotEmpty() + binding.outputsContainer.isVisible = shouldShowOutputs(outputAddresses, outputOpReturns) outputAddresses.forEach { // ... } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
build.gradle(1 hunks)check-alignment.sh(1 hunks)wallet/build.gradle(2 hunks)wallet/src/de/schildbach/wallet/database/dao/DashPayContactRequestDao.kt(1 hunks)wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt(10 hunks)wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt(7 hunks)wallet/src/de/schildbach/wallet/ui/dashpay/OnPreBlockProgressListener.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/dashpay/PlatformRepo.kt(0 hunks)wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt(0 hunks)wallet/src/de/schildbach/wallet/ui/transactions/TransactionResultViewBinder.kt(6 hunks)
💤 Files with no reviewable changes (2)
- wallet/src/de/schildbach/wallet/ui/dashpay/PlatformRepo.kt
- wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1417
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/explore/MerchantDao.kt:0-0
Timestamp: 2025-08-08T16:48:49.964Z
Learning: In PR dashpay/dash-wallet#1417, HashEngineering chose to defer adding Room indexes for gift_card_providers (provider, denominationsType, merchantId) to a follow-up PR; do not block the current PR on this optimization. Files: features/exploredash/.../data/explore/MerchantDao.kt and features/exploredash/.../data/dashspend/GiftCardProvider.kt.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1423
File: wallet/src/de/schildbach/wallet/ui/EditProfileActivity.kt:418-446
Timestamp: 2025-08-25T14:48:39.247Z
Learning: HashEngineering prefers to refactor and reuse topup code for balance validation logic improvements in DashPay activities like EditProfileActivity, rather than implementing individual fixes in the current PR.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1422
File: common/src/main/java/org/dash/wallet/common/util/Constants.kt:68-69
Timestamp: 2025-08-25T15:00:20.777Z
Learning: HashEngineering prefers to keep HTTP logging enabled in release mode (using log.info instead of gating with BuildConfig.DEBUG) to debug production errors, even though this may leak URLs in production logs. This is a deliberate trade-off for debugging purposes in the Dash wallet project.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1410
File: wallet/src/de/schildbach/wallet/data/InvitationLinkData.kt:79-84
Timestamp: 2025-07-12T07:12:04.769Z
Learning: HashEngineering prefers to handle defensive validation improvements for URI parsing in follow-up PRs rather than including them in the current PR when the main focus is on replacing Firebase with AppsFlyer.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1425
File: wallet/src/de/schildbach/wallet/ui/more/SettingsScreen.kt:122-151
Timestamp: 2025-09-05T06:47:44.508Z
Learning: HashEngineering prefers to defer UI edge case fixes like the CoinJoin finishing state display issue (showing "Finishing ...%" when mode is OFF but status is FINISHING) to later PRs when the main focus is on redesigning the settings screen.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1422
File: wallet/src/de/schildbach/wallet/data/PaymentIntent.java:476-481
Timestamp: 2025-08-25T15:04:18.461Z
Learning: HashEngineering prefers to keep payeeUsername and payeeUserId in PaymentIntent.toString() logs unredacted for debugging purposes, prioritizing troubleshooting capability over PII protection concerns in the Dash wallet project.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1425
File: common/src/main/java/org/dash/wallet/common/ui/components/DashButton.kt:94-101
Timestamp: 2025-09-05T06:00:25.088Z
Learning: HashEngineering prefers to fix the DashButton alpha dimming issue (using explicit disabled colors instead of overall alpha) in a follow-up PR rather than including it in the current settings screen redesign PR.
📚 Learning: 2025-09-05T06:16:12.649Z
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1425
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:134-145
Timestamp: 2025-09-05T06:16:12.649Z
Learning: The dash-wallet project has a minimum SDK version of 24, so API availability checks for methods introduced in API 23 or earlier are not needed.
Applied to files:
build.gradlewallet/build.gradle
📚 Learning: 2025-08-08T16:48:49.964Z
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1417
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/explore/MerchantDao.kt:0-0
Timestamp: 2025-08-08T16:48:49.964Z
Learning: In PR dashpay/dash-wallet#1417, HashEngineering chose to defer adding Room indexes for gift_card_providers (provider, denominationsType, merchantId) to a follow-up PR; do not block the current PR on this optimization. Files: features/exploredash/.../data/explore/MerchantDao.kt and features/exploredash/.../data/dashspend/GiftCardProvider.kt.
Applied to files:
wallet/src/de/schildbach/wallet/ui/dashpay/OnPreBlockProgressListener.ktwallet/src/de/schildbach/wallet/database/dao/DashPayContactRequestDao.kt
🧬 Code graph analysis (1)
wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt (1)
wallet/src/de/schildbach/wallet/service/platform/TopUpRepository.kt (2)
checkTopUps(88-88)checkTopUps(344-378)
🪛 Shellcheck (0.11.0)
check-alignment.sh
[warning] 8-8: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.
(SC2166)
[warning] 38-38: Prefer [ p ] || [ q ] as [ p -o q ] is not well defined.
(SC2166)
🔇 Additional comments (9)
wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt (5)
357-372: LGTM: Performance tracking added correctly.The Stopwatch timing measurement is properly implemented with variable declaration and logging.
1311-1344: LGTM: Foreground service startup tracking enhanced.The logging additions provide good visibility into the foreground service startup flow, including the retry mechanism for Android 12+ restrictions.
1359-1428: LGTM: Comprehensive shutdown tracking added.The logging enhancements provide excellent visibility into the service cleanup sequence, which will help diagnose shutdown-related issues and potential race conditions. The comment on line 1365 clearly explains the cleanup coordination mechanism.
1401-1402: LGTM: Null-safety improved for blockchain store cleanup.Replacing the non-null assertion operator (
!!) with the safe-call operator (?.) prevents potential NPE during service shutdown if the stores failed to initialize properly.
1451-1451: LGTM: Timeout callback logging added.The logging helps track when the Android 15+ foreground service time limit is reached.
wallet/src/de/schildbach/wallet/ui/transactions/TransactionResultViewBinder.kt (1)
79-81: LGTM!Promoting these to class fields improves state management and enables proper reuse in methods like
setSentToReturn.build.gradle (1)
6-7: Verify SNAPSHOT version availability in configured repositories.The SNAPSHOT version updates align with the PR's fix intent (sync stalls), but SNAPSHOT versions carry development risk. Ensure that both versions are available in the configured Maven repositories (lines 78-79 include a Sonatype snapshots repository, which should be sufficient).
check-alignment.sh (1)
43-116: LGTM — solid script architecture for ELF alignment checking.The alignment detection logic is well-structured: proper APK/APEX extraction, accurate ELF filtering, and correct alignment regex (line 104 validates 2**14 and higher powers of 2). Error handling, temporary cleanup, and colorized output are all well-implemented.
wallet/build.gradle (1)
61-63: Request manual verification of SNAPSHOT dependency usage and testing.The codebase shows no direct usages of
dashj-x11-android,dashj-scrypt-android, ordashj-bls-androidin application code. These libraries appear to be transitive dependencies or indirectly used through other dashj SDK libraries (dash-sdk-java,dash-sdk-android). However, the concern about using SNAPSHOT versions (unstable development builds) for major version jumps remains valid from a stability perspective.
- Confirm dependency type: Are these transitive dependencies pulled by
dash-sdk-java/dash-sdk-android, or are they direct but unused?- Testing coverage: Have integration tests been performed against these SNAPSHOT versions to validate compatibility?
- Related changes: Review the related dashpay/dashj PR or branch to understand the scope of changes in the 1.0.0 release.
wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt
Outdated
Show resolved
Hide resolved
wallet/src/de/schildbach/wallet/ui/dashpay/OnPreBlockProgressListener.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt (1)
374-393: Prevent PreBlockStage regressions from out-of-order async completions.The concurrent
asyncblocks can complete in any order, causingupdateSyncStatusto emit stages non-monotonically. For example, ifTopups(stage 15) completes beforeGetInvites(stage 13), the UI will see the stage jump forward then regress backward, breaking the expected progress indicator behavior.As noted in the previous review, please run these updates sequentially or collect results and emit stages in order:
- coroutineScope { - awaitAll( - async { - updateInvitations() - updateSyncStatus(PreBlockStage.GetInvites) - }, - async { - updateTransactionMetadata() - updateSyncStatus(PreBlockStage.TransactionMetadata) - }, - async { - updateContactProfiles(userId, min(lastContactRequestTimeToMe, lastContactRequestTimeFromMe)) - updateSyncStatus(PreBlockStage.GetUpdatedProfiles) - }, - async { - checkTopUps() - updateSyncStatus(PreBlockStage.Topups) - } - ) - } + updateInvitations() + updateSyncStatus(PreBlockStage.GetInvites) + updateTransactionMetadata() + updateSyncStatus(PreBlockStage.TransactionMetadata) + updateContactProfiles(userId, min(lastContactRequestTimeToMe, lastContactRequestTimeFromMe)) + updateSyncStatus(PreBlockStage.GetUpdatedProfiles) + checkTopUps() + updateSyncStatus(PreBlockStage.Topups)
🧹 Nitpick comments (1)
wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt (1)
385-385: Consider optimizing the timestamp logic for the zero-edge case.Using
min(lastContactRequestTimeToMe, lastContactRequestTimeFromMe)is safe and ensures all updates are captured. However, when one timestamp is0(no requests in that direction) and the other is non-zero (recent activity), themin()returns0, causing all profiles to be re-fetched rather than just recent updates.While this is functionally correct and only affects performance during the first sync after establishing contacts in one direction, you could optimize by handling the zero case explicitly:
- updateContactProfiles(userId, min(lastContactRequestTimeToMe, lastContactRequestTimeFromMe)) + val effectiveTimestamp = when { + lastContactRequestTimeToMe == 0L -> lastContactRequestTimeFromMe + lastContactRequestTimeFromMe == 0L -> lastContactRequestTimeToMe + else -> min(lastContactRequestTimeToMe, lastContactRequestTimeFromMe) + } + updateContactProfiles(userId, effectiveTimestamp)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
wallet/androidTest/de/schildbach/wallet/performance/README.md(1 hunks)wallet/androidTest/de/schildbach/wallet/performance/WalletSavePerformanceTest.kt(1 hunks)wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt(24 hunks)wallet/src/de/schildbach/wallet/service/PreBlockStage.kt(0 hunks)wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt(8 hunks)wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/more/ContactSupportViewModel.kt(1 hunks)
💤 Files with no reviewable changes (1)
- wallet/src/de/schildbach/wallet/service/PreBlockStage.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt
- wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1417
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/explore/MerchantDao.kt:0-0
Timestamp: 2025-08-08T16:48:49.964Z
Learning: In PR dashpay/dash-wallet#1417, HashEngineering chose to defer adding Room indexes for gift_card_providers (provider, denominationsType, merchantId) to a follow-up PR; do not block the current PR on this optimization. Files: features/exploredash/.../data/explore/MerchantDao.kt and features/exploredash/.../data/dashspend/GiftCardProvider.kt.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1423
File: wallet/src/de/schildbach/wallet/ui/EditProfileActivity.kt:418-446
Timestamp: 2025-08-25T14:48:39.247Z
Learning: HashEngineering prefers to refactor and reuse topup code for balance validation logic improvements in DashPay activities like EditProfileActivity, rather than implementing individual fixes in the current PR.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1422
File: common/src/main/java/org/dash/wallet/common/util/Constants.kt:68-69
Timestamp: 2025-08-25T15:00:20.777Z
Learning: HashEngineering prefers to keep HTTP logging enabled in release mode (using log.info instead of gating with BuildConfig.DEBUG) to debug production errors, even though this may leak URLs in production logs. This is a deliberate trade-off for debugging purposes in the Dash wallet project.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1410
File: wallet/src/de/schildbach/wallet/data/InvitationLinkData.kt:79-84
Timestamp: 2025-07-12T07:12:04.769Z
Learning: HashEngineering prefers to handle defensive validation improvements for URI parsing in follow-up PRs rather than including them in the current PR when the main focus is on replacing Firebase with AppsFlyer.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1425
File: wallet/src/de/schildbach/wallet/ui/more/SettingsScreen.kt:122-151
Timestamp: 2025-09-05T06:47:44.508Z
Learning: HashEngineering prefers to defer UI edge case fixes like the CoinJoin finishing state display issue (showing "Finishing ...%" when mode is OFF but status is FINISHING) to later PRs when the main focus is on redesigning the settings screen.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1422
File: wallet/src/de/schildbach/wallet/data/PaymentIntent.java:476-481
Timestamp: 2025-08-25T15:04:18.461Z
Learning: HashEngineering prefers to keep payeeUsername and payeeUserId in PaymentIntent.toString() logs unredacted for debugging purposes, prioritizing troubleshooting capability over PII protection concerns in the Dash wallet project.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1425
File: common/src/main/java/org/dash/wallet/common/ui/components/DashButton.kt:94-101
Timestamp: 2025-09-05T06:00:25.088Z
Learning: HashEngineering prefers to fix the DashButton alpha dimming issue (using explicit disabled colors instead of overall alpha) in a follow-up PR rather than including it in the current settings screen redesign PR.
📚 Learning: 2025-09-05T06:47:44.508Z
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1425
File: wallet/src/de/schildbach/wallet/ui/more/SettingsScreen.kt:122-151
Timestamp: 2025-09-05T06:47:44.508Z
Learning: HashEngineering prefers to defer UI edge case fixes like the CoinJoin finishing state display issue (showing "Finishing ...%" when mode is OFF but status is FINISHING) to later PRs when the main focus is on redesigning the settings screen.
Applied to files:
wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt
📚 Learning: 2025-08-08T16:48:49.964Z
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1417
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/explore/MerchantDao.kt:0-0
Timestamp: 2025-08-08T16:48:49.964Z
Learning: In PR dashpay/dash-wallet#1417, HashEngineering chose to defer adding Room indexes for gift_card_providers (provider, denominationsType, merchantId) to a follow-up PR; do not block the current PR on this optimization. Files: features/exploredash/.../data/explore/MerchantDao.kt and features/exploredash/.../data/dashspend/GiftCardProvider.kt.
Applied to files:
wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt
🧬 Code graph analysis (1)
wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt (1)
wallet/src/de/schildbach/wallet/service/platform/TopUpRepository.kt (2)
checkTopUps(88-88)checkTopUps(344-378)
🪛 detekt (1.23.8)
wallet/androidTest/de/schildbach/wallet/performance/WalletSavePerformanceTest.kt
[warning] 268-268: String.format("%02d", index % 256) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
🪛 LanguageTool
wallet/androidTest/de/schildbach/wallet/performance/README.md
[grammar] ~48-~48: This phrase is duplicated. You should probably use “abandon abandon” only once.
Context: ...nic**: "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about" - **Scri...
(PHRASE_REPETITION)
[uncategorized] ~70-~70: The preposition ‘to’ seems more likely in this position.
Context: ...t Wallets 1. Place your .wallet file in androidTest/assets/ 2. Update the tes...
(AI_HYDRA_LEO_REPLACE_IN_TO)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: check
- GitHub Check: build
🔇 Additional comments (6)
wallet/src/de/schildbach/wallet/ui/more/ContactSupportViewModel.kt (1)
76-76: Verify that 10 MB log limit is sufficient for sync stall diagnosis.The reduction from 20 MB to 10 MB is technically sound, but given the PR's focus on fixing sync stalls, please confirm that halving the log attachment size still provides adequate diagnostic information for tracking down synchronization issues.
Can you clarify the rationale for reducing the log size limit? Is this driven by email provider attachment constraints, or has analysis shown that 10 MB is sufficient for capturing sync stall events?
wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt (5)
85-85: LGTM!The
minimport is correctly added to support the timestamp comparison logic introduced later in the file.
216-216: LGTM!These logging statements improve observability for debugging sync flow issues.
Also applies to: 227-227
267-293: LGTM!The split of contact request timestamps into bidirectional tracking (
lastContactRequestTimeToMeandlastContactRequestTimeFromMe) is well-implemented. The 10-minute overlap window is a good defensive measure against clock skew and timing edge cases.
306-306: LGTM!The timestamp parameters correctly match each request direction—
lastContactRequestTimeFromMefor outgoing requests andlastContactRequestTimeToMefor incoming requests.Also applies to: 333-333
1339-1339: LGTM!Passing
truetoinitSyncensures the first contact update runs synchronously during pre-block download, which correctly blocks blockchain sync until platform data is current.
wallet/androidTest/de/schildbach/wallet/performance/WalletSavePerformanceTest.kt
Show resolved
Hide resolved
wallet/androidTest/de/schildbach/wallet/performance/WalletSavePerformanceTest.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
common/src/main/java/org/dash/wallet/common/data/BlockchainServiceConfig.kt (2)
27-32: Consider adding KDoc to clarify enum usage.The enum values are descriptive, but adding documentation would help clarify when each fix strategy is applied and what each state represents in the blockchain store recovery process.
37-37: Evaluate ifopenmodifier is needed.The class is marked
open, allowing subclassing. Consider if this is intentionally designed for extension or testing, or if it should be final by default for better encapsulation.wallet/src/de/schildbach/wallet/service/TestingSPVBlockStore.kt (1)
34-73: Test blockstore helper is straightforward; consider less extreme sleep durationThe blocking behavior via
blockGetMethodand the interrupt handling/exception mapping look fine for inducing timeouts in tests. UsingThread.sleep(Long.MAX_VALUE)is functionally OK but quite extreme; you might prefer a loop with shorter sleeps (e.g., 1–5 seconds) that still relies on interruption to break out, which is a bit clearer and avoids depending on very large sleep values.If you adopt a shorter looping sleep, just confirm that your bitcoinj timeout/
TimeoutErrorListenerstill reliably fires in your test setup.wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt (1)
583-614: Repeated inexact repeating alarm may restart service indefinitely; consider one‑shot or cancelation
rescheduleService()sets an inexact repeating alarm every time it’s called, with a 1‑minute initial delay and 15‑minute interval, and thealarmManager.cancel(alarmIntent)call is commented out. Combined with calls fromtimeoutErrorListenerand from idle detection intickReceiverwhenblockchainState?.replaying == true, this can cause the service to be woken and restarted every 15 minutes indefinitely, even after the blockchain is healthy again.To reduce long‑term background churn and battery impact, you might want to either:
- Use a one‑shot alarm (e.g.,
setExactAndAllowWhileIdle()where appropriate), or- Cancel the repeating alarm when the service reaches a clean, fully synced state (e.g., after
blockchainState.replayingis false and sync finished).Example of a one‑shot approach:
- alarmManager.setInexactRepeating( - AlarmManager.RTC_WAKEUP, - restartTime, - AlarmManager.INTERVAL_FIFTEEN_MINUTES, - alarmIntent - ) + alarmManager.setExactAndAllowWhileIdle( + AlarmManager.RTC_WAKEUP, + restartTime, + alarmIntent + )Please double‑check how
WalletApplication.scheduleStartBlockchainService()already restarts this service and decide whetherrescheduleService()should be one‑shot or explicitly canceled after successful recovery.Also applies to: 969-1003
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
build.gradle(1 hunks)common/src/main/java/org/dash/wallet/common/data/BlockchainServiceConfig.kt(1 hunks)wallet/build.gradle(1 hunks)wallet/src/de/schildbach/wallet/service/BlockchainService.java(0 hunks)wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt(30 hunks)wallet/src/de/schildbach/wallet/service/TestingSPVBlockStore.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/PeerListFragment.java(2 hunks)wallet/src/de/schildbach/wallet/ui/dashpay/PlatformRepo.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/more/ContactSupportViewModel.kt(1 hunks)
💤 Files with no reviewable changes (1)
- wallet/src/de/schildbach/wallet/service/BlockchainService.java
🚧 Files skipped from review as they are similar to previous changes (1)
- wallet/src/de/schildbach/wallet/ui/dashpay/PlatformRepo.kt
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1417
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/explore/MerchantDao.kt:0-0
Timestamp: 2025-08-08T16:48:49.964Z
Learning: In PR dashpay/dash-wallet#1417, HashEngineering chose to defer adding Room indexes for gift_card_providers (provider, denominationsType, merchantId) to a follow-up PR; do not block the current PR on this optimization. Files: features/exploredash/.../data/explore/MerchantDao.kt and features/exploredash/.../data/dashspend/GiftCardProvider.kt.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1423
File: wallet/src/de/schildbach/wallet/ui/EditProfileActivity.kt:418-446
Timestamp: 2025-08-25T14:48:39.247Z
Learning: HashEngineering prefers to refactor and reuse topup code for balance validation logic improvements in DashPay activities like EditProfileActivity, rather than implementing individual fixes in the current PR.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1422
File: common/src/main/java/org/dash/wallet/common/util/Constants.kt:68-69
Timestamp: 2025-08-25T15:00:20.777Z
Learning: HashEngineering prefers to keep HTTP logging enabled in release mode (using log.info instead of gating with BuildConfig.DEBUG) to debug production errors, even though this may leak URLs in production logs. This is a deliberate trade-off for debugging purposes in the Dash wallet project.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1410
File: wallet/src/de/schildbach/wallet/data/InvitationLinkData.kt:79-84
Timestamp: 2025-07-12T07:12:04.769Z
Learning: HashEngineering prefers to handle defensive validation improvements for URI parsing in follow-up PRs rather than including them in the current PR when the main focus is on replacing Firebase with AppsFlyer.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1425
File: wallet/src/de/schildbach/wallet/ui/more/SettingsScreen.kt:122-151
Timestamp: 2025-09-05T06:47:44.508Z
Learning: HashEngineering prefers to defer UI edge case fixes like the CoinJoin finishing state display issue (showing "Finishing ...%" when mode is OFF but status is FINISHING) to later PRs when the main focus is on redesigning the settings screen.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1422
File: wallet/src/de/schildbach/wallet/data/PaymentIntent.java:476-481
Timestamp: 2025-08-25T15:04:18.461Z
Learning: HashEngineering prefers to keep payeeUsername and payeeUserId in PaymentIntent.toString() logs unredacted for debugging purposes, prioritizing troubleshooting capability over PII protection concerns in the Dash wallet project.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1425
File: common/src/main/java/org/dash/wallet/common/ui/components/DashButton.kt:94-101
Timestamp: 2025-09-05T06:00:25.088Z
Learning: HashEngineering prefers to fix the DashButton alpha dimming issue (using explicit disabled colors instead of overall alpha) in a follow-up PR rather than including it in the current settings screen redesign PR.
📚 Learning: 2025-09-05T06:16:12.649Z
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1425
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:134-145
Timestamp: 2025-09-05T06:16:12.649Z
Learning: The dash-wallet project has a minimum SDK version of 24, so API availability checks for methods introduced in API 23 or earlier are not needed.
Applied to files:
build.gradlewallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt
📚 Learning: 2025-09-23T05:19:47.861Z
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1428
File: wallet/src/de/schildbach/wallet/ui/dashpay/PlatformRepo.kt:1065-1068
Timestamp: 2025-09-23T05:19:47.861Z
Learning: The Dash wallet app doesn't use non-encrypted wallets, so getWalletEncryptionKey() will never return null in practice. Defensive null-safe programming with ?.let is appropriate for methods like getIdentityFromPublicKeyId() in PlatformRepo.kt.
Applied to files:
wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt
📚 Learning: 2025-08-08T13:29:37.702Z
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1419
File: wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt:312-331
Timestamp: 2025-08-08T13:29:37.702Z
Learning: Dash Wallet: For network presence checks, if a transaction is not in the wallet, we generally won’t have broadcast peer data (likely 0) and chain-locks only arrive after the block containing the transaction; thus fallback confidence checks on the local transaction often remain false. It’s safe but usually non-effective to include them; primary detection should rely on the wallet-known transaction.
Applied to files:
wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt
📚 Learning: 2025-04-19T07:01:17.535Z
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
Applied to files:
wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt
🧬 Code graph analysis (1)
wallet/src/de/schildbach/wallet/ui/PeerListFragment.java (1)
wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt (2)
service(1109-1112)getConnectedPeers(1777-1779)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (9)
wallet/build.gradle (1)
288-288: androidTest/assets directory verified—change is valid.The directory exists and contains test resources (
test_wallet.wallet), confirming the gradle configuration addition is correctly referencing an active test asset location.build.gradle (1)
6-7: I need to search for more information about these library versions to verify them.Let me search for more context about this specific PR and the dashj sync stall fix.
Verify that dashjVersion 21.1.13-SNAPSHOT and dppVersion 2.0.2 exist and are accessible in your build environment.
The dashjVersion update to
21.1.13-SNAPSHOTcannot be confirmed in public Maven repositories; the latest available version in Maven Central is21.1.9. Additionally, dppVersion2.0.2does not match documented Dash Platform release versions (e.g.,2.0.0,0.24-SNAPSHOT). Before merging, confirm:
- Both versions are available in your configured repositories (likely internal or snapshot repositories)
- The SNAPSHOT dependency for dashjVersion is intentional and temporary (SNAPSHOTs can introduce build instability)
- The exact artifact groupIds and artifactIds match what your build uses—the repository lookups may have failed due to incorrect paths
common/src/main/java/org/dash/wallet/common/data/BlockchainServiceConfig.kt (1)
48-48: Ignore this review comment—the constant is properly used and accessible.The constant
BLOCKCHAIN_STORE_MEMORY_FAILUREis actively accessed elsewhere in the codebase. It's imported and used inBlockchainServiceImpl.ktwith both getter operations (lines 563, 1178) and setter operations (lines 564, 1269) viaserviceConfig.get()andserviceConfig.set(). The key correctly follows the DataStore pattern and has clear purpose: tracking and managing blockstore memory timeout recovery.Likely an incorrect or invalid review comment.
wallet/src/de/schildbach/wallet/ui/more/ContactSupportViewModel.kt (1)
77-81: Log attachment cap reduction is reasonableLowering
MAX_LOGS_SIZEfrom 20 MB to 15 MB keeps reports smaller without affecting logic; the selection and attachment loop still behaves correctly.wallet/src/de/schildbach/wallet/ui/PeerListFragment.java (1)
20-24: Defensive null handling and logging for connected peers look goodWrapping
service.getConnectedPeers()with a null check and defaulting toCollections.emptyList()avoids NPEs and is compatible with the updated non‑null API inBlockchainServiceImpl.getConnectedPeers(). The extralog.info("connected peers: {}")provides useful visibility and does not affect existingonLoadFinished()behavior.If you want to be extra strict on the non‑null contract, you could now simplify
onLoadFinished()by removing thepeers == nullbranch once you’ve confirmed no otherBlockchainServiceimplementation returns null.Also applies to: 315-323, 343-352
wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt (4)
1178-1295: Blockstore “copy‑from‑store” recovery logic is careful but complex; a few edge considerationsThe new recovery path for
BLOCKCHAIN_STORE_MEMORY_FAILURE(copying ~500 blocks/headers into temp SPV stores, swapping files, and reloading) plusverifyBlockStores()and the refinedBlockStoreExceptionhandling are well structured: temp stores are closed infinally, old files are deleted only after successful copy, and verification is logged.A couple of points to keep in mind:
- The copy loops walk backwards from
chainHeadusing the existing stores; if those stores are badly corrupted,getPrev()may still fail. You already catchExceptionand clean temp files, so behavior is safe, just worth being aware that recovery may fall back to full file deletion on subsequent runs.- The file‑lock detection branch rethrows as an
Error, which is then caught by the outerThrowablehandler later, logging and stopping the service without deleting files — this matches the intent to avoid clobbering files in use by another process.verifyBlockStore()now logs how many blocks were walked; this should help diagnose partial chains. The hardcoded depth (100) and the copy depth (500) are consistent.No blocking issues here; mainly calling out that the behavior under severe corruption is “retry until manual intervention”, which seems acceptable for this recovery path.
Also applies to: 1326-1345, 1956-2006
1592-1686: Cleanup coordination improvements look solid; minor mutex guard is helpfulUsing
isCleaningUppluscleanupDeferredto serialize cleanups acrossonDestroy()calls and coordinate with the next service’sonCreate()(cleanupDeferred?.await()) is a good improvement. The explicit logging, removal of listeners, forcedpeerGroup!!.forceStop(7_000), safe store closing, and wakelock checks all read correctly.The additional
if (checkMutex.isLocked) checkMutex.unlock()guard infinallyprotects against leaving the mutex locked if an exception occurs during cleanup. Overall shutdown sequencing is clearer and more robust now.
1095-1102: Idle‑detection rescheduling behaviorWhen idling is detected in
tickReceiverandblockchainState?.replaying == true, the new call torescheduleService()beforestopSelf()ensures stalled long‑running syncs get another chance to recover, which aligns with the PR goal of addressing sync stalls. Coupled with the non‑nullgetConnectedPeers()API (returningemptyList()whenpeerGroupis null), this makes the UI and service interactions more predictable.Just keep in mind the interaction with the repeating alarm behavior discussed earlier; the logic itself is fine.
Also applies to: 1757-1779
1700-1716: Foreground‑service timeout handling is appropriateThe
onTimeout(startId, fgsType)override logs the event, shows a user‑visible notification (showTimeoutNotification()), and callsstopSelf()inside a try/catch before delegating tosuper.onTimeout. The notification’s title selection based on CoinJoin/sync/replay state is a nice UX touch.This aligns with Android’s requirement to stop the FGS promptly on timeout and should help explain to users why background sync paused.
Please confirm against the current Android SDK docs that
onTimeoutonLifecycleServiceis available at the annotated API level and thatServiceInfo.FOREGROUND_SERVICE_TYPE_DATA_SYNCis the correct FGS type for this service.Also applies to: 1718-1747
common/src/main/java/org/dash/wallet/common/data/BlockchainServiceConfig.kt
Show resolved
Hide resolved
| /** | ||
| * Adjusts thread priorities for Android 15+ background optimization | ||
| */ | ||
| private fun adjustThreadPriorityForState(isBackground: Boolean) { | ||
| if (Build.VERSION.SDK_INT >= 35) { // Android 15 | ||
| try { | ||
| val peerGroup = this.peerGroup | ||
| if (peerGroup != null) { | ||
| if (isBackground) { | ||
| log.info("App backgrounded on Android 15+, boosting network thread priority") | ||
| // For Android 15, boost thread priority when backgrounded to counteract throttling | ||
| android.os.Process.setThreadPriority(android.os.Process.THREAD_PRIORITY_URGENT_AUDIO) | ||
| } else { | ||
| log.info("App foregrounded on Android 15+, resetting thread priority to default") | ||
| // Reset to default priority when foregrounded | ||
| android.os.Process.setThreadPriority(android.os.Process.THREAD_PRIORITY_DEFAULT) | ||
| } | ||
| } | ||
| } catch (e: Exception) { | ||
| log.warn("Failed to adjust thread priority for Android 15: {}", e.message) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Thread priority adjustment currently affects the main thread, not PeerGroup threads
adjustThreadPriorityForState(isBackground) runs in the DefaultLifecycleObserver callbacks registered with ProcessLifecycleOwner, which are invoked on the main thread. Calling android.os.Process.setThreadPriority(...) without a thread ID in this context changes the current (main) thread’s priority, not any PeerGroup/network threads.
As a result, when the app goes to background on future Android 15+ (SDK 35), this code boosts main thread priority to THREAD_PRIORITY_URGENT_AUDIO, which is very high and may have unintended scheduling/battery consequences, while not necessarily helping network I/O threads.
Unless you explicitly intend to change the main thread’s scheduling priority, consider either:
- Removing this adjustment for now, or
- Restricting it to a more moderate priority (or to a specific worker thread, if you can obtain its thread ID).
For example, to avoid extreme priorities:
- android.os.Process.setThreadPriority(android.os.Process.THREAD_PRIORITY_URGENT_AUDIO)
+ android.os.Process.setThreadPriority(android.os.Process.THREAD_PRIORITY_DEFAULT)or simply skip the background boost until you can safely target the actual network threads.
Please cross‑check Android’s latest guidance on thread priorities for background network services on API 35+; if you do decide to keep priority tweaking, ensure it targets the correct threads and uses a supported priority level.
🏁 Script executed:
#!/bin/bash
# Search for where adjustThreadPriorityForState is called
rg -n "adjustThreadPriorityForState" --type=ktLength of output: 26
🏁 Script executed:
#!/bin/bash
# Search for DefaultLifecycleObserver registration and lifecycle callbacks
rg -n "DefaultLifecycleObserver\|ProcessLifecycleOwner" --type=kt -C 5Length of output: 26
🏁 Script executed:
#!/bin/bash
# Search for other BlockchainService implementations
rg -n "class.*BlockchainService\|interface BlockchainService" --type=kt -C 3Length of output: 26
🏁 Script executed:
#!/bin/bash
# Search for getConnectedPeers implementation and usage
rg -n "getConnectedPeers" --type=kt -C 2Length of output: 26
🏁 Script executed:
#!/bin/bash
# Check if PeerListFragment exists and how it uses getConnectedPeers
rg -n "class PeerListFragment" --type=kt -A 30Length of output: 26
🌐 Web query:
Android Process.setThreadPriority() behavior thread ID parameter
💡 Result:
Short answer: the tid parameter is a Linux thread id (pid_t / LWP id). Passing 0 means "current thread"; passing a non‑zero tid targets that specific thread ID (the kernel LWP/TID). Changes may require appropriate privileges (or be limited to threads in your process). [1][2]
Sources:
- Android native headers showing androidSetThreadPriority(pid_t tid, int prio) — "Thread ID zero means current thread." [1]
- Android runtime/source references using kernel TID (pid_t) for per-thread operations. [2]
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the BlockchainServiceImpl.kt file and search for lifecycle observer registration
fd "BlockchainServiceImpl" --type f
Length of output: 64
🏁 Script executed:
#!/bin/bash
# Search for DefaultLifecycleObserver and ProcessLifecycleOwner without type restriction
rg "DefaultLifecycleObserver|ProcessLifecycleOwner" -C 5Length of output: 8189
🏁 Script executed:
#!/bin/bash
# Search for adjustThreadPriorityForState calls
rg "adjustThreadPriorityForState" -C 3Length of output: 1984
🏁 Script executed:
#!/bin/bash
# Find all BlockchainService interface/class definitions
rg "interface BlockchainService|class.*BlockchainService" -C 3Length of output: 2218
🏁 Script executed:
#!/bin/bash
# Look for getConnectedPeers implementations
rg "fun getConnectedPeers|getConnectedPeers\(" -C 2Length of output: 1648
Verified: getConnectedPeers() contract and thread priority concern
Part 1 — getConnectedPeers() non‑null contract: Verification complete. Only one BlockchainService implementation exists (BlockchainServiceImpl), and it correctly returns emptyList() when peerGroup is null. The non‑null contract is properly implemented.
Part 2 — Thread priority issue confirmed: adjustThreadPriorityForState() is called from DefaultLifecycleObserver lifecycle callbacks (onStart/onStop) registered with ProcessLifecycleOwner, which execute on the main thread. Calling setThreadPriority() without a thread ID affects the current (main) thread. Setting THREAD_PRIORITY_URGENT_AUDIO on the main thread is problematic and unintended—this boosted priority is meant for network I/O threads, not the main UI thread.
Required fix: Either remove the thread priority adjustment or change it to a moderate priority. For example:
- android.os.Process.setThreadPriority(android.os.Process.THREAD_PRIORITY_URGENT_AUDIO)
+ android.os.Process.setThreadPriority(android.os.Process.THREAD_PRIORITY_DEFAULT)Or defer the adjustment until you can safely target the actual network threads.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt (2)
169-181: UnregisterappLifecycleObserverinonDestroy()to avoid leaks and duplicate callbacksYou register
appLifecycleObserverwithProcessLifecycleOwnerinonCreate()but never remove it inonDestroy(). If the service is stopped and later restarted (e.g., viascheduleStartBlockchainServiceor the new alarms), old observers keep a strong reference to deadBlockchainServiceImplinstances and still receive lifecycle callbacks.Add a matching
removeObserver()inonDestroy():override fun onDestroy() { - log.info(".onDestroy()") - super.onDestroy() + log.info(".onDestroy()") + ProcessLifecycleOwner.get().lifecycle.removeObserver(appLifecycleObserver) + super.onDestroy()This also prevents multiple observers from stacking up and calling
adjustThreadPriorityForState()multiple times per lifecycle event.Also applies to: 1195-1207, 1646-1658
169-181: Thread-priority tweak currently boosts the main thread, not PeerGroup/network threads
DefaultLifecycleObservercallbacks run on the main thread, andandroid.os.Process.setThreadPriority(...)without a TID changes the current thread’s priority. As written, when the app goes to background on Android 15+ you’re boosting the main/UI thread toTHREAD_PRIORITY_URGENT_AUDIO, which is very high and doesn’t actually target PeerGroup/network threads.Unless you deliberately want the UI thread at audio priority, consider either disabling this for now or dialing it back to a safe level (e.g.
THREAD_PRIORITY_DEFAULT), and later, if needed, explicitly targeting the actual network threads (e.g. via a customThreadFactorythat records thread IDs).Also applies to: 2079-2101
🧹 Nitpick comments (4)
wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt (4)
560-616: TimeoutError handling + reschedule flow looks good; confirm long‑term restart cadenceThe
TimeoutErrorListenerwiring now looks correct: it’s registered viaaddTimeoutErrorListener(executor, timeoutErrorListener)and unregistered inonDestroy(), and theBLOCKSTORE_MEMORY_FAILUREflag + analytics +rescheduleService()gives you a clear recovery path.Two design points to double‑check:
rescheduleService()usesAlarmManager.setInexactRepeating(..., restartTime, AlarmManager.INTERVAL_FIFTEEN_MINUTES, alarmIntent). Despite the “restart in 1 minute” comments, this will keep waking the service about every 15 minutes after the first ~1‑minute fire until the alarm is explicitly cancelled. If you intend a one‑shot restart, considersetExactAndAllowWhileIdle/setExact(or plainset) instead, and cancelling thePendingIntentonce sync is healthy.rescheduleService()is invoked both onBLOCKSTORE_MEMORY_ACCESSand when idling whileblockchainState?.replaying == true. WithFLAG_UPDATE_CURRENTa single repeating alarm is reused, but you may still want to define when and where that alarm is cleared to avoid indefinite wakeups once the node is fully synced and stable.Functionally this is fine; just make sure the wakeup cadence matches your intended battery/UX trade‑offs.
Also applies to: 872-889, 1157-1164
200-201: Cleanup coordination withisCleaningUp/cleanupDeferredlooks solid, small simplification possibleThe new coordination logic around
isCleaningUpandcleanupDeferredinonDestroy()nicely prevents overlapping cleanups and lets new service instances wait on previous teardown viacleanupDeferred?.await().One minor simplification: because this coroutine explicitly does
checkMutex.lock()before cleanup and is the only place (besidesnetworkCallback.check()) that takes that lock, theif (checkMutex.isLocked) { checkMutex.unlock() }guard infinallyis effectively always true for this coroutine when it has progressed past the lock. You could drop theisLockedcheck and rely on structured usage of the mutex, but this is purely cosmetic and not blocking.Also applies to: 204-205, 1670-1683, 1749-1757
1388-1406: ImprovedBlockStoreExceptionhandling for file locks vs corruptionDifferentiating
OverlappingFileLockException/file‑lock cases from general corruption and not deleting the blockchain files when a lock is detected is a good call; this avoids clobbering a valid store when two services briefly contend for it.The only nit here is stylistic: you’re throwing a
java.lang.Error, which is typically reserved for JVM‑level failures; aRuntimeException(or customIllegalStateException) would be more idiomatic for “cannot safely create blockstore” while still terminating the service. Not critical, but worth considering for consistency with the rest of the codebase.
1978-2000:verifyBlockStore(s)diagnostics are useful; consider shutting down the executorThe updated
verifyBlockStore()with aStopwatchand “height delta” logging is helpful for diagnosing store health, and the wrapper that runs it viaScheduledThreadPoolExecutorwith a 1‑second timeout is reasonable.One small improvement:
verifyBlockStores()creates aScheduledThreadPoolExecutor(1)but never callsshutdown()/shutdownNow(). Over long app lifetimes this leaves an extra thread around solely for startup verification. Not urgent, but you could add atry/finallyaround the verification sequence and callscheduledExecutorService.shutdown()in thefinallyblock to clean it up.Also applies to: 2027-2077
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt(33 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1417
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/explore/MerchantDao.kt:0-0
Timestamp: 2025-08-08T16:48:49.964Z
Learning: In PR dashpay/dash-wallet#1417, HashEngineering chose to defer adding Room indexes for gift_card_providers (provider, denominationsType, merchantId) to a follow-up PR; do not block the current PR on this optimization. Files: features/exploredash/.../data/explore/MerchantDao.kt and features/exploredash/.../data/dashspend/GiftCardProvider.kt.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1423
File: wallet/src/de/schildbach/wallet/ui/EditProfileActivity.kt:418-446
Timestamp: 2025-08-25T14:48:39.247Z
Learning: HashEngineering prefers to refactor and reuse topup code for balance validation logic improvements in DashPay activities like EditProfileActivity, rather than implementing individual fixes in the current PR.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1422
File: common/src/main/java/org/dash/wallet/common/util/Constants.kt:68-69
Timestamp: 2025-08-25T15:00:20.777Z
Learning: HashEngineering prefers to keep HTTP logging enabled in release mode (using log.info instead of gating with BuildConfig.DEBUG) to debug production errors, even though this may leak URLs in production logs. This is a deliberate trade-off for debugging purposes in the Dash wallet project.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1410
File: wallet/src/de/schildbach/wallet/data/InvitationLinkData.kt:79-84
Timestamp: 2025-07-12T07:12:04.769Z
Learning: HashEngineering prefers to handle defensive validation improvements for URI parsing in follow-up PRs rather than including them in the current PR when the main focus is on replacing Firebase with AppsFlyer.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1425
File: wallet/src/de/schildbach/wallet/ui/more/SettingsScreen.kt:122-151
Timestamp: 2025-09-05T06:47:44.508Z
Learning: HashEngineering prefers to defer UI edge case fixes like the CoinJoin finishing state display issue (showing "Finishing ...%" when mode is OFF but status is FINISHING) to later PRs when the main focus is on redesigning the settings screen.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1422
File: wallet/src/de/schildbach/wallet/data/PaymentIntent.java:476-481
Timestamp: 2025-08-25T15:04:18.461Z
Learning: HashEngineering prefers to keep payeeUsername and payeeUserId in PaymentIntent.toString() logs unredacted for debugging purposes, prioritizing troubleshooting capability over PII protection concerns in the Dash wallet project.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1425
File: common/src/main/java/org/dash/wallet/common/ui/components/DashButton.kt:94-101
Timestamp: 2025-09-05T06:00:25.088Z
Learning: HashEngineering prefers to fix the DashButton alpha dimming issue (using explicit disabled colors instead of overall alpha) in a follow-up PR rather than including it in the current settings screen redesign PR.
📚 Learning: 2025-09-23T05:19:47.861Z
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1428
File: wallet/src/de/schildbach/wallet/ui/dashpay/PlatformRepo.kt:1065-1068
Timestamp: 2025-09-23T05:19:47.861Z
Learning: The Dash wallet app doesn't use non-encrypted wallets, so getWalletEncryptionKey() will never return null in practice. Defensive null-safe programming with ?.let is appropriate for methods like getIdentityFromPublicKeyId() in PlatformRepo.kt.
Applied to files:
wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt
📚 Learning: 2025-08-08T13:29:37.702Z
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1419
File: wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt:312-331
Timestamp: 2025-08-08T13:29:37.702Z
Learning: Dash Wallet: For network presence checks, if a transaction is not in the wallet, we generally won’t have broadcast peer data (likely 0) and chain-locks only arrive after the block containing the transaction; thus fallback confidence checks on the local transaction often remain false. It’s safe but usually non-effective to include them; primary detection should rely on the wallet-known transaction.
Applied to files:
wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt
📚 Learning: 2025-09-05T06:16:12.649Z
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1425
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:134-145
Timestamp: 2025-09-05T06:16:12.649Z
Learning: The dash-wallet project has a minimum SDK version of 24, so API availability checks for methods introduced in API 23 or earlier are not needed.
Applied to files:
wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt
📚 Learning: 2025-04-19T07:01:17.535Z
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
Applied to files:
wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt
📚 Learning: 2025-04-16T17:07:27.359Z
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1377
File: wallet/src/de/schildbach/wallet/service/platform/PlatformService.kt:0-0
Timestamp: 2025-04-16T17:07:27.359Z
Learning: For PlatformService in the Dash Wallet, the implementation should avoid throwing exceptions when Constants.SUPPORTS_PLATFORM is false. The platform should only be initialized when SUPPORTS_PLATFORM is true, and initialization should be done lazily to defer Platform creation until needed.
Applied to files:
wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt (3)
741-818: ModernConnectivityManager.NetworkCallbackregistration/unregistration looks correctThe new
networkCallbackimplementation (available/lost/capabilities‑changed) and the guarded registration viaregisterDefaultNetworkCallback()with anetworkCallbackRegisteredflag, plus symmetric unregistration inonDestroy(), are all correct and thread‑safe (everything funneled throughserviceScopeand theonCreateCompletedbarrier).The logging and impediment updates (
Impediment.NETWORK) +check()integration give you much better visibility and control over starting/stoppingpeerGroup; no issues from my side.Also applies to: 1415-1421, 1655-1658
1770-1787: Android 15+onTimeouthandling and user notification look correctThe
onTimeout(startId, fgsType)override cleanly handles the 6‑hour FGS timeout: logs, shows a user‑facing notification tailored to CoinJoin vs replaying vs generic background work, then callsstopSelf()and finallysuper.onTimeout(...).This is aligned with Android 15 requirements and gives users a clear explanation of why background processing paused; no changes needed.
Also applies to: 1789-1818
1848-1850: Non‑nullgetConnectedPeers()contract is a nice improvementChanging
getConnectedPeers()to always return a non‑nullList<Peer>(empty list whenpeerGroupis null) simplifies callers and removes the need for null checks without changing semantics.Looks good as implemented.
| val blockchainStoreMemoryError = serviceConfig.get(BLOCKCHAIN_STORE_MEMORY_FAILURE) ?: false | ||
| if (blockchainStoreMemoryError /*&& serviceConfig.getBlockStoreLastFix() == BlockStoreLastFix.COPY_FILES*/) { | ||
| try { | ||
| // the stores are open, copy 100 blocks to a new store. | ||
| log.info("attempting to fix blockchain memory stalls by copying from store: {}", serviceConfig.getBlockStoreLastFix()) | ||
|
|
||
| val blockchainFileTemp = File(getDir("blockstore", MODE_PRIVATE), Constants.Files.BLOCKCHAIN_FILENAME + "-temp") | ||
| val headerFileTemp = File(getDir("blockstore", MODE_PRIVATE), Constants.Files.HEADERS_FILENAME + "-temp") | ||
|
|
||
| // Clean up any existing temp files from previous failed attempts | ||
| blockchainFileTemp.delete() | ||
| headerFileTemp.delete() | ||
|
|
||
| var blockStoreTemp: SPVBlockStore? = null | ||
| var headerStoreTemp: SPVBlockStore? = null | ||
|
|
||
| try { | ||
| blockStoreTemp = SPVBlockStore(Constants.NETWORK_PARAMETERS, blockchainFileTemp) | ||
| headerStoreTemp = SPVBlockStore(Constants.NETWORK_PARAMETERS, headerFileTemp) | ||
|
|
||
| // Copy blockchain blocks | ||
| var cursor = blockStore!!.chainHead | ||
| val chainHead = cursor | ||
| blockStoreTemp.put(cursor) | ||
| var blockCount = 1 | ||
| for (i in 0 until 500) { | ||
| cursor = cursor!!.getPrev(blockStore!!) | ||
| if (cursor == null || cursor.header == Constants.NETWORK_PARAMETERS.genesisBlock) { | ||
| break | ||
| } | ||
| blockStoreTemp.put(cursor) | ||
| blockCount++ | ||
| } | ||
| blockStoreTemp.chainHead = chainHead | ||
| log.info("copied {} blocks to temp store", blockCount) | ||
|
|
||
| // Copy header blocks | ||
| cursor = headerStore!!.chainHead | ||
| val headerChainHead = cursor | ||
| headerStoreTemp.put(cursor) | ||
| var headerCount = 1 | ||
| for (i in 0 until 500) { | ||
| cursor = cursor!!.getPrev(headerStore!!) | ||
| if (cursor == null || cursor.header == Constants.NETWORK_PARAMETERS.genesisBlock) { | ||
| break | ||
| } | ||
| headerStoreTemp.put(cursor) | ||
| headerCount++ | ||
| } | ||
| headerStoreTemp.chainHead = headerChainHead | ||
| log.info("copied {} headers to temp store", headerCount) | ||
|
|
||
| // Close temp stores first | ||
| headerStoreTemp.close() | ||
| headerStoreTemp = null | ||
| blockStoreTemp.close() | ||
| blockStoreTemp = null | ||
|
|
||
| // Close old stores | ||
| blockStore!!.close() | ||
| headerStore!!.close() | ||
|
|
||
| // Delete old files | ||
| if (!blockChainFile!!.delete()) { | ||
| log.error("Failed to delete old blockchain file") | ||
| throw IOException("Failed to delete old blockchain file") | ||
| } | ||
| if (!headerChainFile!!.delete()) { | ||
| log.error("Failed to delete old header chain file") | ||
| throw IOException("Failed to delete old header chain file") | ||
| } | ||
|
|
||
| // Rename temp files to actual files | ||
| if (!blockchainFileTemp.renameTo(blockChainFile!!)) { | ||
| log.error("Failed to rename temp blockchain file") | ||
| throw IOException("Failed to rename temp blockchain file") | ||
| } | ||
| if (!headerFileTemp.renameTo(headerChainFile!!)) { | ||
| log.error("Failed to rename temp header file") | ||
| throw IOException("Failed to rename temp header file") | ||
| } | ||
|
|
||
| log.info("completed file replacement, now reloading the block stores") | ||
| blockStore = SPVBlockStore(Constants.NETWORK_PARAMETERS, blockChainFile) | ||
| blockStore?.chainHead // detect corruptions as early as possible | ||
| headerStore = SPVBlockStore(Constants.NETWORK_PARAMETERS, headerChainFile) | ||
| headerStore?.chainHead // detect corruptions as early as possible | ||
| log.info("block stores reloaded successfully") | ||
|
|
||
| // Record that we attempted this fix | ||
| serviceConfig.set(BLOCKCHAIN_STORE_LAST_FIX, BlockStoreLastFix.COPY_FROM_STORE.name) | ||
| serviceConfig.set(BLOCKCHAIN_STORE_MEMORY_FAILURE, false) | ||
| } finally { | ||
| // Ensure temp stores are closed even if an error occurs | ||
| try { | ||
| blockStoreTemp?.close() | ||
| } catch (e: Exception) { | ||
| log.warn("Error closing temp blockStore", e) | ||
| } | ||
| try { | ||
| headerStoreTemp?.close() | ||
| } catch (e: Exception) { | ||
| log.warn("Error closing temp headerStore", e) | ||
| } | ||
| } | ||
| } catch (e: Exception) { | ||
| log.error("Failed to fix blockchain memory stalls by copying from store", e) | ||
| // Clean up temp files on failure | ||
| try { | ||
| val blockchainFileTemp = File(getDir("blockstore", MODE_PRIVATE), Constants.Files.BLOCKCHAIN_FILENAME + "-temp") | ||
| val headerFileTemp = File(getDir("blockstore", MODE_PRIVATE), Constants.Files.HEADERS_FILENAME + "-temp") | ||
| blockchainFileTemp.delete() | ||
| headerFileTemp.delete() | ||
| } catch (cleanupEx: Exception) { | ||
| log.warn("Error cleaning up temp files", cleanupEx) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Blockstore COPY_FROM_STORE repair: clarify failure behavior and avoid using closed stores
The COPY_FROM_STORE repair path is a good idea, but the failure branch is a bit ambiguous:
- In the
tryblock you close the originalblockStore/headerStore, delete/rename files, and then reopen new stores. If anything in that sequence throws, thecatch (e: Exception)only logs and cleans up temp files; it does not reopen the original stores or reset any flags. - After this
catchyou still proceed toverifyBlockStores()and laterBlockChain(...)creation usingblockStore/headerStore, which may now be closed or inconsistent, likely leading to an immediateBlockStoreExceptionand hard failure on startup.
Consider tightening this by either:
- Re‑throwing on repair failure to abort startup cleanly (and potentially fall back to a full chain reset in a higher‑level handler), or
- Explicitly re‑opening the original stores (or deleting them and resetting MN lists) in the
catchso downstream code doesn’t see a half‑repaired/closed store.
Also, you might want to record that COPY_FROM_STORE failed (e.g., another BlockStoreLastFix state) to avoid re‑attempting the same repair on every start if it cannot succeed on this device.
|
The comments will be addressed in the regression PR. |
…fix/sync-stalls
Issue being fixed or feature implemented
Related PR's and Dependencies
dashpay/dashj#287
Screenshots / Videos
How Has This Been Tested?
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.