Forward port discard abci responses change by samricotta · Pull Request #9286 · tendermint/tendermint

samricotta · 2022-08-18T09:08:44Z

Forward porting abci responses change

*backport of sam/abci-responses Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>

* update to toml

* update last responses key

…on (#9275) * config: Move discard_abci_responses flag into its own storage section Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update config comment to highlight space saving tradeoff Signed-off-by: Thane Thomson <connect@thanethomson.com> Signed-off-by: Thane Thomson <connect@thanethomson.com>

cmwaters

Looks good however there's a lot of formatting changes which makes this PR hard to read.

If there is formatting changes it's best do be done in a separate PR. Make sure you're using go 1.18

cmwaters · 2022-08-18T09:51:59Z

CHANGELOG_PENDING.md


 ### IMPROVEMENTS

+- [config] \#9054 Flag added to overwrite abciresponses.


I don't think we need this as it's first introduced in v0.34.21

cmwaters · 2022-08-18T09:56:36Z

state/store.go

+var (
+	lastABCIResponseKey = []byte("lastABCIResponseKey")
+)


Suggested change

var (

lastABCIResponseKey = []byte("lastABCIResponseKey")

)

var lastABCIResponseKey = []byte("lastABCIResponseKey")

nit

williambanfield · 2022-08-18T17:31:27Z

CHANGELOG_PENDING.md


 ### IMPROVEMENTS

+- [config] \#9054 Flag added to overwrite abciresponses.


Can we give a slightly broader explanation of what this does? "Configuration flag added to permit Tendermint to discard all ABCIResponses except for the most recent."

Was just saying in an above comment that perhaps a changelog is not necessary if it's in the changelog in v0.34.21

Ive changed it to what Will has suggested but I also think that Callum has a point. I think maybe kick this one out

tychoish · 2022-08-19T15:20:41Z

libs/json/doc.go

 // times emitted as "0001-01-01T00:00:00Z" as with encoding/json):
 //
-// 	time.Date(2020, 6, 8, 16, 21, 28, 123, time.FixedZone("UTC+2", 2*60*60))
+// 	time.Date(2020, 6, 8, 16, 21, 28, 123, time.FixedZone("UTC+2", 2*60*60))\


I would avoid this change.

tychoish · 2022-08-19T15:21:59Z

state/store.go

+		tmos.Exit(fmt.Sprintf(`LoadLastABCIResponses: Data has been corrupted or its spec has
+			changed: %v\n`, err))


why is this an exit and not a panic?

tbh I cannot remember exactly why this was an exit rather than a panic. What would be the reasoning to change it to a panic just so im clear

I see that there are 5 such calls throughout the state/store.go file and Exit has been the convention in this file for more than 5 years now. I'd be very interested in knowing what the reasoning here was originally.

Looking at all the places this is called from, it doesn't seem as though there are any defers prior to where these functions are called, but at first glance it seems a little dangerous to have Exits embedded within the code instead of panics.

It may be worth a follow-up investigation here after this PR - perhaps a codebase-wide audit of where Exit's used instead of panic to see whether it's still applicable, and where it is applicable we should document why it's applicable in comments colocated with the Exit calls.

thanethomson

Minor nits, but otherwise LGTM.

thanethomson · 2022-08-20T10:24:38Z

cmd/tendermint/commands/reindex_event.go

 the tooling will reindex until the latest block height(inclusive). User can omit
 either or both arguments.
+
+Note: This operation requires ABCIResponses. Do not set DiscardABCIResponses to true if you


Suggested change

Note: This operation requires ABCIResponses. Do not set DiscardABCIResponses to true if you

Note: This operation requires ABCI responses. Do not set DiscardABCIResponses to true if you

thanethomson · 2022-08-20T10:33:31Z

state/store.go

+}
+
+type StoreOptions struct {
+


Suggested change

thanethomson · 2022-08-20T10:57:51Z

state/store.go

+		tmos.Exit(fmt.Sprintf(`LoadLastABCIResponses: Data has been corrupted or its spec has
+			changed: %v\n`, err))


I see that there are 5 such calls throughout the state/store.go file and Exit has been the convention in this file for more than 5 years now. I'd be very interested in knowing what the reasoning here was originally.

Looking at all the places this is called from, it doesn't seem as though there are any defers prior to where these functions are called, but at first glance it seems a little dangerous to have Exits embedded within the code instead of panics.

It may be worth a follow-up investigation here after this PR - perhaps a codebase-wide audit of where Exit's used instead of panic to see whether it's still applicable, and where it is applicable we should document why it's applicable in comments colocated with the Exit calls.

samricotta and others added 4 commits August 18, 2022 10:53

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

4be78e3

*backport of sam/abci-responses Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>

Small update to toml.go for abci-responses (#9232)

dab1abe

* update to toml

update default (#9235)

ecc2b7b

Update to ABCILastResponseskey (#9253)

5e31b29

* update last responses key

samricotta requested a review from ebuchman as a code owner August 18, 2022 09:08

samricotta requested a review from a team August 18, 2022 09:08

samricotta force-pushed the f-port-abci-responses branch from 64d9188 to 3f34caf Compare August 18, 2022 09:44

cmwaters approved these changes Aug 18, 2022

View reviewed changes

Merge branch 'main' into f-port-abci-responses

1d2a832

thanethomson mentioned this pull request Aug 18, 2022

v0.37 release plan #9091

Closed

39 tasks

williambanfield approved these changes Aug 18, 2022

View reviewed changes

removing unnecessary formatting (#9291)

f8d959d

samricotta force-pushed the f-port-abci-responses branch 2 times, most recently from 45cebeb to 2d92ab1 Compare August 19, 2022 15:19

tychoish approved these changes Aug 19, 2022

View reviewed changes

formatting changes

572a920

samricotta force-pushed the f-port-abci-responses branch from 2d92ab1 to 572a920 Compare August 19, 2022 15:26

Merge branch 'main' into f-port-abci-responses

fa1fc3f

thanethomson approved these changes Aug 20, 2022

View reviewed changes

samricotta and others added 2 commits August 22, 2022 10:44

Merge branch 'main' into f-port-abci-responses

45f0d4b

review updates

d91954a

samricotta merged commit aa303ed into main Aug 22, 2022

samricotta deleted the f-port-abci-responses branch August 22, 2022 10:53

cmwaters mentioned this pull request Sep 15, 2022

state: restore previous error message #9435

Merged

thanethomson mentioned this pull request Dec 23, 2022

v0.37 release plan cometbft/cometbft#41

Closed

38 tasks


		### IMPROVEMENTS

		- [config] \#9054 Flag added to overwrite abciresponses.

		tmos.Exit(fmt.Sprintf(`LoadLastABCIResponses: Data has been corrupted or its spec has
		changed: %v\n`, err))

	Note: This operation requires ABCIResponses. Do not set DiscardABCIResponses to true if you
	Note: This operation requires ABCI responses. Do not set DiscardABCIResponses to true if you

Conversation

samricotta commented Aug 18, 2022

Uh oh!

cmwaters left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thanethomson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants