fix(evmengine): nil panic with optimistic build enabled #128

zsystm · 2024-09-18T05:28:49Z

cmtAPI is lazily set, so during replaying blocks it is nil.
we should add nil checking logic to avoid panic.
this PR is non state breaking meaning code changes won't do any state changes. so the PR doesn't require hard fork.

issue: #127

edisonz0718 · 2024-09-19T05:36:30Z

client/x/evmengine/keeper/keeper.go

@@ -172,6 +172,11 @@ func (k *Keeper) parseAndVerifyProposedPayload(ctx context.Context, msg *types.M
 //
 // Note that the validator set can change, so this is an optimistic check.
 func (k *Keeper) isNextProposer(ctx context.Context, currentProposer []byte, currentHeight int64) (bool, error) {
+ // cometAPI is lazily set and may be nil on startup (e.g. replayBlocks).


I think cometAPI is set during node start. It doesn't seem to be lazily set.

@edisonz0718
By ‘lazily set,’ I meant that, as you can see in the following code, cometAPI is only set after newCometNode finishes, which includes the process of replaying blocks (if any).
During this replay, the postFinalizeBlock is called, but at that point, cometAPI is still nil.

story/client/app/start.go

Lines 133 to 140 in 9603826

cmtNode, err := newCometNode(ctx, &cfg.Comet, app, privVal)

if err != nil {

return nil, errors.Wrap(err, "create comet node")

}

rpcClient := rpclocal.New(cmtNode)

cmtAPI := comet.NewAPI(rpcClient)

app.SetCometAPI(cmtAPI)

I will update the comments to clarify this.

@edisonz0718
updated comment in 6cb1e6d

Ok make sense

limengformal · 2024-09-23T05:42:14Z

@edisonz0718 @zsystm Will this change cause any state change? Meaning do we need hard fork to apply this change?

zsystm · 2024-09-23T05:46:22Z

@limengformal
No. This PR is not related any state changes. So we don't need hard fork for this change.

cmtAPI is lazily set, so during replyBlocks it is nil.

github-actions · 2024-10-01T00:25:43Z

Binary uploaded successfully 🎉

📦 Version Name: 0.10.1-unstable-0d090ff
📦 Download Source: AWS S3

cmtAPI is lazily set, so during replyBlocks it is nil.

* feat(x/evmstaking): max unbond withdrawal * chore(x/evmstaking): bump log level * feat(x/evmstaking): spendable coin max amount with tests * feat(x/evmstaking): pass zero amount * chore(x/evmstaking): move logs * chore(release): finalize client 0.10.1 stable release * chore(release): begin story v0.10.2 unstable release * feat(cmd): one block rollback (#157) add cometBFT's one block rollback cmd * chore(release): finalize client 0.10.2 stable release * chore(release): begin client 0.11.0 unstable release * fix(x/evmstaking): endblock unbond branch check (#163) * fix(x/evmstaking): branch condition on max spendable * test(x/evmstaking): add expected call in tests * feat(cli): add unjail validator subcommand (#170) * feat(cli): add unjail validator subcommand * fix: use static predeploy ca in validator logic * chore(release): finalize client 0.11.0 stable release * chore(release): begin story client v0.12.0 unstable release * feat(epochs): port epochs module from cosmos-sdk (#101) * chore: modify path for mockgen (#121) * chore(evmstaking): remove unnecessary codes (#125) * add test cases for evmstaking/types/params increased test coverage to 100% changes validate functions use concrete type instead of any. there is no reasoning for using any. rename withdraw test suite to avoid name conflict with param test suite * add test cases for evmstaking/keeper/unjail increased test coverage to 100% * add test cases for genesis increased coverage to 71.4% * fix syntax after rebase * add test cases for genesis (#112) increase coverage to 100% * fix(evmstaking): query withdrawal queue (#113) it should append QueueElementsPrefixSuffix when query elements of queue it fixes #84 * test(evmengine): add test cases for db (#120) * add test cases to db * test(evmengine): add test cases for helper (#131) * add test cases for helper increased test coverage to 100% * fix ci * add test cases for keeper (#132) increased test coverage to 89.7% add ci rule to not use t.Parallel to avoid data race issue for cosmos orm table. * test(evmengine): add test cases for genesis (#134) increase coverage to 83.3% moved common function to keeper_internal_test.go * add test cases for genesis (#135) * test(evmengine): add test cases for params (#136) increased coverage to 77.3% * add test cases for params (#137) increased coverage to 100% * feat(script): auto add binary version to s3 file (#142) * add version txt * fixed task name * fixed url * fixed url * opti title * finish test * fixed platform * fixed pre-commit * fix(evmengine): nil panic with optimistic build enabled (#128) cmtAPI is lazily set, so during replyBlocks it is nil. * test(evmengine/keeper): add test cases for msg server (#100) changes of mockEngineAPI - forceInvalidNewPayloadV3 and forceInvalidForkchoiceUpdatedV3 are added to simulate failed apis changes of engineMock - Add storeKey to make engineMock's methods dependent on sdk.Context for better testability NewBlock - Because of above changes, we need to rlp encode and decode block data. But if we create a block with nil withdrawals and nil withdrawalHash, rlp doesn't work well (couldn't figure out the root cause) - To avoid rlp error, pass non-nil withdrawals so withdrawalHash can be set as non-nil emptyHash value. * test(evmengine): add test cases for abci (#143) increased coverage to 83.8% added mock getPayloadV3Func for mocking getPayloadV3 * feat(genesis): disable vote extension by default (#173) * refactor(panic): remove unused or unnecessary panic code (#171) * feat(mint): customized mint module (#169) * feat(mint): port mint module from cosmos sdk v0.50.7 * feat(mint): port mint module from cosmos sdk v0.50.7 * feat(mint): port mint module from cosmos sdk v0.50.7 * feat(mint): port mint module from cosmos sdk v0.50.7 * feat(mint): remove useless parameters * feat(mint): new inflation function * feat(mint): mint parameters change event log processing * feat(mint): mint module api * feat(mint): placeholder for param change events * feat(mint): fix unit test * feat(mint): fix based on comments * feat(mint): fix based on comments * feat(mint): update readme * feat(mint): update readme * feat(netconf): fix genesis config of mint module in local netconf (#175) * feat(contracts): transparent proxies and el genesis (#165) * Change UUPS by TransparentUpgradeableProxy, use etching to generate initial contracts * etch all the contracts * git ignore local dumps * fix contracts/script/EtchInitialState.s.sol, use it also as setup for tests * fix GenerateAlloc for UpgradeEntryPoint, add allocations and test upgradeability * lint and cl fixes * temporarily disabled solhint in CI/CD * temporarily remove run_solidity_lint * temporarily remove lint form workflow * remove need for env function in test * build(deps): bump github.com/spf13/cobra from 1.8.0 to 1.8.1 (#19) Bumps [github.com/spf13/cobra](https://github.com/spf13/cobra) from 1.8.0 to 1.8.1. - [Release notes](https://github.com/spf13/cobra/releases) - [Commits](spf13/cobra@v1.8.0...v1.8.1) --- updated-dependencies: - dependency-name: github.com/spf13/cobra dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Jongwon Park <jwparktom@gmail.com> * feat(api): add epochs api (#176) * feat(api): add epochs api * feat(api): add epoch info api * feat(mint): update mint param type (#182) * feat(mint): update param type * feat(mint): make param value consistent among example and tests * feat(cli): add key conversion subcommand (#174) * feat(cli): add key conversion subcommand * adds uncompressed pubkey (hex) as input * fix(CI/CD): fix solhint (#190) * fix(solhint): rm redundant pre-commit linting hook * fix(solhint): update linting script & pkg version * chore(contracts): apply linting fixes * chore(cmd): fix validator flag linting issue --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Jongwon Park <contact@parkjongwon.com> Co-authored-by: zsystm <124245155+zsystm@users.noreply.github.com> Co-authored-by: Jongwon Park <jwparktom@gmail.com> Co-authored-by: Narangde <hansol.lee@storyprotocol.xyz> Co-authored-by: zsystm <actor93kor@gmail.com> Co-authored-by: Haodi <82733821@qq.com> Co-authored-by: Ze <edisonz0718@gmail.com> Co-authored-by: Zerui Ge <gezerui1997@gmail.com> Co-authored-by: Ramarti <raul.mf86@gmail.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Seb <sebsadface@gmail.com>

zsystm self-assigned this Sep 18, 2024

zsystm marked this pull request as ready for review September 18, 2024 05:33

zsystm requested review from LeoHChen, AndyBoWu, jdubpark, edisonz0718, ezreal1997, limengformal, Narangde and leeren as code owners September 18, 2024 05:33

edisonz0718 changed the title ~~fix(evmengine): nil panic in isNextProposer during ReplayBlocks with evm-build-optimistic enabled~~ fix(evmengine): nil panic in isNextProposer optimistic build enabled Sep 18, 2024

edisonz0718 changed the title ~~fix(evmengine): nil panic in isNextProposer optimistic build enabled~~ fix(evmengine): nil panic with optimistic build enabled Sep 18, 2024

edisonz0718 reviewed Sep 19, 2024

View reviewed changes

zsystm requested a review from edisonz0718 September 19, 2024 06:15

edisonz0718 approved these changes Sep 19, 2024

View reviewed changes

zsystm force-pushed the zsystm/fix/nil-rpc-during-replays branch from 6cb1e6d to f210b70 Compare September 20, 2024 01:48

limengformal approved these changes Oct 1, 2024

View reviewed changes

zsystm added 2 commits October 1, 2024 09:16

fix nillable cmtAPI

dd6d312

cmtAPI is lazily set, so during replyBlocks it is nil.

update comment

91805bd

zsystm force-pushed the zsystm/fix/nil-rpc-during-replays branch from f210b70 to 91805bd Compare October 1, 2024 00:18

zsystm merged commit 0d090ff into piplabs:main Oct 1, 2024
6 checks passed

zsystm deleted the zsystm/fix/nil-rpc-during-replays branch October 1, 2024 00:23

leeren pushed a commit that referenced this pull request Oct 4, 2024

fix(evmengine): nil panic with optimistic build enabled (#128)

a9bcea0

cmtAPI is lazily set, so during replyBlocks it is nil.

leeren pushed a commit to leeren/story-fork that referenced this pull request Oct 10, 2024

fix(evmengine): nil panic with optimistic build enabled (piplabs#128)

7549d1f

cmtAPI is lazily set, so during replyBlocks it is nil.

leeren pushed a commit to leeren/story-fork that referenced this pull request Oct 10, 2024

fix(evmengine): nil panic with optimistic build enabled (piplabs#128)

51d3280

cmtAPI is lazily set, so during replyBlocks it is nil.

leeren pushed a commit to leeren/story-fork that referenced this pull request Oct 10, 2024

fix(evmengine): nil panic with optimistic build enabled (piplabs#128)

7bd8508

cmtAPI is lazily set, so during replyBlocks it is nil.

leeren pushed a commit to leeren/story-fork that referenced this pull request Oct 10, 2024

fix(evmengine): nil panic with optimistic build enabled (piplabs#128)

53a7d6a

cmtAPI is lazily set, so during replyBlocks it is nil.

leeren pushed a commit that referenced this pull request Oct 15, 2024

fix(evmengine): nil panic with optimistic build enabled (#128)

0604658

cmtAPI is lazily set, so during replyBlocks it is nil.

leeren pushed a commit that referenced this pull request Oct 15, 2024

fix(evmengine): nil panic with optimistic build enabled (#128)

95023e2

cmtAPI is lazily set, so during replyBlocks it is nil.

leeren pushed a commit that referenced this pull request Oct 15, 2024

fix(evmengine): nil panic with optimistic build enabled (#128)

34095f0

cmtAPI is lazily set, so during replyBlocks it is nil.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(evmengine): nil panic with optimistic build enabled #128

fix(evmengine): nil panic with optimistic build enabled #128

zsystm commented Sep 18, 2024 •

edited

Loading

edisonz0718 Sep 19, 2024

zsystm Sep 19, 2024 •

edited

Loading

zsystm Sep 19, 2024

edisonz0718 Sep 19, 2024

limengformal commented Sep 23, 2024

zsystm commented Sep 23, 2024 •

edited

Loading

github-actions bot commented Oct 1, 2024

	cmtNode, err := newCometNode(ctx, &cfg.Comet, app, privVal)
	if err != nil {
	return nil, errors.Wrap(err, "create comet node")
	}

	rpcClient := rpclocal.New(cmtNode)
	cmtAPI := comet.NewAPI(rpcClient)
	app.SetCometAPI(cmtAPI)

fix(evmengine): nil panic with optimistic build enabled #128

fix(evmengine): nil panic with optimistic build enabled #128

Conversation

zsystm commented Sep 18, 2024 • edited Loading

edisonz0718 Sep 19, 2024

Choose a reason for hiding this comment

zsystm Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

zsystm Sep 19, 2024

Choose a reason for hiding this comment

edisonz0718 Sep 19, 2024

Choose a reason for hiding this comment

limengformal commented Sep 23, 2024

zsystm commented Sep 23, 2024 • edited Loading

github-actions bot commented Oct 1, 2024

Binary uploaded successfully 🎉

zsystm commented Sep 18, 2024 •

edited

Loading

zsystm Sep 19, 2024 •

edited

Loading

zsystm commented Sep 23, 2024 •

edited

Loading