Skip to content

Commit a5a5038

Browse files
fmolettampaulucci
authored andcommitted
fix(l1): validate incoming pooled tx blobs (#4963)
**Motivation** Enable hive p2p tests `TestBlobTxWithMismatchedSidecar ` & `TestBlobTxWithoutSidecar` In order to pass these tests we have to: * Be able to decode pooled transactions without blobs (aka plain eip4844 transactions instead of a wrapped eip4844 with its blobsbundle) * Disconnect from peers that send transactions without blobs or blob bundles that don't match the versioned hashes <!-- Why does this pull request exist? What are its goals? --> **Description** * Handle the case of plain `Eip4844` transaction when RLP-decoding `WrappedEip4844` transactions * Disconnect from peers that send empty/mismatched blobs <!-- A clear and concise general description of the changes this PR introduces --> <!-- Link to issues: Resolves #111, Resolves #222 --> Closes #3745, part of #4941 --------- Co-authored-by: Martin Paulucci <martin.c.paulucci@gmail.com>
1 parent eea14fa commit a5a5038

File tree

5 files changed

+78
-26
lines changed

5 files changed

+78
-26
lines changed

.github/workflows/pr-main_l1.yaml

Lines changed: 22 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -16,27 +16,27 @@ concurrency:
1616

1717
jobs:
1818
detect-changes:
19-
runs-on: ubuntu-latest
20-
outputs:
21-
run_tests: ${{ steps.finish.outputs.run_tests }}
22-
steps:
23-
- uses: actions/checkout@v4
24-
- uses: dorny/paths-filter@v3
25-
id: filter
26-
with:
27-
filters: |
28-
run_tests:
29-
- '!crates/l2/**'
30-
- name: finish
31-
id: finish
32-
run: |
33-
if [[ "${GITHUB_EVENT_NAME}" != "pull_request" ]]; then
34-
echo "run_tests=true" >> "$GITHUB_OUTPUT"
35-
else
36-
echo "run_tests=${{ steps.filter.outputs.run_tests }}" >> "$GITHUB_OUTPUT"
37-
fi
38-
- name: Print result
39-
run: echo "run_tests=${{ steps.finish.outputs.run_tests }}"
19+
runs-on: ubuntu-latest
20+
outputs:
21+
run_tests: ${{ steps.finish.outputs.run_tests }}
22+
steps:
23+
- uses: actions/checkout@v4
24+
- uses: dorny/paths-filter@v3
25+
id: filter
26+
with:
27+
filters: |
28+
run_tests:
29+
- '!crates/l2/**'
30+
- name: finish
31+
id: finish
32+
run: |
33+
if [[ "${GITHUB_EVENT_NAME}" != "pull_request" ]]; then
34+
echo "run_tests=true" >> "$GITHUB_OUTPUT"
35+
else
36+
echo "run_tests=${{ steps.filter.outputs.run_tests }}" >> "$GITHUB_OUTPUT"
37+
fi
38+
- name: Print result
39+
run: echo "run_tests=${{ steps.finish.outputs.run_tests }}"
4040

4141
lint:
4242
# "Lint" is a required check, don't change the name
@@ -187,7 +187,7 @@ jobs:
187187
artifact_prefix: rpc_compat
188188
- name: "Devp2p tests"
189189
simulation: devp2p
190-
limit: discv4|eth|snap/Ping|Amplification|Status|StorageRanges|ByteCodes|GetBlockHeaders|SimultaneousRequests|SameRequestID|ZeroRequestID|GetBlockBodies|MaliciousHandshake|MaliciousStatus|Transaction|NewPooledTxs|GetBlockReceipts|LargeTxRequest|InvalidTxs|BlockRangeUpdate|AccountRange|GetTrieNodes|GetByteCodes|GetStorageRanges|Findnode|BlobViolations
190+
limit: discv4|eth|snap
191191
artifact_prefix: devp2p
192192
- name: "Engine Auth and EC tests"
193193
simulation: ethereum/engine

crates/common/types/blobs_bundle.rs

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,10 @@ impl BlobsBundle {
8080
Self::default()
8181
}
8282

83+
pub fn is_empty(&self) -> bool {
84+
self.blobs.is_empty() && self.commitments.is_empty() && self.proofs.is_empty()
85+
}
86+
8387
// In the future we might want to provide a new method that calculates the commitments and proofs using the following.
8488
#[cfg(feature = "c-kzg")]
8589
pub fn create_from_blobs(
@@ -179,6 +183,23 @@ impl BlobsBundle {
179183

180184
Ok(())
181185
}
186+
187+
pub fn validate_blob_commitment_hashes(
188+
&self,
189+
blob_versioned_hashes: &[H256],
190+
) -> Result<(), BlobsBundleError> {
191+
if self.commitments.len() != blob_versioned_hashes.len() {
192+
return Err(BlobsBundleError::BlobVersionedHashesError);
193+
}
194+
for (commitment, blob_versioned_hash) in
195+
self.commitments.iter().zip(blob_versioned_hashes.iter())
196+
{
197+
if *blob_versioned_hash != kzg_commitment_to_versioned_hash(commitment) {
198+
return Err(BlobsBundleError::BlobVersionedHashesError);
199+
}
200+
}
201+
Ok(())
202+
}
182203
}
183204

184205
impl RLPEncode for BlobsBundle {

crates/common/types/transaction.rs

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,20 @@ impl RLPEncode for WrappedEIP4844Transaction {
133133
impl RLPDecode for WrappedEIP4844Transaction {
134134
fn decode_unfinished(rlp: &[u8]) -> Result<(WrappedEIP4844Transaction, &[u8]), RLPDecodeError> {
135135
let decoder = Decoder::new(rlp)?;
136-
let (tx, decoder) = decoder.decode_field("tx")?;
136+
let Ok((tx, decoder)) = decoder.decode_field("tx") else {
137+
// Handle the case of blobless transaction
138+
let (tx, rest) = EIP4844Transaction::decode_unfinished(rlp)?;
139+
return Ok((
140+
WrappedEIP4844Transaction {
141+
tx,
142+
wrapper_version: None,
143+
// Empty blobs bundles are not valid
144+
blobs_bundle: BlobsBundle::empty(),
145+
},
146+
rest,
147+
));
148+
};
149+
137150
let (wrapper_version, decoder) = decoder.decode_optional_field();
138151
let (blobs, decoder) = decoder.decode_field("blobs")?;
139152
let (commitments, decoder) = decoder.decode_field("commitments")?;

crates/networking/p2p/rlpx/connection/server.rs

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,9 @@ use crate::{
3636
types::Node,
3737
};
3838
use ethrex_blockchain::Blockchain;
39-
use ethrex_common::types::MempoolTransaction;
4039
#[cfg(feature = "l2")]
4140
use ethrex_common::types::Transaction;
41+
use ethrex_common::types::{MempoolTransaction, P2PTransaction};
4242
use ethrex_storage::{Store, error::StoreError};
4343
use ethrex_trie::TrieError;
4444
use futures::{SinkExt as _, Stream, stream::SplitSink};
@@ -1027,8 +1027,26 @@ async fn handle_incoming_message(
10271027
send(state, Message::PooledTransactions(response)).await?;
10281028
}
10291029
Message::PooledTransactions(msg) if peer_supports_eth => {
1030+
// If we receive a blob transaction without blobs or with blobs that don't match the versioned hashes we must disconnect from the peer
1031+
for tx in &msg.pooled_transactions {
1032+
if let P2PTransaction::EIP4844TransactionWithBlobs(itx) = tx
1033+
&& (itx.blobs_bundle.is_empty()
1034+
|| itx
1035+
.blobs_bundle
1036+
.validate_blob_commitment_hashes(&itx.tx.blob_versioned_hashes)
1037+
.is_err())
1038+
{
1039+
warn!(
1040+
peer=%state.node,
1041+
"disconnected from peer. Reason: Invalid/Missing Blobs",
1042+
);
1043+
send_disconnect_message(state, Some(DisconnectReason::SubprotocolError)).await;
1044+
return Err(PeerConnectionError::DisconnectSent(
1045+
DisconnectReason::SubprotocolError,
1046+
));
1047+
}
1048+
}
10301049
if state.blockchain.is_synced() {
1031-
// TODO(#3745): disconnect from peers that send invalid blob sidecars
10321050
if let Some(requested) = state.requested_pooled_txs.get(&msg.id) {
10331051
let fork = state.blockchain.current_fork().await?;
10341052
if let Err(error) = msg.validate_requested(requested, fork).await {

crates/networking/p2p/rlpx/eth/transactions.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ pub struct PooledTransactions {
224224
// id is a u64 chosen by the requesting peer, the responding peer must mirror the value for the response
225225
// https://github.com/ethereum/devp2p/blob/master/caps/eth.md#protocol-messages
226226
pub id: u64,
227-
pooled_transactions: Vec<P2PTransaction>,
227+
pub pooled_transactions: Vec<P2PTransaction>,
228228
}
229229

230230
impl PooledTransactions {

0 commit comments

Comments
 (0)