[DataAvailability] Implement new unit tests for execution data backend

[illia-malachyn]

Closes #8059, #7587, #8231, #8135

Changes:

1. No more separate test for a streaming from a spork root block. Any test does it!
2. Result info provider extended with new functionality: it recognizes execution forks now!
3. Removed index from execution data tracker
4. Removed tests for different backfill types as they are not needed.

Streamer streams all available blocks at the start, so either such tests should be there, or they should be removed, since they're basically useless - no matter how many blocks are available, all of them are streamed to the subscription.

5. Introduce a data provider interface instead of a callback.
   Initially, I wanted to do it for testing, so I could easily mock this behavior and write good tests for the backend. However, the problem is that this logic is not injected into the endpoints. Therefore, it is not as useful for the backend tests, but they're useful for the tests in the subscription package (which might be extended/refactored in the [Data Availability] Introduction of a new subscription package #8131). Also, the interface makes it easier to reason about both the backend and subscription packages, as well as their interaction.

Please, review in the following order:
optimistic sync package (changes to result provider + its tests) -> subscription package (changes to streamer and response handler/data provider interface) -> backend -> tests for backend

Note, the tests for events and account statutes MUST fail in this PR, as the result provider and response handler func were significantly changed. We should merge PRs for the events/accounts into this one, then check if CI passes, and only after that we can merge everything into the feature/optimistic_sync

To be done in this PR:

• enhance error handling

[illia-malachyn]

@AndriiDiachuk

[codecov-commenter]

Codecov Report

❌ Patch coverage is 55.94855% with 137 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
engine/access/subscription/mock/data_provider.go	0.00%	23 Missing ⚠️
engine/access/state_stream/mock/api.go	0.00%	16 Missing ⚠️
engine/access/state_stream/backend/backend.go	0.00%	12 Missing ⚠️
engine/access/state_stream/backend/handler.go	41.17%	10 Missing ⚠️
engine/access/subscription/data_provider.go	0.00%	10 Missing ⚠️
...ess/subscription/tracker/execution_data_tracker.go	0.00%	9 Missing ⚠️
.../optimistic_sync/execution_result/info_provider.go	85.48%	7 Missing and 2 partials ⚠️
engine/access/rpc/backend/accounts/accounts.go	0.00%	8 Missing ⚠️
...gine/access/state_stream/backend/backend_events.go	0.00%	8 Missing ⚠️
...ss/state_stream/backend/execution_data_provider.go	88.00%	3 Missing and 3 partials ⚠️
... and 9 more

📢 Thoughts on this report? Let us know!

[illia-malachyn]

@peterargue can u look into that ?

[illia-malachyn]

@peterargue @UlyanaAndrukhiv @AndriiDiachuk, guys, I guess this is ready for review. I'd really appreciate your comments/criticisms. If it feels too clumsy and complex, I can split it up into several smaller PRs.

[UlyanaAndrukhiv]

Overall, this looks good! I’ve left a few suggestions for improvements. I mostly skipped missing godocs for this round. I am also considering a refactoring of ExecutionDataBackend and executionDataProvider, but I’d like to try it first—if it works, I will provide a separate comment afterward.

[peterargue]

thanks for the cleanup. I like your refactor of the provider.

[peterargue]

is this needed? shouldn't this also be returned by b.headers.ByBlockID(startBlockID)?

[illia-malachyn]

IDK actually. This code existed for a long period of time so I'm afraid to remove it. I'm not even sure we need these trackers at all. I've created a task to investigate it and remove them in case we don't #8238

[peterargue]

I don't think "ignorable" is the right framing. how about

Suggested change

	func (s *BackendExecutionDataSuite2) TestExecutionDataProviderIgnorableErrors() {
	func (s *BackendExecutionDataSuite2) TestExecutionDataProviderCriteriaNotMet() {

[illia-malachyn]

TestExecutionDataProviderErrors is also a spectre of errors where a criteria is not met. However, in one case, we stop streaming data, and in another we keep doing it.

[peterargue]

this doesn't mean the fork was abandoned, just that the provided result's fork does not include the parent result.

[illia-malachyn]

Hmm, will sth like that work?

func (p *Provider) findResultAndExecutors(...) (flow.Identifier, flow.IdentifierList, error) {
    // ... (ByBlockID call)

    var (
        hasParentMismatch bool
        lastErr           error
    )

    for executionResultID, executionReceiptList := range allReceiptsForBlock.GroupByResultID() {
        // ...
        if err := p.isExecutorGroupMeetingCriteria(result, receipts, criteria); err != nil {
            if errors.Is(err, optimistic_sync.ErrParentResultMismatch) { // 1. We return a different error
                hasParentMismatch = true
            }
            lastErr = err
            continue
        }
        // ... (collect matchingResults)
    }

    if len(matchingResults) == 0 {
        // If we found results but they all had the wrong parent, 
        // this is where we might eventually conclude the fork we are following is gone.
        if hasParentMismatch {
             return flow.ZeroID, nil, optimistic_sync.ErrForkAbandoned // 2. We state that this is a fork-abandoned error
        }
        
        if lastErr != nil {
            return flow.ZeroID, nil, lastErr
        }
        return flow.ZeroID, nil, optimistic_sync.ErrNotEnoughAgreeingExecutors
    }
    // ...
}

[peterargue]

When we talk about an abandoned result, we mean a result that conflicts with a sealed result for a given block. Given results form a chain, if an ancestor result is abandoned, you can deduce that all of its ancestors must be as well.

Checking if a result conflicts with a sealed result is simple. All you have to do is check if there was a match for the block, and if not, check if the block is sealed (i.e. has a sealed result). I think Uliana added functionality to check this already.

In the case where the result is not yet sealed, but its ancestor is abandoned, you could optimize by flagging the result abandoned early so the client could react sooner. I don't think this is required in the first version.

my preference is to keep this functionality simple and and focused. The job of the results provider is to find the result given the provided criteria. then given a result, the snapshot provider can indicate if the result is abandoned or not. I think in the optimized case, we could do something like check for the next result. if there is no match, check the status of the previous result to see if it is abandoned. I'm not 100% sure if this is the right approach though, so let's leave it for later and just use the simple sealed check for now.

[peterargue]

these errors relate to a specific execution result, but this method queries a set of results for a block. Instead of returning these, I think this method should simply state that no result matched the requested criteria.

looking at the returned errors, it seems there are a few cases:

• The block has not been executed yet (no receipts found)
• The block is before the node's available history
• No results found that matched the criteria (unsealed block)
• No results found that matched the criteria (sealed block)
• The criteria was invalid

[peterargue]

I'm not sure it's useful to return the specific error here. we generally just want to know if this result matched the criteria.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch illia-malachyn/new-execution-data-tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}