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: block generator inner transactions #5506

Merged
merged 35 commits into from Jun 30, 2023

Conversation

tzaffi
Copy link
Contributor

@tzaffi tzaffi commented Jun 27, 2023

Summary

This is a bugfix as inner transactions weren't actually being saved to the database after #5478 . The main problem was that only ledger.AddBlock() was used to persist blocks which were missing ApplyData. This function doesn't modify the block which it is adding (it can evaluate a different block for validation purposes though). The fix works by using logic similar to the transaction pool and the recent simulator:

  1. use eval.StartEvaluator() with generate = true to instantiate a block evaluator
  2. call eval.TransactionGroup() on each group that the generator provides
  3. call eval.GenerateBlock() at the end to create a validated block which contains the evaluated ApplyData with inner transactions
  4. call ledger.AddValidatedBlock() to persist.

Additional changes have been hilighted in my comments below.

TODO's Carried Over

The ones that haven't yet been done should be carried over to Part 3: swaps

  • remove all explicit ApplyData settings/params as Ledger.Eval() should be in charge of this now and all existing references are empty (generator.go)
  • the values of the effects map should be slices of a type different from TxTypeID (DONE: now they're plain strings)
  • the generator only uses effects map to update txnCounter (or intra) NOPE: this is now being updated from the block's evaluator directly. However, there is still an assertion that the number of expected transactions is what the evaluator provided.
  • there is a unit test that asserts the total number of transactions for each TxTypeID with effects is exactly as expected (DONE: see TestEffectsMap )
  • the effects are only used at report time in run.go to split out the various effects
  • introduce generate_apps.go for application specific generator code
  • introduce generator_ledger.go for ledger specific generator code
  • return the number of transactions generated instead of updating intra (generator.go)
  • README improvements:
    • incorporate spirit of the comment below explaining the complexity of getActualAppCall()
    • an in-depth explanation of how the block generator works
  • need to update explanatory comment for the effects map in the next PR for because of the new teal files and updated names
  • "these may not make sense for the swap optin" (make_transactions.go)
  • add sanity checks similar to the one for the case appTxTypeCreate for the other cases (generateAppCallInternal())
  • figure out if there is some simple code organization to help with handling the dependencies between states of applications. Maybe we can plan for some larger architecture changes in the future (i.e. using the ledger, it wasn't available in earlier versions of this tool).
  • remove type appData struct field kind if still unused after Part 3 (generator_types.go)
  • lines 319-334 of generate.go should become: vBlock, ledgerTxnCount, err := g.generateBlock()
  • OR: generating the tx groups is part of generating the block, so could push 319-340 into g.evaluateBlock, and rename evaluateBlock to generateBlock
  • rand.Seed has been deprecated since Go 1.20 and an alternative has been available since Go 1.0: Programs that call Seed and then expect a specific sequence of results from the global random source (using functions such as Int) can be broken when a dependency changes how much it consumes from the global random source. To avoid such breakages, programs that need a specific result sequence should use NewRand(NewSource(seed)) to obtain a random generator that other packages cannot access. (SA1019)

Test Plan

In addition to some new unit tests, I've tested it locally using the same method described on #5478

FAQ

Q: How to calculate the number of inner transactions in the database?

A:

select count(*) from txn where extra->'root-txid' is not null;

for k := range effects {
keys = append(keys, k)
}
sort.Strings(keys)
Copy link
Contributor Author

@tzaffi tzaffi Jun 28, 2023

Choose a reason for hiding this comment

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

Now sorting the transactions portion of the local report and adding a sum-total including "effects". For example:

scenario:config.appboxes.small.yml
test_duration_seconds:12
test_duration_actual_seconds:12.022910
transaction_app_boxes_call_total:496
transaction_app_boxes_create_total:106
transaction_app_boxes_optin_total:4260
transaction_effect_inner_tx_total:8520
transaction_effect_payment_sibling_total:4366
transaction_ALL_total:17748
...

@codecov
Copy link

codecov bot commented Jun 28, 2023

Codecov Report

Merging #5506 (c5ae50b) into master (74b5f19) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5506      +/-   ##
==========================================
- Coverage   55.83%   55.81%   -0.02%     
==========================================
  Files         446      446              
  Lines       63223    63223              
==========================================
- Hits        35299    35287      -12     
- Misses      25561    25572      +11     
- Partials     2363     2364       +1     

see 12 files with indirect coverage changes

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

@tzaffi tzaffi marked this pull request as ready for review June 28, 2023 04:24
@tzaffi tzaffi requested review from shiqizng and winder June 28, 2023 04:58
shiqizng
shiqizng previously approved these changes Jun 29, 2023
Copy link
Contributor

@shiqizng shiqizng left a comment

Choose a reason for hiding this comment

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

lgtm.

tools/block-generator/generator/generate.go Outdated Show resolved Hide resolved
tools/block-generator/generator/generate.go Outdated Show resolved Hide resolved
Co-authored-by: shiqizng <80276844+shiqizng@users.noreply.github.com>
Comment on lines +319 to +322
intra := uint64(0)
txGroupsAD := [][]txn.SignedTxnWithAD{}
for intra < minTxnsForBlock {
txGroupAD, numTxns, err := g.generateTxGroup(g.round, intra)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the txgroup stuff get pushed into g.evaluateBlock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps lines 319-332 could become:

intra, txGroupsAD := g.generateTxGroups()

Or perhaps lines 319-334 could become:

vBlock, ledgerTxnCount, err := g.generateBlock()

Any other ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

generating the tx groups is part of generating the block, so I was thinking you could push 319-340 into g.evaluateBlock, and rename evaluateBlock to generateBlock

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.

Just a few nits, I don't think they need to be addressed in this PR.

@winder winder merged commit 5ed50c4 into algorand:master Jun 30, 2023
22 checks passed
@tzaffi tzaffi deleted the tools-blockgen-innertxns branch June 30, 2023 18:21
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