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

Staking pool tests #744

Merged
merged 7 commits into from
Mar 30, 2018
Merged

Conversation

cwgoes
Copy link
Contributor

@cwgoes cwgoes commented Mar 29, 2018

Includes basic tests and integration tests with random state / random operations.

Ref #731

@cwgoes cwgoes requested a review from rigelrozanski March 29, 2018 12:56
@cwgoes cwgoes requested a review from ebuchman as a code owner March 29, 2018 12:56
@cwgoes
Copy link
Contributor Author

cwgoes commented Mar 29, 2018

Random integration tests are currently failing; somehow candidate.Liabilities is becoming negative with only bond, unbond, and addTokens operations.

@rigelrozanski Anything unusual here - https://github.com/cosmos/cosmos-sdk/blob/cwgoes/staking-pool-tests/x/stake/pool_test.go#L221 ? Tests pass when the addTokens operation is never performed, but adding tokens should never cause negative candidate.Liabilities.

@rigelrozanski
Copy link
Contributor

nice!

hot tip: whenever there are a whole whack ton of error messages - it's better practice to use require instead of assert so that it stops after the first error. Also we should add more detailed output in the error message it's difficult to understand what's happening by just looking at the message @cwgoes

@cwgoes
Copy link
Contributor Author

cwgoes commented Mar 29, 2018

require.Equal was just what I needed, thanks @rigelrozanski

Tests now display helpful messages about what operation caused the error

@rigelrozanski rigelrozanski changed the base branch from rigel/pool to rigel/tick-tests March 30, 2018 20:35
@rigelrozanski rigelrozanski force-pushed the cwgoes/staking-pool-tests branch from 174b50a to cb8f666 Compare March 30, 2018 20:37
@rigelrozanski rigelrozanski force-pushed the cwgoes/staking-pool-tests branch from cb8f666 to 8699394 Compare March 30, 2018 20:39
for _, candidate := range cA {

// nonnegative ex rate
require.Equal(t, false, candidate.delegatorShareExRate().LT(sdk.ZeroRat),
Copy link
Contributor

Choose a reason for hiding this comment

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

require.False


// run random operations in a random order on a random state, assert invariants hold
func TestIntegrationInvariants(t *testing.T) {
r := rand.New(rand.NewSource(int64(42)))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is generating one deterministic number each time and reusing it everywhere... what you want is two instances of rand within each loop: rand.New(rand.NewSource(time.Now().UnixNano()))

.... making this fix now

@codecov
Copy link

codecov bot commented Mar 30, 2018

Codecov Report

Merging #744 into rigel/tick-tests will increase coverage by 0.92%.
The diff coverage is 100%.

@@                 Coverage Diff                  @@
##           rigel/tick-tests     #744      +/-   ##
====================================================
+ Coverage             61.01%   61.93%   +0.92%     
====================================================
  Files                    62       62              
  Lines                  3255     3255              
====================================================
+ Hits                   1986     2016      +30     
+ Misses                 1128     1098      -30     
  Partials                141      141

@rigelrozanski
Copy link
Contributor

@cwgoes fyi - just overhauled the negative operations test... passing now, had nothing to do with rational overflows - there were a series of logical errors in the way which the random tests were constructed - I ended up pairing it down to a single candidate scenario to remove a lot of the obfuscation. We should continue to make these random tests more clear - I think there is a better way of constructing the "random operation" among other structure which will make these tests more maintainable. - if you want to get on that as an additional PR to refactor these tests further that would be useful.

Although I really like the idea behind fuzz testing/ random testing as we've here we need to put extra work into code clarity

@rigelrozanski rigelrozanski merged commit f3b41b2 into rigel/tick-tests Mar 30, 2018
@rigelrozanski rigelrozanski deleted the cwgoes/staking-pool-tests branch March 30, 2018 22:54
@rigelrozanski rigelrozanski changed the title WIP: Staking pool tests Staking pool tests Mar 30, 2018
@rigelrozanski
Copy link
Contributor

rigelrozanski commented Apr 2, 2018

Main changes which were implemented in my commits here:

  • visual cleanup/naming cleanup/clarity
  • now use variables defined in test_common.go
  • use of more specific require/assert functions
  • use of a simple single candidate scenario instead of creating a bunch of candidates and modifying one (I think the original could still valuable as an additional more complex test which can be created now that we have created a working simple scenario)
  • use of random numbers as intended (previously was not using random numbers at all just reusing a single deterministic number)
  • Removal of the idea of tokensA (should always be zero) there is just the affected tokens now (just named tokens) 77bb710...450c88c#diff-4291616d92886c45f80d20cb5c5be739R298
  • fix how the affects sign for add tokens 77bb710...450c88c#diff-4291616d92886c45f80d20cb5c5be739R269

@cwgoes
Copy link
Contributor Author

cwgoes commented Apr 2, 2018

I don't think it was reusing the same random number, e.g.

package main

import (
	"fmt"
	"math/rand"
)

func main() {
	r := rand.New(rand.NewSource(42))
	for i := 0; i < 10; i++ {
		fmt.Printf("Random number: %d\n", r.Int31n(1000))
	}
}
go run test.go
Random number: 305
Random number: 987
Random number: 668
Random number: 750
Random number: 423
Random number: 345
Random number: 357
Random number: 176
Random number: 128
Random number: 643

Each time the rand instance is utilized its internal state is updated. Setting a constant seed causes the program to repeat a constant sequence of random numbers (which will be the same sequence when the program is run again), but not the same number each call. In this case, that would mean that the test cases were pseudorandom but still deterministic, which seemed convenient for test debugging. Reseeding should still work, but I'm not convinced that it's necessary.

@rigelrozanski
Copy link
Contributor

thanks.. I stand corrected!

mmsqe pushed a commit to mmsqe/cosmos-sdk that referenced this pull request Sep 12, 2024
* Problem: tx executor can't do dependency analysis

Solution:
- change the api to allow static analysis on tx body

* fix

* changelog

* cleanup

* Update CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

---------

Signed-off-by: yihuang <[email protected]>
dudong2 pushed a commit to b-harvest/cosmos-sdk that referenced this pull request Oct 17, 2024
* Problem: tx executor can't do dependency analysis

Solution:
- change the api to allow static analysis on tx body

* fix

* changelog

* cleanup

* Update CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

---------

Signed-off-by: yihuang <[email protected]>
dudong2 pushed a commit to b-harvest/cosmos-sdk that referenced this pull request Oct 30, 2024
* Problem: tx executor can't do dependency analysis

Solution:
- change the api to allow static analysis on tx body

* fix

* changelog

* cleanup

* Update CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

---------

Signed-off-by: yihuang <[email protected]>
dudong2 added a commit to b-harvest/cosmos-sdk that referenced this pull request Nov 6, 2024
* Problem: parallel tx execution is not supported (#205)

add basic support in sdk:
- add a TxExecutor baseapp option
- add TxIndex/TxCount/MsgIndex in context

Update CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

fix misspell

fix lint

run gci

fix lint

gci seems not compatible with gofumpt

* Support object store (#206)

generic interface

generic btree

generic cachekv

generic transient store

support ObjStore

changelog

Update CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

object store key

Apply review suggestions

fix merge conflict

fix snapshot

revert dependers

* prefix store support object store (#236)

* Problem: fee collection not compatible with parallel execution (#237)

* Problem: no efficient way to collect fee

Solution:
- support an idea of virtual account in bank module, where the incoming
  coins are accumulated in a per-tx object store first, then accumulate
  and credit to the real account at end blocker.

  it's nesserary to support parallel tx execution, where we try not to
  access shared states.

more efficient sum

support SendCoinsFromModuleToAccountVirtual

fix test

fix test

* fix lint

* fix test

* fix test

* fix test

* fix test

* fix test

* fix mock keeper

* try fix lint

* try fix lint

* reuse code

* try fix linter

* Update x/bank/keeper/send.go

Signed-off-by: yihuang <[email protected]>

* algin panic call

* fix error handling

* try fix lint

* nolintlint generate falst postiive

---------

Signed-off-by: yihuang <[email protected]>

* Problem: MultiStore interface is bloated (#240)

* Problem: MultiStore interface is bloated

Solution:
- Split out specialied methods from it, keeping the MultiStore generic

* Update store/CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

---------

Signed-off-by: yihuang <[email protected]>

* Problem: no API to use the new CoW branch store (#243)

* Support RunAtomic API

* add unit test

* Problem: block gas used not set in context (#252)

Solution:
- fix the way context is updated

* Problem: invalid tx returns negative gas wanted (#253)

Solution:
- add checks

* Problem: signature verification result not cache between incarnations of same tx (cosmos#565)

* Problem: signature verification result not cache between incarnations of
same tx

Closes: cosmos#564

Solution:
- introduce incarnation cache that's shared between incarnations of the
  same tx

* Update CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

* Update types/context.go

Signed-off-by: yihuang <[email protected]>

* Update types/context.go

Signed-off-by: yihuang <[email protected]>

* fix nil convert

---------

Signed-off-by: yihuang <[email protected]>

* Problem: gas consumed differs after enabled cache (cosmos#570)

* Problem: gas consumed differs after enabled cache

* fix test

* Revert "Problem: gas consumed differs after enabled cache"

This reverts commit f33944e.

* Revert "Problem: signature verification result not cache between incarnations of same tx (cosmos#565)"

This reverts commit 5a1594f.

* keep interface

* Problem: tx executor can't do dependency analysis (cosmos#744)

* Problem: tx executor can't do dependency analysis

Solution:
- change the api to allow static analysis on tx body

* fix

* changelog

* cleanup

* Update CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

---------

Signed-off-by: yihuang <[email protected]>

* Problem: can't extend abci listeners in streaming manager (#269)

Solution:
- add API StreamingManager to allow extending the abci listeners, mainly
  for versiondb support

Update CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

cleanup

* chore: apply custom-ed store

* feat: Append send restrictionto Delegate/Undelegate Coins (#202)

* chore: Update go.mod for custom store

* chore: fix lint

* chore: fix lint

* chore: Update go.mod for custom cosmos-sdk/store

* Revert "chore: fix lint"

This reverts commit 8e7af64.

---------

Signed-off-by: yihuang <[email protected]>
Co-authored-by: yihuang <[email protected]>
Co-authored-by: mmsqe <[email protected]>
Co-authored-by: Kyuhyeon Choi <[email protected]>
mmsqe pushed a commit to mmsqe/cosmos-sdk that referenced this pull request Dec 12, 2024
* Problem: tx executor can't do dependency analysis

Solution:
- change the api to allow static analysis on tx body

* fix

* changelog

* cleanup

* Update CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

---------

Signed-off-by: yihuang <[email protected]>
mmsqe pushed a commit to mmsqe/cosmos-sdk that referenced this pull request Dec 12, 2024
* Problem: tx executor can't do dependency analysis

Solution:
- change the api to allow static analysis on tx body

* fix

* changelog

* cleanup

* Update CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

---------

Signed-off-by: yihuang <[email protected]>
mmsqe pushed a commit to mmsqe/cosmos-sdk that referenced this pull request Dec 16, 2024
* Problem: tx executor can't do dependency analysis

Solution:
- change the api to allow static analysis on tx body

* fix

* changelog

* cleanup

* Update CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

---------

Signed-off-by: yihuang <[email protected]>
mmsqe pushed a commit to mmsqe/cosmos-sdk that referenced this pull request Dec 16, 2024
* Problem: tx executor can't do dependency analysis

Solution:
- change the api to allow static analysis on tx body

* fix

* changelog

* cleanup

* Update CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

---------

Signed-off-by: yihuang <[email protected]>
dudong2 added a commit to b-harvest/cosmos-sdk that referenced this pull request Dec 17, 2024
* Problem: parallel tx execution is not supported (#205)

add basic support in sdk:
- add a TxExecutor baseapp option
- add TxIndex/TxCount/MsgIndex in context

Update CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

fix misspell

fix lint

run gci

fix lint

gci seems not compatible with gofumpt

* Support object store (#206)

generic interface

generic btree

generic cachekv

generic transient store

support ObjStore

changelog

Update CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

object store key

Apply review suggestions

fix merge conflict

fix snapshot

revert dependers

* prefix store support object store (#236)

* Problem: fee collection not compatible with parallel execution (#237)

* Problem: no efficient way to collect fee

Solution:
- support an idea of virtual account in bank module, where the incoming
  coins are accumulated in a per-tx object store first, then accumulate
  and credit to the real account at end blocker.

  it's nesserary to support parallel tx execution, where we try not to
  access shared states.

more efficient sum

support SendCoinsFromModuleToAccountVirtual

fix test

fix test

* fix lint

* fix test

* fix test

* fix test

* fix test

* fix test

* fix mock keeper

* try fix lint

* try fix lint

* reuse code

* try fix linter

* Update x/bank/keeper/send.go

Signed-off-by: yihuang <[email protected]>

* algin panic call

* fix error handling

* try fix lint

* nolintlint generate falst postiive

---------

Signed-off-by: yihuang <[email protected]>

* Problem: MultiStore interface is bloated (#240)

* Problem: MultiStore interface is bloated

Solution:
- Split out specialied methods from it, keeping the MultiStore generic

* Update store/CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

---------

Signed-off-by: yihuang <[email protected]>

* Problem: no API to use the new CoW branch store (#243)

* Support RunAtomic API

* add unit test

* Problem: block gas used not set in context (#252)

Solution:
- fix the way context is updated

* Problem: invalid tx returns negative gas wanted (#253)

Solution:
- add checks

* Problem: signature verification result not cache between incarnations of same tx (cosmos#565)

* Problem: signature verification result not cache between incarnations of
same tx

Closes: cosmos#564

Solution:
- introduce incarnation cache that's shared between incarnations of the
  same tx

* Update CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

* Update types/context.go

Signed-off-by: yihuang <[email protected]>

* Update types/context.go

Signed-off-by: yihuang <[email protected]>

* fix nil convert

---------

Signed-off-by: yihuang <[email protected]>

* Problem: gas consumed differs after enabled cache (cosmos#570)

* Problem: gas consumed differs after enabled cache

* fix test

* Revert "Problem: gas consumed differs after enabled cache"

This reverts commit f33944e.

* Revert "Problem: signature verification result not cache between incarnations of same tx (cosmos#565)"

This reverts commit 5a1594f.

* keep interface

* Problem: tx executor can't do dependency analysis (cosmos#744)

* Problem: tx executor can't do dependency analysis

Solution:
- change the api to allow static analysis on tx body

* fix

* changelog

* cleanup

* Update CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

---------

Signed-off-by: yihuang <[email protected]>

* Problem: can't extend abci listeners in streaming manager (#269)

Solution:
- add API StreamingManager to allow extending the abci listeners, mainly
  for versiondb support

Update CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

cleanup

* chore: apply custom-ed store

* feat: Append send restrictionto Delegate/Undelegate Coins (#202)

* chore: Update go.mod for custom store

* chore: fix lint

* chore: fix lint

* chore: Update go.mod for custom cosmos-sdk/store

* Revert "chore: fix lint"

This reverts commit 8e7af64.

---------

Signed-off-by: yihuang <[email protected]>
Co-authored-by: yihuang <[email protected]>
Co-authored-by: mmsqe <[email protected]>
Co-authored-by: Kyuhyeon Choi <[email protected]>
mmsqe pushed a commit to mmsqe/cosmos-sdk that referenced this pull request Dec 18, 2024
* Problem: tx executor can't do dependency analysis

Solution:
- change the api to allow static analysis on tx body

* fix

* changelog

* cleanup

* Update CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

---------

Signed-off-by: yihuang <[email protected]>
dudong2 added a commit to b-harvest/cosmos-sdk that referenced this pull request Dec 19, 2024
* feat: Append send restrictionto Delegate/Undelegate Coins (#202)

* feat: apply cronos (#208)

* Problem: parallel tx execution is not supported (#205)

add basic support in sdk:
- add a TxExecutor baseapp option
- add TxIndex/TxCount/MsgIndex in context

Update CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

fix misspell

fix lint

run gci

fix lint

gci seems not compatible with gofumpt

* Support object store (#206)

generic interface

generic btree

generic cachekv

generic transient store

support ObjStore

changelog

Update CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

object store key

Apply review suggestions

fix merge conflict

fix snapshot

revert dependers

* prefix store support object store (#236)

* Problem: fee collection not compatible with parallel execution (#237)

* Problem: no efficient way to collect fee

Solution:
- support an idea of virtual account in bank module, where the incoming
  coins are accumulated in a per-tx object store first, then accumulate
  and credit to the real account at end blocker.

  it's nesserary to support parallel tx execution, where we try not to
  access shared states.

more efficient sum

support SendCoinsFromModuleToAccountVirtual

fix test

fix test

* fix lint

* fix test

* fix test

* fix test

* fix test

* fix test

* fix mock keeper

* try fix lint

* try fix lint

* reuse code

* try fix linter

* Update x/bank/keeper/send.go

Signed-off-by: yihuang <[email protected]>

* algin panic call

* fix error handling

* try fix lint

* nolintlint generate falst postiive

---------

Signed-off-by: yihuang <[email protected]>

* Problem: MultiStore interface is bloated (#240)

* Problem: MultiStore interface is bloated

Solution:
- Split out specialied methods from it, keeping the MultiStore generic

* Update store/CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

---------

Signed-off-by: yihuang <[email protected]>

* Problem: no API to use the new CoW branch store (#243)

* Support RunAtomic API

* add unit test

* Problem: block gas used not set in context (#252)

Solution:
- fix the way context is updated

* Problem: invalid tx returns negative gas wanted (#253)

Solution:
- add checks

* Problem: signature verification result not cache between incarnations of same tx (cosmos#565)

* Problem: signature verification result not cache between incarnations of
same tx

Closes: cosmos#564

Solution:
- introduce incarnation cache that's shared between incarnations of the
  same tx

* Update CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

* Update types/context.go

Signed-off-by: yihuang <[email protected]>

* Update types/context.go

Signed-off-by: yihuang <[email protected]>

* fix nil convert

---------

Signed-off-by: yihuang <[email protected]>

* Problem: gas consumed differs after enabled cache (cosmos#570)

* Problem: gas consumed differs after enabled cache

* fix test

* Revert "Problem: gas consumed differs after enabled cache"

This reverts commit f33944e.

* Revert "Problem: signature verification result not cache between incarnations of same tx (cosmos#565)"

This reverts commit 5a1594f.

* keep interface

* Problem: tx executor can't do dependency analysis (cosmos#744)

* Problem: tx executor can't do dependency analysis

Solution:
- change the api to allow static analysis on tx body

* fix

* changelog

* cleanup

* Update CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

---------

Signed-off-by: yihuang <[email protected]>

* Problem: can't extend abci listeners in streaming manager (#269)

Solution:
- add API StreamingManager to allow extending the abci listeners, mainly
  for versiondb support

Update CHANGELOG.md

Signed-off-by: yihuang <[email protected]>

cleanup

* chore: apply custom-ed store

* feat: Append send restrictionto Delegate/Undelegate Coins (#202)

* chore: Update go.mod for custom store

* chore: fix lint

* chore: fix lint

* chore: Update go.mod for custom cosmos-sdk/store

* Revert "chore: fix lint"

This reverts commit 8e7af64.

---------

Signed-off-by: yihuang <[email protected]>
Co-authored-by: yihuang <[email protected]>
Co-authored-by: mmsqe <[email protected]>
Co-authored-by: Kyuhyeon Choi <[email protected]>

* chore: Use basechain/develop-v0.50.11-store/v1.1.1

---------

Signed-off-by: yihuang <[email protected]>
Co-authored-by: yihuang <[email protected]>
Co-authored-by: mmsqe <[email protected]>
Co-authored-by: Kyuhyeon Choi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants