Skip to content

fix(spv): match masternode owner/voting key hashes in compact filters#863

Merged
xdustinface merged 1 commit into
dashpay:devfrom
xdustinface:fix/spv-mn-owner-voting-filter
Jul 10, 2026
Merged

fix(spv): match masternode owner/voting key hashes in compact filters#863
xdustinface merged 1 commit into
dashpay:devfrom
xdustinface:fix/spv-mn-owner-voting-filter

Conversation

@xdustinface

@xdustinface xdustinface commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

During SPV historical sync, dash-spv chose which blocks to download by matching the wallet's watched scriptPubKeys against BIP157/158 compact filters. The matcher built its FilterQuery from scriptPubKeys only. Dash Core, however, also inserts a ProRegTx's owner (keyIDOwner) and voting (keyIDVoting) key hashes into the block's compact filter as bare 20-byte hash160 elements, not as P2PKH scripts (see ExtractSpecialTxFilterElements). A masternode owner who controls only the owner/voting keys, with no wallet-owned input or output in the ProRegTx, therefore never matched, the block was never downloaded, and the payload-aware wallet scan never ran on it.

This adds each provider owner and voting key hash (bare 20-byte hash160) to the compact-filter query. key-wallet surfaces them via WalletInfoInterface::monitored_filter_elements, extracting the same bytes (same order) the BIP37 bloom path already inserts via address_payload_bytes. key-wallet-manager exposes WalletInterface::monitored_filter_elements_for and folds the extra elements into the shared FilterQuery (its other length bucket) in check_compact_filters_for_elements (renamed from check_compact_filters_for_script_pubkeys). dash-spv's filter manager now passes each wallet's elements alongside its scripts in both the batch scan and the new-script rescan, including the per-wallet attribution queries.

Step 1 of #861: owner/voting only. Collateral outpoint and proTxHash follow in stacked branches.

Summary by CodeRabbit

  • New Features

    • Compact block-filter scanning now detects additional monitored wallet elements, including provider owner and voting key hashes.
    • Wallet rescans and synchronization attribute matching blocks more accurately across monitored wallets.
    • Wallet interfaces now expose configured additional filter elements for monitoring.
  • Bug Fixes

    • Fixed missed matches when relevant data appears as a bare compact-filter element rather than a script-based match.

During SPV historical sync, `dash-spv` chose which blocks to download by matching the wallet's watched scriptPubKeys against BIP157/158 compact filters. The matcher built its `FilterQuery` from scriptPubKeys only. Dash Core, however, also inserts a `ProRegTx`'s owner (`keyIDOwner`) and voting (`keyIDVoting`) key hashes into the block's compact filter as bare 20-byte `hash160` elements, not as P2PKH scripts (see `ExtractSpecialTxFilterElements`). A masternode owner who controls only the owner/voting keys, with no wallet-owned input or output in the `ProRegTx`, therefore never matched, the block was never downloaded, and the payload-aware wallet scan never ran on it.

This adds each provider owner and voting key hash (bare 20-byte `hash160`) to the compact-filter query. `key-wallet` surfaces them via `WalletInfoInterface::monitored_filter_elements`, extracting the same bytes (same order) the BIP37 bloom path already inserts via `address_payload_bytes`. `key-wallet-manager` exposes `WalletInterface::monitored_filter_elements_for` and folds the extra elements into the shared `FilterQuery` (its `other` length bucket) in `check_compact_filters_for_elements` (renamed from `check_compact_filters_for_script_pubkeys`). `dash-spv`'s filter manager now passes each wallet's elements alongside its scripts in both the batch scan and the new-script rescan, including the per-wallet attribution queries.

Step 1 of dashpay#861: owner/voting only. Collateral outpoint and `proTxHash` follow in stacked branches.
@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Wallet compact-filter matching now includes monitored bare provider owner and voting key hashes alongside scriptPubKeys. Wallet interfaces expose these elements, the matcher accepts them, and SPV scan/rescan attribution uses both query dimensions.

Changes

Compact filter element matching

Layer / File(s) Summary
Wallet monitored element sources
key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs, key-wallet-manager/src/wallet_interface.rs, key-wallet-manager/src/process_block.rs
Wallet metadata extracts provider owner and voting key hash elements, and wallet-manager interfaces expose them with empty defaults.
Element-aware compact-filter matcher
key-wallet-manager/src/matching.rs, key-wallet-manager/src/lib.rs
Compact-filter queries combine scriptPubKeys with extra elements; exports and tests cover the updated API and bare hash matching.
SPV scan and rescan integration
dash-spv/src/sync/filters/manager.rs
Batch scans snapshot, union, match, and attribute wallet scripts and bare elements while preserving existing batching and event flow.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant WalletInfo
  participant WalletManager
  participant FiltersManager
  participant CompactFilterMatcher

  WalletInfo->>WalletManager: provide monitored filter elements
  WalletManager->>FiltersManager: return wallet scripts and elements
  FiltersManager->>CompactFilterMatcher: match unioned scripts and elements
  CompactFilterMatcher-->>FiltersManager: return matching filter keys
  FiltersManager->>WalletManager: attribute matches per wallet
Loading

Possibly related issues

Possibly related PRs

Suggested labels: ready-for-review

Suggested reviewers: ZocoLini

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: compact-filter matching now includes masternode owner/voting key hashes.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.00000% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.61%. Comparing base (1860089) to head (a2b34ca).
⚠️ Report is 2 commits behind head on dev.

Files with missing lines Patch % Lines
...allet/managed_wallet_info/wallet_info_interface.rs 0.00% 13 Missing ⚠️
key-wallet-manager/src/process_block.rs 0.00% 6 Missing ⚠️
key-wallet-manager/src/wallet_interface.rs 0.00% 3 Missing ⚠️
dash-spv/src/sync/filters/manager.rs 94.59% 2 Missing ⚠️
key-wallet-manager/src/matching.rs 97.56% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #863      +/-   ##
==========================================
- Coverage   73.62%   73.61%   -0.01%     
==========================================
  Files         324      324              
  Lines       73195    73374     +179     
==========================================
+ Hits        53888    54013     +125     
- Misses      19307    19361      +54     
Flag Coverage Δ
core 77.08% <ø> (ø)
ffi 46.07% <ø> (-0.02%) ⬇️
rpc 20.00% <ø> (ø)
spv 90.92% <94.59%> (-0.15%) ⬇️
wallet 73.11% <63.49%> (+0.05%) ⬆️
Files with missing lines Coverage Δ
key-wallet-manager/src/lib.rs 74.82% <ø> (ø)
key-wallet-manager/src/matching.rs 96.10% <97.56%> (+0.23%) ⬆️
dash-spv/src/sync/filters/manager.rs 97.61% <94.59%> (+0.03%) ⬆️
key-wallet-manager/src/wallet_interface.rs 13.04% <0.00%> (-1.96%) ⬇️
key-wallet-manager/src/process_block.rs 90.82% <0.00%> (-1.34%) ⬇️
...allet/managed_wallet_info/wallet_info_interface.rs 69.69% <0.00%> (-4.16%) ⬇️

... and 9 files with indirect coverage changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (1)

358-379: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a unit test for monitored_filter_elements.

No test in this file exercises the new extraction logic directly (that a ProviderOwnerKeys/ProviderVotingKeys account's hash160 is returned and other account types are excluded). As per coding guidelines, "Write unit tests for new functionality" for **/*.rs, and this is non-trivial logic (not a plain getter), so it falls outside the "skip tests for trivial methods" learning.

🤖 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 `@key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs` around
lines 358 - 379, Add a focused unit test for
WalletInfo::monitored_filter_elements covering ProviderOwnerKeys and
ProviderVotingKeys accounts with PubkeyHash/ScriptHash addresses, asserting
their 20-byte hashes are returned, while addresses with unsupported payloads and
non-provider account types are excluded. Reuse the file’s existing
wallet/account test helpers and assert the complete returned vector.

Source: Coding guidelines

🤖 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.

Nitpick comments:
In `@key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs`:
- Around line 358-379: Add a focused unit test for
WalletInfo::monitored_filter_elements covering ProviderOwnerKeys and
ProviderVotingKeys accounts with PubkeyHash/ScriptHash addresses, asserting
their 20-byte hashes are returned, while addresses with unsupported payloads and
non-provider account types are excluded. Reuse the file’s existing
wallet/account test helpers and assert the complete returned vector.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0ae380c4-6642-4e66-9354-3d3302216180

📥 Commits

Reviewing files that changed from the base of the PR and between 66f0873 and a2b34ca.

📒 Files selected for processing (6)
  • dash-spv/src/sync/filters/manager.rs
  • key-wallet-manager/src/lib.rs
  • key-wallet-manager/src/matching.rs
  • key-wallet-manager/src/process_block.rs
  • key-wallet-manager/src/wallet_interface.rs
  • key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs

@github-actions github-actions Bot added the ready-for-review CodeRabbit has approved this PR label Jul 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-review CodeRabbit has approved this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants