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

wallet2: don't use DNS to obtain segregation heights #8408

Merged
merged 1 commit into from
Jul 6, 2022

Conversation

tobtoht
Copy link
Contributor

@tobtoht tobtoht commented Jun 27, 2022

Related: #8407

There are no other places where DNS is used in wallet2, so the --no-dns flag can be removed.

@sethforprivacy
Copy link
Contributor

Just confirming that the hard-coded DNS addresses here match what I'm seeing in packet captures when attempting to generate transactions with Cake Wallet (which uses wallet2). Thanks for the quick PR!

@selsta
Copy link
Collaborator

selsta commented Jun 28, 2022

+1 for removing the segregation heights code. Maybe it would make sense to keep the --no-dns option for now. It might be needed in the future and also functionality can be added to disable OpenAlias when --no-dns is specified, which would be nice for privacy. Obviously this would be done in a separate patch. What do you think @tobtoht?

@tobtoht
Copy link
Contributor Author

tobtoht commented Jun 28, 2022

@selsta

Suggestion adopted in the interest of getting this patch merged before the v0.18 tag. We can discuss how wallets should handle OpenAlias in a separate issue.

Copy link
Collaborator

@selsta selsta left a comment

Choose a reason for hiding this comment

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

Code reviewed and tested.

Copy link
Contributor

@jeffro256 jeffro256 left a comment

Choose a reason for hiding this comment

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

Beautiful. The fewer DNS requests made, the better everyone's privacy is

@jeffro256
Copy link
Contributor

This feature was added in #3419 by @moneromooo-monero. @moneromooo-monero what are your thoughts about this PR?

@selsta
Copy link
Collaborator

selsta commented Jun 30, 2022

@jeffro256 There are no TXT records added to e.g. segheights.moneropulse.org so I don't think this was ever used. Back in 2018 there was this MoneroV fork so that's why this got added, see for example:

https://monero.stackexchange.com/questions/7826/how-can-individuals-safeguard-themselves-and-the-community-against-a-key-reusing

If this is ever needed in the future and also if this is deemed as the right approach we can trivially revert it and bring it back in a point release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants