fix(baseapp): signal then panic at halt-height

[michaelfig]

Description

Refs: #337, #305
Closes: Agoric/agoric-sdk#8326

Implements both newer Cosmos panic and Agoric-compatible signal behaviour when --halt-height is reached. First, try to SIGINT self, then SIGTERM self, followed by the CONSENSUS FAILURE!!! panic. This will stop the state machine in all cases and should exit the process as Agoric expects.

---

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
• run make lint and make test
• 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)

[JimLarson]

You can compare to my attempt at a fix: https://github.com/agoric-labs/cosmos-sdk/tree/8326-exit-on-halt

Also note that it will fix Agoric/agoric-sdk#8326

[michaelfig]

You can compare to my attempt at a fix: https://github.com/agoric-labs/cosmos-sdk/tree/8326-exit-on-halt

Here is my annotated version of the code I see there today:

// halt attempts to gracefully shutdown the node via SIGINT and SIGTERM falling
// back on os.Exit if both fail.
func (app *BaseApp) halt() {
	app.logger.Info("halting node per configuration", "height", app.haltHeight, "time", app.haltTime)

	p, err := os.FindProcess(os.Getpid())
	if err == nil {
		// attempt cascading signals in case SIGINT fails (os dependent)
		sigIntErr := p.Signal(syscall.SIGINT)
		sigTermErr := p.Signal(syscall.SIGTERM)

		if sigIntErr == nil || sigTermErr == nil {
			// mfig: sigIntErr and sigTermErr are always nil if the process
			// was found and the signal was sent.  It has nothing to do with
			// how the process handles the signal, thus just returning here
			// does not prevent new blocks from being produced.  A panic
			// is the most appropriate way to prevent further progress.
			return
		}
	}

	// Resort to exiting immediately if the process could not be found or killed
	// via SIGINT/SIGTERM signals.
	app.logger.Info("failed to send SIGINT/SIGTERM; exiting...")
	os.Exit(0) // mfig: this should not be a 0 (success) exit code
}

[gibson042]

As noted, I'm uncomfortable with using something other than block time to establish "now". Is there a way to get a block context into the function?

[gibson042]

Suggested change

	// checkHalt checkes if height or time exceeds halt-height or halt-time respectively.
	// checkHalt forces a process exit if height or time exceeds halt-height or halt-time respectively.

[michaelfig]

Thanks, I've rephrased.

[gibson042]

Isn't this invalid? time.Unix requires POSIX epoch timestamp input in the form of sec and nsec arguments, and returns a time.Time which is incomparable with int64.

Suggested change

	case app.haltTime > 0 && time.Unix() > int64(app.haltTime):
	case app.haltTime > 0 && time.Now().Unix() > int64(app.haltTime):

(but I'm uncomfortable with using local time rather than block time for this)

[michaelfig]

Ah. You were misled by the fact the call wasn't using a static function of the time package, because that package was shadowed by checkHalt's time argument. So, it was actually using the argument's instance method, Time.Unix. To avoid this confusion, I've renamed the argument to tm instead.

[gibson042]

switch seems too verbose here.

Suggested change

	var halt bool
	switch {
	case app.haltHeight > 0 && uint64(height) > app.haltHeight:
	halt = true
	case app.haltTime > 0 && time.Unix() > int64(app.haltTime):
	halt = true
	}
	halt := (app.haltHeight > 0 && uint64(height) > app.haltHeight) ||
	(app.haltTime > 0 && time.Unix() > int64(app.haltTime))

or

Suggested change

	var halt bool
	switch {
	case app.haltHeight > 0 && uint64(height) > app.haltHeight:
	halt = true
	case app.haltTime > 0 && time.Unix() > int64(app.haltTime):
	halt = true
	}
	var halt bool
	if app.haltHeight > 0 && uint64(height) > app.haltHeight {
	halt = true
	} else if app.haltTime > 0 && time.Unix() > int64(app.haltTime) {
	halt = true
	}

[michaelfig]

Thanks!

[michaelfig]

I'm uncomfortable with using something other than block time to establish "now". Is there a way to get a block context into the function?

The CometBFT block header as supplied to app.BeginBlock is exactly what calls checkHalt with in-consensus arguments:

 	app.checkHalt(req.Header.Height, req.Header.Time)

[gibson042]

Suggestions for further clarification:

Suggested change

	// checkHalt forces a state machine halt and attempts to kill the current
	// process if height or tm exceeds halt-height or halt-time respectively.
	func (app *BaseApp) checkHalt(height int64, tm time.Time) {
	// checkHalt forces a state machine halt and attempts to kill the current
	// process if block height or timestamp exceeds halt-height or halt-time respectively.
	func (app *BaseApp) checkHalt(blockHeight int64, blockTime time.Time) {

[gibson042]

Suggested change

	app.logger.Info("halt per configuration", "height", app.haltHeight, "time", app.haltTime)
	app.logger.Info("halt per configuration", "haltHheight", app.haltHeight, "haltTime", app.haltTime, "blockHeight", height, "blockTime", tm)

[mhofman]

FYI, the checkHalt from #305 (restored here) was an unmodified port of cosmos#16639. That said newer versions of cosmos-sdk take a slightly different approach, so I'm totally ok deviating from the upstream patch as we'll have to re-integrate it anyway.

Overall LGTM, sorry for dropping the ball on this review.