Repository navigation
fix(dashmate): add dedicated onion bind for Core 24 - #5342
Conversation
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.
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe 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. ChangesTor Configuration Tests
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk is identified in the reviewed test changes; normal checks remain appropriate. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Waiting for bot review — coderabbitai ✓ · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
|
✅ Final review complete — no blockers (commit f7c150f) · triage: low |
thepastaclaw
left a comment
There was a problem hiding this comment.
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:
lowbygpt-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); agentphase1-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; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort medium); agentphase2-reviewer
|
Bots are done — your move: post |
PastaPastaPasta
left a comment
There was a problem hiding this comment.
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
-bindset and-listenonionon, rc.3 requires a non-wildcard=onionbind. The port-lessbind=127.0.0.1=onionresolves throughBaseParams().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.
-bindis 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:19896andGot tor service IDon all 4 nodes. The single Test Suite failure ("Quorum not found in cache") also happens on basev5.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:23is still pinned. - Since
core.tor.enableddefaults to true for new setups, it's worth one local-network run on a24.0.0-rc.3image 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
|
/self-reviewed |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
Issue being fixed or feature implemented
Dash Core 24 (
v24.0.0-rc.3, dashpay/dash#7786) refuses to start when-bindis set and-listenonionis enabled unless a dedicated, non-wildcard=onionbind exists:Core only adds its default onion bind when no
-bindis given, and Dashmate always rendersbind=0.0.0.0. Withcore.tor.enabled=true, the rendereddash.confhaslistenonion=1and 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=0and works on Core 24.What was done?
bind=127.0.0.1=onionnext to the existingbind=0.0.0.0.=onionbind to the chain's onion target port (OnionServiceTargetPort: 9996 / 19996 / 19796 / 19896).# networkblock, because-bindis network-only. It renders inside[test],[devnet]or[regtest]for non-mainnet networks.ADD_ONION), targeting this bind, and the sidecar shares Core's network namespace.How Has This Been Tested?
listenonion=0, no onion bind);v24.0.0-rc.3andv23.1.8(init.cpp):OnionServiceTargetPort;listenonion="1"in the config;Bound to 127.0.0.1:19896, the regtest onion target from the port-less=onionbind;Got tor service ID …, then advertised the.onionaddress.Local note: to load the migration suite without a WASM build, the published
@dashevo/wasm-dpp@5.0.0-beta.2distwas used, with its integrity hash checked. That only affected the test environment.Breaking Changes
None.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
PR Hygiene ·
f9af8db/skip-botsproceeds without the ones not yet reported/self-reviewedWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.