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: expose InitChain tx responses #1941

Merged
merged 32 commits into from
Jul 23, 2024

Conversation

n0izn0iz
Copy link
Contributor

@n0izn0iz n0izn0iz commented Apr 17, 2024

Save and allow to fetch genesis txs responses

Before

❯ curl 'http://127.0.0.1:26657/block_results?height=0' -s | head -n 20
{
  "jsonrpc": "2.0",
  "id": "",
  "error": {
    "code": -32603,
    "message": "Internal error",
    "data": "height must be greater than 0"
  }
}

After

❯ curl 'http://127.0.0.1:26657/block_results?height=0' -s | head -n 20
{
  "jsonrpc": "2.0",
  "id": "",
  "result": {
    "height": "0",
    "results": {
      "deliver_tx": [
        {
          "ResponseBase": {
            "Error": null,
            "Data": null,
            "Events": null,
            "Log": "msg:0,success:true,log:,events:[]",
            "Info": ""
          },
          "GasWanted": "50000",
          "GasUsed": "240261825"
        },
        {
          "ResponseBase": {
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.

@github-actions github-actions bot added 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Apr 17, 2024
Copy link

codecov bot commented Apr 17, 2024

Codecov Report

Attention: Patch coverage is 54.54545% with 10 lines in your changes missing coverage. Please review.

Project coverage is 55.01%. Comparing base (4ff6bc7) to head (83dd5a0).

Files Patch % Lines
gno.land/pkg/gnoland/app.go 0.00% 8 Missing ⚠️
tm2/pkg/bft/rpc/core/blocks.go 80.00% 1 Missing ⚠️
tm2/pkg/bft/state/store.go 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1941   +/-   ##
=======================================
  Coverage   55.01%   55.01%           
=======================================
  Files         595      595           
  Lines       79731    79744   +13     
=======================================
+ Hits        43861    43871   +10     
- Misses      32552    32555    +3     
  Partials     3318     3318           
Flag Coverage Δ
contribs/gnodev 25.65% <ø> (-0.35%) ⬇️
contribs/gnofaucet 15.31% <ø> (+0.85%) ⬆️
contribs/gnokeykc 0.00% <ø> (ø)
contribs/gnomd 0.00% <ø> (ø)
gno.land 64.14% <0.00%> (-0.10%) ⬇️
tm2 54.43% <85.71%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

gno.land/pkg/gnoland/app.go Outdated Show resolved Hide resolved
@moul
Copy link
Member

moul commented Apr 21, 2024

Is this common on other blockchains? I have concerns about the size of the response generated. I see advantages in having this new information, but I also think that it could be OK to just consider that "the genesis is there, and we don't need to know how."

@n0izn0iz
Copy link
Contributor Author

n0izn0iz commented Apr 21, 2024

I don't know if it's common in other blockchains but in order to index the portal loop we need to get the genesis transactions results otherwise indexers can't restore their state after a portal loop upgrade since the previous version transactions are embedded in genesis

@moul
Copy link
Member

moul commented Apr 22, 2024

The Portal Loop isn't ideal for indexers. Indexers should focus on networks like test3 and test4. If we can enhance the Portal Loop to benefit other uses, we're open to it. Share how improving it could help beyond just the Portal Loop.

Related with #1791.

@leohhhn
Copy link
Contributor

leohhhn commented Apr 22, 2024

Hey @n0izn0iz, the Portal Loop is a bit specific when it comes to the way it works. It is not fully persistent will the way it handles state, and it can prove difficult to try and index it.

We discussed this with Berty as well - we can either 1. add a way to fetch genesis transactions, 2. think of a different solution to index the Portal Loop.

I'll bring this up on the engineer meeting today.

@n0izn0iz
Copy link
Contributor Author

n0izn0iz commented Apr 22, 2024

I also think it's important for other networks

What happens if you add a package in genesis that creates a grc20 token and want to index a "grc20.created" event, or "users.registered" events or dao membership history (some daos will probably be seeded in genesis, no?)

IMO there is many cases where it's desired to not have to extract this information in two ways (one for post-genesis-state via state inspection and one for regular txs via events) but instead run the same logic for all txs

#all_txs_are_equal 🙅🏽‍♂️

Maybe adding a gnoland subcommand to generate genesis result could be a way?
At least then the indexers could fetch the genesis and generate the results (but they would also need to know the correct chain binary for each chain)

Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

I'm a bit late to the party 🙏

I like this approach, and I think it works fine, but I think we can do even better -- please check out my leftover comments regarding data saves

tm2/pkg/bft/consensus/replay.go Outdated Show resolved Hide resolved
tm2/pkg/bft/consensus/replay.go Outdated Show resolved Hide resolved
@n0izn0iz n0izn0iz force-pushed the expose-genesis-response branch 2 times, most recently from 97733cb to e4dd0f6 Compare June 10, 2024 20:14
@n0izn0iz
Copy link
Contributor Author

sorry for the push --force, I push-forced again to restore history before squash

@n0izn0iz n0izn0iz changed the title feat: expose InitChain response feat: expose InitChain tx responses Jun 10, 2024
@n0izn0iz n0izn0iz marked this pull request as ready for review June 10, 2024 21:10
tm2/pkg/bft/state/store.go Outdated Show resolved Hide resolved
@moul
Copy link
Member

moul commented Jun 19, 2024

What about a server starting with a quick sync snapshot, beginning at block 1M?

@moul
Copy link
Member

moul commented Jun 19, 2024

The Genesis file is approximately 700MB on test3.

@n0izn0iz
Copy link
Contributor Author

n0izn0iz commented Jun 19, 2024

What about a server starting with a quick sync snapshot, beginning at block 1M?

I don't understand the question

The Genesis file is approximately 700MB on test3.

The full genesis goes in the ABCI protocol and it can be fetched from the api currently. I don't understand why we couldn't return and expose the results also

@moul
Copy link
Member

moul commented Jun 24, 2024

What about a server starting with a quick sync snapshot, beginning at block 1M?
I don't understand the question

What happens when someone starts at block 1M instead of block 0? Will your virtual block 1M differ from other nodes that started from an earlier genesis? Could you add a test to expose this case for a chain starting with a sync snapshot?

The Genesis file is approximately 700MB on test3.
The full genesis goes in the ABCI protocol and it can be fetched from the api currently. I don't understand why we couldn't return and expose the results also

I think I don't fully understand the purpose of this PR. Could someone please explain the different elements (genesis, virtual block 0, and upcoming blocks) and how they are affected by this PR, perhaps with a visual schema?

@Kouteki Kouteki requested a review from ajnavarro June 27, 2024 16:27
tm2/pkg/bft/consensus/replay_test.go Outdated Show resolved Hide resolved
tm2/pkg/bft/consensus/replay_test.go Outdated Show resolved Hide resolved
tm2/pkg/bft/consensus/replay_test.go Outdated Show resolved Hide resolved
tm2/pkg/bft/rpc/core/blocks.go Show resolved Hide resolved
tm2/pkg/bft/rpc/core/blocks.go Show resolved Hide resolved
@n0izn0iz
Copy link
Contributor Author

n0izn0iz commented Jul 17, 2024

@moul

What happens when someone starts at block 1M instead of block 0? Will your virtual block 1M differ from other nodes that started from an earlier genesis? Could you add a test to expose this case for a chain starting with a sync snapshot?

How could I start at block 1M? I don't see a StartHeight field in any init types (neither GenesisDoc, nor InitChainRequest)

I think I don't fully understand the purpose of this PR. Could someone please explain the different elements (genesis, virtual block 0, and upcoming blocks) and how they are affected by this PR, perhaps with a visual schema?

The way a tendermint chain is initialized is through the InitChain ABCI endpoint

The in-memory GenesisDoc is passed to this call, in the case of gno (and most tendermint chains), this GenesisDoc contains transactions that are executed during the InitChain call

Currently, the result of these transactions is lost, this is very annoying for indexing as, among other things, you loose events from those transactions.

This PR returns the result of these genesis transactions in the InitChain response.

These results are stored as belonging to block #0 because the transactions in block #1 and subsequent blocks come from the mempool and are executed after the InitChain routine

tm2/pkg/bft/rpc/core/blocks.go Outdated Show resolved Hide resolved
tm2/pkg/bft/abci/types/testing/mock_application.go Outdated Show resolved Hide resolved
tm2/pkg/bft/state/store.go Show resolved Hide resolved
@thehowl thehowl merged commit 3aa0f0b into gnolang:master Jul 23, 2024
82 of 83 checks passed
@n0izn0iz
Copy link
Contributor Author

❤️

@n0izn0iz n0izn0iz deleted the expose-genesis-response branch July 23, 2024 11:42
gfanton pushed a commit to gfanton/gno that referenced this pull request Jul 23, 2024
Save and allow to fetch genesis txs responses

### Before

```
❯ curl 'http://127.0.0.1:26657/block_results?height=0' -s | head -n 20
{
  "jsonrpc": "2.0",
  "id": "",
  "error": {
    "code": -32603,
    "message": "Internal error",
    "data": "height must be greater than 0"
  }
}
```

### After

```
❯ curl 'http://127.0.0.1:26657/block_results?height=0' -s | head -n 20
{
  "jsonrpc": "2.0",
  "id": "",
  "result": {
    "height": "0",
    "results": {
      "deliver_tx": [
        {
          "ResponseBase": {
            "Error": null,
            "Data": null,
            "Events": null,
            "Log": "msg:0,success:true,log:,events:[]",
            "Info": ""
          },
          "GasWanted": "50000",
          "GasUsed": "240261825"
        },
        {
          "ResponseBase": {
```

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

- [ ] 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](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>

---------

Signed-off-by: Norman Meier <[email protected]>
Co-authored-by: Morgan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Functionality that contains breaking changes 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants