test(masternode-upgrade): standalone masternode-identity upgrade harness + spec#890
test(masternode-upgrade): standalone masternode-identity upgrade harness + spec#890shumkov wants to merge 4 commits into
Conversation
Reproducible harness for the DET 0.9.3 baseline of the masternode-identity upgrade test: stand up a dashmate v3 local network (protocol 11), extract each masternode's owner/voting/payout identity keys, and fund a DET wallet so the identity can be topped up and withdrawn from. Baseline DET: v0.9.3 (commit 3268b73), dash-sdk platform rev 29f7492e (platform 2.1.0, grovedb 3.1 -> decodes GroveDBProof::V0 only). Includes verify_signed_message.py (dependency-free secp256k1 recovery) used in the upgrade phase to prove migrated keys still sign correctly. Secrets (keys.json, wallet DBs, profile snapshots) are gitignored and regenerated locally, never committed.
) Second half of the harness: upgrade the SAME network in place from dashmate v3 to v4 (protocol 11 -> 12, GroveDBProof V0 -> V1), then verify the migrated evonode identity under the new DET. Upgrade DET: PR #887 fix/qa-followups-885 (commit 8a47745, base feat/legacy-identity-migration), dash-sdk platform rev 93b967f9 (platform 4.0). - 04_upgrade_to_v4.sh: dashmate@4 in-place upgrade, waits for protocol 12. - p2p_proxy.py: bridges 127.0.0.1:19899 -> 20001; required because the new DET's SPV client hardcodes regtest Core P2P port 19899 while dashmate uses 20001. - query_identity_dapi.sh: on-chain identity liveness/balance via DAPI, bypassing the client under test. Verified: DB migration v11 -> v38 clean, identity + owner/voting/payout keys survive, on-chain identity live on protocol 12, and both owner and payout keys still sign correctly (Sign Message -> verify_signed_message.py, byte-identical to dashd signmessage). Withdrawal-through-new-DET is blocked locally by the SPV verifier's DIP24 rotation-quorum dependency (llmq_test_dip0024, minSize 4) that a 3-masternode local net can't form; see SPEC.md 'Known limitation'.
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds a documented, reproducible harness for testing masternode identity migration across a Dashmate v3-to-v4 upgrade, including local network setup, key extraction, funding, DAPI liveness checks, P2P proxying, and deterministic signed-message verification. ChangesMasternode identity upgrade validation
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
⛔ Blockers found — Sonnet deferred (commit 7279666) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
docs/testing/masternode-identity-upgrade/harness/README.md (1)
38-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify a language for the fenced code block.
Adding a language identifier (e.g.,
bashorshell) improves syntax highlighting and resolves markdown linting warnings.♻️ Proposed refactor
-``` +```bash python3 verify_signed_message.py <key_address> <base64_signature> "<message>"</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/testing/masternode-identity-upgrade/harness/README.mdaround lines 38 -
40, Specify a shell language identifier on the fenced code block containing the
verify_signed_message.py command, using bash or shell while preserving the
command unchanged.</details> <!-- cr-comment:v1:0bbad907dad979aecda65ffe --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>docs/testing/masternode-identity-upgrade/harness/02_extract_masternode_keys.py (1)</summary><blockquote> `24-27`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_ **Use context managers for file operations.** In several places, files are opened directly as arguments without a `with` block, leaving them to be closed by the garbage collector. It is a best practice to use context managers to ensure file handles are deterministically closed immediately after use. - `docs/testing/masternode-identity-upgrade/harness/02_extract_masternode_keys.py#L24-L27`: use `with open(DASHMATE_CONFIG) as f:` to read the config. - `docs/testing/masternode-identity-upgrade/harness/02_extract_masternode_keys.py#L60-L61`: use `with open(OUT, "w") as f:` to write the output JSON. - `docs/testing/masternode-identity-upgrade/harness/03_fund_address.py#L39-L39`: use `with open(DASHMATE_CONFIG) as f:` to read the config and extract the password. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/testing/masternode-identity-upgrade/harness/02_extract_masternode_keys.py
around lines 24 - 27, Use context managers for every affected file operation: in
docs/testing/masternode-identity-upgrade/harness/02_extract_masternode_keys.py
lines 24-27, update rpc_password() to read DASHMATE_CONFIG through with open; at
lines 60-61, write OUT through with open; and in
docs/testing/masternode-identity-upgrade/harness/03_fund_address.py line 39,
read DASHMATE_CONFIG through with open while extracting the password.</details> <!-- cr-comment:v1:aa2ab5ce5106929dec496312 --> </blockquote></details> <details> <summary>docs/testing/masternode-identity-upgrade/harness/verify_signed_message.py (2)</summary><blockquote> `70-71`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _⚡ Quick win_ **Provide a helpful error for OpenSSL 3.0+ environments.** On modern systems with OpenSSL 3.0+ (like Ubuntu 22.04+ or recent macOS), `hashlib.new("ripemd160")` may throw a `ValueError` because RIPEMD160 has been moved to the legacy provider. Catching this and explaining it will save developers a lot of debugging time. <details> <summary>🛠 Proposed refactor</summary> ```diff def hash160(b: bytes) -> bytes: - return hashlib.new("ripemd160", hashlib.sha256(b).digest()).digest() + try: + return hashlib.new("ripemd160", hashlib.sha256(b).digest()).digest() + except ValueError as e: + raise RuntimeError("ripemd160 hash not available. On OpenSSL 3.0+, you may need to enable the legacy provider in your OpenSSL configuration.") from e🤖 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 `@docs/testing/masternode-identity-upgrade/harness/verify_signed_message.py` around lines 70 - 71, Update hash160 to catch the ValueError raised when RIPEMD160 is unavailable, and re-raise it with a clear message explaining that OpenSSL 3.0+ may require enabling the legacy provider. Preserve the existing hash calculation when RIPEMD160 is available.
102-104: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSupport uncompressed public keys based on the signature header.
While DET produces compressed signatures, conditionally encoding the public key based on the header value will make this script robust against uncompressed signatures as well.
✨ Proposed refactor
- # DET signs compressed keys ("+4" in the header), so encode compressed - prefix = b"\x02" if q[1] % 2 == 0 else b"\x03" - return b58check(bytes([version]) + hash160(prefix + q[0].to_bytes(32, "big"))) + # Determine compressed vs uncompressed from the header + if header >= 31: + prefix = b"\x02" if q[1] % 2 == 0 else b"\x03" + pubkey = prefix + q[0].to_bytes(32, "big") + else: + pubkey = b"\x04" + q[0].to_bytes(32, "big") + q[1].to_bytes(32, "big") + return b58check(bytes([version]) + hash160(pubkey))🤖 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 `@docs/testing/masternode-identity-upgrade/harness/verify_signed_message.py` around lines 102 - 104, Update the public-key encoding in the signature verification helper around the hash160 and b58check return to inspect the signature header: use the compressed prefix and x-coordinate encoding for compressed signatures, and include the full uncompressed public-key encoding for uncompressed signatures. Preserve the existing version and Base58Check construction.
🤖 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 `@docs/testing/masternode-identity-upgrade/harness/verify_signed_message.py`:
- Around line 65-67: Update dash_message_hash to encode the message before
constructing the payload, use the encoded byte length in the varint prefix, and
reuse those encoded bytes when concatenating the signed-message data.
---
Nitpick comments:
In
`@docs/testing/masternode-identity-upgrade/harness/02_extract_masternode_keys.py`:
- Around line 24-27: Use context managers for every affected file operation: in
docs/testing/masternode-identity-upgrade/harness/02_extract_masternode_keys.py
lines 24-27, update rpc_password() to read DASHMATE_CONFIG through with open; at
lines 60-61, write OUT through with open; and in
docs/testing/masternode-identity-upgrade/harness/03_fund_address.py line 39,
read DASHMATE_CONFIG through with open while extracting the password.
In `@docs/testing/masternode-identity-upgrade/harness/README.md`:
- Around line 38-40: Specify a shell language identifier on the fenced code
block containing the verify_signed_message.py command, using bash or shell while
preserving the command unchanged.
In `@docs/testing/masternode-identity-upgrade/harness/verify_signed_message.py`:
- Around line 70-71: Update hash160 to catch the ValueError raised when
RIPEMD160 is unavailable, and re-raise it with a clear message explaining that
OpenSSL 3.0+ may require enabling the legacy provider. Preserve the existing
hash calculation when RIPEMD160 is available.
- Around line 102-104: Update the public-key encoding in the signature
verification helper around the hash160 and b58check return to inspect the
signature header: use the compressed prefix and x-coordinate encoding for
compressed signatures, and include the full uncompressed public-key encoding for
uncompressed signatures. Preserve the existing version and Base58Check
construction.
🪄 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: 99f9f5e0-9dba-4d57-9dad-9b3317f80657
📒 Files selected for processing (10)
docs/testing/masternode-identity-upgrade/SPEC.mddocs/testing/masternode-identity-upgrade/harness/.gitignoredocs/testing/masternode-identity-upgrade/harness/01_setup_v3_network.shdocs/testing/masternode-identity-upgrade/harness/02_extract_masternode_keys.pydocs/testing/masternode-identity-upgrade/harness/03_fund_address.pydocs/testing/masternode-identity-upgrade/harness/04_upgrade_to_v4.shdocs/testing/masternode-identity-upgrade/harness/README.mddocs/testing/masternode-identity-upgrade/harness/p2p_proxy.pydocs/testing/masternode-identity-upgrade/harness/query_identity_dapi.shdocs/testing/masternode-identity-upgrade/harness/verify_signed_message.py
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At the pinned head, three blocking issues undermine the harness's stated secret-exclusion and verification guarantees: documented profile artifacts can remain trackable, the message verifier hashes non-ASCII messages incorrectly, and the documented Core cross-check invokes the daemon instead of an RPC client. Two additional reproducibility suggestions survive; the macOS xargs claim and the proposed Platform checkout pin were refuted.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking | 🟡 2 suggestion(s)
🤖 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 `docs/testing/masternode-identity-upgrade/harness/.gitignore`:
- [BLOCKING] docs/testing/masternode-identity-upgrade/harness/.gitignore:4-8: Ignore documented profile snapshots and SQLite sidecars
The PR states that wallet databases and profile snapshots are gitignored, but the README permits an arbitrary HOME profile path and the spec names snapshots backup-1 and backup-2. Under the harness, those names do not match backups/ or home-*/, and *.db does not match SQLite sidecars such as data.db-journal, data.db-wal, or data.db-shm. git check-ignore confirms these paths remain trackable, and a focused SQLite reproduction showed a rollback journal retaining prior secret-like page contents; the pinned DET 0.9.3 database also serializes imported identity private keys. Add rules for both sidecars and the documented snapshot names so a user cannot accidentally stage this local key material.
In `docs/testing/masternode-identity-upgrade/harness/verify_signed_message.py`:
- [BLOCKING] docs/testing/masternode-identity-upgrade/harness/verify_signed_message.py:61-67: Serialize the UTF-8 byte length of signed messages
Python len(message) counts Unicode code points, while the pinned Rust signed_msg_hash uses str::len(), which is the UTF-8 byte length, before hashing message.as_bytes(). The current verifier therefore computes a different digest for a valid message such as Dash 🚀 and reports MISMATCH for its genuine DET/Core signature. varint() also raises OverflowError at 65,536 bytes even though the DET multiline field has no length cap. Encode once, serialize that byte length with the full CompactSize range, and hash the same bytes.
In `docs/testing/masternode-identity-upgrade/harness/README.md`:
- [BLOCKING] docs/testing/masternode-identity-upgrade/harness/README.md:43-45: Replace the unusable dashd cross-check command
dashd is the node daemon, not the RPC command-line client, so dashd signmessage is not a valid way to invoke the wallet RPC. The prerequisites provide Core through the Dashmate network rather than a local dashd executable, and the extracted keys live in the local_seed main wallet. Document a working Dashmate Core CLI or authenticated JSON-RPC invocation that selects wallet main; otherwise readers cannot reproduce the byte-identical signmessage cross-check claimed here and in the spec.
- [SUGGESTION] docs/testing/masternode-identity-upgrade/harness/README.md:24-27: Include the funding helper in the ordered workflow
The ordered procedure moves from 02_extract_masternode_keys.py to 04_upgrade_to_v4.sh and tells the reader only to fund from Core. The committed 03_fund_address.py is explicitly the phase-1 helper and includes deterministic sending and confirmation waiting, but the README never names or invokes it. Add 03_fund_address.py <receive-address> [amount] to step 3 so the documented workflow actually uses the supplied funding phase.
In `docs/testing/masternode-identity-upgrade/harness/04_upgrade_to_v4.sh`:
- [SUGGESTION] docs/testing/masternode-identity-upgrade/harness/04_upgrade_to_v4.sh:25-30: Fail when protocol activation never completes
The activation loop has no deadline or attempt limit. If validators stall or never activate protocol 12, the harness waits forever instead of producing a failed test result; the diagnostic curl can also block indefinitely because it has no maximum time. Add a finite, generously sized deadline with a nonzero exit and apply the same readiness bound to the setup loop in 01_setup_v3_network.sh.
…scenario Adds a second, independent scenario to the spec + harness README: a password-protected wallet created in DET 0.9.3 (AES-256-GCM seed encryption, Argon2 password KDF) survives the 0.9.3 -> PR-887 DB migration and unlocks with the original password in the new DET. Includes step-by-step reproduction and the storage-layer check (data.db wallet.uses_password = 1).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/testing/masternode-identity-upgrade/harness/README.md`:
- Around line 58-60: Update the fenced SQL block in the README by changing its
opening fence from an unlabeled fence to a sql-labeled fence, preserving the
existing command and closing fence.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 11da9a1f-15a0-4cac-b3f7-7f770fe45e99
📒 Files selected for processing (2)
docs/testing/masternode-identity-upgrade/SPEC.mddocs/testing/masternode-identity-upgrade/harness/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/testing/masternode-identity-upgrade/SPEC.md
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Full-range revalidation confirms that all five carried-forward prior findings remain valid: three are blocking and two are suggestions. The latest delta introduces one additional valid documentation suggestion because its encrypted-wallet database check uses only the macOS profile path. The surviving blockers require a preliminary REQUEST_CHANGES review.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking | 🟡 1 suggestion(s)
5 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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 `docs/testing/masternode-identity-upgrade/harness/README.md`:
- [SUGGESTION] docs/testing/masternode-identity-upgrade/harness/README.md:57-62: Use the platform-specific profile path for the encryption check
The new sqlite3 command always queries <profile>/Library/Application Support/Dash-Evo-Tool/data.db, which is only the macOS location. With the documented HOME-isolated workflow on native Linux, ProjectDirs resolves the database under <profile>/.config/dash-evo-tool/data.db. The command therefore fails or checks the wrong file on Linux; document platform-specific paths or define the database path separately.
- verify_signed_message.py: encode UTF-8 byte length via proper CompactSize (not Python char count), matching Dash Core's varint scheme and fixing overflow above 64KB. - .gitignore: cover documented snapshot/backup names and sqlite sidecar files (-wal/-shm/-journal). - README.md: replace invalid `dashd signmessage` example with a real Dashmate Core CLI invocation; document the 03_fund_address.py step; add SQL fence language tag; note the Linux sqlite profile path. - 04_upgrade_to_v4.sh: bound the protocol-activation wait loop to 60 attempts instead of looping forever. Co-Authored-By: Codex Sol <noreply@openai.com>
Claudius-Maginificent
left a comment
Claudius-Maginificent
left a comment
There was a problem hiding this comment.
🧐 Grumpy review — 4-way pass (security + project-consistency + QA + Codex Sol)
Reviewed at 7279666c. Four independent reviewers produced 36 raw findings, consolidated to 26 after merging 10 duplicate clusters; 20 comments are posted here after dropping what 7279666c already fixed (see below). Zero CRITICAL, zero HIGH — that distribution is honest rather than generous: this PR adds no product code, touches no .rs file, and is unreachable from CI (tests.yml/clippy.yml filter on **/*.rs and friends; nothing under docs/** matches) and from every packaging path. The blast radius of everything here is the operator who runs the harness.
Advisory only — commenting, not requesting changes.
✅ What is genuinely good (verified, not assumed)
- No secrets committed anywhere in the tree or in any commit — the only 64-hex hits are the public secp256k1 domain constants. The
.gitignorewas written before the secrets exist. - The SPEC's load-bearing architectural claims hold.
core_backend_modedropped, migration v38's "SPV-only now" note,SpvProviderinsrc/context_provider.rs,REGTEST_P2P_PORT = 19899, DB v11→v38 — all independently spot-checked against the currentorigin/v1.0-devtip. - The crypto core survived adversarial testing. An independent re-implementation of ECDSA signing on top of the script's own primitives round-trips correctly; wrong-message and bit-flipped inputs are correctly rejected; hash160/Base58Check matches a known Bitcoin test vector.
- The dip0024 limitation is diagnosed to the quorum-parameter level and escalated as an open question rather than buried. That is the right instinct.
03_fund_address.pyhardcodes its RPC endpoint and cannot be pointed at mainnet even by a careless operator.- All shell scripts are shellcheck-clean.
- Supply-chain check came back clean: OSV.dev and the GitHub Global Advisory Database report no known advisories for dashmate 3.0.2 / 4.0.0 / any version.
🟡 The MEDIUM items worth your attention
- [
01_setup_v3_network.sh:15] Unguarded destructive first step. The documented step 1 force-removes everydashmate_*container and volume on the host and runsrm -rf ~/.dashmatewith no prompt, no dry-run, and no scoping to the local network. A developer with a co-resident testnet or mainnet dashmate node loses its Docker volumes — which hold the Core wallet — merely by starting this test. - [
01_setup_v3_network.sh:33] Real correctness bug — readiness check passes at height 0.!='1'on a string returns true for Tenderdash's'0', so the loop exits and prints ">> v3 network ready" against a chain with zero blocks, making step 2 fail for reasons unrelated to its actual cause. The same loop is also unbounded (unlike04, which7279666cbounded — the two are now inconsistent). - [
SPEC.md:41]test(...)title, zero tests. 0.rsfiles, nothing CI-enforced, and the scripts print rather than assert the behaviours the Results section records as "pass". The PR body is honest that UI steps are manual; the gap is between that honesty and the title plus the unqualified pass verdicts. - [
SPEC.md:1] Undeclareddocs/testing/docs root + branch is 2 commits behindv1.0-dev(mergeable_state: "blocked"). Genuinely mitigating: the convention that governs placement (docs/gui-testing/) landed in #860 — one of the two commits this branch is behind — so it was invisible from this base. - [
04_upgrade_to_v4.sh:11] Backup recipe writes a secret-bearing.dashmatetarball into$PWD, and the root.gitignorehas no*.tgz/keys.json/*.dbentry (the harness-local one does not govern the repo root). - [
01_setup_v3_network.sh:21]npx -ywith lifecycle scripts enabled on a host expected to hold masternode keys — exposure, not a live compromise. - [
README.md:14] macOS-only in practice, never says so, and omitsDASH_EVO_DATA_DIRfor the new DET.
The remaining LOW findings are robustness/idiom/DRY nits, posted inline.
🗑️ Dropped as already fixed in 7279666c (and duplicating existing threads)
Credit where due — the fix commit landed after this review ran and resolved three real findings, so they are not re-posted:
- The message-hash byte-length bug (
dash_message_hashvarinted the character count, not the UTF-8 byte length — a confident false negative on any non-ASCII message). Now correctly encodes-then-measures, andvarint()was upgraded to full CompactSize, which also fixes theOverflowErrorabove 64 KB. This was our headline correctness bug; nice fix. dashd signmessagenaming the wrong executable — now a workingnpx dashmate core cli "-rpcwallet=main signmessage ..."invocation, with SPEC.md updated to say "Dash Core'ssignmessageRPC".- README never naming
03_fund_address.py— now step 4 of the walkthrough.
Also partially addressed and re-scoped rather than dropped: the unbounded-poll-loop finding (04 bounded, 01 still not) and the macOS-path finding (the sqlite3 check now lists both paths; the Prerequisites rationale and DASH_EVO_DATA_DIR remain).
Bottom line
A genuinely useful, honestly-written harness. The concept is sound and the fixes are small — nothing here is about the idea, and the crypto that matters most is correct. The two I would not merge without are the unguarded rm -rf and the height-0 readiness check.
| DASHMATE_V3="${DASHMATE_V3:-dashmate@3.0.2}" # last stable v3 line (3.1 never released) | ||
| NODES="${NODES:-3}" | ||
|
|
||
| echo ">> Removing any existing local network + ~/.dashmate" |
There was a problem hiding this comment.
🟡 Unconditional rm -rf ~/.dashmate plus force-removal of every dashmate_* container and volume — no confirmation, no scoping, no backup (merge-blocking)
The documented first step of the runbook destroys state with no prompt and no dry-run. The grep '^dashmate_' glob is not scoped to the local network — it matches every dashmate container and volume on the host, including a testnet or mainnet masternode the developer happens to run on the same machine. Docker volumes for such a node hold the Core wallet; removing them is irreversible and, for a real masternode, means loss of key material and access to collateral. The || true on the wipe lines also means set -e will not halt a partial failure.
The comment on line 9 warns "Wipes any existing ~/.dashmate", but a comment is not a guardrail.
Recommendation: Gate the wipe behind an explicit opt-in — require FORCE_WIPE=1 or an interactive y/N confirmation that lists exactly which containers and volumes will be destroyed before touching anything. Scope the grep to the local-network prefix actually created by dashmate setup local rather than the broad ^dashmate_, so a co-resident testnet/mainnet node cannot be caught. Better still, honour a DASHMATE_HOME/--home override so the harness never points at the developer's default dashmate home. Drop the || true on the wipe so a partial failure stops the run.
🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (security-engineer-smythe + project-reviewer-adams + qa-engineer-marvin + Codex Sol)
| npx -y "$DASHMATE_V3" group start --verbose | ||
|
|
||
| echo ">> Waiting for platform blocks to flow" | ||
| until curl -s --max-time 3 http://127.0.0.1:46657/status \ |
There was a problem hiding this comment.
🟡 Readiness check exits the wait loop at height 0, declaring the network ready before it produced a block — and the loop is unbounded
Two coupled problems in this wait loop.
(1) Correctness. until terminates when the condition returns 0, and the predicate returns 0 for any height that is not the literal string '1'. Tenderdash reports latest_block_height as '0' in the window after it starts serving /status but before the first block. '0' != '1' is true, so the loop exits immediately and the script prints >> v3 network ready against a chain with zero blocks. The intent is clearly "wait until height has advanced past 1" — a > comparison on an integer, not a != on a string. Real consequence: step 2 (02_extract_masternode_keys.py) then fails for reasons unrelated to its actual cause.
(2) Robustness. The loop has no iteration cap, no max-wait, and no escape hatch — if dashmate/docker never reaches the expected height (image pull failure, port clash, OOM), it hangs forever. It also prints nothing at all while waiting. Note 04_upgrade_to_v4.sh was bounded to 60 attempts in 7279666c; this loop was not, so the two are now inconsistent — and 03_fund_address.py already models the right pattern with range(30) + raise SystemExit(1).
Recommendation:
sys.exit(0 if int(json.load(sys.stdin)['sync_info']['latest_block_height']) > 1 else 1)and bound the loop with a max retry count + explicit non-zero exit and diagnostic on timeout, matching what 04 now does. Additionally, assert protocol 11 before baseline work rather than printing an "expect protocol 11" message.
🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (security-engineer-smythe + project-reviewer-adams + qa-engineer-marvin + Codex Sol)
| rm -rf ~/.dashmate | ||
|
|
||
| echo ">> dashmate setup local (${DASHMATE_V3}, ${NODES} masternodes)" | ||
| npx -y "$DASHMATE_V3" setup local --node-count="$NODES" --no-debug-logs --miner-interval=1m --verbose |
There was a problem hiding this comment.
🟡 npx -y installs and executes dashmate plus its full transitive tree with lifecycle scripts enabled
Six separate npx -y dashmate@<ver> invocations across 01 and 04 auto-download and execute an npm package with -y suppressing the install confirmation, no --ignore-scripts, no lockfile, and no integrity pinning. The top-level version pin (dashmate@3.0.2, dashmate@4.0.0) is the right instinct and materially better than a range — but the pin does not extend to the transitive dependency tree, which resolves fresh on every run and executes preinstall/install/postinstall hooks with the invoking user's full permissions.
Research performed and stated plainly: OSV.dev returned {} for dashmate 3.0.2, 4.0.0 and all versions — no known advisories; the GitHub Global Advisory Database is likewise clean for the npm package. This is a supply-chain exposure finding, not a report of a live compromise.
The exposure matters here specifically because the same host is expected to hold masternode owner/voting/payout keys.
Recommendation: Pass --ignore-scripts to the npx invocations if dashmate tolerates it, or install dashmate once from a committed lockfile / pinned container image rather than resolving the tree on every run. At minimum, document in README "Prerequisites" that these scripts execute third-party npm lifecycle code and should be run on a scratch machine or container, not on a host holding mainnet keys.
🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (security-engineer-smythe + project-reviewer-adams + qa-engineer-marvin + Codex Sol)
| # Drive emits GroveDBProof::V1 and DET 0.9.3 can no longer decode proofs | ||
| # (expected — mirrors a real mainnet upgrade). | ||
| # | ||
| # BACK UP FIRST if you want to be able to return to the v3 state: |
There was a problem hiding this comment.
🟡 Backup recipe writes a secret-bearing ~/.dashmate tarball into $PWD, and the root .gitignore does not cover it
The header comment instructs the operator to run tar czf dashmate-home.tgz -C ~ .dashmate and docker run ... -v "$PWD":/b alpine tar czf "/b/$v.tgz" .... Both write into the current working directory, and the comment never states which directory that must be.
A .dashmate archive is high-value: it contains the Core wallet (owner/voting/payout private keys), the dashmate RPC passwords, and operator BLS keys.
Verified: the harness-local .gitignore covers *.tgz, but that file only governs docs/testing/masternode-identity-upgrade/harness/. The repository root .gitignore has no *.tgz, keys.json, or *.db entry (confirmed against this branch's HEAD). An operator who runs the recipe from the repo root — the natural cwd — produces an untracked-but-stageable secret archive.
Recommendation: Make the recipe path-explicit and self-protecting rather than relying on the operator's cwd: write backups to an absolute path outside the repo (e.g. ${BACKUP_DIR:-$HOME/dashmate-backups}) and mkdir -p it in the script. Failing that, add *.tgz, keys.json, and *.db to the root .gitignore so the rule holds regardless of cwd. Belt and braces: do both.
🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (security-engineer-smythe + project-reviewer-adams + qa-engineer-marvin + Codex Sol)
| DASHMATE_V4="${DASHMATE_V4:-dashmate@4.0.0}" | ||
|
|
||
| echo ">> Stopping v3 group" | ||
| npx -y dashmate@3.0.2 group stop --force || true |
There was a problem hiding this comment.
🟢 04 hardcodes dashmate@3.0.2 while 01 parameterizes the same value via $DASHMATE_V3
01 exposes DASHMATE_V3="${DASHMATE_V3:-dashmate@3.0.2}" specifically so an operator can pin a different v3 patch release, and uses that variable for every dashmate@3 invocation. 04 exposes DASHMATE_V4 but then stops the v3 group with a literal npx -y dashmate@3.0.2 group stop --force || true — it does not read $DASHMATE_V3 at all.
Override DASHMATE_V3 when setting up (the documented use case for the variable) and the teardown silently runs a different dashmate version against the network the other one created — npx will happily fetch 3.0.2 and operate on a config it did not write.
Recommendation: Use "${DASHMATE_V3:-dashmate@3.0.2}" in 04's stop command, matching 01's pattern — or hoist both version pins into a shared sourced versions.sh so the two scripts cannot disagree about which network they are talking to.
🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (security-engineer-smythe + project-reviewer-adams + qa-engineer-marvin + Codex Sol)
| network on protocol 11. | ||
| 2. **Phase 0:** Run `02_extract_masternode_keys.py` to write the gitignored | ||
| `keys.json` file. | ||
| 3. **Phase 1:** In *DET 0.9.3*, load an Evonode identity using a proTxHash and |
There was a problem hiding this comment.
🟢 Manual DET UI steps re-invent the docs/gui-testing/ tier instead of using its TEMPLATE.md and safety rules
Steps 3, 8 and 10 of the walkthrough, and the whole "Additional check — encrypted wallet survives the upgrade" section, are manual driving of the compiled binary through a real display. That is precisely the tier docs/gui-testing/ owns: it ships a canonical scenarios/TEMPLATE.md with structured sections (Verifies / Tier justification / Prerequisites / Setup / Procedure / Safety constraints / Expected outcome / Known gotchas) plus seven "Non-negotiable safety rules". This PR re-derives that structure ad hoc and re-derives the isolation rule in its own words.
Mitigating context, stated plainly: docs/gui-testing/ landed on v1.0-dev via PR #860 (commit 1397b434), which is one of the two commits this branch is behind. The authors could not have used a template that did not exist on their base — this is drift created by the branch being stale, not by carelessness.
Recommendation: Rebase onto current v1.0-dev, read docs/gui-testing/README.md, and re-express the manual UI portions as a scenario file from scenarios/TEMPLATE.md, keeping the genuinely novel dashmate/network/key tooling as scripts the scenario links to. Do not maintain two scenario formats.
🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (security-engineer-smythe + project-reviewer-adams + qa-engineer-marvin + Codex Sol)
| `.gitignore`). The DET UI steps are still manual today — the value here is the | ||
| deterministic network/keys/verification tooling an automated test can build on. | ||
|
|
||
| ## Prerequisites |
There was a problem hiding this comment.
🟢 Harness README duplicates docs/local-network.md and diverges on how to obtain the RPC password
docs/local-network.md already documents dashmate local-network setup, the 20302 seed RPC port, and the sanctioned way to read the RPC password:
dashmate config get core.rpc.users.dashmate.password --config=local_seedThe harness restates the prerequisites and instead parses ~/.dashmate/config.json directly (cfg['configs']['local_seed']['core']['rpc']['users']['dashmate']['password']), hardcodes SEED_RPC = 'http://127.0.0.1:20302/', and never references docs/local-network.md.
Two documented ways to obtain the same value is one too many — and the config.json path is an internal layout that dashmate is free to change between v3 and v4, which is a real hazard given this harness's entire purpose is spanning a v3→v4 upgrade.
Recommendation: Link docs/local-network.md from this README rather than restating prerequisites, and prefer the supported dashmate config get accessor over reaching into config.json internals. At minimum, note that the config.json shape is version-coupled.
🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (security-engineer-smythe + project-reviewer-adams + qa-engineer-marvin + Codex Sol)
| the profile. | ||
| 6. **Phase 2:** Run `04_upgrade_to_v4.sh` to upgrade the network in place from | ||
| v3 to v4 and wait for protocol 12. | ||
| 7. **Phase 3:** Run `p2p_proxy.py`. This is **required for the new DET**; see the |
There was a problem hiding this comment.
🟢 The NN_ step-numbering convention is applied to only 4 of the documented steps
The walkthrough numbers 10 steps. Scripts 01–04 carry the NN_ prefix; but p2p_proxy.py (step 7), query_identity_dapi.sh (step 9), and verify_signed_message.py (step 10) are unprefixed.
The convention self-evidently exists — it is just abandoned halfway, so an ls of the directory no longer tells you the running order and the README becomes the only source of sequence.
Recommendation: Either prefix the remaining ordered scripts (05_p2p_proxy.py, 07_query_identity_dapi.sh, 08_verify_signed_message.py) or drop the prefixes entirely and let the README own sequencing. Half a convention is worse than either whole one.
🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (security-engineer-smythe + project-reviewer-adams + qa-engineer-marvin + Codex Sol)
| @@ -0,0 +1,36 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
🟢 p2p_proxy.py permanently encodes a workaround for a hardcoded DET port with no tracking issue
The gotcha claim checks out — const REGTEST_P2P_PORT: u16 = 19899; is confirmed at src/wallet_backend/mod.rs:2528 on origin/v1.0-dev, so DET genuinely cannot reach dashmate's local_1 Core on 20001. Credit for diagnosing it precisely.
But the PR's response is to commit a permanent TCP proxy and a README note, while the other limitation it found (dip0024) gets a prominent SPEC "Open question for the PR authors". A hardcoded regtest P2P port that makes the product unusable against a stock dashmate local network is a product defect of the same family and deserves the same visibility. As written, the workaround will quietly outlive the bug and nobody will know to delete it.
Recommendation: File a GitHub issue for the hardcoded REGTEST_P2P_PORT (making it configurable) and reference it (#NNNN) from this docstring and the README gotcha, so the proxy has a defined death date.
🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (security-engineer-smythe + project-reviewer-adams + qa-engineer-marvin + Codex Sol)
| @@ -0,0 +1,15 @@ | |||
| # Never commit regenerable local secrets / fixtures — they contain private keys | |||
There was a problem hiding this comment.
🟢 .gitignore comment "(the test wallet has no password)" went stale within this same PR
The header reads: "Never commit regenerable local secrets / fixtures — they contain private keys and recoverable wallet seeds (the test wallet has no password)."
A later commit on this branch adds the "Additional scenario: password-encrypted wallet survives the upgrade" flow, whose entire point is a wallet created with a strong password. So the parenthetical is now wrong for one of the two wallets the harness produces, within the same PR that wrote it.
Minor, but it is precisely the cross-artifact drift that compounds: the rationale for ignoring *.db is now half-stated.
Recommendation: Reword to state the invariant rather than a per-wallet fact — e.g. "these files contain private keys and wallet seeds; a no-password wallet's seed is recoverable outright, and a password-protected one is still an offline-attackable target." The ignore rule is right either way; only the justification needs to stop being version-specific.
🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (security-engineer-smythe + project-reviewer-adams + qa-engineer-marvin + Codex Sol)
There was a problem hiding this comment.
Preliminary review — Codex only
Exact-head verification confirms two in-scope blocking defects in initial network setup: the script destructively wipes broad Dashmate state without explicit consent, and its readiness predicate accepts block height zero. Two suggestions also remain: the setup wait is unbounded, as identified by the partially resolved prior finding, and the extracted WIF file is created with permissive default filesystem permissions. Five of the six indexed prior findings are fully resolved.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Source: Codex reviewer gpt-5.6-sol; verifier gpt-5.6-sol.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 2 suggestion(s)
🤖 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 `docs/testing/masternode-identity-upgrade/harness/01_setup_v3_network.sh`:
- [BLOCKING] docs/testing/masternode-identity-upgrade/harness/01_setup_v3_network.sh:15-18: Require explicit consent before deleting Dashmate state
The first documented workflow step force-removes every container and volume whose name begins with `dashmate_`, then recursively deletes the user's entire default `~/.dashmate` directory. These operations are not scoped to the network created by this harness, so unrelated Dashmate profiles, wallets, configuration, keys, and node data on the same host can be irreversibly destroyed without confirmation. Require an explicit destructive opt-in and run the harness against an isolated Dashmate home or configuration before deleting any state.
- [BLOCKING] docs/testing/masternode-identity-upgrade/harness/01_setup_v3_network.sh:33-34: Do not treat block height zero as network readiness
The `until` command succeeds for every height except the literal string `"1"`. Tenderdash can serve `/status` with `latest_block_height` equal to `"0"` before producing its first block, so the script can immediately announce that the v3 network is ready and advance to key extraction against an unready network. Parse the height numerically and require it to advance beyond 1.
- [SUGGESTION] docs/testing/masternode-identity-upgrade/harness/01_setup_v3_network.sh:33-36: Bound the v3 setup readiness wait
The prior timeout finding explicitly requested a finite bound for both the protocol-activation loop and this setup-readiness loop. The latest commit bounds `04_upgrade_to_v4.sh`, but this loop still waits forever if Dashmate, Docker, or the status endpoint never becomes usable. Add a maximum attempt count, a diagnostic timeout message, and a nonzero exit, matching the bounded pattern in the upgrade script.
In `docs/testing/masternode-identity-upgrade/harness/02_extract_masternode_keys.py`:
- [SUGGESTION] docs/testing/masternode-identity-upgrade/harness/02_extract_masternode_keys.py:61: Create the private-key file with restricted permissions
`open(OUT, "w")` creates `keys.json` using the process umask; with the common `022` setting, verification produced mode `0644`. The file contains every masternode's owner, voting, and payout WIF private keys, so it should not be readable by other local users. `.gitignore` prevents ordinary Git discovery but does not enforce filesystem access controls. Create the file as mode `0600` and explicitly restrict an existing file before writing it.
| echo ">> Removing any existing local network + ~/.dashmate" | ||
| docker ps -a --format '{{.Names}}' | grep '^dashmate_' | xargs -r docker rm -f >/dev/null 2>&1 || true | ||
| docker volume ls --format '{{.Name}}' | grep '^dashmate_' | xargs -r docker volume rm >/dev/null 2>&1 || true | ||
| rm -rf ~/.dashmate |
There was a problem hiding this comment.
🔴 Blocking: Require explicit consent before deleting Dashmate state
The first documented workflow step force-removes every container and volume whose name begins with dashmate_, then recursively deletes the user's entire default ~/.dashmate directory. These operations are not scoped to the network created by this harness, so unrelated Dashmate profiles, wallets, configuration, keys, and node data on the same host can be irreversibly destroyed without confirmation. Require an explicit destructive opt-in and run the harness against an isolated Dashmate home or configuration before deleting any state.
source: ['codex']
| until curl -s --max-time 3 http://127.0.0.1:46657/status \ | ||
| | python3 -c "import json,sys; sys.exit(0 if json.load(sys.stdin)['sync_info']['latest_block_height']!='1' else 1)" 2>/dev/null; do |
There was a problem hiding this comment.
🔴 Blocking: Do not treat block height zero as network readiness
The until command succeeds for every height except the literal string "1". Tenderdash can serve /status with latest_block_height equal to "0" before producing its first block, so the script can immediately announce that the v3 network is ready and advance to key extraction against an unready network. Parse the height numerically and require it to advance beyond 1.
| until curl -s --max-time 3 http://127.0.0.1:46657/status \ | |
| | python3 -c "import json,sys; sys.exit(0 if json.load(sys.stdin)['sync_info']['latest_block_height']!='1' else 1)" 2>/dev/null; do | |
| until curl -s --max-time 3 http://127.0.0.1:46657/status \ | |
| | python3 -c "import json,sys; sys.exit(0 if int(json.load(sys.stdin)['sync_info']['latest_block_height']) > 1 else 1)" 2>/dev/null; do |
source: ['codex']
| until curl -s --max-time 3 http://127.0.0.1:46657/status \ | ||
| | python3 -c "import json,sys; sys.exit(0 if json.load(sys.stdin)['sync_info']['latest_block_height']!='1' else 1)" 2>/dev/null; do | ||
| sleep 5 | ||
| done |
There was a problem hiding this comment.
🟡 Suggestion: Bound the v3 setup readiness wait
The prior timeout finding explicitly requested a finite bound for both the protocol-activation loop and this setup-readiness loop. The latest commit bounds 04_upgrade_to_v4.sh, but this loop still waits forever if Dashmate, Docker, or the status endpoint never becomes usable. Add a maximum attempt count, a diagnostic timeout message, and a nonzero exit, matching the bounded pattern in the upgrade script.
source: ['codex']
| ("payoutPrivkey", "payoutAddress")): | ||
| entry[role] = call(pw, "dumpprivkey", [st[addr_key]], wallet="main") | ||
| out[p["proTxHash"][:8]] = entry | ||
| json.dump(out, open(OUT, "w"), indent=2) |
There was a problem hiding this comment.
🟡 Suggestion: Create the private-key file with restricted permissions
open(OUT, "w") creates keys.json using the process umask; with the common 022 setting, verification produced mode 0644. The file contains every masternode's owner, voting, and payout WIF private keys, so it should not be readable by other local users. .gitignore prevents ordinary Git discovery but does not enforce filesystem access controls. Create the file as mode 0600 and explicitly restrict an existing file before writing it.
| json.dump(out, open(OUT, "w"), indent=2) | |
| fd = os.open(OUT, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) | |
| os.fchmod(fd, 0o600) | |
| with os.fdopen(fd, "w") as output_file: | |
| json.dump(out, output_file, indent=2) |
source: ['codex']
4 participants
What
Adds a documented, reproducible test harness + spec for verifying that a
masternode (evonode) identity added as a standalone identity in DET 0.9.3
survives an in-place upgrade to a newer DET (validated here against PR #887),
across a
dashmatev3→v4 network upgrade (protocol 11→12).Docs/scripts only — no product code changes.
Spec
See
docs/testing/masternode-identity-upgrade/SPEC.md. Versions exercised:3268b736)29f7492e) — GroveDBProof V0fix/qa-followups-885(8a47745a)93b967f9) — V1Results
dashd signmessage(deterministic RFC6979), accepted bydashd verifymessage.Finding for the authors
A withdrawal through the new DET can't be exercised on a local network. Its
SPV-only proof verifier (
SpvProvider, no core-RPC fallback) needs anllmq_test_dip0024rotation quorum (type 103, minSize 4) that a 3-masternodedashmatelocal net can't form, and regtest exposes no param to shrink it — somasternodes_ready()never becomes true and every proof-verified op fails with"servers unreachable". DET 0.9.3 was unaffected because it read quorum keys from
its local Core node over RPC. Open question: is local
dashmatea supportedtarget for the SPV verifier, or should the masternode sync fall back to the
legacy
mnlistdiffpath when no rotation quorum exists? Until then thekey-signing check is the local substitute for an end-to-end withdrawal. Details
in SPEC.md "Known limitation".
Contents
docs/testing/masternode-identity-upgrade/SPEC.md— short spec: goal, versions, steps, results, findingharness/— reproducible scripts (network setup, key extraction, funding, v3→v4 upgrade, SPV P2P-port proxy, DAPI liveness query) andverify_signed_message.py, a dependency-free secp256k1 verifier for the key-functionality checkSecrets (private keys, wallet DBs, profile snapshots) are gitignored and
regenerated locally — nothing sensitive is committed. DET UI steps are manual
today; this lands the deterministic network/keys/verification tooling an
automated test can build on.
Commits
test(masternode-upgrade): DET 0.9.3 baseline harness + spectest(masternode-upgrade): v3->v4 upgrade + new-DET verification (PR #887)🤖 Generated with Claude Code
Summary by CodeRabbit