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.

Sign up for GitHub

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

Jump to bottom

panic redacted makes debugging difficult #11539

Closed
4 tasks
tac0turtle opened this issue Apr 4, 2022 · 10 comments · Fixed by #11960
Closed
4 tasks

panic redacted makes debugging difficult #11539

tac0turtle opened this issue Apr 4, 2022 · 10 comments · Fixed by #11960
Assignees
@technicallyty
Labels
T: Client UX T: Dev UX UX for SDK developers (i.e. how to call our code) T: UX

Comments

@tac0turtle
Copy link
Member

Summary

When a user runs into this error they have no insight what the issue is and/or how to fix it.

Problem Definition

panic redacted for safety is horrible ux

Proposal

return more descriptive errors when a user encounters an issue. We should only redact the errors that are sensitive not all of them

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@tac0turtle tac0turtle added T: UX T: Dev UX UX for SDK developers (i.e. how to call our code) T: Client UX labels Apr 4, 2022
@alexanderbez
Copy link
Contributor

Yes, yes, yes. I don't know why this was ever added. Not a good choice IMO. Please remove it or change it to something nicer.

cc @aaronc

@yihuang
Copy link
Collaborator

yihuang commented Apr 5, 2022

#9576 does this PR and --trace helps?

@alexanderbez
Copy link
Contributor

alexanderbez commented Apr 5, 2022 •
edited
Loading

No, it's the fact that it's in the tx response, not querying I think.

@ValarDragon
Copy link
Contributor

ValarDragon commented Apr 8, 2022 •
edited
Loading

Super supportive of removing this, I have never found this helpful. I also don't know why it was added.,Have lost dozens of hours due to this existing though.

The only thing it hides at most is your filesystem path to the binary and potentially the folder in which you execute the binary from. This hardly seems like sensitive info to me.

@yihuang
Copy link
Collaborator

yihuang commented Apr 8, 2022

https://github.com/cosmos/cosmos-sdk/blob/master/x/auth/middleware/recovery.go#L75
I think we do return stack trace in panic case?
In previous version, theres' stack trace if pass --trace to the start command.

@cmwaters
Copy link
Contributor

I stumbled across this because I was going to create this exact same issue and yes please fix this if you can. I was going to suggest having a flag to return all panics instead of redacting them

@yihuang
Copy link
Collaborator

yihuang commented Apr 11, 2022

I stumbled across this because I was going to create this exact same issue and yes please fix this if you can. I was going to suggest having a flag to return all panics instead of redacting them

did you use --trace parameter?

@tac0turtle
Copy link
Member Author

did you use --trace parameter?

this works if you are the node operator, but if you are a user using a rpc endpoint and get this error, you end up testing random things to see if it works

@yihuang
Copy link
Collaborator

yihuang commented Apr 11, 2022 •
edited
Loading

did you use --trace parameter?

this works if you are the node operator, but if you are a user using a rpc endpoint and get this error, you end up testing random things to see if it works

I see, so I guess we are talking about whether to turn on the --trace by default?

@alexanderbez
Copy link
Contributor

I'm not sure what --trace even does, as whenever I've used it it seemed to do nothing for me. Regardless, if that is the fix to show the full error or stack trace, then we should make it true by default. Or even just remove it entirely.

@julienrbrt julienrbrt assigned julienrbrt and unassigned julienrbrt May 6, 2022
@alexanderbez alexanderbez mentioned this issue May 16, 2022
Merged
19 tasks
@technicallyty technicallyty self-assigned this May 16, 2022
@mergify mergify bot closed this as completed in #11960 May 19, 2022
mergify bot pushed a commit that referenced this issue May 19, 2022
@technicallyty
refactor: remove redacted message (#11960)
bcf2088 
## Description

Closes: #11539



---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
mergify bot pushed a commit that referenced this issue May 19, 2022
@technicallyty @mergify
refactor: remove redacted message (#11960)
8ab98df 
## Description

Closes: #11539

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)

(cherry picked from commit bcf2088)

# Conflicts:
#	CHANGELOG.md
#	errors/abci.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@technicallyty technicallyty

Labels
T: Client UX T: Dev UX UX for SDK developers (i.e. how to call our code) T: UX
Projects
Cosmos-SDK
Archived in project
Cosmos SDK Maintenance
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor: remove panic redacted message cosmos/cosmos-sdk
7 participants
@yihuang @alexanderbez @ValarDragon @tac0turtle @cmwaters @julienrbrt @technicallyty