feat: check eth events indexed in range #12728
Conversation
chain/index/events.go
Outdated
| startCid, err := si.getTipsetKeyCidByHeight(ctx, f.MinHeight) | ||
| if err != nil { | ||
| if errors.Is(err, ErrNotFound) { | ||
| // Null round for the start of the range is acceptable | ||
| return nil | ||
| } | ||
| return xerrors.Errorf("failed to get tipset key cid for start height: %w", err) | ||
| } |
There was a problem hiding this comment.
In the ErrNotFound case, I'd like it to increment by 1 until it finds one <= MaxHeight that's not a null round.
In the special case where there are only null rounds between MinHeight and MaxHeight, pretend that it's indexed and don't return an error, they'll just get no results.
So you'll need a loop here, and a local copy of f.MinHeight to increment.
chain/index/events.go
Outdated
| } | ||
|
|
||
| if exists, err := si.isTipsetIndexed(ctx, startCid); err != nil || !exists { | ||
| return xerrors.Errorf("start tipset not indexed: %w", err) |
There was a problem hiding this comment.
| return xerrors.Errorf("start tipset not indexed: %w", err) | |
| return xerrors.Errorf("filter start tipset is not indexed: %w", err) |
chain/index/events.go
Outdated
| return xerrors.Errorf("start tipset not indexed: %w", err) | ||
| } | ||
| if exists, err := si.isTipsetIndexed(ctx, endCid); err != nil || !exists { | ||
| return xerrors.Errorf("end tipset not indexed: %w", err) |
There was a problem hiding this comment.
| return xerrors.Errorf("end tipset not indexed: %w", err) | |
| return xerrors.Errorf("filter end tipset is not indexed: %w", err) |
chain/index/events.go
Outdated
| if head == nil || f.MaxHeight > head.Height() { | ||
| return xerrors.Errorf("range end is in the future: %w", err) | ||
| } | ||
| return ErrNotFound // End is unindexed |
There was a problem hiding this comment.
We're back in the null round case, I think that we should walk backward to find the first non-null lower than MaxHeight. Basically the same as the approach outlined above for MinHeight but in the other direction. You may find a way to abstract the algorithm to make it a reusable function, or it may be easier to just write it all out.
Basically: let's contract the range if the edges contain null rounds, walk inward until we find a min and max that are not null rounds. Be careful not to iterate beyond the other bound (don't increment from Min beyond the Max value, and don't decrement from Max beyond the Min value). The special case is when there are only null rounds between them, don't error, just return. There's another special case of contracting to min==max, you could easily do an if block with startCid.Equals(endCid) around the second isTipsetIndexed call to deal with that (although it's not critical since it'd just mean two queries for the same thing if it wasn't handled).
|
@akaladarshi : I assume you'll re-request review when it's ready. |
|
|
||
| return xerrors.Errorf("failed to get tipset key cid by height: %w", err) | ||
| // Find the first non-null round in the range | ||
| startCid, err := si.findFirstNonNullRound(ctx, &minHeight, maxHeight) |
There was a problem hiding this comment.
The use of the pointer to update it here is clever, but a little too non-obvious from a maintenance perspective, there's no obvious notice here it gets altered. I think you'd be better off returning a new minHeight along with the cid. You'll have to var startCid, endCid cid.Cid and not use := for these calls though.
There was a problem hiding this comment.
[]byte sorry, not cid.Cid
which is another point - not being a CID, let's be careful with naming of these things, startKeyBytes perhaps?
| // If all rounds are null, consider the range valid | ||
| if endCid == nil { |
There was a problem hiding this comment.
oo, interesting case, you should never get here, right, since we do the walk for startCid first? If you can't come up with a plausible way to get here then I think we should error here and say something about this being "unexpected" (we want someone to report this error if they ever stumble upon it)
| func (si *SqliteIndexer) checkTipsetByKeyCid(ctx context.Context, tipsetKeyCid []byte, height abi.ChainEpoch) error { | ||
| exists, err := si.isTipsetIndexed(ctx, tipsetKeyCid) | ||
| if err != nil { | ||
| return xerrors.Errorf("failed to check if tipset at height %d is indexed: %w", height, err) | ||
| } | ||
|
|
||
| if exists { | ||
| return nil // null round | ||
| } | ||
|
|
||
| return ErrNotFound // tipset is not indexed | ||
| } |
There was a problem hiding this comment.
I'm not really sure the height is necessary here, but the main thing I want to do is clarify what this is for. How about we rename this one checkTipsetIndexedStatus since you've dropped it form its parent
| func (si *SqliteIndexer) checkTipsetByKeyCid(ctx context.Context, tipsetKeyCid []byte, height abi.ChainEpoch) error { | |
| exists, err := si.isTipsetIndexed(ctx, tipsetKeyCid) | |
| if err != nil { | |
| return xerrors.Errorf("failed to check if tipset at height %d is indexed: %w", height, err) | |
| } | |
| if exists { | |
| return nil // null round | |
| } | |
| return ErrNotFound // tipset is not indexed | |
| } | |
| func (si *SqliteIndexer) checkTipsetIndexedStatus(ctx context.Context, tipsetKeyCid []byte, height abi.ChainEpoch) error { | |
| exists, err := si.isTipsetIndexed(ctx, tipsetKeyCid) | |
| if err != nil { | |
| return xerrors.Errorf("failed to check if tipset at height %d is indexed: %w", height, err) | |
| } else if exists { | |
| return nil // has been indexed | |
| } | |
| return ErrNotFound | |
| } |
| switch { | ||
| case f.TipsetCid != cid.Undef: | ||
| tipsetKeyCid = f.TipsetCid.Bytes() | ||
| case f.MinHeight >= 0 && f.MinHeight == f.MaxHeight: | ||
| tipsetKeyCid, err = si.getTipsetKeyCidByHeight(ctx, f.MinHeight) | ||
| if err != nil { | ||
| if err == ErrNotFound { | ||
| // this means that this is a null round and there exist no events for this epoch | ||
| return nil | ||
| } |
There was a problem hiding this comment.
you've lost all of this logic in the process of refactoring, so we have a couple of problems now:
- We're not testing the case where the filter specifies a specific
TipsetCid- which we can do directly without needing to look at the range (case 1 above) - We're not taking the shortcut path for the case where the filter specifies a min==max, that one is easy because it's just one tipset to test (case 2 above)
- Your new code just jumps straight in and assumes that
f.MinHeightandf.MaxHeightare useful, but that may not be the case. They're allowed to have-1values, which signifies that neither have been specified (we do that in). Which is why theLine 1844 in 12d76bd
>= 0check existed in the original case.
So I think you'll need to restore some form of the original switch to accommodate those two cases above and then for the third case where f.MinHeight >= 0 && f.MaxHeight >= 0, and maybe && f.MinHeight < f.MaxHeight just for good measure, just to be defensive about possible inputs.
| cid, err := si.getTipsetKeyCidByHeight(ctx, height) | ||
| if err == nil { | ||
| *minHeight = height // Update the minHeight to the found height | ||
| return cid, nil | ||
| } | ||
| if !errors.Is(err, ErrNotFound) { | ||
| return nil, xerrors.Errorf("failed to get tipset key cid for height %d: %w", height, err) | ||
| } |
There was a problem hiding this comment.
Let's switch this for readability, since programmers are used to seeing errors handled first (but not always, just the majority of the time), we introduce a cognitive hiccup when we switch things around; but if we keep it close to the expected flow then it's easier to skim
| cid, err := si.getTipsetKeyCidByHeight(ctx, height) | |
| if err == nil { | |
| *minHeight = height // Update the minHeight to the found height | |
| return cid, nil | |
| } | |
| if !errors.Is(err, ErrNotFound) { | |
| return nil, xerrors.Errorf("failed to get tipset key cid for height %d: %w", height, err) | |
| } | |
| cid, err := si.getTipsetKeyCidByHeight(ctx, height) | |
| if err != nil { | |
| if !errors.Is(err, ErrNotFound) { | |
| return nil, xerrors.Errorf("failed to get tipset key cid for height %d: %w", height, err) | |
| } | |
| // else null round, keep searching | |
| continue | |
| } | |
| *minHeight = height // Update the minHeight to the found height | |
| return cid, nil |
|
Good so far, nice to be checking those bounds in a sophisticated way now. See notes inline; some minor suggestions and one larger one about the missing functionality checking all the variations of a filter that got lost in the refactor. |
|
@akaladarshi : do you think you'll be able to incorporate feedback by EOD 2024-12-18? Not required, but I think it would be nice to get merged before folks start disappearing for winter break. |
Hey @BigLep, I am OOO this week will be back next week. |
Related Issues
#12709
Proposed Changes