network: Ignore invalid tags #4517
Conversation
network/wsPeer.go
Outdated
| case protocol.TxnTag: | ||
| case protocol.UniCatchupReqTag: | ||
| case protocol.UniEnsBlockReqTag: | ||
| case protocol.VoteBundleTag: |
There was a problem hiding this comment.
we need that exhaustive switch statement linter
There was a problem hiding this comment.
that would be cool, but these tags are not defined in an enum... idk if there is something to lint switching on const Tag variable assignments
There was a problem hiding this comment.
let me see if I can put a test together
There was a problem hiding this comment.
Added a test for this, TestPeerReadLoopSwitchAllTags
Codecov Report
@@ Coverage Diff @@
## master #4517 +/- ##
==========================================
+ Coverage 53.62% 53.65% +0.02%
==========================================
Files 432 432
Lines 54057 54063 +6
==========================================
+ Hits 28990 29005 +15
+ Misses 22821 22813 -8
+ Partials 2246 2245 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
cce
force-pushed
the
ignore-invalid-tags
branch
from
79d7554
to
0c6f169
Compare
cce
force-pushed
the
ignore-invalid-tags
branch
from
1ae1b6d
to
c91c1a4
Compare
| } | ||
|
|
||
| func getProtocolTags(t *testing.T) []string { | ||
| file := filepath.Join("../protocol", "tags.go") |
There was a problem hiding this comment.
does is work for both go test ./network and (cd ./network && go test ./) ?
There was a problem hiding this comment.
I read in this blog post that go test sets the working directory to the package where the test is running even in these cases, so you can load up test data from local files/directories that are also in the package directory... it seems to be working from CI which is running from the base go-algorand dir
| @@ -53,6 +53,11 @@ import ( | |||
|
|
|||
| const sendBufferLength = 1000 | |||
|
|
|||
| func init() { | |||
| // this allows test code to use out-of-protocol message tags and have them go through | |||
| allowCustomTags = true | |||
There was a problem hiding this comment.
Are we confident that go will never load this test package in production?
Why not keep this false, and whichever test that needs this feature, can set it to true then false, and not run in parallel.
There was a problem hiding this comment.
Test code can't be compiled into non-test packages, so it is safe.. but it is true that to parallelize the network tests we'd need to track down which ones needed this flag set. When I did this a while ago I think there were some tests that failed and so I didn't want to track down exactly which ones.
Summary
Ignore messages with invalid tags before they hit the readBuffer queue, and add metrics for these occurrences.
Also removes unused message tag
UniCatchupReqTag(deprecated by @algonautshant in #1916)Test Plan