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
Collaborator

@mitjat mitjat commented Mar 15, 2023

Resolves https://app.clickup.com/t/8669n7f27, partially resolves https://app.clickup.com/t/8669n7gt3 (for most of consensus types)

This PR:

  • Implements the rest of ConsensusApiLite (introduced in refactor: Initial support for Cobalt nodes #326), an interface that unifies communication with Cobalt and Damask nodes.
  • 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 https://app.clickup.com/t/8669n7gt3; 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 changed the title consensus: Internal types for node responses analyzer/consensus: Full Cobalt support. Internal types for node responses. Mar 15, 2023
storage/oasis/client.go Outdated Show resolved Hide resolved
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
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.

None yet

3 participants