Skip to content

fix(dash): normalize PlatformNodeId to canonical byte order at consensus boundary#889

Merged
QuantumExplorer merged 1 commit into
devfrom
claude/issue-887-fix-57947a
Jul 14, 2026
Merged

fix(dash): normalize PlatformNodeId to canonical byte order at consensus boundary#889
QuantumExplorer merged 1 commit into
devfrom
claude/issue-887-fix-57947a

Conversation

@QuantumExplorer

@QuantumExplorer QuantumExplorer commented Jul 14, 2026

Copy link
Copy Markdown
Member

Fixes #887

Problem

PlatformNodeId (#885) held bytes in two different orders depending on construction path, so values from the two paths never compared equal:

Consequences: key-wallet's provider matching (account_checker.rs) could never recognize a wallet-owned platform node id from a decoded ProRegTx, and Display of a decoded id showed the reversed form matching neither dashmate, Tenderdash, nor explorers.

Fix

Normalize at the consensus boundary so the newtype always holds canonical forward bytes:

  • consensus_decode reverses the wire bytes into canonical order.
  • consensus_encode reverses back to wire order, so re-encoding is byte-identical to the original wire bytes (payload hashes, SML entry hashes, and merkle roots are unaffected).
  • from_ed25519_public_key, Display, FromHex, serde, and the bincode layout are unchanged.

Tests

  • consensus_round_trip now uses asymmetric bytes and asserts wire order is the byte-reversal of canonical order.
  • New consensus_decode_yields_canonical_order uses the mainnet evonode vector from the issue: wire 8cb97997…7968 decodes to canonical 68794393…b98c and re-encodes byte-identically.
  • New decoded_wire_id_matches_derived_id asserts a wire-decoded id equals the id derived from the matching Ed25519 public key — the exact mismatch from the issue.
  • Updated the real ProRegTx v2 fixture test (test_payload_version_2_encoding_and_decoding): the decoded id is now the canonical (reversed) form; the existing re-encode assertion confirms wire bytes are unchanged.

Ran: cargo test -p dashcore --all-features, cargo test -p key-wallet --all-features, cargo test -p dash-spv and cargo test -p dash-spv-ffi --test dashd_sync with dashd regtest integration tests — all green. cargo fmt --check and cargo clippy -p dashcore --all-features --all-targets clean.

Note for downstream

  • dashpay/platform picks this up via pin bump (per the issue, no downstream workaround was added, so no double-reversal).
  • Masternode-list snapshots persisted before this fix stored the wire-reversed bytes in the bincode blob; lists re-synced after this fix hold canonical bytes. Anyone comparing persisted pre-fix ids against freshly derived/decoded ones should re-sync the list.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Corrected platform node ID encoding and decoding to consistently handle canonical and wire byte order.
    • Improved compatibility when reading and writing versioned provider registration data.
    • Ensured decoded node IDs match their expected canonical representation and preserve accurate round-trip serialization.
  • Tests

    • Expanded coverage for node ID conversion, provider registration payloads, and Ed25519-derived identifiers.

…sus boundary

PlatformNodeId held bytes in two different orders depending on how it
was constructed: consensus_decode kept the wire bytes as-is (Dash Core
serializes the field as a uint160, whose internal blob order is
reversed), while from_ed25519_public_key produced the canonical forward
Tenderdash node id (SHA256(pubkey)[0..20]). Ids from the two paths
never compared equal, so wallet-owned platform node ids could never be
recognized from a decoded ProRegTx, and Display of a decoded id showed
the reversed form that matches neither dashmate nor explorers.

Normalize at the consensus boundary so the newtype always holds
canonical forward bytes: consensus_decode reverses the wire bytes into
canonical order and consensus_encode reverses back to wire order.
from_ed25519_public_key, Display, FromHex, and the bincode layout are
unchanged.

Fixes #887

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9a3f990d-30c1-4a1b-b146-d5d487a517ef

📥 Commits

Reviewing files that changed from the base of the PR and between ac8828c and 47bb91e.

📒 Files selected for processing (2)
  • dash/src/blockdata/transaction/special_transaction/provider_registration.rs
  • dash/src/platform_node_id.rs

📝 Walkthrough

Walkthrough

PlatformNodeId now stores canonical forward-order bytes and reverses them only for Dash consensus wire encoding. Documentation and tests were updated, including provider-registration payload expectations and equality with Ed25519-derived identifiers.

Changes

PlatformNodeId normalization

Layer / File(s) Summary
Canonical storage and consensus encoding
dash/src/platform_node_id.rs
Documentation distinguishes canonical bytes from Dash Core wire order, while consensus encoding and decoding reverse the 20-byte identifier at the boundary.
Byte-order regression coverage
dash/src/platform_node_id.rs, dash/src/blockdata/transaction/special_transaction/provider_registration.rs
Tests verify canonical decoding, byte-identical round trips, Ed25519-derived identifier equality, and the updated version-2 provider payload value.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: normalizing PlatformNodeId byte order at the consensus boundary.
Linked Issues check ✅ Passed The changes match #887 by normalizing decode/encode byte order and updating tests to preserve wire round-trips and match derived IDs.
Out of Scope Changes check ✅ Passed The PR is tightly scoped to PlatformNodeId byte-order normalization and related tests, with no clear unrelated changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/issue-887-fix-57947a

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 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.37%. Comparing base (2045cd2) to head (47bb91e).
⚠️ Report is 1 commits behind head on dev.

Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #889   +/-   ##
=======================================
  Coverage   74.37%   74.37%           
=======================================
  Files         327      327           
  Lines       74747    74771   +24     
=======================================
+ Hits        55590    55610   +20     
- Misses      19157    19161    +4     
Flag Coverage Δ
core 77.29% <100.00%> (+0.03%) ⬆️
ffi 50.30% <ø> (-0.01%) ⬇️
rpc 20.00% <ø> (ø)
spv 91.08% <ø> (-0.04%) ⬇️
wallet 74.40% <ø> (ø)
Files with missing lines Coverage Δ
...ction/special_transaction/provider_registration.rs 65.65% <100.00%> (ø)
dash/src/platform_node_id.rs 98.82% <100.00%> (+5.38%) ⬆️

... and 5 files with indirect coverage changes

@github-actions github-actions Bot added the ready-for-review CodeRabbit has approved this PR label Jul 14, 2026
@QuantumExplorer QuantumExplorer merged commit 7221d86 into dev Jul 14, 2026
35 checks passed
@QuantumExplorer QuantumExplorer deleted the claude/issue-887-fix-57947a branch July 14, 2026 10:44
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.

PlatformNodeId byte order differs between consensus_decode (wire-reversed) and from_ed25519_public_key (canonical) — payload/derived ids never match

1 participant