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.

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

fix: invalid txs_results returned for legacy ABCI responses #3031

Merged
merged 61 commits into from
Jul 5, 2024

Conversation

andynog
Copy link
Contributor

@andynog andynog commented May 7, 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

@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
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
state/errors.go Outdated Show resolved Hide resolved
state/store.go Outdated Show resolved Hide resolved
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

Copy link
Collaborator

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

api/api_test.go Outdated Show resolved Hide resolved
rpc/core/blocks.go Show resolved Hide resolved
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
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 <[email protected]>
Co-authored-by: Daniel <[email protected]>
Co-authored-by: Anton Kaliaev <[email protected]>
Co-authored-by: Daniel Wedul <[email protected]>
(cherry picked from commit db6b608)
mergify bot pushed a commit that referenced this pull request 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

- [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 <[email protected]>
Co-authored-by: Daniel <[email protected]>
Co-authored-by: Anton Kaliaev <[email protected]>
Co-authored-by: Daniel Wedul <[email protected]>
(cherry picked from commit db6b608)

# Conflicts:
#	state/errors.go
#	state/state_test.go
#	state/store.go
#	state/store_test.go
andynog added a commit that referenced this pull request Jul 5, 2024
#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 <[email protected]>
andynog added a commit that referenced this pull request Jul 8, 2024
andynog added a commit that referenced this pull request Jul 9, 2024
#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 <[email protected]>
Co-authored-by: Anton Kaliaev <[email protected]>
@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

txs_results not returned properly on dYdX nodes
9 participants