Add CalculateGas integration-tests & improve deterministicgas docs #706

ysv · 2023-11-07T12:09:12Z

Description

Reviewers checklist:

Try to write more meaningful comments with clear actions to be taken.
Nit-picking should be unblocking. Focus on core issues.

Authors checklist

Provide a concise and meaningful description
Review the code yourself first, before making the PR.
Annotate your PR in places that require explanation.
Think and try to split the PR to smaller PR if it is big.

This change is

ysv

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion

integration-tests/pkg/client_test.go line 65 at r1 (raw file):

				},
			},
			expectedGas: 65_000 + 1*50_000 + 0*10 + (3-1)*1000, // extra 2 signatures

here is the error I'm getting when running this test:

=== RUN   TestCalculateGas
=== PAUSE TestCalculateGas
=== CONT  TestCalculateGas
    client_test.go:34: Funding accounts for tests, it might take a while...
    client_test.go:34: Test accounts funded
    client_test.go:35: Funding accounts for tests, it might take a while...
    client_test.go:35: Test accounts funded
--- FAIL: TestCalculateGas (1.82s)
=== RUN   TestCalculateGas/single_address_send
estimatedGas: 115000
chainEstimatedGas: 115000
    --- PASS: TestCalculateGas/single_address_send (0.01s)
=== RUN   TestCalculateGas/multisig_bank_send
estimatedGas: 115590
chainEstimatedGas: 115000
    client_test.go:81: 
        	Error:      	Not equal: 
        	            	expected: 117000
        	            	actual  : 115590
        	Test:       	TestCalculateGas/multisig_bank_send
    --- FAIL: TestCalculateGas/multisig_bank_send (0.01s)


Expected :117000
Actual   :115590

Either gas estimation or our gas module doesn't work properly lets discuss

ysv

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions

integration-tests/modules/auth_test.go line 188 at r2 (raw file):

	//requireT.Equal(txRes.GasUsed, txRes.GasWanted) // another option to reproduce is to use chain.TxFactory().WithSimulateAndExecute(true) & this assertion.

	requireT.Equal(txRes.GasUsed, int64(estimatedGas)) // this shouldn't fail.

getting this error here:

Error:      	Not equal: 
        	            	expected: 115000
        	            	actual  : 126360
        	Test:       	TestAuthMultisig
--- FAIL: TestAuthMultisig (1.69s)

ysv

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions

integration-tests/modules/auth_test.go line 188 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

getting this error here:

Error:      	Not equal: 
        	            	expected: 115000
        	            	actual  : 126360
        	Test:       	TestAuthMultisig
--- FAIL: TestAuthMultisig (1.69s)

65_000 + 50_000 + x*10

diff: 11_360 / 10

ysv

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion

x/deterministicgas/spec/README.tmpl.md line 18 at r2 (raw file):

`
Gas = FixedGas + Sum(Gas for each message) + GasForExtraBytes + GasForExtraSignatures

While implementing this PR I realised that our deterministic gas formula doesn't work like this.
We provide limited amount of gas func (cfg Config) TxBaseGas which is equal to 2048*10 + 1*1000 = 21480 but this gas could possibly cover both sig verification & free bytes. And anything over this gas will be charged.

So more correct formula would be:

Gas = FixedGas + Sum(Gas for each message) + max((GasForBytes + GasForSignatures - TxBaseGas),0)

But I'm not sure if we should change it.

…n-tests-for-pkg

wojtek-coreum

Reviewed 2 of 4 files at r1, 4 of 4 files at r3, all commit messages.
Reviewable status: 5 of 6 files reviewed, 3 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @ysv)

integration-tests/modules/auth_test.go line 181 at r3 (raw file):

	t.Logf("Fully signed tx executed, txHash:%s, gasUsed:%d, gasWanted:%d", txRes.TxHash, txRes.GasUsed, txRes.GasWanted)

	// Real gas used might be less that estimation for multisig account because (especially when there are many signers)

Do you mean that we put all the signatures there, not the minimum number?

integration-tests/modules/auth_test.go line 294 at r3 (raw file):

			// estimation uses worst case to estimate number of bytes in tx which causes possible overflow of free bytes.
			// 10 is price for each extra byte over FreeBytes.
			expectedGas: dgc.FixedGas + 1*deterministicgas.BankSendPerCoinGas + 1133*10,

10 -> constant TxSizeCostPerByte

x/deterministicgas/spec/README.tmpl.md line 18 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

While implementing this PR I realised that our deterministic gas formula doesn't work like this.
We provide limited amount of gas func (cfg Config) TxBaseGas which is equal to 2048*10 + 1*1000 = 21480 but this gas could possibly cover both sig verification & free bytes. And anything over this gas will be charged.

So more correct formula would be:
Gas = FixedGas + Sum(Gas for each message) + max((GasForBytes + GasForSignatures - TxBaseGas),0)
But I'm not sure if we should change it.

In ante handler, we charge for signatures and size using TxBaseGas amount. Whatever gas is left is ignored, but if we had to charge more, additional gas is taken from the gas limit declared by user.

wojtek-coreum

Reviewed 1 of 3 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @ysv)

ysv

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @wojtek-coreum)

integration-tests/modules/auth_test.go line 181 at r3 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

Do you mean that we put all the signatures there, not the minimum number?

we put min num of signatures but inside cosmos-sdk handler they estimate the worst case scenario. I haven't understood all the details but it seems like they consider all signatures to be present and each address to be multisign or smth similar

you can check inside x/auth/ante/basic.go:99

Also Lets discuss

integration-tests/modules/auth_test.go line 294 at r3 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

10 -> constant TxSizeCostPerByte

This value is defined inside auth params
Added query

x/deterministicgas/spec/README.tmpl.md line 18 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

In ante handler, we charge for signatures and size using TxBaseGas amount. Whatever gas is left is ignored, but if we had to charge more, additional gas is taken from the gas limit declared by user.

Lets discuss after daily

ysv

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @wojtek-coreum)

a discussion (no related file):
2 more interesting findings

Let me explain on the call:

Consumig gas descriptor: "DeterministicGas (gas required: 50000, message type: \*types.MsgSend)", amount: 50000  
Consumig gas descriptor: "txSize", amount: 11830  
Consumig gas descriptor: "ante verify: secp256k1", amount: 1000  
Consumig gas descriptor: "ante verify: secp256k1", amount: 1000  
Consumig gas descriptor: "ante verify: secp256k1", amount: 1000  
Consumig gas descriptor: "ante verify: secp256k1", amount: 1000  
Consumig gas descriptor: "ante verify: secp256k1", amount: 1000  
Consumig gas descriptor: "ante verify: secp256k1", amount: 1000  
Consumig gas descriptor: "Fixed", amount: 65000  
ChargeFixedGasDecorator: simulate: false  
. gasMeter: BasicGasMeter:  
limit: 115000  
consumed: 65000

Consumig gas descriptor: "txSize", amount: 7570  
Consumig gas descriptor: "txSize", amount: 25270  
Consumig gas descriptor: "OverBonus", amount: 11360  
Consumig gas descriptor: "Fixed", amount: 65000  
ChargeFixedGasDecorator: simulate: true  
. gasMeter: InfiniteGasMeter:  
consumed: 76360

…n-tests-for-pkg

wojtek-coreum

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil and @miladz68)

ysv

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @wojtek-coreum)

a discussion (no related file):

Previously, ysv (Yaroslav Savchuk) wrote…

2 more interesting findings

Let me explain on the call:

Consumig gas descriptor: "DeterministicGas (gas required: 50000, message type: \*types.MsgSend)", amount: 50000  
Consumig gas descriptor: "txSize", amount: 11830  
Consumig gas descriptor: "ante verify: secp256k1", amount: 1000  
Consumig gas descriptor: "ante verify: secp256k1", amount: 1000  
Consumig gas descriptor: "ante verify: secp256k1", amount: 1000  
Consumig gas descriptor: "ante verify: secp256k1", amount: 1000  
Consumig gas descriptor: "ante verify: secp256k1", amount: 1000  
Consumig gas descriptor: "ante verify: secp256k1", amount: 1000  
Consumig gas descriptor: "Fixed", amount: 65000  
ChargeFixedGasDecorator: simulate: false  
. gasMeter: BasicGasMeter:  
limit: 115000  
consumed: 65000

Consumig gas descriptor: "txSize", amount: 7570  
Consumig gas descriptor: "txSize", amount: 25270  
Consumig gas descriptor: "OverBonus", amount: 11360  
Consumig gas descriptor: "Fixed", amount: 65000  
ChargeFixedGasDecorator: simulate: true  
. gasMeter: InfiniteGasMeter:  
consumed: 76360

recalculate numbers myself one more time

x/deterministicgas/spec/README.tmpl.md line 18 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

Lets discuss after daily

decided to update formula here

wojtek-coreum

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil and @miladz68)

ysv

Reviewable status: 4 of 9 files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @wojtek-coreum)

a discussion (no related file):

Previously, ysv (Yaroslav Savchuk) wrote…

recalculate numbers myself one more time

calculation for simulate: true:

32,840 - for txSize
6000 - for signatures
total: 38,840

TxBaseGas: 21480
OverBonus: 11360
total: 32840 (signatures are not included)

This means that during estimation they don't include signature validation (they are also not present in logs) and it only works because they overestimate signatures byte-size which later is enough to cover signatures also.

created cosmos-sdk issue: cosmos/cosmos-sdk#18441

ysv

Reviewable status: 4 of 9 files reviewed, all discussions resolved (waiting on @dzmitryhil, @miladz68, and @wojtek-coreum)

x/deterministicgas/spec/README.tmpl.md line 18 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

decided to update formula here

done

wojtek-coreum

Reviewed 6 of 6 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @miladz68, and @ysv)

integration-tests/modules/auth_test.go line 405 at r5 (raw file):

	for _, tt := range testsNonDeterm {
		tt := tt
		t.Run(tt.name, func(t *testing.T) {

add t.Parallel()?

…n-tests-for-pkg

ysv

Reviewable status: 5 of 9 files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @miladz68, and @wojtek-coreum)

integration-tests/modules/auth_test.go line 405 at r5 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

add t.Parallel()?

Done.

dzmitryhil

Reviewable status: 5 of 9 files reviewed, 1 unresolved discussion (waiting on @miladz68 and @wojtek-coreum)

wojtek-coreum

Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @miladz68)

WIP: CalculateGas integration tests

ed84d52

ysv commented Nov 7, 2023

View reviewed changes

ysv added 2 commits November 7, 2023 16:56

wip

2cc0767

Add tests

aa05082

ysv commented Nov 8, 2023

View reviewed changes

ysv added 2 commits November 8, 2023 18:41

Finalize tests

bdaf64d

reset debug code

5bd6bcd

ysv commented Nov 8, 2023

View reviewed changes

ysv changed the title ~~WIP: CalculateGas integration tests~~ Add CalculateGas integration-tests Nov 8, 2023

ysv marked this pull request as ready for review November 8, 2023 17:45

ysv requested a review from a team as a code owner November 8, 2023 17:45

ysv requested review from dzmitryhil, miladz68 and wojtek-coreum and removed request for a team November 8, 2023 17:45

ysv added 2 commits November 8, 2023 18:45

regen

8f6a890

Merge remote-tracking branch 'origin/master' into yaroslav/integratio…

7ae72ad

…n-tests-for-pkg

wojtek-coreum requested changes Nov 9, 2023

View reviewed changes

wojtek-coreum reviewed Nov 9, 2023

View reviewed changes

ysv requested a review from wojtek-coreum November 9, 2023 10:20

ysv commented Nov 9, 2023

View reviewed changes

Improve test

a43de1c

ysv commented Nov 9, 2023

View reviewed changes

ysv added 2 commits November 9, 2023 11:54

fix

88ad4d5

Merge remote-tracking branch 'origin/master' into yaroslav/integratio…

4f977b1

…n-tests-for-pkg

wojtek-coreum approved these changes Nov 9, 2023

View reviewed changes

ysv requested a review from wojtek-coreum November 9, 2023 14:54

ysv commented Nov 9, 2023

View reviewed changes

wojtek-coreum previously approved these changes Nov 10, 2023

View reviewed changes

finalize

4432869

ysv dismissed wojtek-coreum’s stale review via 4432869 November 10, 2023 12:32

ysv added 2 commits November 10, 2023 13:33

Add ante with DGB & replace cosmos-sdk

e78d0c4

Reset

59c0820

ysv requested a review from wojtek-coreum November 10, 2023 12:34

ysv commented Nov 10, 2023

View reviewed changes

reset go.mod

15e5568

ysv commented Nov 10, 2023

View reviewed changes

Finalize

6c30386

wojtek-coreum requested changes Nov 10, 2023

View reviewed changes

ysv added 2 commits November 10, 2023 16:43

Merge remote-tracking branch 'origin/master' into yaroslav/integratio…

d6fbcd0

…n-tests-for-pkg

Fix

c025032

ysv requested a review from wojtek-coreum November 10, 2023 15:44

ysv commented Nov 10, 2023

View reviewed changes

ysv changed the title ~~Add CalculateGas integration-tests~~ Add CalculateGas integration-tests & improve deterministicgas docs Nov 10, 2023

dzmitryhil approved these changes Nov 13, 2023

View reviewed changes

wojtek-coreum approved these changes Nov 13, 2023

View reviewed changes

ysv merged commit 032dd96 into master Nov 13, 2023
7 checks passed

ysv deleted the yaroslav/integration-tests-for-pkg branch November 13, 2023 11:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CalculateGas integration-tests & improve deterministicgas docs #706

Add CalculateGas integration-tests & improve deterministicgas docs #706

ysv commented Nov 7, 2023 •

edited

Loading

ysv left a comment

ysv left a comment

ysv left a comment

ysv left a comment

wojtek-coreum left a comment

wojtek-coreum left a comment

ysv left a comment

ysv left a comment

wojtek-coreum left a comment

ysv left a comment

wojtek-coreum left a comment

ysv left a comment

ysv left a comment

wojtek-coreum left a comment

ysv left a comment

dzmitryhil left a comment

wojtek-coreum left a comment

Add CalculateGas integration-tests & improve deterministicgas docs #706

Add CalculateGas integration-tests & improve deterministicgas docs #706

Conversation

ysv commented Nov 7, 2023 • edited Loading

Description

Reviewers checklist:

Authors checklist

ysv left a comment

Choose a reason for hiding this comment

ysv left a comment

Choose a reason for hiding this comment

ysv left a comment

Choose a reason for hiding this comment

ysv left a comment

Choose a reason for hiding this comment

wojtek-coreum left a comment

Choose a reason for hiding this comment

wojtek-coreum left a comment

Choose a reason for hiding this comment

ysv left a comment

Choose a reason for hiding this comment

ysv left a comment

Choose a reason for hiding this comment

wojtek-coreum left a comment

Choose a reason for hiding this comment

ysv left a comment

Choose a reason for hiding this comment

wojtek-coreum left a comment

Choose a reason for hiding this comment

ysv left a comment

Choose a reason for hiding this comment

ysv left a comment

Choose a reason for hiding this comment

wojtek-coreum left a comment

Choose a reason for hiding this comment

ysv left a comment

Choose a reason for hiding this comment

dzmitryhil left a comment

Choose a reason for hiding this comment

wojtek-coreum left a comment

Choose a reason for hiding this comment

ysv commented Nov 7, 2023 •

edited

Loading