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

countvonzero · 2023-03-20T02:54:06Z

Motivation

part of #4089

Changes

bootstrap data updater that

load the latest bootstrap data from disk at instantiation time
after app started, periodically query an URL for json update
- validate data against json schema
- validate data with app logic that cannot be enforced by schema
- persist the latest update on disk and prune old ones
- notify subscribers of a new update

note:

test with a memory filesystem from github.com/spf13/afero

bootstrap/updater.go

codecov-commenter · 2023-03-20T19:00:51Z

Codecov Report

Merging #4169 (9eea57b) into develop (947f91b) will decrease coverage by 0.1%.
The diff coverage is 69.1%.

@@            Coverage Diff            @@
##           develop   #4169     +/-   ##
=========================================
- Coverage     76.9%   76.9%   -0.1%     
=========================================
  Files          236     238      +2     
  Lines        24660   24940    +280     
=========================================
+ Hits         18985   19179    +194     
- Misses        4482    4542     +60     
- Partials      1193    1219     +26

Impacted Files	Coverage Δ
bootstrap/types.go	`0.0% <0.0%> (ø)`
bootstrap/updater.go	`71.5% <71.5%> (ø)`
config/config.go	`100.0% <100.0%> (+2.7%)`	⬆️

... and 5 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.

bootstrap/mocks.go

bootstrap/updater.go

bootstrap/updater_test.go

piersy · 2023-03-22T09:55:28Z

Hey @countvonzero it would be useful for me if there was some package doc explaining what the purpose of the bootstrap package is, how its expected to be used and how it functions at a high level.

bootstrap/updater.go

dshulyak · 2023-03-24T06:00:09Z

bootstrap/interface.go

+//go:generate mockgen -package=bootstrap -write_package_comment=false -destination=./mocks.go -source=./interface.go
+
+type Receiver interface {
+	OnBoostrapUpdate(*VerifiedUpdate)


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/)

@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)

@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.

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.

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.

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

bootstrap/schema.json

bootstrap/updater.go

piersy · 2023-03-27T10:13:03Z

bootstrap/updater.go

+	return 0
+}
+
+func (u *Updater) DoIt(ctx context.Context) error {


Should this be exported (also Load)? From what I understand about the updater it seems like the public API should be:

Start Subscribe Close

I see it's used in the test but the test could be moved to this package.

it's a deliberate act on my part. in my view it is more important to put tests in XXX_test pkg here.
there are so many places in go-spacemesh where tests access private members directly and cause race issues.

Load and DoIt makes testing easier. i'll keep this in mind in my next iteration (there are prolly 2 more PRs) and see how i can reduce the exported methods.

I would prefer keeping the public API clean.

I don't think putting the tests in a different package is helping if we then export all the private things needed for the test. In this case it looks like the exported methods could cause race issues if called concurrently, so they're exhibiting the very problem that this approach was trying to avoid.

In my opinion the way to deal with race issues in tests is to structure the tests correctly, and that requires an understanding of the underlying code, and that becomes harder if the public API is not clear about what should and should not be called.

yes your preference is clear to me.

if i were developing low level libraries, i'd agree with you. and even there, you will get disagreement across the team.
my consideration is that we are trying to hit genesis testnet next week. so there is time crunch. i don't feel spending more time perfecting this PR is worthwhile.

piersy · 2023-03-27T10:23:25Z

bootstrap/updater.go

+	u.mu.Lock()
+	defer u.mu.Unlock()


I don't think we need to lock here. u.latest seems like its only accessed by the single internal go routine. It looks like the lock is only required to lock overs subscribers, if so I think calling it subscribersMu would help with readability.

i don't think one should conditionally lock mutable data.
the only data i didn't protect are those that are assigned in New and never get changed in the object's life cycle.

this module is not performance critical. for those that are changed outside of New, i think it's safer/future-proof to protect always.

Well, we disagree on this point :)

I find locking of objects that do not need to be synchronised makes the code much harder to understand, since it implies to me that those objects are going to be accessed by multiple go-routines.

I'm also not convinced that it makes the code any safer since adding locked sections also raises the risk of deadlocks.

Also fine grained locking can lead to bugs such as the race conditions in the broker that would have been much easier to track down if the parts had not been locked, since they would have triggered a data race. Having the fine grained locking technically made them safe from the point of view of the go race detector, but actually that hid the real race conditions.

I think if we actually followed the approach of locking over all mutable fields the code-base would become exceedingly complex.

ok. we disagreed.

piersy · 2023-03-27T11:10:34Z

bootstrap/updater.go

+	u.mu.Lock()
+	defer u.mu.Unlock()
+	ch := make(chan *VerifiedUpdate, 10)
+	u.subscribers = append(u.subscribers, ch)


Do we need to push the latest update to the newly subscribed channel?

this is a design decision. there is no need for now. but i'll keep that in mind in the upcoming iterations.

Is there some doc explaining the design decision?

nope. if in my follow up PRs it looks problematic to you, then i can change then.

countvonzero · 2023-03-27T22:26:04Z

bors merge

## Motivation part of #4089 ## Changes bootstrap data updater that - load the latest bootstrap data from disk at instantiation time - after app started, periodically query an URL for json update - validate data against json schema - validate data with app logic that cannot be enforced by schema - persist the latest update on disk and prune old ones - notify subscribers of a new update note: - test with a memory filesystem from github.com/spf13/afero

bors · 2023-03-27T23:16:17Z

Pull request successfully merged into develop.

Build succeeded:

ci-status
systest-status

create bootstrap data updater

f5914f3

countvonzero requested review from dshulyak, fasmat and poszu as code owners March 20, 2023 02:54

dshulyak reviewed Mar 20, 2023

View reviewed changes

bootstrap/updater.go Outdated Show resolved Hide resolved

countvonzero added 2 commits March 20, 2023 11:45

remove signature

1b20c4f

remove scale encoding for bootstrap json data

2958c88

piersy reviewed Mar 22, 2023

View reviewed changes

bootstrap/mocks.go Outdated Show resolved Hide resolved

piersy reviewed Mar 22, 2023

View reviewed changes

bootstrap/updater.go Outdated Show resolved Hide resolved

piersy reviewed Mar 22, 2023

View reviewed changes

bootstrap/updater_test.go Show resolved Hide resolved

add pkg comment and review feedback

2761e5a

piersy reviewed Mar 23, 2023

View reviewed changes

bootstrap/updater.go Outdated Show resolved Hide resolved

piersy reviewed Mar 23, 2023

View reviewed changes

bootstrap/updater.go Outdated Show resolved Hide resolved

dshulyak approved these changes Mar 24, 2023

View reviewed changes

piersy reviewed Mar 24, 2023

View reviewed changes

bootstrap/schema.json Outdated Show resolved Hide resolved

piersy reviewed Mar 24, 2023

View reviewed changes

bootstrap/schema.json Show resolved Hide resolved

piersy reviewed Mar 24, 2023

View reviewed changes

bootstrap/updater.go Outdated Show resolved Hide resolved

countvonzero added 7 commits March 24, 2023 14:47

Merge branch 'develop' into bootstrap-hare-active-set

2762285

remove dep injection and decouple client api

5061623

Merge branch 'develop' into bootstrap-hare-active-set

0b2e7ab

shorten code and rename

0f0ab27

add test for Start/Close

612ccf1

use httptest instead of mocking http

e8d6d2a

allow empty response

f908178

piersy reviewed Mar 27, 2023

View reviewed changes

Merge branch 'develop' into bootstrap-hare-active-set

0f0d3a4

countvonzero added 2 commits March 27, 2023 14:35

Merge branch 'develop' into bootstrap-hare-active-set

7d0794a

fix go.mod

9eea57b

bors bot changed the title ~~hare active set: create bootstrap data updater~~ [Merged by Bors] - hare active set: create bootstrap data updater Mar 27, 2023

bors bot closed this Mar 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

countvonzero commented Mar 20, 2023 •

edited

Loading

codecov-commenter commented Mar 20, 2023 •

edited by codecov bot

Loading

piersy commented Mar 22, 2023

dshulyak Mar 24, 2023

piersy Mar 24, 2023

countvonzero Mar 24, 2023

dshulyak Mar 25, 2023

countvonzero Mar 26, 2023

dshulyak Mar 27, 2023

piersy Mar 27, 2023

countvonzero Mar 27, 2023

piersy Mar 27, 2023

countvonzero Mar 27, 2023

piersy Mar 27, 2023 •

edited

Loading

countvonzero Mar 27, 2023

piersy Mar 27, 2023 •

edited

Loading

countvonzero Mar 27, 2023

piersy Mar 28, 2023

piersy Mar 27, 2023

countvonzero Mar 27, 2023

piersy Mar 27, 2023

countvonzero Mar 27, 2023

countvonzero commented Mar 27, 2023

bors bot commented Mar 27, 2023

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

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

Conversation

countvonzero commented Mar 20, 2023 • edited Loading

Motivation

Changes

codecov-commenter commented Mar 20, 2023 • edited by codecov bot Loading

Codecov Report

piersy commented Mar 22, 2023

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

piersy Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piersy Mar 27, 2023 • edited Loading

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 Mar 27, 2023

bors bot commented Mar 27, 2023

countvonzero commented Mar 20, 2023 •

edited

Loading

codecov-commenter commented Mar 20, 2023 •

edited by codecov bot

Loading

piersy Mar 27, 2023 •

edited

Loading

piersy Mar 27, 2023 •

edited

Loading