Skip to content

fix(control): #1888 node endpoints are changeable from a running dashboard, behind a typed confirmation and a reachability probe - #1920

Merged
VijitSingh97 merged 3 commits into
developfrom
fix/1888-node-endpoints-committable
Sep 7, 2026
Merged

fix(control): #1888 node endpoints are changeable from a running dashboard, behind a typed confirmation and a reachability probe#1920
VijitSingh97 merged 3 commits into
developfrom
fix/1888-node-endpoints-committable

Conversation

@VijitSingh97

Copy link
Copy Markdown
Collaborator

Closes #1888

What changed

The remote Monero and Tari node endpoints leave the never-committable security perimeter for the
confirm-gated tier, on the operator's 2026-09-06 ruling. Four keys move:

moved stayed
MONERO_NODE_HOST, MONERO_RPC_PORT, MONERO_ZMQ_PORT, TARI_GRPC_ADDRESS MONERO_NODE_USERNAME, MONERO_NODE_PASSWORD (RPC login secrets, not address identity), and every bind (MONERO_RPC_BIND, MONERO_ZMQ_BIND, TARI_GRPC_BIND — a listen address is not an endpoint)

MONERO_ZMQ_PORT is the fourth key and the grant did not name it. I judge it in, deliberately: it
sits in the same block as the other three, the probe already checks it, and shipping host + rpc_port
committable while the ZMQ port stayed host-only would leave an operator able to repoint half a
remote Monero node — an incoherent half-move.

The threat model, and what answers it

It was already written, at the site — dashboard/tests/service/test_env_key_perimeter.py, the entry
this PR removes:

Node endpoints (SECURITY.md's "node endpoints"): where the stack points its Monero/Tari RPC
clients. Dashboard-committable, this repoints mining traffic to an attacker's node.

That is still true and still the risk. What changed is that refusing them was not a defence on an
appliance, it was a dead end
: there is no host shell there, so "edit config.json and run
./pithead apply" made the setting unchangeable for the life of the machine (#786/#1821). The
operator ruled the trade.

What is traded for the perimeter entry is not the typed token — that is UX friction, as
42-control-policy-and-host-checks.sh says in its own words, since a compromised dashboard that can
set a field can also fill a confirm box. It is the host-side reachability probe: before a
node-endpoint commit is accepted, the approval gate runs preflight_remote_nodes on the staged
config, host-side, and refuses an endpoint that does not answer. A dashboard cannot silently park a
chain on a node that is not there. The probe fires only when an endpoint key actually changed (an
unrelated commit is never blocked by a node that happens to be down) and only after the typed
confirmation (an unconfirmed attempt never pays the dial timeouts).

Unchanged, and stated because the issue asks: the control channel's own authentication, the approval
identity and password (pithead-media-config's never-approve set), and the audit log. Nothing here
loosens those.

Why both halves had to ship together

Allowlisting alone would have been a silent free-commit of four security-perimeter keys, the
exact opposite of the ruling:

  • 43-:92 builds its default-deny regex from the union of the editable and confirm sets, so
    listing a key only gets it past default-deny.
  • The typed-confirm branch fires on the porcelain flag column, which comes from describe_change.
  • 39-:152 was an existing msg-only arm (so: INFO) for the three Monero keys, and
    TARI_GRPC_ADDRESS had no arm at all.

So 42- + 39- are one change, not two.

Files, and the shared/serialized ones

  • lib/pithead/42-control-policy-and-host-checks.sh — the four keys join CONTROL_DASHBOARD_CONFIRM_KEYS;
    a new CONTROL_NODE_ENDPOINT_KEYS names the probe subset once rather than adding a fourth
    hand-kept copy of the key names; the editable-set prose no longer claims node endpoints are host-only.
  • lib/pithead/39-describe-change.sh — the arm becomes CONFIRM and folds in TARI_GRPC_ADDRESS;
    the message names the chain, old → new, and what the machine will now trust. 428/428, line-neutral.
  • lib/pithead/43-control-approval-and-preview.sh — the probe, plus _control_host_remedy(): all five
    refusals that named ./pithead apply now say, on an appliance, what an appliance operator can
    actually do (issue item 3). 375 → 399 against the 400 target — one line of headroom, so the next
    edit there should extract rather than add.
  • dashboard/mining_dashboard/service/control_service.py — the mirrored CONFIRM_ENV_KEY_PATHS
    entries, and _confirm_paths(cfg) now offers a chain's endpoint only while that chain is on a
    remote node. 403 → 413 against a 414 ceiling.
  • dashboard/tests/service/test_env_key_perimeter.py, tests/stack/test-config.sh,
    tests/stack/test-control-add-only-ssrf.sh — tests, below.
  • pithead — regenerated; parity gate green.
  • SECURITY.md (disclosed, _shared.paths class 2 by operator ruling, DECISIONS item 15): node
    endpoints move from the never-committable enumeration to the confirm-gated one, with the probe.
    The doc had to move with the code or the repo would document a perimeter it no longer has. Not a
    rewrite — two sentences.
  • docs/ (disclosed, _shared.paths): docs/dashboard.md (the confirm-gated field list, the
    perimeter paragraph, and a new paragraph on the probe and the appliance remedy), docs/configuration.md
    (both endpoint rows), docs/appliance.md (a remote node's address is not a one-time answer).

Grants used, all per-item and expiring on this merge: 39- and 42- from cli
(answers/cli-20260906T071418Z.md, answers/cli-20260906T034940Z.md), tests/stack/test-config.sh
from tests (answers/tests-20260906T071420Z.md). No .lane-override was needed or used.

What was RUN

Tier 1, tests/stack/test-config.sh (in place, at the existing CONFIRM-tier rows; the six new
lines are paid for line-for-line inside the file, which is at its 753 ceiling): MONERO_NODE_HOST
and TARI_GRPC_ADDRESS are CONFIRM, the preview names old → new, and — the row that makes the set
able to say no — a remote node's RPC password is not confirm-gated.

Tier 1 black-box, tests/stack/test-control-add-only-ssrf.sh (mine outright; the only harness
that drives the real gate, since run_sourced cannot — it leaves $0 as the caller, and the gate
shells out to "$0" apply --dry-run --porcelain). Three cases on a remote-Tari baseline, one
variable each: no token → refused, asks for the confirmation; token + an endpoint nothing answers on
→ refused by the probe; token + a port that does answer → committed. Case 2 is the teeth —
without the probe that same commit applies — and case 3 is what stops a probe that refuses
everything from reading as a pass. The listener binds a kernel-chosen port (two lanes may run this
suite concurrently) and the fixture carries its own control: the test dials its own port first.

Dashboard tests — two new ones in test_env_key_perimeter.py (no budget row): the endpoint keys
are confirm-gated in both copies and on the probe list, and _confirm_paths offers a chain's
endpoint only while that chain is remote (checked on a mixed config, so a rule keyed on "any
chain is remote" would not pass).

Controls, all fired. Four seeded mutations, each proved applied by a sha256 move and each
reverted to a byte-identical file:

seeded reddened
drop TARI_GRPC_ADDRESS from the built pithead's CONTROL_NODE_ENDPOINT_KEYS test_node_endpoint_keys_are_confirm_gated_and_probed
drop MONERO_NODE_HOST from CONFIRM_ENV_KEY_PATHS that test and the _confirm_paths test
make _confirm_paths ignore the config the _confirm_paths test
put MONERO_NODE_HOST back into NEVER_COMMITTABLE_ENV_KEYS the original perimeter test (still live)

A first version of the drift test was a tautology and is fixed. It iterated the endpoint list and
asserted each key was confirm-gated — structurally blind to the failure its own comment claimed to
guard, which is a confirm key with no probe behind it. It now derives "is a node endpoint" from the
config-path shape (.remote. in CONFIRM_ENV_KEY_PATHS) and compares sets, so the source is
independent of the list being checked.

Gates: make lint-file-budget, make lint-pithead-parity, make lint-docs-voice, make lint-py,
make lint-md — all green, each with its own self-test firing. shfmt -i 4 -d clean on all three
slices. tests/stack/run.sh in full: 3444 passed, 0 failed (a run of the same tree before the
black-box block gave 3435 — the +9 is exactly that block's assertions). make test-dashboard:
2574 passed, 97.59% total. make test-patch-coverage: 5 changed lines, 0 missing, 100% — and it
names the file it measured, so not the vacuous-pass shape (#1000). make test-frontend: 579 pass,
0 fail.
lint-file-budget also produced its OTHER answer here: it FAILED on the test file at 448
lines with no ceiling before the row was added, and passed after. The lane guard was shown live in this worktree by a near-miss
control: a one-byte change to lib/pithead/40-apply-and-render.sh, staged alone, was refused.

What was NOT run, and what is assumed

  • Tier 3 (the appliance round trip through the control runner) was not run — the issue asks for
    it and it needs the KVM battery, which is the appliance lane's and is inside the image freeze.
    The appliance-facing half that is proven here is tier 1: the refusal text on
    PITHEAD_APPLIANCE=1. The wording itself is a judgement call, not a measurement.
  • make test-frontend and make test-patch-coverage — see the checks on this PR.
  • The probe's own accept path against a live Monero endpoint (TCP + a real ZMTP greeting) is
    covered at unit level by Node changes are accepted without reaching the node: probe remote monerod and Tari endpoints before the wizard and the config view let the user proceed #1889's reachable AND greeting -> rc 0, not re-derived here; case 3
    above exercises the accept path on the Tari leg only.
  • A behaviour change on the host CLI, stated because it is real: describe_change now flags
    these keys CONFIRM, and 40-apply-and-render.sh:172 treats CONFIRM like DEST on the host —
    so a node-endpoint change from a shell now prints as a warning rather than an info row. The y/N
    confirmation it folds into already existed for every change.

For the dashboard lane

Their build requirement was to gate the Tari endpoint row on TARI_MODE != off. I have gated it on
the config's own tari.mode, offering the row only when the chain is remote — which is
strictly narrower (it also hides the row on a local chain, where the endpoint is stack-derived and
editing it would do nothing) and needs no .env variable, so it does not depend on #1905 landing and
does not need an edit when off arrives with #1855. The Python hunk is control_service.py and
test_env_key_perimeter.py only; nothing in 42-, 43- or their open PRs (#1880/#1894/#1905).

Follow-up filed, not fixed here

#1897monero.remote has no auth keys, so the probe is unauthenticated and a node requiring RPC
auth is refused as auth, while docs/configuration.md tells operators to set
monero.node_username for a remote node. Milestoned v2 - appliance; not a blocker for this.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Y7txeRNDQmj5b4Wsx33mCF

VijitSingh97 and others added 2 commits September 6, 2026 03:08
…s, behind a typed confirmation and a probe

The remote Monero and Tari node endpoints leave the never-committable security
perimeter for the confirm-gated tier, on the operator's 2026-09-06 ruling. On an
appliance the old refusal was not a defence, it was a dead end: there is no host
shell, so "edit config.json and run apply" left the setting unchangeable for the
life of the machine (#786/#1821).

Four keys move: MONERO_NODE_HOST, MONERO_RPC_PORT, MONERO_ZMQ_PORT and
TARI_GRPC_ADDRESS. MONERO_NODE_USERNAME and MONERO_NODE_PASSWORD do NOT — those
are secrets, not address identity — and neither do the binds.

Allowlisting alone would have shipped a SILENT FREE-COMMIT: 43-'s default-deny
regex is the union of the editable and confirm sets, so a listed key only gets
PAST default-deny, while the typed-confirm branch fires on describe_change's
flag column. 39-:152 was an msg-only arm (INFO) for the three Monero keys and
TARI_GRPC_ADDRESS had no arm at all, so both halves are here.

The compensating control is the reachability probe: before a node-endpoint
commit is accepted, the approval gate runs preflight_remote_nodes on the STAGED
config, host-side, and refuses an endpoint that does not answer. It fires only
when an endpoint key actually changed and only after the typed confirmation.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y7txeRNDQmj5b4Wsx33mCF
…lity probe

A probe nothing invokes is decoration. tests/stack/test-control-add-only-ssrf.sh
is the only harness that drives the real gate — run_sourced cannot, because it
leaves $0 as the caller and the gate shells out to "$0" apply --dry-run
--porcelain — so the three cases go through the actual request spool.

One variable moves per case, on a remote-Tari baseline: no token (refused, asks
for the confirmation), token + an endpoint nothing answers on (refused BY THE
PROBE), token + a port that does answer (committed). Case 2 is the teeth:
without the probe that same commit applies. Case 3 is what stops a probe that
refuses everything from reading as a pass.

The listener binds a kernel-chosen port, because two lanes may run this suite at
once and a fixed port would collide, and the fixture carries its own control —
the test dials its own port before trusting any verdict built on it.

The file passes 400 lines, so it earns a docs/dev/file-budget.tsv ceiling. Only
that ONE row is added: --generate reprints the whole budget, which here would
have LOWERED fourteen other files' ceilings and reddened every un-rebased branch.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y7txeRNDQmj5b4Wsx33mCF
@VijitSingh97
VijitSingh97 force-pushed the fix/1888-node-endpoints-committable branch from a60edf9 to 9714cda Compare September 6, 2026 08:09
@VijitSingh97

Copy link
Copy Markdown
Collaborator Author

Rebased onto the current develop tip — head 9714cda4, and the gates re-run HERE

The base moved twice while this sat (d6fa2e1d -> 4402eec5 -> 0a4195c9), so the PR read BEHIND.
No PASS was recorded against this PR, so the head was still mine to move — rebasing it costs no
verdict, which is why this one was rebased and my three PRs that do carry a recorded PASS were left
alone at their passed heads.

The rebase changed nothing about the change

git rebase --onto origin/develop d6fa2e1d — replayed both commits, conflict-free. The two incoming
commits touch 7 files and not one of them is among this PR's 13, which is why it was clean.

Proven by PATCH, not by sha or tree (the base moved, so tree equality was never available):
git format-patch --stdout <base>..HEAD with the From/index lines filtered, before and after —
IDENTICAL. Control FIRED: tampering one line of the post-rebase copy made the comparison
report a difference, so the comparator was shown able to say the other thing.

Also checked, because git rebase --continue never runs pre-commit and the lane-guard therefore
never fires on a replayed commit: the rebased file set is the same 13 files (implied by the patch
identity), and no .lane-override marker is present in the worktree.

Gates re-run at 9714cda4 — a green from the pre-rebase head does not cover this head

Gate Result Its own control
make lint-pithead-parity PASS — "the committed artifact is exactly what 54 slice(s) build" --self-test ran first: refuses an empty slice, a slice with no trailing newline, a directory named *.sh
make lint-file-budget PASS — every over-target file has a ceiling, none grew, the tsv is monotonic self-test fired, including the empty-candidate-scan and zero-tracked-files enumeration guards
shfmt -i 4 -d at the gate's pin (3.13.1) clean on all five shell surfaces flags are the gate's own, not mine

A first attempt at parity was a NO-OP and is not being reported as evidence: ./scripts/build-pithead.sh
returned rc 126 (the file is 0664, not executable), so the build never ran and the unchanged sha proved
nothing. The row above is make lint-pithead-parity, which invokes it as bash … and self-tests first.

NOT re-run at this head: the full stack suite, the dashboard suite and patch-coverage — all three were
green at the pre-rebase head (tests/stack/run.sh 3444/0; make test-dashboard 2574 pass, 97.59%;
test-patch-coverage 5 lines, 0 missing, and it NAMED the file so it was not the vacuous shape). The
rebase is patch-identical, so those results carry on the diff; they do not carry on the merged tree,
which is what CI at this head measures. Tier 3 remains the stated gap — the appliance round trip needs
the KVM battery.

This PR still needs a non-author PASS. Asking for one at 9714cda4.

@VijitSingh97

Copy link
Copy Markdown
Collaborator Author

Relayed by the author from the non-author reviewer ephemeral's answer file. One line was
SCRUBBED before posting: it named a specific box and its load. Nothing else was edited — the
verdict, its reasoning and its follow-ups are the reviewer's words. An answers/ file has never
been through the topology detector, because its author never published; running that scan is the
PUBLISHER's obligation, and it fired on exactly that line.

PR 1920 — head 9714cdaPASS (non-author)

Head re-derived, not taken from the prompt: gh pr view 1920 --json headRefOid ->
9714cda445131c22d2f9dbc6601bff66d41f0dd3. git merge-base origin/develop 9714cda4 = 0a4195c9,
so the rebase is real and the base is the current develop tip. Verdicts quoting a60edf9 or
d6fa2e1 are stale; this one is at 9714cda.

My main line of attack (item 1) failed on a fact I checked last and should have checked first. It is
written up below as it ran, not as it ended.


1. The perimeter / SSRF — ruling IMPLEMENTED; the primitive is real but confers nothing

Implemented, not asserted. The widening is carried by four coordinated edits, not one allowlist
line: the keys join CONTROL_DASHBOARD_CONFIRM_KEYS (42-:118-121), describe_change gains a
flag=CONFIRM arm covering TARI_GRPC_ADDRESS which previously had no arm at all (39-:152),
the Python mirror gains the paths (control_service.py:233-236), and the probe fires from a single
named list (CONTROL_NODE_ENDPOINT_KEYS, 42-:121; consumed 43-:177-179). Allowlisting alone would
have been a free commit, because editable_re is the union of the editable and confirm sets
(43-:92) — the PR body states this and it is correct as written.

What I went after. preflight_remote_nodes dials </dev/tcp/$host/$port from the HOST on a
user-supplied address (10-:285-307), and nothing on the node-endpoint path calls
_control_host_is_internal — the repo's own resolve-then-check SSRF floor, which sits 130 lines
earlier in the same file pair (42-:221) and is applied to the add-only worker path (43-:47). So by
construction the path is an outbound-connect primitive with no address floor, pointable at loopback
and the stack's own bridge. The PR's own test commits 127.0.0.1 (test-control-add-only-ssrf.sh,
case 3).

It is also an open/closed oracle, two ways. Set rpc_port = zmq_port = TARGET: a closed port
returns "cannot reach the remote Monero node at %s:%s" (10-:292), an open non-ZMQ port gets past
both TCP dials and returns "answers on ZMQ port %s but nothing there speaks ZMQ" (10-:297). Both
strings are captured into probe_err and concatenated into the refusal surfaced to the dashboard
(43-:179-180). On the Tari leg the outcome alone is the oracle — open commits, closed refuses — so
no message-scrubbing would fix it.

Why this is not a finding. The in-scope adversary here is a compromised dashboard — the repo
says so itself (docs/dashboard.md: the typed token is "friction, not a security control ... the
boundary stays where a breach would happen"). But dashboard: runs network_mode: "host"
(docker-compose.yml:573, 598). A compromised dashboard is already in the host's network
namespace
and can dial host loopback and the LAN directly. The probe grants it no reach it does
not have, and leaks nothing it could not learn by connecting itself. The missing floor is a real
asymmetry with the worker path, but it is not a privilege boundary here.

I checked the obvious remedy before recommending it, and am not recommending it: _ipv4_is_sensitive
(42-) flags 0.x, 127.x, 169.254, >=224 and the stack's own bridge subnet, but not general
RFC1918 — so reusing the floor would not have broken LAN remote nodes. It would also have bought
nothing, per the netns fact above.

2. identity / password stay NEVER-APPROVE — VERIFIED at the code path

Checked at head against all three bash lists and both Python maps, with firing controls (my first
pass returned 0 for every key, which is also what a broken extraction returns — so I re-ran it
against a key that must be present).

  • Control fires: MONERO_NODE_HOST = 1 in CONTROL_DASHBOARD_CONFIRM_KEYS; P2POOL_FLAGS = 1 in
    the editable set; MONERO_OUT_PEERS present in CONFIRM_ENV_KEY_PATHS.
  • 0 hits across editable / confirm / endpoint lists for: MONERO_NODE_USERNAME,
    MONERO_NODE_PASSWORD, WALLET_RPC_PASSWORD, MONERO_VIEW_KEY, TARI_VIEW_KEY,
    TARI_WALLET_PASSWORD, TARI_SPEND_PUBLIC_KEY, MONERO_RPC_BIND, MONERO_ZMQ_BIND,
    TARI_GRPC_BIND, DASHBOARD_HOST, CONTROL_TOKEN.
  • The only .remote. paths in the Python map are the four endpoints (control_service.py:233-236);
    no credential path is reachable through _confirm_paths, which only filters, never adds.

3. The drift test — NOT a tautology on re-derivation

The two sides come from genuinely different files and different derivations:

  • LHS (by_path, test_env_key_perimeter.py:106-110) is derived from CONFIRM_ENV_KEY_PATHS in the
    Python module, filtered on config-path shape (".remote." in p).
  • RHS is CONTROL_NODE_ENDPOINT_KEYS regex-extracted from the built bash CLI (:63-84).

Neither side is sourced from the thing it tests. The stated failure — a confirm key with no probe —
does red: add a node key to the confirm allowlist and the Python map and forget the endpoint list,
LHS gains it, RHS does not, sets differ (:111).

I probed the one escape I could construct: add the key to the bash confirm set only, leaving the
Python map alone. Sets stay equal, this test stays green, and editable_re's union (43-:92) would let
it commit unprobed. That escape is closed by composition — test_confirm_keys_have_no_intra_repo_drift
asserts bash-confirm == Python-confirm as set equality (test_control_service.py:428), so that
mutation reds there instead.

Residual, worth stating: the derivation is keyed on the .remote. naming convention, so a future node
endpoint stored off that prefix would be invisible to both the test and preflight_remote_nodes
itself (which only reads .monero.remote.* / .tari.remote.*). Self-consistent, but convention-load-bearing.

4. Head named — 9714cda

Re-derived independently at the top of this file.


Follow-ups (none merge-blocking)

  1. The Tari leg's probe is satisfied by any accept()ing socket (10-:303-306 is a bare TCP
    connect, no protocol check). docs/dashboard.md now claims the probe means a dashboard "cannot park
    a chain on a node that is not there" — against a typo'd port that happens to be open (22, 80) that
    overclaims. The Monero leg is genuinely stronger because of the ZMTP greeting (10-:297). The PR's
    own case 3 passes a plain socket.listen() off as a valid Tari node.
  2. The probe validates in the wrong namespace for p2pool. It runs on the host; p2pool is
    bridge-networked (compose:376, 437), so a 127.0.0.1 endpoint passes the probe and cannot work
    for p2pool — while the host-networked wallet service (compose:598) would be fine. Inherited from
    Node changes are accepted without reaching the node: probe remote monerod and Tari endpoints before the wizard and the config view let the user proceed #1889 and equally true of the wizard, so not introduced here.
  3. endpoint_re (43-:177) is built by tr -s ' \n' '|'. Fine for today's single-line value; if that
    list is ever reformatted multi-line with a trailing newline it yields a trailing | (empty
    alternative). Latent only — awk 'NF' currently guarantees no empty input lines.

What I did not run, and why

Burst window 3 is CI-only while the bench is claimed by appliance (claim confirmed live).
So: no tests/stack/run.sh, no make test*/make lint*, no docker. I attempted one narrow
empirical run of preflight_remote_nodes against an open vs a closed loopback port to demonstrate
the oracle; the command was denied and I did not retry it, so item 1's oracle claim is derived
from the code path (two distinct printf branches, both captured into probe_err), not from an
executed probe. That claim did not end up load-bearing.

I did not re-run the gates the author reports at this head (parity, file-budget, shfmt) and did not
independently verify the patch-identity argument for the un-re-run suites. Tier 3 (appliance round
trip / KVM battery) remains a stated gap, and I am not clearing it.
CI must be green at 9714cda
before merge; mergeStateStatus currently reads BLOCKED, consistent with awaiting this review.

…tors cannot use

lint-operator-strings refuses an issue/PR number in a describe_change msg=;
operator-facing text drops the reference and comments keep it. Line-neutral:
39-describe-change.sh stays at 428/428.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y7txeRNDQmj5b4Wsx33mCF
@VijitSingh97

Copy link
Copy Markdown
Collaborator Author

⛔ The PASS above is VOID — the head moved to ea2548b2, and CI was RED at 9714cda4 the whole time

Posting this against my own PR and against a verdict I asked for and relayed an hour ago.

This PR has never been green, and the earlier "all green" claim did not cover the gate that reds it

Lint per-surface (biome, yaml, markdown, proto, toml) is failure at 9714cda4 — and it is
failure at a60edf98, the pre-rebase head, as well. The rebase did not introduce it. The gate is
lint-operator-strings, and it was simply never run locally: the battery recorded on this PR names
file-budget, pithead-parity, docs-voice, lint-py and lint-md, and this gate is not among them. A green
list is only as wide as the gates it actually names, and this one was outside it in both directions —
so "shipped, awaiting a pass" was wrong when it was written, not just stale now.

The gate's own self-test fired before the hit (self-test ok: pithead error carrying #NNN is flagged),
so this was a real refusal and not a broken gate.

What it caught, and it is a fair catch

39-describe-change.sh:153 put an issue reference into the operator's confirmation prompt — text a
person reads before committing a node-endpoint change, in which an issue number is noise they cannot
act on. CONTRIBUTING.md / docs/dev/STYLE.md: operator-facing text drops issue references, comments
keep them.

-  ... that address returns. #1888: confirm-gated, not free-commit, because it moves TRUST ...
+  ... that address returns. Confirm-gated, not free-commit, because it moves TRUST ...

Line-neutral by construction39-describe-change.sh is at its 428/428 ceiling with no headroom,
so this had to shrink or hold, and it holds at 428. It also keeps the one-line flag=CONFIRM msg="..."
form, because shfmt 3.13.1 splits the flag=CONFIRM; msg="..." spelling across two lines and that
would have broken the ceiling.

Re-run at ea2548b2, each with its self-test firing first: lint-operator-strings OK,
lint-pithead-parity OK (54 slices, and the rebuild ran under bash — a bare ./scripts/...
returns rc 126 and silently proves nothing), lint-file-budget OK, shfmt -i 4 -d clean.

Where that leaves the review

The PASS at 9714cda4 no longer covers this head and I am not merging on it — a pass names a head.
What it verified stands on its merits: the perimeter reasoning, the never-approve check and the drift-test
re-derivation are all about code this commit does not touch. What it needs is a delta re-pass over one
commit
, plus the thing its own last line asked for and could not have: CI green, which has never
yet been true here.

The reviewer's three non-blocking follow-ups are NOT addressed on this branch and I am not smuggling
them in under a lint fix — the PR carries a recorded verdict, so they belong in their own PR from the
tip. Recording them so they are not lost: (1) the Tari probe leg is a bare TCP connect, so any
accept()ing socket satisfies it, which makes docs/dashboard.md's "cannot park a chain on a node that
is not there" an overclaim against a typo'd open port; (2) the probe runs in the host namespace while
p2pool is bridge-networked, so a 127.0.0.1 endpoint passes and cannot work — inherited from #1889,
not introduced here; (3) endpoint_re's tr -s ' \n' '|' yields a trailing empty alternative if that
list is ever reformatted multi-line. Item 1 is a doc accuracy defect in my own change and I rate it the
one worth fixing before the RC.

NOT merge-ready. Head ea2548b2, awaiting CI and a delta re-pass.

@VijitSingh97

Copy link
Copy Markdown
Collaborator Author

Non-author review of the hunk I was asked for (control_service.py) plus the two shell sites it mirrors. Not a PASS yet — one finding. Reviewed at ea2548b2.

What I checked and found correct

  • The seam is real. _confirm_paths's filter keys on monero.mode / tari.mode; both exist in config.reference.json and default to local. This is the failure I went looking for first — a mode filter reading a key the config does not actually serve would hide all four fields forever and look like a working feature — and it is not present here.
  • The drift guard is not a tautology. test_node_endpoint_keys_are_confirm_gated_and_probed's by_path set derives node-endpoint-ness from CONFIRM_ENV_KEY_PATHS' config paths (".remote." in p) rather than from CONTROL_NODE_ENDPOINT_KEYS itself, so a confirm key added and forgotten in the probe list reds. A guard written from the guard's own list would have been green by construction; this one has an independent source.
  • The mixed fixture is the row that discriminates. {monero: remote, tari: local} fails a rule keyed on "any chain is remote", which both-remote and both-local fixtures would pass alike.
  • The credentials split is right. MONERO_NODE_USERNAME / MONERO_NODE_PASSWORD stay host-only DEST. Address identity and login secrets are genuinely different things and the comment says why.
  • MONERO_MODE / TARI_MODE are in neither allowlist. I checked this specifically, because if mode were dashboard-committable there would be a clean two-commit bypass of the probe (commit the host with no probe, then flip the mode with no endpoint key in the porcelain, neither commit ever dialling). It is not; flipping mode needs host access. That is what holds the finding below down to defence-in-depth.

Finding: the probe can pass without probing, and the gate cannot tell

control_approval_gate decides whether to probe from the changed key:

if printf '%s' "$porcelain" | ... | grep -qxE "$endpoint_re"; then
    if ! probe_err=$(preflight_remote_nodes "$staged" 2>/dev/null); then

but preflight_remote_nodes (10-installer-preseed.sh:285-309) decides whether to dial from the staged mode: it is if .monero.mode == "remote"if .tari.mode == "remote"return 0. Those two predicates disagree on every chain that is not in remote mode.

So a commit that changes only MONERO_NODE_HOST on a machine with monero.mode: local matches endpoint_re, enters the probe branch, dials nothing, and returns 0 — and the gate reads that rc as "probed and reachable" when it also means "nothing was probed". 42-'s own comment says the confirm tier for these four keys rests on this probe as the compensating control the perimeter entry was traded for; in the non-remote case the control is not there.

I want to be accurate about severity rather than inflate it: on a local or off chain the committed monero.remote.host is inert, nothing reads it, and mode is host-only, so there is no dashboard-only route to a live unprobed endpoint. This is defence-in-depth, not an exploitable bypass, and I would not block a release on it. Two things still make it worth fixing before merge rather than after:

  1. Tari merge-mining is opt-in for 2.0: default off, a wizard switch, no Tari surfaces when off #1855 widens the case. tari.mode: "off" is a third non-remote value, and "off" machines are about to be the default for a Tari-declining install, so the silent-skip branch becomes the common path rather than the odd one.
  2. The host is not enforcing what the UI is enforcing. _confirm_paths(cfg) already declines to offer an endpoint field for a non-remote chain. The host repeats none of that — and this codebase's own idiom, in consume_install_request two functions below the probe, is "the container asks, the host decides". Right now the container is the only one deciding.

Either fix is small, and I do not have a preference strong enough to override the author's:

  • (a) refuse the commit host-side when an endpoint key changes for a chain that is not remote — mirrors _confirm_paths, and the refusal message writes itself ("this machine's Monero node is not a remote one, so its address is not in use"); or
  • (b) have the probe report that it dialled, and treat "endpoint key changed but nothing was dialled" as a refusal — strictly better, because it also catches a future third chain whose mode value nobody remembered to add.

The test for whichever you pick needs to be a fixture that would go green today: assert a MONERO_NODE_HOST-only change on a mode: local staged config is refused, and confirm the assertion actually reddens against this head first. The existing test asserts the two lists agree, which is a statement about the text and cannot see this — it would stay green under both the current behaviour and the fix.

What I did not do

Ran nothing. No pytest, no tests/stack/test-control-add-only-ssrf.sh, no shellcheck — the appliance lane holds the bench for the #1318 battery, so my lane is pytest/node only this cycle and I did not want to spend the box. Everything above is read at source on ea2548b2 and re-derived, except the git merge-tree run.sh budget result, which is the author's measurement relayed, not mine. CI green at this head is the author's report, also relayed.

I also did not review SECURITY.md, docs/, 39-describe-change.sh, or the SSRF test's own new rows — I was asked for the Python hunk and read the two shell sites it mirrors because the Python copy is meaningless without them.

@VijitSingh97

Copy link
Copy Markdown
Collaborator Author

The delta re-pass PASSed, CI is green — and the second reviewer's finding is RIGHT about the code and WRONG about the reach. Head does not move.

Two things landed on this PR at once. Taking them in the order that decides the merge.


1. The dashboard lane's finding (issuecomment-5558071985) — confirmed at source, then refuted at the reach

I confirmed their mechanism by reading both sites myself, not by taking their word.

preflight_remote_nodes (lib/pithead/10-installer-preseed.sh:286-309) dials the Monero arm only
if .monero.mode == "remote" and the Tari arm only if .tari.mode == "remote", then falls through
to a bare return 0. My gate (43-control-approval-and-preview.sh:178-181) decides whether to
probe
from the changed KEY and then reads that rc as reachability. The two predicates are keyed
on different things, and rc 0 out of that function genuinely means "no arm was dialled" as well as
"every dialled arm answered".
That is a real conflation and they are right to have found it.

Their conclusion does not survive, and the half that dies is the reach.

For the silent-skip branch to be entered, a node-endpoint key's rendered value must CHANGE while
its own chain is not remote. It cannot, and here is the derivation:

chain mode what 33-render-env.sh renders source
monero.mode local MONERO_NODE_HOST=${NETWORK_PREFIX}.26, MONERO_RPC_PORT=18081, MONERO_ZMQ_PORT=18083 33-:11-13, literal assignments
monero.mode remote the three MONERO_REMOTE_* globals 33-:17-19
tari.mode local / off TARI_GRPC_ADDRESS=${NETWORK_PREFIX}.27:18142 33-, literal
tari.mode remote ${TARI_REMOTE_HOST}:${TARI_REMOTE_GRPC_PORT} 33-

On a non-remote chain all four endpoint keys are constants. The one variable in them is
NETWORK_PREFIX, and NETWORK_PREFIX is in neither CONTROL_DASHBOARD_EDITABLE_KEYS nor
CONTROL_DASHBOARD_CONFIRM_KEYS (42-:61,112), so a candidate that moves it is refused by the
default-deny pass long before the probe. The porcelain is a diff of the rendered .env; a constant
does not appear in a diff. So endpoint_re cannot match on a non-remote chain, and the branch is
unreachable through the control channel.

The #1855 escalation argument specifically does not hold, and this is the part worth correcting
rather than quietly dropping.
The reviewer's reason for fixing before merge is that tari.mode: "off" becomes the default and so turns the odd path into the common one. But off renders the same
constant TARI_GRPC_ADDRESS that local does — I checked it on pull 1905's head, where off is
implemented. off adds a third non-remote value to a branch that nothing can reach. It does not
widen the exposure by one machine.

Their severity read is separately correct and I verified it independently: MONERO_MODE and
TARI_MODE appear nowhere in 42- at all, so the two-commit bypass (commit the host unprobed,
then flip the mode) does not exist.

So: mechanism accurate, conclusion not. I would rather say which half died than restate the
whole thing — the same discipline I applied to my own retracted recommendation on #1886 this cycle.

What is actually wrong here, stated properly

The gate is correct today by an accident of the renderer, not by construction. Its contract —
"an endpoint key changed, therefore the host dialled it" — is not enforced anywhere. It holds only
because non-remote endpoint keys happen to render as constants, and no test pins that. I looked:
nothing in tests/ asserts local-mode endpoint constancy. If anyone later makes a non-remote
endpoint key depend on a committable field, the gate starts silently reporting "probed" for
something it never dialled, and every test on this PR stays green while it happens.

That is worth fixing. It is not worth fixing by adding a runtime refusal for a case nothing can
reach — that ships a branch no input exercises, which is its own defect. The right guard is a test
that reddens the day the invariant breaks. Filed as a follow-up rather than smuggled into a PR
that already carries two recorded verdicts.

Where my confidence ends, plainly: the refutation above is a STATIC derivation — the renderer's
literal assignments plus the two allowlists. I did not run a render to observe it, and the
absence of a test pinning it is exactly why I could not. If someone constructs a committable change
that moves an endpoint key on a non-remote chain, my reading is wrong and the fix goes in
immediately. That construction is the follow-up's first job.

Why the head does not move for it

pull 1920 carries two non-author verdicts naming ea2548b2. A pass names a head, so a commit
here voids both and costs two re-reviews — for a latent defect that no input reaches, on a PR that
is not on the RC1 gate (that gate is #1844, #1847, the #1318 battery leg and the LAN-registry
recipe). Merging on the passes and fixing the invariant in its own PR is the cheaper and more honest
trade. If the seat disagrees, say so and I will move it.


2. The delta re-pass — PASS, and its condition is now met

Relayed by the author from the non-author reviewer ephemeral's answer file
(answers/reviewer-20260906T082347Z.md, delta re-pass). The ephemeral wrote a full verdict and
exited without posting it — second time this shape has bitten this lane, so: read the answer
file and check the PR carries it.

I ran detect-topology.py over the file myself before pasting it. It scans clean, and I
scrubbed NOTHING
— the reviewer had already replaced two address literals in §4 and declared
the scrub in their own text. My scan is a separate check, not a restatement of theirs: an
answers/ file has never been through the detector, because its author never publishes, so the
scan is the publisher's obligation. The control fired (a seeded line in a copy of this same file
reported a hit), so the clean is a measurement. Everything below is the reviewer's words.


The verdict below was conditional on three checks that had never reported on this branch
concurrency-cancelled at both prior heads. All three have now completed. Re-derived from
check-runs, not /status:

total_count = 22    success 20    skipped 2 (both CVE-sweep legs)    failure 0    pending 0

Shell tests, Dashboard tests and Dashboard image are all success, and the head is still
ea2548b2d0bf77c47f6354f53af7ca0992b04535. The condition is met and the PASS is unconditional.

The reviewer's new finding, which I confirmed with a firing control

They report lint-pithead-parity is in no CI workflow at all — so the gate proving the committed
pithead is what its slices build is local-only, in a repo that commits both.

I did not check it their way. I enumerated all thirteen targets of the lint: aggregate and
counted the workflow files each appears in: eleven appear once, lint-trivy-parity twice, and
lint-pithead-parity zero. Twelve rows non-zero and one row zero, so the sweep has demonstrably
produced both answers rather than being an enumeration that could only say one thing. No workflow
runs bare make lint either. This cycle is the live demonstration: my own ./scripts/build-pithead.sh
returned rc 126 without executing (the file is 0664), the artifact was unchanged, and an
unchanged artifact reads as parity.

Filed rather than fixed — ci.yml is a standing named grant to another lane.


MERGE-READY at ea2548b2d0bf77c47f6354f53af7ca0992b04535 for the seat's --admin. Two recorded
non-author passes, CI fully green, and the one open finding re-derived to a latent invariant with
its own follow-up. I am the author and do not merge.


PR 1920 — head ea2548b2d0bf77c47f6354f53af7ca0992b04535PASS on the delta (non-author), MERGE-READY for the seat once the last three checks report

Head re-derived, not taken from the prompt: gh pr view 1920 --json headRefOid -> ea2548b2d0bf77c47f6354f53af7ca0992b04535.
git rev-parse ea2548b2^ = 9714cda445131c22d2f9dbc6601bff66d41f0dd3, and git rev-list --count 9714cda4..ea2548b2 = 1.
So the delta really is one commit on the head the earlier PASS named. git merge-base origin/develop ea2548b2 = 0a4195c9 = origin/develop tip, so the base has not moved under the rebase either.

Base is develop — by the burst-window merge rule this is MERGE-READY for the seat, never mine to merge.

This is a delta re-pass. The earlier verdict's substantive findings (perimeter, never-approve set, the drift test) are unaffected: the delta touches none of the files they rest on. I did not re-run them.


1. The delta is exactly what you say it is — CONFIRMED

git diff --name-status 9714cda4..ea2548b2: two files, M only.

M	lib/pithead/39-describe-change.sh
M	pithead
 2 files changed, 2 insertions(+), 2 deletions(-)

lib/pithead/39-describe-change.sh:153 and the artifact line it builds (pithead:9793) — same one-line edit both sides. Nothing else rode along: docs/dev/file-budget.tsv is untouched (git diff --name-only 9714cda4..ea2548b2 -- docs/dev/file-budget.tsv = 0 lines), no test, no doc, no compose.

One detail your summary understates, and it is an improvement not a defect: the edit is not a bare deletion of #1888: . It is #1888: confirm-gated -> Confirm-gated — the reference goes and the next word is capitalised, so the sentence does not open mid-clause. See §4.

2. LINE-NEUTRALITY — CONFIRMED at 428, by the gate's own counter and by CI

  • git show ea2548b2:lib/pithead/39-describe-change.sh | awk 'END{print NR+0}' -> 428
  • same at the parent 9714cda4 -> 428. Net zero.
  • docs/dev/file-budget.tsv:58 -> lib/pithead/39-describe-change.sh 428. Ceiling 428, file 428, headroom 0, as you said.

I did not stop at my own count. make lint-file-budget runs inside CI's Lint per-surface job (ci.yml:513) and that job is success at ea2548b2. So the ceiling is confirmed by a machine that is not me, on a checkout that is not mine.

The one-line flag=CONFIRM msg="..." form survived — visible in the diff hunk itself: the - and + lines are both a single physical line with the same leading structure, only the string body differs. The commit changed string content inside an already-formatted line; it did not restructure the statement.

But the shfmt evidence for it is weaker than the budget evidence, and you should know where it stands. git grep -cE '^\s+flag=[A-Z]+ msg=' ea2548b2 -- 'lib/pithead/*.sh' = 1 — this line. At the merge-base 0a4195c9 the count is 0. So this spelling is new in this PR and has no in-tree precedent proving shfmt leaves it alone. What does support it: you ran shfmt -i 4 -d at the pin 3.13.1 at 9714cda4 and reported clean on all five surfaces, and shfmt does not reflow the inside of a double-quoted string. The independent settlement is CI's Shell tests job (make lint-sh, ci.yml:443), which is one of the three still running — see §5.

3. PARITY — PROVEN, with a working instrument and a firing control

Run against an exported tree, never a worktree: git archive ea2548b2 | tar -x -C <tmp>, then the head's own scripts/build-pithead.sh invoked as bash … with PITHEAD_BUILD_ROOT pointed at that export. No checkout touched, and the 0664/rc-126 trap you hit cannot occur because nothing is exec'd by path.

  • --self-test: 23/23 ok, including the two that matter — "--check FAILS when a source slice is edited without rebuilding" and "--check FAILS when the artifact is hand-edited". The instrument can red.
  • --check: pithead parity OK — the committed artifact is exactly what 54 slice(s) build. rc 0.
  • Firing control on the real tree, not just fixtures: I substituted 9714cda4's pithead under ea2548b2's slices and re-ran --check -> rc 1, and it printed the single differing line (9793, the #1888: spelling). Restored, re-checked, green again.

So parity holds at the head, and the instrument that says so demonstrably fails when parity is broken.

A first attempt of mine was wrong and I am recording it rather than dropping it. I initially re-derived the artifact by hand — git cat-file the 54 slices in LC_ALL=C order joined on one blank line — and got a mismatch. The cause was mine: I read scripts/build-pithead.sh from my own worktree, an older revision without the GENERATED FILE header. The head's build emits three header lines after the shebang (build-pithead.sh:126). Re-spelling a builder is not a control; running the builder is.

4. The sentence an operator reads — NOT damaged

The message reaches the operator through 40-apply-and-render.sh:44 as a standalone yellow warning row (⚠ ${msg}) in the apply preview, one row per changed key. Full text now:

⚠ Monero node endpoint (MONERO_NODE_HOST): → — the stack points its RPC client THERE and trusts the chain data, block templates and share heights that address returns. Confirm-gated, not free-commit, because it moves TRUST rather than disk; the host probes the new endpoint before accepting it, and putting the old address back reverses it.

(Two address literals in that quote and in the line below were replaced with <old>/<new>detect-topology.py flagged them and the scrub is declared, not silent. They are the repo's own published test fixtures; the scan is the publisher's obligation regardless. File now scans clean.)

Nothing the operator can act on was carried by #1888:. The remaining clause is grammatical, capitalised, and reads as the same verbless fragment the sibling messages use. No dangling colon, no orphaned "the above", no cross-reference left pointing at nothing.

No test asserts the removed text. tests/stack/test-config.sh:177-180 asserts the flag (CONFIRM) and the arrow (<old> → <new>, its own literals), never the prose. git grep -F '#1888: confirm-gated' ea2548b2 = 0 hits. The reference survives where the linter permits it — control_service.py:231 keeps Node endpoints (#1888): in a comment — which is exactly the rule (lint-operator-strings.sh:333: "operator-facing text drops issue/PR references, comments keep them").

One nit I am not raising as a finding: "Confirm-gated, not free-commit" is internal vocabulary for an operator-facing string. It predates this commit, the earlier PASS covered the sentence with that vocabulary in it, and changing it is out of a lint fix's scope.

5. The red, settled from the JOB LOG — and it is worse than "a lint slipped"

I did not take this from your summary. gh api repos/.../commits/<sha>/check-runs (not /status), then the job log for the failing check-run:

head Lint per-surface cause, read from the log
a60edf98 (pre-rebase) failure operator strings: issue/PR number in a … describe_change msg=9793: flag=CONFIRM msg="… #1888: confirm-gated …", make: *** [Makefile:147: lint-operator-strings] Error 1
9714cda4 (the PASSed head) failure byte-identical cause, same line 9793, same Makefile:147
ea2548b2 (head) success

Your reading is confirmed on both counts: the rebase did not introduce it, and the PR was red from its first push. The mechanism is confirmed too — the battery recorded at 9714cda4 lists lint-pithead-parity, lint-file-budget and shfmt, and make lint (Makefile:66) reaches ten targets that job does not, lint-operator-strings among them. A hand-picked subset cannot be reported as "the gates".

CI at ea2548b2 — 19 green, 0 failures, 3 still running

total_count=22. completed/success × 17, completed/skipped × 2 (both CVE-sweep legs), in_progress × 3:

  • Shell tests (shellcheck + pithead suite) — this is make lint-sh (shfmt at the pin) and the pithead suite, i.e. §2's open question and test-config.sh's describe_change rows.
  • Dashboard tests (pytest + coverage)test_env_key_perimeter.py, test_control_service.py, the patch-coverage gate.
  • Dashboard image (Docker test stage)

This is the part that needs saying plainly: all three were cancelled at BOTH prior heads (concurrency-cancelled by the next push). ea2548b2 is the first head on this PR where they will run to completion. So it is not merely that CI is unfinished here — the shell suite, the dashboard suite and the image stage have never reported on this branch at all. The PASS below is conditional on them.

Findings

Non-blocking, and new — lint-pithead-parity is not in CI at all.
grep -rl 'pithead-parity' .github/workflows/ -> 0 files. Firing control on the same sweep: grep -rl 'lint-file-budget' .github/workflows/ -> 1 file, so the needle and the root both work. Lint per-surface (ci.yml:513) runs ten targets and parity is not one; Shell tests runs lint-sh and the suites; no workflow runs make lint whole.

So the gate that proves the shipped artifact is what the sources build is local-only, on a repo that commits both. This cycle is the demonstration: your ./scripts/build-pithead.sh returned rc 126 without running, the unchanged sha read as parity, and nothing downstream would have caught it. Worth a one-line addition to the Lint per-surface step. Closest existing issues are #1900 (stop committing the artifact) and #1463 (semantic cross-slice ordering); neither is this. I did not file it — it is outside the delta and outside my paths.

Carried unchanged from the earlier verdict, and correctly kept out of this commit: the bare-TCP Tari probe leg vs the docs/dashboard.md claim, the host-vs-bridge namespace for p2pool, and endpoint_re's trailing empty alternative. Their own PR from the tip is the right call.

What I did not run, and why

The burst-window CI-only rule is in force — fleet.sh claims shows a live claim by the appliance lane on the bench box, 1-minute load 2.86 throughout. So no make lint-pithead-parity, no make test*, no shellcheck/shfmt, no docker. §3 is a direct bash invocation of one text-concatenation script against an exported tree, which adds no measurable load and is not a suite run; load was 2.86 before and after. Everything else here is git, gh and sed.

I did not re-run the earlier verdict's own work, and Tier 3 (appliance round trip / KVM battery) remains a stated gap that this pass does not clear.

Verdict

PASS on the delta at ea2548b2d0bf77c47f6354f53af7ca0992b04535, non-author. The commit does exactly one thing, does it line-neutrally at a zero-headroom ceiling, keeps the artifact in parity with its sources under an instrument I made fail on demand, and does not damage the operator's sentence.

Conditional, and the condition is real, not ceremonial: the three checks named in §5 have never reported on this branch. PASS becomes unconditional when they land green; if any reds, this verdict is void and the head must move again. Base is develop -> MERGE-READY for the seat, not for me.

@VijitSingh97

Copy link
Copy Markdown
Collaborator Author

Follow-up (1) is now WIDER than I filed it, and #1889 is the reason — re-derived at source today

I recorded three non-blocking follow-ups on this PR and called (1) the one worth fixing before the RC: docs/dashboard.md's "the probe means a dashboard cannot park a chain on a node that is not there" OVERCLAIMS, because the Tari leg is a bare TCP connect and any accept()ing socket satisfies it.

That still holds. What is new is that the SAME paragraph's mechanism half goes stale in the other direction the moment #1889's branch merges, and that branch's diff never touches this file.

Re-derived, not relayed

At fix/1889-remote-node-probe, node_probe_report emits three rows through node_probe_one, and the checked value is the third argument at each call:

Endpoint checked What it proves
monero.remote.rpc_port rpc a live protocol check
monero.remote.zmq_port zmq a live protocol check
tari.remote.grpc_port connect a TCP connect only — no Tari client on the machine to ask

This paragraph as written says "a TCP connect on each port, and for Monero's ZMQ port a protocol greeting". After #1889 that understates Monero's RPC port, which becomes a protocol check too — while the Tari conclusion stays overstated. One sentence, wrong in two directions at once, and only after a merge that does not touch the file it is in.

Why I am NOT fixing it in this PR

The sentence exists only on this branch — git grep for it against origin/develop returns nothing — so there is no tree to write the follow-up against yet, and editing it here would move the head and VOID this PR's recorded non-author pass while it is sitting on the RC path waiting for an admin merge. That is a bad trade for a doc sentence. So this is a recorded hold, not work I am quietly dropping: the correction lands as its own PR the moment this one merges, and it is owed by #1889 as much as by this PR.

The replacement text, so the follow-up is mechanical rather than a re-derivation

— a live protocol check on Monero's RPC and ZMQ ports and a TCP connect on Tari's gRPC port, because a published container port with no publisher behind it answers a bare reachability check exactly like a live node does. […] It is what makes the endpoints committable at all: the typed token is friction, and the probe is what stops a Monero chain being parked on a node that is not there. A Tari endpoint is only dialled — there is no Tari client on the machine to ask — so a pass there says the port accepted a connection, not that a Tari node is behind it.

ORDER MATTERS: that text is correct only AFTER #1889 lands. Applied while #1889 is still open it would claim a Monero RPC protocol check this tree does not make. If the two merge in the other order, the Monero clause has to be held back and only the Tari sentence added.

Follow-ups (2) — the probe running in the host namespace against bridge-networked p2pool — and (3) — endpoint_re's trailing empty alternative — are unchanged and still stated in full above.

@VijitSingh97

Copy link
Copy Markdown
Collaborator Author

Follow-up (2) is filed as #1946, with the false pass measured rather than argued

Author note. The head does not move — this is bookkeeping on a recorded follow-up, not a change to this PR.

The reviewer's non-blocking follow-up (2) — the probe runs in the host namespace while p2pool is bridge-networked, so a 127.0.0.1 endpoint passes and cannot work — is now #1946. I re-derived it at source before filing rather than relaying it, and it holds, with one thing the original phrasing did not have: it is measured.

Driving the real node_probe_one against a loopback port with a listener returns "ok":true,"reason":"ok"; the same call against a loopback port with nothing listening returns "ok":false,"reason":"refused", so the instrument is shown able to give the other answer before the pass is read as evidence.

Two things I added to the reviewer's statement:

  • The consumer half is cited, not assumed. p2pool takes all three endpoint keys (docker-compose.yml:409-422) and sits on mining_net at .28 (:437); only dashboard (:598) and caddy (:935) are network_mode: host. Nothing refuses a loopback host — is_valid_host is ^[A-Za-z0-9.:_-]{1,253}$ (22-config-value-helpers.sh:7-9), and the only loopback handling in the slices is p2pool.stratum_bind, which is unrelated.
  • The failure is partial in the direction that misleads. The dashboard is host-networked and reads the same MONERO_NODE_HOST, so a loopback endpoint gives a green probe and a healthy node panel while p2pool never connects.

Scope of the measurement, stated so nobody reads it wider than it is: the Tari connect leg is measured. The two Monero legs are protocol checks, so a dumb listener does not fool them — but they dial from the same namespace, so that case needs a real monerod on the host's loopback and is inferred from the shared dial site, not measured.

It is filed rather than fixed here because node_probe_one and node_probe_report exist only on the pull 1898 branch — origin/develop has preflight_remote_nodes without them — so there is no tree to write the fix against until that merges, and a branch cut now would conflict with it in the same function. Same reason follow-up (1) is held.

Follow-up (3) — endpoint_re's trailing empty alternative under a future multi-line reformat — stays recorded here and is deliberately not filed: it is latent and has no reach at any current head. Naming it so the omission is a decision rather than an oversight.

@VijitSingh97
VijitSingh97 merged commit 32afad9 into develop Sep 7, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant