8000 fix: invalid `txs_results` returned for legacy ABCI responses (backport #3031) by mergify[bot] · Pull Request #3435 · cometbft/cometbft · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: invalid txs_results returned for legacy ABCI responses (backport #3031) #3435

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 15 commits into from
Jul 9, 2024
Merged

fix: invalid txs_results returned for legacy ABCI responses (backport #3031) #3435

merged 15 commits into from
Jul 9, 2024

Conversation

mergify[bot]
Copy link
Contributor
@mergify mergify bot commented Jul 5, 2024

close: #3002

This PR fixes the issue reported above.

This is not a storage issue in particular, the results are still in storage after an upgrade, but not returned properly by the RPC endpoint. The fix is to make the /block_results endpoint in v0.38 to return properly a legacy ABCI response created with v0.37.

Once this fix is merged on v0.38 and a patch release is cut, any node on v0.38 (e.g. an archive node) that applies the patch release, should have the results returned properly by the RPC /block_results endpoint.

PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments
  • Title follows the Conventional Commits spec
This is an automatic backport of pull request #3031 done by [Mergify](https://mergify.com).

@andynog @mergify
fix: invalid txs_results returned for legacy ABCI responses (#3031)
1c62d03 
close: #3002

This PR fixes the issue reported above.

This is not a storage issue in particular, the results are still in
storage after an upgrade, but not returned properly by the RPC endpoint.
The fix is to make the `/block_results` endpoint in `v0.38` to return
properly a legacy ABCI response created with `v0.37`.

Once this fix is merged on `v0.38` and a patch release is cut, any node
on `v0.38` (e.g. an archive node) that applies the patch release, should
have the results returned properly by the RPC `/block_results` endpoint.

---

#### PR checklist

- [X] Tests written/updated
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] ~~Updated relevant documentation (`docs/` or `spec/`) and code
comments~~
- [X] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

---------

Co-authored-by: Sergio Mena <sergio@informal.systems>
Co-authored-by: Daniel <daniel.cason@informal.systems>
Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
Co-authored-by: Daniel Wedul <github@wedul.com>
(cherry picked from commit db6b608)

# Conflicts:
#	state/errors.go
#	state/state_test.go
#	state/store.go
#	state/store_test.go
@mergify mergify bot requested a review from a team as a code owner July 5, 2024 15:56
@mergify mergify bot added the conflicts label Jul 5, 2024
@mergify Mergify
Copy link
Contributor Author
mergify bot commented Jul 5, 2024

Cherry-pick of db6b608 has failed:

On branch mergify/bp/v0.38.x/pr-3031
Your branch is up to date with 'origin/v0.38.x'.

You are currently cherry-picking commit db6b60822.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	new file:   .changelog/unreleased/bug-fixes/3002-invalid-txs-results.md
	modified:   rpc/core/blocks.go
	modified:   rpc/core/blocks_test.go
	new file:   state/compatibility_test.go

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   state/errors.go
	both modified:   state/state_test.go
	both modified:   state/store.go
	both modified:   state/store_test.go

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@andynog andynog self-assigned this Jul 5, 2024
@andynog
Copy link
Contributor
andynog commented Jul 5, 2024

there are many conflicts in this backport, will manually take care of it

@andynog
Copy link
Contributor
andynog commented Jul 8, 2024

Comments for the reviewers of this backport. I had to manually go through and implement some of the backport changes to adapt them to v0.38.x.

There are some changes from the original PR that needed some "adaptation"

  • There is an error log message that I've added to the BlockResults in the rpc/core/blocks.go as opposed to the one in state/store because the Logger is not available in there. We'd need to include a logger in the StoreOptions that is part of the dbStore so this would need a lot of changes that could even be breaking. Logging the message in rpc is fine because the method from store is only called from there.

  • The compatibility_test tests from the original PR are not really usable in v0.38.x because we don't have the versioned protos in there. Most of the tests in compatibility are related to the versioned protos. But I've kept and adapted some logic so we can at least try to simulate and test the saving of a legacy ABCI response to the store and the loading as a ResponseFinalizeBlock.

@andynog andynog requested a review from sergio-mena July 8, 2024 20:39
melekes
melekes reviewed Jul 9, 2024
View reviewed changes
sergio-mena
sergio-mena approved these changes Jul 9, 2024
View reviewed changes
Copy link
Contributor
@sergio-mena sergio-mena left a comment

Choose a reason for hiding this comment

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

Went through diff-of-diffs. All discrepancies I saw are explained by this. So LGTM

andynog and others added 9 commits July 9, 2024 07:45
@andynog
Merge branch 'v0.38.x' into mergify/bp/v0.38.x/pr-3031
d24f9f0
@andynog @melekes
Update rpc/core/blocks.go
008ac6b 
Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
@andynog @melekes
Update state/compatibility_test.go
e2082b7 
Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
@andynog @melekes
Update state/compatibility_test.go
e8f8f5f 
Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
@andynog @melekes
Update state/compatibility_test.go
f9e7a91 
Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
@andynog @melekes
Update state/compatibility_test.go
d3a36dd 
Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
@andynog @melekes
Update state/compatibility_test.go
2628a04 
Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
@andynog @melekes
Update state/store.go
cbcb0aa 
Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
@andynog
implemented suggestions on PR (#3435)
844aede
@andynog andynog merged commit 0792c8b into v0.38.x Jul 9, 2024
21 checks passed
@andynog andynog deleted the mergify/bp/v0.38.x/pr-3031 branch July 9, 2024 12:00
@andynog andynog mentioned this pull request Jul 9, 2024
Merged
4 tasks
@faddat faddat mentioned this pull request Oct 15, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@melekes melekes melekes left review comments

@andynog andynog andynog left review comments

@sergio-mena sergio-mena sergio-mena approved these changes

Assignees

@andynog andynog

Labels
conflicts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andynog @melekes @sergio-mena
0