Fix #1297: Make Enrollments valid for nomination on only a single block height #1421

omerfirmak · 2020-12-16T11:08:41Z

With this, PreImages are consumed not by new enrollments but by new blocks. Commitments in new enrollments should be the PreImage that would be revealed on the height that Enrollment would be available.

More background info at #1297

AndrejMitrovic

A few comments.

source/agora/consensus/state/ValidatorSet.d

source/agora/consensus/PreImage.d

source/agora/consensus/EnrollmentManager.d

source/agora/consensus/validation/Block.d

AndrejMitrovic · 2020-12-16T12:09:38Z

@linked0 could you also take a look at this and tell us what you think?

omerfirmak · 2020-12-16T14:35:37Z

Thanks for the review @AndrejMitrovic , all of your comments should be addressed now.

AndrejMitrovic · 2020-12-17T02:43:41Z

It would be good to add a network test in source/agora/test/*. There are some examples there where a custom node is instantiated which behaves badly, a similar test could be added where a node tries enroll (or re-enroll) with an enrollment with the wrong random_seed.

linked0 · 2020-12-17T03:01:00Z

@linked0 could you also take a look at this and tell us what you think?

I'm reviewing it now. Sorry for the late review. Anyway, it looks really awesome. I will leave some comments or suggestions soon.

linked0

Good work and very important code change.
But the code about the avail_height was removed unintentionally. Some test code that changed does not go with the original purpose. Please check the comments.

source/agora/consensus/EnrollmentManager.d

omerfirmak · 2020-12-17T07:40:42Z

It would be good to add a network test in source/agora/test/*. There are some examples there where a custom node is instantiated which behaves badly, a similar test could be added where a node tries enroll (or re-enroll) with an enrollment with the wrong random_seed.

Yeah, I wanted to get some feedback before writing a network test.

source/agora/consensus/EnrollmentManager.d

omerfirmak · 2020-12-17T11:05:39Z

Added the network test.

This field would indicate if a enrollment is active or not. It lets expired enrollment entries to linger so that once that Validator would re-enroll we can validate its commitment.

Enrollments will be validated against the past Enrollments to eliminate the replay attack described in Issue bosagora#1297. For this we need a way to efficiently query past enrollments.

As explained in Issue bosagora#1297, current Enrollments can be reused by a third party any time. With Enrollments including the PreImage that of the current height, they will be only valid for a single block.

omerfirmak · 2020-12-17T11:48:53Z

Thanks for the review, @linked0 . I have rebased to v0.x.x.

linked0

Great work.
Some suggestions.

source/agora/test/ValidatorRecurringEnrollment.d

With the enrollments being valid for only a single height, enrollemnts in the pool should be revalidated before nomination.

omerfirmak · 2020-12-18T08:35:11Z

I've applied the changes you suggested @linked0. I'll take a look at the CircleCI failure. Integration tests on my local machine has no problems, its weird.

codecov · 2020-12-18T09:14:20Z

Codecov Report

Merging #1421 (c0993fa) into v0.x.x (a92aead) will increase coverage by 0.15%.
The diff coverage is 96.20%.

@@            Coverage Diff             @@
##           v0.x.x    #1421      +/-   ##
==========================================
+ Coverage   81.14%   81.30%   +0.15%     
==========================================
  Files         120      120              
  Lines       10551    10678     +127     
==========================================
+ Hits         8562     8682     +120     
- Misses       1989     1996       +7

Flag	Coverage Δ
integration	`37.87% <52.94%> (-0.18%)`	⬇️
unittests	`79.97% <96.20%> (+0.17%)`	⬆️

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

Impacted Files	Coverage Δ
source/agora/node/Validator.d	`94.00% <ø> (ø)`
source/agora/test/QuorumPreimage.d	`70.27% <ø> (ø)`
source/agora/consensus/validation/Block.d	`96.89% <78.26%> (-1.12%)`	⬇️
source/agora/test/ValidatorRecurringEnrollment.d	`93.75% <93.10%> (-0.99%)`	⬇️
source/agora/consensus/state/ValidatorSet.d	`88.40% <94.11%> (+0.77%)`	⬆️
source/agora/consensus/EnrollmentManager.d	`95.36% <100.00%> (+1.21%)`	⬆️
source/agora/consensus/EnrollmentPool.d	`83.17% <100.00%> (-2.68%)`	⬇️
source/agora/consensus/PreImage.d	`71.42% <100.00%> (+4.76%)`	⬆️
source/agora/consensus/validation/Enrollment.d	`98.85% <100.00%> (+0.43%)`	⬆️
source/agora/node/Ledger.d	`95.52% <100.00%> (ø)`
... and 7 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 a92aead...c0993fa. Read the comment docs.

omerfirmak · 2020-12-18T10:34:56Z

All green

AndrejMitrovic · 2020-12-21T01:30:21Z

Thank you @omerfirmak, nice job!

Thanks @linked0 for the review!

AndrejMitrovic suggested changes Dec 16, 2020

View reviewed changes

omerfirmak force-pushed the agora-1297-2 branch from 89f0ee9 to 681fd85 Compare December 16, 2020 14:34

linked0 reviewed Dec 17, 2020

View reviewed changes

source/agora/consensus/EnrollmentManager.d Show resolved Hide resolved

linked0 reviewed Dec 17, 2020

View reviewed changes

source/agora/consensus/EnrollmentManager.d Outdated Show resolved Hide resolved

omerfirmak force-pushed the agora-1297-2 branch from 681fd85 to 8d5652c Compare December 17, 2020 11:03

omerfirmak force-pushed the agora-1297-2 branch from 8d5652c to 831b5cc Compare December 17, 2020 11:37

omerfirmak added 4 commits December 17, 2020 14:38

ValidatorSet: Add "active" field to validator_set table

cff4a6e

This field would indicate if a enrollment is active or not. It lets expired enrollment entries to linger so that once that Validator would re-enroll we can validate its commitment.

ValidatorSet: Provide an interface to query Active/Expired Enrollments

3479c87

Enrollments will be validated against the past Enrollments to eliminate the replay attack described in Issue bosagora#1297. For this we need a way to efficiently query past enrollments.

PreImage: Support fetching a PreImage for an arbitrary Height

0063923

EnrollmentManager: Enrollments should be a function of block height

28c70af

As explained in Issue bosagora#1297, current Enrollments can be reused by a third party any time. With Enrollments including the PreImage that of the current height, they will be only valid for a single block.

omerfirmak force-pushed the agora-1297-2 branch from 831b5cc to 3523747 Compare December 17, 2020 11:48

linked0 approved these changes Dec 18, 2020

View reviewed changes

omerfirmak added 2 commits December 18, 2020 11:33

Enrollment: Validate commitment against old Enrollments

0ea9429

Nomination: Revalidate individual enrollments before nominating

f9fc5cf

With the enrollments being valid for only a single height, enrollemnts in the pool should be revalidated before nomination.

omerfirmak force-pushed the agora-1297-2 branch from 3523747 to f9fc5cf Compare December 18, 2020 08:33

omerfirmak force-pushed the agora-1297-2 branch from 9bb854b to f9fc5cf Compare December 18, 2020 09:52

omerfirmak mentioned this pull request Dec 18, 2020

Link system tests with sqlite3 #1436

Closed

Link system tests with sqlite3

c0993fa

linked0 mentioned this pull request Dec 21, 2020

Implement slashing protocol #1438

Merged

AndrejMitrovic approved these changes Dec 21, 2020

View reviewed changes

AndrejMitrovic merged commit b6c7b12 into bosagora:v0.x.x Dec 21, 2020

omerfirmak deleted the agora-1297-2 branch December 21, 2020 09:07

AndrejMitrovic mentioned this pull request Jan 20, 2021

Slashing information validation #1444

Closed

This was referenced Jan 20, 2021

Nodes should be able to recover from exhausting their PreImageCycle #1500

Closed

Test for the cases where Enrollments don't get accepted immediately #1545

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1297: Make Enrollments valid for nomination on only a single block height #1421

Fix #1297: Make Enrollments valid for nomination on only a single block height #1421

omerfirmak commented Dec 16, 2020

AndrejMitrovic left a comment

AndrejMitrovic commented Dec 16, 2020

omerfirmak commented Dec 16, 2020

AndrejMitrovic commented Dec 17, 2020

linked0 commented Dec 17, 2020

linked0 left a comment

omerfirmak commented Dec 17, 2020

omerfirmak commented Dec 17, 2020

omerfirmak commented Dec 17, 2020

linked0 left a comment

omerfirmak commented Dec 18, 2020 •

edited

codecov bot commented Dec 18, 2020 •

edited

omerfirmak commented Dec 18, 2020

AndrejMitrovic commented Dec 21, 2020

Fix #1297: Make Enrollments valid for nomination on only a single block height #1421

Fix #1297: Make Enrollments valid for nomination on only a single block height #1421

Conversation

omerfirmak commented Dec 16, 2020

AndrejMitrovic left a comment

Choose a reason for hiding this comment

AndrejMitrovic commented Dec 16, 2020

omerfirmak commented Dec 16, 2020

AndrejMitrovic commented Dec 17, 2020

linked0 commented Dec 17, 2020

linked0 left a comment

Choose a reason for hiding this comment

omerfirmak commented Dec 17, 2020

omerfirmak commented Dec 17, 2020

omerfirmak commented Dec 17, 2020

linked0 left a comment

Choose a reason for hiding this comment

omerfirmak commented Dec 18, 2020 • edited

codecov bot commented Dec 18, 2020 • edited

Codecov Report

omerfirmak commented Dec 18, 2020

AndrejMitrovic commented Dec 21, 2020

omerfirmak commented Dec 18, 2020 •

edited

codecov bot commented Dec 18, 2020 •

edited