[Merged by Bors] - hare active set: bootstrap and use first consensus block #4241

countvonzero · 2023-04-02T19:34:54Z

Motivation

part of #4089

Changes

hare
- bootstrap/fallback: whenever an update is received for epoch N, use it for epoch N's active set
- steady state: use the 1st hare output in epoch N as active set for epoch N (ActiveSet_N)
- confidence param:
  - the bootstrap active set (targeting for epoch 2), is used btwn [FirstLayer_2, FirstLayer_3+confidence_param)
  - ActiveSet_N will be used btwn [FirstLayer_N+confidence_param, FirstLayer_N+1+confidence_param)
beacon:
- whenever an update is received for an epoch, always use it as the beacon for the epoch
add debug RPC to query the hare active set used by a node for a given epoch
add a new systest TestFallback where bootstrapper will update continuously, and check the nodes use the updated value as beacon/activeset instead of the one calculated locally.
add metrics for update listener for query and update outcome.
add metrics for block certifier to count number of certificate generated/synced for each epoch.

hare/eligibility/oracle.go

countvonzero · 2023-04-02T19:53:01Z

bors try

codecov · 2023-04-02T20:05:32Z

Codecov Report

Merging #4241 (01d71a6) into develop (924f1f4) will decrease coverage by 0.1%.
The diff coverage is 75.5%.

@@            Coverage Diff            @@
##           develop   #4241     +/-   ##
=========================================
- Coverage     76.1%   76.1%   -0.1%     
=========================================
  Files          239     240      +1     
  Lines        25026   25086     +60     
=========================================
+ Hits         19059   19098     +39     
- Misses        4753    4764     +11     
- Partials      1214    1224     +10

Impacted Files	Coverage Δ
common/types/layer.go	`81.0% <ø> (ø)`
sql/blocks/blocks.go	`73.8% <50.0%> (ø)`
blocks/metrics.go	`65.2% <65.2%> (ø)`
beacon/beacon.go	`77.5% <66.6%> (-0.1%)`	⬇️
bootstrap/updater.go	`70.2% <71.4%> (-0.4%)`	⬇️
sql/ballots/ballots.go	`70.4% <72.0%> (-0.3%)`	⬇️
sql/beacons/beacons.go	`83.7% <72.7%> (-4.7%)`	⬇️
hare/eligibility/oracle.go	`64.4% <74.3%> (-1.3%)`	⬇️
api/grpcserver/debug_service.go	`67.4% <75.0%> (+0.7%)`	⬆️
sql/certificates/certs.go	`76.1% <85.0%> (+1.9%)`	⬆️
... and 4 more

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

bors · 2023-04-02T21:00:39Z

try

Build failed:

systest-status

countvonzero · 2023-04-02T23:48:03Z

bors try

bors · 2023-04-03T00:55:10Z

try

Build failed:

systest-status

countvonzero · 2023-04-03T02:21:36Z

bors try

bors · 2023-04-03T02:55:39Z

try

Build succeeded:

bors · 2023-04-03T05:38:16Z

try

Timed out.

hare/eligibility/oracle.go

countvonzero · 2023-04-12T21:59:05Z

bors try

bors · 2023-04-12T22:37:13Z

try

Build failed:

systest-status

countvonzero · 2023-04-13T00:30:10Z

bors try

bors · 2023-04-13T01:11:19Z

try

Build succeeded:

countvonzero · 2023-04-13T05:12:09Z

bors try

bors · 2023-04-13T05:23:35Z

try

Build failed:

ci-status

dshulyak · 2023-04-13T12:16:44Z

i thought that we want block in consensus (e.g tortoise consensus), ofcourse hare is a prerequisite for that. but there is a difference. if tortoise changes its opinion, what do we want to have as an active set?

if we want tortoise output, then implementation should be a bit different. it is not ok to pick first and cache it forever, instead it makes sense re-pick it if tortoise changed opinion. i also thought that we want to monitor actual empty layers in consensus output. so if it was non-empty and then became empty.. what should be done in such case?

dshulyak · 2023-04-13T12:22:36Z

blocks/generator.go

+	epoch := block.LayerIndex.GetEpoch()
+	s.blockCount[epoch]++
+	delete(s.blockCount, epoch-2)
+}


i think this metric might be useful for this component, but in general it has some limitations. node can download and apply block bypassing this generator and it should be good enough for fallback monitoring purposes

i've updated the metrics to track block certificate instead. after all that's what we are looking for.

countvonzero · 2023-04-13T15:58:55Z

i thought that we want block in consensus (e.g tortoise consensus), ofcourse hare is a prerequisite for that. but there is a difference. if tortoise changes its opinion, what do we want to have as an active set?

if we want tortoise output, then implementation should be a bit different. it is not ok to pick first and cache it forever, instead it makes sense re-pick it if tortoise changed opinion. i also thought that we want to monitor actual empty layers in consensus output. so if it was non-empty and then became empty.. what should be done in such case?

@dshulyak the original discussion is to piggyback on hare consensus. the argument is that if there is a block certificate, that means the network agreed. it's not so much about which block is correct for that layer, but that everyone agreed that hare output that block at that time, since all we need is an active set nodes can agree on.

the code path for finding a tortoise verified block is a little more complicated and require consideration for revert as you stated.

dshulyak

i guess it makes sense if it is not going to be a long term solution

dshulyak · 2023-04-14T09:57:33Z

api/grpcserver/debug_service.go

+func (d DebugService) ActiveSet(ctx context.Context, req *pb.ActiveSetRequest) (*pb.ActiveSetResponse, error) {
+	actives, err := d.oracle.ActiveSet(ctx, types.EpochID(req.Epoch))
+	if err != nil {
+		log.With().Error("failed to get active set", log.Uint32("epoch", req.Epoch), log.Err(err))


i prefer return details in status.Errorf and drop this

dshulyak · 2023-04-14T09:59:47Z

beacon/beacon.go

-	pd.logger.With().Info("received beacon update", epoch, beacon)
+	pd.mu.Lock()
+	defer pd.mu.Unlock()
+	if err := beacons.AddOverwrite(pd.cdb, epoch, beacon); err != nil {


maybe Set is a better name

dshulyak · 2023-04-14T10:01:01Z

beacon/beacon.go

+	defer pd.mu.Unlock()
+	if err := beacons.AddOverwrite(pd.cdb, epoch, beacon); err != nil {
+		pd.logger.With().Error("failed to persist fallback beacon", epoch, beacon, log.Err(err))
+	}


this error handling is not sound. i suggest to return an error and fail the node if it is not possible to persist something (or retry, until it is possible to persist it)

it is definitely not an option just to continue

changed to fail the node.

dshulyak · 2023-04-14T10:01:53Z

blocks/generator.go

@@ -60,6 +63,29 @@ func defaultConfig() Config {
 	}
 }

+type stats struct {
+	lock       sync.Mutex


mu is more common

thanks. noted. moved the code to certifier.go and reuse the mutex there.

dshulyak · 2023-04-14T10:03:46Z

blocks/generator.go

+func (s *stats) Get() map[types.EpochID]int {
+	s.lock.Lock()
+	defer s.lock.Unlock()
+	result := make(map[types.EpochID]int)


it is better to implement such methods without allocating memory on heap

dshulyak · 2023-04-14T10:04:58Z

bootstrap/metrics.go

+		namespace,
+		"number of queries made to the update site",
+		[]string{"outcome"},
+	)


maybe track latency

dshulyak · 2023-04-14T10:07:33Z

sql/certificates/certs.go

+		rows   int
+	)
+	if rows, err = db.Exec(`
+		select block from certificates where layer between ?1 and ?2 and valid = 1


do we keep track of invalid certificates? thats a waste

we don't keep track of those that were invalid in the first place.
we only keep certificate that was valid before but later became invalid.

hare output but has no certificate to back it up (this db entry will have certificate as null)

a certificate that was valid when received but no longer valid when the 2nd valid certificate is received.

countvonzero · 2023-04-16T00:53:41Z

bors try

bors · 2023-04-16T01:32:49Z

try

Build succeeded:

countvonzero · 2023-04-16T21:00:11Z

bors merge

## Motivation part of #4089 ## Changes - hare - bootstrap/fallback: whenever an update is received for epoch N, use it for epoch N's active set - steady state: use the 1st hare output in epoch N as active set for epoch N (`ActiveSet_N`) - confidence param: - the bootstrap active set (targeting for epoch 2), is used btwn [`FirstLayer_2`, `FirstLayer_3`+`confidence_param`) - `ActiveSet_N` will be used btwn [`FirstLayer_N`+`confidence_param`, `FirstLayer_N+1`+`confidence_param`) - beacon: - whenever an update is received for an epoch, always use it as the beacon for the epoch - add debug RPC to query the hare active set used by a node for a given epoch - add a new systest TestFallback where bootstrapper will update continuously, and check the nodes use the updated value as beacon/activeset instead of the one calculated locally. - add metrics for update listener for query and update outcome. - add metrics for block certifier to count number of certificate generated/synced for each epoch.

bors · 2023-04-16T21:43:05Z

Pull request successfully merged into develop.

Build succeeded:

countvonzero requested review from dshulyak, fasmat and poszu as code owners April 2, 2023 19:34

countvonzero commented Apr 2, 2023

View reviewed changes

hare/eligibility/oracle.go Outdated Show resolved Hide resolved

bors bot added a commit that referenced this pull request Apr 2, 2023

Try #4241:

6b95f24

countvonzero mentioned this pull request Apr 2, 2023

implement hare active set #4089

Closed

3 tasks

bors bot added a commit that referenced this pull request Apr 2, 2023

Try #4241:

db75994

bors bot added a commit that referenced this pull request Apr 3, 2023

Try #4241:

835daee

dshulyak reviewed Apr 4, 2023

View reviewed changes

hare/eligibility/oracle.go Outdated Show resolved Hide resolved

hare/eligibility/oracle.go Outdated Show resolved Hide resolved

countvonzero added 5 commits April 12, 2023 13:54

add debug service query active set

183ecab

boostrapper adaptation for systest

2b87165

change beacon to use fallback if available

ec4e672

use first block in epoch for hare active set with bootstrap and fallback

1bda9b0

add fallback systest

747d472

countvonzero force-pushed the feed-the-rabbit branch from b147cac to 747d472 Compare April 12, 2023 21:35

Merge branch 'develop' into feed-the-rabbit

9e6919f

bors bot added a commit that referenced this pull request Apr 12, 2023

Try #4241:

36ad82b

bors bot added a commit that referenced this pull request Apr 13, 2023

Try #4241:

fe0abf4

countvonzero added 2 commits April 12, 2023 21:56

add metrics

601bf53

fix log level

ab3a190

bors bot added a commit that referenced this pull request Apr 13, 2023

Try #4241:

4c69d7f

dshulyak reviewed Apr 13, 2023

View reviewed changes

dshulyak approved these changes Apr 14, 2023

View reviewed changes

fasmat mentioned this pull request Apr 15, 2023

[Merged by Bors] - Integrate API v1.9.0 #4301

Closed

7 tasks

countvonzero added 7 commits April 15, 2023 13:26

monitor valid cert instead of block

c918143

remove logging from debug service

4120afd

propagate beacon error up to app node

9cf560f

add duration metrics

9763c82

Merge branch 'develop' into feed-the-rabbit

e177c16

update test name

7b0c655

fix test

01d71a6

bors bot added a commit that referenced this pull request Apr 16, 2023

Try #4241:

cdeb9fc

bors bot changed the title ~~hare active set: bootstrap and use first consensus block~~ [Merged by Bors] - hare active set: bootstrap and use first consensus block Apr 16, 2023

bors bot closed this Apr 16, 2023

fasmat mentioned this pull request Nov 23, 2023

flaky Test_CalcEligibility_MainnetParams #4273

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - hare active set: bootstrap and use first consensus block #4241

[Merged by Bors] - hare active set: bootstrap and use first consensus block #4241

countvonzero commented Apr 2, 2023 •

edited

Loading

countvonzero commented Apr 2, 2023

codecov bot commented Apr 2, 2023 •

edited

Loading

bors bot commented Apr 2, 2023

countvonzero commented Apr 2, 2023

bors bot commented Apr 3, 2023

countvonzero commented Apr 3, 2023

bors bot commented Apr 3, 2023

bors bot commented Apr 3, 2023

countvonzero commented Apr 12, 2023

bors bot commented Apr 12, 2023

countvonzero commented Apr 13, 2023

bors bot commented Apr 13, 2023

countvonzero commented Apr 13, 2023

bors bot commented Apr 13, 2023

dshulyak commented Apr 13, 2023 •

edited

Loading

dshulyak Apr 13, 2023 •

edited

Loading

countvonzero Apr 15, 2023

countvonzero commented Apr 13, 2023

dshulyak left a comment

dshulyak Apr 14, 2023

dshulyak Apr 14, 2023

dshulyak Apr 14, 2023

countvonzero Apr 16, 2023

dshulyak Apr 14, 2023

countvonzero Apr 15, 2023

dshulyak Apr 14, 2023

dshulyak Apr 14, 2023

dshulyak Apr 14, 2023

countvonzero Apr 15, 2023

countvonzero commented Apr 16, 2023

bors bot commented Apr 16, 2023

countvonzero commented Apr 16, 2023

bors bot commented Apr 16, 2023

[Merged by Bors] - hare active set: bootstrap and use first consensus block #4241

[Merged by Bors] - hare active set: bootstrap and use first consensus block #4241

Conversation

countvonzero commented Apr 2, 2023 • edited Loading

Motivation

Changes

countvonzero commented Apr 2, 2023

codecov bot commented Apr 2, 2023 • edited Loading

Codecov Report

bors bot commented Apr 2, 2023

try

countvonzero commented Apr 2, 2023

bors bot commented Apr 3, 2023

try

countvonzero commented Apr 3, 2023

bors bot commented Apr 3, 2023

try

bors bot commented Apr 3, 2023

try

countvonzero commented Apr 12, 2023

bors bot commented Apr 12, 2023

try

countvonzero commented Apr 13, 2023

bors bot commented Apr 13, 2023

try

countvonzero commented Apr 13, 2023

bors bot commented Apr 13, 2023

try

dshulyak commented Apr 13, 2023 • edited Loading

dshulyak Apr 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

countvonzero commented Apr 13, 2023

dshulyak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

countvonzero commented Apr 16, 2023

bors bot commented Apr 16, 2023

try

countvonzero commented Apr 16, 2023

bors bot commented Apr 16, 2023

countvonzero commented Apr 2, 2023 •

edited

Loading

codecov bot commented Apr 2, 2023 •

edited

Loading

dshulyak commented Apr 13, 2023 •

edited

Loading

dshulyak Apr 13, 2023 •

edited

Loading