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

ABCI calls should crash/exit in all cases when the call itself reports and error #496

Merged
merged 3 commits into from
Mar 13, 2023

Conversation

sergio-mena
Copy link
Contributor

Closes #490

Followed the call hierarchy of ABCI methods to make sure that reporting an error ultimately results in a crash/exit (also followed new methods in branch feature/abci++vef).

Found one blind spot that should be considered a bug, and should probably be backported. Hence, I'm opening this against main and not feature/abci++vef


PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments

@sergio-mena sergio-mena added the abci Application blockchain interface label Mar 10, 2023
@sergio-mena sergio-mena requested a review from a team as a code owner March 10, 2023 15:21
@sergio-mena sergio-mena self-assigned this Mar 10, 2023
@sergio-mena sergio-mena added backport-to-v0.37.x Tell Mergify to backport the PR to v0.37.x backport-to-v0.34.x Tell Mergify to backport the PR to v0.34.x labels Mar 10, 2023
Copy link
Contributor

@jmalicevic jmalicevic left a comment

Choose a reason for hiding this comment

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

I assume we will fix the vulnerability failures.

@sergio-mena sergio-mena merged commit 2ab8598 into main Mar 13, 2023
@sergio-mena sergio-mena deleted the sergio/490-crash-on-abci-error branch March 13, 2023 15:28
mergify bot pushed a commit that referenced this pull request Mar 13, 2023
…s and error (#496)

* Review call hierarchies of ABCI methods to make sure they panic on error

* Added changelog

* Trying 1.20.2 explicitly

(cherry picked from commit 2ab8598)

# Conflicts:
#	.changelog/v0.37.0/bug-fixes/496-error-on-applyblock-should-panic.md
mergify bot pushed a commit that referenced this pull request Mar 13, 2023
…s and error (#496)

* Review call hierarchies of ABCI methods to make sure they panic on error

* Added changelog

* Trying 1.20.2 explicitly

(cherry picked from commit 2ab8598)

# Conflicts:
#	.github/workflows/govulncheck.yml
#	state/execution.go
sergio-mena added a commit that referenced this pull request Mar 13, 2023
sergio-mena added a commit that referenced this pull request Mar 13, 2023
…ll itself reports and error (#496)

* Review call hierarchies of ABCI methods to make sure they panic on error

* Added changelog

* Trying 1.20.2 explicitly
sergio-mena added a commit that referenced this pull request Mar 13, 2023
sergio-mena added a commit that referenced this pull request Mar 13, 2023
…the call itself reports and error (#496)

* Review call hierarchies of ABCI methods to make sure they panic on error

* Added changelog

* Trying 1.20.2 explicitly
sergio-mena added a commit that referenced this pull request Mar 15, 2023
…s and error (backport #496) (#520)

* ABCI calls should crash/exit in all cases when the call itself reports and error (#496)

* Review call hierarchies of ABCI methods to make sure they panic on error

* Added changelog

* Trying 1.20.2 explicitly

(cherry picked from commit 2ab8598)

# Conflicts:
#	.github/workflows/govulncheck.yml
#	state/execution.go

* Revert "ABCI calls should crash/exit in all cases when the call itself reports and error (#496)"

This reverts commit d2fae79.

* [partial cherry-pick] ABCI calls should crash/exit in all cases when the call itself reports and error (#496)

* Review call hierarchies of ABCI methods to make sure they panic on error

* Added changelog

* Trying 1.20.2 explicitly

---------

Co-authored-by: Sergio Mena <[email protected]>
sergio-mena added a commit that referenced this pull request Mar 16, 2023
…s and error (backport #496) (#519)

* ABCI calls should crash/exit in all cases when the call itself reports and error (#496)

* Review call hierarchies of ABCI methods to make sure they panic on error

* Added changelog

* Trying 1.20.2 explicitly

(cherry picked from commit 2ab8598)

# Conflicts:
#	.changelog/v0.37.0/bug-fixes/496-error-on-applyblock-should-panic.md

* Revert "ABCI calls should crash/exit in all cases when the call itself reports and error (#496)"

This reverts commit 825ac9e.

* [cherry-picked] ABCI calls should crash/exit in all cases when the call itself reports and error (#496)

* Review call hierarchies of ABCI methods to make sure they panic on error

* Added changelog

* Trying 1.20.2 explicitly

---------

Co-authored-by: Sergio Mena <[email protected]>
roy-dydx pushed a commit to dydxprotocol/cometbft that referenced this pull request Jul 11, 2023
…s and error (backport cometbft#496) (cometbft#519)

* ABCI calls should crash/exit in all cases when the call itself reports and error (cometbft#496)

* Review call hierarchies of ABCI methods to make sure they panic on error

* Added changelog

* Trying 1.20.2 explicitly

(cherry picked from commit 2ab8598)

# Conflicts:
#	.changelog/v0.37.0/bug-fixes/496-error-on-applyblock-should-panic.md

* Revert "ABCI calls should crash/exit in all cases when the call itself reports and error (cometbft#496)"

This reverts commit 825ac9e.

* [cherry-picked] ABCI calls should crash/exit in all cases when the call itself reports and error (cometbft#496)

* Review call hierarchies of ABCI methods to make sure they panic on error

* Added changelog

* Trying 1.20.2 explicitly

---------

Co-authored-by: Sergio Mena <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abci Application blockchain interface backport-to-v0.34.x Tell Mergify to backport the PR to v0.34.x backport-to-v0.37.x Tell Mergify to backport the PR to v0.37.x
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

ABCI: Crash in all cases of undefined app behavior
2 participants