bosagora / agora

POC Node implementation for CoinNet
https://bosagora.io
MIT License
37 stars 22 forks source link

ValidatorSet: Bypass far preimage check on adding new enrollment #3201

Closed omerfirmak closed 2 years ago

omerfirmak commented 2 years ago

If a Validator stays unenrolled more than ValidatorCycle amount of blocks, it would never be able to reveal new preimages after that point.

omerfirmak commented 2 years ago

This is the root cause of our full nodes in production falling behind.

codecov[bot] commented 2 years ago

Codecov Report

Merging #3201 (ed697a7) into v0.x.x (5be86bb) will increase coverage by 0.04%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           v0.x.x    #3201      +/-   ##
==========================================
+ Coverage   79.21%   79.25%   +0.04%     
==========================================
  Files         209      209              
  Lines       18944    18982      +38     
==========================================
+ Hits        15006    15044      +38     
  Misses       3938     3938              
Flag Coverage Δ
integration 20.27% <ø> (ø)
unittests 87.43% <100.00%> (+0.02%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
source/agora/consensus/state/ValidatorSet.d 86.77% <100.00%> (+1.56%) :arrow_up:
source/agora/test/NetworkDiscovery.d 84.09% <0.00%> (-4.55%) :arrow_down:
source/agora/flash/Node.d 80.67% <0.00%> (-0.43%) :arrow_down:
source/agora/consensus/protocol/Nominator.d 92.45% <0.00%> (-0.18%) :arrow_down:
source/agora/consensus/Ledger.d 97.48% <0.00%> (-0.01%) :arrow_down:
source/agora/test/InvalidBlockSigByzantine.d 96.29% <0.00%> (+0.14%) :arrow_up:
source/agora/node/FullNode.d 73.93% <0.00%> (+0.30%) :arrow_up:
source/agora/consensus/state/Ledger.d 90.90% <0.00%> (+0.51%) :arrow_up:
source/agora/node/Validator.d 91.70% <0.00%> (+0.51%) :arrow_up:
source/agora/network/Manager.d 80.58% <0.00%> (+0.64%) :arrow_up:
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5be86bb...ed697a7. Read the comment docs.

omerfirmak commented 2 years ago

This reverts 5784d24

Updated to also conform to the behaviour https://github.com/bosagora/agora/commit/5784d240096667ad0195c7316fa3f5696e64c335 expects

omerfirmak commented 2 years ago

Is the title still valid?

Yeah, I think it sill is. We are already checking if commitment is valid while validating the enrollment. There is no reason to use addPreImage() and force another validation with far preimage guard.

Also at least one test needs an update as the failure looks related.

updated, should be good now.

omerfirmak commented 2 years ago

Green