Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: faster tor startup #6092

Merged
merged 7 commits into from
Jan 29, 2024

Conversation

SWvheerden
Copy link
Collaborator

@SWvheerden SWvheerden commented Jan 22, 2024

Description

This makes so that the nodes/wallets don't have to wait for tor to startup

Motivation and Context

Mobile wallets can take very long to wait for tor on startup effectivity blocking the wallet from even doing background tasks. This allows the wallets to start without an active connection

How Has This Been Tested?

manual, unit tests

@SWvheerden SWvheerden requested a review from a team as a code owner January 22, 2024 15:18
Copy link

github-actions bot commented Jan 22, 2024

Test Results (CI)

1 264 tests   1 264 ✅  16m 19s ⏱️
   39 suites      0 💤
    1 files        0 ❌

Results for commit 04a5ed6.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jan 22, 2024

Test Results (Integration tests)

29 tests  +29   29 ✅ +29   12m 29s ⏱️ + 12m 29s
11 suites +11    0 💤 ± 0 
 2 files   + 2    0 ❌ ± 0 

Results for commit 04a5ed6. ± Comparison against base commit 69421f5.

♻️ This comment has been updated with latest results.

@ghpbot-tari-project ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Jan 22, 2024
@SWvheerden SWvheerden force-pushed the st-faster-tor-startup branch from ffc987c to 497732e Compare January 22, 2024 15:56
@SWvheerden SWvheerden changed the title fix[WIP]: faster tor startup fix: faster tor startup Jan 25, 2024
@SWvheerden SWvheerden force-pushed the st-faster-tor-startup branch from 276e316 to 17e754b Compare January 25, 2024 14:15
Copy link
Collaborator

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

utACK after comments are resolved

applications/minotari_node/src/bootstrap.rs Outdated Show resolved Hide resolved
base_layer/contacts/src/chat_client/src/networking.rs Outdated Show resolved Hide resolved
base_layer/wallet/src/wallet.rs Outdated Show resolved Hide resolved
base_layer/wallet_ffi/src/lib.rs Outdated Show resolved Hide resolved
@SWvheerden SWvheerden merged commit a2872bb into tari-project:development Jan 29, 2024
13 of 14 checks passed
@SWvheerden SWvheerden mentioned this pull request Jan 29, 2024
4 tasks
@SWvheerden SWvheerden deleted the st-faster-tor-startup branch January 31, 2024 08:31
SWvheerden pushed a commit that referenced this pull request Feb 6, 2024
Description
---
Correctly sets up a tor hidden service in the HiddenServiceTransport
Tor control client will not error if a value in a key-value query is
empty, as this can be valid
Disables signal handlers for libtor

Motivation and Context
---

This PR correctly initializes a tor hidden service in the
HiddenServiceTransport. A task is spawned by `create_hidden_service`
that monitors the tor control port connection and automatically tries to
reestablish it if it disconnects. I also fix the incorrect setting of
the proxied address to be the listening port of the Tari node rather
than tor's SOCKS port.

The minor change to the control port client is minor and allows for an
empty value to be returned, which is valid when querying key-value
pairs. In practice, this never happens in our current usage but I
encountered it when debugging and it prevented the real problem from
coming through.

Ref PR #6092 

There is an additional existing problem where libtor handles interrupt
signals and exits e.g. when pressing ctrl+c in the base node to type a
command. I fixed this by [disabling signal
handlers](https://github.com/torproject/torspec/blob/8961bb4d83fccb2b987f9899ca83aa430f84ab0c/control-spec.txt#L3946)
in libtor.

How Has This Been Tested?
---
Manually

What process can a PR reviewer use to test or verify this change?
---

<!-- Checklist -->
<!-- 1. Is the title of your PR in the form that would make nice release
notes? The title, excluding the conventional commit
tag, will be included exactly as is in the CHANGELOG, so please think
about it carefully. -->
Nodes should receive inbound and outbound tor connections.

Breaking Changes
---

- [x] None
- [ ] Requires data directory on base node to be deleted
- [ ] Requires hard fork
- [ ] Other - Please specify

<!-- Does this include a breaking change? If so, include this line as a
footer -->
<!-- BREAKING CHANGE: Description what the user should do, e.g. delete a
database, resync the chain -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants