feat: chain indexer todos [skip changelog]

[akaladarshi]

This PR is part of ChainIndexer tooling PR:

• It addresses the TODO's and comments

[aarshkshah1992]

You need to take a read lock before reading this. See how we do it elsewhere.

[akaladarshi]

Updated.
Added a function isClosed, so we can reuse it.

[aarshkshah1992]

let's return a struct here with these fields instead of returning multiple values here.

[akaladarshi]

Done.

[aarshkshah1992]

This is incorrect. You need to use the tipsetKey.Cid.Bytes path here to get the key to look up the DB.

[akaladarshi]

Done.

[aarshkshah1992]

Please can you implement this TODO to verify events as well ?

[aarshkshah1992]

You can use the loadExecutedMessages function we have for this. Then you can just read messages and events together.

[akaladarshi]

I was little skeptical about events, because we need to do something like currTipset.Height() + 1 to get execution tipset then fetch data for it through loadExecuteMessages.

So should I go ahead with this?

[aarshkshah1992]

@akaladarshi

Hmm..good point.

Yeah let's do it. So, here are the steps:

1. Load tipset tsX at user supplied epoch e
2. Load tipset tsY at epoch e+1
3. Ensure parent of ``tsY==tsX` (if there is a mismatch here, return an error because it means chain has forked and user needs to retry)
4. Load events and messages and validate the counts.

[akaladarshi]

Done.

[aarshkshah1992]

@akaladarshi I think this is incorrect. eventCount will be >0 ONLY if non-reverted events exist. So, we should check for reverted events ONLY if eventCount==0.

[akaladarshi]

Done.

[aarshkshah1992]

@akaladarshi What if the tipset is NOT reverted but the events are reverted ? This query only ensures that the tipset is NOT reverted. Does it have any implications on the correctness of the code /API ?

[akaladarshi]

When you say events are reverted that must mean that it's corresponding message is also reverted.
Shouldn't we mark message inside that tipset reverted as well?

[rvagg]

would it be corruption if one were reverted and the other not? is that something we should also be checking for?

[aarshkshah1992]

OR closed. Not AND, right ?

[aarshkshah1992]

Just pass expectedTs here and use the toTipsetKeyCidBytes function we have to find the database key.

[akaladarshi]

Done.

[aarshkshah1992]

have different if statements for both conditions so the error message can be more granular.

[akaladarshi]

Done.

[aarshkshah1992]

@akaladarshi I think this variable will confuse users more than help them. Let's just get rid of it and simply have a NonRevertedEventsCount value on the IndexValidation struct.

[akaladarshi]

you mean rename the IndexValidation fields?

[aarshkshah1992]

Yes. Let's remove EventsReverted from IndexValidation. We can add it later if we need it.

[akaladarshi]

Then, I think we can also remove the query for checking, if the events were reverted.

if data.nonRevertedEventCount == 0 {
	if err = tx.Stmt(si.stmts.hasRevertedEventsStmt).QueryRowContext(ctx, tsKeyCidBytes).Scan(&data.hasRevertedEvents); err != nil {
		return xerrors.Errorf("failed to check for reverted events: %w", err)
	}
}

[akaladarshi]

Done.

[aarshkshah1992]

Can you add a test for these fields to TestEthGetLogsBasic in eth_filtest_test.go ?

We already call this API there and so this should be easy. See

iv, err := client.ChainValidateIndex(ctx, abi.ChainEpoch(0), false)
	require.NoError(err)
	require.NotNil(iv)

	fmt.Printf("index validation: %v\n", iv)

	iv, err = client.ChainValidateIndex(ctx, abi.ChainEpoch(22), false)
	require.NoError(err)
	require.NotNil(iv)
	fmt.Printf("index validation: %v\n", iv)

[akaladarshi]

The test is failing right now because loadExecutedMessage is not loading any messages, and we are using it to verify the indexed data.

[aarshkshah1992]

You don't need to use a transaction here as you are only reading from the DB.

[akaladarshi]

I was using transaction so I get the consistent view of DB for querying.

[aarshkshah1992]

ensure indexedData is not nil before proceeding here.

[akaladarshi]

Ideally, it will only be nil if error is there and if there is error and indexedData is not nil then that means issue is in DB, so we should return.

[aarshkshah1992]

Yes but no harm in checking it and returning an error of it is nil here so user's node dosen't crash if there exists a bug that causes a nil value here.

[akaladarshi]

Done.

[aarshkshah1992]

You don't need to call MessagesForTipset here and compare number of messages since you are calling loadExecutedMessages below which gives you both messages and events, right ? You can just compare then.

[akaladarshi]

I was about to ask this as well.

So here we are getting inclusionMessage and below we are getting executedMsg, I just wanted to confirm both are same (ideally they should be)

[aarshkshah1992]

Look at what the loadExecutedMessages in it's implementation. Reading the implementation code will give you a better understanding of how it all works together.

[akaladarshi]

Just checked, you're correct it's the same thing, I will update.

[akaladarshi]

Done.

[rvagg]

double lock here because you do it in isClosed too

[rvagg]

how about you just return a mostly-filled IndexValidation from this that can be built on or merged from here? alternatively just 2 ints since that's all its doing

[rvagg]

ah I see why you want a struct for this, but it's still just 2 ints and it might even be clearer to provide them as named arguments here

[rvagg]

see discussion in #12447 (comment), same problem here I think, particularly as you get close to the tip of the chain

[aarshkshah1992]

@rvagg Which is why we error out here if the parent of this tipset is not the original tipset. We can then ask the user to retry as that means that the chain has forked between the two tipsets.

[aarshkshah1992]

I don't think we want to re-execute the tipset/regenerate events here.

[aarshkshah1992]

Merging this and continuing all work as part of the parent PR. Will address all review comments there.

[aarshkshah1992]

Thanks for all your work here @akaladarshi 👍