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

Support for pre-Damask runtime rounds #362

Merged
merged 19 commits into from
Mar 30, 2023
Merged

Support for pre-Damask runtime rounds #362

merged 19 commits into from
Mar 30, 2023

Conversation

mitjat
Copy link
Collaborator

@mitjat mitjat commented Mar 23, 2023

This PR brings support for indexing "old" runtime blocks. Changes:

  • Refactor: Extracts all runtime-related gRPC methods into a new RuntimeApiLite class. This mirrors the existing ConsensusApiLite in scope: provides 1:1 mapping to a subset of RPCs, responses are encoded in indexer-internal types. (Those types often contain fields from the latest oasis-core or oasis-sdk, so we're not fully disconnected from them, but it's a step in the right direction.)
  • Tweaks GetBlock() so it supports old rounds. This is actually a Cobalt vs Damask issue, because we ask technically ask consensus (specifically, its roothash app) for the runtime block info. And its API has changed going from Cobalt to Damask.
  • Reimplements event parsing from oasis-sdk (and uses this reimplementation) such that each SDK Event CBOR is parsed as either a) a list of events or b) a single event. Structure (a) is used for most rounds; structure (b) is used for a few 1000 earliest rounds of Emerald :)

Testing:

  • Indexed emerald from blocks 1 to 114_297, and also at recent heights. No crashes/errors.
  • reindex_and_run.sh for 100 rounds in Damask.

Base automatically changed from mitjat/statecheck-no-internal-apis to main March 24, 2023 21:56
@mitjat mitjat force-pushed the mitjat/cobalt-runtime branch 3 times, most recently from 17e0ed1 to c80694f Compare March 24, 2023 23:46
analyzer/runtime/extract.go Outdated Show resolved Hide resolved
@mitjat mitjat marked this pull request as draft March 28, 2023 00:23
@mitjat mitjat marked this pull request as ready for review March 28, 2023 21:46
Copy link
Collaborator

@Andrew7234 Andrew7234 left a comment

Choose a reason for hiding this comment

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

Looks good; mostly minor comments. The runtime block processing looks cleaner now, thanks

storage/oasis/runtime.go Show resolved Hide resolved
analyzer/consensus/consensus.go Show resolved Hide resolved
Comment on lines -6 to +9
Erc20Transfer = "erc20.transfer"
Erc20Approval = "erc20.approval"
Erc20Transfer = "Transfer"
Erc20Approval = "Approval"
Copy link
Collaborator

Choose a reason for hiding this comment

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

was there a specific reason for this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah what did these erc20. prefixes used to do?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm pretty sure we just accidentally mis-named these when introducing them. We use these as event names for ERC-20 tokens, and the Transfer and Approval names are prescribed by ERC-20. We don't read these names from the ABI currently, but we will, and if we did now, these are the names that would come out of there.

Not related to the rest of the PR, just a drive-by bugfix. It doesn't affect anything, other than how we tag ERC-20 events in our output. @lukaw3d @csillag @buberdds FYI - these are the new values you'll see in evm_log_name once this change deploys. IDK if you depend on the old values in any way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

not clear that having the erc20. prefix was wrong. this system of evm log "names" is completely our own invention. removing the prefix actually seems a little lossy. I just looked, and we use these values as the evm_log_name column. that looks like it's our substitute for the topic id, which the event name doesn't fully convey (it also hashes the parameter types). with the "erc20." we had at least a fighting chance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Log/event names are not our invention. The topic of the event is derived from the name and the types of its params, see e.g. "Topics in Ethereum Log Records" here, or how the event API/ABI defines the name here.
We do expose the topic (which is what we'll eventually want to support filtering on, presumably), but it's a little hidden, in the untyped JSON body field. Example from indexer DB:

runtime           | emerald
round             | 2784
tx_index          | 0
tx_hash           | e415d067861d27a50331bc1ba66097af6cc034b53d1c379c07be858815cc778c
type              | evm.log
body              | {"address":"oXu9ULEcodbSP5Ca5AxBNNghrF4=","topics":["3fJSrRviyJtpwrBo/DeNqpUrp/FjxKEWKPVaTfUjs+8=","AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=","AAAAAAAAAAAAAAAAisMZWuyjmKrHiCUg3RnTx8XmnkY="],"data":"AAAAAAAAAAAAAAAAAAAAAAAAAAAAUrfS3MgM0uQAAAA="}
evm_log_name      | Transfer
evm_log_signature | 3fJSrRviyJtpwrBo/DeNqpUrp/FjxKEWKPVaTfUjs+8=
evm_log_params    | [{"name": "from", "value": "0x0000000000000000000000000000000000000000", "evm_type": "address"}, {"name": "to", "value": "0x8ac3195aeca398aac7882520dd19d3c7c5e69e46", "evm_type": "address"}, {"name": "value", "value": "100000000000000000000000000", "evm_type": "uint256"}]
related_accounts  | {oasis1qqm0rpeswnu8a5p0rphkppg2zulwrg57zskpxp0j,oasis1qqctk3c8a2ualcwufcxn337hchdflyjurgdmqt3e}

I'm open to eventually surfacing the topic(s) (or maybe just the first one, and only for non-anonymous events? i.e. the topic that represents the event signature/type) in a dedicated field.

Copy link
Collaborator

Choose a reason for hiding this comment

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

that's a different "name," originating from solidity code. EVM logs don't have a name, only topic. as the page describes, solidity contracts in particular derive the topic from a name and parameter types.

We do expose the topic (which is what we'll eventually want to support filtering on, presumably)

oh in body, great. hadn't noticed that

Comment on lines +20 to +21
// Implementation of `RuntimeApiLite` that supports all versions of the node ABI
// and all versions of the oasis-sdk ABI. (SDK events are CBOR-encoded according
Copy link
Collaborator

Choose a reason for hiding this comment

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

just curious - there's just a single node ABI used here right? But the resulting grpc.RawMessage is then decoded into one of two possible sdk ABIs? (cobalt/damask)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm perhaps splitting hairs here. All I wanted to say was that this struct works with all versions of everything. It speaks two node ABIs (cobalt and damask, although technically those are not ABI version names, and ABI changes just happen to coincide with launching a new dump-restore chain). See GetBlockHeader() to see two versions being supported; for the other calls, i.e. GetTransactionsWithResults() and GetEventsRaw() and SimulateCall(), damask and cobalt use the same ABI and so we just pretend everything is damask and use the damask SDK.

By SDK decoding and ABIs, I meant how events are encoded. Those are decoded with support for all versions in https://github.com/oasisprotocol/oasis-indexer/blob/90968f1819dcf486c74df323d52b48f081a7ebfb/analyzer/runtime/decode_events.go#L14-L14; note the use of unmarshalSingleOrArray. I think all of this decoding should really happen in nodeapi so that nodeapi can present a unified internal type to the indexer, but I didn't want to refactor more than I had to in this PR.

continue
}
// Iterate over all networks and find the one that contains the runtime.
// Any network will do; we assume that paratime IDs are unique across networks.
Copy link
Collaborator

Choose a reason for hiding this comment

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

paratime IDs are chosen by the entity that registers them

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oooh! A potential for some malarkey then. Very good to know, thanks.
I just left a TODO/note of this; once we sort out how to merge this with your #352, this hacky approach will hopefully not be needed any more.

Actually, we'll need to tweak/expand DefaultChains from #352 a little so that they include some runtime info. The existing DefaultNetworks from oasis-sdk assign a paratime to a specific chaincontext, but really with dump-restores they can be assigned to a "metachain", i.e. to all of the chaincontexts that comprise(d) mainnet.
We could do that inference programmatically, but I think it will be much more readable to just copy paste a li'l more info over into DefaultChains and then use it here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah actually your PR intentionally limits its scope to consensus only. Added a task to https://github.com/oasisprotocol/oasis-indexer/pull/352/files#r1152431146 where I previously started discussing follow-ups.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, and that DefaultNetworks lookup also gets complicated by the custom chains used for testing etc.

@@ -2,9 +2,11 @@ package types

import "fmt"

// Hardcoded event names emitted by ERC-20 contracts.
// TODO: Remove these once we are able to query the ABI of the contract.
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh we have the ABI checked in now. it's in the analyzer directory tho. evmabi.ERC20

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, we could extract them from the ABI already. It's just that it's only one ABI that we have, so it was easier to hardcode than to implement "properly", the way we'll have to once we support arbitrary contracts/ABIs/events. I reworded the TODO. I'm not implementing it here though if that's what you meant; it's completely out of scope for the PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just looked at the abi structures, and it's all Events map[string]Event, so we wouldn't be able to get it from a constant anyway

// unmarshalSingleOrArray tries to interpret `data` as an array of `T`.
// Failing that, it interprets it as a single `T`, and returns a slice
// of size 1 containing that `T`.
func unmarshalSingleOrArray[T any](data []byte, dst *[]T) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

wow our first generic code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

being dragged kicking and screaming into the Century of the Fruitbat :)
(obscure reference, move along)

// Inaccurate: Treat as not using any gas.
// TODO: Decode the tx; if it failed with "out of gas", assume it used all the gas.
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we should treat transactions that came before the GasUsed event as always using all the gas unless the authentication failed

Copy link
Collaborator

Choose a reason for hiding this comment

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

eh let's discuss that outside of this pr

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Possibly. Or we could do what I described in the comment, or something even more complex, like your suggestion from Slack:

in that case we can do
auth failed -> no gas used
insufficient balance (outside evm) -> no gas used
most things within evm -> all gas used

Or we could leave things as-is, which I argued for in slack (but didn't update this TODO afterwards):

I tried charging full gas_limit to all txs, and the fallout took me a while to figure out ...

It turns out that some failed txs have a huge gas_limit; for example, I hit one with MAX_INT64. When you add other txs' gas limits to it, the block gas limit overflows int64 (and could easily overflow uint64). So that blows up in other places that expect total block gas to not exceed uint64; the db even uses int64. [GitHub edit: Maybe we'd avoid this with the full three-pronged approach above, but I have no impulse to play around more; see also below.]

Long story short, I'm leaving failed txs with no GasUsed event as-is, meaning we estimate them to have used 0 gas.
I also like this product-wise. If the gas we show is going to be a little wrong (and it always will be, no matter how we refine our heuristic), it should at least be easy to explain on the rare occasion when we have to acknowledge the discrepancy.

So -- removing the TODO.

@mitjat mitjat enabled auto-merge March 30, 2023 20:12
@mitjat mitjat merged commit 341df91 into main Mar 30, 2023
@mitjat mitjat deleted the mitjat/cobalt-runtime branch March 30, 2023 20:37
@@ -45,24 +48,6 @@ func (rc *RuntimeClient) EVMSimulateCall(ctx context.Context, round uint64, gasP
return rc.nodeApi.EVMSimulateCall(ctx, round, gasPrice, gasLimit, caller, address, value, data)
}

// Name returns the name of the client, for the RuntimeSourceStorage interface.
func (rc *RuntimeClient) Name() string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

😈 goodbye name

blockData.Size += blockTransactionData.Size
}
return &blockData, nil
}

func extractEvents(blockData *BlockData, relatedAccountAddresses map[apiTypes.Address]bool, eventsRaw []*nodeapi.RuntimeEvent) (*extractEventResult, error) {
func sumGasUsed(events []*EventData) (sum uint64, foundGasUsedEvent bool) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

there aren't really multiple though, are there?

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.

reviewed

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