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

cmd/workload: fixed filter test request error handling #31424

Merged
merged 3 commits into from
Mar 20, 2025

Conversation

zsfelfoldi
Copy link
Contributor

This PR fixes the broken request error handling of the workload filter tests. Until now validateHistoryPruneErr was invoked with fq.Err as an input which was always nil and a timeout or http error was reported as a result content mismatch.
Also, in case of errPrunedHistory it is wrong to return here without setting an error because then it will look like a valid empty result and the check will later fail. So instead errPrunedHistory is always returned now (without printing an error message) and the callers of run should handle this special case (typically ignore silently).

@jwasinger jwasinger self-assigned this Mar 19, 2025
continue
}
processed = append(processed, qt)
if len(processed)%50 == 0 {
fmt.Println(" processed:", len(processed), "remaining", len(queries), "failed:", failed, "result mismatch:", mismatch)
fmt.Println(" processed:", len(processed), "remaining", len(queries), "failed:", failed, "pruned:", pruned, "result mismatch:", mismatch)
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, we should replace Println calls with logging statements throughout this utility. But it's not really a blocker for this specific PR.

@@ -206,14 +212,10 @@ func (fq *filterQuery) run(client *client, historyPruneBlock *uint64) {
Addresses: fq.Address,
Topics: fq.Topics,
})
if err != nil {
if err = validateHistoryPruneErr(fq.Err, uint64(fq.FromBlock), historyPruneBlock); err == errPrunedHistory {
Copy link
Contributor

Choose a reason for hiding this comment

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

crap. big oversight on my part.. whoops

Copy link
Contributor

Choose a reason for hiding this comment

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

It was correct at some point. I must have inadvertently broken it before my original PR was merged.

jwasinger
jwasinger previously approved these changes Mar 19, 2025
@@ -71,6 +71,9 @@ func filterGenCmd(ctx *cli.Context) error {
f.updateFinalizedBlock()
query := f.newQuery()
query.run(f.client, nil)
if query.Err == errPrunedHistory {
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should error here. It's not really possible to generate meaningful tests on a pruned node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it so that the generator exits on errors instead of collecting them into a separate file. It also fails on history pruning errors. Now I think it doesn't make sense to collect errors into a file during generation as the test vectors should be generated in a correct environment. On the other hand it does make sense to collect errors during the performance test so I added it there (the --errors flag was there in filterperf anyways but it didn't do anything until now).

@fjl fjl changed the title cmd/workload: fixed filter workload test request error handling cmd/workload: fixed filter test request error handling Mar 20, 2025
@fjl fjl merged commit 03cc294 into ethereum:master Mar 20, 2025
3 of 4 checks passed
@fjl fjl added this to the 1.15.6 milestone Mar 20, 2025
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.

3 participants