feat: improve external tor UX, add file socket control support, add tails support#4643
Open
wodoro wants to merge 2 commits into
Open
feat: improve external tor UX, add file socket control support, add tails support#4643wodoro wants to merge 2 commits into
wodoro wants to merge 2 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughTorrc handling changed from unordered key/value maps to ordered preformatted lines; overrides now support multi-valued directives and optional external torrc files. External-Tor connection logic added ControlSocket (unix domain) support, heuristics and resolver logic, related protocol/controller overloads, utilities, config fields, and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant TorService as "TorService"
participant TorTransportConfig as "TorTransportConfig"
participant TorrcClientConfigFactory as "TorrcClientConfigFactory"
participant TorrcFileGenerator as "TorrcFileGenerator"
participant FileSystem as "FileSystem"
TorService->>TorTransportConfig: read `torrcOverrideFilePath` & `torrcOverrides`
alt override file path non-empty & readable
TorService->>TorTransportConfig: parseTorrcOverrideFile(path) -> Map<String,List<String>>
TorService-->>TorService: overrides = file entries
else
TorService-->>TorService: overrides = inline torrcOverrides
end
TorService->>TorrcClientConfigFactory: torrcClientConfigLines(overrides)
TorrcClientConfigFactory->>TorrcClientConfigFactory: build base lines, apply DISABLE_NETWORK, apply overrides (multi-value)
TorrcClientConfigFactory-->>TorService: List<String> torrcLines
TorService->>TorrcFileGenerator: new TorrcFileGenerator(path, torrcLines, customDAs)
TorrcFileGenerator->>FileSystem: write each line + "\n"
FileSystem-->>TorrcFileGenerator: write complete
TorrcFileGenerator-->>TorService: torrc file created
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
wodoro
force-pushed
the
improve_tor_override
branch
2 times, most recently
from
fc6984c to
3a029bb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
network/tor/tor/src/test/java/bisq/network/tor/TorrcClientConfigFactoryTest.java (1)
136-160: End-to-end test withTorrcFileGenerator.Good coverage of the full flow: generate lines → write torrc → verify file content.
Consider adding a test for the edge case where an override key has an empty list value, to document whether this intentionally removes the directive or is an unsupported scenario.
💡 Optional: Add test for empty list override
`@Test` void overrideWithEmptyListRemovesKeyFromOutput() { when(mockGenerator.generate()).thenReturn(new HashMap<>(Map.of( "SocksPort", "auto", "DataDirectory", "/tmp/tor" ))); List<String> lines = factory.torrcClientConfigLines( Map.of("SocksPort", List.of())); // empty list assertThat(lines).doesNotContain("SocksPort auto"); assertThat(lines.stream().filter(l -> l.startsWith("SocksPort "))).isEmpty(); assertThat(lines).contains("DataDirectory /tmp/tor"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/tor/tor/src/test/java/bisq/network/tor/TorrcClientConfigFactoryTest.java` around lines 136 - 160, Add a test to cover the edge case where an override key maps to an empty list so we document and verify intended behavior of torrcClientConfigLines; specifically, in TorrcClientConfigFactoryTest create a test (e.g., overrideWithEmptyListRemovesKeyFromOutput) that stubs mockGenerator.generate() to return defaults (SocksPort -> "auto", DataDirectory -> "/tmp/tor"), calls factory.torrcClientConfigLines(Map.of("SocksPort", List.of())), and asserts the resulting lines do not contain "SocksPort auto", contain "DataDirectory /tmp/tor", and have no lines starting with "SocksPort " (also consider verifying behavior of DISABLE_NETWORK if relevant), so the test documents whether an empty override removes the directive.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@network/tor/tor/src/test/java/bisq/network/tor/TorrcClientConfigFactoryTest.java`:
- Around line 136-160: Add a test to cover the edge case where an override key
maps to an empty list so we document and verify intended behavior of
torrcClientConfigLines; specifically, in TorrcClientConfigFactoryTest create a
test (e.g., overrideWithEmptyListRemovesKeyFromOutput) that stubs
mockGenerator.generate() to return defaults (SocksPort -> "auto", DataDirectory
-> "/tmp/tor"), calls factory.torrcClientConfigLines(Map.of("SocksPort",
List.of())), and asserts the resulting lines do not contain "SocksPort auto",
contain "DataDirectory /tmp/tor", and have no lines starting with "SocksPort "
(also consider verifying behavior of DISABLE_NETWORK if relevant), so the test
documents whether an empty override removes the directive.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 09034b0c-ed22-4c06-944c-46d1da235c97
📒 Files selected for processing (6)
network/tor/tor-common/src/main/java/bisq/network/tor/common/torrc/TorrcFileGenerator.javanetwork/tor/tor/src/main/java/bisq/network/tor/TorService.javanetwork/tor/tor/src/main/java/bisq/network/tor/TorTransportConfig.javanetwork/tor/tor/src/main/java/bisq/network/tor/TorrcClientConfigFactory.javanetwork/tor/tor/src/test/java/bisq/network/tor/TorTransportConfigTorrcOverridesTest.javanetwork/tor/tor/src/test/java/bisq/network/tor/TorrcClientConfigFactoryTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- network/tor/tor/src/main/java/bisq/network/tor/TorTransportConfig.java
- network/tor/tor-common/src/main/java/bisq/network/tor/common/torrc/TorrcFileGenerator.java
- network/tor/tor/src/test/java/bisq/network/tor/TorTransportConfigTorrcOverridesTest.java
wodoro
force-pushed
the
improve_tor_override
branch
from
3a029bb to
70cbd9e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
network/tor/tor/src/test/java/bisq/network/tor/TorrcClientConfigFactoryTest.java (1)
55-57: Avoidnullin test setup for constructor dependencies.At Line 55, passing
nullforPasswordDigestmakes the test more brittle than necessary. Use a mock/stub dependency instead.Suggested change
import bisq.network.tor.common.torrc.TorrcFileGenerator; import bisq.network.tor.common.torrc.TorrcConfigGenerator; +import net.freehaven.tor.control.PasswordDigest; @@ void setUp() { mockGenerator = Mockito.mock(TorrcConfigGenerator.class); + PasswordDigest passwordDigest = Mockito.mock(PasswordDigest.class); // spy on a factory instance with null PasswordDigest — safe because // clientTorrcGenerator() is mocked and hashedControlPassword is never accessed. // Use doReturn().when() to avoid calling the real method during stub setup. - TorrcClientConfigFactory base = new TorrcClientConfigFactory(false, tempDir, null); + TorrcClientConfigFactory base = new TorrcClientConfigFactory(false, tempDir, passwordDigest); factory = Mockito.spy(base); doReturn(mockGenerator).when(factory).clientTorrcGenerator(); }As per coding guidelines: "Avoid nullable values; use
Optionaland@Nullableannotation where needed".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/tor/tor/src/test/java/bisq/network/tor/TorrcClientConfigFactoryTest.java` around lines 55 - 57, The test constructs TorrcClientConfigFactory with a null PasswordDigest which is brittle; replace the null with a mock/stub PasswordDigest (e.g., create a Mockito.mock(PasswordDigest.class) or a lightweight test implementation) and pass that into the TorrcClientConfigFactory constructor before creating the spy and stubbing clientTorrcGenerator(), ensuring TorrcClientConfigFactory(...) uses the mock PasswordDigest rather than null.network/tor/tor/src/main/java/bisq/network/tor/TorrcClientConfigFactory.java (2)
50-52: Follow existing generator pattern; defensive copy is optional.At Line 51,
baseConfig.put(...)mutates the map returned byclientTorrcGenerator().generate(). This is safe becauseBaseTorrcGenerator.generate()returns a freshHashMap<>on each call. However, other generators in the codebase (e.g.,ClientTorrcGenerator) follow the same direct-mutation pattern without defensive copies. If you prefer defensive programming, wrapping innew HashMap<>(...)is reasonable but not required for correctness given the current design.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/tor/tor/src/main/java/bisq/network/tor/TorrcClientConfigFactory.java` around lines 50 - 52, The code currently mutates the map returned by clientTorrcGenerator().generate() by calling baseConfig.put(DISABLE_NETWORK, "1"); to match the existing generator pattern in TorrcClientConfigFactory and ClientTorrcGenerator you should keep the direct-mutation approach (no defensive copy) so behavior is consistent; if you prefer defensive programming, replace the assignment with a copy via new HashMap<>(clientTorrcGenerator().generate()) and then put DISABLE_NETWORK into that new map before returning.
54-60: Consider using LinkedHashMap or sorting keys for deterministic torrc output.Lines 54 and 59 iterate unordered HashMaps directly.
BaseTorrcGenerator.javacreates the base config withnew HashMap<>()(line 47), andTorTransportConfig.javacreates torrc overrides withnew HashMap<>()(line 76). This causes non-deterministic iteration order, resulting in different line ordering across runs. Replace withLinkedHashMapor sort keys before emitting to ensure stable, reproducible torrc files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/tor/tor/src/main/java/bisq/network/tor/TorrcClientConfigFactory.java` around lines 54 - 60, The torrc emission in TorrcClientConfigFactory currently iterates baseConfig and torrcOverrides (variables baseConfig and torrcOverrides) which are created as plain HashMap instances upstream (see BaseTorrcGenerator and TorTransportConfig), causing non-deterministic ordering; fix this by making iteration deterministic: either change the map constructions to preserve insertion order (use LinkedHashMap when creating the base config in BaseTorrcGenerator and the overrides in TorTransportConfig) or, if changing creators is undesirable, sort the keys in TorrcClientConfigFactory before emitting (e.g., iterate over new TreeSet<>(baseConfig.keySet()) and new TreeSet<>(torrcOverrides.keySet()) or collect and sort the key lists) so lines.add(...) is called in a stable order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@network/tor/tor/src/main/java/bisq/network/tor/TorrcClientConfigFactory.java`:
- Around line 50-52: The code currently mutates the map returned by
clientTorrcGenerator().generate() by calling baseConfig.put(DISABLE_NETWORK,
"1"); to match the existing generator pattern in TorrcClientConfigFactory and
ClientTorrcGenerator you should keep the direct-mutation approach (no defensive
copy) so behavior is consistent; if you prefer defensive programming, replace
the assignment with a copy via new HashMap<>(clientTorrcGenerator().generate())
and then put DISABLE_NETWORK into that new map before returning.
- Around line 54-60: The torrc emission in TorrcClientConfigFactory currently
iterates baseConfig and torrcOverrides (variables baseConfig and torrcOverrides)
which are created as plain HashMap instances upstream (see BaseTorrcGenerator
and TorTransportConfig), causing non-deterministic ordering; fix this by making
iteration deterministic: either change the map constructions to preserve
insertion order (use LinkedHashMap when creating the base config in
BaseTorrcGenerator and the overrides in TorTransportConfig) or, if changing
creators is undesirable, sort the keys in TorrcClientConfigFactory before
emitting (e.g., iterate over new TreeSet<>(baseConfig.keySet()) and new
TreeSet<>(torrcOverrides.keySet()) or collect and sort the key lists) so
lines.add(...) is called in a stable order.
In
`@network/tor/tor/src/test/java/bisq/network/tor/TorrcClientConfigFactoryTest.java`:
- Around line 55-57: The test constructs TorrcClientConfigFactory with a null
PasswordDigest which is brittle; replace the null with a mock/stub
PasswordDigest (e.g., create a Mockito.mock(PasswordDigest.class) or a
lightweight test implementation) and pass that into the TorrcClientConfigFactory
constructor before creating the spy and stubbing clientTorrcGenerator(),
ensuring TorrcClientConfigFactory(...) uses the mock PasswordDigest rather than
null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d5c8cb9d-001f-4c92-80e6-3d4f486cf8f2
📒 Files selected for processing (6)
network/tor/tor-common/src/main/java/bisq/network/tor/common/torrc/TorrcFileGenerator.javanetwork/tor/tor/src/main/java/bisq/network/tor/TorService.javanetwork/tor/tor/src/main/java/bisq/network/tor/TorTransportConfig.javanetwork/tor/tor/src/main/java/bisq/network/tor/TorrcClientConfigFactory.javanetwork/tor/tor/src/test/java/bisq/network/tor/TorTransportConfigTorrcOverridesTest.javanetwork/tor/tor/src/test/java/bisq/network/tor/TorrcClientConfigFactoryTest.java
✅ Files skipped from review due to trivial changes (1)
- network/tor/tor/src/test/java/bisq/network/tor/TorTransportConfigTorrcOverridesTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- network/tor/tor/src/main/java/bisq/network/tor/TorService.java
- network/tor/tor/src/main/java/bisq/network/tor/TorTransportConfig.java
- network/tor/tor-common/src/main/java/bisq/network/tor/common/torrc/TorrcFileGenerator.java
wodoro
force-pushed
the
improve_tor_override
branch
from
70cbd9e to
d3be566
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@network/tor/tor/src/main/java/bisq/network/tor/TorTransportConfig.java`:
- Around line 78-115: Both parsers use HashMap which doesn't preserve insertion
order; change the Map implementations in parseTorrcOverrideConfig and
parseTorrcOverrideFile to LinkedHashMap (e.g., replace new HashMap<>() with new
LinkedHashMap<>()) so directive order from the config object or file is
preserved when TorrcClientConfigFactory.torrcClientConfigLines() iterates them;
keep the return type as Map<String, List<String>> and ensure
java.util.LinkedHashMap is available/imported.
In
`@network/tor/tor/src/test/java/bisq/network/tor/TorrcClientConfigFactoryTest.java`:
- Around line 109-123: The test multipleBridgeOverrideValuesAllAppearInOutput
currently only asserts presence and count, so add a deterministic order check on
the torrcClientConfigLines output to ensure order-sensitive directives are
emitted in the expected sequence; update the test that builds lines from
factory.torrcClientConfigLines(...) (the lines variable) to include a
sequence/assertion that "Bridge obfs4 192.0.2.1:1234 FP1" appears before "Bridge
obfs4 192.0.2.2:5678 FP2" (e.g. check indexOf or use an assertion that verifies
the two Bridge entries are in that order) while keeping the existing
presence/count assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cbf153b9-1bec-406f-9578-9c5e67a5e5dc
📒 Files selected for processing (11)
apps/api-app/src/main/resources/api_app.confapps/desktop/desktop-app/src/main/resources/desktop.confapps/node-monitor-web-app/src/main/resources/node_monitor.confapps/oracle-node-app/src/main/resources/oracle_node.confapps/seed-node-app/src/main/resources/seed_node.confnetwork/tor/tor-common/src/main/java/bisq/network/tor/common/torrc/TorrcFileGenerator.javanetwork/tor/tor/src/main/java/bisq/network/tor/TorService.javanetwork/tor/tor/src/main/java/bisq/network/tor/TorTransportConfig.javanetwork/tor/tor/src/main/java/bisq/network/tor/TorrcClientConfigFactory.javanetwork/tor/tor/src/test/java/bisq/network/tor/TorTransportConfigTorrcOverridesTest.javanetwork/tor/tor/src/test/java/bisq/network/tor/TorrcClientConfigFactoryTest.java
✅ Files skipped from review due to trivial changes (1)
- network/tor/tor/src/test/java/bisq/network/tor/TorTransportConfigTorrcOverridesTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- network/tor/tor/src/main/java/bisq/network/tor/TorrcClientConfigFactory.java
- network/tor/tor-common/src/main/java/bisq/network/tor/common/torrc/TorrcFileGenerator.java
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
network/tor/tor/src/test/java/bisq/network/tor/TorTransportConfigTorrcOverridesTest.java (2)
159-173: Same suggestion: usecontainsExactlyto verify file parsing order.The torrc-style file is read line-by-line, so the order should be deterministic. Using
containsExactlywould catch any regressions in order preservation.♻️ Suggested strengthening
- assertThat(result.get("Bridge")).containsExactlyInAnyOrder( - "obfs4 1.2.3.4:1234 FP1", - "obfs4 5.6.7.8:5678 FP2"); + assertThat(result.get("Bridge")).containsExactly( + "obfs4 1.2.3.4:1234 FP1", + "obfs4 5.6.7.8:5678 FP2");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/tor/tor/src/test/java/bisq/network/tor/TorTransportConfigTorrcOverridesTest.java` around lines 159 - 173, The test parseTorrcOverrideFileAccumulatesRepeatedKeys should assert order preservation: update the assertion on the "Bridge" key in the test to use containsExactly (not containsExactlyInAnyOrder) so it expects "obfs4 1.2.3.4:1234 FP1" then "obfs4 5.6.7.8:5678 FP2" in that exact order when calling TorTransportConfig.parseTorrcOverrideFile; keep the existing containsExactly on "UseBridges" unchanged.
69-78: Consider usingcontainsExactlyto verify order preservation.Since
LinkedHashMapis used specifically to preserve directive order, and order can matter for Bridge directives (Tor may try them sequentially), usingcontainsExactlyinstead ofcontainsExactlyInAnyOrderwould verify this behavior is maintained.♻️ Suggested strengthening
- assertThat(overrides.get("Bridge")).containsExactlyInAnyOrder( - "obfs4 192.0.2.1:1234 FP1", - "obfs4 192.0.2.2:5678 FP2" - ); + assertThat(overrides.get("Bridge")).containsExactly( + "obfs4 192.0.2.1:1234 FP1", + "obfs4 192.0.2.2:5678 FP2" + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/tor/tor/src/test/java/bisq/network/tor/TorTransportConfigTorrcOverridesTest.java` around lines 69 - 78, The test TorTransportConfigTorrcOverridesTest currently uses containsExactlyInAnyOrder for the "Bridge" assertion which allows permutation; change that assertion to use containsExactly so the test verifies the LinkedHashMap order is preserved (keep the "UseBridges" assertion as-is), locating the behavior via TorTransportConfig.from(...) and checking the returned map from getTorrcOverrides() to ensure the Bridge entries are asserted in exact order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@network/tor/tor/src/test/java/bisq/network/tor/TorTransportConfigTorrcOverridesTest.java`:
- Around line 159-173: The test parseTorrcOverrideFileAccumulatesRepeatedKeys
should assert order preservation: update the assertion on the "Bridge" key in
the test to use containsExactly (not containsExactlyInAnyOrder) so it expects
"obfs4 1.2.3.4:1234 FP1" then "obfs4 5.6.7.8:5678 FP2" in that exact order when
calling TorTransportConfig.parseTorrcOverrideFile; keep the existing
containsExactly on "UseBridges" unchanged.
- Around line 69-78: The test TorTransportConfigTorrcOverridesTest currently
uses containsExactlyInAnyOrder for the "Bridge" assertion which allows
permutation; change that assertion to use containsExactly so the test verifies
the LinkedHashMap order is preserved (keep the "UseBridges" assertion as-is),
locating the behavior via TorTransportConfig.from(...) and checking the returned
map from getTorrcOverrides() to ensure the Bridge entries are asserted in exact
order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 88e3b18c-c2f9-46ea-ad2d-d98ce6d8abdb
📒 Files selected for processing (11)
apps/api-app/src/main/resources/api_app.confapps/desktop/desktop-app/src/main/resources/desktop.confapps/node-monitor-web-app/src/main/resources/node_monitor.confapps/oracle-node-app/src/main/resources/oracle_node.confapps/seed-node-app/src/main/resources/seed_node.confnetwork/tor/tor-common/src/main/java/bisq/network/tor/common/torrc/TorrcFileGenerator.javanetwork/tor/tor/src/main/java/bisq/network/tor/TorService.javanetwork/tor/tor/src/main/java/bisq/network/tor/TorTransportConfig.javanetwork/tor/tor/src/main/java/bisq/network/tor/TorrcClientConfigFactory.javanetwork/tor/tor/src/test/java/bisq/network/tor/TorTransportConfigTorrcOverridesTest.javanetwork/tor/tor/src/test/java/bisq/network/tor/TorrcClientConfigFactoryTest.java
wodoro
force-pushed
the
improve_tor_override
branch
from
d623910 to
adf660a
Compare
Contributor
Author
|
I'll mark this as ready as soon as I can confirm the issue #4606 is resolved by this |
wodoro
force-pushed
the
improve_tor_override
branch
from
91d4270 to
5f68ddc
Compare
wodoro
changed the title
wodoro
force-pushed
the
improve_tor_override
branch
2 times, most recently
from
6a06d3a to
d293469
Compare
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
network/tor/tor/src/main/java/bisq/network/tor/TorService.java (2)
100-120:⚠️ Potential issue | 🟠 MajorReset
isRunningwhen external-Tor initialization fails.Line 101 marks the service as running before the new failure path on Lines 117-119. If this throws, later
initialize()calls will short-circuit at Lines 102-104 even though startup never completed, so a transient external-Tor outage becomes unrecoverable until process restart.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/tor/tor/src/main/java/bisq/network/tor/TorService.java` around lines 100 - 120, The initialize() method sets isRunning to true early but never clears it when the external-Tor branch fails; update initialize() so that if evaluateUseExternalTor()/connectedToExternalTor(externalTorConfigMap) fails and you throw the RuntimeException, you first reset isRunning back to false (e.g., isRunning.set(false) or compareAndSet(true, false)); best placed just before throwing in the external-Tor failure path (and consider wrapping the external-Tor block starting at useExternalTor.set(...) in a try/catch/finally to ensure isRunning is cleared on any exception). Ensure you reference isRunning, initialize(), connectedToExternalTor(), externalTorConfigMap and the external-Tor error throw site when making the change.
376-389:⚠️ Potential issue | 🟠 MajorDo not collapse external Tor config lines into a
Set.Line 381 drops file order and makes repeated keys resolve nondeterministically once they are inserted into
externalTorConfigMap. That breaks the newtorrcOverrideFilePathreuse path for configs that contain multiple occurrences of a directive, because whicheverControlPort/CookieAuthFileline happens to be seen last wins arbitrarily.♻️ Suggested fix
- String torConfig = readExternalTorConfig(externalTorConfigPath); - Set<String> lines = torConfig.lines().collect(Collectors.toSet()); - for (String line : lines) { + String torConfig = readExternalTorConfig(externalTorConfigPath); + externalTorConfigMap.clear(); + for (String line : torConfig.lines().toList()) { line = line.trim(); if (!line.isEmpty() && !line.startsWith("#")) { int firstSpaceIndex = line.indexOf(" "); if (firstSpaceIndex != -1) { String key = line.substring(0, firstSpaceIndex); String value = line.substring(firstSpaceIndex + 1); externalTorConfigMap.put(key, value); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/tor/tor/src/main/java/bisq/network/tor/TorService.java` around lines 376 - 389, The code in readExternalTorConfigMap currently collects torConfig.lines() into a Set, losing file order and causing nondeterministic key resolution; change it to iterate lines in the original file order (e.g., collect to a List or iterate the Stream directly) instead of using a Set so repeated directives (ControlPort, CookieAuthFile, etc.) are processed in file order and the last occurrence deterministically overrides earlier ones when inserting into externalTorConfigMap.
🧹 Nitpick comments (4)
network/tor/tor/src/test/java/bisq/network/tor/ExternalTorConfigHeuristicsTest.java (1)
43-50: Assert replacement, not just presence.These checks pass even if
apply(...)appends a second directive and leaves the original placeholder behind. Since this code is rewriting control-auth settings, it is worth asserting that the placeholder/original directive is gone and that each rewritten directive appears only once.Possible assertion upgrade
assertThat(result).contains("CookieAuthentication 1"); assertThat(result).contains("CookieAuthFile " + cookieAuthFilePath); assertThat(result).contains("ControlSocket " + controlSocketPath); + assertThat(result).doesNotContain("## ControlSocket /path/to/tor/control.socket"); + assertThat(result.lines().filter(line -> line.startsWith("ControlSocket "))).hasSize(1); @@ assertThat(result).contains("CookieAuthentication 0"); assertThat(result).contains("CookieAuthFile /path/to/control_auth_cookie"); assertThat(result).contains("ControlSocket " + controlSocketPath); + assertThat(result.lines().filter(line -> line.startsWith("ControlSocket "))).hasSize(1);As per coding guidelines "Add or update tests when behavior changes; keep tests deterministic and fast".
Also applies to: 76-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/tor/tor/src/test/java/bisq/network/tor/ExternalTorConfigHeuristicsTest.java` around lines 43 - 50, Replace the loose contains assertions in ExternalTorConfigHeuristicsTest (the block calling ExternalTorConfigHeuristics.apply with template, controlSocketPath and List.of(cookieAuthFilePath)) with stricter assertions that the placeholder/original directives are removed and each rewritten directive appears exactly once; specifically, assert the full resulting string equals the expected template with the three directives ("CookieAuthentication 1", "CookieAuthFile <cookieAuthFilePath>", "ControlSocket <controlSocketPath>") substituted in place of the placeholder, or alternatively assert that the placeholder is absent and that occurrences of each directive (using the result string) have count == 1. Apply the same replacement for the other test block around lines 76-83 that checks the same behavior.network/tor/tor/src/main/java/bisq/network/tor/TorrcClientConfigFactory.java (1)
49-61: Make directive ordering explicit at this boundary.The return type is now ordered, but the input contract is still
Map<String, List<String>>, so the emitted torrc order is only as stable as whateverMapimplementation the caller happens to pass in. If preserving operator-specified order matters here, prefer an ordered directive type at the API boundary, or at least make the insertion-order requirement explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/tor/tor/src/main/java/bisq/network/tor/TorrcClientConfigFactory.java` around lines 49 - 61, The torrcClientConfigLines method relies on caller map iteration order (torrcOverrides) but its signature uses unordered Map; change the API to require an ordered map or enforce ordering inside: update the method signature to accept LinkedHashMap<String,List<String>> (or List<Map.Entry<String,List<String>>>), or immediately copy torrcOverrides into a new LinkedHashMap<String,List<String>> before iterating, then build lines from that ordered map; update callers to pass an ordered map or adapt to the new signature (references: torrcClientConfigLines, torrcOverrides, clientTorrcGenerator().generate(), baseConfig).network/tor/tor/src/test/java/bisq/network/tor/TorServiceTest.java (1)
106-134: Test config creation doesn't match test intent.The test name suggests external Tor is enabled, but the
TorTransportConfigis created withuseExternalTor=falseat line 126. The test relies on the file content (UseExternalTor 1) rather than the config field. This works becauseTorServicereads the setting from the config file, but it's potentially confusing.Consider either:
- Updating the test name to clarify it tests file-based
UseExternalTordetection- Or adding a comment explaining the distinction between
TorTransportConfig.useExternalTorand the file-basedUseExternalTordirective🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/tor/tor/src/test/java/bisq/network/tor/TorServiceTest.java` around lines 106 - 134, Rename or clarify the test initializeFailsWhenExternalTorIsEnabledButControlServerIsUnreachable (or add a brief inline comment) to indicate it verifies file-based "UseExternalTor" detection rather than the TorTransportConfig.useExternalTor field; specifically, update the test name or add a comment near the TorTransportConfig construction explaining that TorService reads the UseExternalTor directive from the external_tor.config file (not the boolean passed into TorTransportConfig), referencing TorTransportConfig and TorService so future readers understand why useExternalTor is false while the file contains "UseExternalTor 1".network/tor/tor/src/main/java/bisq/network/tor/ExternalTorConfigHeuristics.java (1)
56-68: Consider consolidating the duplicateisPresent()check.The
cookieAuthFilePath.isPresent()check appears twice (lines 56 and 62). While not incorrect, consolidating the check would slightly improve readability.♻️ Optional refactor
if (cookieAuthFilePath.isPresent()) { configWithDetectedValues = configWithDetectedValues .replace("CookieAuthentication 0", "CookieAuthentication 1") .replace("CookieAuthFile /path/to/control_auth_cookie", "CookieAuthFile " + cookieAuthFilePath.get()); - } - - if (cookieAuthFilePath.isPresent()) { log.info("Detected system Tor control socket at '{}'. Writing external_tor.config with ControlSocket='{}', CookieAuthentication=1 and CookieAuthFile='{}'.", systemTorControlSocketPath, systemTorControlSocketPath, cookieAuthFilePath.get()); } else { log.info("Detected system Tor control socket at '{}'. Writing external_tor.config with ControlSocket='{}' and leaving CookieAuthentication disabled because no auth cookie file was found.", systemTorControlSocketPath, systemTorControlSocketPath); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/tor/tor/src/main/java/bisq/network/tor/ExternalTorConfigHeuristics.java` around lines 56 - 68, Consolidate the duplicated cookieAuthFilePath.isPresent() checks in ExternalTorConfigHeuristics by performing the presence check once, capturing cookieAuthFilePath.get() into a local variable (e.g., cookiePath) when present, then update configWithDetectedValues (replace CookieAuthentication and CookieAuthFile) and call log.info using that local cookiePath; if not present, run the alternate log.info path using systemTorControlSocketPath—this removes the repeated isPresent() call while keeping the same behavior for configWithDetectedValues and logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@common/src/main/java/bisq/common/util/StringUtils.java`:
- Around line 187-201: Annotate the nullable contract for StringUtils.unquote by
marking both the parameter and return type with `@Nullable` (e.g., change
signature to public static `@Nullable` String unquote(`@Nullable` String value)),
add the corresponding import for your project's nullable annotation (e.g.,
javax.annotation.Nullable or org.jetbrains.annotations.Nullable), and adjust any
static-analysis settings if needed; keep the method body unchanged.
In `@network/tor/tor/src/main/java/bisq/network/tor/TorService.java`:
- Around line 526-540: The ControlEndpoint record currently stores mutually
exclusive state using raw nulls (fields controlHost, controlPort,
controlSocketPath) which violates the project's nullability rules; update the
design to avoid sentinel nulls by either (A) splitting into two explicit types
(e.g., SocketControlEndpoint and HostPortControlEndpoint) and replacing usages
of ControlEndpoint with the appropriate subtype, or (B) make nullable fields
explicit by wrapping them in Optional and/or annotating with `@Nullable` and
updating factory methods forPort and forSocket and the uses of
usesControlSocket() accordingly so callers must handle absent values explicitly
(update all call sites that construct or deconstruct ControlEndpoint like
forPort, forSocket, and usesControlSocket).
In
`@network/tor/tor/src/test/java/bisq/network/tor/controller/TorControlProtocolTest.java`:
- Around line 51-54: The test currently calls torControlProtocol.close() and
then serverThread.join(TimeUnit.SECONDS.toMillis(2)) but does not fail if the
helper thread remains alive; change the teardown to assert the server thread
actually terminated: after the join call, check serverThread.isAlive() and fail
the test (or assertFalse) if it’s still alive, optionally interrupt or dump
diagnostics before failing; reference the existing torControlProtocol.close()
call and the serverThread.join(TimeUnit.SECONDS.toMillis(2)) invocation to
locate where to add the post-join liveliness check and assertion.
---
Outside diff comments:
In `@network/tor/tor/src/main/java/bisq/network/tor/TorService.java`:
- Around line 100-120: The initialize() method sets isRunning to true early but
never clears it when the external-Tor branch fails; update initialize() so that
if evaluateUseExternalTor()/connectedToExternalTor(externalTorConfigMap) fails
and you throw the RuntimeException, you first reset isRunning back to false
(e.g., isRunning.set(false) or compareAndSet(true, false)); best placed just
before throwing in the external-Tor failure path (and consider wrapping the
external-Tor block starting at useExternalTor.set(...) in a try/catch/finally to
ensure isRunning is cleared on any exception). Ensure you reference isRunning,
initialize(), connectedToExternalTor(), externalTorConfigMap and the
external-Tor error throw site when making the change.
- Around line 376-389: The code in readExternalTorConfigMap currently collects
torConfig.lines() into a Set, losing file order and causing nondeterministic key
resolution; change it to iterate lines in the original file order (e.g., collect
to a List or iterate the Stream directly) instead of using a Set so repeated
directives (ControlPort, CookieAuthFile, etc.) are processed in file order and
the last occurrence deterministically overrides earlier ones when inserting into
externalTorConfigMap.
---
Nitpick comments:
In
`@network/tor/tor/src/main/java/bisq/network/tor/ExternalTorConfigHeuristics.java`:
- Around line 56-68: Consolidate the duplicated cookieAuthFilePath.isPresent()
checks in ExternalTorConfigHeuristics by performing the presence check once,
capturing cookieAuthFilePath.get() into a local variable (e.g., cookiePath) when
present, then update configWithDetectedValues (replace CookieAuthentication and
CookieAuthFile) and call log.info using that local cookiePath; if not present,
run the alternate log.info path using systemTorControlSocketPath—this removes
the repeated isPresent() call while keeping the same behavior for
configWithDetectedValues and logging.
In
`@network/tor/tor/src/main/java/bisq/network/tor/TorrcClientConfigFactory.java`:
- Around line 49-61: The torrcClientConfigLines method relies on caller map
iteration order (torrcOverrides) but its signature uses unordered Map; change
the API to require an ordered map or enforce ordering inside: update the method
signature to accept LinkedHashMap<String,List<String>> (or
List<Map.Entry<String,List<String>>>), or immediately copy torrcOverrides into a
new LinkedHashMap<String,List<String>> before iterating, then build lines from
that ordered map; update callers to pass an ordered map or adapt to the new
signature (references: torrcClientConfigLines, torrcOverrides,
clientTorrcGenerator().generate(), baseConfig).
In
`@network/tor/tor/src/test/java/bisq/network/tor/ExternalTorConfigHeuristicsTest.java`:
- Around line 43-50: Replace the loose contains assertions in
ExternalTorConfigHeuristicsTest (the block calling
ExternalTorConfigHeuristics.apply with template, controlSocketPath and
List.of(cookieAuthFilePath)) with stricter assertions that the
placeholder/original directives are removed and each rewritten directive appears
exactly once; specifically, assert the full resulting string equals the expected
template with the three directives ("CookieAuthentication 1", "CookieAuthFile
<cookieAuthFilePath>", "ControlSocket <controlSocketPath>") substituted in place
of the placeholder, or alternatively assert that the placeholder is absent and
that occurrences of each directive (using the result string) have count == 1.
Apply the same replacement for the other test block around lines 76-83 that
checks the same behavior.
In `@network/tor/tor/src/test/java/bisq/network/tor/TorServiceTest.java`:
- Around line 106-134: Rename or clarify the test
initializeFailsWhenExternalTorIsEnabledButControlServerIsUnreachable (or add a
brief inline comment) to indicate it verifies file-based "UseExternalTor"
detection rather than the TorTransportConfig.useExternalTor field; specifically,
update the test name or add a comment near the TorTransportConfig construction
explaining that TorService reads the UseExternalTor directive from the
external_tor.config file (not the boolean passed into TorTransportConfig),
referencing TorTransportConfig and TorService so future readers understand why
useExternalTor is false while the file contains "UseExternalTor 1".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9a6ce77a-7067-4905-bb96-46e41d473440
📒 Files selected for processing (25)
apps/api-app/src/main/resources/api_app.confapps/desktop/desktop-app/src/main/resources/desktop.confapps/node-monitor-web-app/src/main/resources/node_monitor.confapps/oracle-node-app/src/main/resources/oracle_node.confapps/seed-node-app/src/main/resources/seed_node.confcommon/src/main/java/bisq/common/application/ConfigUtil.javacommon/src/main/java/bisq/common/util/StringUtils.javacommon/src/test/java/bisq/common/application/ConfigUtilTest.javacommon/src/test/java/bisq/common/util/StringUtilsTest.javanetwork/network/src/main/resources/tor/external_tor.confignetwork/tor/tor-common/src/main/java/bisq/network/tor/common/torrc/Torrc.javanetwork/tor/tor-common/src/main/java/bisq/network/tor/common/torrc/TorrcFileGenerator.javanetwork/tor/tor/src/main/java/bisq/network/tor/ExternalTorConfigHeuristics.javanetwork/tor/tor/src/main/java/bisq/network/tor/TorService.javanetwork/tor/tor/src/main/java/bisq/network/tor/TorTransportConfig.javanetwork/tor/tor/src/main/java/bisq/network/tor/TorrcClientConfigFactory.javanetwork/tor/tor/src/main/java/bisq/network/tor/UseExternalTorResolver.javanetwork/tor/tor/src/main/java/bisq/network/tor/controller/TorControlProtocol.javanetwork/tor/tor/src/main/java/bisq/network/tor/controller/TorController.javanetwork/tor/tor/src/test/java/bisq/network/tor/ExternalTorConfigHeuristicsTest.javanetwork/tor/tor/src/test/java/bisq/network/tor/TorServiceTest.javanetwork/tor/tor/src/test/java/bisq/network/tor/TorTransportConfigTorrcOverridesTest.javanetwork/tor/tor/src/test/java/bisq/network/tor/TorrcClientConfigFactoryTest.javanetwork/tor/tor/src/test/java/bisq/network/tor/UseExternalTorResolverTest.javanetwork/tor/tor/src/test/java/bisq/network/tor/controller/TorControlProtocolTest.java
wodoro
force-pushed
the
improve_tor_override
branch
3 times, most recently
from
54962bc to
3dd28da
Compare
wodoro
changed the title
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (11)
build-logic/packaging/src/main/kotlin/bisq/gradle/packaging/PackagingPlugin.kt (1)
97-103: Resolveonion-graterfromrootProject, not from a fixed parent-depth walk.This assumes the plugin is always applied from a module exactly three directories below the repo root. If that layout changes,
onionGraterDirjust stays unset and the Linux package silently drops the Tails profiles.♻️ Suggested change
- val repoRoot = project.projectDir.parentFile.parentFile.parentFile - val onionGraterPath = File(repoRoot, "network/tor/tor/onion-grater") + val onionGraterPath = File(project.rootProject.projectDir, "network/tor/tor/onion-grater")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build-logic/packaging/src/main/kotlin/bisq/gradle/packaging/PackagingPlugin.kt` around lines 97 - 103, The current code in PackagingPlugin.kt resolves onion-grater by walking three parent directories from project.projectDir which can fail if module layout changes; instead resolve the path from the Gradle root project directory (rootProject.projectDir) and set onionGraterDir from File(rootProject.projectDir, "network/tor/tor/onion-grater") if it exists. Update the logic that assigns onionGraterDir (replace usage of project.projectDir.parentFile.parentFile.parentFile and onionGraterPath) to derive the repo root from rootProject.projectDir so the Linux packaging always finds the Tails/Whonix profiles regardless of the module depth.network/tor/tor/src/main/java/bisq/network/tor/UseExternalTorResolver.java (1)
27-33: Make this signature match the actual call contract.
TorServicenow passes a Whonix-or-Tails flag here, and both string inputs can benull. Renaming the boolean to reflect Tails support and annotating the nullable strings would make the override behavior and null contract explicit. As per coding guidelines "Avoid nullable values; useOptionaland@Nullableannotation where needed".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/tor/tor/src/main/java/bisq/network/tor/UseExternalTorResolver.java` around lines 27 - 33, The method signature for evaluateUseExternalTorValue must match the actual call contract: rename the boolean parameter isWhonix to a clearer name reflecting the Whonix-or-Tails flag (e.g., isWhonixOrTails) and declare the two String parameters (torSkipLaunch and useExternalTorFromConfig) as nullable (add `@Nullable`) or change them to Optional<String> to express their nullability; update any callers if you choose Optional. Keep the boolean useExternalTorFromJvmConfig as-is, preserve the early return when the Whonix-or-Tails flag is set, and update Javadoc/comments to document the new parameter meanings and null contract for evaluateUseExternalTorValue.network/tor/tor-common/src/main/java/bisq/network/tor/common/torrc/TorrcFileGenerator.java (1)
42-52: Consider phasing out theMap<String, String>overload once callers are migrated.This PR’s main behavior change is support for repeated directives, but this public overload still exposes the old lossy shape. New call sites can accidentally opt back into “one value per key” and silently lose repeatable entries like
Bridge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/tor/tor-common/src/main/java/bisq/network/tor/common/torrc/TorrcFileGenerator.java` around lines 42 - 52, The public constructor TorrcFileGenerator(Path torrcPath, Map<String,String> torrcConfigMap, Set<DirectoryAuthority> customDirectoryAuthorities) and helper mapToLines allow callers to supply a Map which loses repeated directives; phase this overload out by updating call sites to use the List<String> constructor (TorrcFileGenerator(Path, List<String>, Set<DirectoryAuthority>)) so repeatable directives like "Bridge" are preserved, then mark the Map-based constructor as `@Deprecated` (or remove it) and add a TODO comment to delete it once all callers are migrated; ensure mapToLines is removed or made private and removed alongside the deprecated constructor.network/tor/tor/src/test/java/bisq/network/tor/TorTransportConfigTorrcOverridesTest.java (1)
149-202: Consider one tab-delimited line test for parser robustness.Adding a
Key\tValuecase would lock in whitespace-delimiter behavior and prevent silent regressions.As per coding guidelines: “Add or update tests when behavior changes; keep tests deterministic and fast.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/tor/tor/src/test/java/bisq/network/tor/TorTransportConfigTorrcOverridesTest.java` around lines 149 - 202, Add a new unit test in TorTransportConfigTorrcOverridesTest that verifies TorTransportConfig.parseTorrcOverrideFile correctly treats a tab as the key/value delimiter (e.g., write a temp file line like "SocksPort\t9050" and assert result.get("SocksPort") contains "9050"); ensure the test uses `@TempDir`, mirrors existing style (skipping comments/blanks) and is deterministic/fast so it prevents regressions in whitespace-delimiter handling.network/tor/tor/src/main/java/bisq/network/tor/TorService.java (3)
368-388:readExternalTorConfigMapsilently swallows parsing errors for malformed lines.Lines without a space separator (e.g., malformed config entries) are silently ignored. Consider logging a warning for such lines to aid debugging.
📝 Suggested improvement
if (!line.isEmpty() && !line.startsWith("#")) { int firstSpaceIndex = line.indexOf(" "); if (firstSpaceIndex != -1) { String key = line.substring(0, firstSpaceIndex); String value = line.substring(firstSpaceIndex + 1); externalTorConfigMap.put(key, value); + } else { + log.debug("Skipping malformed config line (no key-value separator): '{}'", line); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/tor/tor/src/main/java/bisq/network/tor/TorService.java` around lines 368 - 388, readExternalTorConfigMap currently ignores malformed config lines that lack a space separator; update the parsing loop in readExternalTorConfigMap to log a warning when a non-empty, non-comment line does not contain a space (i.e., when firstSpaceIndex == -1) and include the offending line (and preferably activeExternalTorConfigPath or externalTorConfigPath) in the warning so callers can diagnose bad entries, while leaving valid key/value insertion into externalTorConfigMap unchanged.
448-464: Return value ofcreateTorrcConfigFileis unused.The method now returns
List<String>(line 448), but the caller at line 139 ignores the return value. Either remove the return statement or document why it's retained (e.g., for testing or future use).♻️ Option 1: Change return type to void
- private List<String> createTorrcConfigFile(Path dataDirPath, PasswordDigest hashedControlPassword) { + private void createTorrcConfigFile(Path dataDirPath, PasswordDigest hashedControlPassword) { TorrcClientConfigFactory torrcClientConfigFactory = TorrcClientConfigFactory.builder() ... torrcFileGenerator.generate(); - return torrcConfigLines; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/tor/tor/src/main/java/bisq/network/tor/TorService.java` around lines 448 - 464, The method createTorrcConfigFile currently returns a List<String> (torrcConfigLines) but callers ignore the return; change the method to return void (remove the return value and update the signature) and keep the body that builds overrides, calls TorrcFileGenerator.generate(), and constructs torrcConfigLines, or alternatively, if the returned lines are intentionally kept for tests/consumers, add a JavaDoc explaining that and ensure the caller(s) actually use the return; update all usages of createTorrcConfigFile accordingly and remove the unused local return to eliminate the dead return value.
297-300: Overly broad exception handling may obscure root causes.Catching generic
Exceptioninstead of specific exception types (TorControlAuthenticationFailed, etc.) makes it harder to diagnose authentication failures. The original specific exception handling provided clearer error categorization.♻️ Suggested improvement for clearer error handling
try { torController.authenticate(); isAuthenticated = true; - } catch (Exception e) { - log.warn("Authentication to Tor Control failed.", e); + } catch (TorControlAuthenticationFailed e) { + log.warn("Authentication to Tor Control failed: {}", e.getMessage()); + return false; + } catch (TorControlConnectionClosedException e) { + log.warn("Tor control connection closed during authentication.", e); return false; }Also applies to: 312-317, 327-330
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/tor/tor/src/main/java/bisq/network/tor/TorService.java` around lines 297 - 300, The catch blocks in TorService (around the authentication and control operations) currently catch generic Exception (e.g., in the block that logs "Authentication to Tor Control failed."), which hides specific failure causes; update these catch clauses to handle the specific checked/expected exceptions (such as TorControlAuthenticationFailed and other relevant Tor control exceptions thrown by the underlying library) instead of Exception, and keep a final catch for unexpected RuntimeException/Throwable if you need a fallback; ensure each specific catch logs the exception with context (preserve the log.warn(..., e) behavior) and return false where the original logic expects failure—apply the same change to the other similar blocks referenced (the blocks around lines 312-317 and 327-330) in TorService to maintain consistent, precise error handling.network/tor/tor/src/main/java/bisq/network/tor/ExternalTorConfigHeuristics.java (2)
44-72: Consider logging when heuristics don't apply.The early return on line 71 exits without any logging, making it harder to diagnose why external Tor was not auto-configured. A debug log would help operators understand the decision path.
📝 Suggested improvement
source = "Tails OS detected, using onion grater at '" + TAILS_ONION_GRATER_ADDRESS + "'"; } else { + log.debug("No accessible system Tor control socket found at '{}' and not running on Tails. " + + "Returning original torConfig unchanged.", systemTorControlSocketPath); return torConfig; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/tor/tor/src/main/java/bisq/network/tor/ExternalTorConfigHeuristics.java` around lines 44 - 72, In ExternalTorConfigHeuristics.apply, add a debug/process log just before the early return so operators see why heuristics didn't apply: inspect controlSocketExists, controlSocketAccessible, cookieAuthFilePath.isPresent(), and isTails and log a concise message (e.g., "No external Tor auto-configured: control socket missing/not accessible, cookie auth file missing, not Tails") including the detected values (systemTorControlSocketPath and cookieAuthFileCandidates outcome) to help diagnosis; place the log right before the branch that currently does "return torConfig" so the behavior of apply, configWithDetectedValues, controlSocketAccessible and isTails is clear in logs.
60-72: String replacement approach is fragile and should be hardened.While the template currently contains all expected placeholders (
## ControlSocket /path/to/tor/control.socket,#UseExternalTor 1,ControlPort 127.0.0.1:9051), relying on exact string matches is brittle—silent failures can occur if the template format drifts. Consider using structured config parsing (e.g., properties file format with explicit keys) or adding validation logic that ensures replacements succeeded before proceeding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/tor/tor/src/main/java/bisq/network/tor/ExternalTorConfigHeuristics.java` around lines 60 - 72, The current exact-string replacements in ExternalTorConfigHeuristics (using configWithDetectedValues.replace(...) with literals like "## ControlSocket /path/to/tor/control.socket", "#UseExternalTor 1", and "ControlPort 127.0.0.1:9051") are brittle; change to a robust approach: parse the config into key/value entries (or search for keys like ControlSocket, ControlPort, UseExternalTor) and update values using key-based replacement logic (e.g., detect and replace the ControlSocket key with systemTorControlSocketPath or set ControlPort to TAILS_ONION_GRATER_ADDRESS and enable UseExternalTor), then validate that the expected keys were present and updated (or log/return original torConfig on failure) so silent failures cannot occur when the template format drifts.network/tor/tor/src/main/java/bisq/network/tor/controller/TorControlProtocol.java (2)
306-415: Consider extracting common retry logic.Both
createAndConnectTcpControlSocketandcreateAndConnectUnixControlSocketshare identical retry/sleep/logging patterns. A shared helper could reduce duplication, though the current approach is acceptable given the different socket creation semantics.♻️ Conceptual helper approach
private <T> T connectWithRetry( String endpointDesc, SocketChannelSupplier<T> channelSupplier, SocketConnector<T> connector) throws InterruptedException, CannotConnectWithTorException { // Shared retry loop with logging }This is optional since the current implementation is clear and the duplication is limited to two methods.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/tor/tor/src/main/java/bisq/network/tor/controller/TorControlProtocol.java` around lines 306 - 415, Both createAndConnectTcpControlSocket and createAndConnectUnixControlSocket share the same retry/sleep/logging/exception-wrapping flow — extract that into a shared helper (e.g., connectWithRetry) that accepts an endpoint description string, a SocketChannel supplier (for opening the channel with the correct ProtocolFamily or path), and a connector callback that performs socketChannel.connect(address); keep the same MAX_CONNECTION_ATTEMPTS, Thread.sleep(200) retry timing, logging calls (debug/warn/info/error), ensure the helper captures lastException, closes the socketChannel on failure, wraps the final error in an IOException and throws CannotConnectWithTorException; then rewrite createAndConnectTcpControlSocket and createAndConnectUnixControlSocket to call connectWithRetry supplying the appropriate channel creation logic and connect action.
389-401:UnsupportedOperationExceptioncaught for Unix sockets.Catching
UnsupportedOperationExceptionat line 389 is prudent since Unix domain sockets may not be supported on all JVMs/platforms. However, this should likely fail fast rather than retry, as support won't suddenly appear.♻️ Suggested: Fail fast on UnsupportedOperationException
- } catch (IOException | UnsupportedOperationException e) { + } catch (UnsupportedOperationException e) { + // Unix domain sockets not supported on this platform - fail immediately + String errorMessage = "Unix domain sockets are not supported on this platform: " + controlSocketPath; + log.error(errorMessage, e); + throw new CannotConnectWithTorException(new IOException(errorMessage, e)); + } catch (IOException e) { lastException = e; log.warn("Connection attempt {} to Tor control socket {} failed: {}. Closing channel.", connectionAttempt + 1, controlSocketPath, e.getMessage());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/tor/tor/src/main/java/bisq/network/tor/controller/TorControlProtocol.java` around lines 389 - 401, The catch currently lumps UnsupportedOperationException with IOException in TorControlProtocol's connection loop, which causes futile retries when Unix sockets aren't supported; change the catch to handle UnsupportedOperationException separately in the connection attempt (the block around socketChannel open/connection in TorControlProtocol) so that on UnsupportedOperationException you close the channel if open, set lastException accordingly and immediately abort/throw (fail fast) instead of continuing retry logic, while retaining the existing retry behavior for IOExceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/desktop-app-launcher/package/linux/prepare_tails.sh`:
- Around line 60-63: The script currently uses chmod o+r on $TOR_COOKIE which
makes the cookie world-readable; instead, change ownership to the intended user
(amnesia) and restrict permissions so only that user can read the file (e.g.,
chown to amnesia and set owner-only read like chmod u+r or chmod 400). Update
the block that references TOR_COOKIE in prepare_tails.sh to replace the chmod
o+r step with commands that chown $TOR_COOKIE to amnesia and set owner-only read
permission, and keep the echo message reflecting the stricter permission change.
- Around line 43-45: The current pgrep check only warns when Bisq2 remains after
the grace period; change it to abort the script to avoid half-reconfigured state
by replacing the warning with an error exit: in the pgrep -x "Bisq2" > /dev/null
2>&1 block (the check that currently echoes "Warning: Bisq2 did not exit within
30 seconds."), print the error to stderr and call exit 1 (or another non-zero
code) so the script stops immediately when Bisq2 is still running.
In `@build-logic/packaging/src/main/kotlin/bisq/gradle/packaging/JPackageTask.kt`:
- Around line 106-135: The staging logic in stageLinuxAppContent reuses
build/packaging/app-content and leaves old onion-grater YAMLs behind; update
stageLinuxAppContent to clear the target staging directory before repopulating:
for the onion-grater case, compute onionGraterStageDir (as currently) and if it
exists recursively delete its contents (or delete and recreate the directory)
before calling Files.createDirectories(...) and copying files from
onionGraterDir.asFile.get().toPath(); ensure
contentPaths.add(stagingDir.resolve("onion-grater")) remains after the fresh
copy and preserve existing behavior for prepareTails.sh.
- Around line 127-131: Files.list(srcDir) opens a Stream<Path> that isn't
closed, leaking directory handles; wrap the stream in a try-with-resources or
Kotlin use block (e.g. Files.list(srcDir).use { ... }) so the stream is closed
after copying, and keep the existing filter { it.toString().endsWith(".yml") }
and forEach logic that copies to onionGraterStageDir to preserve behavior.
In `@network/tor/tor/onion-grater/40_bisq.yml`:
- Around line 20-24: The YAML has a mis-indentation where the replacement for
the 'status/bootstrap-phase' rule is not nested under the 'response' key; adjust
indentation so the 'replacement: \'250-status/bootstrap-phase=NOTICE BOOTSTRAP
PROGRESS=100 TAG=done SUMMARY="Done"\'' entry is a child of the corresponding
'response' block for the 'pattern: \'status/bootstrap-phase\'' rule, ensuring
'response' contains the 'pattern'/'replacement' pair rather than leaving them as
siblings.
In
`@network/tor/tor/src/main/java/bisq/network/tor/controller/TorControlReader.java`:
- Around line 88-92: The finally block in TorControlReader currently only
enqueues CONNECTION_CLOSED_SENTINEL when !isStopped, which can leave threads
blocked in replies.take() on an intentional close(); change the finally so that
replies.add(CONNECTION_CLOSED_SENTINEL) is always executed (unconditionally)
while keeping the log.warn("Tor control connection closed unexpectedly.")
guarded by !isStopped; this ensures any thread blocked on replies (used by
readLine()/replies.take()) is woken when close() is invoked while leaving the
unexpected-close log behavior unchanged.
In
`@network/tor/tor/src/main/java/bisq/network/tor/TorrcClientConfigFactory.java`:
- Around line 49-61: The torrc output order is not deterministic because
BaseTorrcGenerator.generate() returns a HashMap while torrcClientConfigLines()
in TorrcClientConfigFactory (and downstream
ClientTorrcGenerator/TestNetworkTorrcGenerator) expect insertion order; change
BaseTorrcGenerator.generate() to construct and return a LinkedHashMap
(preserving insertion order of entries as built), and ensure any other
generators that build maps (ClientTorrcGenerator, TestNetworkTorrcGenerator)
also use LinkedHashMap when creating their baseConfig so that
torrcClientConfigLines() receives an insertion-ordered Map and produces
deterministic line ordering.
In `@network/tor/tor/src/main/java/bisq/network/tor/TorTransportConfig.java`:
- Around line 106-111: The parsing in TorTransportConfig currently finds the
delimiter using trimmed.indexOf(' ') which ignores other whitespace (tabs) and
can skip valid key-value lines; change the logic that computes spaceIndex to
locate the first whitespace character (e.g., use a regex or loop to find
Character.isWhitespace(char) on trimmed) and then use that index to extract key
and value (keep the existing variables trimmed, spaceIndex, key, value and the
trimming/strip behavior for value) so tab- or multi-whitespace-delimited lines
are handled correctly.
---
Nitpick comments:
In
`@build-logic/packaging/src/main/kotlin/bisq/gradle/packaging/PackagingPlugin.kt`:
- Around line 97-103: The current code in PackagingPlugin.kt resolves
onion-grater by walking three parent directories from project.projectDir which
can fail if module layout changes; instead resolve the path from the Gradle root
project directory (rootProject.projectDir) and set onionGraterDir from
File(rootProject.projectDir, "network/tor/tor/onion-grater") if it exists.
Update the logic that assigns onionGraterDir (replace usage of
project.projectDir.parentFile.parentFile.parentFile and onionGraterPath) to
derive the repo root from rootProject.projectDir so the Linux packaging always
finds the Tails/Whonix profiles regardless of the module depth.
In
`@network/tor/tor-common/src/main/java/bisq/network/tor/common/torrc/TorrcFileGenerator.java`:
- Around line 42-52: The public constructor TorrcFileGenerator(Path torrcPath,
Map<String,String> torrcConfigMap, Set<DirectoryAuthority>
customDirectoryAuthorities) and helper mapToLines allow callers to supply a Map
which loses repeated directives; phase this overload out by updating call sites
to use the List<String> constructor (TorrcFileGenerator(Path, List<String>,
Set<DirectoryAuthority>)) so repeatable directives like "Bridge" are preserved,
then mark the Map-based constructor as `@Deprecated` (or remove it) and add a TODO
comment to delete it once all callers are migrated; ensure mapToLines is removed
or made private and removed alongside the deprecated constructor.
In
`@network/tor/tor/src/main/java/bisq/network/tor/controller/TorControlProtocol.java`:
- Around line 306-415: Both createAndConnectTcpControlSocket and
createAndConnectUnixControlSocket share the same
retry/sleep/logging/exception-wrapping flow — extract that into a shared helper
(e.g., connectWithRetry) that accepts an endpoint description string, a
SocketChannel supplier (for opening the channel with the correct ProtocolFamily
or path), and a connector callback that performs socketChannel.connect(address);
keep the same MAX_CONNECTION_ATTEMPTS, Thread.sleep(200) retry timing, logging
calls (debug/warn/info/error), ensure the helper captures lastException, closes
the socketChannel on failure, wraps the final error in an IOException and throws
CannotConnectWithTorException; then rewrite createAndConnectTcpControlSocket and
createAndConnectUnixControlSocket to call connectWithRetry supplying the
appropriate channel creation logic and connect action.
- Around line 389-401: The catch currently lumps UnsupportedOperationException
with IOException in TorControlProtocol's connection loop, which causes futile
retries when Unix sockets aren't supported; change the catch to handle
UnsupportedOperationException separately in the connection attempt (the block
around socketChannel open/connection in TorControlProtocol) so that on
UnsupportedOperationException you close the channel if open, set lastException
accordingly and immediately abort/throw (fail fast) instead of continuing retry
logic, while retaining the existing retry behavior for IOExceptions.
In
`@network/tor/tor/src/main/java/bisq/network/tor/ExternalTorConfigHeuristics.java`:
- Around line 44-72: In ExternalTorConfigHeuristics.apply, add a debug/process
log just before the early return so operators see why heuristics didn't apply:
inspect controlSocketExists, controlSocketAccessible,
cookieAuthFilePath.isPresent(), and isTails and log a concise message (e.g., "No
external Tor auto-configured: control socket missing/not accessible, cookie auth
file missing, not Tails") including the detected values
(systemTorControlSocketPath and cookieAuthFileCandidates outcome) to help
diagnosis; place the log right before the branch that currently does "return
torConfig" so the behavior of apply, configWithDetectedValues,
controlSocketAccessible and isTails is clear in logs.
- Around line 60-72: The current exact-string replacements in
ExternalTorConfigHeuristics (using configWithDetectedValues.replace(...) with
literals like "## ControlSocket /path/to/tor/control.socket", "#UseExternalTor
1", and "ControlPort 127.0.0.1:9051") are brittle; change to a robust approach:
parse the config into key/value entries (or search for keys like ControlSocket,
ControlPort, UseExternalTor) and update values using key-based replacement logic
(e.g., detect and replace the ControlSocket key with systemTorControlSocketPath
or set ControlPort to TAILS_ONION_GRATER_ADDRESS and enable UseExternalTor),
then validate that the expected keys were present and updated (or log/return
original torConfig on failure) so silent failures cannot occur when the template
format drifts.
In `@network/tor/tor/src/main/java/bisq/network/tor/TorService.java`:
- Around line 368-388: readExternalTorConfigMap currently ignores malformed
config lines that lack a space separator; update the parsing loop in
readExternalTorConfigMap to log a warning when a non-empty, non-comment line
does not contain a space (i.e., when firstSpaceIndex == -1) and include the
offending line (and preferably activeExternalTorConfigPath or
externalTorConfigPath) in the warning so callers can diagnose bad entries, while
leaving valid key/value insertion into externalTorConfigMap unchanged.
- Around line 448-464: The method createTorrcConfigFile currently returns a
List<String> (torrcConfigLines) but callers ignore the return; change the method
to return void (remove the return value and update the signature) and keep the
body that builds overrides, calls TorrcFileGenerator.generate(), and constructs
torrcConfigLines, or alternatively, if the returned lines are intentionally kept
for tests/consumers, add a JavaDoc explaining that and ensure the caller(s)
actually use the return; update all usages of createTorrcConfigFile accordingly
and remove the unused local return to eliminate the dead return value.
- Around line 297-300: The catch blocks in TorService (around the authentication
and control operations) currently catch generic Exception (e.g., in the block
that logs "Authentication to Tor Control failed."), which hides specific failure
causes; update these catch clauses to handle the specific checked/expected
exceptions (such as TorControlAuthenticationFailed and other relevant Tor
control exceptions thrown by the underlying library) instead of Exception, and
keep a final catch for unexpected RuntimeException/Throwable if you need a
fallback; ensure each specific catch logs the exception with context (preserve
the log.warn(..., e) behavior) and return false where the original logic expects
failure—apply the same change to the other similar blocks referenced (the blocks
around lines 312-317 and 327-330) in TorService to maintain consistent, precise
error handling.
In `@network/tor/tor/src/main/java/bisq/network/tor/UseExternalTorResolver.java`:
- Around line 27-33: The method signature for evaluateUseExternalTorValue must
match the actual call contract: rename the boolean parameter isWhonix to a
clearer name reflecting the Whonix-or-Tails flag (e.g., isWhonixOrTails) and
declare the two String parameters (torSkipLaunch and useExternalTorFromConfig)
as nullable (add `@Nullable`) or change them to Optional<String> to express their
nullability; update any callers if you choose Optional. Keep the boolean
useExternalTorFromJvmConfig as-is, preserve the early return when the
Whonix-or-Tails flag is set, and update Javadoc/comments to document the new
parameter meanings and null contract for evaluateUseExternalTorValue.
In
`@network/tor/tor/src/test/java/bisq/network/tor/TorTransportConfigTorrcOverridesTest.java`:
- Around line 149-202: Add a new unit test in
TorTransportConfigTorrcOverridesTest that verifies
TorTransportConfig.parseTorrcOverrideFile correctly treats a tab as the
key/value delimiter (e.g., write a temp file line like "SocksPort\t9050" and
assert result.get("SocksPort") contains "9050"); ensure the test uses `@TempDir`,
mirrors existing style (skipping comments/blanks) and is deterministic/fast so
it prevents regressions in whitespace-delimiter handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cc15ce27-4675-46bd-a186-31de4066f5dc
📒 Files selected for processing (35)
.gitignoreapps/api-app/src/main/resources/api_app.confapps/desktop/desktop-app-launcher/package/linux/prepare_tails.shapps/desktop/desktop-app/src/main/resources/desktop.confapps/node-monitor-web-app/src/main/resources/node_monitor.confapps/oracle-node-app/src/main/resources/oracle_node.confapps/seed-node-app/src/main/resources/seed_node.confbuild-logic/packaging/src/main/kotlin/bisq/gradle/packaging/JPackageTask.ktbuild-logic/packaging/src/main/kotlin/bisq/gradle/packaging/PackagingPlugin.ktbuild-logic/packaging/src/main/kotlin/bisq/gradle/packaging/jpackage/package_formats/LinuxPackages.ktcommon/src/main/java/bisq/common/application/ConfigUtil.javacommon/src/main/java/bisq/common/platform/LinuxDistribution.javacommon/src/main/java/bisq/common/util/StringUtils.javacommon/src/test/java/bisq/common/application/ConfigUtilTest.javacommon/src/test/java/bisq/common/util/StringUtilsTest.javanetwork/network/src/main/resources/tor/external_tor.confignetwork/tor/tor-common/src/main/java/bisq/network/tor/common/torrc/Torrc.javanetwork/tor/tor-common/src/main/java/bisq/network/tor/common/torrc/TorrcFileGenerator.javanetwork/tor/tor/onion-grater/40_bisq.ymlnetwork/tor/tor/onion-grater/40_bisq_tails.ymlnetwork/tor/tor/src/main/java/bisq/network/tor/ExternalTorConfigHeuristics.javanetwork/tor/tor/src/main/java/bisq/network/tor/TorService.javanetwork/tor/tor/src/main/java/bisq/network/tor/TorTransportConfig.javanetwork/tor/tor/src/main/java/bisq/network/tor/TorrcClientConfigFactory.javanetwork/tor/tor/src/main/java/bisq/network/tor/UseExternalTorResolver.javanetwork/tor/tor/src/main/java/bisq/network/tor/controller/TorControlConnectionClosedException.javanetwork/tor/tor/src/main/java/bisq/network/tor/controller/TorControlProtocol.javanetwork/tor/tor/src/main/java/bisq/network/tor/controller/TorControlReader.javanetwork/tor/tor/src/main/java/bisq/network/tor/controller/TorController.javanetwork/tor/tor/src/test/java/bisq/network/tor/ExternalTorConfigHeuristicsTest.javanetwork/tor/tor/src/test/java/bisq/network/tor/TorServiceTest.javanetwork/tor/tor/src/test/java/bisq/network/tor/TorTransportConfigTorrcOverridesTest.javanetwork/tor/tor/src/test/java/bisq/network/tor/TorrcClientConfigFactoryTest.javanetwork/tor/tor/src/test/java/bisq/network/tor/UseExternalTorResolverTest.javanetwork/tor/tor/src/test/java/bisq/network/tor/controller/TorControlProtocolTest.java
wodoro
force-pushed
the
improve_tor_override
branch
3 times, most recently
from
3605427 to
ad4777b
Compare
wodoro
force-pushed
the
improve_tor_override
branch
from
ad4777b to
7545fc5
Compare
HenrikJannsen
requested changes
Apr 13, 2026
Contributor
|
@alvasw Could you review as well? |
wodoro
force-pushed
the
improve_tor_override
branch
from
7545fc5 to
014869d
Compare
Contributor
|
@wodoro Can you look into the Ci errors? |
Contributor
Author
|
Sounds like GitHub flakiness. trigger rerun ? |
wodoro
force-pushed
the
improve_tor_override
branch
from
014869d to
68f20eb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/desktop-app-launcher/package/linux/prepare_tails.sh (1)
59-63: ⚡ Quick winEnforce ownership even when
DataDirectoryalready exists.
chown -R amnesia:amnesiaruns only inside theif [ ! -d ]block. On a rerun (or if the directory was created earlier as root), an existingDataDirectorykeeps its current ownership, soamnesiamay be unable to write to its own persistent data dir.♻️ Always create and re-assert ownership
echo "Redirect user data to Tails Persistent Storage at $DataDirectory ..." -if [ ! -d "$DataDirectory" ]; then - mkdir -p "$DataDirectory" - chown -R amnesia:amnesia "$DataDirectory" -fi +mkdir -p "$DataDirectory" +chown -R amnesia:amnesia "$DataDirectory"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/desktop-app-launcher/package/linux/prepare_tails.sh` around lines 59 - 63, The script only changes ownership of $DataDirectory when it is newly created, leaving existing directories potentially owned by root; update prepare_tails.sh so that after ensuring the directory exists (keep the mkdir -p "$DataDirectory" inside the existing block or run it unconditionally), always run chown -R amnesia:amnesia "$DataDirectory" (i.e., move or add the chown command outside/after the if [ ! -d "$DataDirectory" ] block) so ownership is re-asserted on every run and amnesia can write to the persistent storage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/desktop-app-launcher/package/linux/prepare_tails.sh`:
- Line 10: prepare_tails.sh assumes
ONION_GRATER_SRC="$BISQ_LIB_DIR/onion-grater/40_bisq_tails.yml" is present and
has two issues: ownership of DataDirectory is only fixed on creation and the
script can print “Preparations complete.” even when
/home/amnesia/.local/share/Bisq2 exists as a non-symlink and steps were skipped.
Fix prepare_tails.sh by (1) ensuring the DataDirectory ownership is enforced
regardless of existence—run a chown/chmod (e.g., chown -R amnesia:amnesia
"$DataDirectory") after any creation check or unconditionally if safe,
referencing the DataDirectory variable; (2) detect
/home/amnesia/.local/share/Bisq2 (or the variable used) being an existing
non-symlink and make the script exit non-zero or bail before printing success
(adjust the branch that currently warns+skips to set a failure flag or return
non-zero); and (3) only print “Preparations complete.” after all checks and
ownership fixes succeed (use the ONION_GRATER_SRC and BISQ_LIB_DIR variables to
locate files and gate final success message).
---
Nitpick comments:
In `@apps/desktop/desktop-app-launcher/package/linux/prepare_tails.sh`:
- Around line 59-63: The script only changes ownership of $DataDirectory when it
is newly created, leaving existing directories potentially owned by root; update
prepare_tails.sh so that after ensuring the directory exists (keep the mkdir -p
"$DataDirectory" inside the existing block or run it unconditionally), always
run chown -R amnesia:amnesia "$DataDirectory" (i.e., move or add the chown
command outside/after the if [ ! -d "$DataDirectory" ] block) so ownership is
re-asserted on every run and amnesia can write to the persistent storage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6bce9e8b-1dae-47c8-b331-61967534919e
📒 Files selected for processing (14)
.gitignoreapps/api-app/src/main/resources/api_app.confapps/desktop/desktop-app-launcher/package/linux/prepare_tails.shapps/desktop/desktop-app/src/main/resources/desktop.confapps/node-monitor-web-app/src/main/resources/node_monitor.confapps/oracle-node-app/src/main/resources/oracle_node.confapps/seed-node-app/src/main/resources/seed_node.confbuild-logic/packaging/src/main/kotlin/bisq/gradle/packaging/JPackageTask.ktbuild-logic/packaging/src/main/kotlin/bisq/gradle/packaging/PackagingPlugin.ktbuild-logic/packaging/src/main/kotlin/bisq/gradle/packaging/jpackage/package_formats/LinuxPackages.ktcommon/src/main/java/bisq/common/application/ConfigUtil.javacommon/src/main/java/bisq/common/platform/LinuxDistribution.javacommon/src/main/java/bisq/common/util/StringUtils.javacommon/src/test/java/bisq/common/application/ConfigUtilTest.java
💤 Files with no reviewable changes (4)
- common/src/main/java/bisq/common/application/ConfigUtil.java
- common/src/main/java/bisq/common/platform/LinuxDistribution.java
- common/src/test/java/bisq/common/application/ConfigUtilTest.java
- common/src/main/java/bisq/common/util/StringUtils.java
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (8)
- build-logic/packaging/src/main/kotlin/bisq/gradle/packaging/PackagingPlugin.kt
- apps/desktop/desktop-app/src/main/resources/desktop.conf
- build-logic/packaging/src/main/kotlin/bisq/gradle/packaging/jpackage/package_formats/LinuxPackages.kt
- apps/seed-node-app/src/main/resources/seed_node.conf
- apps/oracle-node-app/src/main/resources/oracle_node.conf
- apps/node-monitor-web-app/src/main/resources/node_monitor.conf
- apps/api-app/src/main/resources/api_app.conf
- build-logic/packaging/src/main/kotlin/bisq/gradle/packaging/JPackageTask.kt
wodoro
force-pushed
the
improve_tor_override
branch
from
68f20eb to
b628e99
Compare
Contributor
|
@alvasw Could you review as well? |
alvasw
requested changes
Jun 13, 2026
alvasw
left a comment
alvasw
left a comment
Contributor
There was a problem hiding this comment.
Conceptual Review
-
Bisq won't receive incoming connections even though the onion verification will pass. Bisq won't work for makers (clobbering the orderbook and increasing mediation). Tails blocks global binds and this change is missing.
Tails only allows loopback binds (see Firewall rules). -
It'd be better to parse
/etc/os-releasebecause all Linux distributions use it and it simplifies the other cases too.NAME="Tails"ID="tails"
-
Firewall hardening possible. Tor Browser, OnionCirc, Onion-Share, ... all run in isolated network namespaces and have fail-safe rules:
-
JPackageTaskcopies theonion-graterprofiles for Tails and Whonix into package.onion-graterofficially supports Bisq1.- Better only copy Tails'
onion-graterprofile
- Better only copy Tails'
-
prepare-tails.sh- Better handle directly at setup (deb install)
- Use
postinstandprerm/postrmscripts in deb to automatically install and enable Bisq's onion-grater profile on Tails. - User doesn't need to do anything except to install the deb.
- Use
- Better handle directly at setup (deb install)
-
Check at startup if persistence is activated or data directory is on external drive.
- Otherwise user loses access to Bisq, dead offers, more mediation, ...
- Tell user and abort early - otherwise lost data (ask explicit permission)
-
40_bisq_tails.yml- AppArmor profiles missing or argument is wrong.
- Check reference in Tails repo, yes onionshare does have it
- AppArmor profiles missing or argument is wrong.
-
We should mirror Tor's original override behaviour, we had compatibility issues in the past with Whonix and Tails.
Tor's man page:
Note
Configuration options on the command line override those in configuration files. See THE
CONFIGURATION FILE FORMAT for more information.
The following options in this section are only recognized on the tor command line, not in a
configuration file.
- Move integration tests from unit test module to intergration test module to speed-up build.
Code Review
- The control port and control socket code paths can be merged by using
SocketAddress
TorController:
@@ -32,16 +32,8 @@
this.bootstrapEvent = bootstrapEvent;
}
- public void initialize(int controlPort) {
- initialize("127.0.0.1", controlPort);
- }
-
- public void initialize(String controlHost, int controlPort) {
- torControlProtocol.initialize(controlHost, controlPort);
- }
-
- public void initialize(Path controlSocketPath) {
- torControlProtocol.initialize(controlSocketPath);
+ public void initialize(SocketAddress socketAddress) {
+ torControlProtocol.initialize(socketAddress);
}
public void authenticate(byte[] authCookie) {TorControlProtocol:
@@ -45,64 +38,27 @@
torControlReader = new TorControlReader();
}
- public void initialize(int port) {
- initialize("127.0.0.1", port);
- }
-
- public void initialize(String host, int port) {
+ public void initialize(SocketAddress socketAddress) {
try {
- SocketChannel socketChannel = createAndConnectTcpControlSocket(host, port);
- initializeControlConnection(socketChannel,
- Channels.newInputStream(socketChannel),
- Channels.newOutputStream(socketChannel),
- host + ":" + port);
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt(); // Restore interrupted state
- log.error("TorControlProtocol initialization for control endpoint {}:{} was interrupted.", host, port, e);
- close();
- throw new CannotConnectWithTorException(e);
- } catch (CannotConnectWithTorException e) {
- log.error("TorControlProtocol failed to connect to Tor control endpoint {}:{}.", host, port, e);
- close();
- throw e;
- } catch (Exception e) {
- log.error("Unexpected exception during TorControlProtocol initialization for control endpoint {}:{}.", host, port, e);
+ SocketChannel socketChannel = new TorControlSocketChannelFactory(socketAddress).createAndConnect();
+ initializeControlConnection(socketChannel);
+
+ } catch (Exception e) {
+ if (e instanceof InterruptedException) {
+ Thread.currentThread().interrupt(); // Restore interrupted state
+ }
+
+ log.error("TorControlProtocol initialization for control endpoint {}.", socketAddress, e);
close();
throw new CannotConnectWithTorException(e);
}
}
- public void initialize(Path controlSocketPath) {
- try {
- SocketChannel socketChannel = createAndConnectUnixControlSocket(controlSocketPath);
- initializeControlConnection(socketChannel,
- Channels.newInputStream(socketChannel),
- Channels.newOutputStream(socketChannel),
- controlSocketPath.toString());
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- log.error("TorControlProtocol initialization for control socket {} was interrupted.", controlSocketPath, e);
- close();
- throw new CannotConnectWithTorException(e);
- } catch (CannotConnectWithTorException e) {
- log.error("TorControlProtocol failed to connect to Tor control socket {}.", controlSocketPath, e);
- close();
- throw e;
- } catch (Exception e) {
- log.error("Unexpected exception during TorControlProtocol initialization for control socket {}.", controlSocketPath, e);
- close();
- throw new CannotConnectWithTorException(e);
- }
- }
-
- private void initializeControlConnection(AutoCloseable controlConnection,
- InputStream inputStream,
- OutputStream outputStream,
- String endpointDescription) {
- this.controlConnection = controlConnection;
- this.outputStream = Optional.of(outputStream);
- this.torControlReader.start(inputStream);
- log.info("TorControlProtocol initialized successfully for control endpoint {}", endpointDescription);
+ private void initializeControlConnection(SocketChannel socketChannel) throws IOException {
+ this.controlConnection = socketChannel;
+ this.outputStream = Optional.of(Channels.newOutputStream(socketChannel));
+ this.torControlReader.start(Channels.newInputStream(socketChannel));
+ log.info("TorControlProtocol initialized successfully for control endpoint {}", socketChannel.getRemoteAddress());
}
@Override
@@ -303,117 +259,6 @@
}
}
- private SocketChannel createAndConnectTcpControlSocket(String host,
- int port) throws InterruptedException, CannotConnectWithTorException {
- int connectionAttempt = 0;
- Exception lastException = null;
-
- while (connectionAttempt < MAX_CONNECTION_ATTEMPTS) {
- SocketChannel socketChannel = null;
- try {
- // The Tor Control Port communication is typically unencrypted.
- // This is considered safe because the connection is made to 127.0.0.1 (localhost),
- // ensuring that the communication does not leave the local machine and is not
- // exposed to external networks. Authentication (via cookie or password)
- // is handled by the Tor control protocol itself after connection.
- //
- // Explicitly use INET (IPv4) so the socket's local address appears as
- // "127.0.0.1" in /proc/net/tcp. Java's default NioSocketImpl creates
- // AF_INET6 dual-stack sockets, causing the local address to appear as
- // "::ffff:127.0.0.1" in /proc/net/tcp6. On Tails, onion-grater resolves
- // the client PID via psutil.net_connections() matching on local address;
- // the IPv6-mapped form does not match, so PID lookup fails and
- // onion-grater silently closes the connection.
- socketChannel = SocketChannel.open(StandardProtocolFamily.INET);
- var socketAddress = new InetSocketAddress(host, port);
- log.debug("Attempting to connect to Tor control endpoint {} ({}/{})", socketAddress, connectionAttempt + 1, MAX_CONNECTION_ATTEMPTS);
- socketChannel.connect(socketAddress);
- log.info("Successfully connected control socket to Tor endpoint {} after {} attempts", socketAddress, connectionAttempt + 1);
- return socketChannel;
- } catch (ConnectException e) {
- lastException = e;
- log.warn("ConnectException on attempt {} to Tor control endpoint {}:{}: {}. Closing channel.",
- connectionAttempt + 1, host, port, e.getMessage());
- if (socketChannel != null) {
- try {
- socketChannel.close();
- } catch (IOException closeEx) {
- log.warn("Failed to close channel after ConnectException on endpoint {}:{}: {}",
- host, port, closeEx.getMessage(), closeEx);
- }
- }
- } catch (IOException e) {
- lastException = e;
- log.warn("IOException on attempt {} to Tor control endpoint {}:{}: {}. Closing channel.",
- connectionAttempt + 1, host, port, e.getMessage());
- if (socketChannel != null) {
- try {
- socketChannel.close();
- } catch (IOException closeEx) {
- log.warn("Failed to close channel after IOException on endpoint {}:{}: {}",
- host, port, closeEx.getMessage(), closeEx);
- }
- }
- }
-
- connectionAttempt++;
- if (connectionAttempt < MAX_CONNECTION_ATTEMPTS) {
- log.debug("Connection attempt to Tor control endpoint {}:{} failed. Retrying in 200ms...", host, port);
- Thread.sleep(200);
- }
- }
-
- String errorMessage = "Failed to connect to Tor control endpoint " + host + ":" + port +
- " after " + MAX_CONNECTION_ATTEMPTS + " attempts.";
- IOException wrapperException = new IOException(errorMessage, lastException);
- log.error(errorMessage, wrapperException);
- throw new CannotConnectWithTorException(wrapperException);
- }
-
- private SocketChannel createAndConnectUnixControlSocket(Path controlSocketPath) throws InterruptedException, CannotConnectWithTorException {
- int connectionAttempt = 0;
- Exception lastException = null;
-
- while (connectionAttempt < MAX_CONNECTION_ATTEMPTS) {
- SocketChannel socketChannel = null;
- try {
- ProtocolFamily protocolFamily = StandardProtocolFamily.UNIX;
- socketChannel = SocketChannel.open(protocolFamily);
- UnixDomainSocketAddress socketAddress = UnixDomainSocketAddress.of(controlSocketPath);
- log.debug("Attempting to connect to Tor control socket {} ({}/{})",
- controlSocketPath, connectionAttempt + 1, MAX_CONNECTION_ATTEMPTS);
- socketChannel.connect(socketAddress);
- log.info("Successfully connected control socket to Tor socket {} after {} attempts",
- controlSocketPath, connectionAttempt + 1);
- return socketChannel;
- } catch (IOException | UnsupportedOperationException e) {
- lastException = e;
- log.warn("Connection attempt {} to Tor control socket {} failed: {}. Closing channel.",
- connectionAttempt + 1, controlSocketPath, e.getMessage());
- if (socketChannel != null) {
- try {
- socketChannel.close();
- } catch (IOException closeEx) {
- log.warn("Failed to close socket channel for control socket {}: {}",
- controlSocketPath, closeEx.getMessage(), closeEx);
- }
- }
- }
-
- connectionAttempt++;
- if (connectionAttempt < MAX_CONNECTION_ATTEMPTS) {
- log.debug("Connection attempt to Tor control socket {} failed. Retrying in 200ms...", controlSocketPath);
- Thread.sleep(200);
- }
- }
-
- String errorMessage = "Failed to connect to Tor control socket " + controlSocketPath +
- " after " + MAX_CONNECTION_ATTEMPTS + " attempts.";
- IOException wrapperException = new IOException(errorMessage, lastException);
- log.error(errorMessage, wrapperException);
- throw new CannotConnectWithTorException(wrapperException);
- }
-
private void sendCommand(String command) {
if (closeInProgress) {
return;@Slf4j
public class TorControlSocketChannelFactory {
private static final int MAX_CONNECTION_ATTEMPTS = 10;
private final SocketAddress socketAddress;
private int connectionAttempt;
TorControlSocketChannelFactory(SocketAddress socketAddress) {
this.socketAddress = socketAddress;
}
SocketChannel createAndConnect() throws InterruptedException {
Exception lastException = null;
while (connectionAttempt < MAX_CONNECTION_ATTEMPTS) {
SocketChannel socketChannel = null;
try {
++connectionAttempt;
socketChannel = connectToControlSocketRaw();
return socketChannel;
} catch (Exception e) {
lastException = e;
log.warn("Connection attempt {} to Tor control endpoint {} failed. Closing channel.",
connectionAttempt, socketAddress, e);
closeSocketChannel(socketChannel, socketAddress);
}
if (connectionAttempt < MAX_CONNECTION_ATTEMPTS) {
log.debug("Connection attempt to Tor control endpoint {} failed. Retrying in 200ms...", socketAddress);
//noinspection BusyWait
Thread.sleep(200);
}
}
String errorMessage = "Failed to connect to Tor control endpoint " + socketAddress +
" after " + MAX_CONNECTION_ATTEMPTS + " attempts.";
IOException wrapperException = new IOException(errorMessage, lastException);
log.error(errorMessage, wrapperException);
throw new CannotConnectWithTorException(wrapperException);
}
private SocketChannel connectToControlSocketRaw() throws IOException {
log.debug("Attempting to connect to Tor control endpoint {} ({}/{})", socketAddress, connectionAttempt, MAX_CONNECTION_ATTEMPTS);
SocketChannel socketChannel = SocketChannel.open(socketAddress);
log.info("Successfully connected control socket to Tor endpoint {} after {} attempts", socketAddress, connectionAttempt);
return socketChannel;
}
private void closeSocketChannel(SocketChannel socketChannel, SocketAddress socketAddress) {
if (socketChannel != null) {
try {
socketChannel.close();
} catch (IOException e) {
log.warn("Failed to close channel on endpoint {}", socketAddress, e);
}
}
}
}TorService:
@@ -175,7 +177,8 @@
FileMutatorUtils.deleteOnExit(controlPortFilePath);
- torController.initialize(controlPort);
+ var socketAddress = new InetSocketAddress("127.0.0.1", controlPort);
+ torController.initialize(socketAddress);
torController.authenticate(hashedControlPassword);
torController.bootstrap();
@@ -288,69 +291,55 @@
return socksProxyFactory.getSocks5Proxy(streamId);
}
+ private void doCookieAuthentication(Map<String, String> torConfigMap) {
+ String authCookiePath = StringUtils.unquote(torConfigMap.get(COOKIE_AUTH_FILE));
+ try {
+ byte[] authCookie = Files.readAllBytes(Paths.get(authCookiePath));
+ torController.authenticate(authCookie);
+
+ } catch (IOException e) {
+ throw new TorControlAuthenticationFailed("Couldn't read the COOKIE_AUTH_FILE '" +
+ authCookiePath + "' from the config.", e);
+
+ } catch (TorControlConnectionClosedException e) {
+ throw new TorControlAuthenticationFailed("Tor control connection was closed unexpectedly" +
+ " when trying to authenticate.", e);
+
+ } catch (Exception e) {
+ throw new TorControlAuthenticationFailed("Authentication to Tor Control failed.", e);
+ }
+ }
+
private boolean connectedToExternalTor(Map<String, String> torConfigMap) {
- ControlEndpoint controlEndpoint = getControlEndpoint(torConfigMap);
-
if (torController != null) {
torController.shutdown();
}
- torController = new TorController(transportConfig.getBootstrapTimeout(), transportConfig.getHsUploadTimeout(), bootstrapEvent);
+
+ torController = new TorController(transportConfig.getBootstrapTimeout(),
+ transportConfig.getHsUploadTimeout(), bootstrapEvent);
+
try {
- if (controlEndpoint instanceof UnixSocketControlEndpoint socketControlEndpoint) {
- torController.initialize(socketControlEndpoint.controlSocketPath());
- } else if (controlEndpoint instanceof TcpControlEndpoint tcpControlEndpoint) {
- torController.initialize(tcpControlEndpoint.controlHost(), tcpControlEndpoint.controlPort());
+ SocketAddress socketAddress = getControlSocketAddress(torConfigMap);
+ torController.initialize(socketAddress);
+
+ // No authentication required, but we still need to send an empty
+ // AUTHENTICATE call to be able to send control commands
+ torController.authenticate();
+
+ if (torConfigMap.containsKey(COOKIE_AUTH_FILE)) {
+ doCookieAuthentication(torConfigMap);
}
+
} catch (CannotConnectWithTorException e) {
log.warn("Could not connect to external Tor control endpoint.", e);
torController.shutdown();
torController = null;
return false;
- }
- boolean isAuthenticated = false;
- boolean noCookieAuthenticationRequired = Optional.ofNullable(torConfigMap.get(COOKIE_AUTHENTICATION)).orElse("0").equals("0");
- if (noCookieAuthenticationRequired) {
- // No authentication required, but we still need to send an empty
- // AUTHENTICATE call to be able to send control commands
- try {
- torController.authenticate();
- isAuthenticated = true;
- } catch (Exception e) {
- log.warn("Authentication to Tor Control failed.", e);
- return false;
- }
+ } catch (TorControlAuthenticationFailed e) {
+ log.warn("Authentication to Tor Control failed.", e);
+ return false;
}
-
- if (!isAuthenticated) {
- if (torConfigMap.containsKey(COOKIE_AUTH_FILE)) {
- String authCookiePath = StringUtils.unquote(torConfigMap.get(COOKIE_AUTH_FILE));
- try {
- byte[] authCookie = Files.readAllBytes(Paths.get(authCookiePath));
- torController.authenticate(authCookie);
- isAuthenticated = true;
- } catch (IOException e) {
- log.warn("Couldn't read the COOKIE_AUTH_FILE '{}' from the config.", authCookiePath);
- } catch (TorControlConnectionClosedException e) {
- log.warn("Tor control connection was closed unexpectedly when trying to authenticate.", e);
- return false; // since pipe is broken
- } catch (Exception e) {
- log.warn("Authentication to Tor Control failed.", e);
- }
- } else {
- log.warn("The user did not provide the path to the COOKIE_AUTH_FILE.");
- }
- }
-
- if (!isAuthenticated) {
- log.info("We try to call authenticate without parameters for the case that authentication is not required.");
- try {
- torController.authenticate();
- } catch (Exception e) {
- log.warn("Authentication to Tor Control failed.", e);
- return false;
- }
- }
// We do not call torController.bootstrap as we do not need to bootstrap Tor. Instead, we set the event to
// trigger the application state update.
@@ -362,11 +351,11 @@
return true;
}
- private ControlEndpoint getControlEndpoint(Map<String, String> externalTorConfigMap) {
+ private SocketAddress getControlSocketAddress(Map<String, String> externalTorConfigMap) {
String controlSocketString = externalTorConfigMap.get(CONTROL_SOCKET);
if (controlSocketString != null && !controlSocketString.isBlank()) {
Path controlSocketPath = Path.of(StringUtils.unquote(controlSocketString));
- return ControlEndpoint.forSocket(controlSocketPath);
+ return UnixDomainSocketAddress.of(controlSocketPath);
}
String controlPortString = externalTorConfigMap.get(CONTROL_PORT);
@@ -384,7 +373,7 @@
controlHost = "127.0.0.1";
controlPort = Integer.parseInt(tokens[0]);
}
- return ControlEndpoint.forPort(controlHost, controlPort);
+ return new InetSocketAddress(controlHost, controlPort);
}
private void readExternalTorConfigMap() {- private sealed interface ControlEndpoint permits TcpControlEndpoint, UnixSocketControlEndpoint {
-
- static ControlEndpoint forPort(String controlHost, int controlPort) {
- return new TcpControlEndpoint(controlHost, controlPort);
- }
-
- static ControlEndpoint forSocket(Path controlSocketPath) {
- return new UnixSocketControlEndpoint(controlSocketPath);
- }
- }
-
- private record TcpControlEndpoint(String controlHost, int controlPort) implements ControlEndpoint {
- @Override
- @NonNull
- public String toString() {
- return String.format("TcpControlEndpoint(host: %s, port: %d)", controlHost, controlPort);
- }
- }
-
- private record UnixSocketControlEndpoint(Path controlSocketPath) implements ControlEndpoint {
- @Override
- @NonNull
- public String toString() {
- return String.format("UnixSocketControlEndpoint(controlSocketPath: %s)", controlSocketPath);
- }
- }
}torConfig.lines().toList()converts aStream<String>to aList<String>without using the list (only iterating).
@@ -392,20 +381,23 @@
Path externalTorConfigPath = getExternalTorConfigPath();
activeExternalTorConfigPath = Optional.of(externalTorConfigPath);
externalTorConfigMap.clear();
- String torConfig = readExternalTorConfig(externalTorConfigPath);
- for (String line : torConfig.lines().toList()) {
- line = line.trim();
- if (!line.isEmpty() && !line.startsWith("#")) {
- int firstSpaceIndex = line.indexOf(" ");
- if (firstSpaceIndex != -1) {
- String key = line.substring(0, firstSpaceIndex);
- String value = line.substring(firstSpaceIndex + 1);
- externalTorConfigMap.put(key, value);
- } else {
- log.warn("Ignoring malformed line (no key/value separator) in external Tor config: '{}'", line);
- }
- }
- }
+
+ readExternalTorConfig(externalTorConfigPath)
+ .lines()
+ .map(String::trim)
+ .filter(line -> !line.isEmpty() && !line.startsWith("#"))
+ .forEach(line -> {
+ int firstSpaceIndex = line.indexOf(" ");
+ if (firstSpaceIndex != -1) {
+ String key = line.substring(0, firstSpaceIndex);
+ String value = line.substring(firstSpaceIndex + 1);
+ externalTorConfigMap.put(key, value);
+ } else {
+ log.warn("Ignoring malformed line (no key/value separator) in external " +
+ "Tor config: '{}'", line);
+ }
+ });
+
} catch (IOException e) {
log.warn("Could not read external tor config file.", e);
}- The torrc config map to line conversion should happen in
TorrcFileGeneratornottorrcClientConfigFactory
TorService:
@@ -485,22 +477,21 @@
torInstaller.installIfNotUpToDate();
}
- private List<String> createTorrcConfigFile(Path dataDirPath, PasswordDigest hashedControlPassword) {
+ private void createTorrcConfigFile(Path dataDirPath, PasswordDigest hashedControlPassword) {
TorrcClientConfigFactory torrcClientConfigFactory = TorrcClientConfigFactory.builder()
.isTestNetwork(transportConfig.isTestNetwork())
.dataDirPath(dataDirPath)
.hashedControlPassword(hashedControlPassword)
.build();
- Map<String, List<String>> overrides = resolveOverrides(dataDirPath);
- List<String> torrcConfigLines = torrcClientConfigFactory.torrcClientConfigLines(overrides);
+ Map<String, List<String>> torrcOverrideConfigs = resolveOverrides(dataDirPath);
+ Map<String, List<String>> torrcConfigMap = torrcClientConfigFactory.torrcClientConfigMap(torrcOverrideConfigs);
Path torrcPath = dataDirPath.resolve("torrc");
var torrcFileGenerator = new TorrcFileGenerator(torrcPath,
- torrcConfigLines,
+ torrcConfigMap,
transportConfig.getDirectoryAuthorities());
torrcFileGenerator.generate();
- return torrcConfigLines;
}
/**TorrcClientConfigFactory:
@@ -46,20 +46,11 @@
this.hashedControlPassword = hashedControlPassword;
}
- public List<String> torrcClientConfigLines(Map<String, List<String>> torrcOverrides) {
- Map<String, String> baseConfig = clientTorrcGenerator().generate();
- baseConfig.put(DISABLE_NETWORK, "1");
-
- List<String> lines = new ArrayList<>();
- baseConfig.forEach((key, value) -> {
- if (!torrcOverrides.containsKey(key)) {
- lines.add(key + " " + value);
- }
- });
- torrcOverrides.forEach((key, values) ->
- values.forEach(value -> lines.add(key + " " + value))
- );
- return lines;
+ public Map<String, List<String>> torrcClientConfigMap(Map<String, List<String>> torrcOverrides) {
+ Map<String, List<String>> torrcClientConfig = clientTorrcGenerator().generate();
+ torrcClientConfig.put(DISABLE_NETWORK, List.of("1"));
+ torrcClientConfig.putAll(torrcOverrides);
+ return torrcClientConfig;
}
TorrcConfigGenerator clientTorrcGenerator() {TorrcFileGenerator:
@@ -21,42 +21,35 @@
import java.io.IOException;
import java.nio.file.Path;
-import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Set;
public class TorrcFileGenerator {
private final Path torrcPath;
- private final List<String> torrcConfigLines;
+ private final Map<String, List<String>> torrcConfigMap;
private final Set<DirectoryAuthority> customDirectoryAuthorities;
public TorrcFileGenerator(Path torrcPath,
- List<String> torrcConfigLines,
+ Map<String, List<String>> torrcConfigMap,
Set<DirectoryAuthority> customDirectoryAuthorities) {
this.torrcPath = torrcPath;
- this.torrcConfigLines = torrcConfigLines;
+ this.torrcConfigMap = torrcConfigMap;
this.customDirectoryAuthorities = customDirectoryAuthorities;
}
- // Constructor to keep the change to List<String> smaller in scope
- public TorrcFileGenerator(Path torrcPath,
- Map<String, String> torrcConfigMap,
- Set<DirectoryAuthority> customDirectoryAuthorities) {
- this(torrcPath, mapToLines(torrcConfigMap), customDirectoryAuthorities);
- }
-
- private static List<String> mapToLines(Map<String, String> map) {
- List<String> lines = new ArrayList<>();
- map.forEach((key, value) -> lines.add(key + " " + value));
- return lines;
- }
-
public void generate() {
StringBuilder torrcStringBuilder = new StringBuilder();
- torrcConfigLines.forEach(line ->
- torrcStringBuilder.append(line).append("\n")
- );
+
+ for (Map.Entry<String, List<String>> entry : torrcConfigMap.entrySet()) {
+ String key = entry.getKey();
+ for (String value : entry.getValue()) {
+ torrcStringBuilder.append(key)
+ .append(" ")
+ .append(value)
+ .append("\n");
+ }
+ }
customDirectoryAuthorities.forEach(dirAuthority ->
torrcStringBuilder.append("DirAuthority ").append(dirAuthority.getNickname())- It's better to add multi-value support at the TorrcConfigGenerator interface level.
TorrcConfigGenerator:
@@ -17,8 +17,9 @@
package bisq.network.tor.common.torrc;
+import java.util.List;
import java.util.Map;
public interface TorrcConfigGenerator {
- Map<String, String> generate();
+ Map<String, List<String>> generate();
}BaseTorrcGenerator:
@@ -48,20 +49,20 @@
}
@Override
- public Map<String, String> generate() {
- Map<String, String> torConfigMap = new LinkedHashMap<>();
- torConfigMap.put(DATA_DIRECTORY, dataDirPath.toAbsolutePath().toString());
+ public Map<String, List<String>> generate() {
+ Map<String, List<String>> torConfigMap = new LinkedHashMap<>();
+ torConfigMap.put(DATA_DIRECTORY, List.of(dataDirPath.toAbsolutePath().toString()));
- torConfigMap.put(CONTROL_PORT, CONTROL_PORT_AUTO);
- torConfigMap.put(CONTROL_PORT_WRITE_TO_FILE, controlPortWriteFilePath.toAbsolutePath().toString());
- torConfigMap.put(HASHED_CONTROL_PASSWORD, hashedControlPassword);
+ torConfigMap.put(CONTROL_PORT, List.of(CONTROL_PORT_AUTO));
+ torConfigMap.put(CONTROL_PORT_WRITE_TO_FILE, List.of(controlPortWriteFilePath.toAbsolutePath().toString()));
+ torConfigMap.put(HASHED_CONTROL_PASSWORD, List.of(hashedControlPassword));
String logLevel = isTestNetwork ? "debug" : "notice";
torConfigMap.put("Log",
- logLevel + " file " + dataDirPath.resolve("debug.log").toAbsolutePath()
+ List.of(logLevel + " file " + dataDirPath.resolve("debug.log").toAbsolutePath())
);
- torConfigMap.put(SOCKS_PORT, SOCKS_PORT_DISABLED);
+ torConfigMap.put(SOCKS_PORT, List.of(SOCKS_PORT_DISABLED));
return torConfigMap;
}
}ClientTorrcGenerator:
@@ -33,9 +34,9 @@
}
@Override
- public Map<String, String> generate() {
- Map<String, String> torConfigMap = baseTorrcConfigGenerator.generate();
- torConfigMap.put(SOCKS_PORT, SOCKS_PORT_AUTO);
+ public Map<String, List<String>> generate() {
+ Map<String, List<String>> torConfigMap = baseTorrcConfigGenerator.generate();
+ torConfigMap.put(SOCKS_PORT, List.of(SOCKS_PORT_AUTO));
return torConfigMap;
}
}DirectoryAuthorityTorrcGenerator:
@@ -29,24 +30,24 @@
}
@Override
- public Map<String, String> generate() {
- Map<String, String> torConfigMap = baseTorrcConfigGenerator.generate();
+ public Map<String, List<String>> generate() {
+ Map<String, List<String>> torConfigMap = baseTorrcConfigGenerator.generate();
- torConfigMap.put("AuthoritativeDirectory", "1");
- torConfigMap.put("V3AuthoritativeDirectory", "1");
- torConfigMap.put("ContactInfo", "auth-" + nickname + "@test.test\n");
+ torConfigMap.put("AuthoritativeDirectory", List.of("1"));
+ torConfigMap.put("V3AuthoritativeDirectory", List.of("1"));
+ torConfigMap.put("ContactInfo", List.of("auth-" + nickname + "@test.test\n"));
- torConfigMap.put("AssumeReachable", "1");
- torConfigMap.put("TestingV3AuthInitialVotingInterval", "20");
+ torConfigMap.put("AssumeReachable", List.of("1"));
+ torConfigMap.put("TestingV3AuthInitialVotingInterval", List.of("20"));
- torConfigMap.put("TestingV3AuthInitialVoteDelay", "4");
- torConfigMap.put("TestingV3AuthInitialDistDelay", "4");
+ torConfigMap.put("TestingV3AuthInitialVoteDelay", List.of("4"));
+ torConfigMap.put("TestingV3AuthInitialDistDelay", List.of("4"));
- torConfigMap.put("V3AuthVotingInterval", "20");
- torConfigMap.put("V3AuthVoteDelay", "4");
- torConfigMap.put("V3AuthDistDelay", "4");
+ torConfigMap.put("V3AuthVotingInterval", List.of("20"));
+ torConfigMap.put("V3AuthVoteDelay", List.of("4"));
+ torConfigMap.put("V3AuthDistDelay", List.of("4"));
- torConfigMap.put("ExitPolicy", "accept *:*");
+ torConfigMap.put("ExitPolicy", List.of("accept *:*"));
return torConfigMap;RelayTorrcGenerator:
@@ -27,12 +28,12 @@
}
@Override
- public Map<String, String> generate() {
- Map<String, String> torConfigMap = baseTorrcConfigGenerator.generate();
+ public Map<String, List<String>> generate() {
+ Map<String, List<String>> torConfigMap = baseTorrcConfigGenerator.generate();
- torConfigMap.put("ExitRelay", "1");
- torConfigMap.put("ExitPolicy", "accept 127.0.0.0/8:*,accept private:*,accept *:*,reject *:*");
- torConfigMap.put("ExitPolicyRejectPrivate", "0");
+ torConfigMap.put("ExitRelay", List.of("1"));
+ torConfigMap.put("ExitPolicy", List.of("accept 127.0.0.0/8:*,accept private:*,accept *:*,reject *:*"));
+ torConfigMap.put("ExitPolicyRejectPrivate", List.of("0"));
return torConfigMap;
}TestNetworkTorrcGenerator:
@@ -40,33 +41,32 @@
private final Optional<Integer> dirPort = Optional.empty();
@Override
- public Map<String, String> generate() {
- Map<String, String> torConfigMap = baseTorrcConfigGenerator.generate();
+ public Map<String, List<String>> generate() {
+ Map<String, List<String>> torConfigMap = baseTorrcConfigGenerator.generate();
- torConfigMap.put("TestingTorNetwork", "1");
- torConfigMap.put("TestingDirAuthVoteExit", "*");
- torConfigMap.put("TestingDirAuthVoteHSDir", "*");
+ torConfigMap.put("TestingTorNetwork", List.of("1"));
+ torConfigMap.put("TestingDirAuthVoteExit", List.of("*"));
+ torConfigMap.put("TestingDirAuthVoteHSDir", List.of("*"));
- torConfigMap.put("V3AuthNIntervalsValid", "2");
- torConfigMap.put("TestingDirAuthVoteGuard", "*");
- torConfigMap.put("TestingMinExitFlagThreshold", "0");
+ torConfigMap.put("V3AuthNIntervalsValid", List.of("2"));
+ torConfigMap.put("TestingDirAuthVoteGuard", List.of("*"));
+ torConfigMap.put("TestingMinExitFlagThreshold", List.of("0"));
+ nickname.ifPresent(nickname -> torConfigMap.put("Nickname", List.of(nickname)));
+ torConfigMap.put("ShutdownWaitLength", List.of("2"));
+ torConfigMap.put("DisableDebuggerAttachment", List.of("0"));
- nickname.ifPresent(nickname -> torConfigMap.put("Nickname", nickname));
- torConfigMap.put("ShutdownWaitLength", "2");
- torConfigMap.put("DisableDebuggerAttachment", "0");
+ torConfigMap.put("ProtocolWarnings", List.of("1"));
+ torConfigMap.put("SafeLogging", List.of("0"));
+ torConfigMap.put("LogTimeGranularity", List.of("1"));
- torConfigMap.put("ProtocolWarnings", "1");
- torConfigMap.put("SafeLogging", "0");
- torConfigMap.put("LogTimeGranularity", "1");
+ orPort.ifPresent(orPort -> torConfigMap.put("OrPort", List.of(String.valueOf(orPort))));
+ torConfigMap.put("Address", List.of("127.0.0.1"));
+ torConfigMap.put("ServerDNSDetectHijacking", List.of("0"));
- orPort.ifPresent(orPort -> torConfigMap.put("OrPort", String.valueOf(orPort)));
- torConfigMap.put("Address", "127.0.0.1");
- torConfigMap.put("ServerDNSDetectHijacking", "0");
+ torConfigMap.put("ServerDNSTestAddresses", List.of(""));
- torConfigMap.put("ServerDNSTestAddresses", "");
-
- dirPort.ifPresent(dirPort -> torConfigMap.put("DirPort", String.valueOf(dirPort)));
+ dirPort.ifPresent(dirPort -> torConfigMap.put("DirPort", List.of(String.valueOf(dirPort))));
return torConfigMap;
}- The
torrcOverridesconfig options doesn't restrict nested data types.
TorTransportConfig:
@@ -51,8 +49,8 @@
(int) TimeUnit.SECONDS.toMillis(config.getInt("socketTimeout")),
config.getBoolean("testNetwork"),
parseDirectoryAuthorities(config.getList("directoryAuthorities")),
- parseTorrcOverrideConfig(config.getConfig("torrcOverrides")),
+ parseTorrcOverrideConfig(config.getObject("torrcOverrides")),
config.getInt("sendMessageThrottleTime"),
config.getInt("receiveMessageThrottleTime"),
config.getBoolean("useExternalTor")
@@ -75,49 +73,24 @@
return allDirectoryAuthorities;
}
- private static Map<String, List<String>> parseTorrcOverrideConfig(com.typesafe.config.Config torrcOverrides) {
- Map<String, List<String>> result = new LinkedHashMap<>();
- torrcOverrides.entrySet().forEach(entry -> {
- Object unwrapped = entry.getValue().unwrapped();
- if (unwrapped instanceof List<?> values) {
- List<String> stringValues = new ArrayList<>();
- values.forEach(v -> stringValues.add(String.valueOf(v)));
- result.put(entry.getKey(), stringValues);
- } else {
- result.put(entry.getKey(), List.of(String.valueOf(unwrapped)));
- }
- });
- return result;
+ private static Map<String, List<String>> parseTorrcOverrideConfig(ConfigObject torrcOverrides) {
+ return TorrcOverrideConfigParser.parse(torrcOverrides);
}TorrcOverrideConfigParser:
public class TorrcOverrideConfigParser {
static Map<String, List<String>> parse(ConfigObject torrcOverrides) {
return torrcOverrides.entrySet().stream()
.map(TorrcOverrideConfigParser::tryToParseTorrcOverrideEntry)
.filter(TorrcOverrideConfigParser::isValidTorrcOverrideEntry)
.map(TorrcOverrideConfigParser::flattenTorrcOverrideOptionalValue)
.collect(Collectors.toMap(Pair::getFirst, Pair::getSecond));
}
private static Pair<String, Optional<List<String>>> tryToParseTorrcOverrideEntry(Map.Entry<String, ConfigValue> entry) {
String key = entry.getKey();
ConfigValue configValue = entry.getValue();
Optional<List<String>> value = tryToParseBasicDataType(configValue)
.map(List::of)
.or(tryToParseObject(configValue));
return new Pair<>(key, value);
}
private static boolean isValidTorrcOverrideEntry(Pair<String, Optional<List<String>>> keyValuePair) {
Optional<List<String>> optionalValue = keyValuePair.getSecond();
return optionalValue.isPresent();
}
private static Pair<String, List<String>> flattenTorrcOverrideOptionalValue(Pair<String, Optional<List<String>>> keyValuePair) {
//noinspection OptionalGetWithoutIsPresent
return new Pair<>(keyValuePair.getFirst(), keyValuePair.getSecond().get());
}
private static Optional<String> tryToParseBasicDataType(ConfigValue configValue) {
return tryToParseNumber(configValue)
.or(tryToParseString(configValue))
.or(tryToParseBoolean(configValue));
}
private static Supplier<Optional<? extends List<String>>> tryToParseObject(ConfigValue configValue) {
return () -> {
if (configValue.valueType() == ConfigValueType.OBJECT) {
ConfigObject valueObject = (ConfigObject) configValue;
List<String> values = valueObject.values()
.stream()
.map(TorrcOverrideConfigParser::tryToParseBasicDataType)
.filter(Optional::isPresent)
.map(Optional::get)
.toList();
return Optional.of(values);
}
return Optional.empty();
};
}
private static Optional<String> tryToParseNumber(ConfigValue configValue) {
if (configValue.valueType() == ConfigValueType.NUMBER) {
int value = (int) configValue.unwrapped();
return Optional.of(String.valueOf(value));
}
return Optional.empty();
}
private static Supplier<Optional<? extends String>> tryToParseString(ConfigValue configValue) {
return () -> {
if (configValue.valueType() == ConfigValueType.STRING) {
String value = (String) configValue.unwrapped();
return Optional.of(value);
}
return Optional.empty();
};
}
private static Supplier<Optional<? extends String>> tryToParseBoolean(ConfigValue configValue) {
return () -> {
if (configValue.valueType() == ConfigValueType.BOOLEAN) {
boolean value = (boolean) configValue.unwrapped();
return Optional.of(String.valueOf(value));
}
return Optional.empty();
};
}
}- The
torrcOverrideFilePathargument is optional, so it's better to use theOptional<String>data type.
TorTransportConfig:
@@ -51,8 +49,8 @@
(int) TimeUnit.SECONDS.toMillis(config.getInt("socketTimeout")),
config.getBoolean("testNetwork"),
parseDirectoryAuthorities(config.getList("directoryAuthorities")),
- config.hasPath("torrcOverrideFilePath") ? config.getString("torrcOverrideFilePath") : "",
+ parseTorrcOverrideFilePath(dataDirPath, config),
config.getInt("sendMessageThrottleTime"),
config.getInt("receiveMessageThrottleTime"),
config.getBoolean("useExternalTor")- /**
- * Parses a torrc-style override file into a map of key → list of values.
- * Each non-blank, non-comment line is expected to have the form {@code Key Value}.
- * Repeated keys (e.g. multiple {@code Bridge} lines) accumulate into a list so that
- * all entries appear in the generated torrc.
- */
- public static Map<String, List<String>> parseTorrcOverrideFile(Path filePath) throws IOException {
- Map<String, List<String>> result = new LinkedHashMap<>();
- for (String line : Files.readAllLines(filePath)) {
- String trimmed = line.strip();
- if (trimmed.isEmpty() || trimmed.startsWith("#")) {
- continue;
- }
- int spaceIndex = -1;
- for (int i = 0; i < trimmed.length(); i++) {
- if (Character.isWhitespace(trimmed.charAt(i))) {
- spaceIndex = i;
- break;
- }
- }
- if (spaceIndex < 0) {
- continue; // bare key with no value — skip
- }
- String key = trimmed.substring(0, spaceIndex);
- String value = trimmed.substring(spaceIndex + 1).strip();
- result.computeIfAbsent(key, k -> new ArrayList<>()).add(value);
+ private static Optional<Path> parseTorrcOverrideFilePath(Path dataDirPath,
+ com.typesafe.config.Config config) {
+ if (!config.hasPath("torrcOverrideFilePath")) {
+ return Optional.empty();
+ }
+
+ String filePath = config.getString("torrcOverrideFilePath");
+ if (filePath.isBlank()) {
+ return Optional.empty();
}
- return result;
+
+ Path torrcOverridePath = Path.of(filePath);
+ return torrcOverridePath.isAbsolute() ? Optional.of(torrcOverridePath) :
+ Optional.of(dataDirPath.resolve(torrcOverridePath));
}
private static String getStringFromConfigValue(ConfigValue configValue, String key) {
@@ -136,13 +109,7 @@
private final boolean isTestNetwork;
private final Set<DirectoryAuthority> directoryAuthorities;
private final Map<String, List<String>> torrcOverrides;
- /**
- * Optional path to a torrc-style file whose entries override {@code torrcOverrides}.
- * Supports both absolute paths and paths relative to the data directory.
- * When non-empty this file takes precedence over the inline {@code torrcOverrides} map.
- * Leave empty (the default) to use {@code torrcOverrides} instead.
- */
- private final String torrcOverrideFilePath;
+ private final Optional<Path> torrcOverrideFilePath;
private final int sendMessageThrottleTime;
private final int receiveMessageThrottleTime;
private final boolean useExternalTor;
@@ -155,7 +122,7 @@
boolean isTestNetwork,
Set<DirectoryAuthority> directoryAuthorities,
Map<String, List<String>> torrcOverrides,
- String torrcOverrideFilePath,
+ Optional<Path> torrcOverrideFilePath,
int sendMessageThrottleTime,
int receiveMessageThrottleTime,
boolean useExternalTor) {TorService:
@@ -430,9 +422,9 @@
}
private Path getExternalTorConfigPath() {
- String torrcOverrideFilePath = transportConfig.getTorrcOverrideFilePath();
- if (torrcOverrideFilePath != null && !torrcOverrideFilePath.isBlank()) {
- Path overrideFilePath = resolveTorrcOverrideFilePath(transportConfig.getDataDirPath(), torrcOverrideFilePath);
+ Optional<Path> torrcOverrideFilePath = transportConfig.getTorrcOverrideFilePath();
+ if (torrcOverrideFilePath.isPresent()) {
+ Path overrideFilePath = torrcOverrideFilePath.get();
if (Files.exists(overrideFilePath)) {
return overrideFilePath;
}
@@ -485,22 +477,21 @@
torInstaller.installIfNotUpToDate();
}
- private List<String> createTorrcConfigFile(Path dataDirPath, PasswordDigest hashedControlPassword) {
+ private void createTorrcConfigFile(Path dataDirPath, PasswordDigest hashedControlPassword) {
TorrcClientConfigFactory torrcClientConfigFactory = TorrcClientConfigFactory.builder()
.isTestNetwork(transportConfig.isTestNetwork())
.dataDirPath(dataDirPath)
.hashedControlPassword(hashedControlPassword)
.build();
- Map<String, List<String>> overrides = resolveOverrides(dataDirPath);
- List<String> torrcConfigLines = torrcClientConfigFactory.torrcClientConfigLines(overrides);
+ Map<String, List<String>> torrcOverrideConfigs = resolveOverrides(dataDirPath);
+ Map<String, List<String>> torrcConfigMap = torrcClientConfigFactory.torrcClientConfigMap(torrcOverrideConfigs);
Path torrcPath = dataDirPath.resolve("torrc");
var torrcFileGenerator = new TorrcFileGenerator(torrcPath,
- torrcConfigLines,
+ torrcConfigMap,
transportConfig.getDirectoryAuthorities());
torrcFileGenerator.generate();
- return torrcConfigLines;
}
/**
@@ -510,52 +501,19 @@
* file path is empty or the file cannot be read.
*/
private Map<String, List<String>> resolveOverrides(Path dataDirPath) {
- String filePathStr = transportConfig.getTorrcOverrideFilePath();
- if (filePathStr == null || filePathStr.isBlank()) {
+ Optional<Path> torrcOverrideFilePath = transportConfig.getTorrcOverrideFilePath();
+ if (torrcOverrideFilePath.isEmpty()) {
return transportConfig.getTorrcOverrides();
}
+
+ Path overrideFilePath = torrcOverrideFilePath.get();
try {
- Path overrideFilePath = resolveTorrcOverrideFilePath(dataDirPath, filePathStr);
- return TorTransportConfig.parseTorrcOverrideFile(overrideFilePath);
+ return TorrcFileParser.parseTorrcOverrideFile(overrideFilePath);
+
} catch (IOException e) {
log.warn("Could not read torrcOverrideFilePath '{}', falling back to torrcOverrides. Error: {}",
- filePathStr, e.getMessage());
+ overrideFilePath, e.getMessage());
return transportConfig.getTorrcOverrides();
}
}
-
- private Path resolveTorrcOverrideFilePath(Path dataDirPath, String torrcOverrideFilePath) {
- Path overrideFilePath = Path.of(torrcOverrideFilePath);
- if (!overrideFilePath.isAbsolute()) {
- overrideFilePath = dataDirPath.resolve(overrideFilePath);
- }
- return overrideFilePath;
- }- Use dynamic path lookup so that Gradle handles directory setup and cross platform path lookup ("/", "\")
PackagePlugin:
val onionGraterDirectory: Directory = project.rootProject.layout
.projectDirectory
.dir("tor")
.dir("tor")
.dir("onion-grater")
onionGraterDir.set(onionGraterDirectory)-
prepare-tails.sh- Better to use
cpinstead ofrsync(cpdoes exist always)
- Better to use
-
TorControlReader: Useaddin line 92 becauseoffercan silently fail (correct in line 110)
Code Review Notes
-
Check if all .conf files updated
- ./apps/api-app/src/main/resources/api_app.conf
- ./apps/desktop/desktop-app/src/main/resources/desktop.conf
- ./apps/node-monitor-web-app/src/main/resources/node_monitor.conf
- ./apps/oracle-node-app/src/main/resources/oracle_node.conf
- ./apps/seed-node-app/src/main/resources/seed_node.conf
-
Looks outdated - ./network/network/src/integrationTest/resources/test.conf
-
ConfigUtils.parseBooleanFlag edge-cases:
- null
- leading-trailing blanks
- 1, 0, -1
- true, True, tRue
- yes, Yes, yEs
- "\u00A0true" (unicode whitespace) - fails but not important
-
StringUtils.unquote (looks good!) - edge-cases:
- null - leads to NullPointerException
- In ConfigUtils.parseBooleanFlag you used String.trim to remove blanks, you could use it here too for consistency, but not needed
- Doesn't support escapes and nesting, but not needed
-
Check official Tails marker files
- Legacy/Marker file: /etc/amnesia_version
- Recommended: /etc/os-release
- *ID=tails
- *NAME="Tails"
-
Lookup
--app-content
--app-content <additional content>[,<additional content>...]
A comma separated list of paths to files and/or directories
to add to the application payload.
This option can be used more than once.
- Check for
onion-grater-add, we officially support Whonix and Bisq's profile is integrated upstream into the OS.onion-grater-addmissing in Tails butonion-graterrunningonion-graternot inPATHbut listening on port 951? - (ss -tlpn)- find binary through
cmdlinein/proc/<pid>/cmdline- binary in
/usr/local/lib/ - cross-reference with Tails source code - https://gitlab.tails.boum.org/tails/tails/-/blob/stable/config/chroot_local-includes/usr/local/lib/onion-grater
- binary in
- find binary through
- Check included profiles: onion-share, Tor Browser, ...
- Which package installed and why Bisq missing? -
dpkg -Snot working?- Cross-reference with
ls - Check Tails source code: https://gitlab.tails.boum.org/tails/tails/-/tree/stable/config/chroot_local-includes/etc/onion-grater.d
- Cross-reference with
- Which package installed and why Bisq missing? -
Footnotes
Contributor
Author
|
Keeping explicit AF_INET. Working on the rest |
Contributor
Author
|
I have tried using the shipped profile and it doesn't work, its missing an important pattern: also using |
wodoro
added a commit
to wodoro/bisq2
that referenced
this pull request
Jun 14, 2026
Code review (network/tor): - Unify TCP and Unix control sockets behind a single SocketAddress; connection/retry logic moves to TorControlSocketChannelFactory and TorController/TorControlProtocol expose initialize(SocketAddress). - Make multi-value torrc first-class: TorrcConfigGenerator returns Map<String, List<String>>; TorrcFileGenerator consumes the map directly. TorrcClientConfigFactory exposes torrcClientConfigMap. - Extract TorrcOverrideConfigParser (HOCON, type-checked, ConfigList-aware) and TorrcFileParser; type torrcOverrideFilePath as Optional<Path>. - Stream cleanup in readExternalTorConfigMap; TorControlReader uses add(). - Move socket/service integration tests to the integrationTest source set. Tails support: - Detect Tails via the official /etc/os-release (ID=tails) marker. - Ship the onion-grater profile in the deb (fix: apps/desktop is its own Gradle build, so project.rootProject was the wrong root and the profile was silently dropped) and ship only the Tails profile. - deb postinst/prerm auto-enable the profile on Tails, preserving jpackage's templated desktop integration. - Warn with an Alert popup (continue / shut down) instead of crashing when the data directory is not on Persistent Storage; detection lives in TailsPersistenceGuard, the decision in the desktop layer. - prepare_tails.sh uses cp instead of rsync. - Resolve the onion-grater dir via Gradle's Directory DSL. Verified on Tails (kept as-is after testing): the onion-grater apparmor-profiles matcher matches the unconfined client via its exe path, and Tor calls back to 127.0.0.1 so the maker socket binds loopback (Tails blocks global binds). Co-Authored-By: alvasw <95712046+alvasw@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
wodoro
added a commit
to wodoro/bisq2
that referenced
this pull request
Jun 14, 2026
Code review (network/tor): - Unify TCP and Unix control sockets behind a single SocketAddress; connection/retry logic moves to TorControlSocketChannelFactory and TorController/TorControlProtocol expose initialize(SocketAddress). - Make multi-value torrc first-class: TorrcConfigGenerator returns Map<String, List<String>>; TorrcFileGenerator consumes the map directly. TorrcClientConfigFactory exposes torrcClientConfigMap. - Extract TorrcOverrideConfigParser (HOCON, type-checked, ConfigList-aware) and TorrcFileParser; type torrcOverrideFilePath as Optional<Path>. - Stream cleanup in readExternalTorConfigMap; TorControlReader uses add(). - Move socket/service integration tests to the integrationTest source set. Tails support: - Detect Tails via the official /etc/os-release (ID=tails) marker. - Ship the onion-grater profile in the deb (fix: apps/desktop is its own Gradle build, so project.rootProject was the wrong root and the profile was silently dropped) and ship only the Tails profile. - deb postinst/prerm auto-enable the profile on Tails, preserving jpackage's templated desktop integration. - Warn with an Alert popup (continue / shut down) instead of crashing when the data directory is not on Persistent Storage; detection lives in TailsPersistenceGuard, the decision in the desktop layer. - prepare_tails.sh uses cp instead of rsync. - Resolve the onion-grater dir via Gradle's Directory DSL. Verified on Tails (kept as-is after testing): the onion-grater apparmor-profiles matcher matches the unconfined client via its exe path, and Tor calls back to 127.0.0.1 so the maker socket binds loopback (Tails blocks global binds). Co-Authored-By: alvasw <95712046+alvasw@users.noreply.github.com>
wodoro
force-pushed
the
improve_tor_override
branch
from
9dc900a to
8538f45
Compare
Code review (network/tor): - Unify TCP and Unix control sockets behind a single SocketAddress; connection/retry logic moves to TorControlSocketChannelFactory and TorController/TorControlProtocol expose initialize(SocketAddress). - Make multi-value torrc first-class: TorrcConfigGenerator returns Map<String, List<String>>; TorrcFileGenerator consumes the map directly. TorrcClientConfigFactory exposes torrcClientConfigMap. - Extract TorrcOverrideConfigParser (HOCON, type-checked, ConfigList-aware) and TorrcFileParser; type torrcOverrideFilePath as Optional<Path>. - Stream cleanup in readExternalTorConfigMap; TorControlReader uses add(). - Move socket/service integration tests to the integrationTest source set. Tails support: - Detect Tails via the official /etc/os-release (ID=tails) marker. - Ship the onion-grater profile in the deb (fix: apps/desktop is its own Gradle build, so project.rootProject was the wrong root and the profile was silently dropped) and ship only the Tails profile. - deb postinst/prerm auto-enable the profile on Tails, preserving jpackage's templated desktop integration. - Warn with an Alert popup (continue / shut down) instead of crashing when the data directory is not on Persistent Storage; detection lives in TailsPersistenceGuard, the decision in the desktop layer. - prepare_tails.sh uses cp instead of rsync. - Resolve the onion-grater dir via Gradle's Directory DSL. Verified on Tails (kept as-is after testing): the onion-grater apparmor-profiles matcher matches the unconfined client via its exe path, and Tor calls back to 127.0.0.1 so the maker socket binds loopback (Tails blocks global binds). Co-Authored-By: alvasw <95712046+alvasw@users.noreply.github.com>
wodoro
force-pushed
the
improve_tor_override
branch
from
8538f45 to
09b049c
Compare
Contributor
Author
|
applied requested changes and rebased on main |
Contributor
Author
|
tested deb build on tails, inbound and outbound connections made successfully, offers visible, and everything seems to be working as expected |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bridgedirectives or similar thingsjava.lang.ClassCastException: class java.lang.Integer cannot be cast to class java.lang.Stringissue mentioned in Bisq 2 (v2.1.10) ignoring --torControlPort CLI flags on Tails OS #4606 (comment)torrcOverrideFilePathconfigtorrcOverrideFilePathcan now be used for configuring external tor connection detailsuseExternalTorflag UX by automatically detecting it's tail os and setting the ControlPort to 951useExternalTorflag is true, which can be very confusing otherwiseprepare_tails.shfor easier setup on tailsSummary by CodeRabbit
New Features
Documentation
Tests