Confirm contact list creation during account creation #2057
Comments
alltheseas
added
bug
|
@ericholguin confirms independently an additional in person bug report |
|
Bug reporter confirmed they have only used Damus, and no other nostr apps. |
|
I am concerned that this is not a one-off, and new folks are dropping off Damus without having a chance to test it. |
|
@jb55 advises
|
alltheseas
changed the title
|
@danieldaquino @jb55 @ericholguin @kernelkind Can one of yall grab this to the current sprint? The unhappy path here is a shit experience for new damus users. We have seen an uptick in new folks, and I dont want to keep scaring them off due to bread and butter stuff not working. Thank yalls 🙏 |
|
On Sat, Mar 16, 2024 at 08:14:21PM -0700, alltheseas wrote:
@danieldaquino @jb55 @ericholguin @kernelkind
Can one of yall grab this to the current sprint? The unhappy path here is a shit experience for new damus users. We have seen an uptick, and I dont want to keep scaring them off due to bread and butter stuff not working.
Thank yalls 🙏
yeah let's add it to the sprint. I can look into it.
|
jb55
changed the title
|
fyi @ericholguin |
Feedback from bug reported when I suggested to try keys creation on stable wifi |
fwiw I've never been able to reproduce this. anyone else? |
|
I've created dozens of test accounts. Has not happened to me |
|
this is why I suspect it must have happened when the connection to the relay was down. I can't think of anything else. "stable wifi" or not. |
When you say relay, do you mean specifically damus relay? |
|
On Tue, Apr 09, 2024 at 09:35:02AM GMT, alltheseas wrote:
> this is why I suspect it must have happened when the connection to the relay was down. I can't think of anything else. "stable wifi" or not.
When you say relay, do you mean specifically damus relay?
would have to be disconnected from all relays in the bootstrap list
|
|
Feedback from new person onboarding was that the limbo profile state (no following list, no relay list) was casued by Bitwarden app interaction during onboarding. User switched view to bitwarden to save the key, and went back to Damus. When user stays in damus throughout onboarding flow it works. |
|
@danieldaquino this is the onboarding limbo ticket we discussed in today's standup |
alltheseas
removed this from the 1.9 post purple testflight, threads, longform, video caching milestone
Actually, @jb55, do we have the mechanism to subscribe to NostrDB merged into |
Regarding this item, I am more and more convinced that simply hooking up to NostrDB and making sure this contact list event is saved locally will solve most or all the problems here. Most of the solutions I have in mind for this item will likely become obsolete once we have the ability to save and read this contact list directly to/from NostrDB. On the repro side, I have good news! I was able to more reliably (3/3 tries) repro this issue (or at least something very similar) without resorting to hacky changes of code. More stable, less hacky reproDevice: iPhone 15 simulator
Steps:
Results: Symptoms are present 3 out of 3 tries |
|
@jb55 I believe my fix is still incomplete. When I go through the test (similar to the repo in the previous comment) I am now able to actually follow users and get that list persistently saved across app restarts, but I still see some weirdness like posts from those follows not showing up on the home feed. Currently investigating it |
|
(I forgot to post it here, but I sent a draft for this fix on Friday: https://groups.google.com/a/damus.io/g/patches/c/B0H8UK5HQNE) |
I fixed it! Re-testing with new version |
|
On Mon, Apr 22, 2024 at 02:37:01PM -0700, Daniel D’Aquino wrote:
> @jb55 I believe my fix is still incomplete. When I go through the test (similar to the repo in the previous comment) I am now able to actually follow users and get that list persistently saved across app restarts, but I still see some weirdness like posts from those follows not showing up on the home feed. Currently investigating it
I fixed it! Re-testing with new version
thanks!
|
|
Sent the patch! Details in https://groups.google.com/a/damus.io/g/patches/c/a8kI0CO2yOc @jb55, @alltheseas, if you don't get a chance to see the contact list First Aid in action, here is how the flow looks like:
The right-most picture is the one people without this issue will see. The first 3 pictures is what people who have the issue will see and go through (if they choose to reset). |
|
Is the first aid always a manual action? Does it help the current folks who did not have successfully formed relay lists, follow lists? During onboarding did the changes you made prevent limbo state, or reduce chance of limbo state happening in the first place? |
Yes, I made it a manual action for two reasons:
All in all, making an automated decision on this could lead to some people inadvertently losing all of their contact list, which would really really bad, so I chose to leave it a manual action to prevent this.
Yes, this First Aid option was made specifically for those folks!
Yes, I also made changes to increase robustness and prevent this issue in the first place. To summarize those changes:
|
|
Thanks Daniel for the comprehensive and thoughtful approach. |
|
On Mon, Apr 22, 2024 at 04:19:48PM GMT, Daniel D’Aquino wrote:
Sent the patch!
Details in https://groups.google.com/a/damus.io/g/patches/c/a8kI0CO2yOc
@jb55, @alltheseas, if you don't get a chance to see the contact list First Aid in action, here is how the flow looks like:
looks great!
|
|
@jb55 we forgot to push this to v1.8_relay_fix_and_video_player. Can I go ahead and cherry-pick the commits from master to that branch? |
|
On Mon, Apr 29, 2024 at 05:17:59PM -0700, Daniel D’Aquino wrote:
@jb55 we forgot to push this to [v1.8_relay_fix_and_video_player](https://github.com/damus-io/damus/commits/v1.8_relay_fix_and_video_player/).
Can I go ahead and cherry-pick the commits from master to that branch?
sure! I'm not sure what that branch is.
|
Sounds good, I will do that now and then close this ticket
That branch is the one for the 1.8 AppStore release |
This commit adds a mechanism to add the contact list to storage as soon as it is generated, and thus it reduces the risk of poor network conditions causing issues. Changelog-Fixed: Improve reliability of contact list creation during onboarding Closes: damus-io#2057 Signed-off-by: Daniel D’Aquino <[email protected]> Reviewed-by: William Casarin <[email protected]> Link: [email protected] Signed-off-by: William Casarin <[email protected]>
|
Cherry-picked the commits associated with this ticket: to the 1.8 release branch at https://github.com/damus-io/damus/commits/v1.8_relay_fix_and_video_player/ Closing this ticket! |
This commit fixes an issue where the Damus relay (Or other bootstrap relays) would be added to the user's relay list even though they explicitly removed it. The root cause of the issue lies in the way we load bootstrap relays. The default bootstrap relays would be initially loaded even though the user already has a bootstrap list stored, just in case all the relays on the user list fails. This would cause the app to inadvertently connect to relays that the user did not select whenever there is a connectivity issue with all their listed relays. The fix is to simply not add the default bootstrap list when the user already has a list stored. We do not need to use bootstrap relays in order to get our relay list, because that list is already stored in both UserDefaults as well as on NostrDB through the user's contact list event. (A contact list which is also locally loaded on startup since the fix related to damus-io#2057) Issue reproduction + Testing ---------------------------- Procedure: 1. Disconnect from all relays, and disconnect from the Damus relay last. 2. Connect to a local relay (that you control). Connection should be successful. 3. Quit the app completely. 4. Stop the local relay. 5. Restart the app. 6. Go to the relay list view. 7. Check the relay list. It should list the one local relay selected by the user Issue reproduction: - Device: iPhone 15 simulator - iOS: 17.4 - Damus: 1.8 (`97169f4fa276723bfab28ca304953ec206c904d2`) - Result: ISSUE REPRODUCED - Details: On step 7, the relay list only lists the Damus relay Fix test: - Device: iPhone 15 simulator - iOS: 17.4 - Damus: This commit - Result: PASS - Details: On step 7, the local relay is listed even though connection is unsuccessful. No notes are loaded since no relays were able to connect successfully Quick regression check ---------------------- PASS Device: iPhone 15 simulator iOS: 17.4 Damus: This commit Steps: 1. Reinstall app from scratch 2. Create a new account, go through onboarding 3. Make sure that new account connects to bootstrap relays. PASS 4. Sign out 5. Sign in with previously existing account (The one from the previous test) (Notice no UserDefaults exists for this user at that point) 6. Make sure relay list is loaded to the latest relay list known to the bootstrap relays (i.e. connects only to the Damus relay) (It cannot recover the latest relay list pointing only to the local relay, since the bootstrap relays have no knowledge about that relay or the contact lists stored there.). PASS Note: The behavior on step 6 is not a bug, it is an expected limitation. In fact, this behavior is privacy protecting, as the user may not want those public relays from knowing about its connection preference to the local relay (and its address) Other information ------------------ Q: How is this test using local relays related or equivalent to Tor relay list described in damus-io#2186? A: Those Tor relays need dedicated software (such as Orbot VPN) to be running successfully in order for Damus to make a successful connection to them. If at any moment that VPN stops working, it would trigger the same situation as described in the test above, where all relay connections fail at once. Q: In damus-io#2186, the user reports that the Damus relay is added, but does not describe the Damus relay replacing existing relays. What is the difference? A: I believe the difference is in the order in which relays are added or removed. We have to remember that the relay we just disconnected from will likely still have version N-1 of our contact list event, where it still includes itself on that list. Changelog-Fixed: Fix issue where bootstrap relays would inadvertently be added to the user's list on connectivity issues Closes: damus-io#2186 Signed-off-by: Daniel D’Aquino <[email protected]> Reviewed-by: William Casarin <[email protected]>
Unless the user signed up after changes from Github issue damus-io#2057, the contact list delegate would never be set due to a logic error, which means latest_contact_event_changed would never get called and the app would never save a local contact list reference to pull from — which caused issues when switching to different relays. Testing -------- PASS Device: iPhone 15 simulator iOS: 17.5 Setup: Manually removed UserSettingsStore::latest_contact_event_id_hex value to replicate the entry condition for the bug Steps: 1. Add new relay (relay.zap.store) 2. Remove all other relays 3. Attempt to add relay. Ensure new relay can be added 4. Remove all relays 5. Add the `wss://notify-staging.damus.io` relay (which will not save any events) 6. Restart app 7. Try to add a new relay. Ensure a new relay can be added 8. Make a test post. Ensure the new test post is posted successfully. Changelog-Fixed: Fixed some scenarios where the contact list would never be saved locally and cause issues when switching relays. Closes: damus-io#2293 Signed-off-by: Daniel D’Aquino <[email protected]>
Unless the user signed up after changes from Github issue #2057, the contact list delegate would never be set due to a logic error, which means latest_contact_event_changed would never get called and the app would never save a local contact list reference to pull from — which caused issues when switching to different relays. Testing -------- PASS Device: iPhone 15 simulator iOS: 17.5 Setup: Manually removed UserSettingsStore::latest_contact_event_id_hex value to replicate the entry condition for the bug Steps: 1. Add new relay (relay.zap.store) 2. Remove all other relays 3. Attempt to add relay. Ensure new relay can be added 4. Remove all relays 5. Add the `wss://notify-staging.damus.io` relay (which will not save any events) 6. Restart app 7. Try to add a new relay. Ensure a new relay can be added 8. Make a test post. Ensure the new test post is posted successfully. Changelog-Fixed: Fixed some scenarios where the contact list would never be saved locally and cause issues when switching relays. Closes: #2293 Signed-off-by: Daniel D’Aquino <[email protected]>
Unless the user signed up after changes from Github issue #2057, the contact list delegate would never be set due to a logic error, which means latest_contact_event_changed would never get called and the app would never save a local contact list reference to pull from — which caused issues when switching to different relays. Testing -------- PASS Device: iPhone 15 simulator iOS: 17.5 Setup: Manually removed UserSettingsStore::latest_contact_event_id_hex value to replicate the entry condition for the bug Steps: 1. Add new relay (relay.zap.store) 2. Remove all other relays 3. Attempt to add relay. Ensure new relay can be added 4. Remove all relays 5. Add the `wss://notify-staging.damus.io` relay (which will not save any events) 6. Restart app 7. Try to add a new relay. Ensure a new relay can be added 8. Make a test post. Ensure the new test post is posted successfully. Changelog-Fixed: Fixed some scenarios where the contact list would never be saved locally and cause issues when switching relays. Closes: #2293 Signed-off-by: Daniel D’Aquino <[email protected]>
solution
We should only continue onboarding if we get a contact list creation confirmation from the server.
https://damus.io/nevent1qqswkugx9lh2lye8snjxgmwl70p85qeanhe99erm49al04qa9nptsec46djng
problem observation
I’m suspecting that some unhappy path damus onboarding leads to some limbo state where there is no relay list, or contact list
I could not recreate with two new test profiles.
diagnosis
The text was updated successfully, but these errors were encountered: