chore(gossipsub): cleanups #1096
Conversation
|
Should we use the conventional commits style for the PR's names? |
|
Yes. Good point. I'll edit. (Main application for conventional commits though are the commit messages themselves, since they are used for compiling the version summary. |
* add some docs * add missing penalty for invalid messages * simplify code * remove unused gossipsub fields
kaiserd
force-pushed
the
chore/gossipsub-cleanups
branch
from
ffaa698
to
5acaf9f
Compare
|
I believe the PR title is used as the commit message that will end up in the master branch when we squash and merge the PR. |
|
Oh, yes, true. I sometimes add more sub-points to it. |
| proc handleSubscribe(f: FloodSub, | ||
| peer: PubSubPeer, | ||
| topic: string, | ||
| subscribe: bool) = |
There was a problem hiding this comment.
it might be better to follow this pattern https://github.com/vacp2p/nim-libp2p/pull/1095/files#diff-94a6536a89bc744b23b27b6d4577eee57ed6e4ed0b1f13f077b9db22c16ae8b8R280-R284. @arnetheduck how are you formatting it? Let's add it to the readme so we all share the same formatting rules.
There was a problem hiding this comment.
my preference is to format code with https://github.com/arnetheduck/nph which broadly follows https://status-im.github.io/nim-style-guide/formatting.style.html
| var rpcMsg = decodeRpcMsg(data).valueOr: | ||
| debug "failed to decode msg from peer", peer, err = error | ||
| raise newException(CatchableError, "") | ||
| raise newException(CatchableError, "Peer msg couldn't be decoded") |
There was a problem hiding this comment.
I believe we should either log and discard the exception or create a sequence Parent/Child that gets logged only once. Unfortunately, this seems not possible in Nim. I created this issue some time ago, but it was closed without a solution nim-lang/Nim#20677. @arnetheduck thoughts?
There was a problem hiding this comment.
you can: (ref CatchableError)(mgs: "test", parent: otherException) - when catching, you can use the parent field to print all parents as well - this latter thing does not exist but the core support in nim is there.
|
|
||
| g.replenishFanout(topic) # Make sure fanout is populated |
There was a problem hiding this comment.
Why has the replenishFanout condition been modified?
There was a problem hiding this comment.
the same condition is checked inside of replenishFanout already
| fanoutPeers = g.fanout.getOrDefault(topic).toSeq() | ||
| elif peers.len < g.parameters.dLow: | ||
| # not subscribed or bad mesh, send to fanout peers | ||
| # when flood-publishing, fanout won't help since all potential peers have |
There was a problem hiding this comment.
not sure this is true, I guess only if we assume that peers.len < g.parameters.dLow is always false when flood publishing is enabled even with the limit of peers to flood
There was a problem hiding this comment.
when flood publishing is turned on, all possible peers will already have been added to the possible peers to send to - ie everyone that is subscribed gets a message, so there's no point to try again with fanout peers (which are a subset)
|
LGTM. |
factored out from: #1092