Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@svyatonik
Copy link
Contributor

There are 3 issues currently (each one takes its own commit):

  • the main issue that causes (almost) instant disconnect is that consensus gossip messages are sent to light nodes && light nodes do not have gossip listeners registered. I've fixed that by ignoring light nodes in ConsensusGossip - think that is the correct way;
  • the second issue is a typo in LightCallExecutor::version - instead of "version" it should call "Core_version". Currently aura verifier calls this - that should be fixed by Light GRANDPA import handler #1669 ;
  • the third one is the most complex, linked to the peer reputation system. The initial node reputation is zero. Normally, when full nodes are connected to each other, they first interchanging with GRANDPA messages => their reputation is increased. But because of (first issue) above, we do not send gossip messages to light nodes. And when some block is announced, reputation of the peer is changed by -4 here. This leads to the situation when node has negative reputation and we disconnect it here. @tomaka I need your help here - there are several ways to fix this, but I'm not sure what's better. Also - comment to BLOCK_ANNOUNCE_REPUTATION_CHANGE states that 'Since this has a small cost, we decrease the reputation of the node, and will increase it back later if the import is successful.' - I haven't found that. Could you, please, point me to that code? Probably it isn't executed on light, because I have seen on-announce-disconnects even after peer has provided us with several valid blocks.

Adding suggested reviewers to the PR (skipping Rob :) ).
Cc @andresilva and @niklasad1 (I'm not sure, but the second issue could be the issue you have found today as well).

@svyatonik svyatonik added A3-in_progress Pull request is in progress. No review needed at this stage. M4-core labels May 7, 2019
@svyatonik svyatonik requested review from tomaka and tomusdrw May 7, 2019 16:01
@tomaka
Copy link
Contributor

tomaka commented May 7, 2019

@tomaka I need your help here - there are several ways to fix this, but I'm not sure what's better.

Good point, I think that's missing actually.
#2440 would fix the instant disconnect, but the reputation added back on import is indeed missing I think.

@svyatonik
Copy link
Contributor Author

Decided to remove penalty from on_announce_block at all, after chatting with Pierre. The reputation will slowly decay towards zero after #2440 anyway.

@svyatonik svyatonik added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels May 8, 2019
@gavofyork gavofyork merged commit c83c4eb into master May 8, 2019
@gavofyork gavofyork deleted the fix_light_connection_issues branch May 8, 2019 10:24
MTDK1 pushed a commit to bdevux/substrate that referenced this pull request Jul 10, 2019
* ignore light nodes in ConsensusGossip

* fixed method name

* temporary disabled penalty when block is announced

* remove traces of BLOCK_ANNOUNCE_REPUTATION_CHANGE
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants