GH 784 - Change Neighborhood DB to contain Free World Bit and Country code #444

czarte · 2024-05-17T16:06:44Z

No description provided.

…arnings in ip_country package

…nted out tests for 1000 ips

…e and free_world_bit in Gossip

…Record, NodeRecordInner_0v1, handle_new_public_ip

…Tests for NodeLocation

…tests for neigborhood_database

…deRecordInputs

… 5 seconds

…s down

ip_country/src/main.rs

ip_country/src/lib.rs

bertllll · 2024-07-09T23:08:30Z

multinode_integration_tests/src/utils.rs

@@ -138,6 +138,7 @@ impl From<&dyn MASQNode> for AccessibleGossipRecord {
                accepts_connections: masq_node.accepts_connections(),
                routes_data: masq_node.routes_data(),
                version: 0,
+                country_code: "".to_string(),


Shouldn't this always be some letters? Like "ZZ" at least. But as well as that, I wonder why you didn't add a new method to the trait MASQNode which would return the country code of that Node.
Therefore here we want to read that as if from inside the Node by using a getter method as you can see those others up here.

This has led me to a question, how would you prepare (or mock up) an exact country code for a Node in the test, because that's not a command line parameter, right? The only way, now, it can get set is to derive it from the ip provided or detected at this Node. However, we cannot just joggle with the ip, set by the inputs, because we need it to stay the correct one, set for the whole Docker container when started from an image. So, I think, we actually cannot have the Multinode test Real Nodes be using such a country code of a free choice. It will ever be a narrow range of IPs, the same one as we have to distinguish between every container with a Node.

Did you consider what country code it generates if you go and look at the standard IPs used by these controlled Nodes in the tests? It might be interesting to know. What country is it? Is there still a way to give it a special country code taking place only in multinode tests, similarly to how we work a special chain designed only for this environment. It may not doesn't make any sense here. I'm just sprinkling ideas so that you can reach new perspectives. Anyway, knowing what country code matches the containers' IPs might be good knowledge.

bertllll · 2024-07-09T23:51:12Z

multinode_integration_tests/tests/blockchain_interaction_test.rs

@@ -28,7 +28,7 @@ use node_lib::sub_lib::wallet::Wallet;
 #[test]


I hope you made sure on your computer that this test passes. I remember you trying so I believe you did. I'm saying so because it's a big weakness of this test that it does its job only if the developer doesn't forget to run the multinode tests in his dev environment. More than 90% of time we have no chances of an alarm ringing from this test as it sits down here just in silence and happy. :)

bertllll · 2024-07-12T16:59:24Z

ip_country/src/country_finder.rs

+    }
+
+    fn initialize_country_finder_ipv4(data: (Vec<u64>, usize)) -> Vec<CountryBlock> {
+        //TODO write two tests - one for deserializer and one for loop to fill up the vec


Have you made these tests??

Edit: I've seen some, even though you mixed all this together in a single test which is suboptimal (you can think about that too). At any rate, this comment needs to go away.

I made this test deserialize_country_blocks_ipv4_and_ipv6_adn_fill_vecs. So do there have to be two tests instead of one? I was designing this test to simulate the process of loading the CountryBock objects to memory on the Node startup. So we have a pretty close idea of how this deserialization will affect the Node startup.

ip_country/src/country_finder.rs

bertllll · 2024-07-12T17:14:44Z

ip_country/src/country_finder.rs

+        result
+    }
+
+    fn initialize_country_finder_ipv6(data: (Vec<u64>, usize)) -> Vec<CountryBlock> {


The same as previously.

However, I've got a sudden feeling that these two functions are just unwanted boilerplate.

You can inline their guts where you're calling them.

pub fn new(ipv4_data: (Vec<u64>, usize), ipv6_data: (Vec<u64>, usize)) -> Self { Self { ipv4: CountryBlockDeserializerIpv4::new(ipv4_data).collect_vec(), ipv6: CountryBlockDeserializerIpv6::new(ipv6_data).collect_vec() } }

🤷‍♂️

ip_country/src/country_finder.rs

node/src/blockchain/blockchain_bridge.rs

bertllll · 2024-07-12T18:37:35Z

node/src/neighborhood/overall_connection_status.rs

    )]
    fn can_not_create_a_new_connection_without_node_addr() {
        let descriptor_with_no_ip_address = NodeDescriptor {
            blockchain: Chain::EthRopsten,
            encryption_public_key: PublicKey::from(vec![0, 0, 0]),
            node_addr_opt: None,
+            country_code: "ZZ".to_string(),


Why does a descriptor need to contain the country code? I cannot understand. I actually think it's even wrong.
We need that descriptor just to contact another Node and this Node will respond with a NodeRecord about himself and there is the place where we should care about the country code for the first time.

It should probably go away from inside this structure.

bertllll · 2024-07-12T18:42:41Z

node/src/node_configurator/unprivileged_parse_args_configuration.rs

+        Ok(mode) => Ok(NeighborhoodConfig {
+            mode,
+            min_hops,
+            country: "ZZ".to_string(),


Why does this always say ZZ. It's hardcoded. Why do you need this field actually? You know about a place where we later modify it to something else? Even if there was such a weird place then this should have been an option.

I do think this is wrong. Probably a needless field.

node/src/sub_lib/cryptde.rs

node/src/sub_lib/node_addr.rs

bertllll · 2024-07-12T20:06:56Z

ip_country/src/countries.rs

+use std::collections::HashMap;
+
+lazy_static! {
+    pub static ref COUNTRIES: Vec<Country> = vec![


Think about how we could audit this list so that we can conclude the iso codes match the full country names correctly. I'm sure this has been test unprotected by a test.

Do we also want to fetch this information from the Internet and compare it with this list?

bertllll · 2024-07-12T20:25:15Z

ip_country/src/countries.rs

+use std::collections::HashMap;
+
+lazy_static! {
+    pub static ref COUNTRIES: Vec<Country> = vec![


After more reading, I found out that this list exceeds the count of countries reported on the Internet. They don't mention there would be any country with the position 250. They say the count is 249. Look at the indexes at our code here, they do end in 250 even if I make sure the sentinel value ZZ isn't counted in. That's slightly concerning. I'm afraid you will have to check the list against some other source you find somewhere and discover where the mismatch is.

bertllll · 2024-07-12T20:27:43Z

ip_country/src/countries.rs

+    }
+
+    #[test]
+    #[should_panic(expected = "There are only 251 Countries; no Country is at index 4096")]


This message isn't right. Correctly, you should display this number lowered by the sentinel item, and also, which is stated up in this file, the list is itself somehow biased. The number should be (250 - 1) = 249.

multinode_integration_tests/tests/bookkeeping_test.rs

node/src/neighborhood/mod.rs

bertllll · 2024-07-12T20:58:24Z

node/src/neighborhood/mod.rs

+                .as_ref()
+                .unwrap()
+                .country_code
+        );


This is a bit ridiculous. Curating metadata about ourselves. We don't need any metadata for our record really. 🤔

But can we do otherwise?

I think we constructing metadata for every node in the neighborhood, even the root node.

node/src/neighborhood/mod.rs

bertllll

Continue ip_country.rs ln. 97

ip_country/src/ip_country.rs

node/src/bootstrapper.rs

node/src/neighborhood/node_location.rs

node/src/bootstrapper.rs

node/src/neighborhood/node_record.rs

bertllll · 2024-07-16T17:00:39Z

node/src/neighborhood/node_record.rs

    ) -> NodeRecord {
+        let mut country = String::default();


As shown above, this should be rewritten as an optional value inside an Option.

bertllll · 2024-07-16T17:05:55Z

node/src/neighborhood/node_record.rs

@@ -321,6 +343,9 @@ pub struct NodeRecordMetadata {
    pub last_update: u32,
    pub node_addr_opt: Option<NodeAddr>,
    pub unreachable_hosts: HashSet<String>,
+    pub node_location_opt: Option<NodeLocation>,
+    pub node_distrust_score: u32,


What is this node_distrust_score about? I'm suspecting it to be unused or barely useful. Maybe you should just throw it away?

bertllll · 2024-07-16T17:19:11Z

node/src/neighborhood/node_record.rs

+        };
+        let node_record_data_duplicate = node_record_data.clone();
+        let node_record_data_with_neighbor = node_record_data.clone();
+        let node_record_data_mod_key = node_record_data.clone();


If I'd been you I would never have given names to these clones. In my opinion it's only distracting. You use different names even thought they are the same... You see why you thought it could be a good idea, but I think you should've only cloned the node_record_data at every place where it is required. No names, simple.

Of course, with the mutable ones it would have to work differently: there you can either keep what you have now or maybe better you could move the variable initialization as near as possible to the place where the variable is passed in the function in the next step.

node/src/neighborhood/node_record.rs

bertllll

Continue neighborhood_database.rs can_get_mutable_root, ln. 409.

bertllll · 2024-07-16T18:36:58Z

node/src/neighborhood/neighborhood_database.rs

-        let mut node_record = NodeRecord::new(
-            public_key,
+        let location = if let Some(node_addr) = neighborhood_mode.node_addr_opt() {
+            get_node_location(Some(node_addr.ip_addr))


I've noticed that you have to face this trouble quite often. You have an Option.
However, the function get_node_location is declared as taking only an Option. It can cause some clumsiness.

I recommend this signature for that function (and in the future a method):

fn get_node_location<IpAddress: AsRef<IpAddr>>(ip_opt: Option<IpAddress>){ // the body }

This should allow you to pass even the type NodeAddr directly. Because it will be dereferenced to IpAddr automatically.

To make it work as expected, you, however, need to implement the AsRef trait.

// pseudo code impl AsRef for Option<NodeAddr>{ type Output = Option<IpAddr> fn as_ref(&self)->Self::Output{ self.node_addr_opt() } }

Because we want the default behavior of Node without specified IP to work with ZZ country_code, I rewrote this part using the default() impl for NodeAddr, giving me ZZ.

bertllll · 2024-07-16T18:40:56Z

node/src/neighborhood/neighborhood_database.rs

+            version: 0,
+            location,
+        };
+        let mut node_record = NodeRecord::new(public_key, cryptde, node_record_data);


Here this has me thinking why did you introduce these NodeRecordInputs but did not include also the public key and cryptde, I believe it would be possible to do it. What have your arguments been like?

I took public_key and cryptde from node_record_data because of their lifetime. Do you think I should put it there and create that lifetime for the whole struct?

bertllll · 2024-07-16T22:42:20Z

node/src/neighborhood/neighborhood_database.rs

-        let mut node_record = NodeRecord::new(
-            public_key,
+        let location = if let Some(node_addr) = neighborhood_mode.node_addr_opt() {
+            get_node_location(Some(node_addr.ip_addr))


More importantly, why do we again compute the location of this Node if we probably did it already when working with the bootstrapper which uses the famous BoostrapperConfig and that is where I think you should've probably put this once already computed value into the part of the big aggregated structure of params where the field is called the neighborhood_config. This config is later given to the Neighborhood Actor and during its construction, where we probably also are building the NeighborhoodDatabase you could reuse the already known value instead of going through a repetition of this action.

That was actually my idea, to create the NodeLocation in bootstrapper. But we did not agree on where to store this NodeLocation.

node/src/neighborhood/neighborhood_database.rs

bertllll

Continue by test_ip_country_performance, ln. 123.

node/src/neighborhood/neighborhood_database.rs

bertllll · 2024-07-17T21:39:12Z

node/src/neighborhood/neighborhood_database.rs

+            country_code: "AU".to_string(),
+            free_world_bit: true,
+        });
+        this_node.resign();


As stated previously. No need for the green lines.

node/src/neighborhood/neighborhood_database.rs

node/src/neighborhood/node_location.rs

bertllll · 2024-07-17T22:30:22Z

node/src/neighborhood/node_location.rs

+            node.node_addr_opt().unwrap()
+        );
+        assert_eq!(node_record.inner.country_code, "US")
+    }


I think you forgot to assert the free world bit.

node/src/neighborhood/node_location.rs

bertllll · 2024-07-17T22:35:02Z

node/src/neighborhood/node_location.rs

+            country_code: country_record.1.clone(),
+            free_world_bit: country_record.2,
+        });
+        node.metadata.node_addr_opt = Some(NodeAddr::new(&country_record.0, &[8000 % 10000]));


What is the use for this &[8000 % 10000]? Isn't it really just 8000 % 10000 = 2000 and therefore why wouldn't you simply introduce 2000 without any indirection?

bertllll · 2024-07-18T17:29:00Z

node/src/neighborhood/node_location.rs

+}
+
+#[allow(dead_code, unused_imports)]
+#[cfg(not(test))]


Hmmm. These should be simply

#[cfg(test)]

but there is a bigger problem than that...

...because these test don't apply for "unit tests". And I'm resisting to pull them through some adjustments that I believe they need and then to call them "integration tests". I believe these either should be co called "ocassional tests" which is a special category that we have discussed multiple times, but was never implemented to this date. (Because it requires some thinking about our test tree in a bigger picture.) or it's an absolutely legitimate answer, I think, saying we don't need these anymore, as they can be understood more like spikes that gave a rough idea about the time requirements when we were wondering exactly about this, we might do that less in the future, because we now know the concept is bearable.

Therefore I'm suggesting you to bring this comment up in front of Dan and ask him of his decisive opinion.

node/src/neighborhood/node_location.rs

bertllll · 2024-07-18T17:52:30Z

node/src/neighborhood/node_location.rs

+        assert!(timeend.duration_since(timestart).unwrap().as_secs() < 20);
+        while !result_vec.is_empty() {
+            let location = result_vec.remove(0);
+            assert_eq!(location.unwrap().free_world_bit, true);


I still think we might end up deleting these tests but:

This is quite a vague assertion. You could at least also assert that we get some minimum number of same string entries by looking at the country code contained in the structure. You can fill a hashmap by it so that we allow for more than just one country to be found in case we are exercising the test across the boarder of international distribution of ip blocks. At the end, though, you simply assert that you have probably maximally two unique keys in the HashMap. Key = country code, value = occurrence. The occurrence should also give such a sum as 1000.

This isn't perfect but it might be somewhat better than nothing.

bertllll · 2024-07-18T17:54:41Z

node/src/neighborhood/node_location.rs

+    }
+
+    #[test]
+    fn get_node_location_for_test_with_1000_v6_middle_val_ips() {


I still think we might end up deleting these tests but:

This is a bit unjust because the IPV4 was tested in high values but here you're only after middle values. Maybe you should perform such a test on the high values instead but still on IPV6.

node/src/neighborhood/node_location.rs

node/src/test_utils/neighborhood_test_utils.rs

bertllll · 2024-07-19T16:31:44Z

node/src/test_utils/neighborhood_test_utils.rs

@@ -156,15 +207,20 @@ impl NodeRecord {
        base_rate: u64,
        accepts_connections: bool,
        routes_data: bool,
+        node_location: Option<NodeLocation>,


node_location -> node_location_opt

node/src/test_utils/neighborhood_test_utils.rs

bertllll · 2024-07-19T16:50:54Z

node/tests/ui_gateway_test.rs

@@ -162,6 +166,13 @@ fn daemon_does_not_allow_node_to_keep_his_client_alive_integration() {
    let assertion_lookup_pattern_2 =
        |_port_spec_ui: &str| "Received shutdown order from client 1".to_string();
    let second_port = connected_and_disconnected_assertion(2, assertion_lookup_pattern_2);
+    //TODO Create card to Change infrastructure and add node_client.send here and it should return result - we send the suhtdown request in loop until it not returns


a) Have you made that card? You probably should. And this comment should contain its reference number.

b) If you decided not to tie yourself up with sketching the card, we can at least improve the wording:
//TODO make a card to create a test utility with a node_client.send and keep sending the shutdown request in a loop until the Node doesn't response anymore.

The problem is, I remember, that the shutdown "conversation" is impaired and the Node, despite the usual believes, only absorbs the request but "forgets" to send any response back to the client. If my suspicion is right then, of course, you will run into troubles with the suggested solution. Maybe the card actually should be made right now and this note should become a part of it.

bertllll · 2024-07-19T16:55:00Z

node/tests/ui_gateway_test.rs

+    //TODO Create card to Change infrastructure and add node_client.send here and it should return result - we send the suhtdown request in loop until it not returns
+    loop {
+        if let Ok(_stream) = TcpStream::connect(format!("127.0.0.1:{}", ui_redirect.port)) {
+        } else {


My feeling is that the empty body of the "Ok" branch makes this loop too tight. I'd rather see a short sleep for this thread, even a few millis long so that the processor can progress on some other job in the meantime.

bertllll · 2024-07-19T18:22:32Z

node/tests/utils.rs

-            ensure_node_home_directory_exists("integration", test_name)
-        } else {
-            node_home_directory("integration", test_name)
+        running_test();


Do you know why you put this line in here? I can't come up with a good argument for keeping it. Please take it seriously and think it out thoroughly. If you have troubles to believe it is needed you probably should remove it.

bertllll · 2024-07-19T18:23:45Z

node/src/sub_lib/neighborhood.rs

@@ -162,6 +163,7 @@ pub struct NodeDescriptor {
    pub blockchain: Chain,
    pub encryption_public_key: PublicKey,
    pub node_addr_opt: Option<NodeAddr>,
+    pub country_code: String,


Still not sure why it belongs here. Hopefully you are gonna come to discuss it with Dan together.

bertllll · 2024-07-19T18:25:05Z

node/src/sub_lib/neighborhood.rs

        }
    }
 }

 impl From<(&NodeRecord, Chain, &dyn CryptDE)> for NodeDescriptor {
    fn from(tuple: (&NodeRecord, Chain, &dyn CryptDE)) -> Self {
        let (node_record, blockchain, cryptde) = tuple;
+        let ip_addr = match node_record.node_addr_opt() {


ip_addr -> ip_addr_opt

node/src/neighborhood/gossip_acceptor.rs

ip_country/src/country_block_stream.rs

bertllll · 2024-07-20T20:19:10Z

ip_country/src/country_block_stream.rs

+    // can reuse guts of contains for before and after and then replace arms of match witch calls to those fns
+    pub fn contains(&self, ip_addr: IpAddr) -> bool {
+        match self {
+            IpRange::V4(begin, end) => match ip_addr {


Not sure but maybe the word begin should be exchanged for either "beginning" or "start" because "begin" only works as a verb and I don't think you meant it so.

ip_country/src/country_block_stream.rs

bertllll · 2024-07-20T20:31:55Z

ip_country/src/country_block_stream.rs

+
+    pub fn in_range(&self, ip_addr: IpAddr) -> Ordering {
+        match (ip_addr, self) {
+            (IpAddr::V4(ip), IpRange::V4(low, hi)) => {


I'd prefer if you could rename the variable from hi to high with that at all places through this function.
Alternatively, you can pick the same names you placed for the containes function: beginning (or start) and end.

ip_country/src/country_block_stream.rs

bertllll · 2024-07-20T21:09:07Z

ip_country/src/country_block_stream.rs

+        }
+    }
+
+    pub fn in_range(&self, ip_addr: IpAddr) -> Ordering {


This name doesn't fits. It evokes that this function should return a boolean, answering a question like "Is this IP in this range". Maybe range_related_ordering. These names are tough. Grrr.

bertllll · 2024-07-20T21:16:08Z

ip_country/src/country_block_stream.rs

+                let (low_range, hi_range) = (u128::from(*low), u128::from(*hi));
+                let ip_num = u128::from(ip);
+                if ip_num < low_range {
+                    Ordering::Greater


Why do I have a feeling that these comparison operators are misused here?
Go and check with me:
ip_num < low_end can be read as "if our examined IP is smaller than the smallest IP in the range", and we should logically conclude "then the ordering is 'Less'", however you say the opposite, that is "Greater".

I think this counter-intuitive approach should be changed towards what I depicted, it shouldn't stay as it is.

ip_country/src/country_block_stream.rs

bertllll · 2024-07-20T21:29:22Z

ip_country/src/country_block_stream.rs

+            }
+            Ok(ip) => ip,
+        };
+        Ok(ip_addr)


You can rewrite this without the second return clause. Just don't create the ip_addr variable and return implicitly by leaving the match stm.

bertllll · 2024-07-20T21:33:14Z

ip_country/src/country_block_stream.rs

+
+    fn try_from(string_record: StringRecord) -> Result<Self, Self::Error> {
+        let mut iter = string_record.iter();
+        let start_ip = Self::ip_addr_from_iter(&mut iter)?;


rename please ip_addr_from_iter to something like read_next_ip_addr_from_iter

bertllll · 2024-07-20T21:37:44Z

ip_country/src/country_block_stream.rs

+        Ok(ip_addr)
+    }
+
+    fn validate_ip_addresses(start_ip: IpAddr, end_ip: IpAddr) -> Result<(), String> {


Maybe rather validate_ip_range_ending_values

ip_country/src/country_block_stream.rs

bertllll · 2024-07-21T16:45:02Z

ip_country/src/bit_queue.rs

+        }
+        let initial_bits_added = self.add_some_back_bits(bit_data, bit_count);
+        bit_data >>= initial_bits_added;
+        bit_count -= initial_bits_added;


This could be a closure called three times in here.

let compensate_added_bits = |number_of_bits_added|{ bit_data >>= number_of_bits_added; bit_count -= number_of_bits_added; };

...and use it like this:

let initial_bits_added = self.add_some_back_bits(bit_data, bit_count); compensate_added_bits(initial_bits_added); let byte_bits_added = self.add_back_bytes(bit_data, bit_count); compensate_added_bits(byte_bits_added); let final_bits_added = self.add_some_back_bits(bit_data, bit_count); compensate_added_bits(final_bits_added); if bit_count != 0 { panic!("Didn't add all the bits: {} left", bit_count); }

bertllll · 2024-07-21T16:53:26Z

ip_country/src/bit_queue.rs

+        let mut bit_data = 0u64;
+        let mut bit_position = 0usize;
+        let (initial_bit_data, initial_bit_count) = self.take_some_front_bits(bit_count);
+        bit_data |= initial_bit_data << bit_position;


This << bit_position operation is probably unnecessary since the bit_position always equals 0 at this moment and bit shift by 0 does nothing.

You can ask Dan anyway to avoid issues if I'm a moron :-)

I put in the << bit_position for symmetry with the other two operations. Bert's right: it's completely cosmetic. If you don't like the way it looks, you can take it out: your decision.

ip_country/src/bit_queue.rs

bertllll · 2024-07-22T10:18:07Z

ip_country/src/country_block_serde.rs

+
+Since every start address is composed of differences from the address before it, the very first
+start address is compared against 255.255.255.254 for IPv4 and
+FFFF:FFFF:FFFF:FFFF:FFFF:FFFF:FFFF:FFFE for IPv6.


I don't think this paragraph is true.

When I compare with a paragraph that starts at the line 20 and that I cite here:

Each block is assumed to end at the address immediately before the one where the next block starts.
If the data contains no block starting at the lowest possible address (0.0.0.0 for IPv4,
0:0:0:0:0:0:0:0 for IPv6), the deserializer will invent one starting at that address, ending just
before the first address specified in the data, and having country index 0. The last block ends at
the maximum possible address: 255.255.255.255 for IPv4, FFFF:FFFF:FFFF:FFFF:FFFF:FFFF:FFFF:FFFF
for IPv6.,

these two are in contradiction and I think only the higher situated one should stay in.

If you aren't sure feel free to consult with Dan.

bertllll · 2024-07-22T19:45:12Z

ip_country/src/bit_queue.rs

+        if (self.front_blank_bit_count == 8) && (self.byte_queue.len() > 2) {
+            let _ = self.byte_queue.pop_front();
+            self.front_blank_bit_count = 0;
+        }


If you rewrite lines 186 - 189

if (self.front_blank_bit_count == 8) && (self.byte_queue.len() > 2) { let _ = self.byte_queue.pop_front(); self.front_blank_bit_count = 0; }

into

if (self.front_full_bit_count() == 0) && (self.byte_queue.len() > 2) { let _ = self.byte_queue.pop_front(); self.front_blank_bit_count = 0; }

then you get the same code which is across lines 167 - 170,

and therefore you can make a method or a closure containing those and call it twice in this big function. The question is what name it should take on. Maybe ditch_leading_blank_byte_if_needed.

Moreover, there is an opportunity to have an even more universal function with different specific implementations:

fn ditch_leading_blank_byte_if_needed(&self, extra_condition: bool){ if (self.front_full_bit_count() == 0) && extra_condition { let _ = self.byte_queue.pop_front(); self.front_blank_bit_count = 0; } }

which is intended as: // for fn take_some_front_bits self.ditch_leading_blank_byte_if_needed(self.byte_queue.len() > 2); // for fn take_some_front_bytes self.ditch_leading_blank_byte_if_needed(true);

bertllll · 2024-07-22T20:22:07Z

ip_country/src/bit_queue.rs

+            *front_ref >> bits_to_take
+        } else {
+            0
+        };


Lines 175 - 184 should probably make an existence to a separate function. Something like extract_bits_from_the_front_byte.

bertllll · 2024-07-22T20:23:32Z

ip_country/src/bit_queue.rs

+            let _ = self.byte_queue.pop_front();
+            self.front_blank_bit_count = 0;
+        }
+        return (bit_data as u64, bits_to_take);


The return word (and then also the semicolon) is redundant. In Rust its use this way is considered unidiomatic.

bertllll · 2024-07-22T20:29:48Z

ip_country/src/bit_queue.rs

+        }
+        let mut bit_data = 0u64;
+        let mut bit_position = 0usize;
+        let (initial_bit_data, initial_bit_count) = self.take_some_front_bits(bit_count);


I'd like if you added some blank lines for better readability.
Those are at least these after which I'd place the space: 59, 63, 67, 73

bertllll · 2024-07-22T20:35:22Z

ip_country/src/bit_queue.rs

+        let (byte_bit_data, byte_bit_count) = self.take_front_bytes(bit_count);
+        bit_data |= byte_bit_data << bit_position;
+        bit_position += byte_bit_count;
+        bit_count -= byte_bit_count;


Even take_front_bytes is written so that it may return (0, 0). Therefore I'd expect what we can see below, the if statement, that would look like if byte_bit_count > 0 for this case.

I recommend a consultation with Dan.

Again, Bert is right. These four cases should be treated the same. Putting in the if after the first take_some_front_bits() and the take_front_bytes() would make all four cases look the same. Removing the ifs after the second take_some_front_bits() and take_some_back_bits() would also make all four cases look the same. None of the if statements are strictly necessary. In the (0, 0) case, the if statement will save a little time; in every other case it will add a little time.

ip_country/src/bit_queue.rs

bertllll · 2024-07-22T23:47:19Z

ip_country/src/bit_queue.rs

+            bit_data |= final_back_bit_data << bit_position;
+            bit_position += final_back_bit_count;
+        }
+        bit_count -= final_back_bit_count;


The lines 68 - 73 and 74 - 79 can be factored out as:

fn final_bits_extraction( take_some_bits: fn(&BitQueue) -> (u64,usize), bit_data: &mut u64, bit_count: &mut usize, bit_position: &mut usize) { let (final_back_bit_data, final_back_bit_count) = self.take_some_back_bits(bit_count); if final_back_bit_count > 0 { bit_data |= final_back_bit_data << bit_position; bit_position += final_back_bit_count; } bit_count -= final_back_bit_count; }

(Maybe you will have to play around with to find out how a valid reference to that function taking bits can be arranged. I used a "function pointer" style that implies a pure function call without capturing any context, so that is not closure-compatible. Maybe it's not gonna work with exactly this syntax but it feels close. You don't hesitate to ask me for help to make this work or to convert it to more a generic concept, the Fn trait. I suspect the compiler could come to argue about the lifetime at the reference before the BitQueue value. I might know what needs to be done for peace out there. )

You'll make use of this generic fn like this (or I suppose it a working solution):

let ( final_front_bit_data, final_front_bit_count ) = final_bits_extraction( self.take_some_front_bits, &mut bit_data, &mut bit_count, &mut bit_position );

ip_country/src/bit_queue.rs

bertllll · 2024-07-23T14:41:58Z

ip_country/src/bit_queue.rs

+            .back_mut()
+            .expect("There should be a back byte");
+        *back_ref |= (bit_data << back_full_bit_count) as u8;
+        self.back_blank_bit_count -= bits_to_add;


Lines 121 - 128 could be an outboard method called try_completing_last_byte.

bertllll · 2024-07-23T14:43:33Z

ip_country/src/bit_queue.rs

+        if self.back_blank_bit_count == 0 {
+            self.byte_queue.push_back(0);
+            self.back_blank_bit_count = 8;
+        }


These lines repeat as well.

lns. 117 - 120

lns. 129 - 132

It should probably be a special method.

bertllll · 2024-07-23T18:36:46Z

ip_country/src/bit_queue.rs

+        if self.back_blank_bit_count == 8 {
+            let _ = self.byte_queue.pop_back();
+            self.back_blank_bit_count = 0;
+        }


This is something Dan should be ask to comment. My feeling is this operation (lines 147 - 150) may not make sense when you consider it together with the operation (lines 129 - 132) from the fn add_some_back_bits. We should also clarify that the higher function calling these does so in this order: add_some_back_bits and without anything between, affecting the state, then add_back_bytes.

Now, if we come to think of what the two operations do you will eventually conclude that do the same but reversely. Therefore calling them sequentially has a neutral effect, they even out each other, and the aftermath is the same as if we deleted them both.

I'd like to keep this as a bit of defensive programming. Right now, we only ever call this function after calling add_some_back_bits(), but that might not always be true.

ip_country/src/bit_queue.rs

bertllll · 2024-07-23T21:08:57Z

ip_country/src/bit_queue.rs

+        (self.byte_queue.len() * 8) - self.back_blank_bit_count - self.front_blank_bit_count
+    }
+
+    #[allow(unused_assignments)]


Honestly, I don't know why this is a thing. Can you confirm the lint treats an issue which is still somewhere here?

Regarding how you can proceed, you can try compiling this crate by its ci/all.sh or maybe ci/unit_tests.sh, ideally when you've completed the job on the other comments and now you have quite clean code. Comment out the lint and try compiling again. If it throws out a warning (and probably kills the build) then it may still be necessary. On the other hand, it could also hint you how to resolve this by enhancement of the code itself, if you're lucky to meet Clippy and its wittiness.

If you've embarked on this then maybe scroll right away down to line 46 and test it on as well as you did here.

bertllll · 2024-07-24T10:18:06Z

ip_country/src/country_block_serde.rs

+        self.bit_queue_ipv6.add_bits(country_index as u64, 9);
+        self.prev_start_ipv6 = start;
+        self.prev_end_ipv6 = end;
+    }


Well, well, well...

These functions are good, at least I cannot see issues in the functionality. However, they always have almost identical twins, one for ipv4, another for ipv6. When you look closely it's an awful amount of duplicated code, not only above this comment, but also below.

That's a reason I do think this will require some care, possibly traitifying the tiny basic-block methods that make the only differences out here factually.

I've paid some overhead in my energy to receive good enough examples for you of how that can worked out. My belief is the following rearchitecture should be quite close to being able to compile. I haven't had a chance to try out. This is all just from rough sketching in my IDE.

My goal stood as making as much code as possible work generically, plus introduction of multiple auxiliary traits which are meant for the places where the implementations for Ipv4 and Ipv6 factually differ, because such must be being solved separately. I tried to isolate these code sections to be the smallest possible, and the rest of the code was planned to become common for both.

It kinda turned out a good idea am I ever the one who can give an opinion on my own work.

I strived to provide the full skeleton of what the rearchitecture calls for, sometimes even filled in with the actual code, sometimes empty and containing only todo!(). Those wait for you of course, but they either have been exercised just for a single implementation or should be fairly easy to figure out. There are also comments which I think should all go eventually away. They are there just to help you perhaps.

Both the CountryBlockSerializer and CountryBlockDeserialiser have been grabbed and only the IP-version-specific code was factored out. Therefore you have just one struct that can be universally used for both IP versions according to what type of IP you stuff it up with.

I don't know what you (or other guys) think but I see the reduction of duplication outbalance the modest increase in syntax complexity caused mainly by the nature of generic code. Hopefully you're gonna find a good way to make most of this proposed code, properly plugged in, as it gave me a hard time, without letting me progress on the review awhile.

Thanks for listening to my advice about refactoring 🙏

pub struct CountryBlockSerializer { ipv4: VersionedIPSerializer<Ipv4Addr>, ipv6: VersionedIPSerializer<Ipv6Addr> } struct VersionedIPSerializer<VersionedIP>{ prev_start: VersionedIP, prev_end: VersionedIP, bit_queue: BitQueue, } pub trait Serializer<VersionedIP>{ fn differences(from: VersionedIP, to: VersionedIP) -> Vec<Difference> where Self: Sized; fn add_bits(&mut self, bit_data: u64, bit_count: usize); } impl Serializer for VersionedIPSerializer<Ipv6Addr>{ fn differences(from: Ipv6Addr, to: Ipv6Addr) -> Vec<Difference> where Self: Sized{ todo!() } fn add_bits(&mut self, bit_data: u64, bit_count: usize){ todo!() } } impl Serializer for VersionedIPSerializer<Ipv4Addr>{ fn differences(from: Ipv4Addr, to: Ipv4Addr) -> Vec<Difference> where Self: Sized{ todo!() } fn add_bits(&mut self, bit_data: u64, bit_count: usize){ todo!() } } // Here, I'd rather see a private declaration to prevent this implementation from escaping this // local namespace while it's going to be used for types that have much wider usage than that but // this method will be useless elsewhere trait PlusMinusOneIP { fn plus_one_ip(ip: Self) -> Self; fn minus_one_ip(ip: Self) -> Self; } impl PlusMinusOneIP for Ipv4Addr{ fn plus_one_ip(ip: Self) -> Self{ todo!() } fn minus_one_ip(ip: Self) -> Self{ todo!() } } impl PlusMinusOneIP for Ipv6Addr{ fn plus_one_ip(ip: Self) -> Self { todo!() } fn minus_one_ip(ip: Self) -> Self { todo!() } } impl <VersionedIP> VersionedIPSerializer<VersionedIP> where VersionedIP: PlusMinusOneIP { fn add_ip(&mut self, start: VersionedIP, end: VersionedIP, country_index: usize) { let expected_start = VersionedIP::plus_one_ip(self.prev_end_ipv4); if start != expected_start { self.add_ipv4(expected_start, VersionedIP::minus_one_ip(start), 0) } let differences = Self::differences(self.prev_start_ip(), start); let difference_count_minus_one = (differences.len() - 1) as u64; self.add_bits(difference_count_minus_one, 2); differences.into_iter().for_each(|difference| { self.add_bits(difference.index as u64, 2); self.add_bits(difference.value, 8); }); self.add_bits(country_index as u64, 9); self.set_prev_start_ip(start); self.set_prev_end_ip(end); } fn prev_start_ip(&self) -> Self::VersionedIp{ self.prev_start } fn prev_end_ip(&self) -> Self::VersionedIp{ self.prev_end } fn set_prev_start_ip(&mut self, start_ip: Self::VersionedIp){ self.prev_start = start_ip } fn set_prev_end_ip(&mut self, end_ip: Self::VersionedIp){ self.prev_end = end_ip } } impl <VersionedIP, SegmentBitRep> VersionedIPSerializer<VersionedIP> where VersionedIP: IPIntoSegments, SegmentBitRep: PartialEq, u64: From<SegmentBitRep> { fn differences(from: VersionedIP, to: VersionedIP) -> Vec<Difference> { let pairs = from.segments().into_iter().zip(to.segments().into_iter()); pairs .enumerate() .flat_map(|(index, (from_segment, to_segment)): (_,(SegmentBitRep, SegmentBitRep))| { if to_segment == from_segment { None } else { Some(Difference { index, value: u64::from(to_segment), }) } }) .collect() } } trait IPIntoSegments<BitsPerSegment, const SegmentsCount: usize>{ fn segments(&self)->[BitsPerSegment;SegmentsCount]; } impl IPIntoSegments<u8, 4> for Ipv4Addr{ fn segments(&self) -> [u8;4] { self.octets() } } impl IPIntoSegments<u16, 8> for Ipv6Addr{ fn segments(&self) -> [u16;8] { self.segments() } } // Now the implementation on the deserialization follows #[derive(Debug)] pub struct CountryBlockDeserializer<VersionedIP> where VersionedIP: Debug { prev_record: StreamRecord<VersionedIP>, bit_queue: BitQueue, empty: bool, } pub trait Deserializer<VersionedIP> where VersionedIP: IPIntoSegments, Self: Sized{ fn get_record(bit_queue: &mut BitQueue, prev_start: VersionedIP)->Option<StreamRecord<VersionedIP>>{ let mut segments = prev_start.segments(); let difference_count = Self::read_difference_count(bit_queue); let differences = Self::read_differences(); if differences.len() < difference_count { return None; } StreamRecord::<VersionedIP>::new(differences, segments.to_vec()) } fn max_ip_value()->VersionedIP; fn read_difference_count(bit_queue: &mut BitQueue) -> usize; fn read_differences()->Vec<Difference>; } // A full example of implementation for Ipv4Addr impl Deserializer<Ipv4Addr> for CountryBlockDeserializer<Ipv4Addr>{ fn read_difference_count(bit_queue: &mut BitQueue) -> usize { (&self.bit_queue.take_bits(2)? + 1) as usize } fn read_differences()->Vec<Difference>{Self::read_differences_generic(2,8)} fn max_ip_value() -> Ipv4Addr { Ipv4Addr::new(255, 255, 255, 255) } } impl Deserializer<Ipv6Addr> for CountryBlockDeserializer<Ipv6Addr>{ fn read_difference_count(bit_queue: &mut BitQueue) -> usize { todo!() } fn read_differences() -> Vec<Difference> { todo!() } fn max_ip_value() -> Ipv6Addr { todo!() } } impl <VersionedIP> CountryBlockDeserializer<VersionedIP> where VersionedIP: PlusMinusOneIP, IpRange: From<(VersionedIP, VersionedIP)>, { fn next(&mut self) -> Option<CountryBlock> { if self.empty { return None; } let next_record_opt = Self::get_record(&mut self.bit_queue, self.prev_record.start); match next_record_opt { Some(next_record) => { let prev_block = CountryBlock { ip_range: IpRange::from(( self.prev_record.start, VersionedIP::minus_one_ip(next_record.start), )), country: Country::from(self.prev_record.country_idx), }; self.prev_record = next_record; Some(prev_block) } None => { self.empty = true; Some(CountryBlock { ip_range: IpRange::from(( self.prev_record.start, Ipv4Addr::new(255, 255, 255, 255), )), country: Country::from(self.prev_record.country_idx), }) } } } fn read_differences_generic(difference_count: usize, index_bit_count: u64, value_bit_count: u64) -> Vec<Difference>{ (0..difference_count) .map(|_| { Some(Difference { index: bit_queue.take_bits(index_bit_count)? as usize, value: bit_queue.take_bits(value_bit_count)?, }) }) .flatten() .collect() } } impl CountryBlockDeserializer<Ipv4Addr>{ pub fn new(country_data_ipv4: (Vec<u64>, usize)) -> Self { let mut bit_queue = bit_queue_from_country_data(country_data_ipv4); let prev_record = Self::get_record(&mut bit_queue, Ipv4Addr::new(255, 255, 255, 254)) .expect("Empty BitQueue"); Self { prev_record, bit_queue, empty: false, } } } #[derive(Debug)] struct StreamRecord<VersionedIP> where VersionedIP: Debug { start: VersionedIP, country_idx: usize, } impl <VersionedIP, SegmentBitRep, const SegmentsCount: usize> StreamRecord<VersionedIP> where VersionedIP: From<Segments>, u64: From<SegmentBitRep>, { fn new(differences: Vec<Difference>, segments: [SegmentBitRep;SegmentsCount]) -> Option<StreamRecord<VersionedIP>>{ differences .into_iter() .for_each(|d| segments[d.index] = SegmentBiteRep::from(d.value)); Some(StreamRecordIpv4 { start: VersionedIP::from(segments), country_idx: bit_queue.take_bits(9)? as usize, }) } } // Each of these from impls needs a test impl From<(Ipv4Addr, Ipv4Addr)> for IpRange { fn from((start, end): (Ipv4Addr, Ipv4Addr)) -> Self { todo!() } } impl From<(Ipv6Addr, Ipv6Addr)> for IpRange { fn from((start, end): (Ipv6Addr, Ipv6Addr)) -> Self { todo!() } }

This code is not 100% of what should reside in this file. As some functions would remain intact, I left them out from my suggested design. For example bit_queue_from_country_data.

Operate carefully, because my code is often intended to replace some existing code, including standalone functions etc. Those need to be identified and erased for no longer being useful.

bertllll · 2024-07-24T20:28:48Z

ip_country/src/country_block_serde.rs

+    fn differences_ipv4(from: Ipv4Addr, to: Ipv4Addr) -> Vec<Difference> {
+        let pairs = from.octets().into_iter().zip(to.octets().into_iter());
+        pairs
+            .into_iter()


This .into_iter() might be redundant. If nothing has changed without my awareness, the zip function above produces an iterator which you can pick up on directly with the tool set of standard methods for iterators, such as enumerate, that is used here.

However, as it's been shown in the examples above of the big rearrangement that is favourable for the whole Serializer, I've rewritten the duplicated differences function already.

bertllll · 2024-07-27T17:02:39Z

ip_country/src/country_block_serde.rs

+            .flatten()
+            .collect::<Vec<Difference>>();
+        if differences.len() < difference_count {
+            return None;


This is disputable?!

It could also be regarded as a corrupted record in the generated source file with country blocks. Please ask Dan if he is 100% sure we don't wanna catch this issue while we can. It basically says ignore every record that is wrong this way and when we come and receive the generated file we'd have no idea that some country blocks are missing.

bertllll

Continue country_block_serde.rs from the beginning of the tests

bertllll · 2024-07-29T20:17:56Z

ip_country/src/country_block_serde.rs

+                    Ipv4Addr::from_str("1.2.3.4").unwrap(),
+                    Ipv4Addr::from_str("1.2.3.5").unwrap(),
+                ),
+                country: Country::try_from("AS").unwrap().clone(),


I don't think that any of these clones need to take place (3 in pv4_country_blocks and 3 in pv6_country_blocks). You are creating owned values from a thin air and once you have it you are cloning it as if one piece wouldn't be enough.

bertllll · 2024-07-29T21:30:20Z

ip_country/src/country_block_serde.rs

+                bit_queue.take_bits(2).unwrap(),
+                bit_queue.take_bits(8).unwrap(),
+                bit_queue.take_bits(9).unwrap(),
+            );


This block of take_bits(X) calls is noticeably repeating throughout this test. A standalone function to be called multiple times is the way to go.

However, to make the readability even better it's advisable to come up with an auxiliary struct that can hold all the values in its named fields.

First the holder:

fn bit_assert_country_blocks_for_ipv4( bit_queue: &mut BitQueue, expected_difference_count_minus_one: u64, expected_differences: Vec<(u64, u64)>, expected_country_code: &str { let difference_count_minus_one = bit_queue.take_bits(2).unwrap(); let differences = (0..difference_count_minus_one) .map(|_|{ let index = bit_queue.take_bits(2).unwrap(); let value = bit_queue.take_bits(8).unwrap(); (index, value) }) .collect::<Vec<(u64, u64)>>(); let country_index = bite_queue.take_bits(9).unwrap(); let country_code = Country::from(country_index as usize).iso3166; assert_eq!( difference_count_minus_one, expected_difference_count_minus_one, "We expected difference count {} but found {}", difference_count_minus_one, expected_difference_count_minus_one ); assert_eq!( differences, expected_differences, "We expected these differences (<index>, <value>) from the previous block {:?} but found {:?}", differences, expected_differences, ) assert_eq!( country_code, expected_country_code, "We expected country code {} but found {}", country_code, expected_country_code, ) }

Once you have such a function ready, using it would look as follows (this example matches the first body of assertions in this test):

assert_country_blocks_from_read_bits(3, vec![(0, 0), (1, 0), (2, 0), (3, 0)], "ZZ");

For those records where there are a fewer differences it would certainly look different. The number of
elements in the vector of expected index-value pairs would correspond to the number of factual differences. (Never less than one and more than four pairs). It's your responsibility to prepare that vector
correctly for the auxiliary function being discussed.

bertllll · 2024-07-29T22:00:09Z

ip_country/src/country_block_serde.rs

+        }
+        let remaining_bit_count = bit_queue.len();
+        let data = bit_queue.take_bits(remaining_bit_count).unwrap();
+        bit_data.push(data);


You can place the lines 710 - 723 inside an outboard method that takes only one parameter which is a function pointer fn()->Vec or closure Fn() -> Vec. Then you use that function with two different values for that argument, one case in each test in next_works_for_ipv4 and next_works_for_ipv6.

your_outboard_method(ipv4_country_blocks); // or your_outboard_method(ipv6_country_blocks);

bertllll · 2024-07-29T22:03:22Z

ip_country/src/country_block_serde.rs

+        }
+        let remaining_bit_count = bitqueue.len();
+        let data = bitqueue.take_bits(remaining_bit_count).unwrap();
+        vec_64.push(data);


You can reuse the same outboard method from the previous comment also here: lines 792 - 801.

bertllll · 2024-07-29T22:09:44Z

ip_country/src/country_block_serde.rs

+    }
+
+    #[test]
+    fn add_works_for_ipv6() {


Now go back to the ip4 equivalent of this test. I want you to refactor this one the same way I showed you for the first one.

You must reinvent a function like bit_assert_country_blocks_for_ipv6 and fill it with guts that will match the pattern I used previously. It'll be almost the same. Perhaps adjust only the counts of bits to read in the take_bits methods.

bertllll · 2024-07-29T22:11:24Z

ip_country/src/country_block_serde.rs

+        }
+        let remaining_bit_count = bit_queue.len();
+        let data = bit_queue.take_bits(remaining_bit_count).unwrap();
+        bit_data.push(data);


This is where you will place your prepared method that uses the same code as in the ipv4 counterpart.

bertllll · 2024-07-29T22:11:38Z

ip_country/src/country_block_serde.rs

+        }
+        let remaining_bit_count = bitqueue.len();
+        let data = bitqueue.take_bits(remaining_bit_count).unwrap();
+        vec_64.push(data);


You've got your function to perform these lines. Reuse it.

bertllll · 2024-07-29T22:16:36Z

ip_country/src/country_block_serde.rs

+                Ipv6Addr::from_str("0:0:0:0:0:0:0:0").unwrap(),
+                Ipv6Addr::from_str("1:2:3:4:5:6:7:7").unwrap(),
+            ),
+            country: Country::from(0usize), // sentinel


I think this test has its point in making sure these end blocks have the country codes set to some other value than "ZZ" and when you call finish on the whole thing, it won't override any of them by ZZ. Therefore you probably should rework this first record at least. Get rid of the sentinel here.

bertllll · 2024-07-30T15:03:28Z

node/src/neighborhood/gossip_acceptor.rs

+        let keys = expected_db
+            .keys()
+            .iter()
+            .map(|key| (*key).clone())


This should probably be only

.map(|key|key.clone())

node/src/neighborhood/gossip_acceptor.rs

* continue of code review changes * test for panic in handle_retrieve_transactions * fixing more comments from review * removing TODO * implementing review, added two new tests for ip_country * removing unnecesary country_code from node_descriptor * added cfg(test) and cfg(not(test)) for static ref COUNTRY_CODE_FINDER, created test_dbip_country.rs * change country_code member of NodeRecordInner_0v1 to country_code_opt * simplifying tests in node_record.rs and neighborhood_database.rs * resolving review comments * final changes in gossip_acceptor from review * continue of implementing review comments * fix test return value * added ip_country to ci/all.sh * cherry-pick from GH-784-fix-for-Vojta and implementing iterator for deserializer * try to create test for fmt * tests for print ip v4 and v6 vec[u8] in country_block_serde.rs * ci for ip_country + resolved conflicts of clippy * masq_lib dependencies solved * dependency masq_lib for ip_country in node workspace * remove commented out code + fix condition for non empty bit_queue * formatting * 5 seconds for wait_for_log in requested_chain_meets_different_db_chain_and_panics_integration * fixing impl for AGR in multinode tests * formatting * fix nonexistent NodeAddr in multinode tests for AGR * change single match to if * try to fix build in actions for MacOS * try to find out disk space on MacOS container in Actions * add diagnostics commands on failure * check diskspace before ci/all.sh workflow runs * diagnose mounted disks * analyze /var/ diskspace * remove advanced diagnosticst for disk space * switch macos to version 13 * diagnostics for /Users/runner/work/Node/Node/node/target/release * fasten diagnostics * fasten diagnostics fix * fasten diagnostics back * GH-784-review-two-state: fixes that could work on Actions, appeasing it * fixing review * implementing CountryBlock count in generated rs file, and infrastructure * fixing ip_country tests * fix import of ip_country to multinode_integration_tests * formatting * fix the reference for COUNTRY_CODE_FINDER * implemenitng initalization checker for COUNTRY_CODE_FINDER * CountryCodeFinder initializer * fixed comments from review * removed custom PartialEq for NodeLocation * get back with COUNTRY_CODE_FINDER --------- Co-authored-by: Bert <[email protected]>

dnwiebe and others added 30 commits March 28, 2024 20:49

About to simplify

9e83a53

Almost right

9709bfe

All BitQueue tests passing

0b7c81a

A little cleanup

e4740d3

Can serialize IPv4 now

b6478ff

Serialization is working for IPv6 now

bcdf410

Deserializes IPv4 CountryBlocks

f4ee361

Minor corrections

7c92b86

Deserialization works for IPv6

69d6a0b

Minor cleanup

8f5387b

Added documentation of compressed format

880e1c3

Completed outstanding CSV validation

fdcb8be

Now generates code

d71041b

country_finder now works

cda7329

Now the error flow works for ip-country::main.

b9e9822

Added generated IP/Country database file

6f8a82f

remove unnecesary parenthesis and add Slovak IP to test

9dcd67a

node_location.rs with first few tests

a976dbe

tests for assing 100 and 10 ips with the NodeLocation + fixing some w…

35253da

…arnings in ip_country package

formatting

05f6324

remove tests, change processing of get_node_location fn, adding comme…

1ec7cb3

…nted out tests for 1000 ips

added free_world_bit and country_code to Gossip. test for country_cod…

861ae36

…e and free_world_bit in Gossip

changes to NodeRecordInner_0v1

0a20e71

removed id from NodeLocation, added NodeLocation processing into Node…

d112b79

…Record, NodeRecordInner_0v1, handle_new_public_ip

add lazy_static COUNTRY_CODE_DIGEST, and its processing fn to use in …

785baf7

…Tests for NodeLocation

remove free_world_bit from NodeRecordInner_0v1, fixing tests, fixing …

b9f72f2

…tests for neigborhood_database

fixing tests for NodeLocation in neighborhood DB

a6c1c33

fixed tests for neighborhood_database

67ab0b9

formatting and reducing variables to NodeRecord::new() with struct No…

336b9d1

…deRecordInputs

add free_world_bit to NodeLocation, fixed tests for NodeRecord

7b68d13

czarte added 5 commits July 2, 2024 14:55

formatting

4e8f30e

remove argument to pass even on fail

7fdb882

fix node_remembers_its_neighbors_across_a_bounce by thread::sleep for…

e19a4a3

… 5 seconds

switch from thread::sleep to loop with Tcp connection to None until i…

9f01938

…s down

remove unused import

68f3c04

bertllll reviewed Jul 9, 2024

View reviewed changes

bertllll reviewed Jul 12, 2024

View reviewed changes

bertllll reviewed Jul 15, 2024

View reviewed changes

bertllll reviewed Jul 16, 2024

View reviewed changes

bertllll reviewed Jul 17, 2024

View reviewed changes

bertllll reviewed Jul 18, 2024

View reviewed changes

bertllll reviewed Jul 19, 2024

View reviewed changes

bertllll reviewed Jul 20, 2024

View reviewed changes

bertllll reviewed Jul 22, 2024

View reviewed changes

bertllll reviewed Jul 23, 2024

View reviewed changes

bertllll reviewed Jul 24, 2024

View reviewed changes

bertllll reviewed Jul 27, 2024

View reviewed changes

bertllll reviewed Jul 28, 2024

View reviewed changes

bertllll reviewed Jul 29, 2024

View reviewed changes

bertllll reviewed Jul 30, 2024

View reviewed changes

czarte closed this Nov 4, 2024

		@@ -28,7 +28,7 @@ use node_lib::sub_lib::wallet::Wallet;
		#[test]

GH 784 - Change Neighborhood DB to contain Free World Bit and Country code #444

GH 784 - Change Neighborhood DB to contain Free World Bit and Country code #444

Uh oh!

Conversation

czarte commented May 17, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bertllll left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bertllll left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

czarte Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

czarte Jul 26, 2024 •

edited

Loading