[Merged by Bors] - sql: store ballots, blocks and layers in sqlite #3047

dshulyak · 2022-01-07T09:04:44Z

Motivation

The existing approach lacks atomicity, causal durability (e.g. an atx may not be on disk when a ballot is saved), and the durability can't be enforced in general without running every leveldb operation in sync mode. All this problems will result in subtle bugs that are hard to diagnose.

For those 3 requirements, we want to maintain all state in a single db. Moving state to a single leveldb will require us to enforce isolation ourselves (by maintaining separate namespace and manually concatenating keys like we do in some modules), beside that we have to create every single index manually while with sqlite we can just do CREATE INDEX and sqlite will do it for us and probably do a better job.

Another significant benefit is that we can duplicate some state in sql table to avoid loading the whole structure into memory. For instance it will be relevant for atx, which is a large (10kb) and usually, after it was validated, we want to know only the associated smesher and weight of the atx. This would be problematic with leveldb, and would require adding custom index.

related: #2918

Changes

general plumbing for core database stuff (db, transaction, migrations) using https://github.com/crawshaw/sqlite that is a relatively simple wrapper around C sqlite
tables and for layers, blocks and ballots
reworked ZeroLayer, it is relevant only for hare_output, which can be in 3 states - nil, empty, non-empty.
SetZeroLayer update hare_output to empty state, at which tortoise will vote against all blocks within hdist.
updated to golang 1.16 for embed module. note that there is a bug with go mode tidy in 1.16 so i had to manually fix go.sum and disable go mod tidy on ci - cmd/go: missing sum after updating a different package golang/go#44129

Test Plan

existing and new uts

golang/go#44129

countvonzero · 2022-01-12T05:31:41Z

wow! super happy to move to relational database for data integrity. a couple high level questions

do you observe any performance degradation?
in one of my previous project we ported geth to use sqlite because we need multi-threaded read and leveldb doesn't support that. my recollection was that there was a performance penalty due to the sheer amount of data.
wrt to go version, why not go straight to 17? the tidy bug seems to be fixed at that version.

dshulyak · 2022-01-12T06:02:35Z

do you observe any performance degradation?

i observed an improvement with more data, and small degradation when there is very little data.
for example for loading 10 000 layers

benchmark                             old ns/op       new ns/op       delta
BenchmarkRerun/Verifying/10000-16     15456916985     13337943478     -13.71%

but relative improvement will be much larger than -13.71% when encoding will be replaced, as xdr wastes a lot of time.
with 100, 1000 layers leveldb is slightly faster, but i think this is less relevant.

i actually expected sqlite to be faster, as leveldb data structure is write-optimized. @countvonzero do you remember if database/sql was used in the previous project or a thin wrapper? there are some explanations why database/sql is not a good choice for sqlite, as sqlite is very different from other databases (https://turriate.com/articles/making-sqlite-faster-in-go, https://crawshaw.io/blog/go-and-sqlite).

wrt to go version, why not go straight to 17? the tidy bug seems to be fixed at that version.

@noamnelke was against updating versions if we are not using new features, so i decided not to rush with 1.17. anyway i think we will want to use 1.18 in near future, as it introduces significant improvements to the language (fuzzing support in stdlib, and generics)

countvonzero · 2022-01-12T06:13:02Z

do you remember if database/sql was used in the previous project or a thin wrapper?

yes. we were. we didn't explore other libraries further because core geth devs did not support the change.

countvonzero

initial pass only on sql package

countvonzero · 2022-01-12T06:23:35Z

sql/ballots/ballots.go

+		if err != io.EOF {
+			return nil, fmt.Errorf("copy sig %w", err)
+		}
+		sigBytes = nil


shouldn't this be an error too? same for pubkeyBytes
a ballot has to have signature and public key

i made it this way because for testing i don't need signature and pubkey, can change it to a stricter validation

actually, it is very inconvenient to use valid signature and pubkey for testing, so i would rather leave it as it is, and enforce validation in other places (like it works now)

hmmm... for me, the biggest benefit of using sql-like database is the guarantee on data integrity. if we cannot trust that whatever in the database is correct, then it lost a lot of its value to me.

we should have NOT NULL constraints for these two fields for ballot.

it is a benefit for sure but not the most significant. i think that it doesn't matter much because we didn't enforce it with leveldb, so for me it is irrelevant if it will be enforced with sqlite.

what i see as a main benefit is that it is much harder to make mistakes writing custom keys, when i joined almost all mesh dbcode was subtly (and some not so much subtly) broken

but what is the cost of adding them? why not add them? isn't it nice to be able to reap more benefit from sqlite?

added for layers, but can't add them for signature and pubkey because genesis ballot doesn't have them and it messes up some tests.

sql/layers/layers.go

sql/migrations.go

sql/layers/layers.go

sql/database.go

sql/blocks/blocks.go

sql/migrations/1_initial.sql

.github/workflows/ci.yml

.golangci.yml

.github/workflows/ci.yml

mesh/meshdb.go

sql/ballots/ballots.go

sql/blocks/blocks.go

sql/migrations/0001_initial.sql

mesh/mesh.go

sql/migrations/0001_initial.sql

syncer/syncer_test.go

appveyor.yml

hare/hare.go

dshulyak · 2022-01-14T06:52:28Z

bors merge

## Motivation The existing approach lacks atomicity, causal durability (e.g. an atx may not be on disk when a ballot is saved), and the durability can't be enforced in general without running every leveldb operation in sync mode. All this problems will result in subtle bugs that are hard to diagnose. For those 3 requirements, we want to maintain all state in a single db. Moving state to a single leveldb will require us to enforce isolation ourselves (by maintaining separate namespace and manually concatenating keys like we do in some modules), beside that we have to create every single index manually while with sqlite we can just do `CREATE INDEX` and sqlite will do it for us and probably do a better job. Another significant benefit is that we can duplicate some state in sql table to avoid loading the whole structure into memory. For instance it will be relevant for atx, which is a large (10kb) and usually, after it was validated, we want to know only the associated smesher and weight of the atx. This would be problematic with leveldb, and would require adding custom index. related: #2918 ## Changes - general plumbing for core database stuff (db, transaction, migrations) using https://github.com/crawshaw/sqlite that is a relatively simple wrapper around C sqlite - tables and for layers, blocks and ballots - reworked ZeroLayer, it is relevant only for hare_output, which can be in 3 states - nil, empty, non-empty. SetZeroLayer update hare_output to empty state, at which tortoise will vote against all blocks within hdist. - updated to golang 1.16 for `embed` module. note that there is a bug with go mode tidy in 1.16 so i had to manually fix go.sum and disable go mod tidy on ci - golang/go#44129 ## Test Plan existing and new uts

bors · 2022-01-14T07:13:49Z

Build failed:

ci-stage2

dshulyak · 2022-01-14T07:34:23Z

bors merge

## Motivation The existing approach lacks atomicity, causal durability (e.g. an atx may not be on disk when a ballot is saved), and the durability can't be enforced in general without running every leveldb operation in sync mode. All this problems will result in subtle bugs that are hard to diagnose. For those 3 requirements, we want to maintain all state in a single db. Moving state to a single leveldb will require us to enforce isolation ourselves (by maintaining separate namespace and manually concatenating keys like we do in some modules), beside that we have to create every single index manually while with sqlite we can just do `CREATE INDEX` and sqlite will do it for us and probably do a better job. Another significant benefit is that we can duplicate some state in sql table to avoid loading the whole structure into memory. For instance it will be relevant for atx, which is a large (10kb) and usually, after it was validated, we want to know only the associated smesher and weight of the atx. This would be problematic with leveldb, and would require adding custom index. related: #2918 ## Changes - general plumbing for core database stuff (db, transaction, migrations) using https://github.com/crawshaw/sqlite that is a relatively simple wrapper around C sqlite - tables and for layers, blocks and ballots - reworked ZeroLayer, it is relevant only for hare_output, which can be in 3 states - nil, empty, non-empty. SetZeroLayer update hare_output to empty state, at which tortoise will vote against all blocks within hdist. - updated to golang 1.16 for `embed` module. note that there is a bug with go mode tidy in 1.16 so i had to manually fix go.sum and disable go mod tidy on ci - golang/go#44129 ## Test Plan existing and new uts

bors · 2022-01-14T08:33:46Z

Pull request successfully merged into develop.

Build succeeded:

ci-stage2

dshulyak added 3 commits January 7, 2022 09:30

require 1.16

9db0894

add blocks

d239429

add ballots

a3b24c0

dshulyak changed the title ~~Sqlite database~~ sql: store ballots and blocks in sqlite database Jan 7, 2022

dshulyak marked this pull request as draft January 7, 2022 09:06

dshulyak added 25 commits January 9, 2022 09:08

add layers

a5d2a09

add important todo

f40a077

return number of consumed rows from Exec

2bcaa89

update docstrings

ad6ffeb

upgrade crawshaw/sqlite to support returning

f18515b

finish with basic tests

14a924a

update to 1.16 everywhere

eb08d65

make mesh tests pass

7646bc8

fix permissions in benchmarks

227dcd2

tortoise: load validity for all blocks

b0a4d32

go mod tidy

7270b9d

hare output

10b7d37

rework empty layer

3d6774c

change signature and pubkey to varchar

3000b4a

fix expectation in base ballot

5b2f098

Merge branch 'develop' into sqlite-database

6e1f1cc

fixes after merge

678fa13

renamer blocks.Layer to blocks.LayerIDs

813c9a8

cover with basic tests

5526f42

add missing dep

87d4b2a

somethiung is messed up with this dependency

0028fe7

add tx test

3424346

disable tidy test on ci, dependency needs to be added manually

a121c02

golang/go#44129

Merge branch 'develop' into sqlite-database

02dcd3d

ensure directories are created

9b9646a

countvonzero reviewed Jan 12, 2022

View reviewed changes

fixes

1d96819

nkryuchkov reviewed Jan 12, 2022

View reviewed changes

sql/migrations/0001_initial.sql Show resolved Hide resolved

dshulyak added 2 commits January 12, 2022 11:44

update missed go version

b10db8f

convert indentation to 4 spaces

5419e9b

nkryuchkov approved these changes Jan 12, 2022

View reviewed changes

dshulyak added 3 commits January 13, 2022 06:43

add layers_status table

9ccf710

revert change in syncer

f7759ce

deflake syncer tests

466ddeb

countvonzero approved these changes Jan 13, 2022

View reviewed changes

dshulyak added 7 commits January 14, 2022 07:18

1.17 update

2dd6811

fix remaining

7948671

go mod tidy

6fe36cf

1.17 tags changes

461a505

revert not null contrains on sig and pubkey

98cde78

go get after go mod tidy

912a203

fix test

f26b559

Merge branch 'develop' into sqlite-database

6f13036

bors bot changed the title ~~sql: store ballots, blocks and layers in sqlite~~ [Merged by Bors] - sql: store ballots, blocks and layers in sqlite Jan 14, 2022

bors bot closed this Jan 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - sql: store ballots, blocks and layers in sqlite #3047

[Merged by Bors] - sql: store ballots, blocks and layers in sqlite #3047

dshulyak commented Jan 7, 2022 •

edited

Loading

countvonzero commented Jan 12, 2022

dshulyak commented Jan 12, 2022

countvonzero commented Jan 12, 2022

countvonzero left a comment

countvonzero Jan 12, 2022

dshulyak Jan 12, 2022

dshulyak Jan 12, 2022

countvonzero Jan 12, 2022

dshulyak Jan 12, 2022

countvonzero Jan 13, 2022

dshulyak Jan 14, 2022

dshulyak commented Jan 14, 2022

bors bot commented Jan 14, 2022

dshulyak commented Jan 14, 2022

bors bot commented Jan 14, 2022

[Merged by Bors] - sql: store ballots, blocks and layers in sqlite #3047

[Merged by Bors] - sql: store ballots, blocks and layers in sqlite #3047

Conversation

dshulyak commented Jan 7, 2022 • edited Loading

Motivation

Changes

Test Plan

countvonzero commented Jan 12, 2022

dshulyak commented Jan 12, 2022

countvonzero commented Jan 12, 2022

countvonzero left a comment

Choose a reason for hiding this comment

countvonzero Jan 12, 2022

Choose a reason for hiding this comment

dshulyak Jan 12, 2022

Choose a reason for hiding this comment

dshulyak Jan 12, 2022

Choose a reason for hiding this comment

countvonzero Jan 12, 2022

Choose a reason for hiding this comment

dshulyak Jan 12, 2022

Choose a reason for hiding this comment

countvonzero Jan 13, 2022

Choose a reason for hiding this comment

dshulyak Jan 14, 2022

Choose a reason for hiding this comment

dshulyak commented Jan 14, 2022

bors bot commented Jan 14, 2022

dshulyak commented Jan 14, 2022

bors bot commented Jan 14, 2022

dshulyak commented Jan 7, 2022 •

edited

Loading