Skip to content
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

fix: invalid txs_results returned for legacy ABCI responses #3031

Merged
merged 61 commits into from
Jul 5, 2024
Merged

fix: invalid txs_results returned for legacy ABCI responses #3031

merged 61 commits into from
Jul 5, 2024

Conversation

andynog
Copy link
Contributor

@andynog andynog commented May 7, 2024 •
edited
Loading

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

@andynog andynog added bug Something isn't working abci Application blockchain interface rpc labels May 7, 2024
@andynog andynog self-assigned this May 7, 2024
@andynog andynog requested review from a team as code owners May 7, 2024 19:30
melekes
melekes reviewed May 8, 2024
View reviewed changes
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

state/store.go Outdated Show resolved Hide resolved
state/store.go Outdated Show resolved Hide resolved
sergio-mena
sergio-mena reviewed May 8, 2024
View reviewed changes
state/errors.go Outdated Show resolved Hide resolved
sergio-mena
sergio-mena reviewed May 8, 2024
View reviewed changes
state/store.go Outdated Show resolved Hide resolved
sergio-mena
sergio-mena reviewed May 8, 2024
View reviewed changes
state/store.go Outdated Show resolved Hide resolved
@andynog andynog added backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x backport-to-v1.x Tell Mergify to backport the PR to v1.x labels May 8, 2024
@andynog
Copy link
Contributor Author

andynog commented May 10, 2024

I am working on some additional tests to ensure the logic holds and it makes sense

cason
cason reviewed May 10, 2024
View reviewed changes
Copy link
Contributor

@cason cason left a comment

Choose a reason for hiding this comment

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

No reserve against this change.

I would move the added test unit elsewhere thought.

.changelog/unreleased/bug-fixes/3002-invalid-txs-results.md Outdated Show resolved Hide resolved
api/api_test.go Outdated Show resolved Hide resolved
rpc/core/blocks.go Show resolved Hide resolved
sergio-mena
sergio-mena approved these changes Jul 5, 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.

Thanks a lot for this!

@andynog andynog added this pull request to the merge queue Jul 5, 2024
Merged via the queue into main with commit db6b608 Jul 5, 2024
39 checks passed
@andynog andynog deleted the andy/3002-invalid-txs-results branch July 5, 2024 15:54
mergify bot pushed a commit that referenced this pull request Jul 5, 2024
@andynog @mergify
fix: invalid txs_results returned for legacy ABCI responses (#3031)
d800ffc 
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)
mergify bot pushed a commit that referenced this pull request Jul 5, 2024
@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
This was referenced Jul 5, 2024
Merged
Merged
andynog added a commit that referenced this pull request Jul 5, 2024
@mergify @andynog
fix: invalid txs_results returned for legacy ABCI responses (backport
8b262bd 
#3031) (#3434)

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
<hr>This is an automatic backport of pull request #3031 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: Andy Nogueira <me@andynogueira.dev>
andynog added a commit that referenced this pull request Jul 8, 2024
@andynog
Revert "fix: invalid txs_results returned for legacy ABCI responses (…
c3a2aea 
…#3031)"

This reverts commit 1c62d03.
andynog added a commit that referenced this pull request Jul 9, 2024
@mergify @andynog @melekes
fix: invalid txs_results returned for legacy ABCI responses (backport
0792c8b 
#3031) (#3435)

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
<hr>This is an automatic backport of pull request #3031 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: Andy Nogueira <me@andynogueira.dev>
Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
@andynog
Copy link
Contributor Author

andynog commented Jul 9, 2024

Hi @mbreithecker, @mmsqe, @johnletey, @jhernandezb and @SpicyLemon

This fixed has been back ported to v0.38.x (#3435) and it will be available in the next release in the v0.38 line. Thanks for all feedback and testing. Please let us know if you encounter any issues while using the fix.

Thanks again,

Andy.

@mbreithecker
Copy link

Thank you very much!

@ljoss17 ljoss17 mentioned this pull request Jul 22, 2024
Closed
@jasonsong0 jasonsong0 mentioned this pull request Aug 28, 2024
Merged
10 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

@cason cason cason left review comments

@SpicyLemon SpicyLemon SpicyLemon left review comments

@melekes melekes melekes approved these changes

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

Assignees

@andynog andynog

Labels
abci Application blockchain interface backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x backport-to-v1.x Tell Mergify to backport the PR to v1.x bug Something isn't working rpc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

txs_results not returned properly on dYdX nodes
9 participants
@andynog @sergio-mena @mbreithecker @johnletey @jhernandezb @mmsqe @melekes @cason @SpicyLemon