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

fix: halt-height behavior is not deterministic #305

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

mhofman
Copy link

@mhofman mhofman commented Sep 13, 2023

Description

Refs: Agoric/agoric-sdk#8326

Port (cherry-pick with modifications) of cosmos#16639

Adapt by using BeginBlock and panic, similar to the adaptation in crypto-org-chain/chain-main#998
Adapted the Unit test as well

Once we reach the version of cosmos-sdk (v0.50) with the FinalizeBlock semantics (CometBFT v0.38), we can adopt the upstream change more directly.


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 in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • 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 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)

@mhofman mhofman requested a review from JimLarson September 13, 2023 21:18
Copy link

@JimLarson JimLarson left a comment

Choose a reason for hiding this comment

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

Please add a note in the PR description stating whether we should abandon this change in favor of the cherry-pick source once we upgrade to that point.

CHANGELOG.md Outdated
@@ -37,6 +37,8 @@ Ref: https://keepachangelog.com/en/1.0.0/

## [Unreleased]

* (baseapp) [#305](https://github.com/agoric-labs/cosmos-sdk/pull/305) Make sure we don't execute blocks beyond the halt height.

Choose a reason for hiding this comment

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

Move this to CHANGELOG-Agoric.md - creating it if #306 doesn't land first.

(Moving our changes to a separate file makes it easier to merge changes from cosmos.)

Also, note that it's a backport of cosmos#16639. This will also make future merges easier.

Copy link
Author

Choose a reason for hiding this comment

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

Done, but I'll wait until #306 merges first.

Port of cosmos#16639

Co-authored-by: yihuang <huang@crypto.com>
@mhofman mhofman force-pushed the mhofman/8326-check-halt-begin branch from a7b31b8 to 762e033 Compare September 19, 2023 23:09
@mhofman mhofman merged commit 7b8423a into Agoric Sep 19, 2023
30 of 31 checks passed
@mhofman mhofman deleted the mhofman/8326-check-halt-begin branch September 19, 2023 23:58
JimLarson added a commit to Agoric/agoric-sdk that referenced this pull request Nov 5, 2023
The backport referenced above has the agd process hang around after halting.
Wrote a wrapper utility which scans stderr for the halt application message,
interrupts the process, then propagates its error signal. Conventional shell
scripting is not up to the challenge of expressing this behavior.
JimLarson added a commit that referenced this pull request Nov 9, 2023
This reverts commit 7b8423a.

This commit caused test failures in agoric-sdk.
Reverting to land other changes in agoric-sdk.
We'll restore the change and try to debug the failure
against a smaller diff in the future.
JimLarson added a commit that referenced this pull request Nov 9, 2023
…337)

* Revert "fix: halt-height behavior is not deterministic (#305)"

This reverts commit 7b8423a.

This commit caused test failures in agoric-sdk.
Reverting to land other changes in agoric-sdk.
We'll restore the change and try to debug the failure
against a smaller diff in the future.
JimLarson added a commit to Agoric/agoric-sdk that referenced this pull request Nov 9, 2023
…#305"

This reverts commit efcc3b0.

We decided to roll back the problematic change (agoric-labs/cosmos-sdk#305).
JimLarson added a commit to Agoric/agoric-sdk that referenced this pull request Nov 10, 2023
The backport referenced above has the agd process hang around after halting.
Wrote a wrapper utility which scans stderr for the halt application message,
interrupts the process, then propagates its error signal. Conventional shell
scripting is not up to the challenge of expressing this behavior.
JimLarson added a commit to Agoric/agoric-sdk that referenced this pull request Nov 10, 2023
…#305"

This reverts commit efcc3b0.

We decided to roll back the problematic change (agoric-labs/cosmos-sdk#305).
JimLarson added a commit to Agoric/agoric-sdk that referenced this pull request Nov 13, 2023
The backport referenced above has the agd process hang around after halting.
Wrote a wrapper utility which scans stderr for the halt application message,
interrupts the process, then propagates its error signal. Conventional shell
scripting is not up to the challenge of expressing this behavior.
JimLarson added a commit to Agoric/agoric-sdk that referenced this pull request Nov 13, 2023
…#305"

This reverts commit efcc3b0.

We decided to roll back the problematic change (agoric-labs/cosmos-sdk#305).
mhofman pushed a commit to Agoric/agoric-sdk that referenced this pull request Dec 6, 2023
The backport referenced above has the agd process hang around after halting.
Wrote a wrapper utility which scans stderr for the halt application message,
interrupts the process, then propagates its error signal. Conventional shell
scripting is not up to the challenge of expressing this behavior.
mhofman pushed a commit to Agoric/agoric-sdk that referenced this pull request Dec 6, 2023
…#305"

This reverts commit efcc3b0.

We decided to roll back the problematic change (agoric-labs/cosmos-sdk#305).
michaelfig pushed a commit that referenced this pull request Jul 5, 2024
Port of cosmos#16639

Co-authored-by: yihuang <huang@crypto.com>
JeancarloBarrios pushed a commit that referenced this pull request Sep 28, 2024
Port of cosmos#16639

Co-authored-by: yihuang <huang@crypto.com>
JeancarloBarrios pushed a commit that referenced this pull request Sep 28, 2024
…337)

* Revert "fix: halt-height behavior is not deterministic (#305)"

This reverts commit 7b8423a.

This commit caused test failures in agoric-sdk.
Reverting to land other changes in agoric-sdk.
We'll restore the change and try to debug the failure
against a smaller diff in the future.
JeancarloBarrios pushed a commit that referenced this pull request Sep 28, 2024
Port of cosmos#16639

Co-authored-by: yihuang <huang@crypto.com>
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.

3 participants