[DataAvailability] add a check for executors passed by a client #8233
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
UlyanaAndrukhiv
requested review from
AndriiDiachuk,
illia-malachyn and
peterargue
module/executiondatasync/optimistic_sync/execution_result/info_provider.go
Outdated
Show resolved
Hide resolved
module/executiondatasync/optimistic_sync/execution_result/info_provider.go
Outdated
Show resolved
Hide resolved
module/executiondatasync/optimistic_sync/execution_result/info_provider.go
Outdated
Show resolved
Hide resolved
…m:onflow/flow-go into UlianaAndrukhiv/8204-add-executors-checks
…xecutions by block ID. Updated tests
…ndling for fork-aware endpoints. Added test
…rding to suggestions. Added tests
module/executiondatasync/optimistic_sync/execution_result/info_provider.go
Outdated
Show resolved
Hide resolved
module/executiondatasync/optimistic_sync/execution_result/info_provider.go
Outdated
Show resolved
Hide resolved
| header, err := e.headers.ByBlockID(blockID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get header by block ID %v: %w", blockID, err) | ||
| } | ||
| // Lookup the finalized block ID at the height of the requested block | ||
| blockIDFinalized, err := e.headers.BlockIDByHeight(header.Height) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to lookup finalized block ID at height %d: %w", header.Height, err) | ||
| } | ||
| // If the requested block conflicts with finalized block, return error | ||
| if blockIDFinalized != blockID { | ||
| return nil, optimistic_sync.NewBlockFinalityMismatchError(blockID, blockIDFinalized) | ||
| } |
There was a problem hiding this comment.
I believe the intention is to return a helpful error to the user if they are requesting data for a sealed block, and no ExecutionResults match their criteria. In this case, it's unlikely that their criteria will ever be met. Rather than returning PreconditionFailed indicating that their request may succeed in the future, we could return a more definitive error that their request did not match for a sealed block and probably never will. they may want to relax their criteria or investigate what went wrong.
I don't think I was totally clear in my last comment. what I was saying is you cannot do a height based comparison to check if a block is sealed for non-finalized blocks since there may be consensus forks (multiple blocks with the same height) up until finalization. The most efficient way to check if a block is sealed is:
- get the block's header
- check if it is finalized by:
a. useBlockIDByHeightto check if a block is indexed for the height. If not, it's not finalized.
b. check if the returned block ID matches the requested block ID. If not, it's not finalized. - check if the block's height is
<=the sealed block's height
Step 2 was missing in your original changes. Step 3 is missing in the current changes.
In either case, we only want to do this check and returned the improved error if we couldn't find a result that matched the criteria
There was a problem hiding this comment.
Sorry about that — I had misunderstood the earlier comment. I’ve updated the logic in c26a53e.
Please take a look at the error handling in these changes.
Close: #8204
Context
This PR implements the following checks to
criteriavalidation:required executor IDs(ensure that all executor IDs provided through the criteria actually exist among the known execution nodes).required executor IDscount (ensure that the required executor number does not exceed the actual number ofavailable executors).agreeing executorscount (ensure that the agreeing executors count does not exceed the actual number ofavailable executors).criteriacannot be met.Additional updates: