Skip to content

fix(dashmate): add dedicated onion bind for Core 24 - #5342

Merged
shumkov merged 2 commits into
v5.0-devfrom
shumkov/core24-dashmate-onion
Oct 9, 2026
Merged

shumkov merged 2 commits into
v5.0-devfrom
shumkov/core24-dashmate-onion

Conversation

@shumkov

@shumkov shumkov commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

Issue being fixed or feature implemented

Dash Core 24 (v24.0.0-rc.3, dashpay/dash#7786) refuses to start when -bind is set and -listenonion is enabled unless a dedicated, non-wildcard =onion bind exists:

The automatic Tor onion service requires a dedicated onion bind. Use a specific address such as -bind=127.0.0.1:=onion, or disable the service with -listenonion=0.

Core only adds its default onion bind when no -bind is given, and Dashmate always renders bind=0.0.0.0. With core.tor.enabled=true, the rendered dash.conf has listenonion=1 and no onion bind, so Core 24 cannot start on any node that has Tor enabled.

This came up in the Core 24 × Platform 5.0.0-beta.2 devnet rehearsal. Tor was disabled there; that path renders listenonion=0 and works on Core 24.

What was done?

  • When Tor is enabled, the Core template renders bind=127.0.0.1=onion next to the existing bind=0.0.0.0.
    • The port is omitted on purpose. Both Core 23.1.8 and Core 24 resolve a port-less =onion bind to the chain's onion target port (OnionServiceTargetPort: 9996 / 19996 / 19796 / 19896).
    • Dashmate therefore doesn't copy Core's per-network constants.
    • Core 23 ends up bound to the same onion target address it used as its default before.
  • The bind lives in the # network block, because -bind is network-only. It renders inside [test], [devnet] or [regtest] for non-mainnet networks.
  • The template comment now explains why the explicit onion bind is needed and where it has to sit.
  • The Tor sidecar is unchanged. Core registers the onion service through the control port (ADD_ONION), targeting this bind, and the sidecar shares Core's network namespace.

How Has This Been Tested?

  • Red → green on the template tests:
    • original template: 6 passing / 4 failing (missing onion bind);
    • with the fix: 10 passing.
  • Targeted Tor template and migration suites: 23 passing. They cover:
    • all four networks with Tor enabled (bind present, inside the network section) and disabled (listenonion=0, no onion bind);
    • one upgrade from a 4.1.x-shaped config through the current migrations, then enabling Tor.
  • Full Dashmate unit suite: 865 passing. Lint is clean on the changed files.
  • Checked against Core sources v24.0.0-rc.3 and v23.1.8 (init.cpp):
    • the port-less onion bind resolves to OnionServiceTargetPort;
    • Core 23.1.8 accepts the syntax and keeps its previous onion target.
  • On devnet, the Tor-disabled path was verified on Core 24 rc.3 nodes.
  • The Tor-enabled path ran in this PR's CI on Core 23.1.8. The Test Suite's local network runs with Tor on (run 37809030257, job 113439409907). All 4 nodes logged:
    • listenonion="1" in the config;
    • Bound to 127.0.0.1:19896, the regtest onion target from the port-less =onion bind;
    • Got tor service ID …, then advertised the .onion address.
  • The Tor-enabled path on Core 24 is covered only by the source check above; it has not been run live.
  • An independent code review found no must-fix issues, and its suggestions are applied (port-less bind, explanatory comment, original migration tests kept unchanged).

Local note: to load the migration suite without a WASM build, the published @dashevo/wasm-dpp@5.0.0-beta.2 dist was used, with its integrity hash checked. That only affected the test environment.

Breaking Changes

None.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed
  • If I added or changed GroveDB structure, I described it in the area's structure.rs, regenerated grovedb-structure.json, and checked the structure viewer link posted on this pull request

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Tests
    • Expanded coverage for migrating older configurations with Tor disabled or enabled.
    • Added checks for Tor onion settings across mainnet, testnet, devnet, and local networks.

PR Hygiene · f9af8db

  • Bots — coderabbitai not yet · thepastaclaw not yet — /skip-bots proceeds without the ones not yet reported
  • Self-review — post /self-reviewed
  • Build running
  • Approvals — you own every area touched; none needed

When every merge requirement is met, the PR Hygiene check passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.

Render bind=127.0.0.1=onion beside the network bind when Tor is enabled, allowing Core to choose its chain-specific onion port. Explain why the listener is required and belongs in the network section; pin that placement for every network and the lack of a preceding section on mainnet.

Preserve the original focused Tor migration tests and cover upgrades before and after enabling Tor in one separate case. Core 23.1.8 and Core 24 rc.3 resolve portless onion binds using OnionServiceTargetPort; no migration or Tor sidecar changes are needed.

Test would have caught this in CI: ✖ before fix, ✔ after. Against the original template: 6 passing / 4 failing; restored fix: 10 passing. Combined Tor and migration suites: 23 passing. Full Dashmate unit suite: 865 passing; targeted lint and whitespace checks pass.
@shumkov shumkov added this to the v5.0.0 milestone Oct 8, 2026
@coderabbitai

coderabbitai Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration
  • Configuration used: Repository: dashpay/platform/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: d42ec89b-938a-4bbc-870b-0cffb8abf178

📥 Commits

Reviewing files that changed from the base of the PR and between f7c150f and f9af8db.


⛔ Files ignored due to path filters (1)
  • packages/dashmate/templates/core/dash.conf.dot is excluded by !**/*.dot

📒 Files selected for processing (1)
  • packages/dashmate/test/unit/config/configFile/migrateConfigFileFactory.spec.js

 ___________________________________________________________
< Veni, Vidi, Codici Fixi. I came, I saw, I fixed the code. >
 -----------------------------------------------------------
  \
   \   (\__/)
       (•ㅅ•)
       /   づ

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: dashpay/platform/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 6cf5e02e-a749-4162-9165-e89fd0e1911e

📥 Commits

Reviewing files that changed from the base of the PR and between bb78fa2 and f7c150f.


⛔ Files ignored due to path filters (1)
  • packages/dashmate/templates/core/dash.conf.dot is excluded by !**/*.dot

📒 Files selected for processing (2)
  • packages/dashmate/test/unit/config/configFile/migrateConfigFileFactory.spec.js
  • packages/dashmate/test/unit/templates/torTemplates.spec.js

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



📝 Walkthrough

Walkthrough

The tests now cover Tor-enabled and Tor-disabled template output across mainnet, testnet, devnet, and local networks. Migration tests also verify rendered Dash Core configuration before and after enabling Tor for legacy configs from three versions.

Changes

Tor Configuration Tests

Layer / File(s) Summary
Per-network Tor template coverage
packages/dashmate/test/unit/templates/torTemplates.spec.js
Tests check enabled Tor settings and clearnet and onion bind directives across four networks. Disabled Tor tests check that only the clearnet bind remains and that onion, Tor control, and proxy settings are absent.
Migrated configuration coverage
packages/dashmate/test/unit/config/configFile/migrateConfigFileFactory.spec.js
Tests check rendered configuration for legacy configs from three versions, including devnet, first with Tor disabled and then with Tor enabled.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix


Merge Risk: ⚪ Minimal · up to f7c15

No actionable merge-blocking risk is identified in the reviewed test changes; normal checks remain appropriate.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding a dedicated onion bind for Dash Core 24 in dashmate.


✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR

🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR


  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Waiting for bot review — coderabbitai ✓ · thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

@github-actions github-actions Bot added the waiting-bots Waiting for the review bots to report on this head label Oct 8, 2026
@thepastaclaw

thepastaclaw commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

✅ Final review complete — no blockers (commit f7c150f) · triage: low

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Final validation — Phase 1 + Phase 2

Verified the three-file PR diff at f7c150f and found no actionable in-scope defects. The conditional onion bind is correctly placed in each network's configuration section, preserves Tor-disabled behavior, and uses syntax confirmed in Core 23.1.8 and Core 24 rc.3; added tests cover all four networks and legacy-config migrations. This was a static review: the supplied CI snapshot shows Dashmate tests and lint passing, with the Dashmate helper image build and PR Hygiene pending; live Tor-enabled startup remains unverified.

Review provenance

Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: low by gpt-6.1-sol (effort low) — The diff adds a small conditional onion bind to a Dashmate configuration template and focused regression tests, making it a contained behavioral fix whose correctness is straightforward to confirm.
  • Phase 1 reviewers: gemini-3.8-flash-high — general (completed, effort high); agent phase1-reviewer
  • Phase 1 model: gemini-3.8-flash-high — antigravity quota: weekly 94% left, 5h 68% left
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort medium); agent phase2-reviewer

@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

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

@github-actions github-actions Bot added waiting-self-review Waiting for the author to post /self-reviewed and removed waiting-bots Waiting for the review bots to report on this head labels Oct 8, 2026
PastaPastaPasta
PastaPastaPasta previously approved these changes Oct 8, 2026

@PastaPastaPasta PastaPastaPasta left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ACK f7c150f

Checked against Core v24.0.0-rc.3, v24.0.x and develop (identical in the bind/onion block of init.cpp), plus v23.1.8:

  • With -bind set and -listenonion on, rc.3 requires a non-wildcard =onion bind. The port-less bind=127.0.0.1=onion resolves through BaseParams().OnionServiceTargetPort() (9996 / 19996 / 19796 / 19896), and those values are the same on 23.1.8 and 24.
  • On Core 23 the onion listener and Tor target stay the same as the old implicit default, so the onion address doesn't change.
  • -bind is network-only, and the new line renders inside [test]/[devnet]/[regtest] and at the top level on mainnet.
  • No later Core commits or open Core PRs change the bind or onion logic. A future bitcoin#36170 backport would not affect this config.
  • CI's Tor-enabled local network shows Bound to 127.0.0.1:19896 and Got tor service ID on all 4 nodes. The single Test Suite failure ("Quorum not found in cache") also happens on base v5.0-dev.

Non-blocking nits:

  • The template comment says "An explicit bind disables Core's automatic onion target", which is Core 24 behavior only. "(Core 24+)" would make it accurate while dashpay/dashd:23 is still pinned.
  • Since core.tor.enabled defaults to true for new setups, it's worth one local-network run on a 24.0.0-rc.3 image before the Core 24 rollout.

🤖 Posted autonomously by Claude on behalf of pasta.

…shmate-onion

# Conflicts:
#	packages/dashmate/test/unit/config/configFile/migrateConfigFileFactory.spec.js
@shumkov

shumkov commented Oct 9, 2026

Copy link
Copy Markdown
Collaborator Author

/self-reviewed

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

@github-actions github-actions Bot added waiting-bots Waiting for the review bots to report on this head and removed waiting-self-review Waiting for the author to post /self-reviewed labels Oct 9, 2026
@shumkov
shumkov merged commit 1e69a4c into v5.0-dev Oct 9, 2026
8 of 10 checks passed
@shumkov
shumkov deleted the shumkov/core24-dashmate-onion branch October 9, 2026 08:10
@QuantumExplorer QuantumExplorer mentioned this pull request Oct 13, 2026
8 of 26 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-bots Waiting for the review bots to report on this head

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants