Skip to content

fix(key-wallet): platform node id is the Tenderdash node ID (SHA256[0..20]), not hash160#884

Merged
QuantumExplorer merged 4 commits into
devfrom
claude/platform-node-id-883
Jul 14, 2026
Merged

fix(key-wallet): platform node id is the Tenderdash node ID (SHA256[0..20]), not hash160#884
QuantumExplorer merged 4 commits into
devfrom
claude/platform-node-id-883

Conversation

@QuantumExplorer

@QuantumExplorer QuantumExplorer commented Jul 14, 2026

Copy link
Copy Markdown
Member

Closes #883.

Problem

key-wallet computed the masternode platform node id as HASH160(ed25519_pubkey), but the canonical value — what dashmate derives from node_key.json, what Tenderdash announces on the platform P2P network, and what ProRegTx builders (DashSync, DSLocalMasternode.m:458-459) write on-chain — is the Tenderdash/CometBFT node ID: SHA256(pubkey)[0..20]. Wallet-derived node ids therefore never matched real on-chain platform_node_id values, so ProviderPlatformKeys matching could never associate a wallet's platform-node key with its own evonode registration. Follow-up in the #878/#879/#881 series: operator BLS keys now match DashSync end-to-end; this was the remaining divergence.

Changes

  • derivation_slip10::tenderdash_node_id — new single audited primitive: SHA256(pubkey)[0..20], documented against the CometBFT convention and explicitly contrasted with hash160.
  • Pool keying — the DerivedKey::EdDSA branch of AddressPool::generate_address_at_index and EdDSAAccount::derive_address_at now wrap the node id (not hash160) in the P2PKH-style payload. That payload is exactly what both check_provider_platform_key_in_transaction_for_match sites compare against reg.platform_node_id, so matching now hits real registrations with no matcher changes.
  • SLIP-0010 identifier()/fingerprints intentionally unchanged — the issue listed identifier() as a fix site, but BIP32/SLIP-0010 key identifiers are spec-defined as hash160, used only for extended-key fingerprint metadata, and never compared against on-chain fields; changing them would silently alter xpub encodings for no matching benefit. The platform node id gets its own explicit primitive instead.

Scope notes

  • PlatformNodeId newtype (issue item 2, "consider"): not done here — retyping ProviderRegistrationPayload.platform_node_id is a breaking dash-crate API change rippling into rpc-json/spv/ffi and downstream platform consumers; better as a dedicated follow-up.
  • Persisted pools: wallets serialized with the old hash160-keyed platform pool entries keep those stale entries until regenerated; there was nothing on-chain they could ever have matched, so no working setup regresses.

Tests

  • ed25519_platform_node_id_matches_tenderdash_convention — pins platform node key 0 (m/9'/5'/3'/4'/0') for the shared test mnemonic: pubkey 3130c143…8cf789, node id 302f2615e6955cce8ed3cff81e8011bfd3a2991f, both cross-checked with an independent Ed25519 implementation (pyca/cryptography).
  • platform_pool_entries_are_keyed_by_tenderdash_node_id — end-to-end through the managed pool: the generated entry's payload equals the pinned node id. Verified both tests fail without the source change (payload was the hash160 value eaa83b51…).

Full cargo test -p key-wallet --all-features (546 passed), clippy -D warnings, and fmt are clean. The existing ProRegTx platform matching tests are self-consistent (they feed the pool's own payload) and pass under the new convention.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Updated Ed25519-derived addresses to use the Tenderdash platform node ID format.
    • Improved compatibility between derived addresses, provider platform keys, and on-chain node identifiers.
    • Added validation for Ed25519 public key size during address generation.
  • Tests

    • Added coverage for platform node ID derivation and provider key matching against reference values.

…160 (#883)

key-wallet computed the masternode platform node id as
HASH160(ed25519_pubkey), but the canonical value — what dashmate derives
from node_key.json, what Tenderdash announces on the platform P2P
network, and what ProRegTx builders (DashSync) write on-chain — is the
Tenderdash/CometBFT node ID: SHA256(pubkey)[0..20]. Wallet-derived node
ids therefore never matched real on-chain platform_node_id values, so
ProviderPlatformKeys matching could never hit an actual evonode
registration.

- Add derivation_slip10::tenderdash_node_id as the single audited
  primitive (SHA256(pubkey)[0..20]).
- Key the ProviderPlatformKeys pool entries by node id: the
  DerivedKey::EdDSA branch of AddressPool::generate_address_at_index and
  EdDSAAccount::derive_address_at now wrap the node id (not hash160) in
  the P2PKH-style payload the ProRegTx matcher compares against
  reg.platform_node_id.
- SLIP-0010 fingerprints/identifier() are intentionally unchanged: BIP32
  key identifiers are spec-defined as hash160 and never compared against
  on-chain fields.

Golden vectors (cross-checked with an independent Ed25519
implementation, pyca/cryptography): platform node key 0 for the shared
test mnemonic has pubkey 3130c143...8cf789 and node id
302f2615e6955cce8ed3cff81e8011bfd3a2991f. Tests pin the primitive and
the pool payload end-to-end; without the fix the pool payload is the
hash160 value eaa83b51... and the new tests fail.

Closes #883

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

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@QuantumExplorer, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 49 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ff65114d-63bd-4012-b34f-b73050436d2d

📥 Commits

Reviewing files that changed from the base of the PR and between 4958114 and 90f02c5.

📒 Files selected for processing (3)
  • key-wallet/src/account/eddsa_account.rs
  • key-wallet/src/derivation_slip10.rs
  • key-wallet/src/managed_account/address_pool.rs
📝 Walkthrough

Walkthrough

EdDSA platform node IDs now use the first 20 bytes of SHA-256 over the Ed25519 public key. Account and provider-pool address derivation use this value, with feature-gated EdDSA support and tests covering node ID and payload storage.

Changes

Platform node ID derivation

Layer / File(s) Summary
Node ID contract and account derivation
key-wallet/src/derivation_slip10.rs, key-wallet/src/account/eddsa_account.rs
Adds tenderdash_node_id and uses its 20-byte result for EdDSA-derived address payloads.
Provider pool address integration
key-wallet/src/managed_account/address_pool.rs, key-wallet/src/transaction_checking/account_checker.rs
Gates DerivedKey::EdDSA behind the eddsa feature and derives provider-pool payloads from validated Tenderdash node IDs; matching comments describe the same convention.
Node ID derivation tests
key-wallet/src/tests/provider_key_derivation_tests.rs
Adds EdDSA tests for the reference node ID and provider-platform pool payload storage.

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

Suggested labels: ready-for-review

Suggested reviewers: zocolini, xdustinface

🚥 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 summarizes the main change: switching platform node IDs to Tenderdash SHA256[:20] instead of hash160.
Linked Issues check ✅ Passed The PR switches platform node id derivation to SHA256(pubkey)[0..20], adds a pinned test vector, and updates matching paths as requested in #883.
Out of Scope Changes check ✅ Passed The diff stays focused on platform-node derivation, matching, feature gating, and tests, with no unrelated code paths changed.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/platform-node-id-883

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.

@QuantumExplorer QuantumExplorer left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Reviewed

derivation_slip10 is compiled only with the eddsa feature, but the
DerivedKey::EdDSA match arm referencing tenderdash_node_id was
unconditional, breaking default-feature builds. Gate the variant and its
arm like KeySource's EdDSA variants — the variant is only constructible
from the (already gated) EdDSA key sources.

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

codecov Bot commented Jul 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.61538% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.30%. Comparing base (bc6cbf9) to head (90f02c5).
⚠️ Report is 2 commits behind head on dev.

Files with missing lines Patch % Lines
key-wallet/src/managed_account/address_pool.rs 44.44% 5 Missing ⚠️
key-wallet/src/account/eddsa_account.rs 95.83% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #884      +/-   ##
==========================================
+ Coverage   74.23%   74.30%   +0.07%     
==========================================
  Files         326      326              
  Lines       74585    74636      +51     
==========================================
+ Hits        55370    55462      +92     
+ Misses      19215    19174      -41     
Flag Coverage Δ
core 77.09% <ø> (ø)
ffi 50.32% <ø> (+<0.01%) ⬆️
rpc 20.00% <ø> (ø)
spv 91.11% <ø> (+0.17%) ⬆️
wallet 74.40% <84.61%> (+0.11%) ⬆️
Files with missing lines Coverage Δ
key-wallet/src/derivation_slip10.rs 51.75% <100.00%> (+0.68%) ⬆️
...wallet/src/transaction_checking/account_checker.rs 53.98% <ø> (ø)
key-wallet/src/account/eddsa_account.rs 76.05% <95.83%> (+4.46%) ⬆️
key-wallet/src/managed_account/address_pool.rs 66.75% <44.44%> (-0.36%) ⬇️

... and 10 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.

Actionable comments posted: 2

🧹 Nitpick comments (1)
key-wallet/src/account/eddsa_account.rs (1)

351-355: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use the infallible hash constructor here.

node_id is already a [u8; 20], so PubkeyHash::from_byte_array(node_id) can replace from_slice(...).expect(...) in both:

  • key-wallet/src/account/eddsa_account.rs#L351-L355
  • key-wallet/src/managed_account/address_pool.rs#L511-L515
🤖 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/account/eddsa_account.rs` around lines 351 - 355, Replace the
fallible PubkeyHash construction in both key-wallet/src/account/eddsa_account.rs
lines 351-355 and key-wallet/src/managed_account/address_pool.rs lines 511-515
with the infallible from_byte_array constructor, passing the existing node_id
array directly; remove the redundant expect calls while preserving the
Payload::PubkeyHash usage.

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.

Inline comments:
In `@key-wallet/src/account/eddsa_account.rs`:
- Around line 349-355: Replace the fallible
PubkeyHash::from_slice(...).expect(...) conversions with infallible
PubkeyHash::from_byte_array(node_id) in key-wallet/src/account/eddsa_account.rs
lines 349-355 and key-wallet/src/managed_account/address_pool.rs lines 511-515,
preserving the existing payload construction.

In `@key-wallet/src/derivation_slip10.rs`:
- Around line 39-44: Replace the expect-based conversion in tenderdash_node_id
with a non-panicking fixed-size array copy from the first 20 hash bytes,
preserving the [u8; 20] return value and SHA-256-derived node ID.

---

Nitpick comments:
In `@key-wallet/src/account/eddsa_account.rs`:
- Around line 351-355: Replace the fallible PubkeyHash construction in both
key-wallet/src/account/eddsa_account.rs lines 351-355 and
key-wallet/src/managed_account/address_pool.rs lines 511-515 with the infallible
from_byte_array constructor, passing the existing node_id array directly; remove
the redundant expect calls while preserving the Payload::PubkeyHash usage.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 77748e79-9751-40ab-b0bf-edc25b2e6d46

📥 Commits

Reviewing files that changed from the base of the PR and between bc6cbf9 and 4958114.

📒 Files selected for processing (5)
  • key-wallet/src/account/eddsa_account.rs
  • key-wallet/src/derivation_slip10.rs
  • key-wallet/src/managed_account/address_pool.rs
  • key-wallet/src/tests/provider_key_derivation_tests.rs
  • key-wallet/src/transaction_checking/account_checker.rs

Comment thread key-wallet/src/account/eddsa_account.rs Outdated
Comment thread key-wallet/src/derivation_slip10.rs
QuantumExplorer and others added 2 commits July 14, 2026 11:22
…ctor

Codecov flagged the EdDSAAccount::derive_address_at change as
uncovered. Pin it to the same independently computed Tenderdash node id
as the pool-level tests so both implementations of the convention are
locked to the vector.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Address review: PubkeyHash::from_byte_array instead of
from_slice().expect(), and copy_from_slice instead of
try_into().expect() in tenderdash_node_id — no panics in library code.

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

Copy link
Copy Markdown
Member Author

Reviewed

@QuantumExplorer QuantumExplorer merged commit a959459 into dev Jul 14, 2026
36 of 39 checks passed
@QuantumExplorer QuantumExplorer deleted the claude/platform-node-id-883 branch July 14, 2026 04:35
QuantumExplorer added a commit that referenced this pull request Jul 14, 2026
PR #884 added derivation_slip10::tenderdash_node_id and this branch
added PlatformNodeId::from_ed25519_public_key — two copies of the same
SHA256(pubkey)[0..20] derivation. Keep the dash-crate newtype as the
single audited implementation and delegate the key-wallet helper to it;
the #884 golden-vector tests pin the output unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer added a commit that referenced this pull request Jul 14, 2026
…lds (#885)

* feat(dash): dedicated PlatformNodeId newtype for platform_node_id fields

The platform_node_id on ProRegTx was typed Option<PubkeyHash> (and raw
Option<[u8; 20]> on ProUpServTx), but the value is a Tenderdash/CometBFT
node ID — SHA256(ed25519_pubkey) truncated to 20 bytes — not a hash160
public key hash. Sharing the PubkeyHash type is what let the two
conventions get conflated in the first place (#883, fixed in #884).

Introduce dashcore::PlatformNodeId, a 20-byte newtype with hex
Display/FromStr, serde, bincode, and consensus encoding, plus
from_ed25519_public_key encoding the canonical CometBFT derivation.
Use it for platform_node_id on ProviderRegistrationPayload,
ProviderUpdateServicePayload, and EntryMasternodeType::HighPerformance.

All wire/persisted encodings stay byte-identical: consensus encoding is
the same raw 20 bytes (existing test vectors still pass, and the v2
ProRegTx round-trip test now also asserts the decoded node-ID value),
and a regression test locks the bincode layout to the old PubkeyHash
layout so persisted dash-spv masternode-list snapshots keep decoding.
key-wallet's platform-key matchers now compare raw bytes explicitly,
making the hash160-vs-node-ID distinction visible at the comparison
site; behavior is unchanged. The key-wallet-ffi C ABI is untouched.

Follow-up to #883.

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

* refactor(key-wallet): delegate tenderdash_node_id to PlatformNodeId

PR #884 added derivation_slip10::tenderdash_node_id and this branch
added PlatformNodeId::from_ed25519_public_key — two copies of the same
SHA256(pubkey)[0..20] derivation. Keep the dash-crate newtype as the
single audited implementation and delegate the key-wallet helper to it;
the #884 golden-vector tests pin the output unchanged.

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

* test(dash): cover PlatformNodeId hex/serde paths and zero-fill encoding

Close the patch-coverage gaps codecov flagged: hex Display/FromStr
round-trip (including wrong-length and non-hex failures), serde JSON
round-trip as a hex string, and the zero-filled wire encoding of v2
high-performance ProRegTx / ProUpServTx payloads whose platform fields
are None.

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

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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

1 participant