-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Backup, restore and create a new onion address via GUI #3044
Conversation
If I make a backup of first HS and later restore that I have 3 HS where first and third are the same but have diff. dir names (fist is '0'). Leads to a timeout then... I think very first HS should also use timestamp as directory name. And seems handling of the original in case of restore is not correct. |
On OSX there is a .DS_Store file and caused an exception: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NACK
desktop/src/main/java/bisq/desktop/main/settings/network/NetworkSettingsView.java
Show resolved
Hide resolved
desktop/src/main/java/bisq/desktop/main/settings/network/NetworkSettingsView.java
Outdated
Show resolved
Hide resolved
is fixed in b4818d8
that is a bug. onto it... |
@freimair The build of your last commit failed on travis. |
@ripcurlx yes I know and I have not figured out why suddenly, the desktop app tests start to fail. I am on to adding tests and get to the ground of that. Update: Travis tests the merge (that is what surprised me). Recently, the test failing was introduced in master -> that is why it only recently showed up. The code works fine but the test fails as soon as something concerned about the TradeManager is at stake. Looking into resolving that circular dependency... |
do you want me to change something? |
No, I'll review the PR asap. |
@freimair Can you add the code part for correctly filtering the users node address I sent you in the patch? It is same code as used in arbitration and it maked the list containing only the correct addresses even if wrong addressed would not have any effect in your use case. |
@ManfredKarrer Is there something still open after the last commit from @freimair? |
I don't have time for reviewing again and testing. Also I think @freimair did not implemented the suggestions I made yet. Maybe better to keep it for later as it carries high risk. |
efa257d
to
0ffe563
Compare
Sorry for being very late to the party :( This is very loosely related to the PR but maybe will be helpful for the future. IMO all secret keys should be derived from the Bitcoin seed. This includes the private key behind the onion address (or many of them if we want to support that). This would entirely mitigate the need for multiple separate backups we now expect from the user. It would also make Bisq inline with what Bitcoin users expect from wallets software. To large extent the standards for this are ready as BIP-39 (+family) and SLIP-s. |
0ffe563
to
8b08774
Compare
is there still something I should change? |
The filed inside the zip are executable, original files not.
Original:
|
Tested again backup and restore with app with no offers or trades open. 7jkfjawas3o3vu7k was original address. Backed it up, the renewed it. trkdvguyptjajozb was the new one. The I restored 7jkfjawas3o3vu7k. ################################################################ ################################################################ |
NACK |
ad backup and restore: that is expected. unused hidden services are cleaned up once a trade is complete and only after a restart of the application. as for the executable flags: I do not see this behavior on my linux machine... onto it. EDIT: zip by itself only stores files, no attributes. There are additional headers for storing such things but whether they are used and read properly strongly depends on the creator/reader software. I have seen different zip file viewers show different file mods on the same zip file. Hence, there is not a whole lot one can do against that. Besides, it really does not matter here, as the files in the zip-file are text only. |
is there anything else I should change? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NACK.
Based on my prior comment requiring validation during restore.
@@ -1073,6 +1073,15 @@ setting.preferences.dao.fullNodeInfo.cancel=No, I stick with lite node mode | |||
settings.net.btcHeader=Bitcoin network | |||
settings.net.p2pHeader=Bisq network | |||
settings.net.onionAddressLabel=My onion address | |||
settings.net.renewAddressButton=Create new address | |||
settings.net.renewAddress=Please be aware that you will loose your reputation when you create and use a new onion address. Furthermore, you need to restart the application to set your new onion address active.\n\nProceed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
settings.net.renewAddress=Please be aware that you will loose your reputation when you create and use a new onion address. Furthermore, you need to restart the application to set your new onion address active.\n\nProceed? | |
settings.net.renewAddress=Please be aware that you will lose your reputation when you create and use a new onion address. Furthermore, you need to restart the application to set your new onion address active.\n\nProceed? |
settings.net.exportAddressFileEnding=Bisq Onion Address Backup (*.bisq) | ||
settings.net.importAddressButton=Restore address | ||
settings.net.importAddressFileDialog=Select the backup file | ||
settings.net.importAddress=Please be aware that you will loose your reputation when you restore a previous onion address (but restore your reputation connected to the restored onion address). Furthermore, you need to restart the application to set your new onion address active. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
settings.net.importAddress=Please be aware that you will loose your reputation when you restore a previous onion address (but restore your reputation connected to the restored onion address). Furthermore, you need to restart the application to set your new onion address active. | |
settings.net.importAddress=Please be aware that you will lose your reputation when you restore a previous onion address (but restore your reputation connected to the restored onion address). Furthermore, you need to restart the application to set your new onion address active. |
@@ -1073,6 +1073,15 @@ setting.preferences.dao.fullNodeInfo.cancel=No, I stick with lite node mode | |||
settings.net.btcHeader=Bitcoin network | |||
settings.net.p2pHeader=Bisq network | |||
settings.net.onionAddressLabel=My onion address | |||
settings.net.renewAddressButton=Create new address | |||
settings.net.renewAddress=Please be aware that you will loose your reputation when you create and use a new onion address. Furthermore, you need to restart the application to set your new onion address active.\n\nProceed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should emphasize the significance of this action. Although it wont remove the old address immediately if there are open offers/trades, they will not be able to back up the old address after creating a new address. So perhaps highly recommend that they backup prior to creating a new address if there are open trades/offers or perhaps even if there are closed trades (just in case).
settings.net.exportAddressFileEnding=Bisq Onion Address Backup (*.bisq) | ||
settings.net.importAddressButton=Restore address | ||
settings.net.importAddressFileDialog=Select the backup file | ||
settings.net.importAddress=Please be aware that you will loose your reputation when you restore a previous onion address (but restore your reputation connected to the restored onion address). Furthermore, you need to restart the application to set your new onion address active. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned with renew address, perhaps we should emphasize the significance of this action as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NACK - Could you please address #3044 (review)? Thanks!
@freimair I activated yesterday codacy's PR Quality Review for the Bisq repository. It found for your PR a couple of issues which you can view in the Details link next to the PR check. Please review and push fixes for this issues to trigger a re-review by codacy. Thanks! |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This issue has been automatically closed because of inactivity. Feel free to reopen it if you think it is still relevant. |
this PR is postponed. it might be reactivated later. see bisq-network/projects#23 |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This issue has been automatically closed because of inactivity. Feel free to reopen it if you think it is still relevant. |
This PR adds features that allow the user to
all from within the GUI.
How do you make sure that there are no more open offers or even ongoing trades before you import/renew the address?
I don't. I have reworked the P2P backend so that a single client can have multiple hidden services up and running at any time. If the user renews/imports an onion address, a new hidden service is started. Hidden services are only removed once no more open offers or ongoing trades are there (checked for after every completed trade).
All during runtime?
No. A restart is required for the import and renew features.
Contains DE translations as well.
Here are some impressions:
Expected behaviour (for testers)
closes #1055, closes #1056, subsequently closes #909 and does take care of a lot of preparation work for #2873.