fix(alloy-eips): `SimpleCoder::decode_one()` should return `Ok(None)` #1818

StackOverflowExcept1on · 2024-12-22T12:56:17Z

Motivation

Resolves #1388

For example you want to decode 4096 FEs. In this case the first time decode_one() will return data, but the second time (because of loop) it will return Err(()) and it will fail the whole decode_all().

alloy/crates/eips/src/eip4844/builder.rs

Lines 246 to 259 in 24ab11e

    
           fn decode_all(&mut self, blobs: &[Blob]) -> Option<Vec<Vec<u8>>> { 
        
               let mut fes = 
        
                   blobs.iter().flat_map(|blob| blob.chunks(32).map(WholeFe::new)).map(Option::unwrap); 
        
               let mut res = Vec::new(); 
        
               loop { 
        
                   match Self::decode_one(&mut fes) { 
        
                       Ok(Some(data)) => res.push(data), 
        
                       Ok(None) => break, 
        
                       Err(()) => return None, 
        
                   } 
        
               } 
        
               Some(res) 
        
           }

Solution

Returns Ok(None) if there is no data (empty iterator, length prefix is 0).

PR Checklist

Added Tests
Added Documentation
Breaking changes

StackOverflowExcept1on · 2024-12-22T14:13:41Z

crates/eips/src/eip4844/builder.rs

@@ -434,6 +436,17 @@ mod tests {
        assert_eq!(decoded, data);
    }

+    #[test]
+    fn big_ingestion_strategy() {
+        let data = vec![1u8; 126_945];


Blob has size of 128 * 1024 bytes. First 32 bytes are used as length, thus leaving 4095 FEs / 4096 FEs. This constant was calculated as 128 * 1024 - 32 - 4095 (4095 zeros in each FE).

…e_one()`

StackOverflowExcept1on · 2024-12-22T15:23:21Z

crates/eips/src/eip4844/builder.rs

+        if blobs
+            .iter()
+            .flat_map(|blob| blob.chunks(FIELD_ELEMENT_BYTES).map(WholeFe::new))
+            .any(|fe| fe.is_none())
+        {
+            return None;
+        }
+
+        let mut fes = blobs
+            .iter()
+            .flat_map(|blob| blob.chunks(FIELD_ELEMENT_BYTES).map(WholeFe::new_unchecked));


Previously it didn't work correctly because .map(WholeFe::new)).map(Option::unwrap) was executed lazily (when decode_one() reached the wrong WholeFe, it would panic).

I decided to check this in a non-lazy way to do not pass Option<Option<Option<Option<WholeFe>> to decode_one().

crates/eips/src/eip4844/mod.rs

mattsse

tysm for looking into this,

all of this looks fine to me, although not an expert when it comes to the internals, but these changes match with what I discovered when I looked into the issue earlier

some nits

crates/eips/src/eip4844/mod.rs

mattsse · 2024-12-23T18:07:20Z

crates/eips/src/eip4844/utils.rs

no functional changes here, right?

I could leave it u64, but then I'd have to make as usize in all usages, which isn't very convenient. Maybe it makes sense to introduce FIELD_ELEMENT_BYTES_USIZE: usize = FIELD_ELEMENT_BYTES as usize? Yes, there are no other functional changes.

mattsse · 2024-12-23T18:07:54Z

crates/eips/src/eip4844/builder.rs

+        let last_unused_blob_index = self.fe / FIELD_ELEMENTS_PER_BLOB as usize;
+        self.blobs.get_mut(last_unused_blob_index).expect("never empty")


this makes sense

mattsse · 2024-12-23T18:09:14Z

crates/eips/src/eip4844/builder.rs

+        let Some(first) = fes.next() else {
+            return Ok(None);
+        };


this also makes sense, because before this would always return an error for iters len > 1, right?

this returned an error when there was no more data left in the iterator. but that's a little different from what we want from decode_all().

mattsse · 2024-12-23T18:11:24Z

crates/eips/src/eip4844/builder.rs

+        if !matches!(blobs.len(), 1..=MAX_BLOBS_PER_BLOCK) {
+            return None;
+        }


this restriction will be problematic soon, because this constant should no longer be used, so I'd rather remove this check entirely

what's the problem? if the number of blobs is increased, we just update the constants and everything will work.

the max number of blobs will be decoupled from this constant ethereum/EIPs#9129

@klkvr should we use the eip7840::BlobParams::cancun() for now or remove this entirely, I guess the proper way of doing this would be to pass around the max_block setting but this seems a bit much...
ref https://github.com/alloy-rs/alloy/pull/1828/files

yeah don't think we need this check

custom implementations can store the constant in coder if needed

Ok, I'll remove the use of MAX_BLOBS_PER_BLOCK in this place and here for now:

alloy/crates/eips/src/eip4844/builder.rs

Lines 206 to 209 in 8ab278c

// if there are too many bytes

if num_bytes > BYTES_PER_BLOB * MAX_BLOBS_PER_BLOCK {

return Err(());

}

We could then do a check that blobs: &[Blob] is not empty and that num_bytes does not exceed 2 MiB (it's limit of 16 blobs against the current 6 blobs and is quite normal practice to defend against attacks of allocating too much memory).

…ES as usize`

mattsse · 2024-12-24T13:18:24Z

crates/eips/src/eip4844/builder.rs

+        if blobs
+            .iter()
+            .flat_map(|blob| blob.chunks(FIELD_ELEMENT_BYTES_USIZE).map(WholeFe::new))
+            .any(|fe| fe.is_none())
+        {
+            return None;
+        }
+
+        let mut fes = blobs
+            .iter()
+            .flat_map(|blob| blob.chunks(FIELD_ELEMENT_BYTES_USIZE).map(WholeFe::new_unchecked));


yeah as pointed out, this overhead should be fine

StackOverflowExcept1on requested review from DaniPopes, gakonst, mattsse and onbjerg as code owners December 22, 2024 12:56

fix(alloy-eips): SimpleCoder::decode_one() should return Ok(None)

7baf239

StackOverflowExcept1on force-pushed the fix-simple-coder branch from c89713b to 7baf239 Compare December 22, 2024 13:40

StackOverflowExcept1on commented Dec 22, 2024

View reviewed changes

refactor(alloy-eips): add more data validation to `SimpleCoder::decod…

07ee47d

…e_one()`

StackOverflowExcept1on commented Dec 22, 2024

View reviewed changes

add more tests

cf74dc0

StackOverflowExcept1on commented Dec 22, 2024

View reviewed changes

crates/eips/src/eip4844/mod.rs Show resolved Hide resolved

StackOverflowExcept1on added 4 commits December 23, 2024 19:36

also fix incorrect encoding

6722552

Merge remote-tracking branch 'origin/main' into fix-simple-coder

5ec7c63

wtf with CI

fa7275f

allocate 1 FE in ingest_partial_fe if needed

72be5cc

mattsse requested changes Dec 23, 2024

View reviewed changes

StackOverflowExcept1on added 4 commits December 23, 2024 21:40

introduce `const FIELD_ELEMENT_BYTES_USIZE: usize = FIELD_ELEMENT_BYT…

dbaa6ef

…ES as usize`

Merge remote-tracking branch 'origin/main' into fix-simple-coder

eb099cf

resolve conversation about MAX_BLOBS_PER_BLOCK usage

d540d81

Merge remote-tracking branch 'origin/main' into fix-simple-coder

e6f4c8a

StackOverflowExcept1on requested review from mattsse and klkvr December 23, 2024 20:25

mattsse approved these changes Dec 24, 2024

View reviewed changes

mattsse merged commit eb1798a into alloy-rs:main Dec 24, 2024
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(alloy-eips): `SimpleCoder::decode_one()` should return `Ok(None)` #1818

fix(alloy-eips): `SimpleCoder::decode_one()` should return `Ok(None)` #1818

StackOverflowExcept1on commented Dec 22, 2024 •

edited

Loading

StackOverflowExcept1on Dec 22, 2024

StackOverflowExcept1on Dec 22, 2024

StackOverflowExcept1on Dec 22, 2024 •

edited

Loading

mattsse left a comment

mattsse Dec 23, 2024

StackOverflowExcept1on Dec 23, 2024

mattsse Dec 23, 2024

mattsse Dec 23, 2024

StackOverflowExcept1on Dec 23, 2024

mattsse Dec 23, 2024

StackOverflowExcept1on Dec 23, 2024

mattsse Dec 23, 2024 •

edited

Loading

klkvr Dec 23, 2024

StackOverflowExcept1on Dec 23, 2024

StackOverflowExcept1on Dec 23, 2024 •

edited

Loading

mattsse Dec 24, 2024

	fn decode_all(&mut self, blobs: &[Blob]) -> Option<Vec<Vec<u8>>> {
	let mut fes =
	blobs.iter().flat_map(\|blob\| blob.chunks(32).map(WholeFe::new)).map(Option::unwrap);

	let mut res = Vec::new();
	loop {
	match Self::decode_one(&mut fes) {
	Ok(Some(data)) => res.push(data),
	Ok(None) => break,
	Err(()) => return None,
	}
	}
	Some(res)
	}

		let last_unused_blob_index = self.fe / FIELD_ELEMENTS_PER_BLOB as usize;
		self.blobs.get_mut(last_unused_blob_index).expect("never empty")

	// if there are too many bytes
	if num_bytes > BYTES_PER_BLOB * MAX_BLOBS_PER_BLOCK {
	return Err(());
	}

fix(alloy-eips): SimpleCoder::decode_one() should return Ok(None) #1818

fix(alloy-eips): SimpleCoder::decode_one() should return Ok(None) #1818

Conversation

StackOverflowExcept1on commented Dec 22, 2024 • edited Loading

Motivation

Solution

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StackOverflowExcept1on Dec 22, 2024 • edited Loading

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StackOverflowExcept1on Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fix(alloy-eips): `SimpleCoder::decode_one()` should return `Ok(None)` #1818

fix(alloy-eips): `SimpleCoder::decode_one()` should return `Ok(None)` #1818

StackOverflowExcept1on commented Dec 22, 2024 •

edited

Loading

StackOverflowExcept1on Dec 22, 2024 •

edited

Loading

mattsse Dec 23, 2024 •

edited

Loading

StackOverflowExcept1on Dec 23, 2024 •

edited

Loading