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: wait synced requires initial connected peer #1359

Conversation

mmrrnn
Copy link
Collaborator

@mmrrnn mmrrnn commented Jan 8, 2025

#1348

Description

Finish node syncing not only when initial_sync_achieved but also initial_connected_peers

Motivation and Context

Sometimes app finishes node syncing when no initial_connected_peers. It leads to a situation where we begin mining on an outdated chain. This chain is synced later, once a connection to a peer is finally established.

How Has This Been Tested?

App sometimes doesn't connect to any peer and receives initial_sync_achieved as true. Not sure how to reproduce it on demand.

@mmrrnn mmrrnn force-pushed the wait_synced_requires_initial_connected_peer branch from 75cdbb9 to 950e300 Compare January 8, 2025 10:40
@mmrrnn mmrrnn force-pushed the wait_synced_requires_initial_connected_peer branch from 950e300 to c27c357 Compare January 8, 2025 10:45
@mmrrnn mmrrnn marked this pull request as ready for review January 8, 2025 10:45
@mmrrnn mmrrnn changed the title Wait synced requires initial connected peer fix: wait synced requires initial connected peer Jan 8, 2025
@mmrrnn mmrrnn self-assigned this Jan 8, 2025
@brianp brianp added the on hold Issue is being held for progress later label Jan 10, 2025
Copy link
Collaborator

@brianp brianp left a comment

Choose a reason for hiding this comment

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

This branch looks good, but when the base node suffers any issues users get stuck on setup screen now. Which is correct behaviour, but we need some base node fixes in place before we apply this, as it will make ti seem more broken, as the errors were hidden before and seemingly all was working.

@brianp brianp removed the on hold Issue is being held for progress later label Jan 10, 2025
@brianp
Copy link
Collaborator

brianp commented Jan 10, 2025

Even with the updated base node. This branch won't finalize syncing and move away from the setup screen. Base node logs say the chain is up to date but we remain stuck on setup screen.

If I sync from main and then checkout this branch, with the updated base node I can't seem to find peers. Base node log will again confirm I do have peers and am connected. But the setup screen is not updated, and I never progress forward.

@mmrrnn
Copy link
Collaborator Author

mmrrnn commented Jan 13, 2025

I wanted to prevent from false positive initial_sync_achieved when no connections. Seems working better now, I'll test it further later today. I think the core problem is somewhere in the node itself:

=== tip_res: TipInfoResponse { metadata: Some(MetaData { best_block_height: 62287, best_block_hash: [169, 209, 208, 1, 18, 31, 35, 144, 103, 143, 95, 35, 133, 90, 144, 2, 218, 226, 233, 58, 64, 141, 1, 89, 153, 15, 123, 211, 120, 162, 96, 209], accumulated_difficulty: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 118, 211, 142, 14, 220, 39, 52, 195, 244, 70, 192, 108, 136], pruned_height: 0, timestamp: 1733631880 }), initial_sync_achieved: false, base_node_state: Listening, failed_checkpoints: false }
=== sync_progress: SyncProgressResponse { tip_height: 0, local_height: 0, state: Startup, short_desc: "Waiting for peer data: 0/3", initial_connected_peers: 0 }
=== tip: Response { metadata: MetadataMap { headers: {"content-type": "application/grpc", "date": "Wed, 08 Jan 2025 09:48:44 GMT", "grpc-status": "0"} }, message: TipInfoResponse { metadata: Some(MetaData { best_block_height: 62287, best_block_hash: [169, 209, 208, 1, 18, 31, 35, 144, 103, 143, 95, 35, 133, 90, 144, 2, 218, 226, 233, 58, 64, 141, 1, 89, 153, 15, 123, 211, 120, 162, 96, 209], accumulated_difficulty: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 118, 211, 142, 14, 220, 39, 52, 195, 244, 70, 192, 108, 136], pruned_height: 0, timestamp: 1733631880 }), initial_sync_achieved: true, base_node_state: Listening, failed_checkpoints: false }, extensions: Extensions }
=== tip_res: TipInfoResponse { metadata: Some(MetaData { best_block_height: 62287, best_block_hash: [169, 209, 208, 1, 18, 31, 35, 144, 103, 143, 95, 35, 133, 90, 144, 2, 218, 226, 233, 58, 64, 141, 1, 89, 153, 15, 123, 211, 120, 162, 96, 209], accumulated_difficulty: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 118, 211, 142, 14, 220, 39, 52, 195, 244, 70, 192, 108, 136], pruned_height: 0, timestamp: 1733631880 }), initial_sync_achieved: true, base_node_state: Listening, failed_checkpoints: false }
=== sync_progress: SyncProgressResponse { tip_height: 0, local_height: 0, state: Done, short_desc: "Listening", initial_connected_peers: 0 }
=== initial_sync_achieved

@brianp
Copy link
Collaborator

brianp commented Jan 14, 2025

Tested. It does seem to work better now. The one thing I don't like is that we still say we're waiting for 0/3 peers, then we get 1/3 peers and we move on.

Lets change the text to align with what we're doing.

Connecting to network peers... {{initial_connected_peers}} connected

Edit: Actually I am still running into the problem of not making it passed the setup screen on both sync peers, and block sync.

It only happens to me on this branch 🫣

@mmrrnn mmrrnn marked this pull request as draft January 14, 2025 14:25
@mmrrnn
Copy link
Collaborator Author

mmrrnn commented Jan 14, 2025

Coverted it to draft. We need to investigate it in the base node repo. We shouldn't receive initial_sync_achieved as true, when no connected peers

@mmrrnn mmrrnn closed this Jan 16, 2025
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.

2 participants