-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix listchannels rpc call race condition #9849
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
base: master
Are you sure you want to change the base?
Conversation
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Need to still make sure whether notifying although the AccessManger is not active for this peer might have some side effects. |
In case the pending channel would confirm while we already restarted and the peer remained offline there was a race condition where we would not register the peer properly with the event store.
3c47ff7
to
f7e022e
Compare
Check the code and I think it's important to notify the ChannelNotifier subscriber even if the Peer is offline and hence the AccessManager would normally error out. |
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.
In case the pending channel would confirm while we already
restarted and the peer remained offline there was a race
condition where we would not register the peer properly with the
event store.
It looks like there's a bigger problem in the accessman
- if we skip the error checking here, it means when the peer comes back online, it won't be a protected peer?
@@ -4306,7 +4306,8 @@ func (s *server) notifyOpenChannelPeerEvent(op wire.OutPoint, | |||
remotePub *btcec.PublicKey) error { | |||
|
|||
// Call newOpenChan to update the access manager's maps for this peer. | |||
if err := s.peerAccessMan.newOpenChan(remotePub); err != nil { | |||
err := s.peerAccessMan.newOpenChan(remotePub) | |||
if err != nil && !errors.Is(err, ErrNoPeerScore) { |
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.
I don't think we should add exceptions here, otherwise this ErrNoErrSocre
won't be used. If it's correct that this error is redundant, we should remove it in the accessman
, otherwise we should make sure this error won't happen in the first place.
In case the pending channel would confirm while we already
restarted and the peer remained offline there was a race
condition where we would not register the peer properly with the
event store.
Run into this on regtest while testing something else:
The pending channel does not notify the event store here:
and the rpc
ListChannels
call errors out: