search

filecoin-project / eudico

lotus, but also other things
Other
19 stars 14 forks source link

Mir intro #185

Closed dnkolegov closed 2 years ago

dnkolegov commented 2 years ago

This PR adds MirBFT into Eudico

codecov-commenter commented 2 years ago

Codecov Report

Merging #185 (9430b98) into eudico (fe4d9ac) will increase coverage by 1.66%. The diff coverage is 53.39%.

Impacted file tree graph

@@            Coverage Diff             @@
##           eudico     #185      +/-   ##
==========================================
+ Coverage   20.81%   22.48%   +1.66%     
==========================================
  Files         561      575      +14     
  Lines       61432    62095     +663     
==========================================
+ Hits        12790    13960    +1170     
+ Misses      46161    45527     -634     
- Partials     2481     2608     +127
Impacted Files Coverage Δ
chain/consensus/common/types.go 0.00% <0.00%> (ø)
chain/consensus/delegcns/mine.go 61.53% <0.00%> (-4.09%) :arrow_down:
chain/consensus/hierarchical/types.go 46.93% <0.00%> (-4.18%) :arrow_down:
chain/consensus/mir/utils.go 0.00% <0.00%> (ø)
chain/consensus/tendermint/mine.go 0.00% <0.00%> (ø)
chain/consensus/tendermint/tendermint.go 0.00% <0.00%> (ø)
chain/consensus/tendermint/types.go 0.00% <0.00%> (ø)
chain/consensus/tendermint/utils.go 0.00% <0.00%> (ø)
chain/consensus/tspow/mine.go 69.41% <0.00%> (+2.74%) :arrow_up:
chain/types/message.go 34.86% <0.00%> (-2.40%) :arrow_down:
... and 67 more
dnkolegov commented 2 years ago

Thank you for your review.

  • What is ideal consensus?

It is a primitive for testing: it orders input messages by adding them into a block in the same order they were received, it works only for one tested instance.

  • It would be nicer (and easier for me to review) if there was a separate PR just for the Mir-related code.

Sorry for that. But adding Mir required fixing other stuff too. See Alfonso's suggestions and comments.

  • Could you add a few comments in the code, saying (in just a few words) what the code is doing? That would also make it easier for me to read.

Sure. What part of the code you didn't understand?

matejpavlovic commented 2 years ago

Sure. What part of the code you didn't understand?

Mostly mine.go, mir_agent.go and utils.go have almost no comments. It's not that I didn't understand them at all, but I think it would make the code easier to read if a few comments were saying in English what the lines of code are doing. The other files, I assume, are mostly boilerplate required by Eudico that we don't care about too much yet.

But otherwise all is fine with me, if it works (and when @adlrocha submits his review) we can merge and go on implementing support for multiple nodes.