Ethanfrey/fix trace flag

[ethanfrey]

Description

The --trace flag was somehow disabled in the last year or two. It is a very helpful flag for local debugging or dev nets, as it returns the full stack trace in the abci error logs (this was working in January 2018). Maybe this was disabled with new error handling?

In any case, this PR makes two changes:

• Add a debug flag to BaseApp, so it doesn't redact errors (including panics) and shows a full stack trace if present (via %+v).
• When redacting, we never change the abci code (which we had done with ErrPanic), as that could cause a consensus failure between a node running with BaseApp.debug and one without.

This is very helpful for testnets, or debugging with sentry nodes on a mainnet.

Note that in of itself, it doesn't change the functionality of any apps, you must enable that with the following lines in app.go:

	debug := viper.GetBool(cli.TraceFlag)
	baseAppOptions = append(baseAppOptions, bam.SetDebug(debug))

(I am happy for a deeper integration, but it didn't seem like the above code fit in the start command

@alessio @alexanderbez I would like to include this in #6522 Can you please discuss and add a label if accepted. (I have no ability to add labels)

Note: the functionality of --trace is documented here: https://github.com/cosmos/cosmos-sdk/blob/v0.38.4/CHANGELOG.md#060-june-22-2017 and there is no entry stating it should be disabled.

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer
• Review Codecov Report in the comment section below once CI passes

[codecov]

Codecov Report

Merging #6551 into backport/release/v0.38.x will increase coverage by 0.07%.
The diff coverage is 60.84%.

@@                     Coverage Diff                      @@
##           backport/release/v0.38.x    #6551      +/-   ##
============================================================
+ Coverage                     51.50%   51.58%   +0.07%     
============================================================
  Files                           336      337       +1     
  Lines                         20577    20629      +52     
============================================================
+ Hits                          10599    10641      +42     
- Misses                         9175     9182       +7     
- Partials                        803      806       +3     

[alexanderbez]

LGTM, I left some comments. Although, this wont do anything as-is. A trace flag needs to be added to the CLI.

[ethanfrey]

LGTM, I left some comments. Although, this wont do anything as-is. A trace flag needs to be added to the CLI.

A --trace flag is there as of 3 years: https://github.com/tendermint/tendermint/blob/master/libs/cli/setup.go#L31 (what the changelog entry refers to)

[alexanderbez]

LGTM, I left some comments. Although, this wont do anything as-is. A trace flag needs to be added to the CLI.

A --trace flag is there as of 3 years: https://github.com/tendermint/tendermint/blob/master/libs/cli/setup.go#L31 (what the changelog entry refers to)

That entire line is being removed and isn't even used. Please add a --trace flag to server/start.go, should be pretty straightforward.

[ethanfrey]

I'll gladly update the start command.

Do you have a better idea how to add this to baseapp? I just suggest people call the viper.GetBool(TraceFlag), but it would be even cooler to set it in start.

[alexanderbez]

We're removing global viper usage, but in this case, just having it use viper is fine since we're backporting. You don't need anymore changes to baseapp.

[ethanfrey]

Please add a --trace flag to server/start.go, should be pretty straightforward.

Done

[alexanderbez]

We also need to bind it with viper below (note, this will be soon undone, but we're backporting this)

[alexanderbez]

ACK

[ethanfrey]

Thanks

[alexanderbez]

@ethanfrey can you remind me, did we get this into master?

[ethanfrey]

No, this didn't go into master. But I think you said there was a giant refactor of the cli going on, so it probably is better to port once the refactor finished.