In Core they enforce incremental memory allocation to prevent an attacker from claiming a large vector size while providing minimal data. It's set to 5Mb through MAX_VECTOR_ALLOCATE, so instead of allocating everything with Vec::with_capacity(length as usize) we can allocate memory in chunks.

Not sure if we need to bother with that here since we already hard cap things at 4Mb with MAX_VEC_SIZE

Also I noticed we have MAX_ENCODABLE_VALUE=0x02000000 in the code, but it doesn’t seem to be used anywhere. I’m not sure yet why this constant is set to 32 MB, I tried to trace it back in Core and didn’t find a clear reason

It's just an arbitrary anti-DoS limit in Core.

Re your first point I don't think we need more memory protection other than the 4,000,000 byte limit.

Re the second point, thanks for noticing. Its discussed in #3264.

Re your first point I don't think we need more memory protection other than the 4,000,000 byte limit.

you're right

In 7e5ccd2:

Can you add a test that uses a multi-byte length prefix (annoyingly it'll need to have like 260 bytes)? The logic around assigning self.prefix_decoder is a little subtle and currently we don't test the Ok(true) path.

Yeah I had that thought myself yesterday ... can do.

In 7e5ccd2:

See my repeated comments in #4998 about this. (This PR is not in draft but that one is; should we move discussion here?)

Just use usize::try_from(n).map_err(|_| LengthPrefixExceedsMaxError { value: n }) and be done with it. There is no need to special-case 16-bit machines. You can add a comment if you want saying that this error path will only be hit on 16-bit machines (or, I suppose, 18-bit machines, which the current code handles incorrectly). But the logic is the same regardless of machine size.

Ouch, sucks failing in public. Now I've made the change (it belongs in this PR).

Sorry for my tone BTW -- I'm not actually annoyed, I'm just writing fire-and-forget review comments and moving onto the next Github notification.

No stress bro, fail is a fail. You don't have to pull punches with me. Write fast.

In 7e5ccd2 is there a downside to eagerly initializing prefix_decoder in the ByteVecDecoder::new constructor and using it to represent the state of the ByteVecDecoder? If present, decoding the length prefix, else decoding the bytes. Then we could simplify the internal state and drop the prefix_read flag and perhaps simplify logic in the min_bytes_needed method.

This may be possible, and other refactors. I'd like to get this merged and iterate on the internals (as well as adding more tests). I'm hoping to only fix grievous issues, it would be really nice to get this in so we can rc.0 by Nashville.

Ah, since I was doing the PhantomData I did this too. Thanks for pushing me.

While doing so I forgot to re-set it after taking it out of the option and the tests did not fail (because single byte compact size). Goes with @apoelstra's comment above, we need tests that use multi-byte compact size encodings.

fwiw I still see the prefix_decoder in ByteVecDecoder, but looks like VecDecoder<T: Decodable> was updated to the new logic.

Geez, hard to get good help huh. Thanks for being so patient with me.

In 501b350 is the PhantomData required? I am new to the pattern, but the type is being used in the buffer field of VecDecoder<T> so I think the compiler is able to track it without any marker?

Oh I rekon you are right, I'll have a go at that.

Yep, works. Thanks.

In 4c3e8da:

Could you eliminate this cast, for example, by using self.bytes_expected which you obtained two lines above by calling cast_to_usize_if_valid(length)?

Done.

In e22b6a9:

The best we can do is to return 1. What you are returning here (querying a dummy encoder and then multiplying its result by items_left_to_decode) might wildly overestimate, causing Reads to extract unnecessary data from their underlying source and then losing it.

How will it overestimate? If its a vector types with fixed size eg Txids this will be accurate. If its a vector of ByteVecDecoders it will be 1 byte for each left to decode, which is not an overread. Do you have a case in mind that would overread?

I added this test, does it convince you its ok?

    #[cfg(feature = "std")]
    #[test]
    fn decode_vec_from_read_unbuffered_success() {
        let encoded = [0x01, 0xEF, 0xBE, 0xAD, 0xDE, 0xff, 0xff, 0xff, 0xff];
        let mut cursor = Cursor::new(&encoded);

        let got = crate::decode_from_read_unbuffered::<Test, _>(&mut cursor).unwrap();
        assert_eq!(cursor.position(), 5);

        let want = Test(vec![Inner(0xDEAD_BEEF)]);
        assert_eq!(got, want);
    }

Oh, that is not much context. You'll need this too

    #[cfg(feature = "alloc")]
    #[derive(Clone, Debug, PartialEq, Eq)]
    pub struct Test(Vec<Inner>);

    /// The decoder for the [`Test`] type.
    #[cfg(feature = "alloc")]
    #[derive(Default)]
    pub struct TestDecoder(VecDecoder<Inner>);
```

I raised #5067, can we merge and make sure that is closed before final release? We still have the transaction decoders to go and its way harder to review that this one. It also should prove this PR further. I'd like to avoid too many more 24 turn arounds for chats. Nashville is coming and I'm starting to feel like we can get the first primitives RC release out the door.

What if any of them return zero? What if whatever T::encoder().min_bytes_needed() returns is lower from what an actual encoder would return?

I think this is okay, actually, because the initial value of T::decoder().min_bytes_needed() will give an accurate bound on the number of bytes initially consumable by each of the following Ts.