[shumkov]

Issue being fixed or feature implemented

rs-dapi dedupes the raw transactions it streams to each subscribeToTransactionsWithProofs subscription by txid.

From protocol version 15, Platform emits asset unlocks with payload version 2. Their txid leaves out the request height, quorum hash and signature, so every instance Platform re-signs after an expiry has the same txid. Core 24 relays each instance (TransactionAddedToMempool fires for the refreshed one). A subscriber that already received an earlier instance was never sent the refreshed instance, or the one that was mined, so it kept a stale requested height, quorum hash and signature.

Found in the review of #5634 (finding 5). The problem is outside that PR's diff, but its switch to version 2 unlocks is what triggers it. It does not affect consensus.

What was done?

• TransactionStreamState::mark_transaction_delivered now takes the transaction. It marks:

  • the txid, as before. Instant locks and merkle blocks name a transaction by txid, so they keep looking it up there.
  • the instance hash, when it differs from the txid. This is rust-dashcore's Transaction::instance_hash, the hash of the full serialization, and it is what tells re-signed instances apart. It only ever differs for an asset unlock with payload version 2 or higher, and those instances go into a new bounded per-stream set, delivered_tx_instances.

• So a stream dedupes raw transactions by instance. For every other transaction, behaviour and per-stream memory are unchanged.

• All three paths use it:

  • the live ZMQ path
  • the mempool snapshot
  • the history replay (mark_transactions_delivered now takes the decoded matching transactions)

  An instance the replay delivered is therefore not streamed again when Core reports it live.

• The live path decodes the transaction once. It used to decode it twice, once for the txid bytes and once for the hex.

How Has This Been Tested?

TDD:

• should_stream_every_instance_of_a_re_signed_asset_unlock feeds two instances of one version 2 asset unlock (same txid, different instance hashes), then the first instance again, through flush_transaction_pending.
  • Against the unfixed code it failed: left: [0x808a…b8dd] (only the first instance was streamed), right: [0x808a…b8dd, 0xc45c…6c98].
  • With the fix, both instances are streamed once each, in order, and the txid still counts as delivered for instant locks and merkle blocks.
• should_not_stream_again_the_asset_unlock_instances_a_replay_delivered covers the history path: instances a replay delivered are skipped live, and a new instance is not.
• should_dedupe_transactions_blocks_and_instant_locks now passes a transaction instead of raw txid bytes.

Commands:

• cargo test -p rs-dapi: 346 passed, 0 failed, 0 ignored.
• cargo clippy -p rs-dapi --all-features --all-targets: no warnings in rs-dapi. Under rs-dapi's feature set, clippy reports three doc lints in dpp, in data_contract_create_transition/v0/mod.rs:31 and shield_from_asset_lock_transition/v0/mod.rs:62-63. Those files are not touched here and give the same warnings on v5.1-dev.
• cargo fmt --all -- --check: clean.

Breaking Changes

None. Subscribers now receive each instance of a version 2 asset unlock once, where before they received only the first. Every other transaction is streamed exactly as before.

In-place changes to shipped generations

None. rs-dapi is not versioned consensus code.

Checklist:

• I have performed a self-review of my own code
• My Rust, FFI, WASM, Swift or Kotlin changes follow the coding conventions
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
• I have made corresponding changes to the documentation if needed
• If I added or changed GroveDB structure, I described it in the area's structure.rs, regenerated grovedb-structure.json, and checked the structure viewer link posted on this pull request
• If I added or changed a system contract's document schemas, I checked the contract viewer link posted on this pull request

For repository code-owners and collaborators only

• I have assigned this pull request to a milestone

🤖 Generated with Claude Code

PR Hygiene · 45e4c9d

• Bots — thepastaclaw ✓
• Self-review — post /self-reviewed
• Reviewer requests paused — your 5 review slots are occupied; this PR is excluded from reviewers' queues. Required approvals still count without a slot.
• Build green
• Approvals
  • rust-dapi (packages/rs-dapi/src/services/streaming_service/mod.rs, packages/rs-dapi/src/services/streaming_service/transaction_stream.rs) — QuantumExplorer or lklimek

When every merge requirement is met, the PR Hygiene check passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.

Summary by CodeRabbit

• Bug Fixes
  • Transaction streaming now distinguishes different transaction instances that share the same transaction ID, so each distinct instance can be delivered once.
  • Replayed instances continue to be suppressed, while previously unseen instances can be delivered during live, mempool, and historical processing.

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

Transaction streaming now distinguishes instances that share a txid by tracking their instance hashes. Live, mempool, and historical paths use transaction delivery identities for deduplication, while historical Merkle matching continues to use txids.

Changes: Transaction streaming

Layer / File(s)	Summary
Track transaction delivery identities packages/rs-dapi/src/services/streaming_service/transaction_stream.rs, packages/rs-dapi/src/services/streaming_service/mod.rs	The stream state tracks txids and distinct instance hashes in bounded sets. Delivery methods update both sets. The unused txid_bytes_from_bytes helper is removed.
Apply instance-aware deduplication to live and mempool delivery packages/rs-dapi/src/services/streaming_service/transaction_stream.rs	Live transaction events are deserialized before delivery, and live and mempool paths use delivery identities for duplicate detection. Invalid live transactions are skipped.
Preserve historical matching and test instance replay packages/rs-dapi/src/services/streaming_service/transaction_stream.rs	Historical processing uses decoded transactions for bloom matching and delivery tracking. Merkle match flags still use txids. Tests cover distinct asset-unlock instances, repeated-instance suppression, and replay behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Possibly related issues

• feat(platform): move to rust-dashcore dev and decode Core v24 shared masternode transactions (types 10, 11, 12) #5437: The issue identifies the AssetUnlock v2 distinction between txid and instance hash, which this PR handles in transaction streaming.

Possibly related PRs

• dashpay/platform#5417: Adds SQLite alias handling for historical AssetUnlock instances, while this PR applies instance identity to DAPI streaming.
• dashpay/platform#5634: Tests re-signed AssetUnlocks that retain a txid but have distinct instance hashes, the identity case addressed by this PR.
• dashpay/platform#5150: Handles AssetUnlock identity in SQLite history accounting rather than DAPI streaming.

Suggested reviewers: quantumexplorer

Merge Risk: Minimal · up to 45e4c

The change makes the transaction stream deliver every instance of a re-signed asset unlock while leaving other transactions unchanged. No concrete merge-blocking risk was identified.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly summarizes the main change: streaming every distinct instance of a re-signed asset unlock.
Docstring Coverage		Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentati…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 7 functions across 1 files.

Finishing Touches

Generate docstrings

• Commit to this branch
• Create a new PR

Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

• X
• Mastodon
• Reddit
• LinkedIn

Recent review info

Run configuration

• Source mode: Auto
• Repository: dashpay/platform
• Branch: shumkov/v51-dapi-asset-unlock-instances
• File: .coderabbit.yaml
• Commit: 45e4c9d
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: ccd19dfa-5228-4331-be3f-0ec00fad330d

Commits

Reviewing files that changed from the base of the PR and between 9b36712 and 45e4c9d.

Files selected for processing: 2

• packages/rs-dapi/src/services/streaming_service/mod.rs
• packages/rs-dapi/src/services/streaming_service/transaction_stream.rs

Files with no reviewable changes: 1

• packages/rs-dapi/src/services/streaming_service/mod.rs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

[github-actions]

Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[thepastaclaw]

✅ Final review complete — no blockers (commit 45e4c9d) · triage: normal

[thepastaclaw]

Final validation — Phase 1 + Phase 2

At head 393aa1c, the instance-aware deduplication correctly preserves txid-based proof correlation across live delivery, mempool snapshots, and history replay. Two non-blocking improvements remain: avoid retaining decoded history transactions solely for identifier bookkeeping, and clarify the deserialization-failure log label. This was a static review; current CI reports successful Rust workspace tests with no failed code checks, while review and hygiene checks remain pending.

🟡 1 suggestion(s) | 💬 1 nitpick(s)

Approved: nothing here blocks the merge. The non-blocking findings are optional: fix them forward, in this PR or a follow-up, without waiting for another review round.

Review provenance

Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: low by gpt-6.1-sol (effort low) — This is a contained, single-file deduplication fix with focused regression tests across delivery paths, and it does not change consensus, signature verification, or network deserialization rules.
• Phase 1 reviewers: glm-5.3-flash — general (completed, effort high); agent phase1-reviewer, glm-5.3-flash — rust-quality (completed, effort high); agent phase1-reviewer
• Phase 1 model: glm-5.3-flash — zai quota: 5h 100% left, weekly 93% left; passed over gemini-3.8-flash-high (antigravity quota lookup failed)
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort medium); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review (PastaClaw). How to work through them:
- Verify each finding against the code; fix only what is real, as narrowly as the finding.
- If a finding is wrong, decline it on its thread in one or two sentences; the bot answers once and stops.
- To defer one, link a tracking issue on its thread (or reply asking the bot to file one).
- Suggestions are optional: fix them forward in a later PR rather than pushing another round just for them.
- Address every current finding in one push: self-review the full diff first, and don't push piecemeal.
- Keep the PR's scope; don't grow it to satisfy hardening suggestions.
- Carried items were raised in an earlier round: read the thread first, and don't re-fix what you already fixed, declined or deferred there. Disputed and deferred items are already handled and are not listed.

In `packages/rs-dapi/src/services/streaming_service/transaction_stream.rs`:
- [SUGGESTION] packages/rs-dapi/src/services/streaming_service/transaction_stream.rs:1296-1300: Retain delivery identifiers rather than decoded history transactions
  The new `matching_transactions` collection retains every matching decoded transaction alongside its serialized bytes until the block iteration ends, including across response sends. Previously, this bookkeeping retained only txid bytes. The decoded inputs, outputs, scripts, and payloads are needed only to derive delivery identifiers, so this adds avoidable per-block memory and allocation overhead. Successfully decoded bloom matches are also parsed again here after their earlier decode at line 1266. Reuse a single decode per transaction and collect only txid/instance-hash pairs for a batched marking API, preserving the existing raw-bytes bloom fallback when decoding fails.
- [NITPICK] packages/rs-dapi/src/services/streaming_service/transaction_stream.rs:561: Name the log event after the deserialization failure
  The rewritten guard skips the event because transaction deserialization failed, but the retained `transaction_no_txid` label suggests a transaction-identity problem instead. The old helpers also deserialized internally, so the failure condition has not changed; this is a diagnostic naming cleanup on the guard being rewritten. Use a deserialization-specific label so malformed or unsupported payloads are identifiable in production logs.

[github-actions]

Bots are done — your move: post /self-reviewed.
Full checklist in the description.

[github-actions]

Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

The reviewed head correctly distinguishes re-signed asset-unlock instances while preserving txid-based InstantSend and Merkle bookkeeping. The two prior findings are fixed, and no new actionable issues were identified; the supplied Rust workspace CI check passed, with only review/hygiene checks still pending.

🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: normal by gpt-6.1-sol (effort low) — This is a contained but nontrivial Rust streaming-state change affecting transaction deduplication across live, mempool, and replay paths, without modifying consensus, cryptography, or funds movement.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.

No unresolved findings remain from the prior review on this head.

[github-actions]

Bots are done — your move: post /self-reviewed.
Full checklist in the description.