Skip to content

Backport of sam/abci-responses (#9090)#9159

Merged
samricotta merged 28 commits intov0.34.xfrom
abci-responses-flag-merge
Aug 11, 2022
Merged

Backport of sam/abci-responses (#9090)#9159
samricotta merged 28 commits intov0.34.xfrom
abci-responses-flag-merge

Conversation

@samricotta
Copy link
Contributor

Backport of sam/abci-responses #9090

config/config.go Outdated
Comment on lines +409 to +412
// Set false to ensure ABCI responses are persisted.
// ABCI responses are required for /BlockResults RPC queries, and
// to reindex events in the command-line tool.
DiscardABCIResponses bool `mapstructure:"persist-abci-responses"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Set false to ensure ABCI responses are persisted.
// ABCI responses are required for /BlockResults RPC queries, and
// to reindex events in the command-line tool.
DiscardABCIResponses bool `mapstructure:"persist-abci-responses"`
// Set false to ensure ABCI responses are persisted.
// ABCI responses are required for /BlockResults RPC queries, and
// to reindex events in the command-line tool.
DiscardABCIResponses bool `mapstructure:"discard_abci_responses"`

The keys changed from this_style to this-style between releases. I'm also realizing that the config key is persist-abci-responses but the field name is DiscardABCIResponses which have opposite meanings. I think we should change the key to match the field name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this completely makes sense, Ill make those updates

Copy link
Contributor

@williambanfield williambanfield left a comment

Choose a reason for hiding this comment

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

Looks like it needs to have gofmt run in a few spots. Should be good after that.

var (
stateKey = []byte("stateKey")
)
// database key
Copy link
Contributor

Choose a reason for hiding this comment

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

what happened here?

state/store.go Outdated
Comment on lines +27 to +33
const (
prefixValidators = int64(5)
prefixConsensusParams = int64(6)
prefixAllABCIResponses = int64(7)
prefixState = int64(8)
prefixLastABCIResponse = int64(13)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to use all the old prefixes and key encodings

Copy link
Contributor

Choose a reason for hiding this comment

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

Else this breaks v0.34.x

Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably even have tests that ensure keys don't change throughout a release

@williambanfield
Copy link
Contributor

Should we instead be porting this into main so that it goes out with the v0.37 release, or do we need it in v0.34 quickly as well?

@cmwaters
Copy link
Contributor

cmwaters commented Aug 4, 2022

My thought was to get it out in v0.34.21 as well so we get it into users hands quicker

Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

Looks good. Just a few nits

@samricotta samricotta force-pushed the abci-responses-flag-merge branch from 4292895 to 1c98260 Compare August 9, 2022 14:46
@samricotta samricotta merged commit fbd754b into v0.34.x Aug 11, 2022
@samricotta samricotta deleted the abci-responses-flag-merge branch August 11, 2022 08:41
samricotta added a commit that referenced this pull request Aug 11, 2022
*backport of sam/abci-responses

Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
samricotta added a commit that referenced this pull request Aug 11, 2022
*backport of sam/abci-responses

Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
samricotta added a commit that referenced this pull request Aug 18, 2022
*backport of sam/abci-responses

Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
samricotta added a commit that referenced this pull request Aug 19, 2022
*backport of sam/abci-responses

Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
samricotta added a commit that referenced this pull request Aug 22, 2022
* Backport of sam/abci-responses (#9090) (#9159)

Signed-off-by: Thane Thomson <connect@thanethomson.com>
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
Co-authored-by: Thane Thomson <connect@thanethomson.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