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

Indexing error on subgraph #15

Closed
yabirgb opened this issue Oct 13, 2021 · 33 comments · Fixed by #16 · May be fixed by #17
Closed

Indexing error on subgraph #15

yabirgb opened this issue Oct 13, 2021 · 33 comments · Fixed by #16 · May be fixed by #17

Comments

@yabirgb
Copy link

yabirgb commented Oct 13, 2021

Hello! The subgraph had an indexing error https://thegraph.com/hosted-service/subgraph/graphprotocol/compound-v2 and was reported to us (@rotki). Unfortunatly we don't have more information that what is provided in the response

{
  "errors": [
    {
      "message": "indexing_error",
      "locations": [
        {
          "line": 2,
          "column": 2
        }
      ],
      "path": [
        "mintEvents"
      ],
      "extensions": {
        "code": "INTERNAL_SERVER_ERROR",
        "exception": {
          "errors": [
            {
              "message": "indexing_error",
              "locations": [],
              "path": [
                "mintEvents"
              ]
            }
          ]
        }
      }
    }
  ],
  "data": null
}
@pranavdaa
Copy link

Hi @yabirgb this subgraph is failed to sync because of some issues in the mapping file of the subgraph.
You can generate the error yourself using this command -
curl --location --request POST 'https://api.thegraph.com/index-node/graphql' --data-raw '{"query":"{ indexingStatusForCurrentVersion(subgraphName: \"graphprotocol/compound-v2\") { subgraph synced fatalError { message } nonFatalErrors {message } } }"}'

If you want to use this data of compound subgraph either you can submit a pr in the compound subgraph code or sync a new one for yourself addressing the mapping issues

@LefterisJP
Copy link

Hi @yabirgb this subgraph is failed to sync because of some issues in the mapping file of the subgraph. You can generate the error yourself using this command - curl --location --request POST 'https://api.thegraph.com/index-node/graphql' --data-raw '{"query":"{ indexingStatusForCurrentVersion(subgraphName: \"graphprotocol/compound-v2\") { subgraph synced fatalError { message } nonFatalErrors {message } } }"}'

If you want to use this data of compound subgraph either you can submit a pr in the compound subgraph code or sync a new one for yourself addressing the mapping issues

The point of making this issue is to ask for help and need someone to fix this. If this is not the right repository for this can you please point us to the right repo?

@Jannis
Copy link

Jannis commented Oct 13, 2021

Hey @LefterisJP. I'm not sure if https://github.com/graphprotocol/compound-v2-subgraph is the latest version or not but I believe that our team is on the hook for maintaining it at the moment.

The failure was:

{
  "data": {
    "indexingStatusForCurrentVersion": {
      "fatalError": {
        "message": "Mapping aborted at ~lib/@graphprotocol/graph-ts/chain/ethereum.ts, line 439, column 6, with message: Call reverted, probably because an `assert` or `require` in the contract failed, consider using `try_getUnderlyingPrice` to handle this in the mapping.\twasm backtrace:\t    0: 0x2117 - <unknown>!<wasm function 123>\t    1: 0x255e - <unknown>!<wasm function 154>\t    2: 0x25c1 - <unknown>!<wasm function 157>\t    3: 0x2800 - <unknown>!<wasm function 165>\t    4: 0x2a85 - <unknown>!<wasm function 168>\t in handler `handleTransfer` at block #12961232 (0307e807de7a33a8e5483b357ed5df988fe364cb924b1d18189425db09a31c9d)"
      },
      "nonFatalErrors": [],
      "subgraph": "Qmdpkd8yvD4XR3mQMZQmY5nqBQtjEac8gb8RoFFcWan7xE",
      "synced": true
    }
  }
}

which suggests that a getUnderlyingPrice call reverted unexpectedly. It would be easy to use try_getUnderlyingPrice instead and catch the revert but my question would be if that could lead to inaccurate data later.

@Jannis
Copy link

Jannis commented Oct 13, 2021

There are three places in https://github.com/graphprotocol/compound-v2-subgraph/blob/master/src/mappings/markets.ts where getUnderlyingPrice is called. Those would have to be made robust. I also see that @davekaj suggested the oracle code isn't particularly clean (#11).

@Jannis
Copy link

Jannis commented Oct 13, 2021

Looking at markets.ts, I wonder if they introduced a new oracle recently that doesn't have the getUnderlyingPrice function?

@Jannis
Copy link

Jannis commented Oct 13, 2021

We'll try to rewind the subgraph a bit to see if it runs into this again. If not, this would hint at a non-deterministic response from an Ethereum eth_call request. @LefterisJP I'll let you know when we know more!

@yabirgb
Copy link
Author

yabirgb commented Oct 13, 2021

@Jannis Thank you for taking a look into the issue and the information provided :)

@davekaj
Copy link

davekaj commented Oct 13, 2021

Yeah in #10 they updated the oracle, and broke the subgraph. They have the ability to do this whenever.

Looks like the block it failed at was on August 4th, and the governance did pass a proposal that day https://compound.finance/governance/proposals/54 . This could have caused the failure.

@LefterisJP
Copy link

Hello @Jannis and @davekaj thank you so much for looking at this. Really appreciate.

I don't think the subgraph can have been broken at #10 since this was from 08/2020, more than a year ago, and this error literally occured last night or yesterday at the latest.

This subgraph is used by almost all of our users, including myself so we would have noticed if things were broken before. I understand the responsibility for fixing/addressing the issues may be either with your or compound. This is afaik the only compound subgraph and it's now broken for all of our users and I guess also every other app that queries historical compound data. So fixing it is really important.

How can we help push for a fix to the subgraph and re-indexing? Do you need any data from us? Should we ping someone else?

@juanmardefago
Copy link

@LefterisJP the subgraph was broken on 08/2020, and #10 was the fix for that occurrence (since they changed the way the oracle handled underlying usd values if I recall correctly).

What I think @davekaj means is that what happened this time around might be similar to what happened back in 08/2020 that required a fix on the subgraph.

I'll take a deeper look at the issue in a bit!

@davekaj
Copy link

davekaj commented Oct 13, 2021

Yes that is what I meant thanks for clarifying Juan :)

@LefterisJP
Copy link

Thank you @juanmardefago for the clarification. 🙏

Thank you guys. We will monitor this issue.

@schmidsi
Copy link
Member

Not sure if this helps, but TokenTerminal released their own version of a Compound subgraph: https://thegraph.com/explorer/subgraph?id=0x8d67bc0151eaf16ea16a95ca2f13fd80be183d31-0&view=Overview

@schmidsi
Copy link
Member

@LefterisJP
Copy link

Hello no the token terminal subgraph does not index the historical data we need. This subgraph is what we were relying on for compound historical data.

Is there any chance this will get looked at soonish? We have stopped providing compound historical data since this happened and would really appreciate a solution.

@juanmardefago
Copy link

Hey there @LefterisJP!

I tried to find the root cause in my spare time (currently super busy unfortunately), but couldn't yet find exactly what caused the issue. I'll take a look again as soon as possible.

The easiest fix would be to use the try_ methods, although that could lead to small inconsistencies at some particular block heights (given that a revert is what caused the subgraph to fail in the first place). I could try to fix it that way until I get to the bottom of the issue, unfortunately it's not a straightforward one :(

@juanmardefago
Copy link

@LefterisJP I'll create a PR with the "naive" possible fix (basic fix for the error message), but without real understanding on the root cause, it might not work. (but at least it's better to have some sort of fix syncing I guess).

It does take a long time to sync (1-3 days I think?), so I'll keep an eye on it

@LefterisJP
Copy link

Thank you @juanmardefago. Really appreciate it.

Isn't there also someone from the compound team or original authors we can ping for help/confirmation?

@juanmardefago
Copy link

I think as long as it doesn't crash again at the same block, it's probably gonna be a working fix, although as you mentioned, we'll have to look closer and ideally find the root cause with help from the compound team.

@juanmardefago
Copy link

It failed again on a personal deployment, but on an earlier block, with a different error altogether, on code that didn't change.

I'll have to dig deeper 🤔

@juanmardefago
Copy link

Error log in case anyone's interested:

{
   "data":{
      "indexingStatusForPendingVersion":{
         "fatalError":{
            "block":{
               "number":"12949662"
            },
            "message":"Mapping aborted at ~lib/@graphprotocol/graph-ts/chain/ethereum.ts, line 439, column 6, with message: Call reverted, probably because an `assert` or `require` in the contract failed, consider using `try_name` to handle this in the mapping.\twasm backtrace:\t    0: 0x1298 - <unknown>!<wasm function 50>\t    1: 0x1747 - <unknown>!<wasm function 73>\t    2: 0x1902 - <unknown>!<wasm function 76>\t in handler `handleMarketListed` at block #12949662 (48d26f38a2e1bb8708980b06ec3585108798d80787db5f76e6c526aab2b9cde0)"
         },
         "nonFatalErrors":[
            
         ],
         "subgraph":"QmfKmZ7xfT9PRydVGJRRWEdr7rbBcRoKbDyX7fGK7MCq5j"
      }
   }
}

@LefterisJP
Copy link

oh man. That sucks.

Thank you @juanmardefago for the time you are putting in this.

Is it possible to ask for help from compound original authors and or devs to help with debugging this?

@juanmardefago
Copy link

@LefterisJP Weirdly enough, before I could sit down and try to fix it again, I re checked and it was synced.

Here you can check it for yourself, not sure what went wrong, maybe an issue with the underlying node made it break, and a restart fixed it?

The link is a dev subgraph I use for multiple test runs of different subgraphs I code, but I'll leave it alone for a while with the compound subgraph. We could potentially deploy the same code to the official subgraph name so that the fix works for you. (It's also good to remember that the official subgraph also has some JS wrappers on the top level, that allow for some aggregation, which my dev subgraph won't have, so don't expect everything to work on that dev subgraph, but at least it's synced somehow.)

@LefterisJP
Copy link

@LefterisJP Weirdly enough, before I could sit down and try to fix it again, I re checked and it was synced.

Here you can check it for yourself, not sure what went wrong, maybe an issue with the underlying node made it break, and a restart fixed it?

The link is a dev subgraph I use for multiple test runs of different subgraphs I code, but I'll leave it alone for a while with the compound subgraph. We could potentially deploy the same code to the official subgraph name so that the fix works for you. (It's also good to remember that the official subgraph also has some JS wrappers on the top level, that allow for some aggregation, which my dev subgraph won't have, so don't expect everything to work on that dev subgraph, but at least it's synced somehow.)

Hey Juan. I tested it with our tests and with a simple compound account I have and it seems to work fine. So (probably) for us it should work.

Who could help determine if this is a good enough solution to switch it over to the official subgraph? I will go for testing in our bugfixes branch with your subgraph and tell you if we see anything off: rotki/rotki@6f84782

@juanmardefago
Copy link

Well, the solution itself only really does a sort of "try-catch" to the contract calls used for the price oracle, so it doesn't really change anything with the real logic, the only thing that could be changed is whether to default to zero or one in case of catching an error on the contract call (both would be actual invalid values used just to avoid crashing the subgraph for a one-off contract call error).

If that solves the issue and works for you, I'd probably just double check whether defaulting to zero might cause issues (maybe it's used to divide something at some point in the code?), if so I'd just default to one and port it over to the official subgraph.

@yabirgb
Copy link
Author

yabirgb commented Oct 27, 2021

Hola @juanmardefago! We are going to make a release and are evaluating to use your dev subgraph until the official one gets fixed. Are you planning to keep it working at least until the fix reaches the official subgraph? It would help us to decide what to do for the next release.

Thank you again for your work taking this issue, has been really helpful :)

@juanmardefago
Copy link

Hey @yabirgb, I can deploy it to a more properly named subgraph, since I use those dev subgraphs all the time, and I don't want to mistakenly break your UI.

Once it's deployed to the official release you can revert to using the official one. Does that sound good?

@juanmardefago
Copy link

@yabirgb Here you can find the same exact code deployed, but to a subgraph that I won't be touching at all: https://thegraph.com/hosted-service/subgraph/juanmardefago/compound-v2

@LefterisJP
Copy link

Hey @juanmardefago thanks a lot! And if this is okay and we don't see any problem we can probably have you or someone switch the official subgraph to this too as the current version is broken.

@juanmardefago
Copy link

@LefterisJP sure, I don't really have permissions to merge this, but I think @Jannis might be able to. (Also we need to restart the js wrappers, in case you merge this @Jannis)

cwang25 pushed a commit to cwang25/compound-v2-subgraph that referenced this issue Nov 11, 2021
…ymbol to try_getUnderlyingPrice, try_name, try_symbol...etc
@juanmardefago
Copy link

@LefterisJP the official subgraph has been redeployed with the version I had on my development subgraph, it's now live and working again.

@yabirgb
Copy link
Author

yabirgb commented Nov 15, 2021

@juanmardefago Thank you very much for your support and help :)

@juanmardefago
Copy link

@yabirgb You're welcome! I'm just glad I could help get this sorted out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants