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

feat(tm2): store tx results and add endpoint to query them #1546

Merged
merged 12 commits into from
Apr 17, 2024

Conversation

zivkovicmilos
Copy link
Member

@zivkovicmilos zivkovicmilos commented Jan 17, 2024

Description

This PR introduces saving transaction results (execution results) in the node's state DB, and serving them over the node's RPC endpoint (tx).

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@zivkovicmilos zivkovicmilos added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label Jan 17, 2024
@zivkovicmilos zivkovicmilos self-assigned this Jan 17, 2024
Copy link

codecov bot commented Jan 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 47.78%. Comparing base (229bf0e) to head (dd4e54e).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1546      +/-   ##
==========================================
+ Coverage   45.10%   47.78%   +2.67%     
==========================================
  Files         464      393      -71     
  Lines       68039    61608    -6431     
==========================================
- Hits        30689    29437    -1252     
+ Misses      34774    29701    -5073     
+ Partials     2576     2470     -106     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zivkovicmilos zivkovicmilos marked this pull request as ready for review January 18, 2024 13:16
@zivkovicmilos
Copy link
Member Author

@dongwon8247

I've added tx result support for the Gno RPC in this PR, both for regular calls and for clients

@zivkovicmilos
Copy link
Member Author

Ping @ajnavarro @gfanton @thehowl for visibility

Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

nits & q's, lgtm

tm2/pkg/bft/state/store.go Outdated Show resolved Hide resolved
tm2/pkg/bft/state/store.go Outdated Show resolved Hide resolved
tm2/pkg/bft/rpc/core/routes.go Show resolved Hide resolved
tm2/pkg/bft/rpc/core/tx.go Outdated Show resolved Hide resolved
@thehowl thehowl changed the title feat: save & serve tx results feat(tm2): store tx results and add endpoint to query them Feb 1, 2024
Copy link
Contributor

@ajnavarro ajnavarro left a comment

Choose a reason for hiding this comment

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

The changes LGTM, but I have one question:

Transactions and blocks are not stored using the same transaction, so we might have eventually inconsistencies between blocks and transactions stored. Is that a problem?

@zivkovicmilos
Copy link
Member Author

The changes LGTM, but I have one question:

Transactions and blocks are not stored using the same transaction, so we might have eventually inconsistencies between blocks and transactions stored. Is that a problem?

I wish I could give you a better answer to this question, but in order for that to happen I'd need to untangle the mess that is our block building logic, something I'm tackling with @petar-dambovaliev in a separate effort.

We save ABCI results as part of ApplyBlock - these contain information about the transaction execution. I've placed the tx result saving just after it:
https://github.com/zivkovicmilos/gno/blob/d06ba58e5545478273f9a29f84d6c44472ee158d/tm2/pkg/bft/state/execution.go#L109-L124

I'm assuming (hoping, really) that blockExec.db is reverted if any funny business happens. We validate the block before we start hitting the DB with save requests, so this is about as safe as we're gonna get at this point in the codebase

@thehowl
Copy link
Member

thehowl commented Feb 20, 2024

I think after addressing the %x I was talking about, seeing as it's been approved by both me and Antonio, you can go ahead and merge :)

tm2/pkg/bft/state/store.go Outdated Show resolved Hide resolved
tm2/pkg/bft/state/store.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Apr 1, 2024
@zivkovicmilos
Copy link
Member Author

Pinging @ajnavarro and @gfanton for a second screen 🙏

@zivkovicmilos
Copy link
Member Author

zivkovicmilos commented Apr 11, 2024

Update on this PR:

I've spoken internally with @ajnavarro and we agreed a much leaner option would be to store the tx hash -> block number / tx index mapping, rather than tx hash -> tx result, just because it keeps the DB footprint minimal, and we can always reconstruct the result from the state on the fly

I'll update this PR accordingly 🙏

@zivkovicmilos zivkovicmilos marked this pull request as draft April 11, 2024 16:01
@zivkovicmilos
Copy link
Member Author

@ajnavarro @gfanton

I've updated the index:

9371399

Now, instead of

tx hash -> tx result

the mapping is:

tx hash -> (block number, tx index)

and the actual RPC endpoint digs this out of the DB and constructs the response type, so we're avoiding double saves

@zivkovicmilos zivkovicmilos marked this pull request as ready for review April 16, 2024 17:37
Copy link
Member

@gfanton gfanton left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@zivkovicmilos zivkovicmilos merged commit 3d1d26c into gnolang:master Apr 17, 2024
187 of 188 checks passed
@zivkovicmilos zivkovicmilos deleted the feat/save-tx-results branch April 17, 2024 15:57
omarsy pushed a commit to TERITORI/gno that referenced this pull request Apr 21, 2024
)

## Description

This PR introduces saving transaction results (execution results) in the
node's state DB, and serving them over the node's RPC endpoint (`tx`).

<details><summary>Contributors' checklist...</summary>

- [x] Added new tests, or not needed, or not feasible
- [x] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [x] Updated the official documentation or not needed
- [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [x] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants