core/aggsigdb: implement in-memory AggSigDB #249

leolara · 2022-03-18T09:15:04Z

category: feature
ticket: #220

codecov · 2022-03-18T09:16:14Z

Codecov Report

Merging #249 (083f303) into main (4579206) will increase coverage by 0.49%.
The diff coverage is 84.37%.

@@            Coverage Diff             @@
##             main     #249      +/-   ##
==========================================
+ Coverage   55.29%   55.78%   +0.49%     
==========================================
  Files          50       53       +3     
  Lines        3541     3671     +130     
==========================================
+ Hits         1958     2048      +90     
- Misses       1314     1345      +31     
- Partials      269      278       +9

Impacted Files	Coverage Δ
core/aggsigdb/memory.go	`84.04% <84.04%> (ø)`
core/types.go	`4.25% <100.00%> (+4.25%)`	⬆️
tbls/tblsconv/tblsconv.go	`46.75% <0.00%> (-5.15%)`	⬇️
app/app.go	`62.45% <0.00%> (-0.71%)`	⬇️
app/disk.go	`57.14% <0.00%> (ø)`
cmd/gen_simnet.go	`50.70% <0.00%> (ø)`
core/sigagg/sigagg.go	`52.63% <0.00%> (ø)`
core/validatorapi/validatorapi.go	`41.26% <0.00%> (ø)`
testutil/keystore/keystore.go	`50.68% <0.00%> (ø)`
core/aggsigdb/stub.go	`0.00% <0.00%> (ø)`
... 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 4579206...083f303. Read the comment docs.

corverroos · 2022-03-18T09:21:07Z

core/aggsigdb/memory.go

+		blockedQueries: []readQuery{},
+	}
+
+	go db.loop()


I'm not of fan of using hidden goroutines that are never stopped. I would say we have two options:

Add an explicit Run(context.Context) method that we can wire into the lifecycle.Manager

Or even better, follow the reference in DutyDB that doesn't require any additional goroutines.

corverroos · 2022-03-18T09:23:19Z

core/aggsigdb/memory.go

+	db.commands <- writeCommand{
+		duty:   duty,
+		pubKey: pubKey,
+		data:   data,
+	}


the problem with channels are that they give the illusion of safety. The store might actually fail since there is a duplicate key and mismatching value, but you cannot return that error to the caller, who should know about this, since the write didn't succeed.

I would just need to add a response channel with error that could also return nil

yeah that is also possible.

corverroos · 2022-03-18T09:24:06Z

core/aggsigdb/memory.go

+
+// Await implements core.AggSigDB, see its godoc.
+func (db *memDB) Await(ctx context.Context, duty core.Duty, pubKey core.PubKey) (core.AggSignedData, error) {
+	response := make(chan core.AggSignedData)


As in the reference, you need a channel size of 1

corverroos · 2022-03-18T09:24:34Z

core/aggsigdb/memory.go

+}
+
+// memDB is a basic memory implementation of core.AggSigDB.
+type memDB struct {


I prefer that structs are define above their methods

corverroos · 2022-03-18T09:26:02Z

core/aggsigdb/memory.go

+func (db *memDB) execQuery(query readQuery) {
+	data, ok := db.data[db.getKey(query.duty, query.pubKey)]
+	if ok {
+		query.response <- data


this will block forever if the caller already timed out...

I feel very bad I didn't catch this :-(

corverroos · 2022-03-18T09:28:22Z

core/aggsigdb/memory.go

+// and removed from blockedQueries.
+func (db *memDB) processBlockedQueries() {
+	queries := db.blockedQueries
+	db.blockedQueries = []readQuery{}


I'm not a fan of having two workflows (best effort execQuery and then later blockedQueries), simplifying it to a single workflow reduces complexity.

Also you are modifying the blockedQueries array in different places which feel confusing.

corverroos · 2022-03-18T09:29:23Z

core/aggsigdb/memory.go

+
+// memDB is a basic memory implementation of core.AggSigDB.
+type memDB struct {
+	data map[string]core.AggSignedData


suggest using struct as key, that is more explicit.

corverroos · 2022-03-18T09:30:28Z

core/aggsigdb/memory.go

+	duty   core.Duty
+	pubKey core.PubKey
+	data   core.AggSignedData


nit: export struct fields if it is a pure value struct, since other things are accessing the fields. Unexported fields should in general only be used by the thing itself.

I am not sure what you mean. Visibility is at package level and these do not need to be visible outside the package

Indeed, visibility doesn't change. So technically one could either define exported field or unexported fields. But there is still implicit meaning attached to "Upper" fields vs "lower" fields. I always see "Upper" fields as, anyone can use this. While "lower" fields means, only this thing should use.

In the end it is subjective, but I would prefer if we pure struct values have "Upper" fields, since that conveys the meaning that this is a pure value and anyone can access the fields.

corverroos · 2022-03-18T09:32:44Z

core/aggsigdb/memory_test.go

+		Signature: []byte("test signature"),
+	}
+
+	wg := sync.WaitGroup{}


nit: prefer simple variable declaration for zero values: var sg sync.WaitGroup https://dave.cheney.net/practical-go/presentations/gophercon-singapore-2019.html#_use_a_consistent_declaration_style

corverroos · 2022-03-18T09:34:14Z

core/aggsigdb/memory.go

+// if it is not present it will store the query in blockedQueries.
+func (db *memDB) execQuery(query readQuery) {
+	data, ok := db.data[db.getKey(query.duty, query.pubKey)]
+	if ok {


need to error if the data already exists AND is different. Idempotent inserts are supported, but we should error on mismatching data.

This is the read but I understand what you mean, but for execCommand I think

oh yeah sorry

leolara · 2022-03-18T09:44:44Z

@corverroos I think your comments are correct. But I feel you would prefer to do it with mutexes? I think with channels are more idiomatic, more functional style and feel more like immutable programming,. Mutexes feel more imperative.

Of course we can, and perhaps we should in this PR, provide a way to stop the main loop.

…y database

corverroos · 2022-03-18T15:39:48Z

Yeah, the choice is basically channels vs mutexes. In this case, we are mutating in-memory state, so we can't get away from some form of mutability unfortunately. I chose mutexes since I feel it is a bit simpler, and doesn't require an additional goroutine, but I do not think it matters either way in the end.||

You can stick with channels if you want.

corverroos

LGTM

corverroos · 2022-03-18T15:43:36Z

core/aggsigdb/memory.go

+		response: response,
+	}
+
+	return <-response


should probably select on ctx.Done here, even though it should not really happen in practice

corverroos · 2022-03-18T15:44:27Z

core/aggsigdb/memory.go

+}
+
+// loop over commands and queries to serialise them.
+func (db *memDB) loop() {


Change this to Run(ctx) and then we can register this with lifecycle manager and it will stop on shutdown.

corverroos · 2022-03-18T15:46:10Z

core/types.go

@@ -163,3 +164,7 @@ type AggSignedData struct {
 	// Signature is the result of tbls aggregation and is inserted into the data.
 	Signature []byte
 }
+
+func (a AggSignedData) Equal(b AggSignedData) bool {


please add a simple test for this, our test coverage is creeping down

corverroos · 2022-03-18T15:47:47Z

core/aggsigdb/memory.go

+// and removed from blockedQueries.
+func (db *memDB) processBlockedQueries() {
+	queries := db.blockedQueries
+	db.blockedQueries = []readQuery{}


nit: reset by setting to nil, it is slightly more readable

leolara requested a review from corverroos March 18, 2022 09:15

corverroos changed the title ~~core/assisigdb: implementation of core.AggSigDB with a basic in-memory database~~ core/aggsigdb: implement in-memory AggSigDB Mar 18, 2022

corverroos reviewed Mar 18, 2022

View reviewed changes

leolara added 4 commits March 18, 2022 15:34

core/assisigdb: implementation of core.AggSigDB with a basic in-memor…

3b1d18a

…y database

Add test coverage for cancelation of await

98b7acc

Fix deadlock

fa98bf7

Fix several issues

3b08432

leolara force-pushed the leolara/asssigdb-memdb-basic branch from f1ddd5d to 3b08432 Compare March 18, 2022 14:36

corverroos approved these changes Mar 18, 2022

View reviewed changes

leolara and others added 6 commits March 20, 2022 12:52

Fix readability of blockedQueries

704b694

Be able to stop memDB

b2cde19

Add memDB Run

beda74a

suggest (#257)

e3ae946

Add test for core.AggSignedData.Equal

169fd14

Add test to stopping MemDB

083f303

leolara merged commit e8eb600 into main Mar 21, 2022

leolara deleted the leolara/asssigdb-memdb-basic branch March 21, 2022 10:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core/aggsigdb: implement in-memory AggSigDB #249

core/aggsigdb: implement in-memory AggSigDB #249

leolara commented Mar 18, 2022

codecov bot commented Mar 18, 2022 •

edited

Loading

corverroos Mar 18, 2022

corverroos Mar 18, 2022

leolara Mar 18, 2022

corverroos Mar 18, 2022

corverroos Mar 18, 2022

corverroos Mar 18, 2022

corverroos Mar 18, 2022

leolara Mar 18, 2022

corverroos Mar 18, 2022

corverroos Mar 18, 2022

corverroos Mar 18, 2022

leolara Mar 18, 2022

corverroos Mar 18, 2022

corverroos Mar 18, 2022

corverroos Mar 18, 2022

leolara Mar 18, 2022

corverroos Mar 18, 2022

leolara commented Mar 18, 2022

corverroos commented Mar 18, 2022 •

edited

Loading

corverroos left a comment

corverroos Mar 18, 2022

corverroos Mar 18, 2022

corverroos Mar 18, 2022

corverroos Mar 18, 2022

core/aggsigdb: implement in-memory AggSigDB #249

core/aggsigdb: implement in-memory AggSigDB #249

Conversation

leolara commented Mar 18, 2022

codecov bot commented Mar 18, 2022 • edited Loading

Codecov Report

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

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

leolara commented Mar 18, 2022

corverroos commented Mar 18, 2022 • edited Loading

corverroos 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

codecov bot commented Mar 18, 2022 •

edited

Loading

corverroos commented Mar 18, 2022 •

edited

Loading