-
Notifications
You must be signed in to change notification settings - Fork 20.6k
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
eth: check blob transaction validity on the peer goroutine when received #31219
eth: check blob transaction validity on the peer goroutine when received #31219
Conversation
I'm working on test coverage for this in the form of tests in the devp2p eth test suite. |
…dified such that the sidecar commitments don't match the hash in the tx header.
…ise the same hash but containing sidecar should not be disconnected)
Tests passing when run individually. However, running them both (or the entire eth suite) causes them both to fail... |
…hem pass when run with other blob tx tests. temporarily disable another seemingly-unrelated test that when ran in conjunction with invalid sidecar tests causes the latter to fail.
Tests pass when I omit |
So I think I've cracked the core of the failure, and it reveals a rather interesting error: If we connect to a peer, the peer announces/transmits a bad blob tx, causing the client eth handler to return an error and disconnect the peer, the disconnect will not happen while the client is waiting waiting for the peer to read the client's already-pooled txs that it is trying to announce. At least, it seems this is the case. I'm trying to figure out why. |
… bad blob tx tests, when reading eth protocol messages, ensure that we read-and-ignore any pending messages that are unrelated to the message that the test is expecting to proceed.
tbh, I'm actually not sure this is a comprehensive solution: the blobs/proofs could still be modified by a malicious tx-interceptor s.t. the checks introduced here pass, but the tx is ultimately failed to be added to the pool and the tx hash will not be considered for future re-request and inclusion... I kind of wonder if there is any way at all to differentiate between sidecar-tampering-via-mitm and definitively invalid blob tx hash, given that the sender never signs over the full data... Perhaps the correct solution is: if validation of the sidecar commitment integrity wrt I would be inclined to proceed with merging this, or at least merge the component of this that checks |
Although... this PR involves changes to the public blob tx interface. So, perhaps makes sense to get verification of my assumption from someone else before proceeding:
|
This ensures that if we receive a blob transaction where we cannot link the tx header to the sidecar commitments, we will drop the sending peer. This check is added in the protocol handler for the
PooledTransactions
message.closes #31162