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/devp2p: fix modulo in makeBlobTxs #28970

Merged
merged 1 commit into from
Feb 15, 2024
Merged

Conversation

colinlyguo
Copy link
Contributor

@colinlyguo colinlyguo commented Feb 11, 2024

Finding a small inconsistency when playing with blob transactions (using this code snippet as a reference). Changing the module from 2 to 3 can make it consistent with the comments, and by the way, it can consume all blobs when calling makeBlobTxs in the tests (first tx with 2 blobs, second with 1 blob):

	var (
		t1 = s.makeBlobTxs(2, 3, 0x1)
		t2 = s.makeBlobTxs(2, 3, 0x2)
	)

Previously, the first tx only had 1 blob, and the second 1, strictly less than 3 blobs bandwidth provided.

@karalabe
Copy link
Member

Hmmm, this code may or may not originate from a time when 0 blobs were allowed. Currently they are not.

That said, this modulo feels so weird. If the user requests N blobs, why cut it down. So strange.

CC @lightclient ?

@lightclient
Copy link
Member

@fjl might be able to better answer, I think this is from him while he was getting the hivechain to work! My guess is the value was supposed to be 3. IUIC, the modulo allows the caller of makeBlobTxs to submit block-level values and max out at 2 blobs per tx. It's sneaky though and not noted in the function docs.

I think this could be written in a clearer way.

@fjl fjl merged commit 2a1d94b into ethereum:master Feb 15, 2024
2 checks passed
@fjl fjl added this to the 1.13.13 milestone Feb 15, 2024
@colinlyguo colinlyguo deleted the fix-test-suite branch February 19, 2024 08:50
jorgemmsilva pushed a commit to iotaledger/go-ethereum that referenced this pull request Jun 17, 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.

4 participants