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(modshare): start peer manager after Syncer #2677

Merged
merged 1 commit into from
Sep 8, 2023
Merged

Conversation

Wondertan
Copy link
Member

Fixes panic:

panic: invalid type received %!s(<nil>)

goroutine 602 [running]:
github.com/celestiaorg/go-header/p2p.(*subscription[...]).NextHeader(0x103fbeaa0?, {0x103fdcd50, 0x14000723310?})
	/Users/rford/go/pkg/mod/github.com/celestiaorg/go-header@v0.3.1/p2p/subscription.go:44 +0x2ac
github.com/celestiaorg/celestia-node/share/p2p/peers.(*Manager).subscribeHeader(0x1400158c960, {0x103fdcd50, 0x14000723310}, {0x103fbe928?, 0x14000676250?})
	/Users/rford/Programming/Celestia/celestia-node/share/p2p/peers/manager.go:299 +0xc0
created by github.com/celestiaorg/celestia-node/share/p2p/peers.(*Manager).Start in goroutine 262
	/Users/rford/Programming/Celestia/celestia-node/share/p2p/peers/manager.go:180 +0x410

@Wondertan Wondertan added the kind:fix Attached to bug-fixing PRs label Sep 8, 2023
@Wondertan Wondertan self-assigned this Sep 8, 2023
@Wondertan
Copy link
Member Author

Wondertan commented Sep 8, 2023

Alternatively, we can avoid panicking there and just return an error. This will also work but has a downside that all the headers before the Verifier is registered will be dismissed, which is OK imo and up for discussion.

@codecov-commenter
Copy link

Codecov Report

Merging #2677 (803de4b) into main (b1a6349) will decrease coverage by 0.05%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2677      +/-   ##
==========================================
- Coverage   51.39%   51.34%   -0.05%     
==========================================
  Files         159      159              
  Lines       10677    10680       +3     
==========================================
- Hits         5487     5484       -3     
- Misses       4714     4720       +6     
  Partials      476      476              
Files Changed Coverage Δ
nodebuilder/share/module.go 0.00% <0.00%> (ø)

... and 4 files with indirect coverage changes

📢 Have feedback on the report? Share it here.

Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

In the past, we've done InitStore type which is annoying and bulky - and I even prefer this with a comment.

I wonder if we can do the same for InitStore

@Wondertan
Copy link
Member Author

@renaynay, iirc, we couldn't make it this way in InitStore due to some other blocker.

@renaynay
Copy link
Member

renaynay commented Sep 8, 2023

@Wondertan #2678

Copy link
Collaborator

@distractedm1nd distractedm1nd left a comment

Choose a reason for hiding this comment

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

I don't really understand how this ensures that? I thought the lifecycles are only started after everything is provided

@Wondertan
Copy link
Member Author

I don't really understand how this ensures that? I thought the lifecycles are only started after everything is provided

@distractedm1nd, exactly, and lifecycles also started in the dependency graph order, so now it's guaranteed that Syncer will start before PeerMan.
I've tested that on the reconstruction test that consistently fails locally. With this fix, its not

@Wondertan Wondertan enabled auto-merge (squash) September 8, 2023 13:09
@Wondertan Wondertan merged commit 4f047c2 into main Sep 8, 2023
17 of 19 checks passed
@Wondertan Wondertan deleted the hlib/nil-type branch September 8, 2023 13:09
renaynay pushed a commit to renaynay/celestia-node that referenced this pull request Sep 13, 2023
Fixes panic:

```
panic: invalid type received %!s(<nil>)

goroutine 602 [running]:
github.com/celestiaorg/go-header/p2p.(*subscription[...]).NextHeader(0x103fbeaa0?, {0x103fdcd50, 0x14000723310?})
	/Users/rford/go/pkg/mod/github.com/celestiaorg/go-header@v0.3.1/p2p/subscription.go:44 +0x2ac
github.com/celestiaorg/celestia-node/share/p2p/peers.(*Manager).subscribeHeader(0x1400158c960, {0x103fdcd50, 0x14000723310}, {0x103fbe928?, 0x14000676250?})
	/Users/rford/Programming/Celestia/celestia-node/share/p2p/peers/manager.go:299 +0xc0
created by github.com/celestiaorg/celestia-node/share/p2p/peers.(*Manager).Start in goroutine 262
	/Users/rford/Programming/Celestia/celestia-node/share/p2p/peers/manager.go:180 +0x410
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:fix Attached to bug-fixing PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants