Skip to content

[Merged by Bors] - hare active set: create bootstrap data updater #4169

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 14 commits into from
Closed

[Merged by Bors] - hare active set: create bootstrap data updater #4169

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
f5914f3
create bootstrap data updater
countvonzero Mar 20, 2023
1b20c4f
remove signature
countvonzero Mar 20, 2023
2958c88
remove scale encoding for bootstrap json data
countvonzero Mar 20, 2023
2761e5a
add pkg comment and review feedback
countvonzero Mar 22, 2023
2762285
Merge branch 'develop' into bootstrap-hare-active-set
countvonzero Mar 24, 2023
5061623
remove dep injection and decouple client api
countvonzero Mar 26, 2023
0b2e7ab
Merge branch 'develop' into bootstrap-hare-active-set
countvonzero Mar 26, 2023
0f0ab27
shorten code and rename
countvonzero Mar 27, 2023
612ccf1
add test for Start/Close
countvonzero Mar 27, 2023
e8d6d2a
use httptest instead of mocking http
countvonzero Mar 27, 2023
f908178
allow empty response
countvonzero Mar 27, 2023
0f0d3a4
Merge branch 'develop' into bootstrap-hare-active-set
countvonzero Mar 27, 2023
7d0794a
Merge branch 'develop' into bootstrap-hare-active-set
countvonzero Mar 27, 2023
9eea57b
fix go.mod
countvonzero Mar 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions bootstrap/interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package bootstrap

import (
"context"
"net/url"
)

//go:generate mockgen -package=bootstrap -destination=./mocks.go -source=./interface.go

type Receiver interface {
OnBoostrapUpdate(*VerifiedUpdate)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my preference is to avoid dependency injection. this code can be pushed outside by providing a way to subscribe for updates, and then add a goroutine that listens for updates and integrates with go-spacemesh

bootstrap := ...
bootstrap.Listen()
for update := bootstrap.Updates() {
     oracle.UpdateActiveSet(update.Epoch, update.ActiveSet)
     beacon.UpdateBeacon(update.Epoch, update.Beacon)
}

this is sometimes called dependency rejection (https://blog.ploeh.dk/2017/02/02/dependency-rejection/)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dshulyak I have my own thoughts on this but could you elaborate on what you see as the concrete benefits of the approach you proposed? ( I read the article but I'm not sure it maps very well to our codebase and the current situation)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dshulyak
i had two considerations for the current design decisions

  • keeping the code as simple as possible. currently the code doesn't require locking (single-threaded). if allowing to subscribe outside, then it requires locking. and i decide to keep it simple by passing fixed subscribers in the init flow

  • in your model, which i did consider doing,

bootstrap := ...
bootstrap.Listen()
for update := bootstrap.Updates() {
     oracle.UpdateActiveSet(update.Epoch, update.ActiveSet)
     beacon.UpdateBeacon(update.Epoch, update.Beacon)
}

the activeset data is potentially big, and since this data isn't immediately used to oracle or beacon because both components need to wait for X period of time to fall back to the value provided in the update, i don't want to keep multiple copies around in memory. in my prototype, beacon and oracle will each keep a reference to the latest update (the same copy as the updater) and use it as it needs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if allowing to subscribe outside, then it requires locking. and i decide to keep it simple by passing fixed subscribers in the init flow

by doing dependency injection this way you are multiplying complexity. nested code path is always more complex than flat code path. what i shared is flat.

i know that this is a very simple app, but just consider what should be done to understand how active set is updated. in my example i can tell immediately how it is updated. in your - i will have to find how this bootstrap is initialized and then how does it call this thing.

also you will need to have oracle.UpdateActiveSet (or alternative method) to handle changes in the consensus in a good way

the activeset data is potentially big, and since this data isn't immediately used to oracle or beacon because both components need to wait for X period of time to fall back to the value provided in the update, i don't want to keep multiple copies around in memory

i think there is some confusion about how many pointers needs to be around for long time. what i shared doesn't force runtime to keep multiple copies of data for long time, or copy any data... also memory concern is in general doesn't seem very relevant if you consider how this data compares to other data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nested code path is always more complex than flat code path. what i shared is flat.

this i agree with.

i think there is some confusion about how many pointers needs to be around for long time.

yes. i realized this later as well. oracle will keep the ptr to the backing array of the atx ids, which is the same ptr updater will hold onto until the next update.

ok. i'll change to the approach you described.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. i'll change to the approach you described.

btw, i wasn't trying to force a change, if there is no time and/or what you have is enough for it to work, it is certainly ok for me

}

type httpclient interface {
Query(context.Context, *url.URL) ([]byte, error)
}
86 changes: 86 additions & 0 deletions bootstrap/mocks.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

59 changes: 59 additions & 0 deletions bootstrap/schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
{
"$schema": "https://json-schema.org/draft/2020-12/schema",
"$id": "https://spacemesh.io/checkpoint.schema.json.1.0",
"title": "epoch data",
"description": "epoch data for bootstrapping and fallback",
"type": "object",
"required": ["version" ,"signature", "data"],
"properties": {
"version": {
"description": "version of the checkpoint file. should be compatible schema's $id",
"type": "string"
},
"signature": {
"description": "signature of the data portion of this file",
"type": "string"
},
"data": {
"description": "the signed portion of this file",
"type": "object",
"required": ["id", "epochs"],
"properties": {
"id": {
"type": "integer",
"exclusiveMinimum": 0
},
"epochs": {
"type": "array",
"uniqueItems": true,
"minItems": 1,
"items": {
"type": "object",
"properties": {
"epoch": {
"description": "epoch number",
"type": "integer",
"minimum": 0
},
"beacon": {
"description": "the random beacon value for this epoch",
"type": "string",
"minLength": 8
},
"activeSet": {
"description": "the set of ATXs for hare protocol in this epoch",
"type": "array",
"uniqueItems": true,
"minItems": 1,
"items": {
"type": "string"
}
}
}
}
}
}
}
}
}

20 changes: 20 additions & 0 deletions bootstrap/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package bootstrap

//go:generate scalegen -types InnerData,EpochData

type Update struct {
Version string `json:"version"`
Signature string `json:"signature"`
Data InnerData `json:"data"`
}

type InnerData struct {
ID uint32 `json:"id"`
Epochs []EpochData `json:"epochs"`
}

type EpochData struct {
Epoch uint32 `json:"epoch"`
Beacon string `json:"beacon"`
ActiveSet []string `json:"activeSet"`
}
99 changes: 99 additions & 0 deletions bootstrap/types_scale.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading