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

analyzer/consensus: Full Cobalt support. Internal types for node responses. #356

Merged
merged 13 commits into from
Mar 21, 2023

Conversation

mitjat
Copy link
Contributor

@mitjat mitjat commented Mar 15, 2023

Task 1:

#326 introduces a new CobaltConsensusApiLite struct whose methods wrap the oasis-node API: They speak cobalt-style gRPC to the node, then convert the response to indexer-internal types. Only GetGenesisDocument() is implemented for now; follow that pattern to implement the remaining methods.

Implementation note: For simpler methods, try parsing the node output directly as the indexer-internal (= Damask oasis-core) type, so that no conversion is necessary. Some types are bound to not have changed between Cobalt and Damask.

Task 2:

Data pulled from the node is currently converted into v22.2.5 (= oasis-3-compatible) oasis-core types. When oasis-4 comes out, we have two options:

  • Keep indexer pinned to oasis-core 22.2.5, implement converters from oasis-4 types to oasis-3
  • Introduce indexer's own internal types (heavily based on oasis-core but pared down to only what we need; and stable), convert everything into them.

While the first option will work in a pinch, the second option is clearly cleaner and more sustainable long-term.

Warren mentioned that the indexer already has its own internal types: whatever we put in the db tables. However, those are

  • not true types; we fill rows column by column, and don't have ORM
  • outputs of analysis; inputs to analysis are either oasis-core types, or some thin wrappers that are proprietary to the indexer but make use of oasis-core types in their fields.

So the goal is to have ConsensusApiLite and RuntimeApiLite return types that have no oasis-core dependencies. Everything else flows from there.

This PR:

  • Covers task 1: Implements the rest of ConsensusApiLite (introduced in refactor: Initial support for Cobalt nodes #326), an interface that unifies communication with Cobalt and Damask nodes.
  • Partially covers task 2: Migrates that interface from using the latest oasis-core types to using internal types.

The latter has considerable fallout, and introduces considerable boilerplate :/. That said, it's the best solution I see. Re-posting my Slack reasoning for visibility:

The initial plan was to grab the Cobalt response, convert it to a Damask response, and then reuse our existing analyzer code from there. There are two issues with this:

  • It's more work than it should be. For example, in TransactionsWithResults, which includes all the events, some types are more complex than I expected, and can differ quite a lot between versions. For example, when a runtime registers, the registration includes a ton of parameters (executors, TEE hardware, admission policies for nodes, staking rules, etc), and many of them changed their types between versions. I plowed into converting those types, but 150 lines in, I gave up. They were lines that come slowly, because you have to manually hunt down every field and try to cast it, then failing that, convert manually, carefully comparing the fields. With 150 lines of boilerplate-looking, easy-to-make-mistakes code, I was nowhere near done with the conversion. The kicker is that indexer doesn't really use almost any of those fields. But I cannot just replace them with nil, read on.
  • It loses information. We do use all events at a surface level in that we index them, i.e. we store their raw JSON into the DB (and will probably expose it via the web UI). When converting cobalt events to the damask-event format, some fields are lost, some are made up ... and only then does the analyzer get to see the value and store it into the DB.
  • It is not future-proof because indexer will eventually depend on an even newer oasis-core. We've known this, and we planned for it tentatively in Task 2; that task now got largely folded into this PR.

Testing:

  • I ran 10k rounds of consensus pre- and post-PR, will compare DB dumps. Results:
    • There was a bug in storing the body of staking.allowance_change events. The old analyzer stored the base64-encoded JSON representation, but all other events are just plain JSON. This PR fixes that.
    • For nodes that had no TLS addresses (= they were not listening for TLS connections; I'm not clear on what protocol under TLS), we used to store their addresses as {''}. Now, it's correctly {}. (Curlies are how postgres represents arrays.) That said, I see those addresses are now deprecated anyway :). Same for P2P addresses, which are not deprecated.
    • For nodes, various addresses were incorrectly stored doubly-quoted (= as a string with literal ' at beginning and end); fixed now.
    • No other differences.
  • I ran the consensus analyzer against a Cobalt node (except non-tx registry events, because they're not whitelisted by our current node instance) for a good number of rounds. No errors or crashes. I did not go to the trouble of retrofitting statecheck for Cobalt.

@mitjat mitjat force-pushed the mitjat/cobalt-consensus branch from ebc4080 to c3e4ead Compare March 15, 2023 21:36
@mitjat mitjat changed the title consensus: Internal types for node responses analyzer/consensus: Full Cobalt support. Internal types for node responses. Mar 15, 2023
@mitjat mitjat force-pushed the mitjat/cobalt-consensus branch from c3e4ead to fb9209f Compare March 16, 2023 16:54
storage/oasis/client.go Outdated Show resolved Hide resolved
@mitjat mitjat force-pushed the mitjat/cobalt-consensus branch from fb9209f to 026afd1 Compare March 17, 2023 22:32
@mitjat mitjat requested a review from pro-wh March 17, 2023 22:32
@mitjat mitjat force-pushed the mitjat/cobalt-consensus branch from 026afd1 to 67f980b Compare March 20, 2023 17:39
@mitjat mitjat force-pushed the mitjat/cobalt-consensus branch from 67f980b to d369c98 Compare March 21, 2023 22:54
@mitjat mitjat enabled auto-merge March 21, 2023 22:54
Copy link
Collaborator

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

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

let's try it out with this. haven't read through, but the regression test passing is a good sign

@mitjat mitjat merged commit 7a1e6c6 into main Mar 21, 2023
@mitjat mitjat deleted the mitjat/cobalt-consensus branch March 21, 2023 22:59
@csillag csillag mentioned this pull request Dec 19, 2024
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants