fix: remove previous header in Prepare/Process Proposal + provide chain id in baseapp + fix context for verifying txs #15303

facundomedica · 2023-03-08T14:20:31Z

Description

Issue

Some values (like chain ID) were being leaked from the previous block/initialization into PrepareProposal and ProcessProposal, these values are only available if:

The node has never been stopped since the genesis block (as these values are set on InitChain)
The node has already commited a block (as the previous header was being used for the new state of prepare and process proposal).

So if a node is restarted, during the first prepare and process proposal these values won't be populated, and that will cause issues if they are being used.

Solution

Remove any previous header information from a previous block in the prepare and process proposal contexts, making things consistent at every height.

Added ChainID to baseapp
Use an empty header in Commit() with only the chain id set
Fix context for prepare and process proposal

Closes: #15269

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...

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)

…ess Proposal

baseapp/abci.go

…/empty-header-prepproc-prop

08d2 · 2023-03-09T01:38:27Z

Solution: Remove any previous header information from a previous block in the prepare and process proposal contexts, making things consistent at every height.

Does this mean that PrepareProposal and ProcessProposal would lose access to metadata for the previous/committed height? That information is important! It shouldn't be removed.

facundomedica · 2023-03-09T12:53:23Z

@08d2

Does this mean that PrepareProposal and ProcessProposal would lose access to metadata for the previous/committed height? That information is important! It shouldn't be removed.

That data is being leaked from previous blocks, they are not something we can get in prepareproposal and processproposal's request; this could have some nasty side effects. You might be able to query stuff from a previous block, but I'm not entirely sure tbh. We are trying to always provide the chain id tho.

alexanderbez · 2023-03-09T14:25:53Z

So the things that are vital for PrepareProposal and ProcessProposal:

Be based on the previous block's state (we have this today)
Have access to the request types (we have this today)
Have a context that is populated with metadata that could be useful for various use-cases such as Chain-ID (WIP in this PR)

Everything else, e.g. previous header, is useless.

alexanderbez

Approach looks reasonable to me. What do others think?

@tac0turtle @aaronc

baseapp/abci.go

@@ -154,6 +164,10 @@

 // BeginBlock implements the ABCI application interface.
 func (app *BaseApp) BeginBlock(req abci.RequestBeginBlock) (res abci.ResponseBeginBlock) {
+	if req.Header.ChainID != app.chainID {
+		panic(fmt.Sprintf("invalid chain-id on BeginBlock; expected: %s, got: %s", app.chainID, req.Header.ChainID))


baseapp/abci.go

@@ -154,6 +164,10 @@

 // BeginBlock implements the ABCI application interface.
 func (app *BaseApp) BeginBlock(req abci.RequestBeginBlock) (res abci.ResponseBeginBlock) {
+	if req.Header.ChainID != app.chainID {
+		panic(fmt.Sprintf("invalid chain-id on BeginBlock; expected: %s, got: %s", app.chainID, req.Header.ChainID))


wrong button on mobile.. I just wanted to read it again

…/empty-header-prepproc-prop

…cosmos/cosmos-sdk into facu/empty-header-prepproc-prop

kocubinski

LGTM, nice fix.

samricotta

lgtm but maybe someone a bit more seasoned on this one

…in id in baseapp + fix context for verifying txs (#15303) ## Description ### Issue Some values (like chain ID) were being leaked from the previous block/initialization into PrepareProposal and ProcessProposal, these values are only available if: 1. The node has never been stopped since the genesis block (as these values are set on `InitChain`) 2. The node has already commited a block (as the previous header was being used for the new state of prepare and process proposal). So if a node is restarted, during the first prepare and process proposal these values won't be populated, and that will cause issues if they are being used. ### Solution Remove any previous header information from a previous block in the prepare and process proposal contexts, making things consistent at every height. - Added ChainID to baseapp - Use an empty header in Commit() with only the chain id set - Fix context for prepare and process proposal Closes: #15269 --- ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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) (cherry picked from commit 6a03586) # Conflicts: # baseapp/abci_test.go # baseapp/baseapp.go # server/rollback.go # testutil/network/network.go

…in id in baseapp + fix context for verifying txs (backport #15303) (#15377) Co-authored-by: Facundo Medica <[email protected]> Co-authored-by: Facundo Medica <[email protected]>

…in id in baseapp + fix context for verifying txs (cosmos#15303) ## Description ### Issue Some values (like chain ID) were being leaked from the previous block/initialization into PrepareProposal and ProcessProposal, these values are only available if: 1. The node has never been stopped since the genesis block (as these values are set on `InitChain`) 2. The node has already commited a block (as the previous header was being used for the new state of prepare and process proposal). So if a node is restarted, during the first prepare and process proposal these values won't be populated, and that will cause issues if they are being used. ### Solution Remove any previous header information from a previous block in the prepare and process proposal contexts, making things consistent at every height. - Added ChainID to baseapp - Use an empty header in Commit() with only the chain id set - Fix context for prepare and process proposal Closes: cosmos#15269 --- ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)

robert-zaremba · 2023-10-30T13:50:07Z

server/util.go

@@ -455,7 +456,19 @@ func DefaultBaseappOptions(appOpts types.AppOptions) []func(*baseapp.BaseApp) {
 		panic(err)
 	}

-	snapshotDir := filepath.Join(cast.ToString(appOpts.Get(flags.FlagHome)), "data", "snapshots")
+	homeDir := cast.ToString(appOpts.Get(flags.FlagHome))
+	chainID := cast.ToString(appOpts.Get(flags.FlagChainID))


I think this should not be a part of app config / flags. ChainID should be always read from the genesis , and panic if it's not there.

Info about this is missing in UPGRADING.md (0.47)

genesis does not always have the correct genesis, in most cases the file is incorrect

danwt · 2024-09-12T17:48:31Z

In light of this, is there any way to change chain ID on the fly without requiring a full genesis import? Supposing a single trustworthy validator.

Thanks a lot

facundomedica added 2 commits March 8, 2023 11:14

fix: remove previous header to avoid inconsistencies in Prepare\/Proc…

5600add

…ess Proposal

nit

a07fd6f

facundomedica marked this pull request as ready for review March 8, 2023 14:31

facundomedica requested a review from a team as a code owner March 8, 2023 14:31

github-prbot requested review from a team, kocubinski and tac0turtle and removed request for a team March 8, 2023 14:32

facundomedica requested review from JeancarloBarrios and removed request for tac0turtle March 8, 2023 14:32

facundomedica added C:ABCI backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release labels Mar 8, 2023

alexanderbez reviewed Mar 8, 2023

View reviewed changes

baseapp/abci.go Outdated Show resolved Hide resolved

get chain id from genesis

fbe491f

github-actions bot added the C:CLI label Mar 8, 2023

facundomedica added 10 commits March 8, 2023 14:59

progress

ac12759

progress

233a4a4

progress

fe1cd5b

progress

257f4be

progress

06d2021

progress

4a79357

progress

71d93ca

progress

755611d

fix leak

be89d62

Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into facu…

bf4c3f2

…/empty-header-prepproc-prop

facundomedica changed the title ~~fix: remove previous header to avoid inconsistencies in Prepare/Process Proposal~~ fix: remove previous header in Prepare/Process Proposal + provide chain id in baseapp Mar 8, 2023

alexanderbez reviewed Mar 9, 2023

View reviewed changes

baseapp/abci.go Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems Mar 10, 2023

View reviewed changes

julienrbrt previously approved these changes Mar 10, 2023

View reviewed changes

facundomedica and others added 5 commits March 13, 2023 09:47

Merge branch 'main' into facu/empty-header-prepproc-prop

6b8b529

Merge branch 'main' into facu/empty-header-prepproc-prop

02b6ad1

Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into facu…

143af44

…/empty-header-prepproc-prop

remove some unnecessary changes, like new lines

f307b3b

Merge branch 'facu/empty-header-prepproc-prop' of https://github.com/…

89b90da

…cosmos/cosmos-sdk into facu/empty-header-prepproc-prop

github-actions bot removed the C:CLI label Mar 13, 2023

kocubinski approved these changes Mar 13, 2023

View reviewed changes

samricotta approved these changes Mar 13, 2023

View reviewed changes

facundomedica added 2 commits March 13, 2023 12:34

Merge branch 'main' into facu/empty-header-prepproc-prop

ed69776

Merge branch 'main' into facu/empty-header-prepproc-prop

814fc92

julienrbrt added the A:automerge Automatically merge PR once all prerequisites pass. label Mar 13, 2023

mergify bot merged commit 6a03586 into main Mar 13, 2023

mergify bot deleted the facu/empty-header-prepproc-prop branch March 13, 2023 17:15

mergify bot mentioned this pull request Mar 13, 2023

fix: remove previous header in Prepare/Process Proposal + provide chain id in baseapp + fix context for verifying txs (backport #15303) #15377

Merged

facundomedica added a commit that referenced this pull request Mar 13, 2023

fix: sims after merging #15303

6395d70

facundomedica added a commit that referenced this pull request Mar 13, 2023

fix: sims after merging #15303 (#15379)

5a7b442

faddat mentioned this pull request Mar 28, 2023

update linter to 1521 ChihuahuaChain/cosmos-sdk#6

Closed

19 tasks

larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023

fix: sims after merging cosmos#15303 (cosmos#15379)

84d8023

facundomedica mentioned this pull request Jul 26, 2023

fix(simapp): set chain-id on NewSimApp for sim bench tests #17138

Merged

20 tasks

facundomedica mentioned this pull request Oct 30, 2023

feat: upgrade to SDK v0.47 umee-network/umee#2144

Closed

17 tasks

robert-zaremba reviewed Oct 30, 2023

View reviewed changes

0xstepit mentioned this pull request Jul 1, 2024

chore(cachemulti): add Copy method evmos/cosmos-sdk#52

Merged

mikluke mentioned this pull request Oct 16, 2024

[Bug]: genesis_file parameter of config.toml is ignored on start cmd #22277

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: remove previous header in Prepare/Process Proposal + provide chain id in baseapp + fix context for verifying txs #15303

fix: remove previous header in Prepare/Process Proposal + provide chain id in baseapp + fix context for verifying txs #15303

facundomedica commented Mar 8, 2023 •

edited

Loading

08d2 commented Mar 9, 2023 •

edited

Loading

facundomedica commented Mar 9, 2023

alexanderbez commented Mar 9, 2023 •

edited

Loading

alexanderbez left a comment

kocubinski left a comment

samricotta left a comment

robert-zaremba Oct 30, 2023

tac0turtle Oct 30, 2023

danwt commented Sep 12, 2024

fix: remove previous header in Prepare/Process Proposal + provide chain id in baseapp + fix context for verifying txs #15303

fix: remove previous header in Prepare/Process Proposal + provide chain id in baseapp + fix context for verifying txs #15303

Conversation

facundomedica commented Mar 8, 2023 • edited Loading

Description

Issue

Solution

Author Checklist

Reviewers Checklist

08d2 commented Mar 9, 2023 • edited Loading

facundomedica commented Mar 9, 2023

alexanderbez commented Mar 9, 2023 • edited Loading

alexanderbez left a comment

Choose a reason for hiding this comment

kocubinski left a comment

Choose a reason for hiding this comment

samricotta left a comment

Choose a reason for hiding this comment

robert-zaremba Oct 30, 2023

Choose a reason for hiding this comment

tac0turtle Oct 30, 2023

Choose a reason for hiding this comment

danwt commented Sep 12, 2024

facundomedica commented Mar 8, 2023 •

edited

Loading

08d2 commented Mar 9, 2023 •

edited

Loading

alexanderbez commented Mar 9, 2023 •

edited

Loading