eth/fetcher: add metadata validation in tx announcement - #33378
Merged
Conversation
| t.Fatalf("peering failed: %v", err) | ||
| } | ||
|
|
||
| ann := eth.NewPooledTransactionHashesPacket{Types: []byte{0x09}, Sizes: []uint32{uint32(tx.Size())}, Hashes: []common.Hash{tx.Hash()}} |
Contributor
Author
There was a problem hiding this comment.
Yes, however, I’m considering moving this test somewhere else. Since the hive test uses our test suite, we should check the behavior of other clients first. And this case doesn’t seem to be specified in the original proposal. Still thinking about other possible locations …
Member
There was a problem hiding this comment.
Probably a unit test in the eth/tx_fetcher
Contributor
Author
There was a problem hiding this comment.
I moved it there. It does not fully test the behavior, but I guess it is acceptable
| t.Fatalf("peering failed: %v", err) | ||
| } | ||
|
|
||
| ann := eth.NewPooledTransactionHashesPacket{Types: []byte{0x09}, Sizes: []uint32{uint32(tx.Size())}, Hashes: []common.Hash{tx.Hash()}} |
Member
There was a problem hiding this comment.
You can just use random hash and size for announcement. It should be rejected regardless. It's not necessary to build a valid tx
|
|
||
| conn.SetReadDeadline(time.Now().Add(timeout)) | ||
| msg, err := conn.ReadEth() | ||
| if err != nil && !errors.Is(err, os.ErrDeadlineExceeded) { |
Member
There was a problem hiding this comment.
The expectation here is just the timeout right? The peer shouldn't respond us any message?
Contributor
Author
There was a problem hiding this comment.
Yes, but since multiple test cases may have already run before this, NewPooledTransactionHashesPacket and TransactionsPacket can still arrive. The same approach is used in TestNewPooledTxs. (This test has been removed anyway fyi)
rjl493456442
approved these changes
Dec 11, 2025
weiihann
pushed a commit
to weiihann/go-ethereum
that referenced
this pull request
Jan 14, 2026
This PR fixes the bug reported in ethereum#33365. The impact of the bug is not catastrophic. After a transaction is ultimately fetched, validation and propagation will be performed based on the fetched body, and any response with a mismatched type is treated as a protocol violation. An attacker could only waste the limited portion of victim’s bandwidth at most. However, the reasons for submitting this PR are as follows 1. Fetching a transaction announced with an arbitrary type is a weird behavior. 2. It aligns with efforts such as EIP-8077 and ethereum#33119 to make the fetcher smarter and reduce bandwidth waste. Regarding the `FilterType` function, it could potentially be implemented by modifying the Filter function's parameter itself, but I wasn’t sure whether changing that function is acceptable, so I left it as is.
weiihann
pushed a commit
to weiihann/go-ethereum
that referenced
this pull request
Jan 16, 2026
This PR fixes the bug reported in ethereum#33365. The impact of the bug is not catastrophic. After a transaction is ultimately fetched, validation and propagation will be performed based on the fetched body, and any response with a mismatched type is treated as a protocol violation. An attacker could only waste the limited portion of victim’s bandwidth at most. However, the reasons for submitting this PR are as follows 1. Fetching a transaction announced with an arbitrary type is a weird behavior. 2. It aligns with efforts such as EIP-8077 and ethereum#33119 to make the fetcher smarter and reduce bandwidth waste. Regarding the `FilterType` function, it could potentially be implemented by modifying the Filter function's parameter itself, but I wasn’t sure whether changing that function is acceptable, so I left it as is.
gballet
pushed a commit
to BZO95/go-ethereum
that referenced
this pull request
May 21, 2026
This PR fixes the bug reported in ethereum#33365. The impact of the bug is not catastrophic. After a transaction is ultimately fetched, validation and propagation will be performed based on the fetched body, and any response with a mismatched type is treated as a protocol violation. An attacker could only waste the limited portion of victim’s bandwidth at most. However, the reasons for submitting this PR are as follows 1. Fetching a transaction announced with an arbitrary type is a weird behavior. 2. It aligns with efforts such as EIP-8077 and ethereum#33119 to make the fetcher smarter and reduce bandwidth waste. Regarding the `FilterType` function, it could potentially be implemented by modifying the Filter function's parameter itself, but I wasn’t sure whether changing that function is acceptable, so I left it as is.
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes the bug reported in #33365.
The impact of the bug is not catastrophic. After a transaction is ultimately fetched, validation and propagation will be performed based on the fetched body, and any response with a mismatched type is treated as a protocol violation. An attacker could only waste the limited portion of victim’s bandwidth at most.
However, the reasons for submitting this PR are as follows
Regarding the
FilterTypefunction, it could potentially be implemented by modifying the Filter function's parameteer itself, but I wasn’t sure whether changing that function is acceptable, so I left it as is.