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: wallet reuse existing tor address #5092

Merged

Conversation

SWvheerden
Copy link
Collaborator

@SWvheerden SWvheerden commented Jan 9, 2023

Description

Fixes wallet to reuse existing tor address rather than create a new one each time it restarts

Motivation and Context

A wallet should not create a new tor address on startup, but rather reuse the existing ones.

How Has This Been Tested?

Manual

Fixes: #5089

@CjS77 CjS77 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 9, 2023
@stringhandler stringhandler added the P-controversial Process - This PR or Issue is controversial and/or requires more attention that simpler issues label Jan 10, 2023
@stringhandler
Copy link
Collaborator

Labelling this as controversial. The fix is simple enough, but I have two concerns:

  1. This is a very easy mistake to make. There is clearly an unintended consequence in cloning, so there are possibly other places where this "bug" could occur. There is no indication to the developer of the correct way of doing this. I would like to look further into the cause
  2. More importantly, a change in address should not affect the network. In fact, there should be a cucumber test checking this. There seems to be something missing in the comms code that would handle this case

@SWvheerden
Copy link
Collaborator Author

SWvheerden commented Jan 10, 2023

As to the second point.
Yeah, we need to handle this better but the problem is if both clients have the wrong address, they have no way of knowing that the address is wrong. All they know is that the address they have does not respond to any dial request.

If only 1 client has the wrong address, then the client should update it and fix it with the incoming dial.

Wallet could still send transactions as all transactions went through SAF, and SAF does not use the tor address for direct communication.

@SWvheerden SWvheerden force-pushed the sw_fix_wallet_tor_id_reset branch 2 times, most recently from 72c573d to 69e7ef7 Compare January 16, 2023 05:55
@SWvheerden SWvheerden force-pushed the sw_fix_wallet_tor_id_reset branch from 69e7ef7 to 111342e Compare January 19, 2023 10:03
@stringhandler
Copy link
Collaborator

Added a bit more resilience in #5142 to allow multiple addresses. Tested with a with a wallet turning off and on, getting a new tor address and it came online in time

@stringhandler
Copy link
Collaborator

I'd like to merge #5142 first, since having this bug makes it easier to test

@stringhandler stringhandler merged commit 576f44e into tari-project:development Mar 7, 2023
stringhandler pushed a commit that referenced this pull request Mar 8, 2023
### ⚠ BREAKING CHANGES

* **peer_db:** more accurate peer stats per address (5142)
* use consensus hashing API for validator node MMR (5207)
* **consensus:** add balanced binary merkle tree (5189)

### Features

* add favourite flag to contact ([5217](#5217)) ([0371b60](0371b60))
* add indexer config ([5210](#5210)) ([cf95601](cf95601))
* add merge proof for balanced binary merkle tree ([5193](#5193)) ([8962909](8962909))
* **consensus:** add balanced binary merkle tree ([5189](#5189)) ([8d34e8a](8d34e8a))
* log to base dir ([#5197](#5197)) ([5147b5c](5147b5c))
* **peer_db:** more accurate peer stats per address ([5142](#5142)) ([fdad1c6](fdad1c6))


### Bug Fixes

* add grpc commitment signature proto type ([5200](#5200)) ([d523f1e](d523f1e))
* peer seeds for esme/igor ([5202](#5202)) ([1bc226c](1bc226c))
* remove panics from merged BBMT verification ([5221](#5221)) ([a4c5fce](a4c5fce))
* source coverage ci failure ([5209](#5209)) ([80294a1](80294a1))
* use consensus hashing API for validator node MMR ([5207](#5207)) ([de28115](de28115))
* wallet reuse existing tor address ([5092](#5092)) ([576f44e](576f44e))
* **wallet:** avoids empty addresses in node identity ([5224](#5224)) ([1a66312](1a66312))
@SWvheerden SWvheerden deleted the sw_fix_wallet_tor_id_reset branch March 15, 2023 08:32
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-controversial Process - This PR or Issue is controversial and/or requires more attention that simpler issues 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.

[Wallet] on restart changes onion address
3 participants