fix: improve shutdown of blockchainservice#1482
Open
HashEngineering wants to merge 18 commits into
Open
Conversation
…dash-wallet into fix/shutdown-blockchainservice
…fix/shutdown-blockchainservice
Contributor
📝 WalkthroughWalkthroughAdds transaction-cache completeness checks with an automatic rebuild trigger, introduces a CoinJoin shutdown coordination hook awaited during service teardown, updates wallet wipe signaling, and applies small UI and dialog handling tweaks. ChangesTransaction Cache Completeness Verification
Graceful Shutdown Coordination (CoinJoin + Networking)
Miscellaneous UI / App State
Sequence Diagram(s)sequenceDiagram
participant System as App/System
participant CoinJoin as CoinJoinService
participant WalletService as BlockchainServiceImpl
participant PeerGroup as PeerGroup/Networking
WalletService->>CoinJoin: call prepareForShutdown()
CoinJoin->>CoinJoin: set shutdown flag
CoinJoin->>CoinJoin: await refreshKeysJob completion (<=10s)
alt refreshKeysJob finishes
CoinJoin-->>WalletService: returned (ready)
else timeout
CoinJoin-->>WalletService: returned (timeout)
end
WalletService->>PeerGroup: peerGroup.stop()
PeerGroup-->>WalletService: stopped
WalletService->>System: shutdown Dash system
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
wallet/src/de/schildbach/wallet/ui/more/AboutActivity.kt (2)
41-41: Check ktlint import ordering.
androidx.core.net.toUriis placed after non-AndroidX imports (e.g.,org.slf4j.LoggerFactory). If this repo enforces a strict import order via ktlint, this may cause a formatting/lint failure. Consider reordering imports to match the project’s ktlint rules.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/ui/more/AboutActivity.kt` at line 41, The import ordering in AboutActivity.kt is incorrect for ktlint: move the AndroidX import androidx.core.net.toUri so it appears with other AndroidX/android imports before third-party imports like org.slf4j.LoggerFactory; update the import block in AboutActivity (the file containing AboutActivity class) to follow the project's ktlint ordering rules (group standard Kotlin/Java, Android/AndroidX, then third-party) to resolve the lint failure.
71-75: Harden the GitHub link click handling against malformed/blank URL text.
binding.githubLink.text.toString()is assumed to be a valid absolute URL. If the displayed text is ever changed (or contains whitespace / localized formatting),ACTION_VIEWcan fail or the URI conversion can throw. Add a small guard (trim,isNullOrBlank) and optionally wrap intry/catchwith logging + early return.Suggested change
binding.githubLink.setOnClickListener { - val i = Intent(ACTION_VIEW) - i.data = binding.githubLink.text.toString().toUri() - startActivity(i) + val url = binding.githubLink.text?.toString()?.trim().orEmpty() + if (url.isBlank()) { + log.warn("GitHub link text is blank; cannot launch ACTION_VIEW") + return@setOnClickListener + } + try { + startActivity(Intent(ACTION_VIEW, url.toUri())) + } catch (ex: Exception) { + log.error("Failed to open GitHub link: $url", ex) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/ui/more/AboutActivity.kt` around lines 71 - 75, Protect the click handler for binding.githubLink by trimming and validating the link text before converting to a URI: inside the setOnClickListener for binding.githubLink, call binding.githubLink.text?.toString()?.trim() and check isNullOrBlank(), returning early if invalid; then wrap the Intent(ACTION_VIEW)/toUri()/startActivity calls in a try/catch and log the exception (or silently return) to avoid crashes on malformed URIs (reference setOnClickListener, binding.githubLink, Intent(ACTION_VIEW), toUri, startActivity).build.gradle (1)
2-47: (Optional) Consider avoiding-SNAPSHOTon stable branches / ensure CI reproducibility.Using
*-SNAPSHOTversions can make builds non-reproducible over time unless CI strictly pins or uses a deterministic snapshot strategy. If this is intended to ship to users, consider whether the target branch should rely on a released version instead of a snapshot, or whether there’s an existing policy for snapshot usage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.gradle` around lines 2 - 47, The gradle ext properties use snapshot versions (dppVersion = "2.0.5-SNAPSHOT" and dashjVersion = '22.0.2-CJ-SNAPSHOT') which harms reproducibility; update these properties to a released version (remove the -SNAPSHOT) or, if snapshots are required, add a clear comment and CI pinning strategy in the file (e.g., reference to a pinned artifact resolver or job that publishes the snapshot) so dppVersion and dashjVersion are either fixed to a release string or documented/pinned for deterministic CI builds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wallet/src/de/schildbach/wallet/service/CoinJoinService.kt`:
- Around line 105-106: prepareForShutdown()'s shutdown barrier is lossy because
isShuttingDown and refreshKeysJob are sampled without synchronization, allowing
prepareMixing()/startMixing() to slip in and call refreshUnusedKeys() after
shutdown decided it was safe; fix by making the shutdown flag and job
publication atomic/synchronized: use a single Mutex or AtomicBoolean + volatile
refreshKeysJob so prepareForShutdown() sets isShuttingDown atomically, then
publishes/captures refreshKeysJob under the same lock, awaits its completion,
and only clears/changes shutdown state after awaiting; update
startMixing()/prepareMixing() and refreshUnusedKeys() checks to test
isShuttingDown under that same mutex (or use compare-and-set on an
AtomicBoolean) so no new refreshKeysJob can be created after shutdown is
initiated, and remove the premature clearing of the flag (the clear at
refreshKeysJob completion should happen only when safe under the same lock).
In `@wallet/src/de/schildbach/wallet/service/TxDisplayCacheService.kt`:
- Around line 413-426: The early-return in rebuildIfCacheIncomplete() that
checks txGroupCacheDao.getTotalTxCount() == 0 causes a missed rebuild when the
app crashed before any group-cache write but the wallet already contains
transactions; instead, first obtain the wallet (walletData.wallet or
observeWallet()) and its wallet.getTransactionCount(true), and only return early
when the wallet has zero transactions; otherwise proceed to compute
cachedTxCount (txGroupCacheDao.getTotalTxCount()), groupCount
(txGroupCacheDao.getGroupCount()) and displayRowCount
(txDisplayCacheDao.getCount()) and trigger the rebuild logic—i.e., remove or
change the initial total-tx-count short-circuit to depend on
wallet.getTransactionCount(true) == 0.
---
Nitpick comments:
In `@build.gradle`:
- Around line 2-47: The gradle ext properties use snapshot versions (dppVersion
= "2.0.5-SNAPSHOT" and dashjVersion = '22.0.2-CJ-SNAPSHOT') which harms
reproducibility; update these properties to a released version (remove the
-SNAPSHOT) or, if snapshots are required, add a clear comment and CI pinning
strategy in the file (e.g., reference to a pinned artifact resolver or job that
publishes the snapshot) so dppVersion and dashjVersion are either fixed to a
release string or documented/pinned for deterministic CI builds.
In `@wallet/src/de/schildbach/wallet/ui/more/AboutActivity.kt`:
- Line 41: The import ordering in AboutActivity.kt is incorrect for ktlint: move
the AndroidX import androidx.core.net.toUri so it appears with other
AndroidX/android imports before third-party imports like
org.slf4j.LoggerFactory; update the import block in AboutActivity (the file
containing AboutActivity class) to follow the project's ktlint ordering rules
(group standard Kotlin/Java, Android/AndroidX, then third-party) to resolve the
lint failure.
- Around line 71-75: Protect the click handler for binding.githubLink by
trimming and validating the link text before converting to a URI: inside the
setOnClickListener for binding.githubLink, call
binding.githubLink.text?.toString()?.trim() and check isNullOrBlank(), returning
early if invalid; then wrap the Intent(ACTION_VIEW)/toUri()/startActivity calls
in a try/catch and log the exception (or silently return) to avoid crashes on
malformed URIs (reference setOnClickListener, binding.githubLink,
Intent(ACTION_VIEW), toUri, startActivity).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf5eda34-cda1-43e0-9b41-8a7d38eb48f9
📒 Files selected for processing (8)
build.gradlewallet/src/de/schildbach/wallet/WalletApplication.javawallet/src/de/schildbach/wallet/database/dao/TxGroupCacheDao.ktwallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.ktwallet/src/de/schildbach/wallet/service/CoinJoinService.ktwallet/src/de/schildbach/wallet/service/TxDisplayCacheService.ktwallet/src/de/schildbach/wallet/ui/compose_views/CreateInstantUsernameDialog.ktwallet/src/de/schildbach/wallet/ui/more/AboutActivity.kt
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
common/src/main/java/org/dash/wallet/common/ui/dialogs/OffsetDialogFragment.kt (1)
63-72:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBottom padding accumulates on every insets dispatch — causes ever-growing padding.
v.paddingBottom + sys.bottomadds to whatever padding is currently set. Each time the insets listener fires (keyboard toggle, window focus change, re-attach) the bottom padding grows unboundedly. The original padding baseline must be captured once before the listener is set.🐛 Proposed fix
+ val originalBottomPadding = view.paddingBottom ViewCompat.setOnApplyWindowInsetsListener(view) { v, insets -> val sys = insets.getInsets(WindowInsetsCompat.Type.systemBars()) v.setPadding( v.paddingLeft, v.paddingTop, v.paddingRight, - v.paddingBottom + sys.bottom + originalBottomPadding + sys.bottom ) WindowInsetsCompat.CONSUMED }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@common/src/main/java/org/dash/wallet/common/ui/dialogs/OffsetDialogFragment.kt` around lines 63 - 72, The insets listener incorrectly accumulates bottom padding by adding sys.bottom to the current padding on each dispatch; capture the original bottom padding once before calling ViewCompat.setOnApplyWindowInsetsListener (e.g. read v.paddingBottom into an initialBottomPadding variable in OffsetDialogFragment) and inside the listener call v.setPadding(v.paddingLeft, v.paddingTop, v.paddingRight, initialBottomPadding + sys.bottom) so you always base adjustments off the original baseline rather than the mutated padding.
🧹 Nitpick comments (2)
common/src/main/java/org/dash/wallet/common/ui/dialogs/OffsetDialogFragment.kt (2)
112-115: ⚡ Quick win
is AppCompatImageButtoncheck silently skips icon assignment for non-AppCompat views.If any
collapse_buttonin a layout is a plainImageButton(or anotherImageViewsubclass), thesetImageResourcecall is never reached and the button retains whatever src was set in XML. Consider broadening the check toImageView, which covers all common button types that exposesetImageResource.♻️ Proposed refactor
-import androidx.appcompat.widget.AppCompatImageButton +import android.widget.ImageView view.findViewById<View?>(R.id.collapse_button)?.apply { - if (this is AppCompatImageButton) { + if (this is ImageView) { setImageResource(R.drawable.ic_popup_close_circle) } setOnClickListener { dismiss() } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@common/src/main/java/org/dash/wallet/common/ui/dialogs/OffsetDialogFragment.kt` around lines 112 - 115, The current check in OffsetDialogFragment for the collapse_button only handles AppCompatImageButton, so replace the type guard with a broader ImageView check: locate the view lookup using view.findViewById(R.id.collapse_button) in OffsetDialogFragment and change the conditional from "if (this is AppCompatImageButton)" to "if (this is ImageView)" (or remove the specific class check and safely cast to ImageView) and then call setImageResource(R.drawable.ic_popup_close_circle) so all ImageView-based buttons (including plain ImageButton and AppCompatImageButton) receive the icon.
123-127: ⚡ Quick win
launchWhenResumedis deprecated and scheduled for removal.
Lifecycle.launchWhenXmethods have been deprecated as the use of a pausing dispatcher can lead to wasted resources in some cases; it is recommended to useLifecycle.repeatOnLifecycleinstead. For this one-shot "show dialog" use-case,withResumedis the idiomatic replacement.♻️ Proposed fix
fun show(activity: FragmentActivity) { - activity.lifecycleScope.launchWhenResumed { - show(activity.supportFragmentManager, "offset_dialog") + activity.lifecycleScope.launch { + activity.lifecycle.withResumed { + show(activity.supportFragmentManager, "offset_dialog") + } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@common/src/main/java/org/dash/wallet/common/ui/dialogs/OffsetDialogFragment.kt` around lines 123 - 127, The use of the deprecated launchWhenResumed inside OffsetDialogFragment.show should be replaced with the one-shot withResumed pattern: call activity.lifecycleScope.withResumed { show(activity.supportFragmentManager, "offset_dialog") } so the dialog is shown when resumed without using the deprecated pausing dispatcher; update the imports to use androidx.lifecycle.lifecycleScope and androidx.lifecycle.withResumed (or the corresponding lifecycle-runtime-ktx extension) and remove launchWhenResumed usage in the show(activity: FragmentActivity) method.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@common/src/main/java/org/dash/wallet/common/ui/dialogs/OffsetDialogFragment.kt`:
- Around line 63-72: The insets listener incorrectly accumulates bottom padding
by adding sys.bottom to the current padding on each dispatch; capture the
original bottom padding once before calling
ViewCompat.setOnApplyWindowInsetsListener (e.g. read v.paddingBottom into an
initialBottomPadding variable in OffsetDialogFragment) and inside the listener
call v.setPadding(v.paddingLeft, v.paddingTop, v.paddingRight,
initialBottomPadding + sys.bottom) so you always base adjustments off the
original baseline rather than the mutated padding.
---
Nitpick comments:
In
`@common/src/main/java/org/dash/wallet/common/ui/dialogs/OffsetDialogFragment.kt`:
- Around line 112-115: The current check in OffsetDialogFragment for the
collapse_button only handles AppCompatImageButton, so replace the type guard
with a broader ImageView check: locate the view lookup using
view.findViewById(R.id.collapse_button) in OffsetDialogFragment and change the
conditional from "if (this is AppCompatImageButton)" to "if (this is ImageView)"
(or remove the specific class check and safely cast to ImageView) and then call
setImageResource(R.drawable.ic_popup_close_circle) so all ImageView-based
buttons (including plain ImageButton and AppCompatImageButton) receive the icon.
- Around line 123-127: The use of the deprecated launchWhenResumed inside
OffsetDialogFragment.show should be replaced with the one-shot withResumed
pattern: call activity.lifecycleScope.withResumed {
show(activity.supportFragmentManager, "offset_dialog") } so the dialog is shown
when resumed without using the deprecated pausing dispatcher; update the imports
to use androidx.lifecycle.lifecycleScope and androidx.lifecycle.withResumed (or
the corresponding lifecycle-runtime-ktx extension) and remove launchWhenResumed
usage in the show(activity: FragmentActivity) method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76ecb716-872f-4363-b195-e23a15ac19cf
📒 Files selected for processing (1)
common/src/main/java/org/dash/wallet/common/ui/dialogs/OffsetDialogFragment.kt
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.
Issue being fixed or feature implemented
Related PR's and Dependencies
Screenshots / Videos
How Has This Been Tested?
Checklist:
Summary by CodeRabbit
Bug Fixes
Improvements