Adding new load test type for constant sized transactions by Tonix517 · Pull Request #2816 · onflow/flow-go

Tonix517 · 2022-07-18T17:19:09Z

Summary

As part of Dynamic Inclusion Fee feature development (more details can be found at https://www.notion.so/dapperlabs/Variable-Transaction-Fees-Inclusion-Effort-I-20c849105a3945879824e386cc9c3ebf by @janezpodhostnik ), we will need to get proper co-efficient values for the new inclusion fee equation that can fit well with our production transactions with different sizes. Before we do that in production, we will need to be able to create tx with given sizes sent to devnet\testnet to get initial metrics.

This PR adds a new loader test type that allows us to create and send tx with given size.

Test Plan

launch localnet and launch loader with the new test type. monitor dashboards and EN logs to make sure tx went through.

set different tx size parameters and monitor actual tx size

pattyshack · 2022-07-18T17:53:02Z

It's hard to figure out what really changed when many changes happen all in the same PR. Could you break this PR into smaller / coherent pieces? e.g., refactoring sendAddKeyTx into createAddKeyTx & sendAddKeyTx can be one PR. moving inline cadence templates into files can be one (or multiple) PR.

I promise to review the PRs quickly. thx

pattyshack · 2022-07-18T17:29:33Z

+	loadType                   LoadType
+	follower                   TxFollower
+	availableAccountsLo        int
+	maxConstExecTxSizeInByte   uint


the number of arguments passed into NewContLoadGenerator is getting a bit out of hand. could you group these fields into a sub-struct? e.g.,

type ConstExecTx struct { maxSizeInByte authAccNum argSizeInByte payerKeyCount }

pattyshack · 2022-07-18T17:34:35Z

+	// check and cap params for const-exec mode
+	if loadType == ConstExecCostLoadType {
+		if maxConstExecTxSizeInByte > flow.DefaultMaxTransactionByteSize {
+			log.Warn().Msg(


just return error instead of fudging the value? same below

pattyshack · 2022-07-18T17:42:59Z

-	if err != nil {
-		lg.log.Error().Err(err).Msg("failed to setup fav contract")
-		return err
+	if lg.loadType != ConstExecCostLoadType {


not specific to this pr (it's probably something for @AndrewM-SDET to think about). With more and more load types being added to the load generator, we may want to refactor ContLoadGenerator into smaller load type specific units.

At the moment we are probably ok but this is absolutely worth adding to the backlog. I've created a new issue for it at #2825

Tonix517 · 2022-07-18T23:17:30Z

done at #2817

It's hard to figure out what really changed when many changes happen all in the same PR. Could you break this PR into smaller / coherent pieces? e.g., refactoring sendAddKeyTx into createAddKeyTx & sendAddKeyTx can be one PR. moving inline cadence templates into files can be one (or multiple) PR.

I promise to review the PRs quickly. thx

Tonix517 · 2022-07-18T23:18:54Z

done at #2817

It's hard to figure out what really changed when many changes happen all in the same PR. Could you break this PR into smaller / coherent pieces? e.g., refactoring sendAddKeyTx into createAddKeyTx & sendAddKeyTx can be one PR. moving inline cadence templates into files can be one (or multiple) PR.
I promise to review the PRs quickly. thx

BTW I split original large commit into smaller individual commits but still keeping them into the same PR because these changes can be logically grouped.

pattyshack · 2022-07-19T04:00:08Z

+	return nil
+}
+
+func (lg *ContLoadGenerator) getTxSizeWithoutCommnent() uint {


maybe rename to getConstExecCostTxSizeWithoutComment?

pattyshack · 2022-07-19T04:03:20Z

+	authorizerListStr := generateAuthAccountParamList(lg.constExecParam.AuthAccountNum)
+	oneSignatureLen := uint(flow.AddressLength) + flowCrypto.SignatureLenECDSAP256 + 8 /* keyId is uint64 */
+
+	return lg.constExecParam.ArgSizeInByte +


is there a better way to get the size estimate? this seems very brittle.

Janez and I discussed about this. Basically we want to know the size of binary data that takes actual network resources. Our goal is to reach the network saturation point and then collect some metrics. I will leave it to @janezpodhostnik for more info

For the required measurements the actual absolute size is not that important. Let me explain what I mean:

If we write the "size" of the transaction as $T_0 + \Delta T_x$ the minimum size of a transaction with no parameters, comments, authorisers,... + the variable part. The quantity we will measure is the max TPS (the saturation point) of $T_x$ $S_x$ , and what we really interested in is $K$ where $C + K (T_0 + \Delta T_x) = S_x$. Which we can get with:

$$ \displaylines{ C + K (T_0 + \Delta T_1) &=& S_1 \\ C + K (T_0 + \Delta T_2) &=& S_2 \\ (\Delta T_1-\Delta T_2) K &=& S_1-S_2} $$

In general the "size" is a vector of all the different parameters, and we have more than two points, but the point is the accuracy of the constant size of the transaction is not important as we are only interested in the relative size.

However I agree this might be brittle and confusing, so we could instead just not count the constant parts, and only count the dynamic parts:

lg.constExecParam.ArgSizeInByte + generateAuthAccountParamList(lg.constExecParam.AuthAccountNum) + lg.constExecParam.AuthAccountNum*oneSignatureLen + lg.constExecParam.PayerKeyCount*oneSignatureLen

This will make the comparison lg.constExecParam.MaxTxSizeInByte < txSizeWithoutComment inaccurate, but if the constant parts are not that big we can just rewrite it to: lg.constExecParam.MaxTxSizeInByte < txRelativeSizeWithoutComment + TXConstantSizeEstimate.

thanks for the info. Code refined.

can we just marshal the payload without sending it, and get the size from that?

addressed - now tx size is calculated by RLP encoding which is one simple API call away: tx.Encode(). Using protobuf encoding to calculate the size seems to be a little more complex and adds extra assumption that we will be using protobuf for data transfer.

pattyshack · 2022-07-19T04:13:27Z

+		SetPayer(*lg.accounts[0].address)
+	lg.accounts[0].seqNumber += 1
+
+	txArgStr := generateCadenceCommentWithLen(lg.constExecParam.ArgSizeInByte)


using generateCadenceCommentWithLen here is very confusing. maybe create an alias for the function, e.g., generateRandomStringWithLen

o, better yet, maybe move // from generateCadenceCommentWithLen into the template, and just have generateRandomStringWithLen

neat idea - done

janezpodhostnik

Some minor comments. Looks good otherwise.

janezpodhostnik · 2022-07-19T13:32:03Z

 	CompHeavyLoadType     LoadType = "computation-heavy"
 	EventHeavyLoadType    LoadType = "event-heavy"
 	LedgerHeavyLoadType   LoadType = "ledger-heavy"
+	ConstExecCostLoadType LoadType = "const-exec"


It might be nice to add a short comment of what this load type is supposed to do here. Just so it's easier to understand for the next person.

done - put a short comment.

janezpodhostnik · 2022-07-19T13:36:51Z

+	if loadType == ConstExecCostLoadType {
+		if constExecParam.MaxTxSizeInByte > flow.DefaultMaxTransactionByteSize {
+			errMsg := fmt.Sprintf("MaxTxSizeInByte(%d) is larger than DefaultMaxTransactionByteSize."+
+				"Resetting it back to DefaultMaxTransactionByteSize(%d).",


This states Resetting it back to DefaultMaxTransactionByteSize(%d). then returns an error. According to the error text, I expected this to change the input parameter to flow.DefaultMaxTransactionByteSize. Is it expected that the user makes this correction?

The same for the following two errors.

janezpodhostnik · 2022-07-19T14:17:28Z

+	authorizerListStr := generateAuthAccountParamList(lg.constExecParam.AuthAccountNum)
+	oneSignatureLen := uint(flow.AddressLength) + flowCrypto.SignatureLenECDSAP256 + 8 /* keyId is uint64 */
+
+	return lg.constExecParam.ArgSizeInByte +


For the required measurements the actual absolute size is not that important. Let me explain what I mean:

If we write the "size" of the transaction as $T_0 + \Delta T_x$ the minimum size of a transaction with no parameters, comments, authorisers,... + the variable part. The quantity we will measure is the max TPS (the saturation point) of $T_x$ $S_x$ , and what we really interested in is $K$ where $C + K (T_0 + \Delta T_x) = S_x$. Which we can get with:

$$ \displaylines{ C + K (T_0 + \Delta T_1) &=& S_1 \\ C + K (T_0 + \Delta T_2) &=& S_2 \\ (\Delta T_1-\Delta T_2) K &=& S_1-S_2} $$

In general the "size" is a vector of all the different parameters, and we have more than two points, but the point is the accuracy of the constant size of the transaction is not important as we are only interested in the relative size.

However I agree this might be brittle and confusing, so we could instead just not count the constant parts, and only count the dynamic parts:

lg.constExecParam.ArgSizeInByte + generateAuthAccountParamList(lg.constExecParam.AuthAccountNum) + lg.constExecParam.AuthAccountNum*oneSignatureLen + lg.constExecParam.PayerKeyCount*oneSignatureLen

This will make the comparison lg.constExecParam.MaxTxSizeInByte < txSizeWithoutComment inaccurate, but if the constant parts are not that big we can just rewrite it to: lg.constExecParam.MaxTxSizeInByte < txRelativeSizeWithoutComment + TXConstantSizeEstimate.

pattyshack

much better. thx

janezpodhostnik

LGTM!

…til function

Tonix517 requested a review from janezpodhostnik July 18, 2022 17:19

Tonix517 requested review from DEPRICATED-Dapperlabs-ex-employee, Kay-Zee, SaveTheRbtz, pattyshack and simonhf as code owners July 18, 2022 17:19

Tonix517 changed the title ~~Adding new load test type for constant sized tx~~ Adding new load test type for constant sized transactions Jul 18, 2022

Tonix517 force-pushed the tonyz/new_load_type_empty_tx branch from bf107d6 to 4109d7a Compare July 18, 2022 17:33

pattyshack reviewed Jul 18, 2022

View reviewed changes

Tonix517 force-pushed the tonyz/new_load_type_empty_tx branch from 4109d7a to d187893 Compare July 18, 2022 23:16

pattyshack reviewed Jul 19, 2022

View reviewed changes

Tonix517 force-pushed the tonyz/new_load_type_empty_tx branch from d187893 to 9b68f41 Compare July 19, 2022 05:21

janezpodhostnik reviewed Jul 19, 2022

View reviewed changes

Tonix517 force-pushed the tonyz/new_load_type_empty_tx branch 3 times, most recently from 925a4e6 to f6adc62 Compare July 20, 2022 21:15

pattyshack approved these changes Jul 20, 2022

View reviewed changes

SaveTheRbtz approved these changes Jul 21, 2022

View reviewed changes

janezpodhostnik approved these changes Jul 21, 2022

View reviewed changes

Tonix517 force-pushed the tonyz/new_load_type_empty_tx branch from f6adc62 to 58ed8a3 Compare July 21, 2022 22:16

Tony Z added 5 commits July 21, 2022 16:23

[Integration] Adding const-exec transaction template and its utils

d333009

[Integration] Adding new parameters for const-exec

655b691

[Integration] separating add-key transaction creation into separate u…

2bd6108

…til function

[Integration] Adding const-exec main logic

046f216

[Integration] using RLP encoding to calculate tx size

222fd3c

Tonix517 force-pushed the tonyz/new_load_type_empty_tx branch from 58ed8a3 to 222fd3c Compare July 21, 2022 23:23

Tonix517 merged commit 29b7b97 into master Jul 22, 2022

Tonix517 deleted the tonyz/new_load_type_empty_tx branch July 22, 2022 04:51

Conversation

Tonix517 commented Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

pattyshack commented Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tonix517 commented Jul 18, 2022

Uh oh!

Tonix517 commented Jul 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janezpodhostnik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pattyshack left a comment

Choose a reason for hiding this comment

Uh oh!

janezpodhostnik left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Tonix517 commented Jul 18, 2022 •

edited

Loading

pattyshack commented Jul 18, 2022 •

edited

Loading