Skip to content

chore(platform)!: bump rust-dashcore to 719de34b (secp256k1 0.33) - #4932

Closed
PastaPastaPasta wants to merge 6 commits into
v5.0-devfrom
chore/bump-rust-dashcore-secp-033
Closed

PastaPastaPasta wants to merge 6 commits into
v5.0-devfrom
chore/bump-rust-dashcore-secp-033

Conversation

@PastaPastaPasta

@PastaPastaPasta PastaPastaPasta commented Sep 23, 2026 •

Copy link
Copy Markdown
Member

Issue being fixed or feature implemented

Prerequisite for #4844: the DashPay Connect key paths now live in rust-dashcore's key-wallet (dashpay/rust-dashcore#1049, merged), and consuming them needs platform on current rust-dashcore dev. That range includes rust-dashcore #1042 (secp256k1 0.30 → 0.33, rand 0.9, getrandom 0.4, secp256k1 context dropped API-wide), which is breaking for every platform crate that touches secp256k1.

What was done?

Bumps all eight rust-dashcore git dependencies from e4208c90 to 719de34b (rust-dashcore dev at the merge of dashpay/rust-dashcore#1049, which adds the DIP-13 application key paths) and migrates call sites mechanically:

  • drop Secp256k1 contexts (derive_priv, from_priv, from_secret_key, Keypair::new take none); sign / verify / recover become methods on the key or signature;
  • SecretKey::from_slice / from_byte_array → from_secret_bytes (a wrong slice length still maps to InvalidSecretKey); secret_bytes → to_secret_bytes; as_secret_bytes where AsRef<[u8]> went away; thread_rng → rng; from_entropy → from_os_rng;
  • hex rendering moves from the removed secp256k1::hashes re-export to the hex crate (same output).

Code that landed on v5.0-dev after this bump was first written (encrypted_for in rs-sdk and wasm-sdk, moderation charter requests) is migrated the same way. The branch merges v5.0-dev rather than rebasing, since #5113, #5167 and #5188 are stacked on it.

Also: rs-platform-encryption moves to secp256k1 0.33.1 so one secp256k1 is in the graph; simple-signer takes a direct rand 0.8; rs-dpp enables wasm_js for the new getrandom 0.3 / 0.4 on wasm32 (as it already does for 0.2). Toolchain unchanged (already 1.98.1).

Node.js >= 20 for the WASM packages: getrandom 0.3 / 0.4's wasm_js backend reads only globalThis.crypto.getRandomValues; getrandom 0.2's js also fell back to Node's require("crypto"). Node 18 has no WebCrypto global by default, so even deriving a public key traps there (secp256k1 rerandomizes its context afterwards). Node 18 has been end-of-life since April 2025, so engines.node of @dashevo/wasm-sdk, @dashevo/wasm-dpp2 and @dashevo/evo-sdk is raised to >=20, matching dashmate.

Behaviour changes carried by the range (not the migration): #1049 is additive (new path builders only). rust-dashcore #1035 (DIP-15 contact address pools extend as payments arrive) and #1009 (provider transactions consult every fund-bearing account) change platform-wallet behaviour. #1048 and test-only #920 / #1038 are also in range.

Consensus: no serialized bytes or validation results change as far as could be checked: verify / recover call the same libsecp256k1 functions, high-S is still rejected, seeded key generation is unchanged. The bundled libsecp256k1 moves from secp256k1-sys 0.10.1 to 0.14.1; ECDSA verify / recover has been stable across those releases, but it is the one real consensus surface here.

How Has This Been Tested?

  • cargo check --workspace --all-targets, cargo clippy --workspace --all-targets -- -D warnings, cargo fmt --all -- --check, cargo machete: clean.
  • After merging v5.0-dev (1b823095): cargo check --workspace --all-targets clean; cargo clippy -p dash-sdk -p platform-encryption --all-targets -- -D warnings clean; cargo test -p platform-encryption --lib 16 pass; cargo test -p dash-sdk --lib -- encrypted_for moderation_charters put_document 28 pass. The lock change against v5.0-dev is still only secp256k1 0.30.0 → 0.33.1 (sys 0.10.1 → 0.14.1, bitcoin-io dropped).
  • cargo test --lib: drive-abci 3365, dpp 4676, platform-wallet 1375, platform-wallet-ffi 426, rs-sdk-ffi 330, rs-unified-sdk-jni 43, platform-encryption 16, simple-signer 6, dash-sdk 212, drive-proof-verifier 336, platform-wallet-storage 428: all pass.
  • cargo check -p wasm-dpp2 -p wasm-sdk -p wasm-dpp -p wasm-drive-verify --target wasm32-unknown-unknown: clean.

Not run locally: drive-abci integration / strategy tests, iOS and Android FFI builds, JS / wasm-pack bundles.

Breaking Changes

Rust API: secp256k1 0.33 types throughout; wasm-sdk's Dip14ExtendedPrivKey::to_extended_pub_key no longer takes a secp argument. @dashevo/wasm-sdk, @dashevo/wasm-dpp2 and @dashevo/evo-sdk require Node.js >= 20. No wire-format or consensus change intended.

Checklist:

  • I have performed a self-review of my own code
  • 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

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

🤖 Generated with Claude Code

PR Hygiene · 1b82309

  • Bots — coderabbitai skipped after its own rate limit · thepastaclaw skipped after the window
  • Self-review — post /self-reviewed
  • Build green
  • Approvals
    • files with no dedicated owner (Cargo.lock, Cargo.toml, packages/rs-drive-proof-verifier/src/proof/token_direct_purchase.rs and 16 more) — QuantumExplorer or shumkov
    • js-wasm-sdk (packages/js-evo-sdk/README.md, packages/js-evo-sdk/package.json, packages/wasm-sdk/package.json and 5 more) — shumkov
    • dpp (packages/rs-dpp/Cargo.toml, packages/rs-dpp/src/address_funds/platform_address.rs, packages/rs-dpp/src/identity/identity_public_key/key_type.rs and 8 more) — QuantumExplorer or shumkov
    • rs-drive-abci (packages/rs-drive-abci/src/execution/check_tx/v0/mod.rs, packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funding_from_asset_lock/tests.rs, packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/direct_selling/mod.rs and 5 more) — QuantumExplorer or shumkov
    • rs-platform-wallet-ffi (packages/rs-platform-wallet-ffi/src/dashpay.rs, packages/rs-platform-wallet-ffi/src/derivation.rs, packages/rs-platform-wallet-ffi/src/derive_identity_key_at_slot.rs and 9 more) — HashEngineering or ZocoLini or llbartekll or romchornyi
    • rs-platform-wallet (packages/rs-platform-wallet/examples/dpns_marketplace_testnet.rs, packages/rs-platform-wallet/src/masternode/locator.rs, packages/rs-platform-wallet/src/test_support.rs and 17 more) — HashEngineering or ZocoLini or llbartekll or romchornyi
    • rust-sdk-ffi (packages/rs-sdk-ffi/src/address/transitions/transfer.rs, packages/rs-sdk-ffi/src/address/transitions/withdraw.rs, packages/rs-sdk-ffi/src/contested_resource/transitions/cast_vote.rs and 5 more) — lklimek or shumkov
    • rust-sdk (packages/rs-sdk/src/platform/dashpay/contact_request.rs, packages/rs-sdk/src/platform/dpns_usernames/mod.rs, packages/rs-sdk/src/platform/encrypted_for.rs and 6 more) — lklimek or shumkov

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

  • Compatibility
    • Updated wallet, identity, and signing workflows to remain compatible with the latest Dash cryptography libraries.
    • Improved support for building WebAssembly applications.
    • Updated key generation, derivation, and signing across wallet and SDK operations while preserving existing behavior.
  • Bug Fixes
    • Improved private-key input validation in address and identity operations, with invalid keys continuing to return clear errors.

@coderabbitai

coderabbitai Bot commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: dashpay/platform/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 750d9631-3599-42b6-a346-d978db98712c

📥 Commits

Reviewing files that changed from the base of the PR and between 49ad468 and e2fd47d.

📒 Files selected for processing (1)
  • .github/workflows/runner-image-candidate.yml

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


📝 Walkthrough

Walkthrough

The workspace updates its rust-dashcore revision and migrates secp256k1 key handling, signing, derivation, and random-number generation across DPP, wallet, SDK, signer, and WASM code. The changes also configure getrandom for wasm32 builds and add a pull request workflow for runner-image candidates and promotion.

Changes

rust-dashcore API Migration

Layer / File(s) Summary
Dependency and wasm32 configuration
Cargo.toml, packages/rs-dpp/Cargo.toml
Workspace rust-dashcore dependencies point to a newer revision. rs-dpp enables getrandom’s wasm_js feature for wasm32 and marks the renamed dependencies as ignored by cargo-machete.
DPP and Drive key handling
packages/rs-dpp/src/..., packages/rs-drive-abci/src/..., packages/rs-drive-proof-verifier/src/...
DPP address, identity-key, and state-transition code adopts context-free secp256k1 operations and updated secret-byte methods. Drive validation tests and helpers migrate to the same APIs; token ID error messages use hex::encode.
Encryption and platform wallet
packages/rs-platform-encryption/..., packages/rs-platform-wallet/...
The encryption crate updates its secp256k1 version and migrates test RNG and ECDH calls. Platform wallet key derivation, identity crypto, signing, and associated tests use the updated secp256k1 and BIP32 APIs.
Wallet and SDK FFI
packages/rs-platform-wallet-ffi/src/..., packages/rs-sdk-ffi/src/...
FFI key parsing and derivation use fixed-size secret-byte arrays and context-free key operations. Existing error mapping and secret-key wiping guards are retained where noted in the changes.
SDK, signer, strategy tests, and WASM
packages/rs-sdk/src/..., packages/simple-signer/..., packages/strategy-tests/..., packages/wasm-dpp2/..., packages/wasm-sdk/..., packages/rs-unified-sdk-jni/...
SDK entropy generation and key operations, simple-signer key generation, strategy fixtures, JNI test setup, and WASM key derivation and encoding are updated to current APIs.

Runner Image Workflow

Layer / File(s) Summary
Runner image candidate and promotion
.github/workflows/runner-image-candidate.yml
A pull request workflow runs the pinned reusable workflow in candidate mode for active pull requests and promote mode for merged pull requests. It filters events and changes, sets permissions and concurrency, and passes the pull request number, control revision, and Docker Hub credentials.

Priority: ➖ Normal

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

Change: Other

Suggested reviewers: quantumexplorer, zocolini

Merge Risk: ⚪ Minimal · up to e2fd4

No actionable merge-blocking issue remains; this change is ready for normal checks.

Architecture Summary

Architecture risk: 🔵 Low · up to e2fd4

The change affects 15 systems.

Changed systems: packages/rs-platform-wallet, packages/rs-platform-wallet-ffi, packages/rs-dpp, packages/rs-drive-abci, packages/rs-sdk-ffi, packages/rs-sdk, packages/rs-platform-encryption, packages/wasm-dpp2, packages/wasm-sdk, packages/simple-signer, packages/strategy-tests, Cargo.toml, packages/rs-drive-proof-verifier, packages/rs-scripts, packages/rs-unified-sdk-jni

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — packages/rs-platform-wallet (library) was modified; 20 changed files map to changed impact.
  • observed — packages/rs-platform-wallet-ffi (library) was modified; 12 changed files map to changed impact.
  • observed — packages/rs-dpp (library) was modified; 11 changed files map to changed impact.
  • observed — packages/rs-drive-abci (library) was modified; 8 changed files map to changed impact.

Before / after behavior

  • observed — Modified behavior in Cargo.toml: The eight rust-dashcore workspace dependencies (dashcore, dash-network-seeds, dash-spv, key-wallet, key-wallet-ffi, key-wallet-manager, dash-network, dashcore-rpc) have their git rev pinned from e4208c90786a6854bd498315bcb571ef24182c15 to 719de34bd90792efa77aa5e1065ab21ac09206ee, with all other attributes unchanged.
  • observed — Modified behavior in packages/rs-dpp/Cargo.toml: Added a [target.'cfg(all(target_arch = "wasm32", target_os = "unknown"))'.dependencies] section that enables the wasm_js feature on getrandom 0.3 and 0.4 via renamed dependency aliases getrandom_03 and getrandom_04, because secp256k1 0.33 (through rand 0.9) and key-wallet pull those getrandom versions which refuse to build on wasm32-unknown-unknown without the feature.
  • observed — Modified behavior in packages/rs-dpp/Cargo.toml: Added a [package.metadata.cargo-machete] section listing getrandom_03 and getrandom_04 as ignored dependencies.
  • observed — Modified behavior in packages/rs-dpp/src/address_funds/platform_address.rs: Removed the use dashcore::key::Secp256k1; import; verify_bytes_against_witness and From<&PrivateKey> no longer construct Secp256k1 contexts.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 124 functions across 50 files. (1 skipped… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the rust-dashcore dependency bump and the secp256k1 0.33 migration, which are the main changes in the pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 124 functions across 50 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
⚔️ Resolve merge conflicts 💡
  • Resolve merge conflict in branch chore/bump-rust-dashcore-secp-033
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

@github-actions github-actions Bot added this to the v4.2.0 milestone Sep 23, 2026
@github-actions github-actions Bot added the waiting-bots Waiting for the review bots to report on this head label Sep 23, 2026
@thepastaclaw

thepastaclaw commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

⚠️ Automated review could not complete (commit 1b82309)
Reason: phase2/security-auditor lane failed twice: [infra] lane exit 1: API Error: 400 unknown provider for model gpt-6.1-sol

@PastaPastaPasta
PastaPastaPasta force-pushed the chore/bump-rust-dashcore-secp-033 branch from a438d69 to 6fb2f25 Compare September 23, 2026 02:40
@PastaPastaPasta PastaPastaPasta changed the title chore(deps)!: bump rust-dashcore to 929a651a (secp256k1 0.33) chore(platform)!: bump rust-dashcore to 929a651a (secp256k1 0.33) Sep 23, 2026

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Final validation — Phase 2 only (queue backlog)

Verified both supplied findings against head 6fb2f25 and the cached old/new dependency sources. They describe one actionable, non-blocking regression-coverage improvement for the cryptographic upgrade and are consolidated below; no demonstrated signature-validation regression was identified. This verification used source inspection and did not independently rerun the reported test suites.

🟡 1 suggestion(s)

Review provenance

Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)

  • Triage: normal by gpt-6-astra (effort low) — Although broad, the diff is a dependency upgrade with mechanical cryptographic API adaptations, preserving signing, recovery, key parsing and derivation semantics rather than introducing substantive changes to consensus rules or funds movement.
  • Phase 1 reviewers: not run (skipped for throughput: 30 PRs queued, above the 10 limit)
  • Fresh verifier: gpt-6-astra — final-verifier; agent astra-verifier
  • Phase 2 reviewers: gpt-6-astra — general (completed, effort high); agent phase2-reviewer, gpt-6-astra — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6-astra — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6-astra — platform-versioning (completed, effort high); agent phase2-reviewer, gpt-6-astra — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6-astra — 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 each finding against the current code and only fix it if needed.

In `Cargo.toml`:
- [SUGGESTION] Cargo.toml:67: Pin historical signature-validation results across the cryptography upgrade
  This dependency bump replaces the cryptographic implementation used by historical protocol versions. The existing byte-parity test compares two signing paths within the upgraded stack, and the P2SH witness tests generate signatures through that same stack, so these tests do not pin acceptance to the previous dependency. Add fixed regression vectors with expected results established against e4208c90 for `verify_data_signature`, `verify_hash_signature`, and `verify_bytes_against_witness`: valid low-S signatures, high-S counterparts, zero/out-of-range scalars, malformed compact headers, and invalid public keys where applicable. Keep recovery-based P2PKH expectations separate from ordinary ECDSA verification, since their high-S behavior differs, and assert verification-operation counts for successful multisig witnesses. This would retain compatibility evidence in the repository beyond the reported differential checks; it is a coverage recommendation, not evidence of a current consensus regression.

Comment thread Cargo.toml Outdated
Bump every rust-dashcore git dependency from e4208c90 to 719de34b (dashpay/rust-dashcore dev, the merge of #1049). The breaking change in range is rust-dashcore #1042: secp256k1 0.30 -> 0.33, rand 0.9, getrandom 0.4, and the secp256k1 context argument dropped API-wide. The range also carries key-wallet fixes #1035 (DIP-15 contact pools extend as payments arrive), #1009 (provider transactions consult every fund-bearing account), #1048, and test-only #920/#1038.

Mechanical call-site migration: drop Secp256k1 contexts (derive_priv/from_priv/public_key/from_secret_key/Keypair::new take no context); secp.sign_ecdsa/recover_ecdsa/verify_ecdsa become methods on the key/signature; SecretKey::from_slice/from_byte_array become from_secret_bytes (slices go through <[u8; 32]>::try_from mapped to Error::InvalidSecretKey, the error from_slice returned); secret_bytes -> to_secret_bytes; SecretKey/SharedSecret lost AsRef<[u8]>, so use as_secret_bytes; thread_rng -> rng; StdRng::from_entropy -> from_os_rng. The secp256k1::hashes re-export is gone, so hex rendering uses the hex crate (same output).

Seeded RNG bridging (rand 0.8 caller -> secp's rand 0.9 StdRng) now fills a 32-byte seed and calls from_seed, which is what rand_core 0.6 from_rng did, so seeded keys are unchanged; drive-abci tests that feed seed_from_u64 into Keypair::new use the secp re-exported StdRng (seed_from_u64 and ChaCha12 are identical across rand_core 0.6/0.9).

rs-platform-encryption moves its own secp256k1 to 0.33.1 so only one secp256k1 is in the graph. simple-signer takes a direct rand 0.8 dep for the RngCore its callers pass. rs-dpp enables getrandom 0.3/0.4 wasm_js on wasm32-unknown-unknown, which the new transitive getrandom versions require there. Rust toolchain already at 1.98.1, matching rust-dashcore.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@PastaPastaPasta
PastaPastaPasta force-pushed the chore/bump-rust-dashcore-secp-033 branch from 6fb2f25 to 49ad468 Compare September 23, 2026 16:37
@PastaPastaPasta PastaPastaPasta changed the title chore(platform)!: bump rust-dashcore to 929a651a (secp256k1 0.33) chore(platform)!: bump rust-dashcore to 719de34b (secp256k1 0.33) Sep 23, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Your move: thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

@github-actions github-actions Bot added waiting-self-review Waiting for the author to post /self-reviewed and removed waiting-bots Waiting for the review bots to report on this head labels Sep 23, 2026
@github-actions

github-actions Bot commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-review — Final validation — Phase 2 only (queue backlog)

Reviewed the complete 88-file diff at 49ad468 and confirmed one blocking WASM runtime regression affecting supported Node 18 consumers. An independently rebuilt old-versus-new WASM probe reproduced the failure without global WebCrypto and succeeded with WebCrypto initialized before use; 39 DPP address/witness tests, 16 encryption tests, and diff whitespace checks passed. The prior historical-signature coverage recommendation remains intentionally deferred, not fixed.

🔴 1 blocking

Review provenance

Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)

  • Triage: normal by gpt-6-astra (effort low) — The broad diff mechanically adapts dependency APIs, including key generation in packages/rs-dpp/src/identity/identity_public_key/key_type.rs and ECDH accessors in packages/rs-platform-encryption/src/ecdh.rs, without clearly changing cryptographic algorithms, consensus rules, or funds-handling logic beyond the dependency bump.
  • Phase 1 reviewers: not run (skipped for throughput: 11 PRs queued, above the 10 limit)
  • Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
  • Fresh verifier: gpt-6-astra — final-verifier; agent astra-verifier
  • Phase 2 reviewers: gpt-6-astra — general (completed, effort high); agent phase2-reviewer, gpt-6-astra — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6-astra — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6-astra — platform-versioning (completed, effort high); agent phase2-reviewer, gpt-6-astra — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6-astra — security-auditor (completed, effort high); agent phase2-reviewer, gpt-6-astra — general (completed, effort high); agent phase2-reviewer, gpt-6-astra — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6-astra — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6-astra — platform-versioning (completed, effort high); agent phase2-reviewer, gpt-6-astra — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6-astra — 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 each finding against the current code and only fix it if needed.

In `packages/rs-dpp/Cargo.toml`:
- [BLOCKING] packages/rs-dpp/Cargo.toml:97-98: Initialize WebCrypto for supported Node 18 WASM consumers
  Both `wasm-sdk` and `wasm-dpp2` declare Node >=18.18 support, but the newly enabled entropy backend requires `globalThis.crypto.getRandomValues`, which is absent by default in Node 18 script execution. This now breaks deterministic operations too: `WasmSdk.keyPairFromHex` calls `private_key.public_key()`, and secp256k1 0.33 rerandomizes its thread-local context using rand 0.9 after deriving the public key. The previous explicit context skipped randomization on WASM. Neither package's bundle initializer installs Node's WebCrypto implementation. I independently rebuilt a minimal probe with the pinned old/new dependencies: without global WebCrypto, the old derivation returns the expected public key while the new derivation traps with `RuntimeError: unreachable`; a fresh instance with WebCrypto installed succeeds. Initialize `node:crypto.webcrypto` in the Node loading path before Rust operations and add a runtime regression test, or explicitly raise and document the supported Node minimum.

Comment thread packages/rs-dpp/Cargo.toml
infraclaw-dash and others added 2 commits September 29, 2026 05:17
infraclaw: install the reviewed publisher prerequisite only; no application or runner changes.
@github-actions github-actions Bot added waiting-bots Waiting for the review bots to report on this head and removed waiting-self-review Waiting for the author to post /self-reviewed labels Sep 29, 2026
@github-actions

github-actions Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

@github-actions github-actions Bot added waiting-self-review Waiting for the author to post /self-reviewed and removed waiting-bots Waiting for the review bots to report on this head labels Sep 29, 2026

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-review — Preliminary review — Phase 1 blocker gate

The Node 18 compatibility finding remains valid at the exact head: the newly enabled entropy backends require global WebCrypto, but the published WASM packages still advertise Node >=18.18 without initializing it. Historical signature-validation vectors remain useful additional coverage, but no validation regression was demonstrated, and their intentional deferral does not block this mechanical migration.

Validated blockers were found by the Phase-1 review and confirmed by a fresh verifier. Phase 2 is deferred until a fresh same-head revalidation clears the blocker gate.

🔴 1 blocking

1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

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: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: ffi-engineer); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-gate-verifier, role: verifier)

  • Triage: normal by gpt-6-astra (effort low) — Although broad, the diff is a dependency upgrade with mechanical API adaptations, including signature recovery in packages/rs-dpp/src/state_transition/mod.rs, rather than substantive changes to consensus rules, cryptographic algorithms, or key-handling policy.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — architecture-layering (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — ffi-engineer (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — security-auditor (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 below 15% reserve: weekly 13% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
  • Fresh verifier: gpt-6-astra — verifier; agent astra-gate-verifier
  • Phase 2 reviewers: not run (deferred by blocker gate)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-dpp/Cargo.toml`:
- [BLOCKING] packages/rs-dpp/Cargo.toml:97-98: Initialize WebCrypto for supported Node 18 WASM consumers
  (existing thread: https://github.com/dashpay/platform/pull/4932#discussion_r4086369482)
  Both WASM packages still declare Node >=18.18, but the newly enabled getrandom wasm_js backend calls globalThis.crypto.getRandomValues directly, with no Node crypto fallback. Node 18 does not expose that global by default when running ordinary module files, and the bundle wrappers do not initialize it. This also affects operations with supplied private keys, not just explicit random-key generation: secp256k1 0.33.1's global-context rerandomization calls rand::random() when its rand feature is enabled, introducing this entropy dependency into secret-key operations. Consequently, signing and key derivation can fail on a declared-supported runtime; the failure is not necessarily converted into WasmSdkError because thread-RNG initialization can panic. Initialize globalThis.crypto from node:crypto before the affected WASM operations, covering the supported entrypoints, or raise the declared Node minimum to a supported LTS version with global WebCrypto enabled by default.

PastaPastaPasta and others added 3 commits October 4, 2026 22:23
Conflicts resolved:
- Cargo.toml: v5.0-dev's grovedb dce8252f, this branch's rust-dashcore 719de34b.
- Cargo.lock: regenerated from v5.0-dev's lock; the only resolved change
  is secp256k1 0.30.0 -> 0.33.1 (sys 0.10.1 -> 0.14.1, bitcoin-io dropped),
  the same delta the bump produced before.
- rs-platform-encryption tests: v5.0-dev's seeded StdRng, with secp 0.33's
  context-free generate_keypair.
- rs-sdk put_document: v5.0-dev's restructured transition, with rand 0.9's
  from_os_rng / random.
- runner-image-candidate.yml: v5.0-dev's version (controller 1be03edb); the
  feature-branch trigger is no longer needed now that this branch carries
  v5.0-dev's runner-image CI.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
secp256k1 0.33 draws randomness through rand 0.9 / getrandom 0.3, and
key-wallet through getrandom 0.4. Their `wasm_js` backend reads only
`globalThis.crypto.getRandomValues`; getrandom 0.2's `js` feature also fell
back to Node's `require("crypto")`. Node.js exposes the WebCrypto global by
default from v19, so on Node 18 even a deterministic call such as deriving a
public key (secp256k1 rerandomizes its context afterwards) traps.

Node 18 has been end-of-life since April 2025. Raise `engines.node` of
wasm-sdk, wasm-dpp2 and js-evo-sdk to >=20, matching dashmate, and say why
in the evo-sdk README and next to the getrandom features in rs-dpp.

BREAKING CHANGE: @dashevo/wasm-sdk, @dashevo/wasm-dpp2 and @dashevo/evo-sdk
require Node.js >= 20.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…o secp256k1 0.33

Code that landed on v5.0-dev after this bump was first written (encrypted_for
in rs-sdk and wasm-sdk, moderation charter requests) still used the
secp256k1 0.30 / rand 0.8 API. Migrate it the same way as the rest of the
bump: drop the `Secp256k1` context from `PublicKey::from_secret_key`,
`SecretKey::from_slice` -> `from_secret_bytes`, `StdRng::from_entropy` ->
`from_os_rng`, and `Bytes32::random_with_rng` -> `Bytes32::new(rng.random())`
since platform-value's helper takes a rand 0.8 `StdRng`.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
PastaPastaPasta added a commit that referenced this pull request Oct 5, 2026
…nd renamed token payment accessor

Restacked onto v5.0-dev (via #4932), the summary no longer compiled:
token shielded pools (#4760) added seven `TokenTransition` variants, and the
document base's token payment is now read through `token_payment_info_ref`.
The shielded-pool rows (shield, unshield, shielded transfer, mint / burn /
claim / direct purchase to pool) have no typed projection, so the whole
transition is rendered in `details` and the row is incomplete, like the
other untyped material fields.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Waiting for bot review — coderabbitai skipped after its own rate limit · 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.

@github-actions github-actions Bot added waiting-bots Waiting for the review bots to report on this head bot-review-skipped A required review bot did not report; it was skipped by the window or by a person. and removed waiting-self-review Waiting for the author to post /self-reviewed labels Oct 5, 2026
@github-actions

github-actions Bot commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

@thepastaclaw review

No review for 1b823095 yet, so PR Hygiene is asking once. If nothing arrives, the requirement is dropped for this commit and the pull request is labelled bot-review-skipped.

@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

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

@github-actions github-actions Bot added waiting-self-review Waiting for the author to post /self-reviewed and removed waiting-bots Waiting for the review bots to report on this head labels Oct 6, 2026
lklimek added a commit that referenced this pull request Oct 6, 2026
Merge current v5.1-dev and PR #4932, then pin all eight rust-dashcore dependencies to PR #1112 (870af146). Preserve DPP crypto implementations, canonical node IDs, and legacy Core RPC behavior. Use the upstream blst no_std API fix at 71a00877 for WASM compatibility.

Validated wallet/storage, signing/BLS, encryption and RPC regressions, scoped strict Clippy, and WASM builds. The published tree matches the locally tested tree b69a241.

<sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
@lklimek

lklimek commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

We decided to do it on v5.1-dev, see #5307

@lklimek lklimek closed this Oct 6, 2026
PastaPastaPasta added a commit that referenced this pull request Oct 6, 2026
…nd renamed token payment accessor

Restacked onto v5.0-dev (via #4932), the summary no longer compiled:
token shielded pools (#4760) added seven `TokenTransition` variants, and the
document base's token payment is now read through `token_payment_info_ref`.
The shielded-pool rows (shield, unshield, shielded transfer, mint / burn /
claim / direct purchase to pool) have no typed projection, so the whole
transition is rendered in `details` and the row is incomplete, like the
other untyped material fields.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot-review-skipped A required review bot did not report; it was skipped by the window or by a person. waiting-self-review Waiting for the author to post /self-reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants