thor icon indicating copy to clipboard operation
thor copied to clipboard

Cleanup p2p thor + add tests

Open otherview opened this issue 9 months ago • 1 comments

Description

Connected with https://github.com/vechain/protocol-board-repo/issues/165 . It was hard to follow the expected behaviour when combining (allowed-peers, bootnodes, and stored cached peers). This was mainly due to the use of flag value retrieval, directory setup, and a few other operations being mixed with the flag operation behaviour. The PR decouples the setup of data and directories from the flag combination logic. It was also very hard to test. This PR does a bit of refactoring around the p2pcom and makes it more testable. The logic now works as follows:

  • If no flag is set, load fallbackbootnodes and try to load cachedPeers, append these two.
  • if bootnodes is set, ignore fallbackbootnodes and try to load cachedPeers, append these two.
  • If allowedPeers is set, ignore bootnodes, fallbackbootnodes and cachedPeers.
  • Never allow for nodes to be duplicated when appending.

This PR also introduces a test/datagen package that makes it easier to generate randomized data.

Type of change

Please delete options that are not relevant.

  • [x] Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • [x] Unit tests

Checklist:

  • [x] My code follows the style guidelines of this project
  • [x] I have performed a self-review of my code
  • [x] I have commented my code, particularly in hard-to-understand areas
  • [x] I have made corresponding changes to the documentation
  • [x] My changes generate no new warnings
  • [x] I have added tests that prove my fix is effective or that my feature works
  • [x] New and existing unit tests pass locally with my changes
  • [x] New and existing E2E tests pass locally with my changes
  • [x] Any dependent changes have been merged and published in downstream modules
  • [x] I have not added any vulnerable dependencies to my code

otherview avatar Apr 29 '24 14:04 otherview

Codecov Report

Attention: Patch coverage is 43.51852% with 61 lines in your changes are missing coverage. Please review.

Project coverage is 61.91%. Comparing base (aedfc6b) to head (1115d76). Report is 1 commits behind head on master.

Files Patch % Lines
cmd/thor/utils.go 0.00% 29 Missing :warning:
cmd/thor/p2p/p2p.go 60.34% 23 Missing :warning:
p2psrv/server.go 16.66% 5 Missing :warning:
cmd/thor/main.go 0.00% 4 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #724      +/-   ##
==========================================
+ Coverage   61.77%   61.91%   +0.14%     
==========================================
  Files         195      199       +4     
  Lines       18246    18309      +63     
==========================================
+ Hits        11271    11336      +65     
+ Misses       5876     5871       -5     
- Partials     1099     1102       +3     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Apr 29 '24 14:04 codecov-commenter