fix: halt-height behavior is not deterministic

[yihuang]

Description

Closes: #16638

Solution:

• make sure in state machine that we don't execute block beyond halt-height

---

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)

[github-actions]

Change potentially affects state.

Call sequence:

(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).FinalizeBlock (baseapp/abci.go:648)

[testinginprod]

LGTM, but I would like a test case to be added in order to make sure that in case of refactors of the baseapp code we don't miss behaviours.

[alexanderbez]

I don't quite follow. If I say, for example, halt-height == 1000, then when Commit is called indicating block completion, the process will exit after 1000. How is it non-deterministic? Why is the logic duplicated in FinalizeBlock?

[yihuang]

I don't quite follow. If I say, for example, halt-height == 1000, then when Commit is called indicating block completion, the process will exit after 1000. How is it non-deterministic? Why is the logic duplicated in FinalizeBlock?

because sending signal to self process is asynchronous, the current process can run for a while before the signal is handled and stop the node, I seems to observe that when I use --halt-height with local blocks that it don't stop at the expected height exactly.

[alexanderbez]

Mhhh, so os.Exit() is async?? Or do you mean that os.Exit() actually sends a signal, which is caught in the server package, which case that is obviously async, yes.

[yihuang]

Mhhh, so os.Exit() is async?? Or do you mean that os.Exit() actually sends a signal, which is caught in the server package, which case that is obviously async, yes.

I think it only calls os.Exit if signal sending fails

[alexanderbez]

I'm sorry, I don't follow. When the halt-height is reached, os.Exit() will be called:

https://github.com/cosmos/cosmos-sdk/blob/v0.47.3/baseapp/abci.go#L479-L485

[yihuang]

I'm sorry, I don't follow. When the halt-height is reached, os.Exit() will be called:

https://github.com/cosmos/cosmos-sdk/blob/v0.47.3/baseapp/abci.go#L479-L485

You can check the internal of halt method, it don't call os.Exit unconditionally

[alexanderbez]

You can check the internal of halt method, it don't call os.Exit unconditionally

I don't understand what this means. os.Exit() is called when the halt-height is reached, immediately exiting the process AFAIU.

[yihuang]

You can check the internal of halt method, it don't call os.Exit unconditionally

I don't understand what this means. os.Exit() is called when the halt-height is reached, immediately exiting the process AFAIU.

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 {
			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)
}

os.Exit() is not called if signal sending is successful, it's a fallback if signal sending failed, it's not ideal because the shutdown would not be graceful.

[odeke-em]

LGTM and thank you @yihuang! Would be nice though to have a test to lock-in this behavior to prevent future regressions. It is critical code yet tomorrow I can sneakishly delete it and no tests will fail.

[alexanderbez]

Wow I didn't see the PID logic -- completely forgot about that. I believe I originally wrote this code, but I can't recall why not just resort to os.Exit() always?

[yihuang]

Wow I didn't see the PID logic -- completely forgot about that. I believe I originally wrote this code, but I can't recall why not just resort to os.Exit() always?

maybe for graceful shutdown, although right now we don't have critical stuff to cleanup, but for async commit (memiavl is doing that already), it'll be helpful to wait for it to complete before shutdown.
And I'm not sure if the Commit event of the last block should return successfully for cometbft to finish some post logic.

[yihuang]

LGTM and thank you @yihuang! Would be nice though to have a test to lock-in this behavior to prevent future regressions. It is critical code yet tomorrow I can sneakishly delete it and no tests will fail.

no problem, I just feel the solution is not ideal yet, first of all, there are code duplication between Commit and BeginBlocker, and x/upgrade has done something similar just by panic, so I think maybe just do the panic in BeginBlocker would be good enough, aka. remove the signal sending and os.Exit stuff from Commit? WDYT @alexanderbez

[alexanderbez]

LGTM and thank you @yihuang! Would be nice though to have a test to lock-in this behavior to prevent future regressions. It is critical code yet tomorrow I can sneakishly delete it and no tests will fail.

no problem, I just feel the solution is not ideal yet, first of all, there are code duplication between Commit and BeginBlocker, and x/upgrade has done something similar just by panic, so I think maybe just do the panic in BeginBlocker would be good enough, aka. remove the signal sending and os.Exit stuff from Commit? WDYT @alexanderbez

Yes, a panic in BeginBlock is perfectly reasonable IMO. Just make sure it's triggered at HaltHeight + 1

[yihuang]

Yes, a panic in BeginBlock is perfectly reasonable IMO. Just make sure it's triggered at HaltHeight + 1

done, I think the equivalence of panic in latest abci is just simply returning errors.

[github-actions]

Change potentially affects state.

Call sequence:

(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).FinalizeBlock (baseapp/abci.go:649)