-
Notifications
You must be signed in to change notification settings - Fork 56
fix(platform-wallet): lossless mpsc persistence drain — root-cause fix for the sync-watermark freeze #4290
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
+997
−256
Closed
fix(platform-wallet): lossless mpsc persistence drain — root-cause fix for the sync-watermark freeze #4290
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
3e8c27f
fix(platform-wallet): batch wallet-event persistence so the sync wate…
bfoss765 e646622
feat(ffi/jni/kotlin): expose sync_fault_detected to the host
bfoss765 c1d5d15
fix(platform-wallet): drain the lossless persistence channel (mpsc) s…
bfoss765 3c57cb1
fix(platform-wallet): declare log outside the reverted txMetadata hun…
bfoss765 ce9cc1a
fix(platform-wallet): report only store-accepted heights as persisted
bfoss765 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Blocking: Repin the temporary contributor-fork dependencies before merge
All eight rust-dashcore workspace dependencies now use
bfoss765/rust-dashcoreatb5dff6de05e4a354680e5b01a54bae9e642a6ad0. The head commit explicitly describes this as a temporary pin and states that dashpay/rust-dashcore#924 must land; that producer PR is currently open and unmerged, with this revision as its contributor-fork head. Merging this exact head would make clean production builds depend on a contributor-controlled repository rather than the project's governed upstream. After #924 lands, repin these entries to the resulting revision inhttps://github.com/dashpay/rust-dashcoreand updateCargo.lockin the same commit.source: ['codex']
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledged, and the finding is correct — but it resolves upstream rather than by a change on this branch, so the pin is deliberately unchanged.
State verified just now via the API:
mpscpersistence channel this consumer drains — is OPEN, basedev, headbfoss765:fix/wallet-event-persistence-mpsc@2af691ba,mergeStateStatus: BLOCKED,REVIEW_REQUIRED.CHANGES_REQUESTED); its content is carried in the same fork revision.There is no governed-upstream revision to point at yet: both producer PRs originate from the contributor fork into
dashpay/rust-dashcore:dev, so every revision containingtake_persistence_receiver()exists only on the fork today. Repinning now would mean reverting this PR's root-cause fix back onto the lossy broadcast path that caused #4069 in the first place.So this is a genuine merge blocker and should stay flagged — the resolution is sequencing, not a code change here. Once #909 and #924 land on
dashpay/rust-dashcore:dev, this branch repins all eight workspace entries tohttps://github.com/dashpay/rust-dashcoreat the merged revision, withCargo.lockupdated in the same commit. Happy to keep this thread open as that gate.One note for whoever performs the repin, found while verifying the above: the pinned revision
b5dff6dehas diverged from #924's current head (20 ahead / 1 behind). #924 has since gainedf0ab7d89, which makes the persistence channel opt-in — the send half isNoneuntiltake_persistence_receiver()lazily creates it. This consumer already satisfies that contract (the manager takes the receiver exactly once, before it is published to producers), so no adaptation is needed here, but the repin should target the mergeddevrevision rather than a rebase ofb5dff6de.