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

core/txpool/legacypool: fix flaky test TestAllowedTxSize #30975

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

qdm12
Copy link

@qdm12 qdm12 commented Dec 31, 2024

  • it was failing because the maximum data length (previously dataSize) was set to txMaxSize - 213 but should had been txMaxSize - 103 and the last call dataSize+1+uint64(rand.Intn(10*txMaxSize))) would sometimes fail depending on rand.Intn.
  • Maximal transaction data size comment (invalid) replaced by code logic to find the maximum tx length without its data length
  • comments and variable naming improved for clarity
  • 3rd pool add test replaced to add just 1 above the maximum length, which is important to ensure the logic is correct

…ally

- it was failing because the maximum data length (previously `dataSize`) was set to `txMaxSize - 213` but should had been `txMaxSize - 103` and the last call `dataSize+1+uint64(rand.Intn(10*txMaxSize)))` would sometimes fail depending on rand.Intn.
- Maximal transaction data size comment (invalid) replaced by code logic to find the maximum tx length without its data length
- comments and variable naming improved for clarity
- 3rd pool add test replaced to add just 1 above the maximum length, which is important to ensure the logic is correct
Copy link
Member

@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

@fjl fjl changed the title fix(core/txpool/legacypool): fix flaky TestAllowedTxSize core/txpool/legacypool: fix flaky test TestAllowedTxSize Jan 3, 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.

2 participants