Skip to content
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.

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

Add startup check to verify existing DB data matches the algod network. #833

Merged
merged 6 commits into from
Jan 25, 2022

Conversation

shiqizng
Copy link
Contributor

@shiqizng shiqizng commented Jan 24, 2022

Summary

Write and check genesis hash during db load genesis.

Test Plan

Add a test to postgres_integration_test

@shiqizng shiqizng self-assigned this Jan 24, 2022
@shiqizng shiqizng linked an issue Jan 24, 2022 that may be closed by this pull request
@shiqizng shiqizng marked this pull request as ready for review January 24, 2022 17:52
@codecov-commenter
Copy link

codecov-commenter commented Jan 24, 2022

Codecov Report

Merging #833 (2b8d1f3) into develop (59e181f) will increase coverage by 0.04%.
The diff coverage is 60.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #833      +/-   ##
===========================================
+ Coverage    59.17%   59.22%   +0.04%     
===========================================
  Files           32       32              
  Lines         4108     4137      +29     
===========================================
+ Hits          2431     2450      +19     
- Misses        1379     1384       +5     
- Partials       298      303       +5     
Impacted Files Coverage Δ
idb/postgres/postgres.go 48.71% <58.33%> (+0.26%) ⬆️
idb/postgres/internal/encoding/encoding.go 75.56% <66.66%> (-0.18%) ⬇️

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 59e181f...2b8d1f3. Read the comment docs.

Copy link
Contributor

@tolikzinovyev tolikzinovyev left a comment

Choose a reason for hiding this comment

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

Is network ID enough? There might be some other parameters. Why not use the genesis hash?

idb/postgres/internal/encoding/encoding.go Outdated Show resolved Hide resolved
idb/postgres/postgres.go Outdated Show resolved Hide resolved
idb/postgres/postgres.go Outdated Show resolved Hide resolved
idb/postgres/postgres.go Outdated Show resolved Hide resolved
idb/postgres/postgres_integration_test.go Outdated Show resolved Hide resolved
@shiqizng
Copy link
Contributor Author

shiqizng commented Jan 24, 2022

Is network ID enough? There might be some other parameters. Why not use the genesis hash?

genesis hash isn't in bookkeeping.Genesis struct and so the hash is not available in LoadGenesis(genesis bookkeeping.Genesis). Maybe it's better to use Genesis.ID()?

// ID is the effective Genesis identifier - the combination
// of the network and the ledger schema version
func (genesis Genesis) ID() string {
	return string(genesis.Network) + "-" + genesis.SchemaID
}

@tolikzinovyev
Copy link
Contributor

Is network ID enough? There might be some other parameters. Why not use the genesis hash?

genesis hash isn't in bookkeeping.Genesis struct and so the hash is not available in LoadGenesis(genesis bookkeeping.Genesis). Maybe it's better to use Genesis.ID()?

// ID is the effective Genesis identifier - the combination
// of the network and the ledger schema version
func (genesis Genesis) ID() string {
	return string(genesis.Network) + "-" + genesis.SchemaID
}

Seems like there is infrastructure for hashing bookkeeping.Genesis. https://github.com/algorand/go-algorand/blob/master/data/bookkeeping/genesis.go#L121 I just don't know how to invoke it.

@shiqizng
Copy link
Contributor Author

Is network ID enough? There might be some other parameters. Why not use the genesis hash?

genesis hash isn't in bookkeeping.Genesis struct and so the hash is not available in LoadGenesis(genesis bookkeeping.Genesis). Maybe it's better to use Genesis.ID()?

// ID is the effective Genesis identifier - the combination
// of the network and the ledger schema version
func (genesis Genesis) ID() string {
	return string(genesis.Network) + "-" + genesis.SchemaID
}

Seems like there is infrastructure for hashing bookkeeping.Genesis. https://github.com/algorand/go-algorand/blob/master/data/bookkeeping/genesis.go#L121 I just don't know how to invoke it.

I changed the check to use genesis hash. I removed network ID check since the hash includes the ID also.

idb/postgres/internal/encoding/encoding_test.go Outdated Show resolved Hide resolved
idb/postgres/postgres.go Outdated Show resolved Hide resolved
idb/postgres/postgres.go Outdated Show resolved Hide resolved
Copy link
Contributor

@tolikzinovyev tolikzinovyev left a comment

Choose a reason for hiding this comment

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

LGTM

@winder winder changed the title added network check Add startup check to verify existing DB data matches the algod network. Jan 25, 2022
@winder winder merged commit 014875a into develop Jan 25, 2022
@winder winder deleted the shiqi/metastate branch January 25, 2022 18:51
This was referenced Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Verify network when starting Indexer
4 participants