feat: tx simulator #1613

cmwaters · 2023-04-11T19:23:40Z

This PR introduces a new package txsim for contolled fuzz testing at a transaction level. It's purpose is to simulate a wide range of possible user interactions while also being able to apply a considerable load to the network.

How it works

The package is designed in a way to make it easy to compose new fuzzing scenarios, removing the complexity around account nonces and balances, signing, encoding and broadcasting. The basic element is a Sequence. This is an interface that represents a pattern of recursive messages (one could be I send 5 dollars to you and you send 5 dollars back and so forth). Each sequence is provisioned a set of accounts that are responsible for formulating the messages. The accounts are isolated, allowing for a single sequence to be replicated hundreds of times. Sequences are also provided access to the current state as well as a deterministic source of randomness for constructing their output messages.

There are three current sequences:

Blob: a single account create random permutations of PayForBlob messages
Send: two or more accounts transferring funds to one another
Stake: a single account continuously staking, claiming and redelegating

Remaining work:

Create a command line version of the tool
Explore the need for persistence and garbage collection of accounts
Look to brainstorm useful sequences such as goverance proposals and authz.

cmwaters · 2023-04-11T20:09:02Z

Looks like there's some data race occurring but I'm not sure if it's actually triggered from the new code I wrote

…al/fuzz

testing/txsim/run_test.go

evan-forbes

mostly non blocking comments and questions

ideally we can use the testing framework here to replace the integration test (and the smoke test)

I think it would also help to simply clarify the different testing directories, such as testutil, testing, and the test directory in app.

related to
#832 #1162 #829

evan-forbes · 2023-04-12T17:10:07Z

testing/txsim/client.go

+	pollTime   time.Duration
+
+	mtx         sync.Mutex
+	sequence    int


can we somehow clarify what type of sequence this is referring to? is this a testing sequence? a cosmos-sdk sequence (nonce)? a local count of successfully submitted txs?

Yup, sorry for the misunderstanding. This is simply used to multiplex queries across multiple RPC clients (via round robin)

testing/txsim/client.go

evan-forbes · 2023-04-12T17:13:39Z

testing/txsim/client.go

+func (tc *TxClient) next() {
+	tc.sequence = (tc.sequence + 1) % len(tc.rpcClients)
+}


please add a comment describing the purpose of this logic. also, in other places we use the mutex before mutating local txclient state (such as the height), do we need to do that here? if not, can we include that in the comment?

evan-forbes · 2023-04-12T17:15:54Z

testing/txsim/send.go

+	sendAmount     int
+	maxHeightDelay int
+	accounts       []types.AccAddress
+	sequence       int


similar to above, we're using the word sequence for multiple things, which is fine but preferably we document what we mean here

or rename variable to disambiguate. One candidate rename for the Sequence interface is OperatorGenerator

testing/txsim/sequence.go

evan-forbes · 2023-04-12T17:35:35Z

testing/txsim/client.go

+func (qc *QueryClient) next() {
+	qc.sequence = (qc.sequence + 1) % len(qc.connections)
+}


should we document with these that they are relying on the callers to handle mutexes since these are mutating state?

evan-forbes · 2023-04-12T17:49:42Z

testing/txsim/account.go

+	// increment the sequence number for all the signers
+	for _, signer := range signers {
+		am.accounts[signer.String()].Sequence++
+	}


does this action need to be protected by a mutex?

Yeah perhaps. The same accounts should never be used by any two concurrent processes but because there could be concurrent writes to the map, this may panic

testing/txsim/client.go

evan-forbes · 2023-04-12T18:14:52Z

testing/txsim/account.go

+
+	builder.SetFeeAmount(types.NewCoins(types.NewInt64Coin(app.BondDenom, feeAmount)))
+	// the master account is responsible for paying the fees
+	builder.SetFeeGranter(am.masterAccount.Address)


is there a benefit to using fee granter for every tx instead of funding the accounts in some other mechansim (either by including them in the genesis via the existing tooling in testnode or just sending them funds)?

Ideally, I would like the developer implementing a sequence to not have to worry about fees (they simply state the transactions they want to execute) thus I felt using feegrant to be the best solution. Do you have a problem with feegrant?

that makes sense, but what is the difference if their account is adequately funded? They still don't have to worry about gas limit or the specifying a fee, since we're doing that here.

I highly highly doubt it, but there might be some weird scenario where all of our tests use feegrant, so we missed some bug that would otherwise occur in a normal scenario.

It's a valid point. Perhaps it does make the most sense if we leak that complexity to the sequence developers so that they have to over fund their accounts and manage the gas limits (we can hardcode the gas price to the minimum)

testing/txsim/run_test.go

codecov-commenter · 2023-04-13T09:23:28Z

Codecov Report

Merging #1613 (6373b93) into main (952edae) will increase coverage by 1.84%.
The diff coverage is 62.75%.

@@            Coverage Diff             @@
##             main    #1613      +/-   ##
==========================================
+ Coverage   49.05%   50.89%   +1.84%     
==========================================
  Files          85       92       +7     
  Lines        4964     5739     +775     
==========================================
+ Hits         2435     2921     +486     
- Misses       2290     2518     +228     
- Partials      239      300      +61

Impacted Files	Coverage Δ
testutil/testnode/read.go	`0.00% <0.00%> (ø)`
testing/txsim/run.go	`61.81% <61.81%> (ø)`
testing/txsim/stake.go	`65.21% <65.21%> (ø)`
testing/txsim/account.go	`67.44% <67.44%> (ø)`
testing/txsim/client.go	`78.88% <78.88%> (ø)`
testing/txsim/blob.go	`80.64% <80.64%> (ø)`
testing/txsim/send.go	`89.65% <89.65%> (ø)`
app/estimate_square_size.go	`89.09% <100.00%> (ø)`
testutil/testnode/full_node.go	`82.87% <100.00%> (ø)`
testutil/testnode/node_init.go	`64.17% <100.00%> (ø)`
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

evan-forbes

LGTM! 👍 👍 can't wait to write some sequences!

evan-forbes · 2023-04-13T12:55:51Z

testing/txsim/account.go

+	// set default gas limit to cover the costs of most transactions
+	// At 0.001 utia per gas, this equates to 1000utia per transaction
+	// In the future we may want to give some access to the sequence itself
+	gasLimit  = 1000000


we might want to increase this so that we can test blobs larger than ~240 shares, so +1 that having the sequence control this later would be a good idea, or increasing it to about 8 million should be sufficient for the near term

testing/txsim/client.go

rootulp

Overall LGTM, really cool testing tool

rootulp · 2023-04-13T18:09:30Z

testing/txsim/blob.go

+type Range struct {
+	Min int
+	Max int
+}


rootulp · 2023-04-13T18:32:20Z

testing/txsim/blob.go

+			// generate a random namespace for the blob
+			namespace := make([]byte, ns.NamespaceVersionZeroIDSize)
+			_, err := rand.Read(namespace)
+			if err != nil {
+				return Operation{}, fmt.Errorf("generating random namespace: %w", err)
+			}
+			namespaces[i] = ns.MustNewV0(namespace)


[optional] can use

celestia-app/pkg/namespace/random_blob.go

Line 11 in dd19531

func RandomBlobNamespace() Namespace {

Thanks but I want to use the random source provided in the method so that by having a specific seed it should always reproduce the same sequence

testing/txsim/blob.go

testing/txsim/client.go

…al/fuzz

rootulp · 2023-04-20T13:55:35Z

testutil/testnode/node_interaction_api.go

@@ -177,7 +177,7 @@ func (c *Context) FillBlock(squareSize int, accounts []string, broadcastMode str
 	// in order to get the square size that we want, we need to fill half the
 	// square minus a few for the tx (see the square estimation logic in
 	// app/estimate_square_size.go)
-	shareCount := (squareSize * squareSize / 2) - 1
+	shareCount := (squareSize * squareSize / 2) - 2


[out of curiosity] why did the size of one transaction occupy change from 1 to 2 shares? Did this PR fix a bug?

Did this PR fix a bug?

Yes it added math.Ceil to this line because our uint64 conversion was rounding down the number meaning occasionally we estimated too small of a square:

celestia-app/app/estimate_square_size.go

Line 37 in 11374a6

minSize := uint64(math.Ceil(math.Sqrt(float64(totalSharesUsed))))

I was thinking of extracting it out into a separate PR but we're about to overwrite this method entirely with ADR020

tx simulator prototype

b64a530

cmwaters requested a review from evan-forbes as a code owner April 11, 2023 19:23

MSevey requested review from a team and mojtaba-esk and removed request for a team April 11, 2023 19:23

Merge branch 'main' into cal/fuzz

e6d43ac

MSevey requested a review from a team April 11, 2023 19:27

cmwaters self-assigned this Apr 11, 2023

cmwaters added 3 commits April 11, 2023 22:24

clean up logs

9f9ef76

Merge branch 'cal/fuzz' of github.com:celestiaorg/celestia-app into c…

fb36328

…al/fuzz

fix up the data race (I think)

7ef5695

rach-id reviewed Apr 12, 2023

View reviewed changes

testing/txsim/run_test.go Show resolved Hide resolved

testing/txsim/run_test.go Show resolved Hide resolved

testing/txsim/run_test.go Show resolved Hide resolved

result to ignoring race failure

5482401

MSevey requested a review from a team April 12, 2023 13:25

evan-forbes reviewed Apr 12, 2023

View reviewed changes

address PR comments

60e53ca

MSevey requested a review from a team April 13, 2023 09:12

add some concurrency protection to the map

5cf229f

evan-forbes previously approved these changes Apr 13, 2023

View reviewed changes

rootulp previously approved these changes Apr 13, 2023

View reviewed changes

Merge branch 'main' into cal/fuzz

e193524

MSevey requested a review from a team April 14, 2023 08:12

cmwaters added 2 commits April 17, 2023 08:03

Merge branch 'main' into cal/fuzz

3407ec2

allow sequences to dictate gas

846d4b1

cmwaters dismissed stale reviews from evan-forbes and rootulp via 846d4b1 April 18, 2023 08:24

cmwaters added 2 commits April 18, 2023 10:33

Merge branch 'main' into cal/fuzz

b04537f

lint

9bc19b3

cmwaters added 4 commits April 18, 2023 13:49

fix estimation tests

304cf78

Merge branch 'cal/fuzz' of github.com:celestiaorg/celestia-app into c…

f44c569

…al/fuzz

fix node test

34f12c9

Merge branch 'main' into cal/fuzz

6373b93

cmwaters enabled auto-merge (squash) April 19, 2023 13:12

Merge branch 'main' into cal/fuzz

f3d229f

evan-forbes approved these changes Apr 20, 2023

View reviewed changes

rootulp approved these changes Apr 20, 2023

View reviewed changes

cmwaters merged commit 11374a6 into main Apr 20, 2023

cmwaters deleted the cal/fuzz branch April 20, 2023 13:55

rootulp mentioned this pull request May 15, 2023

Test share encoding using random block data using PrepareProposal #1322

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: tx simulator #1613

feat: tx simulator #1613

cmwaters commented Apr 11, 2023 •

edited

Loading

cmwaters commented Apr 11, 2023

evan-forbes left a comment

evan-forbes Apr 12, 2023

cmwaters Apr 13, 2023

evan-forbes Apr 12, 2023

evan-forbes Apr 12, 2023

rootulp Apr 13, 2023 •

edited

Loading

evan-forbes Apr 12, 2023 •

edited

Loading

evan-forbes Apr 12, 2023

cmwaters Apr 13, 2023 •

edited

Loading

evan-forbes Apr 12, 2023

cmwaters Apr 13, 2023

evan-forbes Apr 13, 2023

cmwaters Apr 14, 2023

codecov-commenter commented Apr 13, 2023 •

edited

Loading

evan-forbes left a comment

evan-forbes Apr 13, 2023

rootulp left a comment

rootulp Apr 13, 2023

rootulp Apr 13, 2023

cmwaters Apr 17, 2023

rootulp Apr 20, 2023

cmwaters Apr 20, 2023

feat: tx simulator #1613

feat: tx simulator #1613

Conversation

cmwaters commented Apr 11, 2023 • edited Loading

How it works

cmwaters commented Apr 11, 2023

evan-forbes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rootulp Apr 13, 2023 • edited Loading

Choose a reason for hiding this comment

evan-forbes Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmwaters Apr 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 13, 2023 • edited Loading

Codecov Report

evan-forbes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rootulp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmwaters commented Apr 11, 2023 •

edited

Loading

rootulp Apr 13, 2023 •

edited

Loading

evan-forbes Apr 12, 2023 •

edited

Loading

cmwaters Apr 13, 2023 •

edited

Loading

codecov-commenter commented Apr 13, 2023 •

edited

Loading