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: bugfix block-generator to handle conduit's Init block requests #5449

Merged
merged 15 commits into from Jun 7, 2023

Conversation

tzaffi
Copy link
Contributor

@tzaffi tzaffi commented Jun 3, 2023

Summary

Conduit's recent PR: algorand/conduit#89 broke the assumption that block-generator was making that all block requests must advance the ledger. This PR addresses this breakage by allowing more leniency to a caller which requests exactly the offset round, even if the internal counter has already advanced.

Test Plan

Unit Tests + WOMM (both on empty / non-empty DB)

How to test locally

(using Makefile and run_runner.py of #5450)

❯ make debug-blockgen
...
❯ make run-runner
...
Running test for configuration 'test_config.yml'
generator serving on localhost:11112
Received round request 0, but already at virtualRound 1 (== g.round + g.roundOffset == 1 + 0). Not finishing round.
exiting block generator runner: signal: interrupt
Done running tests!

❯ make enter-pg
...
psql (14.5 (Debian 14.5-1.pgdg110+1))

generator_db=# select count(*) from account;
...
generator_db=# select count(*) from account_asset;
...
generator_db=# select count(*) from block_header;
...

@tzaffi tzaffi changed the title bugfix: block-gen tool handles conduit's Init block requeist NOT READY FOR REVIEW bugfix: block-gen tool handles conduit's Init block requeist Jun 3, 2023
@codecov
Copy link

codecov bot commented Jun 3, 2023

Codecov Report

Merging #5449 (c2a3100) into master (1994341) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5449      +/-   ##
==========================================
- Coverage   55.47%   55.45%   -0.02%     
==========================================
  Files         447      447              
  Lines       63290    63290              
==========================================
- Hits        35108    35096      -12     
- Misses      25800    25820      +20     
+ Partials     2382     2374       -8     

see 16 files with indirect coverage changes

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

@algorandskiy algorandskiy changed the title NOT READY FOR REVIEW bugfix: block-gen tool handles conduit's Init block requeist WIP bugfix: block-gen tool handles conduit's Init block requeist Jun 4, 2023
@tzaffi tzaffi changed the title WIP bugfix: block-gen tool handles conduit's Init block requeist bugfix: block-gen tool handles conduit's Init block request Jun 4, 2023
@tzaffi tzaffi added the Bug-Fix label Jun 4, 2023
@tzaffi tzaffi changed the title bugfix: block-gen tool handles conduit's Init block request tools: bugfix block-generator to handle conduit's Init block requests Jun 4, 2023
@tzaffi tzaffi marked this pull request as ready for review June 5, 2023 03:50
@@ -62,7 +62,7 @@ const (

assetTotal = uint64(100000000000000000)

consensusTimeMilli int64 = 4500
consensusTimeMilli int64 = 3300
Copy link
Contributor Author

Choose a reason for hiding this comment

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

recent consensus upgrade

@tzaffi tzaffi requested a review from a team June 5, 2023 14:47
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.

I'd just like to see the one comment added. Otherwise LGTM. Thanks for the explanation during standup.

shiqizng
shiqizng previously approved these changes Jun 6, 2023
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.

Left some comments, the conditions here were hard for me to follow

tools/block-generator/generator/generate.go Outdated Show resolved Hide resolved
}
numTxnForBlock := g.txnForRound(g.round)
virtualRound := g.round + g.roundOffset
if round+1 < virtualRound || virtualRound < round {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm finding this expression very difficult to understand. I think this expresses the same thing with fewer conditions

Suggested change
if round+1 < virtualRound || virtualRound < round {
// return cached round if available
if round - g.roundOffset + 1 == g.round && len(g.latestBlockMsgp) != 0{
_, err := output.Write(g.latestBlockMsgp)
if err != nil {
return err
}
}
// "genesis" round
if round-g.roundOffset == 0 {
}
// previous logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interestingly, for me, it's more difficult to grok this new approach. I guess each of our brains has different embedded neural nets for such inequalities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to integrate this approach landed at a compromise, hopefully clearer. Introducing nextRound and cachedRound variables: 3c4fc1c

Co-authored-by: Will Winder <wwinder.unh@gmail.com>
@tzaffi tzaffi requested review from winder and shiqizng June 6, 2023 22:53
nextRound := g.round + g.roundOffset
cachedRound := nextRound - 1

if round != nextRound && round != cachedRound {
Copy link
Contributor

Choose a reason for hiding this comment

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

ty 👍

@winder winder merged commit 1230e9a into algorand:master Jun 7, 2023
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants