Change rlp to alloy-rlp #46
Conversation
armaganyildirak
changed the title
There was a problem hiding this comment.
I'm starting to review this.
Great job in getting this done and making all the tests pass.
Since I'm just starting to get familiar with alloy-rlp it will take me a bit to give a proper review. For now, one main comment:
Changes as they are right now generate way too many vector allocations that are freed almost immediately. This is less than ideal. We should try to reduce the number of calls to to_vec as much as possible. This copies the data to a new vector, and most of the time, this is used to move the data to yet another final location. This includes avoiding when possible calls to alloy_rlp::encode. I left an example of avoiding these two in the review.
Following this line of thought, since this is a breaking change: the Encodable and Decodable traits are now different (from rlp to alloy-rlp) we have now the freedom to change the public api since it's no longer an "implementation detail" change. I'd like us to consider these two changes:
pub fn get(&self, key: impl AsRef<[u8]>) -> Option<impl AsRef<[u8]> + '_> and
pub fn get_raw_rlp(&self, key: impl AsRef<[u8]>) -> Option<impl AsRef<[u8]> + '_> I think these two are good part of the sources of temporary vec allocations. I have not checked if this works or how ergonomic it is but we should give it a go in pro of performance.
Unrelated comment, but alloy_rlp already implements encoding and decoding for the ip/ipv4/ipv6 types. We should check if the default implementation is compatible and use that instead of relaying on encoding of the octects ourselves if possible
|
hi @armaganyildirak is this ready for review again? when it is, can you hit the re-request review button so that I get notified? |
There was a problem hiding this comment.
I realized get does not need to change. By updating that function a lot of changes will be triggered that revert the need to use for example as_slice in a good couple of places
Also you can obtain a Bytes from a BytesMut using freeze(). There is no need to copy there.
Note that I've signaled just some of the places where changes can be improved
There was a problem hiding this comment.
yet more vecs and copy that we can avoid. I did not check them all, so it would be good to go over the whole diff and see based on these suggestions, what over copy_from_slice and to_vec can be avoided.
Also remember to merge master to fix the conflicts
It's getting there, good work
src/lib.rs
Outdated
| /// # Panics | ||
| /// | ||
| /// Will panic if data is not sanitized |
There was a problem hiding this comment.
this is not necessary due to the allow lint below
| @@ -119,7 +119,7 @@ | |||
| //! assert_eq!(decoded_enr.ip4(), Some("192.168.0.1".parse().unwrap())); | |||
| //! assert_eq!(decoded_enr.id(), Some("v4".into())); | |||
| //! assert_eq!(decoded_enr.tcp4(), Some(8001)); | |||
| //! assert_eq!(decoded_enr.get("custom_key"), Some(vec![0,0,1].as_slice())); | |||
| //! assert_eq!(decoded_enr.get("custom_key").as_ref().map(AsRef::as_ref), Some(vec![0,0,1]).as_deref()); | |||
There was a problem hiding this comment.
could you check if this can be simplified?
There was a problem hiding this comment.
Sorry I couldn't find a way to make it simple because the first part (.get("custom_key")) is Option<Bytes> and the second part (Some(vec![0, 0, 1])) is Option<Vec<{integer}>>. I couldn't find any relation with these two. I'm waiting your suggestions.
src/lib.rs
Outdated
| /// # Panics | ||
| /// | ||
| /// Will panic if the content does not has a public key |
There was a problem hiding this comment.
this looks like a merge that left some traces. The panic info is already in the next lines
src/lib.rs
Outdated
| let mut list = Vec::<u8>::new(); | ||
| self.signature.as_slice().encode(&mut list); | ||
| self.seq.encode(&mut list); | ||
| for (k, v) in &self.content { | ||
| k.as_slice().encode(&mut list); | ||
| list.extend_from_slice(v); | ||
| } | ||
| let header = Header { | ||
| list: true, | ||
| payload_length: list.len(), | ||
| }; | ||
| header.encode(out); | ||
| out.put_slice(&list); |
There was a problem hiding this comment.
use append_rlp_content here please
| let payload_info = rlp.payload_info()?; | ||
| if rlp.as_raw().len() != payload_info.header_len + payload_info.value_len { | ||
| return Err(DecoderError::RlpInconsistentLengthAndData); | ||
| let payload = &mut &buf[..header.payload_length]; |
There was a problem hiding this comment.
this can panic if buf is of less size than payload_length. You need to check the size of the buffer first
There was a problem hiding this comment.
@armaganyildirak - I think this needs to be addressed.
We should probably fuzz these changes as they are extensive and involve the decoder
There was a problem hiding this comment.
I think we don't need to check it because payload_lenght has to be always less size than buffer size but I have already added the size check. https://github.com/armaganyildirak/enr/blob/alloy-rlp/src/lib.rs#L990C1-L993C1
src/lib.rs
Outdated
| .map_err(|err| EnrError::InvalidRlpData(err.to_string()))?; | ||
| if id_bytes != b"v4" { | ||
| if id_bytes.to_vec() != b"v4" { |
There was a problem hiding this comment.
| if id_bytes.to_vec() != b"v4" { | |
| if id_bytes.as_ref() != b"v4" { |
Try to change rlp to alloy-rlp.