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

fix(taiko_worker): fix a maxBytesPerTxList check issue #282

Merged
merged 3 commits into from
Jul 2, 2024

Conversation

davidtaikocha
Copy link
Member

@davidtaikocha davidtaikocha commented Jul 2, 2024

Also, think there may be an some double counting in your taiko_worker.go code when calculating if the total tx len is within the maxBytesPerTxList limit.

this check:

b, err := encodeAndComporeessTxList(append(env.txs, tx))
appends the current tx length before calling encodeAndComporeessTxList
However, if the transaction was successful it should have already been added to env.txs here:
env.txs = append(env.txs, tx)

You will also run this check even if the tx failed and wasn't added to the tx list, which seems unecassary.

To fix I think you just need to move the maxBytesPerTxList check before committing the tx. Happy to PR this in if I haven't missed anything.

@davidtaikocha davidtaikocha marked this pull request as draft July 2, 2024 05:08
@davidtaikocha davidtaikocha marked this pull request as ready for review July 2, 2024 06:01
@YoGhurt111 YoGhurt111 self-requested a review July 2, 2024 06:13
@davidtaikocha davidtaikocha enabled auto-merge (squash) July 2, 2024 06:13
@davidtaikocha davidtaikocha merged commit f930382 into taiko Jul 2, 2024
2 checks passed
@davidtaikocha davidtaikocha deleted the fix-maxBytesPerTxList branch July 2, 2024 06:21
lwedge99 pushed a commit to sentioxyz/taiko-geth that referenced this pull request Jul 2, 2024
* fix(taiko_worker): fix a `maxBytesPerTxList` check issue

* feat: more changes

* feat: more changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants