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

eth/fetcher: fix fetcher timeout #28220

Merged
merged 8 commits into from
Oct 17, 2023

Conversation

MariusVanDerWijden
Copy link
Member

This PR adds a test and fixes a corresponding bug in the tx fetcher

@fjl
Copy link
Contributor

fjl commented Sep 29, 2023

Damn, sneaky. Please change the constant type to time.Duration, it will make it easier to reason about.

@fjl fjl closed this Sep 29, 2023
@fjl fjl reopened this Sep 29, 2023
@fjl
Copy link
Contributor

fjl commented Sep 29, 2023

(sorry wrong button)

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

I pushed a commit removing some remaining int64->time.Time coercion. LGTM now, ltgy?

@holiman holiman added this to the 1.13.3 milestone Oct 3, 2023
@holiman
Copy link
Contributor

holiman commented Oct 3, 2023

ok  	github.com/ethereum/go-ethereum/eth/downloader	113.460s
?   	github.com/ethereum/go-ethereum/eth/ethconfig	[no test files]
--- FAIL: TestTransactionForgotten (0.00s)
    tx_fetcher_test.go:1563: transaction should be evicted by this point
FAIL
FAIL	github.com/ethereum/go-ethereum/eth/fetcher	43.465s

Appveyor/windows -- the new test is too much reliant on a non-sucky cpu.

@holiman holiman modified the milestones: 1.13.3, 1.13.4 Oct 13, 2023
@MariusVanDerWijden
Copy link
Member Author

Fixed the test, I think your change slightly changed the semantic from < to <= which broke the test

@holiman
Copy link
Contributor

holiman commented Oct 16, 2023

eth/fetcher/tx_fetcher_test.go:2008:3: not enough arguments in call to NewTxFetcher

Maybe needs a rebase or something?

Copy link
Member Author

@MariusVanDerWijden MariusVanDerWijden left a comment

Choose a reason for hiding this comment

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

LGTM

@holiman holiman merged commit 667966c into ethereum:master Oct 17, 2023
2 checks passed
@anewt95
Copy link

anewt95 commented Oct 18, 2023

devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
This changes fixes a bug in the fetcher, where the timeout for how long to remember underpriced transaction was erroneously compared, and the timeout never hit.
---------

Co-authored-by: Martin Holst Swende <martin@swende.se>
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
atenjin pushed a commit to alt-research/go-ethereum that referenced this pull request Apr 4, 2024
This changes fixes a bug in the fetcher, where the timeout for how long to remember underpriced transaction was erroneously compared, and the timeout never hit.
---------

Co-authored-by: Martin Holst Swende <martin@swende.se>
atenjin pushed a commit to alt-research/go-ethereum that referenced this pull request Apr 4, 2024
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.

5 participants