-
Notifications
You must be signed in to change notification settings - Fork 466
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 apps. Part 2: boxes #5478
Conversation
89f751b
to
fbdfafb
Compare
var effects map[TxTypeID][]TxEffect | ||
|
||
func init() { | ||
effects = map[TxTypeID][]TxEffect{ |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: shiqizng <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #5478 +/- ##
==========================================
+ Coverage 55.30% 55.63% +0.33%
==========================================
Files 446 446
Lines 63153 63153
==========================================
+ Hits 34924 35135 +211
+ Misses 25864 25641 -223
- Partials 2365 2377 +12 see 25 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
tools/block-generator/Makefile
Outdated
block-generator: clean | ||
go build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider moving this to the first target so that make
with no arguments builds the binary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Special TxTypeID's recording effects of higher level transactions | ||
effectPaymentTxSibling TxTypeID = "effect_payment_sibling" | ||
effectInnerTx TxTypeID = "effect_inner_tx" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these configurable? I believe the TxTypeID
was previously used for the different types of transactions that would be chosen according to the ratios. This seems more like something that would be in GenerationConfig
like tx_per_block
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right that it's better to keep TxTypeID
for types of transactions that can actually be chosen. I'll work to carve out a separate types for these effects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, one issue here is that I do need to record the effects and these get stored in a type Report map[TxTypeID]TxData
whose key is of type TxTypeID
. So it does complicates things a bit. If we relax Report
to have keys of type string
, I still ought to be able to separate it out. I'll keep you posted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original concern will be addressed in a follow up PR. Here's a commit: tzaffi@291d30f
g.txnCounter += txnCount | ||
// startRound updates the generator's txnCounter based on the latest block header. | ||
// It is assumed that g.round has already been incremented in finishRound() | ||
func (g *generator) startRound() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we unable to manually track the txn counter? There should be a known number of inner transactions. Does it match how many the ledger things there are?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great question and deserves a good answer. I'll investigate a little further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some investigation, I believe it would be difficult to achieve this without some significant downsides. This involves opening up BlockEvaluator.Eval()
to get the TxnCounter
directly from the evaluator. There is -in fact- a method BlockEvaluator.TestingTxnCounter() which is meant for this purpose. However, accessing it means interacting with the BlockEvaluator
that's created during the ledger's call to eval.Eval()
. One relatively non-invasive way I see of achieving this is to modify interface EvalTracer
's AfterBlock()
to take in a new parameter newTxnCounter
that will let us trace this value and then use it in the new method that I'm introducing ledgerAddBlock(). This feels like overkill to me though. Notice that the same method does introduce an improvement: we calculate the number of transactions in the block by looking at all the actual transactions including inners that were evaluated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading a bit more carefully the original comment, it's arguable that the follow up PR DOES address the concern as it:
- gets the theoretical transaction count using
effects
- gets the actual transaction count from
ledgerAddBlock()
- compares them and errors if they're unequal
In a perfect world, we wouldn't have to re-count the transactions and get them directly from the block evaluator, but I think this is good enough going forward.
g.timestamp += consensusTimeMilli | ||
g.round++ | ||
|
||
// Apply pending assets... | ||
g.assets = append(g.assets, g.pendingAssets...) | ||
g.pendingAssets = nil | ||
|
||
// Apply pending apps... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment with some more explanation about whats going on here would be helpful.
I don't think the loops and cleverness around maps & kinds are worth it here. It hurts clarity and I'm not even sure if it's shorter code.
Did you do it this way because you're imagining lots of additional kinds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic around keeping track of the state of each application in the ledger is the most complex part of this PR. It's about to get significantly more complex as being able to generate an AMM swap requires 7 setup steps prior and therefore much more ability to keep track of state. It's worth a discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's figure out if there is some simple code organization to help with that for now. 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).
It isn't production code, so introducing tech debt here is more acceptable than some of our other systems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This depends on where the AMM ends up, so I would defer as a future discussion. I've created a TODO for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran the generator and see boxes. Nice!
I'm OK merging this in even if there is followup work.
// | ||
// appBoxesCreate: 1 sibling payment tx | ||
// appBoxesOptin: 1 sibling payment tx, 2 inner tx | ||
var effects map[TxTypeID][]TxEffect = map[TxTypeID][]TxEffect{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
var effects map[TxTypeID][]TxEffect = map[TxTypeID][]TxEffect{ | |
var effects = map[TxTypeID][]TxEffect{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -93,8 +120,19 @@ func MakeGenerator(dbround uint64, bkGenesis bookkeeping.Genesis, config Generat | |||
gen.genesisHash = bkGenesis.Hash() | |||
} | |||
|
|||
gen.apps = make(map[appKind][]*appData) | |||
gen.pendingApps = make(map[appKind][]*appData) | |||
gen.resetPendingApps() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think resetPendingApps is needed here. could you also remind me why we need both appSlice and appMap?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
appSlice
is used to pick a random app. appMap
is used for
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resetPendingApps()
also sets gen.PendingApps[of each kind]
and gen.PendingAppMap[of each kind]
as we're appending to nil maps of other types panics (as opposed to appending to a nil slice)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tzaffi could we move some methods to helpers file or a file that's just for app scenario? this file is getting large and it's harder to review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The future follow up PR will introduce the following files to break up the code a bit:
I've also introduced TODO's for this PR's description
@@ -763,110 +876,242 @@ func (g *generator) generateAssetTxnInternalHint(txType TxTypeID, round uint64, | |||
fmt.Printf("\n\nthe sender account does not have enough algos for the transfer. idx %d, asset transaction type %v, num %d\n\n", senderIndex, actual, g.reportData[actual].GenerationCount) | |||
os.Exit(1) | |||
} | |||
|
|||
if assetID == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when do you expect this to happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only happens if the assetID didn't get assigned correctly. That is, if I introduced a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add "this should never happen"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: shiqizng <[email protected]>
// TODO: should check for min balance} | ||
g.balances[senderIndex] -= (pstFee + pstAmt) | ||
|
||
return txntest.Group(&createTxn, &paySibTxn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the description you mention that this transaction has two inner transactions. Are you referring to the transaction group containing two transactions?
Looking at blocks generated by the daemon I see pairs of appl/pay transactions but they do not create any inner transactions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The future followup PR exposes the inner transactions and even prints out a count in verbose mode.
# transaction distribution | ||
# tx_pay_fraction: 0.01 | ||
# tx_asset_fraction: 0.01 | ||
# tx_app_fraction: 0.98 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can these lines be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if err != nil { | ||
panic(fmt.Sprintf("failed to generate transaction: %v\n", err)) | ||
// return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
incorporated in future PR. Commit: f631aec
panic(fmt.Sprintf("failed to encode transaction: %v\n", err)) | ||
if len(signedTxns) == 0 { | ||
return fmt.Errorf("failed to generate transaction: no transactions given") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line can be removed? the next condition, if len(signedTxns) != len(ads)
, should be sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future PR, this line is being kept, but the other condition if len(signedTxns) != len(ads)
is going away because there are no ads
any more.
@@ -93,8 +120,19 @@ func MakeGenerator(dbround uint64, bkGenesis bookkeeping.Genesis, config Generat | |||
gen.genesisHash = bkGenesis.Hash() | |||
} | |||
|
|||
gen.apps = make(map[appKind][]*appData) | |||
gen.pendingApps = make(map[appKind][]*appData) | |||
gen.resetPendingApps() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tzaffi could we move some methods to helpers file or a file that's just for app scenario? this file is getting large and it's harder to review.
I'm ok with having a follow up PR to address the TODOs. I believe this PR works as intended. I ran the appboxes scenario locally and saw the rows in app_box table. |
Summary
Enable opting in and calling box apps and populate database table
app_box
which was the last remaining table that wasn't populated.Changes
block-generator
arguments:--log-level
to--conduit-log-level
--verbose
tools/block-generator/generator/config.go
:Stringer
stools/block-generator/generator/generate.go
:gen.appMap
gen.appSlice
gen.pendingAppMap
gen.pendingAppSlice
gen.startRound()
mostly to keep the generator'stxnCounter
in sync with the Ledger'snumTxnForBlock := g.txnForRound(g.round)
is no longer the fixed block size but instead becomes a LOWER BOUNDgen.introspectLedgerVsGenerator()
which is only called under--verbose
and prints out debugging information comparing the generator's state with its ledger's stat.tools/block-generator/generator/make_transactions.go
:makeAppCreateTxn()
- handles an additional pay txn to fund the app in the case of creating a boxes appmakeAppOptinTxn()
which also:makeAppCallTxn()
which is a ABI call that logs the box contentstools/block-generator/generator/teal/poap_boxes.teal
config.XYZ.(small|jumbo).yml
config.allmixed.jumbo.yml
- includes all kinds of transactionsconfig.allmixed.small.yml
config.appboxes.jumbo.yml
TODO's ...
...which were inherited from PR Part 1
app_boxes
...which have been created in this PR
ApplyData
settings/params asLedger.Eval()
should be in charge of this now and all existing references are empty (generator.go
)intra
(generator.go
)type appData struct
fieldkind
if still unused after Part 3 (generator_types.go
)case appTxTypeCreate
for the other cases (generateAppCallInternal()
)make_transactions.go
)effects
map in the next PR for because of the new teal files and updated nameseffects
map should be slices of a type different fromTxTypeID
effects
map to updatetxnCounter
(orintra
)TxTypeID
with effects is exactly as expectedrun.go
to split out the various effectsgenerate_apps.go
for application specific generator codegenerate_ledger.go
for ledger specific generator codeTest Plan
Locally test as follows: