feat: alloy bytes conversion #1552

etherhood · 2024-10-24T14:12:08Z

What was wrong?

Replace Vec with alloy::Bytes or bytes::Bytes based on usage

#1404

How was it fixed?

To-Do

Replace intermediary usage of Vec in some places

Clean up commit history and use conventional commits.

morph-dev

A lot of good work, but also a lot of small comments.

morph-dev · 2024-10-25T09:53:08Z

ethportal-api/src/types/portal_wire.rs

@@ -703,13 +703,13 @@ mod test {

    #[test]
    fn message_encoding_find_content() {
-        let content_key = hex_decode("0x706f7274616c").unwrap();
+        let content_key = Bytes::from("0x706f7274616c");


This should have been: Bytes::from_hex("0x706f7274616c").unwrap()

And you should revert expected_encoded to original value (line 712).

morph-dev · 2024-10-25T09:53:22Z

ethportal-api/src/types/portal_wire.rs

@@ -733,13 +733,14 @@ mod test {

    #[test]
    fn message_encoding_content_content() {
-        let content_val = hex_decode("0x7468652063616b652069732061206c6965").unwrap();
+        let content_val = Bytes::from("0x7468652063616b652069732061206c6965");


morph-dev · 2024-10-25T10:01:39Z

portalnet/src/discovery.rs

@@ -460,7 +464,7 @@ impl AsyncUdpSocket<UtpEnr> for Discv5UdpSocket {
    async fn send_to(&mut self, buf: &[u8], target: &UtpEnr) -> io::Result<usize> {
        let discv5 = Arc::clone(&self.discv5);
        let target = target.0.clone();
-        let data = buf.to_vec();
+        let data = Bytes::from(buf.to_vec());


should be: Bytes::copy_from_slice(buf)

morph-dev · 2024-10-25T10:13:40Z

portalnet/src/find/iterators/findcontent.rs

@@ -657,7 +658,9 @@ mod tests {
                                    let peer_node_id = k.preimage();
                                    query.on_success(
                                        peer_node_id,
-                                        FindContentQueryResponse::Content(found_content.clone()),
+                                        FindContentQueryResponse::Content(
+                                            found_content.clone().into(),


Type of found_content should be changed to RawContentValue. Meaning, at line 583, it should be:
let found_content = RawContentValue::from([0xef]);

Then here, you would have:
FindContentQueryResponse::Content(found_content.clone())

morph-dev · 2024-10-25T11:15:44Z

portalnet/src/overlay/service.rs

@@ -699,7 +699,7 @@ impl<
                                .connect_inbound_stream(cid)
                                .await
                            {
-                                Ok(data) => data,
+                                Ok(data) => data.into(),


nit: I think we can/should be more explicit here:
Ok(data) => RawContentValue::from(data)

morph-dev · 2024-10-25T13:30:55Z

trin-storage/src/lib.rs

        let content_id = key.content_id();
        let value: &[u8] = value.as_ref();
-        self.store.insert(content_id.to_vec(), value.to_vec());
+        self.store
+            .insert(content_id.to_vec(), Bytes::from(value.to_vec()));


This should be: Bytes::copy_from_slice(value)

morph-dev · 2024-10-25T13:36:12Z

trin-storage/src/versioned/id_indexed_v1/store.rs

@@ -265,7 +265,7 @@ impl<TContentKey: OverlayContentKey> IdIndexedV1Store<TContentKey> {
            named_params! {
                ":content_id": content_id,
                ":content_key": content_key,
-                ":content_value": content_value,
+                ":content_value": content_value.to_vec(),


Instead of to_vec() we should use as_ref().

morph-dev · 2024-10-25T13:40:37Z

trin-storage/src/versioned/id_indexed_v1/store.rs

@@ -510,7 +510,11 @@ impl<TContentKey: OverlayContentKey> IdIndexedV1Store<TContentKey> {
            self.usage_stats.entry_count -= to_delete;
            self.usage_stats.total_entry_size_bytes -= deleted_content_size;
            self.usage_stats.report_metrics(&self.metrics);
-            deleted_content.extend(deleted_content_values);
+            deleted_content.extend(


instead of doing it here, conversion should be done while reading data from db. Somewhere around line 481.
For example, something like this:

let key_bytes: Vec<u8> = row.get("content_key")?; let value_bytes: Vec<u8> = row.get("content_value")?; let value = RawContentValue::from(value_bytes); let size: u64 = row.get("content_size")?; TContentKey::try_from_bytes(key_bytes) .map(|key| (key, value, size)) .map_err(|e| { rusqlite::Error::FromSqlConversionFailure(0, Type::Blob, e.into()) })

morph-dev · 2024-10-25T13:42:51Z

trin-storage/src/versioned/id_indexed_v1/store.rs

@@ -811,12 +815,15 @@ mod tests {
        assert!(!store.has_content(&id)?);
        let usage_stats = store.usage_stats();

-        store.insert(&key, value.clone())?;
+        store.insert(&key, value.clone().into())?;


Instead of calling value.into() here and in many places below, we should refactor generate_key_value and generate_key_value_with_content_size so they return RawContentValue

morph-dev · 2024-10-25T13:48:30Z

portalnet/src/discovery.rs

@@ -36,7 +37,7 @@ const TALKREQ_CHANNEL_BUFFER: usize = 100;
 /// ENR key for portal network client version.
 pub const ENR_PORTAL_CLIENT_KEY: &str = "c";

-pub type ProtocolRequest = Vec<u8>;
+pub type ProtocolRequest = Bytes;


After looking more into how this is used, I think it's better if this is not changed to Bytes and left as Vec<u8>.

etherhood · 2024-12-01T17:13:57Z

A lot of good work, but also a lot of small comments.

Addressed all the comments, ready for review

morph-dev

Looks good! Few minor comments.

After you address remaining comments, can you rebase/merge to head. This diverged quite some time ago...

morph-dev · 2024-12-02T11:59:46Z

trin-state/src/storage.rs

@@ -280,10 +278,10 @@ pub mod test {
        for value in value.as_sequence().unwrap() {
            result.push(ContentData {
                key: StateContentKey::deserialize(value.get("content_key").unwrap())?,
-                store_value: hex_decode(
+                store_value: RawContentValue::from_hex(


This whole section can be:

result.push(ContentData { key: StateContentKey::deserialize(&value["content_key"])?, store_value: RawContentValue::deserialize(&value["content_value_offer"])?, lookup_value: RawContentValue::deserialize(&value["content_value_retrieval"])?, });

morph-dev · 2024-12-02T12:01:57Z

trin-storage/src/versioned/id_indexed_v1/store.rs

+    ) -> (IdentityContentKey, RawContentValue) {
+        let (key, value) =
+            generate_key_value_with_content_size(config, distance, CONTENT_DEFAULT_SIZE_BYTES);
+        (key, RawContentValue::copy_from_slice(value.as_ref()))
    }

    fn generate_key_value_with_content_size(


The generate_key_value_with_content_size should return (IdentityContentKey, RawContentValue).

This will simplify generate_key_value, and we can remove several value.into() below.

morph-dev · 2024-12-02T12:06:41Z

portalnet/src/discovery.rs

-        Ok(response)
+        let response = self
+            .discv5
+            .talk_req(enr, protocol, request.to_vec())


With reverted changes, we can just use request (no need for request.to_vec()).

morph-dev · 2024-12-02T12:09:18Z

portalnet/src/overlay/service.rs

@@ -1042,7 +1044,7 @@ impl<
                    // over the uTP stream.
                    let utp = Arc::clone(&self.utp_controller);
                    tokio::spawn(async move {
-                        utp.accept_outbound_stream(cid, content).await;
+                        utp.accept_outbound_stream(cid, content.as_ref()).await;


nit: this should be &content

morph-dev · 2024-12-02T12:11:25Z

portalnet/src/overlay/service.rs

+                .send_talk_req(
+                    destination,
+                    protocol,
+                    Message::from(request).as_ssz_bytes().to_vec(),


No need for .to_vec(), meaning it should be: Message::from(request).as_ssz_bytes()

morph-dev · 2024-12-02T12:13:27Z

portalnet/src/overlay/service.rs

@@ -1595,7 +1600,7 @@ impl<
                }
            };
            let result = utp_controller
-                .connect_outbound_stream(cid, content_payload.to_vec())
+                .connect_outbound_stream(cid, content_payload.as_ref())


This should be: &content_payload

…verlay

morph-dev · 2024-12-02T18:07:45Z

Looks good. Thanks!

etherhood force-pushed the feat/alloy-bytes-conversion branch from 1daad07 to 71175fc Compare October 24, 2024 14:59

etherhood mentioned this pull request Oct 24, 2024

Move away from using Vec<u8> to represent raw content key/value #1404

Open

18 tasks

KolbyML requested a review from morph-dev October 24, 2024 20:02

KolbyML changed the title ~~Feat: alloy bytes conversion~~ feat: alloy bytes conversion Oct 24, 2024

morph-dev requested changes Oct 25, 2024

View reviewed changes

etherhood requested a review from morph-dev December 1, 2024 08:20

morph-dev approved these changes Dec 2, 2024

View reviewed changes

etherhood added 7 commits December 2, 2024 21:23

feat: replaced vec<u8> in portalwire, query info , find content and o…

7fc29c4

…verlay

feat: converting Vec<u8> to bytes::Bytes

484d96e

fix: merge conflicts with master

1be93c5

fix: remove warning related ci issues

d5cab3f

feat: change vec<u8> to raw content value in ContentStore

72adc7c

fix: adding changes requested in PR

5f05358

fix: small fixes

376742e

etherhood force-pushed the feat/alloy-bytes-conversion branch from a6679cf to 376742e Compare December 2, 2024 17:23

morph-dev merged commit d877539 into ethereum:master Dec 2, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: alloy bytes conversion #1552

feat: alloy bytes conversion #1552

etherhood commented Oct 24, 2024 •

edited by morph-dev

Loading

morph-dev left a comment

morph-dev Oct 25, 2024

morph-dev Oct 25, 2024

morph-dev Oct 25, 2024

morph-dev Oct 25, 2024

morph-dev Oct 25, 2024

morph-dev Oct 25, 2024

morph-dev Oct 25, 2024

morph-dev Oct 25, 2024

morph-dev Oct 25, 2024

morph-dev Oct 25, 2024

etherhood commented Dec 1, 2024

morph-dev left a comment

morph-dev Dec 2, 2024

morph-dev Dec 2, 2024

morph-dev Dec 2, 2024

morph-dev Dec 2, 2024

morph-dev Dec 2, 2024

morph-dev Dec 2, 2024

morph-dev commented Dec 2, 2024

feat: alloy bytes conversion #1552

feat: alloy bytes conversion #1552

Conversation

etherhood commented Oct 24, 2024 • edited by morph-dev Loading

What was wrong?

How was it fixed?

To-Do

morph-dev 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etherhood commented Dec 1, 2024

morph-dev 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

morph-dev commented Dec 2, 2024

etherhood commented Oct 24, 2024 •

edited by morph-dev

Loading