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

Query issue when upgrading from v0.37.3 to v0.37.8 #5964

Closed
4 tasks
haasted opened this issue Apr 8, 2020 · 21 comments · Fixed by #5982
Closed
4 tasks

Query issue when upgrading from v0.37.3 to v0.37.8 #5964

haasted opened this issue Apr 8, 2020 · 21 comments · Fixed by #5982
Assignees

Comments

@haasted
Copy link
Contributor

haasted commented Apr 8, 2020

Summary of Bug

After upgrading, I can't query by tx hash unless I specify the --trust-node flag.

Not specifying the flag results in the following stack trace:

$ ./build/emcli q tx 2B08FC02FA89169C28056867FEB58423F0EA2DB9AC4F43F5BC5098C271E9BF69
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x48e2195]

goroutine 1 [running]:
github.com/tendermint/tendermint/lite/proxy.GetCertifiedCommit(0x9bf8, 0x4f96140, 0xc000b6aae0, 0x0, 0x0, 0xc000f0c160, 0x0, 0x0, 0xc00039c230)
  [...]golang/pkg/mod/github.com/tendermint/tendermint@v0.32.9/lite/proxy/query.go:140 +0x105
github.com/cosmos/cosmos-sdk/client/context.CLIContext.Verify(0xc000bab730, 0x4f96140, 0xc000b6aae0, 0x0, 0x0, 0x4f60aa0, 0xc000010010, 0x4c23773, 0x4, 0x0, ...)
  [...]golang/pkg/mod/github.com/cosmos/cosmos-sdk@v0.37.8/client/context/query.go:116 +0x66
github.com/cosmos/cosmos-sdk/x/auth/client/utils.ValidateTxResult(0xc000bab730, 0x4f96140, 0xc000b6aae0, 0x0, 0x0, 0x4f60aa0, 0xc000010010, 0x4c23773, 0x4, 0x0, ...)
  [...]golang/pkg/mod/github.com/cosmos/cosmos-sdk@v0.37.8/x/auth/client/utils/query.go:128 +0x8e
github.com/cosmos/cosmos-sdk/x/auth/client/utils.QueryTx(0xc000bab730, 0x4f96140, 0xc000b6aae0, 0x0, 0x0, 0x4f60aa0, 0xc000010010, 0x4c23773, 0x4, 0x0, ...)
  [...]golang/pkg/mod/github.com/cosmos/cosmos-sdk@v0.37.8/x/auth/client/utils/query.go:93 +0x371
github.com/cosmos/cosmos-sdk/x/auth/client/cli.QueryTxCmd.func1(0xc0000d5180, 0xc000eaf8e0, 0x1, 0x1, 0x0, 0x0)
  [...]golang/pkg/mod/github.com/cosmos/cosmos-sdk@v0.37.8/x/auth/client/cli/query.go:171 +0x105
github.com/spf13/cobra.(*Command).execute(0xc0000d5180, 0xc000eaf8b0, 0x1, 0x1, 0xc0000d5180, 0xc000eaf8b0)
  [...]golang/pkg/mod/github.com/spf13/cobra@v0.0.5/command.go:826 +0x460
github.com/spf13/cobra.(*Command).ExecuteC(0xc0000d4780, 0x4484560, 0xc0000d4780, 0x4c22784)
  [...]golang/pkg/mod/github.com/spf13/cobra@v0.0.5/command.go:914 +0x2fb
github.com/spf13/cobra.(*Command).Execute(...)
  [...]golang/pkg/mod/github.com/spf13/cobra@v0.0.5/command.go:864
github.com/tendermint/tendermint/libs/cli.Executor.Execute(0xc0000d4780, 0x4e1ae00, 0x2, 0xc000e62ae0)
  [...]golang/pkg/mod/github.com/tendermint/tendermint@v0.32.9/libs/cli/setup.go:89 +0x3c
main.main()
  [...]em-ledger/cmd/emcli/main.go:76 +0x33d

Version

Cosmos-SDK v0.37.8

Possible fix

Adding the line
viper.SetDefault(flags.FlagTrustNode, "false")

in the main function of the cli seems to fix the issue. Could it be an issue with the updated Viper library?

Steps to Reproduce

Checkout our the following em-ledger commit: e-money/em-ledger@d2db681

make build

./build/emcli q tx 2B08FC02FA89169C28056867FEB58423F0EA2DB9AC4F43F5BC5098C271E9BF69 --node tcp://emoney.validator.network:80


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@haasted
Copy link
Contributor Author

haasted commented Apr 9, 2020

I just made an attempt with the newest version of Gaia, and it seems the problem is also present there.

To reproduce:

  1. Clone https://github.com/cosmos/gaia/
  2. Check out tag v2.0.7
  3. Make build

The issue can now be demonstrated with the following queries:

./build/gaiacli q tx 136625830D53DE07AE3FDBB35849923908E0C59131EEBECEE6488926B86F63E5 --node tcp://cosmoshub.validator.network:80 --trust-node

Provides expected output with tx details

./build/gaiacli q tx 136625830D53DE07AE3FDBB35849923908E0C59131EEBECEE6488926B86F63E5 --node tcp://cosmoshub.validator.network:80

Crashes with a stacktrace that is very similar to the one in the issue.

@alexanderbez
Copy link
Contributor

@jgimeno do you think you have the bandwidth to tackle this?

@jgimeno
Copy link
Contributor

jgimeno commented Apr 9, 2020

Gonna check!

@jgimeno jgimeno self-assigned this Apr 9, 2020
@jgimeno
Copy link
Contributor

jgimeno commented Apr 9, 2020

@alexanderbez the problem is that 0.37.x does not include this bugfix.

#5618

An option can be to upgrade to 0.38 if possible.

Another one is to release a new 0.37 with this included.

@alexanderbez
Copy link
Contributor

I don't think that is a true fix to the issue, but we should certainly cherry-pick that into the next 0.37.x release. This was working before -- which leads me to believe this has something to do with Viper.

@jgimeno
Copy link
Contributor

jgimeno commented Apr 9, 2020

More information, from Gaia v2.0.4 stopped working.

It included these changes:

(sdk) Bump SDK version to v0.37.5.
(tendermint) Bump Tendermint version to v0.32.8.

That includes internally too a change from viper 1.4.0 to 1.6.1

@jgimeno
Copy link
Contributor

jgimeno commented Apr 9, 2020

Ok I know where is the issue, viper 1.6.0 has this feature

IsSet no longer returns true when an unset key has a flags bound

trustNodeDefined := viper.IsSet(flags.FlagTrustNode)

@alessio
Copy link
Contributor

alessio commented Apr 9, 2020

IsSet no longer returns true when an unset key has a flags bound

Gosh, this breaks quite few things. @jgimeno and I are on the case.

@alessio alessio self-assigned this Apr 9, 2020
@alessio alessio added T:Bug and removed help wanted labels Apr 9, 2020
@alessio
Copy link
Contributor

alessio commented Apr 10, 2020

We'll have to change the pruning flags, their parsing heavily relies on IsSet(). We are on it

@haasted
Copy link
Contributor Author

haasted commented Apr 10, 2020

Would it be an option to downgrade the Viper dependency? Is there something essential in the 1.6 release?

@alessio
Copy link
Contributor

alessio commented Apr 10, 2020

In line of principle, I'm not against downgrading to viper 1.4 - my immediate, natural reaction was to do so. Afterwards, we decided to invest some time in trying to fix the use of viper.IsSet() (in many places it's just plain wrong) whilst keeping cobra/viper in sync with upstream's latest release. If that fails, we fall back to downgrade viper.

@jgimeno
Copy link
Contributor

jgimeno commented Apr 10, 2020

Yeah, the problem is that it means that cobra will need to be downgraded too.

@alexanderbez
Copy link
Contributor

Yeah, the problem is that it means that cobra will need to be downgraded too.

Why? Can you not just simply use the replace directive?

@alessio
Copy link
Contributor

alessio commented Apr 10, 2020

Yes, sure we could. We are trying to fix the problem first though.

@franono
Copy link

franono commented Apr 13, 2020

Experiencing this too when running gaiacli queries on Gaia v2.0.8.

gaiacli q tx 7644CA329A30E58F320B0A1CB8DDB923EB8E2196505A918F06DFC999234C5814

panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0xc8d075]

@alessio
Copy link
Contributor

alessio commented Apr 13, 2020

@franono this should solve it b754065

EDIT: actually I can't reproduce that @franono:

alessio@bangalter:~/work/gaia$ build/gaiacli version --long
name: gaia
server_name: gaiad
client_name: gaiacli
version: 2.0.8
commit: e6f3efff002f74741b05233fe1f05aa893611d53
build_tags: netgo,ledger
go: go version go1.14.2 linux/amd64

alessio@bangalter:~/work/gaia$ build/gaiacli q tx 7644CA329A30E58F320B0A1CB8DDB923EB8E2196505A918F06DFC999234C5814 --trust-node --node tcp://cosmoshub.validator.network:80
height: 1502783
txhash: 7644CA329A30E58F320B0A1CB8DDB923EB8E2196505A918F06DFC999234C5814
code: 0
data: ""
rawlog: '[{"msg_index":0,"success":true,"log":"","events":[{"type":"delegate","attributes":[{"key":"validator","value":"cosmosvaloper1wwspfe7whh3zu4ql5rvpg044lyk6cuu7fpnd9e"},{"key":"amount","value":"407935"}]},{"type":"message","attributes":[{"key":"module","value":"staking"},{"key":"sender","value":"cosmos14fr86wh3dfvpnh88tny4ek257vtp3lr0kkgnx3"},{"key":"action","value":"delegate"}]}]}]'
logs:
- msgindex: 0
  success: true
  log: ""
  events:
  - type: delegate
    attributes:
    - key: validator
      value: cosmosvaloper1wwspfe7whh3zu4ql5rvpg044lyk6cuu7fpnd9e
    - key: amount
      value: "407935"
  - type: message
    attributes:
    - key: module
      value: staking
    - key: sender
      value: cosmos14fr86wh3dfvpnh88tny4ek257vtp3lr0kkgnx3
    - key: action
      value: delegate
info: ""
gaswanted: 200000
gasused: 107930
codespace: ""
tx:
  msg:
  - delegator_address: cosmos14fr86wh3dfvpnh88tny4ek257vtp3lr0kkgnx3
    validator_address: cosmosvaloper1wwspfe7whh3zu4ql5rvpg044lyk6cuu7fpnd9e
    amount:
      denom: uatom
      amount: "407935"
  fee:
    amount:
    - denom: uatom
      amount: "1000"
    gas: 200000
  signatures:
  - |
    pubkey: cosmospub1addwnpepqdexmutzj46e29k5q4xjymtautxpy2uhecrmrj7g29z5fndwrm5q24p2sqv
    signature: !!binary |
      QFe0fKkrt1mwAYnsD3l+yacANlZp8M4miHOW1jEKyDox9RJnyRuuw75a71kH+H8/QycLwp
      8EGLmvnYaDqdkQ6w==
  memo: Stake via Trust Wallet
timestamp: "2020-04-13T09:16:15Z"
events:
- type: delegate
  attributes:
  - key: validator
    value: cosmosvaloper1wwspfe7whh3zu4ql5rvpg044lyk6cuu7fpnd9e
  - key: amount
    value: "407935"
- type: message
  attributes:
  - key: module
    value: staking
  - key: sender
    value: cosmos14fr86wh3dfvpnh88tny4ek257vtp3lr0kkgnx3
  - key: action
    value: delegate

@alessio
Copy link
Contributor

alessio commented Apr 13, 2020

Nevermind @franono. If I remove the --trust-node flag then I can reproduce it. I'm on it.

@alessio alessio removed this from the Backlog milestone Apr 14, 2020
alessio pushed a commit that referenced this issue Apr 14, 2020
github.com/spf13/viper's recent releases introduced a semantic
change in some public API such as viper.IsSet(), which have
broken some of our flags checks. Instead of checking whether
users have changed a flag's default value we should rely on such
defaults and adjust runtime behaviour accordingly. In order to do
so, it's important that we pick sane defaults for all our flags.

The --pruning flag and configuration option now allow for a
fake custom strategy. When users elect custom, then the
pruning-{keep,snapshot}-every options are interpreted and
parsed; else they're ignored.
Zero is pruning-{keep,snapshot}-every default value. When
users choose to set a custom pruning strategy they are
signalling that they want more fine-grainted control, therefore
it's legitimate to expect them to know what they are doing and
enter valid values for both options.

Ref #5964
@alessio
Copy link
Contributor

alessio commented Apr 15, 2020

This is not solved in the v0.37.x series of cosmos-sdk. Hence reopening.

@alessio alessio reopened this Apr 15, 2020
@alessio alessio mentioned this issue Apr 21, 2020
11 tasks
@alessio
Copy link
Contributor

alessio commented Apr 22, 2020

Fixed in cosmos-sdk v0.37.10. Hence closing.

@alessio alessio closed this as completed Apr 22, 2020
@alessio
Copy link
Contributor

alessio commented Apr 22, 2020

We need to release this too #6021. Hence reopening.

@alessio
Copy link
Contributor

alessio commented Apr 23, 2020

This is now fixed in cosmos-sdk v0.37.11. Hence closing.

@alessio alessio closed this as completed Apr 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants