Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 103 additions & 0 deletions docs/testing/masternode-identity-upgrade/SPEC.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
# Masternode-identity upgrade test — spec

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.

🟡 docs/testing/ invents a third docs location that matches no documented convention — and the branch is behind v1.0-dev, where the convention that governs it landed (merge-blocking)

Two findings with one shared remediation.

(1) Undeclared docs root. The repo documents exactly two homes for this artifact. At this branch's merge-base (87ba5b71), CLAUDE.md already read: "docs/ai-design should contain architecture, technical design and manual testing scenarios files, grouped in subdirectories prefixed with ISO-formatted date." On current origin/v1.0-dev, CLAUDE.md keeps that rule and adds docs/gui-testing/ as the home for reusable, non-date-grouped GUI testing practice. This PR creates docs/testing/masternode-identity-upgrade/ — a third top-level docs tree that is (a) not under docs/ai-design/, (b) not ISO-date-prefixed, (c) not docs/gui-testing/, and (d) not mentioned anywhere in CLAUDE.md.

(2) Branch is 2 commits behind origin/v1.0-dev, and GitHub reports mergeable_state: "blocked". Verified via git and the GitHub API, not the diff alone: git merge-base HEAD origin/v1.0-dev = 87ba5b71; git log 87ba5b71..origin/v1.0-dev --oneline shows exactly 2 commits ahead — 1397b434 feat: rewrite Dash Evo Tool onto the new platform-wallet (#860) and 3b32bed2 ci: replace weekly build check with weekly draft pre-release (#896). This is not cosmetic: #860 is the very commit that introduced docs/gui-testing/, so the convention that should govern this PR's placement is invisible from its base. In fairness, that is genuinely mitigating — the authors could not have read a convention that did not exist on their base.

Recommendation: Rebase onto current origin/v1.0-dev (docs-only diff — should be a no-op rebase) and confirm mergeable_state clears. Then re-decide placement with current conventions in view: move to docs/ai-design/2026-07-XX-masternode-identity-upgrade/ if the SPEC is a dated point-in-time record, or put the manual scenario under docs/gui-testing/scenarios/ if the intent is standing reusable practice. If docs/testing/ is genuinely meant to be a new fourth category, declare it in CLAUDE.md's Documentation section in this same PR — an undeclared docs root is drift by definition.

🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (security-engineer-smythe + project-reviewer-adams + qa-engineer-marvin + Codex Sol)


## Goal

Prove that a masternode (evonode) **identity added as a standalone identity in
DET 0.9.3** survives an in-place upgrade to a newer DET — the identity, its keys,
and its on-chain state must all still be usable after the app and the platform
network are upgraded, the way a real user's install would be.

## Versions under test

| Role | DET | pin |
|------|-----|-----|
| baseline (old) | `v0.9.3` — commit `3268b736` | dash-sdk platform rev `29f7492e` (platform 2.1.0, grovedb 3.1) |
| upgrade (new) | PR #887 `fix/qa-followups-885` — commit `8a47745a`, base `feat/legacy-identity-migration` | dash-sdk platform rev `93b967f9` (platform 4.0) |

Network: local `dashmate` group, **3 masternodes**. Started on dashmate **v3**
(protocol 11), then upgraded in place to **v4** (protocol 12) — mirroring a real
mainnet protocol upgrade between the two DET versions.

## What we did

1. **Baseline on 0.9.3 (v3 network / protocol 11).** Loaded an evonode identity
by proTxHash + owner/voting/payout WIFs (for a masternode the platform owner
identity id *is* the proTxHash). Created a DET wallet, funded it from the
local_seed Core wallet, topped the identity up via asset lock, and **withdrew
credits to L1 Dash** — confirmed the asset-unlock landed on the Core chain.
Snapshotted the DET profile (`backup-1` post-load, `backup-2` post-withdrawal).

2. **Upgraded the network v3 -> v4 in place.** `dashmate@4 group start` migrated
the config and images; validators signalled protocol 12, which activated at
the next epoch boundary. From then on Drive emits `GroveDBProof::V1`, which
0.9.3's SDK cannot decode (expected — this is why the app must be upgraded).

3. **Ran the new DET (PR #887) on a copy of the 0.9.3 profile.** The DB migrated
cleanly (schema v11 -> v38, no data loss) and the evonode identity appeared in
the new **Masternodes** section: Active, all three key roles (Voting / Owner /
Payout) and the derived voter identity intact.

## Results

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.

🟡 PR is titled test(masternode-upgrade) but adds zero automated, asserting, or CI-enforced tests; the recorded pass claims are a frozen manual snapshot

Confirmed via git diff origin/v1.0-dev...HEAD --name-only: 10 files changed, all under docs/testing/masternode-identity-upgrade/, 0 .rs files. No #[test], no #[ignore]-gated harness, nothing registered under tests/backend-e2e/ — the project's own convention for network-dependent tests per CLAUDE.md and tests/backend-e2e/README.md. Nothing under docs/** matches the path filters in tests.yml/clippy.yml, so none of this is reachable from CI or any packaging path.

Beyond the absence of CI enforcement, the committed scripts do not assert the behaviours this Results section records as passes. No script inspects the migrated alias/key roles, compares pre/post state, or performs a post-upgrade DET operation. The single DAPI helper prints a balance; it does not check one. So "Migration: pass" is a claim about one operator's one run at one commit, not a reproducible verdict — and nothing will tell the next person if it regresses.

Stated fairly: the PR body is honest that "DET UI steps are manual today", and the harness genuinely is the deterministic groundwork an automated test could build on. The gap is between that honest framing and the test(...) title plus the unqualified "pass" verdicts here.

Recommendation: Either (a) reframe as docs rather than test(...) — this is a QA report plus a reusable manual runbook — or (b) wire the deterministic, non-GUI parts (key extraction, DAPI liveness query, signature verification) into an actual #[ignore]-gated backend-e2e test per tests/backend-e2e/README.md. Either way: record a pre-upgrade manifest (proTxHash, alias, role→address mapping, balance, schema version) and add an orchestrated post-upgrade check that asserts those values rather than printing them. Mark the GUI-driven parts explicitly as manual and re-run-required.

🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (security-engineer-smythe + project-reviewer-adams + qa-engineer-marvin + Codex Sol)


- **Migration: pass.** Identity + keys + alias survive the 0.9.3 -> PR-887 DB
migration.
- **On-chain liveness: pass.** Direct DAPI `getIdentity`/`getIdentityBalance`

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.

🟢 SPEC claims getIdentity/getIdentityBalance were used, but the script only ever calls getIdentityBalance

This Results bullet states: "Direct DAPI getIdentity/getIdentityBalance (bypassing DET) shows the identity live on protocol 12 with a real, growing balance — proof the migrated reference points at the genuine on-chain identity."

query_identity_dapi.sh contains exactly one grpcurl invocation — Platform/getIdentityBalance (:20). There is no getIdentity call anywhere in the harness.

The distinction is not pedantic. getIdentity is what returns the identity's key material and revision — i.e. the direct on-chain corroboration that the migrated keys match the chain — whereas getIdentityBalance proves only that credits exist at that id. This bullet leans on the result as "proof the migrated reference points at the genuine on-chain identity", which is precisely the claim a balance query alone does not support.

Recommendation: Pick one and make them agree — either add the getIdentity grpcurl call to query_identity_dapi.sh (preferred; it is four lines and materially strengthens the evidence), or correct this bullet to say getIdentityBalance only and soften the "genuine on-chain identity" claim to what a balance query actually supports.

🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (security-engineer-smythe + project-reviewer-adams + qa-engineer-marvin + Codex Sol)

(bypassing DET) shows the identity live on protocol 12 with a real, growing
balance — proof the migrated reference points at the genuine on-chain identity.
- **Key functionality: pass.** DET's *Key Info -> Sign Message* signs a known
message with the migrated **owner** and **payout (TRANSFER)** keys; each
signature (a) recovers to the key's expected address, (b) is byte-identical to
what `dashd signmessage` produces with the genuine private key (deterministic
RFC6979), and (c) is accepted by `dashd verifymessage`. This proves the private
key material survived migration and still signs correctly.

## Additional scenario: password-encrypted wallet survives the upgrade

Separately verifies that a **password-protected wallet** created in DET 0.9.3
survives the upgrade and can still be unlocked with the same password.

**What we did.** In DET 0.9.3, created an HD wallet with a strong password
(0.9.3 encrypts the seed with AES-256-GCM, key derived from the password via
Argon2). Confirmed at the storage layer — `data.db` `wallet.uses_password = 1`
for that wallet (vs `0` for a no-password wallet). Snapshotted the profile, then
ran the new DET on a copy.

**Result: pass.** After the DB migration the wallet appears in the new DET as
`HD: <name>`, is correctly recognized as encrypted (it starts **locked** with an
**Unlock** button), and **unlocks with the original password** (the button flips
to **Lock** and address-derivation controls appear). Because the seed is stored
with *authenticated* encryption (AES-256-GCM), a successful unlock is conclusive:
a wrong password or a seed corrupted by migration would fail the GCM auth tag and
refuse to unlock. So the encrypted seed and the password both survive the
migration intact. (Address rows don't populate in the UI while SPV is unsynced —
same dip0024 cause as below — but that's independent of the unlock proof.)

## Known limitation (candidate finding for the PR)

A **withdrawal through the new DET cannot be exercised on a local network.** The
new DET verifies platform proofs *trustlessly* via its own SPV-synced masternode
list (`SpvProvider` in `src/context_provider.rs`; no core-RPC fallback — the old
`core_backend_mode` was dropped, migration v38 notes "SPV-only now"). That SPV
masternode sync uses the DIP24 `qrinfo` path, which needs an `llmq_test_dip0024`
rotation quorum. That quorum is **type 103, size 4, minSize 4** (per dash-spv
`test_utils/masternode_network.rs`), so it cannot form on a 3-masternode local
network, and regtest exposes no param to shrink it (only `llmq_test`,
`llmq_test_instantsend`, `llmq_test_platform` are resizable). Result: the new
DET's `masternodes_ready()` never becomes true, every DAPI address gets banned
("servers unreachable"), and no proof-verified op (balance refresh, withdrawal)
can run locally.

DET 0.9.3 was unaffected because it fetched quorum keys straight from its local
Core node over RPC (`core.get_quorum_public_key`), which has no dip0024 dependency.

**Open question for the PR authors:** is a standard `dashmate` local network a
supported target for the new SPV-only verifier, or should the SPV masternode sync
fall back to the legacy `mnlistdiff` path (or accept trusted core quorum data)
when no rotation quorum exists? Until then, an end-to-end withdrawal has to be
tested on devnet/testnet (which have rotation quorums), and the **key-signing
check above is the local substitute** that proves the migrated keys are usable.

## How to reproduce

See `harness/README.md`. The scripts regenerate the network, keys, and profiles
locally; secrets (private keys, wallet DBs) are gitignored and never committed.
10 changes: 10 additions & 0 deletions docs/testing/masternode-identity-upgrade/harness/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# Never commit regenerable local secrets / fixtures — they contain private keys

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.

🟢 .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)

# and recoverable wallet seeds (the test wallet has no password).
keys.json
*.db
*.sqlite
*.sqlite-*
backups/
home-*/
Comment thread
lklimek marked this conversation as resolved.
Resolved
*.tgz
network-backup*/
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
#!/bin/sh
# Phase 0 — stand up a local platform network on dashmate v3 (protocol 11).
#
# Why v3: v3 Drive (grovedb 4) only emits GroveDBProof::V0, which DET 0.9.3's
# SDK (platform 2.1, grovedb 3.1) can decode. That keeps the 0.9.3 baseline
# working with no time pressure. The network is later upgraded to v4 in
# 04_upgrade_to_v4.sh, which is the actual v3->v4 protocol-11->12 migration.
#
# Requires: docker running, npx available. Wipes any existing ~/.dashmate.
set -e

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"

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.

🟡 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)

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
Comment on lines +15 to +18

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.

🔴 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']


echo ">> dashmate setup local (${DASHMATE_V3}, ${NODES} masternodes)"
npx -y "$DASHMATE_V3" setup local --node-count="$NODES" --no-debug-logs --miner-interval=1m --verbose

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.

🟡 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)


# Fast, steady platform blocks so DKG/sync and manual UI steps don't stall.
for cfg in local_1 local_2 local_3 local_seed; do
npx -y "$DASHMATE_V3" config set --config="$cfg" \
platform.drive.tenderdash.consensus.createEmptyBlocksInterval 10s
done

echo ">> Starting group"
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 \

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.

🟡 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)

| 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
Comment on lines +33 to +34

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.

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

Suggested change
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']

sleep 5
done
Comment on lines +33 to +36

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.

🟡 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']

curl -s http://127.0.0.1:46657/status | python3 -c "import json,sys; d=json.load(sys.stdin); print('platform height', d['sync_info']['latest_block_height'], 'protocol', d['node_info']['protocol_version']['app'])"
echo ">> v3 network ready (expect protocol 11)."
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
#!/usr/bin/env python3
"""Extract each local masternode's platform-identity keys from the running
network, so they can be loaded into DET as a standalone evonode identity.

For a masternode/evonode, the platform OWNER identity id == the proTxHash, and
its keys are the owner / voting / payout address keys. dashmate only persists
the operator BLS key in config.json; the owner/voting/payout private keys live
in the local_seed Core wallet ("main"), retrievable via dumpprivkey.

Writes keys.json next to this script. keys.json is gitignored (contains WIF
private keys) — regenerate it locally, never commit it.

Requires: the network from 01_setup_v3_network.sh running.
"""
import json
import os
import subprocess

DASHMATE_CONFIG = os.path.expanduser("~/.dashmate/config.json")
SEED_RPC = "http://127.0.0.1:20302/"
OUT = os.path.join(os.path.dirname(__file__), "keys.json")


def rpc_password():
cfg = json.load(open(DASHMATE_CONFIG))

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.

🟢 Idiom nits: files opened without context managers; bare except Exception: pass; magic 8-char proTxHash key with no collision guard

Three small slips, grouped:

(1) Unclosed file handles — json.load(open(DASHMATE_CONFIG)) (:25) and json.dump(out, open(OUT, "w"), indent=2) (:61) rely on CPython refcounting to close the handle. The json.dump case is the one that bites: on any non-refcounting interpreter, keys.json can be left truncated — and the whole run depends on that file.

(2) Silent proxy failures — p2p_proxy.py swallows every exception with bare except Exception: pass in pipe() and handle() (:14-20, :26), so a proxy that has silently stopped relaying is indistinguishable from one that is idle. Unhelpful for the one component the README calls "required for the new DET".

(3) Truncated dict key — out[p["proTxHash"][:8]] = entry (:60) uses an unexplained 8-hex-char (32-bit) truncation as a dict key with no collision guard; a collision silently drops a masternode's keys.

Recommendation: Use with open(...) as f: for both reads and the write; log the exception in the proxy's handlers (even a one-line print(..., flush=True)) instead of pass; key the dict on the full proTxHash (or assert no collision before writing) and truncate only for display in the print loop.

🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (security-engineer-smythe + project-reviewer-adams + qa-engineer-marvin + Codex Sol)

return cfg["configs"]["local_seed"]["core"]["rpc"]["users"]["dashmate"]["password"]


def call(pw, method, params=None, wallet=None):
url = SEED_RPC + (f"wallet/{wallet}" if wallet else "")
body = json.dumps({"jsonrpc": "1.0", "id": "h", "method": method, "params": params or []})
r = subprocess.run(

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.

🟢 call() leaks the RPC password into the host process list, ignores curl's exit status, and sets no timeouts

Grouping three defects in this one helper (duplicated verbatim into 03_fund_address.py, see separate note):

(1) Credential in argv. The dashmate RPC password is handed to curl as --user dashmate:{pw}. Process arguments are world-readable on Linux via /proc/<pid>/cmdline and ps aux, so any unprivileged local process can read it for the window each curl runs — and 02 issues one curl per masternode per key role while 03 polls up to 30 times, so the window is repeated, not momentary. Impact on a dev box is genuinely low (the credential guards a localhost regtest node) and is not being inflated — it is flagged as a credential-handling pattern that should not be copied forward.

(2) Exit status ignored. subprocess.run(...) is called without check=True and r.returncode/r.stderr are never inspected. When curl fails — daemon down, wrong port, TLS refusal, i.e. the most likely failure modes in a harness whose whole job is standing networks up and down — r.stdout is empty and the next line raises a bare json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0). That traceback sends the operator debugging JSON instead of connectivity.

(3) No timeouts. No --connect-timeout/--max-time, so a stalled node blocks the helper indefinitely.

Recommendation: Drop the subprocess entirely and use urllib.request from the stdlib with an Authorization: Basic header and an explicit timeout — no new dependency, keeps the credential out of argv, gives real exceptions, removes the fork-per-call cost, and is consistent with verify_signed_message.py's zero-dependency ethos. Add bounded retries.

🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (security-engineer-smythe + project-reviewer-adams + qa-engineer-marvin + Codex Sol)

["curl", "-s", "--user", f"dashmate:{pw}", "--data-binary", body,
"-H", "content-type: text/plain;", url],
capture_output=True, text=True,
)
d = json.loads(r.stdout)
if d.get("error"):
raise RuntimeError(d["error"])
return d["result"]


def main():
pw = rpc_password()
out = {}
for p in call(pw, "protx", ["list", "registered", True]):
st = p["state"]
entry = {
"proTxHash": p["proTxHash"], # == platform owner identity id
"type": p.get("type"),
"ownerAddress": st["ownerAddress"],
"votingAddress": st["votingAddress"],
"payoutAddress": st["payoutAddress"],
"operatorPubKey": st["pubKeyOperator"],
}
for role, addr_key in (("ownerPrivkey", "ownerAddress"),
("votingPrivkey", "votingAddress"),
("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)

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.

🟢 keys.json written with the default umask, leaving WIF private keys world-readable inside the repo worktree

json.dump(out, open(OUT, "w"), indent=2) creates keys.json with the process default umask — 0644 on a typical Linux/macOS developer account, i.e. readable by every local user. The contents are exactly what the module docstring says: owner, voting, and payout WIF private keys for every masternode, plus the proTxHash identifying the platform identity.

Compounding this, the file is written into the repository worktree and stays there after the run. .gitignore prevents normal Git discovery — it does not protect against other local users, backup daemons, editor indexers, or git add -f. This conflicts with least-privilege secret handling (OWASP ASVS V13.3.1).

Recommendation: Create the file with restrictive permissions from the outset rather than chmod-ing after the fact (which leaves a race):

fd = os.open(OUT, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
with os.fdopen(fd, "w") as f:
    json.dump(out, f, indent=2)

Better still, write it outside the repository into a mode-0700 temp directory, document its lifetime, and remove it after the manual import step.

🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (security-engineer-smythe + project-reviewer-adams + qa-engineer-marvin + Codex Sol)

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.

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

Suggested change
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']

print(f"wrote {OUT} ({len(out)} masternode identities)")
for k, v in out.items():
print(f" {k} {v['type']} owner={v['ownerAddress']} payout={v['payoutAddress']}")


if __name__ == "__main__":
main()
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
#!/usr/bin/env python3
"""Fund a DET wallet receive address from the local_seed Core wallet ("main"),
then wait for one confirmation. Used in phase 1 to give the DET wallet UTXOs so
it can top up the identity via asset lock.

Usage: 03_fund_address.py <dash_address> [amount_dash]
Requires: the network running.
"""
import json
import os
import subprocess
import sys
import time

DASHMATE_CONFIG = os.path.expanduser("~/.dashmate/config.json")
SEED_RPC = "http://127.0.0.1:20302/"


def call(pw, method, params=None, wallet=None):

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.

🟢 The entire Core-RPC call() helper and its config lookup are duplicated verbatim across 02 and 03

DASHMATE_CONFIG, SEED_RPC, and the 12-line call(pw, method, params, wallet) body are byte-identical in both files.

The password lookup is duplicated too, but inconsistently: 02 factors it into a named rpc_password() helper (02:24-26) while 03 inlines the same six-level subscript chain on one 100-column line (03:39). Two copies of an RPC client is where the third copy comes from — and the divergence between the named and inlined variants shows the drift started before the code even landed.

The magic 20302 is likewise pinned in two places here and a third in docs/local-network.md.

Recommendation: Extract a dashmate_rpc.py module next to the scripts exposing rpc_password() and call(), and import it from both. That also gives the error handling, timeouts, and the dashmate config get accessor recommended elsewhere in this review exactly one place to live.

🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (security-engineer-smythe + project-reviewer-adams + qa-engineer-marvin + Codex Sol)

url = SEED_RPC + (f"wallet/{wallet}" if wallet else "")
body = json.dumps({"jsonrpc": "1.0", "id": "h", "method": method, "params": params or []})
r = subprocess.run(
["curl", "-s", "--user", f"dashmate:{pw}", "--data-binary", body,
"-H", "content-type: text/plain;", url],
capture_output=True, text=True,
)
d = json.loads(r.stdout)
if d.get("error"):
raise RuntimeError(d["error"])
return d["result"]


def main():
if len(sys.argv) < 2:
print("usage: 03_fund_address.py <dash_address> [amount_dash]")
raise SystemExit(2)
address = sys.argv[1]
amount = float(sys.argv[2]) if len(sys.argv) > 2 else 10.0
pw = json.load(open(DASHMATE_CONFIG))["configs"]["local_seed"]["core"]["rpc"]["users"]["dashmate"]["password"]

txid = call(pw, "sendtoaddress", [address, amount], wallet="main")
print(f"sent {amount} DASH to {address}: {txid}")
for _ in range(30):
conf = call(pw, "gettransaction", [txid], wallet="main").get("confirmations", 0)
if conf >= 1:
print(f"confirmed ({conf} conf)")
return
time.sleep(10)
print("WARNING: not confirmed within timeout")
raise SystemExit(1)


if __name__ == "__main__":
main()
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
#!/bin/sh
# Phase 2 — upgrade the SAME running network from v3 to v4 in place.
#
# dashmate v4 migrates the config file (configFormatVersion 3 -> 4) and bumps
# every image tag to v4 (drive:4, tenderdash 1.6.0, rs-dapi:4). v4 Drive reads
# the v3 on-disk state (the mainnet-supported upgrade path). v4 validators
# signal protocol 12, which activates at the next epoch boundary; from then on
# 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:

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.

🟡 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)

# tar czf dashmate-home.tgz -C ~ .dashmate
# for v in $(docker volume ls --format '{{.Name}}' | grep '^dashmate_'); do
# docker run --rm -v "$v":/vol -v "$PWD":/b alpine tar czf "/b/$v.tgz" -C /vol .
# done
set -e
DASHMATE_V4="${DASHMATE_V4:-dashmate@4.0.0}"

echo ">> Stopping v3 group"
npx -y dashmate@3.0.2 group stop --force || true

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.

🟢 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)


echo ">> Starting group with ${DASHMATE_V4} (migrates config + images)"
npx -y "$DASHMATE_V4" group start --verbose

echo ">> Waiting for protocol 12 to activate (next epoch boundary)"
until curl -s --max-time 3 http://127.0.0.1:46657/block \
| python3 -c "import json,sys; sys.exit(0 if json.load(sys.stdin)['block']['header']['version']['app']=='12' else 1)" 2>/dev/null; do
sleep 30
curl -s http://127.0.0.1:46657/block | python3 -c "import json,sys; h=json.load(sys.stdin)['block']['header']; print('height', h['height'], 'active', h['version']['app'], 'proposed', h.get('proposed_protocol_version'))" 2>/dev/null || true
done
Comment thread
lklimek marked this conversation as resolved.
Resolved
echo ">> Network is now on protocol 12 (v4). Proofs are V1."
88 changes: 88 additions & 0 deletions docs/testing/masternode-identity-upgrade/harness/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
# Harness — masternode-identity upgrade test

Reproducible scripts for the test described in [`../SPEC.md`](../SPEC.md). They
regenerate everything locally; **no secrets or binaries are committed** (see
`.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

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.

🟢 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_seed

The 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)


- Docker running; `npx` (Node) available.
- A checkout of `dashpay/platform` (for DAPI `.proto` files) — default path
`~/Projects/dashpay/platform`.
- `grpcurl` (for `query_identity_dapi.sh`).
- Two DET builds: `v0.9.3` and the PR branch, each run against an **isolated

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.

🟡 Harness is macOS-only in practice but never says so, and omits DASH_EVO_DATA_DIR for the new DET

(Partially addressed in 7279666c — the encrypted-wallet sqlite3 check now documents both the macOS and Linux paths. Two coupled issues remain.)

(1) The macOS assumption is still unstated here. Lines 14-18 tell the operator DET "derives its data dir from $HOME (~/Library/Application Support/Dash-Evo-Tool on macOS)" — the only path given in Prerequisites. This repo targets Linux (x86_64/aarch64), Windows and macOS, and CI is Linux; on Linux the path is ~/.config/dash-evo-tool (src/app_dir.rs). Prerequisites list Docker and npx but never state "macOS only", so a Linux developer reads a $HOME-override rationale written against a path their build does not use.

(2) DASH_EVO_DATA_DIR is never mentioned. The HOME-override advice is correct and justified for the 0.9.3 baseline — that build predates DASH_EVO_DATA_DIR, so HOME really is the only lever. But the new DET supports DASH_EVO_DATA_DIR directly, which is what docs/gui-testing/ safety rule 1 calls for, and it is the more precise instrument (isolates the data dir without relocating everything else keyed off $HOME). The reasoning behind the current advice is sound; it just is not written down — and undocumented good judgement gets refactored away by the next person.

Recommendation: State the macOS assumption in Prerequisites, or parameterize the profile path (PROFILE_DB="${PROFILE_DB:-...}") and give the Linux equivalent alongside. Separately, note why the HOME override is required for 0.9.3, and use DASH_EVO_DATA_DIR for the new-DET steps.

🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (security-engineer-smythe + project-reviewer-adams + qa-engineer-marvin + Codex Sol)

profile** via `HOME` override, e.g.
`HOME=/path/to/profile /path/to/dash-evo-tool` — DET derives its data dir from
`$HOME` (`~/Library/Application Support/Dash-Evo-Tool` on macOS), so this keeps
the test off your real profile. Snapshot the profile dir to back it up.

## Order of operations

| Step | Script / action | Phase |
|------|-----------------|-------|
| 1 | `01_setup_v3_network.sh` — dashmate v3 local net, protocol 11 | 0 |
| 2 | `02_extract_masternode_keys.py` — writes `keys.json` (gitignored) | 0 |
| 3 | *DET 0.9.3*: Load Identity (Evonode) with a proTxHash + its owner/voting/payout WIFs from `keys.json`; create wallet; fund from Core; top up; withdraw to L1. Snapshot the profile. | 1 |
| 4 | `04_upgrade_to_v4.sh` — in-place v3->v4, waits for protocol 12 | 2 |
Comment thread
lklimek marked this conversation as resolved.
Resolved
| 5 | `p2p_proxy.py` — **required for the new DET** (see gotcha) | 3 |
| 6 | *New DET (PR)*: run on a copy of the 0.9.3 profile; confirm the identity migrated (Masternodes screen). | 3 |
| 7 | `query_identity_dapi.sh <proTxHash>` — on-chain liveness/balance | 3 |
| 8 | *New DET*: Masternodes -> identity -> Owner key / Payout key -> Sign Message; copy the signature; verify with `verify_signed_message.py` | 3 |

## Key-functionality check (the local substitute for a withdrawal)

In the new DET, open a migrated key (Owner or Payout), type a known message,
click **Sign Message**, copy the Base64 signature, then:

```
python3 verify_signed_message.py <key_address> <base64_signature> "<message>"
```

Exit 0 / `MATCH` means the migrated private key still produces a valid signature
for its address. For a byte-for-byte cross-check, `dashd signmessage <address>
"<message>"` (the address is in the local_seed `main` wallet) yields the
identical signature — deterministic RFC6979 — proving DET holds the same key.
Comment thread
lklimek marked this conversation as resolved.
Resolved

## Additional check — encrypted wallet survives the upgrade

Verifies a password-protected wallet created in 0.9.3 still unlocks in the new
DET (see SPEC "Additional scenario"). No network is needed for the wallet itself;
run it alongside or independently of the identity flow.

1. **In DET 0.9.3** — Wallets -> **Create Wallet**. Hover the entropy grid, click
**Generate**, tick **"I wrote it down"**, give it a **name** and a **strong
password** (the strength meter must read at least "Strong"), then **Save
Wallet**. Record the name + password.
2. **Confirm it's encrypted at the storage layer** (profile still on 0.9.3):
```
sqlite3 "<profile>/Library/Application Support/Dash-Evo-Tool/data.db" \
"SELECT alias, uses_password FROM wallet;"
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
```
The new wallet must show `uses_password = 1` (a no-password wallet shows `0`).
Comment thread
lklimek marked this conversation as resolved.
Resolved
3. **Snapshot** the profile dir, then **run the new DET on a copy** of it. Choose
*Just Explore*, dismiss the sync modal (*Continue in the background*), open
**Wallets**, and select the wallet.
4. **Unlock it** — click **Unlock**, enter the original password, confirm. Pass =
the wallet is present and the **Unlock button flips to Lock** (authenticated
AES-256-GCM decryption succeeded, so the password + seed survived migration).
A wrong password shows an error and the button stays **Unlock**.

Note: address rows may stay empty while SPV is unsynced (the dip0024 limitation
below) — that does not affect the unlock result.

## Gotchas

- **P2P port proxy (step 5).** The PR's SPV client hardcodes the regtest Core P2P
port `19899`, but dashmate's `local_1` Core listens on `20001`. Run
`python3 p2p_proxy.py` (bridges `127.0.0.1:19899 -> 20001`) before starting the
new DET, or its SPV client can't connect at all.
- **dip0024 / withdrawal.** See SPEC "Known limitation" — the new DET's SPV
verifier needs an `llmq_test_dip0024` rotation quorum that a 3-MN local net
can't form, so proof-verified ops (including withdrawal) fail locally with
"servers unreachable". The Sign Message check above is the workaround.
- **Signing a *fresh* message.** Always sign a message you control and verify
against *that* message. Recovering a stale signature against the wrong message
yields a valid-but-wrong address and looks like a failure.
- **Text entry on non-US keyboard layouts.** Automated typing tools may fail to
map characters; paste via the clipboard instead.
Loading