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

don't broadcast blob txs per eip-4844 #77

Merged
merged 1 commit into from
Jan 19, 2023
Merged

Conversation

roberto-bayardo
Copy link
Collaborator

No description provided.

@Inphi
Copy link
Collaborator

Inphi commented Dec 7, 2022

Can we hold off on merging this? For devnet-v3, we want to broadcast blob txs as usual. So I prefer this gets merged after devnet-v3 as development is targeting that right now.

@roberto-bayardo
Copy link
Collaborator Author

sure we can do that but not sure what the harm is in merging it now unless there are other ELs that we don't expect to implement this part of the spec?

@roberto-bayardo roberto-bayardo force-pushed the roberto-dev branch 2 times, most recently from aa4e23c to 6653a6e Compare December 7, 2022 05:01
@roberto-bayardo
Copy link
Collaborator Author

sure we can do that but not sure what the harm is in merging it now unless there are other ELs that we don't expect to implement this part of the spec?

Oh I see that @timbeiko had it marked as a PR to skip in his notes... I don't recall why we decided not to include it?

@Inphi
Copy link
Collaborator

Inphi commented Dec 7, 2022

iirc, the expectation was that blocking tx announces meant that client should also implement eth/68. Implementing eth/68 is non-trivial. Since it's not essential for devnet-v3, we didn't want to unnecessarily increase the scope of the devnet.

But you're right, it shouldn't hurt to merge just this bit as the blob txs are still announced.

@roberto-bayardo
Copy link
Collaborator Author

Agree we don't want to do eth/68 for devnet. Let me convert this to draft for now while we think it through.

@roberto-bayardo roberto-bayardo marked this pull request as draft December 7, 2022 14:49
@@ -524,6 +524,10 @@ func handleTransactions(backend Backend, msg Decoder, peer *Peer) error {
if tx == nil {
return fmt.Errorf("%w: transaction %d is nil", errDecode, i)
}
if tx.Tx.Type() == types.BlobTxType {
// blob txs should never be broadcast
return fmt.Errorf("transaction %v is a blob tx", i)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If a peer does broadcast txs (say a Nethermind client) and we're part of their inner peerset, this error will occur on both ends of the connection. I'm a bit concerned about how clients handle this failure case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah just seeing your comment on making this a draft. 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have commented this out and left a TODO while we're still interop testing just in case not everybody gets it implemented in time for devnet 4.

@roberto-bayardo roberto-bayardo force-pushed the roberto-dev branch 2 times, most recently from 65b43c0 to 6d4d7b7 Compare January 19, 2023 00:12
@roberto-bayardo roberto-bayardo marked this pull request as ready for review January 19, 2023 00:13
@roberto-bayardo
Copy link
Collaborator Author

Reopening since we want this in devnet-4.

@roberto-bayardo roberto-bayardo merged commit db177b8 into eip-4844 Jan 19, 2023
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