Message Generator: fix serialization and add into_static for serde_sv2 primitives, broken unit tests, new unit test for NMJ message, #949

lorbax · 2024-06-01T16:13:06Z

Solves 1041

Unity tests in main.rs of MG are fixed. One is added. This required to fix the serialization the primitives in the serde case (/protocols/v2/binary_sv2/serde_sv2)
test.json fixed in src directory of MG
mining-proxy config file fixed
Add fix to an issue that @Fi3 pointed out
https://discord.com/channels/950687892169195530/1212660461569441822/1243211841652654151
the fix consisted in the into_static function for serde_sv2 sequences

github-actions · 2024-06-01T16:17:13Z

Bencher

Report	Thu, July 11, 2024 at 10:06:17 UTC
Project	Stratum v2 (SRI)
Branch	MG-fix-primitives-serialization-and-unit-tests
Testbed	sv2

Click to view all benchmark results

Benchmark	Latency	Latency Results nanoseconds (ns) \| (Δ%)	Latency Upper Boundary nanoseconds (ns) \| (%)
client_sv2_handle_message_common	✅ (view plot)	44.56 (+0.01%)	45.23 (98.54%)
client_sv2_handle_message_mining	✅ (view plot)	74.77 (+2.48%)	80.37 (93.03%)
client_sv2_mining_message_submit_standard	✅ (view plot)	14.68 (+0.17%)	14.70 (99.87%)
client_sv2_mining_message_submit_standard_serialize	✅ (view plot)	262.63 (-0.74%)	284.29 (92.38%)
client_sv2_mining_message_submit_standard_serialize_deserialize	✅ (view plot)	602.51 (+1.46%)	627.61 (96.00%)
client_sv2_open_channel	✅ (view plot)	166.32 (+0.23%)	173.28 (95.98%)
client_sv2_open_channel_serialize	✅ (view plot)	274.83 (-2.69%)	293.82 (93.54%)
client_sv2_open_channel_serialize_deserialize	✅ (view plot)	380.07 (+0.55%)	422.42 (89.97%)
client_sv2_setup_connection	✅ (view plot)	161.23 (-1.68%)	174.55 (92.37%)
client_sv2_setup_connection_serialize	✅ (view plot)	493.58 (+4.15%)	508.26 (97.11%)
client_sv2_setup_connection_serialize_deserialize	✅ (view plot)	939.32 (-3.29%)	1,038.57 (90.44%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

github-actions · 2024-06-01T16:17:13Z

Bencher

Report	Tue, July 9, 2024 at 14:39:09 UTC
Project	Stratum v2 (SRI)
Branch	MG-fix-primitives-serialization-and-unit-tests
Testbed	sv1

🚨 2 ALERTS: Threshold Boundary Limits exceeded!

Benchmark	Measure (units)	View	Value	Lower Boundary	Upper Boundary
serialize_deserialize_authorize	RAM Accesses (accesses)	🚨 (view plot \| view alert)	299.00 (+1.35%)		298.35 (100.22%)
serialize_deserialize_handle_authorize	RAM Accesses (accesses)	🚨 (view plot \| view alert)	369.00 (+1.35%)		367.87 (100.31%)

Click to view all benchmark results

Benchmark	Estimated Cycles	Estimated Cycles Results estimated cycles \| (Δ%)	Estimated Cycles Upper Boundary estimated cycles \| (%)	Instructions	Instructions Results instructions \| (Δ%)	Instructions Upper Boundary instructions \| (%)	L1 Accesses	L1 Accesses Results accesses \| (Δ%)	L1 Accesses Upper Boundary accesses \| (%)	L2 Accesses	L2 Accesses Results accesses \| (Δ%)	L2 Accesses Upper Boundary accesses \| (%)	RAM Accesses	RAM Accesses Results accesses \| (Δ%)	RAM Accesses Upper Boundary accesses \| (%)
get_authorize	✅ (view plot)	8,538.00 (+1.17%)	8,726.46 (97.84%)	✅ (view plot)	3,746.00 (+0.14%)	3,853.79 (97.20%)	✅ (view plot)	5,248.00 (+0.06%)	5,399.31 (97.20%)	✅ (view plot)	7.00 (-11.53%)	10.35 (67.64%)	✅ (view plot)	93.00 (+3.17%)	94.22 (98.70%)
get_submit	✅ (view plot)	95,509.00 (-0.05%)	96,131.28 (99.35%)	✅ (view plot)	59,439.00 (-0.05%)	59,773.05 (99.44%)	✅ (view plot)	85,359.00 (-0.05%)	85,828.74 (99.45%)	✅ (view plot)	49.00 (-10.74%)	62.60 (78.28%)	✅ (view plot)	283.00 (+0.31%)	287.65 (98.38%)
get_subscribe	✅ (view plot)	7,991.00 (+0.17%)	8,266.41 (96.67%)	✅ (view plot)	2,841.00 (+0.38%)	2,938.55 (96.68%)	✅ (view plot)	3,971.00 (+0.42%)	4,098.02 (96.90%)	✅ (view plot)	13.00 (-19.10%)	19.85 (65.51%)	✅ (view plot)	113.00 (+0.31%)	116.88 (96.68%)
serialize_authorize	✅ (view plot)	12,291.00 (+0.67%)	12,503.21 (98.30%)	✅ (view plot)	5,317.00 (+0.10%)	5,424.79 (98.01%)	✅ (view plot)	7,411.00 (+0.04%)	7,562.65 (97.99%)	✅ (view plot)	10.00 (-7.17%)	13.33 (75.05%)	✅ (view plot)	138.00 (+1.73%)	140.33 (98.34%)
serialize_deserialize_authorize	✅ (view plot)	24,588.00 (+0.47%)	24,713.56 (99.49%)	✅ (view plot)	9,898.00 (-0.02%)	10,024.26 (98.74%)	✅ (view plot)	13,958.00 (-0.06%)	14,148.06 (98.66%)	✅ (view plot)	33.00 (-9.48%)	41.59 (79.34%)	🚨 (view plot \| view alert)	299.00 (+1.35%)	298.35 (100.22%)
serialize_deserialize_handle_authorize	✅ (view plot)	30,331.00 (+0.59%)	30,360.97 (99.90%)	✅ (view plot)	12,101.00 (+0.04%)	12,208.79 (99.12%)	✅ (view plot)	17,116.00 (-0.01%)	17,277.49 (99.07%)	✅ (view plot)	60.00 (+1.97%)	64.68 (92.76%)	🚨 (view plot \| view alert)	369.00 (+1.35%)	367.87 (100.31%)
serialize_deserialize_handle_submit	✅ (view plot)	126,406.00 (-0.00%)	127,021.98 (99.52%)	✅ (view plot)	73,224.00 (-0.03%)	73,613.89 (99.47%)	✅ (view plot)	104,946.00 (-0.04%)	105,502.88 (99.47%)	✅ (view plot)	120.00 (-0.41%)	131.10 (91.53%)	✅ (view plot)	596.00 (+0.19%)	599.22 (99.46%)
serialize_deserialize_handle_subscribe	✅ (view plot)	27,529.00 (+0.26%)	27,605.43 (99.72%)	✅ (view plot)	9,643.00 (+0.11%)	9,740.55 (99.00%)	✅ (view plot)	13,639.00 (+0.11%)	13,773.78 (99.02%)	✅ (view plot)	62.00 (-5.71%)	73.68 (84.14%)	✅ (view plot)	388.00 (+0.55%)	388.75 (99.81%)
serialize_deserialize_submit	✅ (view plot)	114,999.00 (-0.07%)	115,642.56 (99.44%)	✅ (view plot)	68,001.00 (-0.08%)	68,396.42 (99.42%)	✅ (view plot)	97,559.00 (-0.09%)	98,141.34 (99.41%)	✅ (view plot)	65.00 (-6.18%)	75.29 (86.33%)	✅ (view plot)	489.00 (+0.18%)	492.35 (99.32%)
serialize_deserialize_subscribe	✅ (view plot)	22,922.00 (+0.20%)	23,109.96 (99.19%)	✅ (view plot)	8,195.00 (+0.11%)	8,296.16 (98.78%)	✅ (view plot)	11,542.00 (+0.11%)	11,680.98 (98.81%)	✅ (view plot)	36.00 (-8.35%)	44.07 (81.69%)	✅ (view plot)	320.00 (+0.45%)	321.55 (99.52%)
serialize_submit	✅ (view plot)	99,826.00 (-0.07%)	100,459.28 (99.37%)	✅ (view plot)	61,483.00 (-0.05%)	61,822.35 (99.45%)	✅ (view plot)	88,206.00 (-0.05%)	88,682.17 (99.46%)	✅ (view plot)	49.00 (-11.67%)	62.44 (78.47%)	✅ (view plot)	325.00 (+0.10%)	329.13 (98.75%)
serialize_subscribe	✅ (view plot)	11,328.00 (+0.01%)	11,602.08 (97.64%)	✅ (view plot)	4,188.00 (+0.26%)	4,285.55 (97.72%)	✅ (view plot)	5,828.00 (+0.27%)	5,957.17 (97.83%)	✅ (view plot)	15.00 (-8.06%)	18.93 (79.23%)	✅ (view plot)	155.00 (-0.14%)	159.68 (97.07%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

github-actions · 2024-06-01T16:17:15Z

Bencher

Report	Thu, July 11, 2024 at 10:06:28 UTC
Project	Stratum v2 (SRI)
Branch	MG-fix-primitives-serialization-and-unit-tests
Testbed	sv2

🚨 11 ALERTS: Threshold Boundary Limits exceeded!

Benchmark	Measure (units)	View	Value	Upper Boundary
client_sv2_mining_message_submit_standard	L2 Accesses (accesses)	🚨 (view plot \| view alert)	27.00 (+48.98%)	25.07 (107.70%)
client_sv2_mining_message_submit_standard_serialize	L2 Accesses (accesses)	🚨 (view plot \| view alert)	55.00 (+15.00%)	54.13 (101.61%)
client_sv2_mining_message_submit_standard_serialize_deserialize	Instructions (instructions)	🚨 (view plot \| view alert)	10,585.00 (+0.38%)	10,575.97 (100.09%)
client_sv2_mining_message_submit_standard_serialize_deserialize	L1 Accesses (accesses)	🚨 (view plot \| view alert)	15,399.00 (+0.36%)	15,386.05 (100.08%)
client_sv2_open_channel_serialize_deserialize	Instructions (instructions)	🚨 (view plot \| view alert)	8,027.00 (+0.50%)	8,018.31 (100.11%)
client_sv2_open_channel_serialize_deserialize	L1 Accesses (accesses)	🚨 (view plot \| view alert)	11,671.00 (+0.45%)	11,659.80 (100.10%)
client_sv2_open_channel_serialize_deserialize	L2 Accesses (accesses)	🚨 (view plot \| view alert)	85.00 (+14.73%)	84.35 (100.78%)
client_sv2_setup_connection	L2 Accesses (accesses)	🚨 (view plot \| view alert)	15.00 (+58.04%)	14.88 (100.81%)
client_sv2_setup_connection_serialize	L2 Accesses (accesses)	🚨 (view plot \| view alert)	50.00 (+11.20%)	49.93 (100.14%)
client_sv2_setup_connection_serialize_deserialize	Instructions (instructions)	🚨 (view plot \| view alert)	14,855.00 (+0.28%)	14,845.89 (100.06%)
client_sv2_setup_connection_serialize_deserialize	L1 Accesses (accesses)	🚨 (view plot \| view alert)	21,811.00 (+0.26%)	21,797.63 (100.06%)

Click to view all benchmark results

Benchmark	Estimated Cycles	Estimated Cycles Results estimated cycles \| (Δ%)	Estimated Cycles Upper Boundary estimated cycles \| (%)	Instructions	Instructions Results instructions \| (Δ%)	Instructions Upper Boundary instructions \| (%)	L1 Accesses	L1 Accesses Results accesses \| (Δ%)	L1 Accesses Upper Boundary accesses \| (%)	L2 Accesses	L2 Accesses Results accesses \| (Δ%)	L2 Accesses Upper Boundary accesses \| (%)	RAM Accesses	RAM Accesses Results accesses \| (Δ%)	RAM Accesses Upper Boundary accesses \| (%)
client_sv2_handle_message_common	✅ (view plot)	2,081.00 (+1.41%)	2,130.02 (97.70%)	✅ (view plot)	473.00 (+0.46%)	486.12 (97.30%)	✅ (view plot)	731.00 (-0.15%)	754.53 (96.88%)	✅ (view plot)	11.00 (+52.93%)	11.35 (96.91%)	✅ (view plot)	37.00 (+0.86%)	38.58 (95.91%)
client_sv2_handle_message_mining	✅ (view plot)	8,223.00 (+0.30%)	8,334.53 (98.66%)	✅ (view plot)	2,137.00 (+0.43%)	2,170.89 (98.44%)	✅ (view plot)	3,158.00 (+0.40%)	3,215.15 (98.22%)	✅ (view plot)	40.00 (+3.64%)	43.56 (91.83%)	✅ (view plot)	139.00 (+0.10%)	141.90 (97.96%)
client_sv2_mining_message_submit_standard	✅ (view plot)	6,286.00 (+0.15%)	6,387.01 (98.42%)	✅ (view plot)	1,750.00 (+0.03%)	1,762.83 (99.27%)	✅ (view plot)	2,546.00 (-0.29%)	2,575.18 (98.87%)	🚨 (view plot \| view alert)	27.00 (+48.98%)	25.07 (107.70%)	✅ (view plot)	103.00 (-0.76%)	106.81 (96.43%)
client_sv2_mining_message_submit_standard_serialize	✅ (view plot)	14,657.00 (-0.75%)	15,026.16 (97.54%)	✅ (view plot)	4,694.00 (+0.01%)	4,706.83 (99.73%)	✅ (view plot)	6,752.00 (-0.03%)	6,774.58 (99.67%)	🚨 (view plot \| view alert)	55.00 (+15.00%)	54.13 (101.61%)	✅ (view plot)	218.00 (-1.86%)	229.81 (94.86%)
client_sv2_mining_message_submit_standard_serialize_deserialize	✅ (view plot)	27,469.00 (-0.03%)	27,833.91 (98.69%)	🚨 (view plot \| view alert)	10,585.00 (+0.38%)	10,575.97 (100.09%)	🚨 (view plot \| view alert)	15,399.00 (+0.36%)	15,386.05 (100.08%)	✅ (view plot)	90.00 (+6.99%)	90.41 (99.54%)	✅ (view plot)	332.00 (-0.79%)	344.95 (96.25%)
client_sv2_open_channel	✅ (view plot)	4,417.00 (-1.57%)	4,608.55 (95.84%)	✅ (view plot)	1,461.00 (+0.05%)	1,474.10 (99.11%)	✅ (view plot)	2,157.00 (+0.19%)	2,172.76 (99.27%)	✅ (view plot)	11.00 (-7.66%)	15.12 (72.76%)	✅ (view plot)	63.00 (-3.08%)	68.13 (92.48%)
client_sv2_open_channel_serialize	✅ (view plot)	14,010.00 (-1.36%)	14,463.91 (96.86%)	✅ (view plot)	5,064.00 (+0.02%)	5,077.10 (99.74%)	✅ (view plot)	7,320.00 (+0.03%)	7,338.88 (99.74%)	✅ (view plot)	43.00 (+14.27%)	43.03 (99.94%)	✅ (view plot)	185.00 (-3.32%)	199.09 (92.92%)
client_sv2_open_channel_serialize_deserialize	✅ (view plot)	22,561.00 (-0.33%)	23,010.78 (98.05%)	🚨 (view plot \| view alert)	8,027.00 (+0.50%)	8,018.31 (100.11%)	🚨 (view plot \| view alert)	11,671.00 (+0.45%)	11,659.80 (100.10%)	🚨 (view plot \| view alert)	85.00 (+14.73%)	84.35 (100.78%)	✅ (view plot)	299.00 (-1.71%)	314.74 (95.00%)
client_sv2_setup_connection	✅ (view plot)	4,659.00 (-0.79%)	4,763.39 (97.81%)	✅ (view plot)	1,502.00 (+0.05%)	1,515.10 (99.14%)	✅ (view plot)	2,274.00 (-0.11%)	2,298.94 (98.92%)	🚨 (view plot \| view alert)	15.00 (+58.04%)	14.88 (100.81%)	✅ (view plot)	66.00 (-2.61%)	69.70 (94.69%)
client_sv2_setup_connection_serialize	✅ (view plot)	15,946.00 (-1.83%)	16,520.51 (96.52%)	✅ (view plot)	5,963.00 (+0.01%)	5,976.10 (99.78%)	✅ (view plot)	8,661.00 (+0.06%)	8,677.64 (99.81%)	🚨 (view plot \| view alert)	50.00 (+11.20%)	49.93 (100.14%)	✅ (view plot)	201.00 (-4.45%)	218.59 (91.95%)
client_sv2_setup_connection_serialize_deserialize	✅ (view plot)	35,416.00 (-0.31%)	35,742.28 (99.09%)	🚨 (view plot \| view alert)	14,855.00 (+0.28%)	14,845.89 (100.06%)	🚨 (view plot \| view alert)	21,811.00 (+0.26%)	21,797.63 (100.06%)	✅ (view plot)	110.00 (+9.62%)	114.22 (96.30%)	✅ (view plot)	373.00 (-1.62%)	385.09 (96.86%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

github-actions · 2024-06-01T16:17:19Z

Bencher

Report	Thu, July 11, 2024 at 10:06:16 UTC
Project	Stratum v2 (SRI)
Branch	949/merge
Testbed	sv1

Click to view all benchmark results

Benchmark	Latency	Latency Results nanoseconds (ns) \| (Δ%)	Latency Upper Boundary nanoseconds (ns) \| (%)
client-submit-serialize	✅ (view plot)	6,491.80 (-5.80%)	7,381.15 (87.95%)
client-submit-serialize-deserialize	✅ (view plot)	7,311.30 (-6.35%)	8,362.27 (87.43%)
client-submit-serialize-deserialize-handle/client-submit-serialize-deserialize-handle	✅ (view plot)	7,948.10 (-5.21%)	8,853.93 (89.77%)
client-sv1-authorize-serialize-deserialize-handle/client-sv1-authorize-serialize-deserialize-handle	✅ (view plot)	903.20 (+0.41%)	927.57 (97.37%)
client-sv1-authorize-serialize-deserialize/client-sv1-authorize-serialize-deserialize	✅ (view plot)	686.28 (-1.56%)	717.89 (95.60%)
client-sv1-authorize-serialize/client-sv1-authorize-serialize	✅ (view plot)	244.05 (-1.58%)	254.95 (95.72%)
client-sv1-get-authorize/client-sv1-get-authorize	✅ (view plot)	155.46 (-1.00%)	162.18 (95.86%)
client-sv1-get-submit	✅ (view plot)	6,254.80 (-6.09%)	7,155.31 (87.41%)
client-sv1-get-subscribe/client-sv1-get-subscribe	✅ (view plot)	272.91 (-2.00%)	290.75 (93.86%)
client-sv1-subscribe-serialize-deserialize-handle/client-sv1-subscribe-serialize-deserialize-handle	✅ (view plot)	759.64 (+1.43%)	779.75 (97.42%)
client-sv1-subscribe-serialize-deserialize/client-sv1-subscribe-serialize-deserialize	✅ (view plot)	620.19 (+0.90%)	638.57 (97.12%)
client-sv1-subscribe-serialize/client-sv1-subscribe-serialize	✅ (view plot)	197.87 (-4.05%)	219.51 (90.14%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

jbesraa

@lorbax did initial pass. Also, could you please tide up the commits so you have a couple of detailed commits explaining the problem and the solution you are proposing?

jbesraa · 2024-06-11T14:41:27Z

protocols/v2/binary-sv2/serde-sv2/src/primitives/byte_arrays/b064k.rs

@@ -4,7 +4,7 @@ use core::convert::{TryFrom, TryInto};
 use serde::{de::Visitor, ser, ser::SerializeTuple, Deserialize, Deserializer, Serialize};

 #[derive(Debug, Clone, Eq)]
-enum Inner<'a> {
+pub enum Inner<'a> {


What the reason for making this public? I think having a variable called Inner and making its visibility pub is not great for readability

makes sense

actually it is not even necessary to expose Inner, I removed pub and seems to be compiling without issues ^^

jbesraa · 2024-06-11T14:43:05Z

protocols/v2/binary-sv2/serde-sv2/src/primitives/sequences/option.rs

-                seq.serialize_element(tuple.1)?;
-                seq.end()
+                if serializer.is_human_readable() {
+                    let data_ = self.data.clone().unwrap();


Why not use data from the match result?

Because I had to do this modification several times and I did past and copy, but you are right I should use the data in the match branch!
Thank you!

jbesraa · 2024-06-19T11:17:44Z

protocols/v2/binary-sv2/serde-sv2/src/primitives/sequences/mod.rs

+        let mut last = elem_len;
+        while last <= self.data.len() {
+            let slice = &self.data[first..last];
+            let elem = T::try_from_slice(slice).map_err(|_| ())?;


Should we add an error variant here?

An crate::error::Error::PrimitiveConversionError should be the right solution, what do you think?
BTW this part of code is used only in the message generator and not for production.

Ah, we can leave as is then

Having second thoughts about this. As it is exposed through the public API, it might make more sense to handle errors properly here.
Also, I have a small question:
If self.size is bigger than self.data.len, is returning empty array with self.size cap(the current behavior) makes sense or an error should be returned?

jbesraa · 2024-06-19T11:21:42Z

protocols/v2/binary-sv2/serde-sv2/src/primitives/sequences/option.rs

+                    seq.serialize_element(&tuple.0)?;
+                    seq.serialize_element(tuple.1)?;
+                    seq.end()
+                }
            }
            _ => panic!(),


Could you handle this panic please?

As I understood, the type Sv2Option can never have both fields of the same variants. We should ask @Fi3 for more details

maybe add assert_debug!(false, "EXPLAIN_WHY_THIS_SHOULD_NOT_HAPPEN") instead

jbesraa · 2024-06-19T11:22:39Z

protocols/v2/binary-sv2/serde-sv2/src/primitives/sequences/option.rs

@@ -227,7 +239,15 @@ impl<'s> Sv2Option<'s, U256<'s>> {
                data: Some(data),
            }
        } else {
-            panic!()
+            // this is an already valid seq should be safe to call the unwraps.


why is this unwrap safe?

jbesraa · 2024-06-19T11:22:58Z

protocols/v2/binary-sv2/serde-sv2/src/primitives/sequences/option.rs

@@ -242,7 +262,14 @@ impl<'s> Sv2Option<'s, u32> {
                data: Some(inner),
            }
        } else {
-            panic!()
+            // this is an already valid seq should be safe to call the unwraps.


why is this unwrap safe?

Yes because if self.data is None then self.seq is Some(Inner) (it is the same as above!)

jbesraa · 2024-06-19T11:23:38Z

protocols/v2/binary-sv2/serde-sv2/src/primitives/sequences/seq0255.rs

-                seq.serialize_element(tuple.1)?;
-                seq.end()
+                if serializer.is_human_readable() {
+                    let data_ = self.data.clone().unwrap();


same, why not use data from match result?

my mistake!

jbesraa · 2024-06-19T11:24:14Z

protocols/v2/binary-sv2/serde-sv2/src/primitives/sequences/seq0255.rs

+            // this is an already valid seq should be safe to call the unwraps.
+            // also this library shouldn't be used for priduction envs so is ok do thigs like this
+            // one
+            let data = self.seq.unwrap().parse().unwrap();


why is this unwrap safe?

Same as above, if self.data is None then self.seq is Some()

Hm, that sounds risky. Here we also have two unwraps.
maybe something like this:

impl<'s> Seq0255<'s, u32> { pub fn into_static(self) -> Seq0255<'static, u32> { match (self.data, self.seq) { (Some(data),_) => { }, (_, Some(seq)) => { }, _ => { debug_assert!(false, "This is unreachable because `req` most be defined if `data` is `None`"); unreachable!(); } } } }

Also, I noticed that in both scenarios you provide data and never seq, is that intentional? also could we provide both if they exist?

Seq0255 { seq: None, data: Some(data), }

I believe because this library is used only for the MessageGenerator and the seq variant is never used in that case, but we should ask @Fi3 for that

we need data "variant" for it to be static we can't have a static seq

jbesraa · 2024-06-19T11:24:22Z

protocols/v2/binary-sv2/serde-sv2/src/primitives/sequences/seq0255.rs

+            // this is an already valid seq should be safe to call the unwraps.
+            // also this library shouldn't be used for priduction envs so is ok do thigs like this
+            // one
+            let data = self.seq.unwrap().parse().unwrap();


why is this unwrap safe?

same as above

lorbax · 2024-06-20T15:40:45Z

@lorbax did first initial pass. Also, could you please tide up the commits so you have a couple of detailed commits explaining the problem and the solution you are proposing?

Do you suggest to have just 2 commits? I don't think that it would be better as each commit describe a precise small change (except for fmt)

lorbax · 2024-06-20T16:55:55Z

@lorbax did first initial pass. Also, could you please tide up the commits so you have a couple of detailed commits explaining the problem and the solution you are proposing?

Tomorrow I will remove the fmt btw.
What do you suggest for apply the review? to just apply changes on older commits and force pushing or apply one review commit with a comment pointing to this PR? I would like to do it properly...

jbesraa · 2024-06-20T17:23:25Z

Thanks @lorbax for the answers. You can add as many commits as you like as long as they are reasonable(not fmt or typo fixes) and informative.

I prefer that you add f [INFO_ABOUT_FIX] commits and after review they can be squashed, but I am happy with either.

lorbax · 2024-06-20T17:25:20Z

Thanks @lorbax for the answers. You can add as many commits as you like as long as they are reasonable(not fmt or typo fixes) and informative.

I prefer that you add f [INFO_ABOUT_FIX] commits and after review they can be squashed, but I am happy with either.

Thank you!
Tomorrow I'll read in detail your replies. Thanks for your review!!!

jbesraa · 2024-06-24T16:27:38Z

@lorbax feel free to ping me when this is ready for another review

lorbax · 2024-06-24T17:08:55Z

@lorbax feel free to ping me when this is ready for another review

just wait @Fi3 to reply. Meanwhile I try to figure out some of the answers myself.
When it is ready ro be reviewed, do you want the commits to be squashed? If there won't be issues I can do it!

plebhash · 2024-06-25T19:14:57Z

roles/mining-proxy/config-examples/proxy-config-example.toml

@@ -1,5 +1,5 @@
 upstreams = [
-    { channel_kind = "Extended", address = "0.0.0.0", port = 34265, pub_key = "9auqWEzQDVyd2oe1JVGFLMLHZtCo2FFqZwtKA5gd9xbuEu7PH72"}
+    { channel_kind = "Extended", address = "0.0.0.0", port = 34254, pub_key = "9auqWEzQDVyd2oe1JVGFLMLHZtCo2FFqZwtKA5gd9xbuEu7PH72"}


why is this being changed here? seems totally unrelated to this PR

lorbax · 2024-07-08T17:34:05Z

@jbesraa took me a while, but now I have a better understanding of sequences datatypes in serde_sv2. They are a struct with two fields, one of type Option<Vec<T>> and the other Option<Seq<T>>. This struct behaves as an enum with data: Vec<T> and seq: Seq<T> variants. At the time of writing the code, it was written like this because was more practical for some reasons (that I don't know, haven't tried to replace it with a true enum).
At the beginning, serde_sv2 was equivalent to no_serde library for serialization and deserialization of datatypes in binary format. They had the same functionality and it was possible to use the application with serde by just importing some libraries with with_serde features (for having an idea of which of them imported like this, look at the Message Generator toml). Now I strongly suggest to do not use roles that import libraries with "with_serde" feature, they are not working anymore with that. Nevertheless, it is possible to use this libraries with "with_serde", and indeed it is the correct way to do this, because the MG it is meant to work with this feature.
Now I try to explain what I understood.
In MG test pool-sri-test-extended-1 there is an action that awaits the message NewExtendedMiningJob . This message contains the fields min_ntime and merkle_path which are resp a Sv2Option (which is a Seq0255 under the hood) and Seq0255 . Before deserialization these are represented with the seq: Some(T) and data: None. This is the tipical backtrace if we add a panic here
the call of the function parsers::TryFrom here will eventually panic.
Worth noting that deserialization is performed using the custom deserializer in binary_sv2::serde_sv2. I still have to understand why is_human_readable is used to distinguish when seq field or data field have to be used. I suspect that for serialization is_human_readable is true, and for deserialization is set to false (in serde_sv2::ser and serde_sv2::de). In this case, perhaps it may be possible to use a light version of serde_sv2 just for deserialization, and standard serde (with few modifications) for serialization.

@jbesraa

jbesraa · 2024-07-09T09:55:01Z

@lorbax thanks for digging into this! Maybe its worth breaking this pr to smaller ones if possible so you can get things merged while you are looking into the serde stuff

lorbax · 2024-07-09T10:01:59Z

@lorbax thanks for digging into this! Maybe its worth breaking this pr to smaller ones if possible so you can get things merged while you are looking into the serde stuff

the PR is ready as it is, I won't dig any longer for this serde stuff if not needed.
I am a bit reluctant to split it in smaller pieces, but if it gets not reviewed I will eventually do it.

jbesraa · 2024-07-09T10:16:40Z

It would be nice to change the commits to describe how/what/why its fixed rather than fix X, also I am not sure all comments are addressed? for example the pub Inner..

lorbax · 2024-07-09T14:17:40Z

It would be nice to change the commits to describe how/what/why its fixed rather than fix X, also I am not sure all comments are addressed? for example the pub Inner..

Now I changed the commit history, let me see it does look fine to you

some tests in main.rs of Message Generator are not passing or are commented. This is because serialization and/or deserialization of Sv2 primitives was broken. into_static for serde_sv2 primitives these functions were panicking if the seq field of sequences (Seq<T> or Sv2Option) were Some(...). most rust tests for MG are fixed, one is added

jbesraa · 2024-07-18T15:30:18Z

sorry for the delay here @lorbax, Ill review this in a bit

jbesraa

@lorbax Thanks for breaking this down.
I would appreciate if @plebhash can validate the MG part.

I wonder also if we could avoid some of the repeated code, for example this code (and a couple of others) is repeated across different files

      if serializer.is_human_readable() {
                    let data_ = self.data.clone().unwrap();
                    let mut seq = serializer.serialize_seq(Some(data_.len()))?;
                    for item in data_ {
                        seq.serialize_element(&item)?;
                    }
                    seq.end()
                } else {
                    let tuple = (data.len() as u16, &data[..]);
                    let mut seq = serializer.serialize_tuple(2)?;
                    seq.serialize_element(&tuple.0)?;
                    seq.serialize_element(tuple.1)?;
                    seq.end()
                }

I understand that those changes intended for MG but they live in protocols as public APIs, so I think we should probably handle all edge cases. Let me know if you think otherwise.

jbesraa · 2024-07-25T12:22:30Z

protocols/v2/binary-sv2/serde-sv2/src/primitives/sequences/mod.rs

+        let mut last = elem_len;
+        while last <= self.data.len() {
+            let slice = &self.data[first..last];
+            let elem = T::try_from_slice(slice).map_err(|_| ())?;


Having second thoughts about this. As it is exposed through the public API, it might make more sense to handle errors properly here.
Also, I have a small question:
If self.size is bigger than self.data.len, is returning empty array with self.size cap(the current behavior) makes sense or an error should be returned?

jbesraa · 2024-07-25T12:28:37Z

protocols/v2/binary-sv2/serde-sv2/src/primitives/sequences/option.rs

-                seq.serialize_element(tuple.1)?;
-                seq.end()
+                if serializer.is_human_readable() {
+                    let data_ = data.clone();


u can use masking and do let data = data.clone();. I think its more clear

you might also be able to remove this clone as serialize_element takes a reference

jbesraa · 2024-07-25T12:34:24Z

protocols/v2/binary-sv2/serde-sv2/src/primitives/sequences/option.rs

+            _ => {
+                debug_assert!(
+                    false,
+                    "sv2option can never have boh fields of the same type"


Is there an action expected from a user who hit this? regarding how to debug it? If is, its worth mentioning it IMO

jbesraa · 2024-07-25T12:35:42Z

protocols/v2/binary-sv2/serde-sv2/src/primitives/sequences/option.rs

-                data: Some(data),
+        match (self.data, self.seq) {
+            (None, Some(seq)) => {
+                let data = seq.parse().unwrap();


It would be useful to use .expect and explain why this should be Some

plebhash · 2024-07-28T22:05:43Z

utils/message-generator/src/main.rs

+        //dont clean the following commented code
+        /*
+        let mut bitcoind = os_command(
+            "./test/bin/bitcoind",
+            vec!["--regtest", "--datadir=./test/appdata/bitcoin_data/"],
+            ExternalCommandConditions::new_with_timer_secs(10)
+                .continue_if_std_out_have("sv2 thread start")
+                .fail_if_anything_on_std_err(),
+        )
+        .await;
+        let mut child = os_command(
+            "./test/bin/bitcoin-cli",
+            vec![
+                "--regtest",
+                "--datadir=./test/appdata/bitcoin_data/",
+                "generatetoaddress",
+                "16",
+                "bcrt1qttuwhmpa7a0ls5kr3ye6pjc24ng685jvdrksxx",
+            ],
+            ExternalCommandConditions::None,
+        )
+        .await;
+        child.unwrap().wait().await.unwrap();
+        */


why are we keeping these comments?

//dont clean the following commented code doesn't give any context

because can still be useful in future

maybe you want to change the:
//dont clean the following commented code
with: TODO fix the below tests

the first comment don't say anything about why you want to keep the below comment and people get confused.

plebhash · 2024-07-28T22:07:18Z

utils/message-generator/src/main.rs

+        // dont clean the following commented code
+        /*
+        let mut child = os_command(
+            "rm",
+            vec!["-rf", "./test/appdata/bitcoin_data/regtest"],
+            ExternalCommandConditions::None,
+        )
+        .await;
+        child.unwrap().wait().await.unwrap();
+        */


why are we keeping these comments?

//dont clean the following commented code doesn't give any context

because it_test_against_remote_endpoint is a test that I want to make it pass and not just ignore it. It is also true that there is nothing against to uncomment it and have it as "non-passing test"

same as above. comments are not memo for you.

lorbax · 2024-07-31T07:12:12Z

@jbesraa suggested me some time ago to split this PR in favor of smaller ones
now I see some comments on it, do we still want to split it or it is just fine as it is right now?
@Fi3, @plebhash

Fi3 · 2024-07-31T09:23:18Z

for me is ok also one PR

jbesraa · 2024-07-31T09:37:18Z

for me is ok also one PR

Where is the second PR?
I feel we are mixing MG stuff with protocol level stuff and iam not a big fan of that..

Edit: chatted with @lorbax and we have only this pr to review, which is fine with me.

lorbax · 2024-07-31T12:12:59Z

I recently saw that we have decided to close PR1068 because we want to replace the MG with something else for the CI. I don't see why this does not apply to this PR too, which is needed only for the MG. The library serde_sv2 works only with the MG (anything else is likely to be broken)

pavlenex · 2024-07-31T12:37:40Z

I recently saw that we have decided to close PR1068 because we want to replace the MG with something else for the CI. I don't see why this does not apply to this PR too, which is needed only for the MG. The library serde_sv2 works only with the MG (anything else is likely to be broken)

I wanted to ask this question as well, cc @jbesraa @plebhash Could we add it to the list #1077 and just close for now?

plebhash · 2024-07-31T12:53:21Z

I recently saw that we have decided to close PR1068 because we want to replace the MG with something else for the CI. I don't see why this does not apply to this PR too, which is needed only for the MG. The library serde_sv2 works only with the MG (anything else is likely to be broken)

I wanted to ask this question as well, cc @jbesraa @plebhash Could we add it to the list #1077 and just close for now?

#1041 is already keeping track of this, #1077 is tracking issues of a different nature

but yeah we can close for now

lorbax force-pushed the MG-fix-primitives-serialization-and-unit-tests branch from df10ddf to f0381eb Compare June 1, 2024 16:16

lorbax force-pushed the MG-fix-primitives-serialization-and-unit-tests branch 2 times, most recently from 98e89b6 to a74665a Compare June 1, 2024 16:49

lorbax changed the title ~~Message Generator: fix primitives serialization, fix broken unit tests, added unit test for (de)serialization of NMJ message~~ WIP Message Generator: fix primitives serialization, fix broken unit tests, added unit test for (de)serialization of NMJ message Jun 1, 2024

plebhash added this to the 1.0.2 milestone Jun 4, 2024

plebhash self-assigned this Jun 4, 2024

lorbax force-pushed the MG-fix-primitives-serialization-and-unit-tests branch 2 times, most recently from 9e48228 to 4e98733 Compare June 11, 2024 14:12

jbesraa reviewed Jun 19, 2024

View reviewed changes

lorbax force-pushed the MG-fix-primitives-serialization-and-unit-tests branch 2 times, most recently from 1b5bb89 to 836f6c7 Compare June 21, 2024 15:38

plebhash removed their assignment Jun 21, 2024

plebhash mentioned this pull request Jun 25, 2024

MG understands SubmitSolution as a valid JD message #1003

Merged

plebhash reviewed Jun 25, 2024

View reviewed changes

lorbax force-pushed the MG-fix-primitives-serialization-and-unit-tests branch from 836f6c7 to e38f853 Compare June 26, 2024 13:50

plebhash assigned jbesraa Jul 2, 2024

lorbax force-pushed the MG-fix-primitives-serialization-and-unit-tests branch from e38f853 to 3fca130 Compare July 5, 2024 10:50

lorbax mentioned this pull request Jul 9, 2024

BROKEN unity tests of MG stack #1041

Open

lorbax force-pushed the MG-fix-primitives-serialization-and-unit-tests branch from 3fca130 to 89e4360 Compare July 9, 2024 14:15

lorbax force-pushed the MG-fix-primitives-serialization-and-unit-tests branch from 89e4360 to 36da00e Compare July 9, 2024 14:18

lorbax force-pushed the MG-fix-primitives-serialization-and-unit-tests branch 2 times, most recently from 355d3b5 to 36da00e Compare July 11, 2024 10:02

plebhash requested a review from jbesraa July 15, 2024 17:47

pavlenex linked an issue Jul 16, 2024 that may be closed by this pull request

BROKEN unity tests of MG stack #1041

Open

jbesraa suggested changes Jul 25, 2024

View reviewed changes

plebhash reviewed Jul 28, 2024

View reviewed changes

plebhash mentioned this pull request Jul 28, 2024

move unrelated unit tests away from MG #1076

Open

lorbax closed this Aug 1, 2024

Message Generator: fix serialization and add into_static for serde_sv2 primitives, broken unit tests, new unit test for NMJ message, #949

Message Generator: fix serialization and add into_static for serde_sv2 primitives, broken unit tests, new unit test for NMJ message, #949

Conversation

lorbax commented Jun 1, 2024 • edited Loading

github-actions bot commented Jun 1, 2024 • edited Loading

Bencher

github-actions bot commented Jun 1, 2024 • edited Loading

Bencher

github-actions bot commented Jun 1, 2024 • edited Loading

Bencher

github-actions bot commented Jun 1, 2024 • edited Loading

Bencher

jbesraa left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorbax Jun 21, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorbax commented Jun 20, 2024

lorbax commented Jun 20, 2024

jbesraa commented Jun 20, 2024

lorbax commented Jun 20, 2024

jbesraa commented Jun 24, 2024

lorbax commented Jun 24, 2024

Choose a reason for hiding this comment

lorbax commented Jul 8, 2024 • edited Loading

jbesraa commented Jul 9, 2024

lorbax commented Jul 9, 2024

jbesraa commented Jul 9, 2024

lorbax commented Jul 9, 2024

jbesraa commented Jul 18, 2024

jbesraa 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

Choose a reason for hiding this comment

lorbax commented Jul 31, 2024

Fi3 commented Jul 31, 2024

jbesraa commented Jul 31, 2024 • edited Loading

lorbax commented Jul 31, 2024

pavlenex commented Jul 31, 2024

plebhash commented Jul 31, 2024 • edited Loading

lorbax commented Jun 1, 2024 •

edited

Loading

github-actions bot commented Jun 1, 2024 •

edited

Loading

github-actions bot commented Jun 1, 2024 •

edited

Loading

github-actions bot commented Jun 1, 2024 •

edited

Loading

github-actions bot commented Jun 1, 2024 •

edited

Loading

jbesraa left a comment •

edited

Loading

lorbax Jun 21, 2024 •

edited

Loading

lorbax commented Jul 8, 2024 •

edited

Loading

jbesraa commented Jul 31, 2024 •

edited

Loading

plebhash commented Jul 31, 2024 •

edited

Loading