eth/fetcher: fix blob transaction propagation #30125
Conversation
roberto-bayardo
force-pushed
the
order-announcements
branch
8 times, most recently
from
56e386e
to
a232129
Compare
roberto-bayardo
requested review from
karalabe,
holiman and
rjl493456442
as code owners
roberto-bayardo
force-pushed
the
order-announcements
branch
3 times, most recently
from
1d13749
to
be7157c
Compare
roberto-bayardo
force-pushed
the
order-announcements
branch
from
be7157c
to
ee2b77b
Compare
eth/fetcher/tx_fetcher.go
Outdated
| // blob transactions are never broadcast, so to force them | ||
| // to be fetched immediately we pretend they arrived | ||
| // earlier. | ||
| f.waittime[hash] = f.clock.Now() - mclock.AbsTime(txArriveTimeout) |
There was a problem hiding this comment.
Manipulating the time feels wrong. If we want to blob txs to skip the waitlist all together, we should just do that instead of tricking the mechanism to shuffle them out faster than other txs.
There was a problem hiding this comment.
That was what I implemented in the earlier PR, but @karalabe suggested doing it like this (if I understood him correctly): #30118 (comment)
LMK if there's a strong preference either way.
There was a problem hiding this comment.
@lightclient @karalabe added one more commit which replaces time-travel with explicit skipping of the waitlist if you want to compare: bbdb624
(Note that the test case changed because it resulted in a different peer iteration order, not because the behavior is significantly different -- the only real change to the test case is no need to do a trivial wait for the blob tx to go to feteching state)
roberto-bayardo
force-pushed
the
order-announcements
branch
2 times, most recently
from
88cef9b
to
3ff3634
Compare
|
Pinging to keep this alive... anything else I can provide? I'm still running this PR on my home node, so far so good! |
roberto-bayardo
force-pushed
the
order-announcements
branch
2 times, most recently
from
efcd01c
to
b956778
Compare
cmd/devp2p/internal/ethtest/suite.go
Outdated
| @@ -849,7 +849,16 @@ func (s *Suite) TestBlobViolations(t *utesting.T) { | |||
| if code, _, err := conn.Read(); err != nil { | |||
| t.Fatalf("expected disconnect on blob violation, got err: %v", err) | |||
| } else if code != discMsg { | |||
| t.Fatalf("expected disconnect on blob violation, got msg code: %d", code) | |||
| if code == 24 { | |||
There was a problem hiding this comment.
| if code == 24 { | |
| if code == protoOffset(ethProto)+eth.NewPooledTransactionHashesMsg { |
a bit better imo than using raw code
eth/fetcher/tx_fetcher.go
Outdated
| if sizes[i] == 0 { | ||
| // invalid size parameter, return error | ||
| return fmt.Errorf("announcement from tx %x had an invalid 0 size metadata", hash) | ||
| } |
There was a problem hiding this comment.
I don't think we should special case the size check here. The same way 0 is bad, so is 1 and 2 and probably a few more. We also don't really expect getting sent 0 unless it's a faulty remote client (i.e. a malicious can send 1). Also 0 will fail post-validation after we retrieve the hash, just as say 1 would. Cleaner just to not care about it at this point.
roberto-bayardo
force-pushed
the
order-announcements
branch
from
b956778
to
f83f5a1
Compare
|
We're not 100% sure it's the best API change, but also we're not entirely sure we have a strongly better alternative, so I'll try out a couple variations and will possibly end up pushing a commit on top. |
roberto-bayardo
force-pushed
the
order-announcements
branch
2 times, most recently
from
97a0d28
to
25591c2
Compare
|
Hi! I just made a polish commit locally rjl493456442#13 We will discuss the modification today internally and will push the changes to your branch if |
…minimize nonce-gaps (which cause blob txs to be rejected) - don't wait on fetching blob transactions after announcement is received, since they are not broadcast
Signed-off-by: Roberto Bayardo <[email protected]>
rjl493456442
force-pushed
the
order-announcements
branch
from
25591c2
to
54336c2
Compare
|
Any updates on this change? We're still seeing this issue bite us, but luckily only Sepolia though. We've had to switch Base sepolia tesnet entirely back to calldata because we can't get blobs to keep up with the chain. |
|
We're planning on getting this merged next week, that's what the |
This PR fixes an issue with blob transaction propagation due to the blob transation txpool rejecting transactions with gapped nonces. The specific changes are:
fetch transactions from a peer in the order they were announced to minimize nonce-gaps (which cause blob txs to be rejected
don't wait on fetching blob transactions after announcement is received, since they are not broadcast
Testing: