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

tools: run block generator using a preloaded db #5384

Merged
merged 17 commits into from
May 22, 2023

Conversation

shiqizng
Copy link
Contributor

@shiqizng shiqizng commented May 12, 2023

This PR updates the block generator to work with a db snapshot. The generator creates blocks with synthetic accounts and write to an non-empty database by using an offset value for the round.

@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Merging #5384 (d498cd9) into master (d7c5e54) will decrease coverage by 0.06%.
The diff coverage is 53.19%.

@@            Coverage Diff             @@
##           master    #5384      +/-   ##
==========================================
- Coverage   55.41%   55.36%   -0.06%     
==========================================
  Files         452      452              
  Lines       63643    63672      +29     
==========================================
- Hits        35270    35250      -20     
- Misses      25972    26009      +37     
- Partials     2401     2413      +12     
Impacted Files Coverage Δ
tools/block-generator/generator/server.go 26.74% <0.00%> (-1.31%) ⬇️
tools/block-generator/generator/generate.go 57.86% <62.50%> (+0.58%) ⬆️

... and 9 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@shiqizng shiqizng marked this pull request as ready for review May 15, 2023 16:04
Copy link
Contributor

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

Looks good.

I'm going to try and use this to generate blocks, but before starting that, I had a few questions/comments.

tools/block-generator/generator/utils.go Outdated Show resolved Hide resolved
tools/block-generator/generator/generate.go Outdated Show resolved Hide resolved
tools/block-generator/generator/generate.go Outdated Show resolved Hide resolved
tools/block-generator/generator/generate.go Outdated Show resolved Hide resolved
tools/block-generator/generator/generate.go Outdated Show resolved Hide resolved
tools/block-generator/generator/generate.go Outdated Show resolved Hide resolved
@@ -54,4 +54,6 @@ $(dirname "$0")/block-generator runner \
--test-duration 30s \
--log-level trace \
--postgres-connection-string "host=localhost user=algorand password=algorand dbname=generator_db port=15432 sslmode=disable" \
--scenario ${CONFIG}
--scenario ${CONFIG} \
--db-round ${2:-0} \
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be read from the db at startup time to avoid needing to provide?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, but it seems expensive to create a connection just to read 1 number.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. What's the exact round number I should supply? I can add that to the Makefile I'm introducing in my followup #5394

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RunnerCmd.Flags().Uint64VarP(&runnerArgs.NextDBRound, "next-db-round", "n", 0, "next round on db. It is the next_account_round from Indexer's metastate table.")

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if something like this is going to become more important when running multiple tests back-to-back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it's not working with I provided a scenario directory instead of a scenario file path. I'll fix this in a follow up PR.

Copy link
Contributor

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

LGTM

} else {
prefix = g.genesisID
}
l, err := ledger.OpenLedger(logging.Base(), prefix, true, ledgercore.InitState{
Copy link
Contributor

Choose a reason for hiding this comment

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

just wanted to point out that supplying GetDefaultLocal as configuration to OpenLedger may open you up to underlying changes you don't want/expect. For instance, StorageEngine is being added in this PR, which currently specifies sql, but may specify another engine in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do all storage engines support in memory mode?

timestamp: 0,
rewardsLevel: 0,
rewardsResidue: 0,
rewardsRate: 0,
rewardsRecalculationRound: 0,
reportData: make(map[TxTypeID]TxData),
nextdbround: dbround,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd consider calling this round offset rather than nextdbround since the generator doesn't know about the DB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll address this and the other ones in a follow up PR.

block.BlockHeader.Round = basics.Round(round)
encodedblock := rpcs.EncodedBlockCert{
Block: block,
Certificate: cert,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed? agreement.Certificate{} is used below.

@@ -451,7 +478,16 @@ func (g *generator) WriteBlock(output io.Writer, round uint64) error {

// WriteDeltas generates returns the deltas for payset.
func (g *generator) WriteDeltas(output io.Writer, round uint64) error {
delta, err := g.ledger.GetStateDeltaForRound(basics.Round(round))
// offset round for non-empty database
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// offset round for non-empty database
// the first generataed round has no statedelta.

@@ -13,7 +13,7 @@ fi
POSTGRES_CONTAINER=generator-test-container
POSTGRES_PORT=15432
POSTGRES_DATABASE=generator_db
CONFIG=${2:-"$(dirname $0)/test_config.yml"}
CONFIG=${4:-"$(dirname $0)/test_config.yml"}
Copy link
Contributor

Choose a reason for hiding this comment

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

This script needs a little extra attention if we want to keep using it. Using positional arguments means they're all required and having so many of them gets confusing. As it is, it would be easy to run the script incorrectly.

$1 - conduit binary
$2 - db-round (required if you want a config template)
$3 - genesis file (required if you want a config template)
$4 - config template (optional)

Copy link
Contributor

Choose a reason for hiding this comment

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

My follow up has a python version of this that's a bit more flexible.

Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

Everything looks correct

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.

None yet

4 participants