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

[czarte]

No description provided.

[bertllll]

I see no use in this attribute macro.

[czarte]

this annotation skips formatter for dbip_country.rs, because otherwise formatter wants to reformat that large code of IPs

[bertllll]

Yes. It doubles the same attributes which are inside the file, but it's okay. Ignore this considered solved.

[bertllll]

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]

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]

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.

[czarte]

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.

[bertllll]

It should be testable, the iterator isn't endless, so once it reaches the end, it returns None...and how do you assert, well, easily by the fact that you have escaped from this loop. It's an assertion o its own.

[czarte]

I think we test it in test deserialize_country_blocks_ipv4_and_ipv6_adn_fill_vecs

[bertllll]

Then the comment needs to disappear ;-)

[bertllll]

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()
        }
    }

🤷‍♂️

[bertllll]

Again, this inconsistency, where you assert this at some tests but don't with others doesn't make me happy. Could you think about some more systematical approach?

[bertllll]

You could possibly also add the two more params to be asserted on the CountryCode.

[bertllll]

Such a comment doesn't belong into a test.

Also...it is testable, as it is the only condition at which you can leave the loop...

[bertllll]

Please give this a clearer name. Specific to what does this duration belong to...

[bertllll]

I'm hesitating about these printlns. Should we really print it all the time? Are devs supposed to read direct notes from test execution anyhow? I don't think so. It should probably be printed only for a failure. Also, as I stated in the section with the production code, there should be a better way for choosing a reasonable constraint than these huge values of multiple secs.

It's always best to find a way to refer it relatively to some other duration measured on the same machine and to proof that less time was needed in the test than what was measured in some extreme (the worst case scenario possibly).

[bertllll]

As you can see, this changed from a treatable error to a panic, however, I cannot see any modification following this also in the test tree below. It implies that this panic is not tested. It surprises me because there must have been some tests covering this which should be panicking now. So maybe that test was truly missing even before. That's not an excuse for the present.

You will probably need to write a test which seems to be missing. Unless you find any sew this hole in our test coverage.

[bertllll]

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]

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.

[bertllll]

please remove that (crate) part, it's irritating

I'm also not much happy that it must be public now, does it?

[bertllll]

This is not a way how problems should be solved. Reconsider if there isn't a different way to get this information without making the field public.

How? Make it private again and try compiling the code. Then look at the popped errors and see if you can get around it with a better design.

More importantly, I believe that NodeAddr has already a method by which you can ask about the ip_addr, which is exactly what you should have done in the first place.

[bertllll]

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]

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]

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.

[bertllll]

I think you probably should write a card about this to avoid unnecessary delays in delivery. Once you have the card's number you can rewrite this comment so that it informs the reader about a card with a certain number which is supposed to shrink these long waits.

[bertllll]

This should be node_location_opt.

[bertllll]

Why does it require 6 times more time!!!?

I thought the country code look-up wouldn't take place in the routing engine and I'm almost certain that it doesn't, so tell me why did the required amount of time to complete a usable route changed so drastically. There isn't a reason for that in my opinion and thus I'd like to see this number go much lower but if you think you did affect the times it could be possibly be left slightly enlarged. Still, I think when Dan was choosing this value, he new it was a tricky moment and he ended up with 100 hundred, apparently being enough back then to pass even through the Github Actions. Now, what you did, makes the constraint ridiculous because, it probably represents a plenty of time, much much more than ever needed. Hopefully...

[bertllll]

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

But can we do otherwise?

[bertllll]

Should go away. The commented out code as well as the println! macro.

[bertllll]

I usually prefer to keep the variable name and the class name close enough to each other. If the class is called "Inputs" then it is self-evident to call the variable "inputs" rather than "data". But I understand one cannot keep track of everything all the time, this happens to me a lot too.

[bertllll]

This is really fishy set of lines. I wish that you will conclude this is unnecessary.

Going deeper in it, I must ask myself questions like: Why should the root node record change at all? The gossip was to be Ignored as the preparation of mocks shows at the top of the test. That means that the gossip contained only what we have already known. Thus no change to our database should be done.

I cannot think of a reason why you should allow these changes that you allow by the extra assertions. It doesn't seem right.

Please defend yourself by sharing your understanding with me or try to take these lines back off.

[bertllll]

Continue ip_country.rs ln. 97

[bertllll]

You must inspect this. Step one: find out if it is tested now or still not (by adding a panicking piece of code and seeing it kill a test). Step two: Write a missing test if that's the issue.

[bertllll]

The same action required like described in the previous comment.

[bertllll]

You can get rid of this imperative style of coding by using:

   let mut errors = csv_rdr
        .records()
        .map(|string_record_result| match string_record_result {
            Ok(string_record) => CountryBlock::try_from(string_record),
            Err(e) => Err(format!("CSV format error: {:?}", e)),
        })
        .enumerate()
        .flat_map(|idx, country_block_result| {
            match country_block_result {
                Ok(country_block) => {
                    serializer.add(country_block);
                    None
                }
                Err(e) => Some(format!("Line {}: {}", idx + 1, e)),
            }
        })
        .collect::<Vec<String>>();

[bertllll]

A variable error_list is already in the scope. You can reuse it instead of calling errors.join("\n") again.

[bertllll]

You could make the 64 a manifest constant. Something like: COUNTRY_BLOCK_BIT_SIZE

[bertllll]

For symmetry, this could be named PROPER_TEST_DATA to make a pair with the other static variable BAD_TEST_DATA.

[bertllll]

node_ip -> node_ip_opt

[bertllll]

I still don't fully understand why a Node descriptor needs to present the country code.

[bertllll]

I've seen many tests where the country code is fetched by using the prod code and therefore a big code machinery that isn't particularly useful in a given test. This puts much more work into our unit tests. I do think this is wrong because we know such a lookup isn't necessarily cheap.

You should create real and mock versions of an abstract class, called like my example or similar: NodeLocator. This trait will have probably a single method which is what you currently be using as a bare function, this get_node_location() .

You should think enough to identify the Node modules that are likely to be linked with processing the Node location lookup. Most of these test should use only the mock version of this class (you will have two responsibilities: A) to assert on params of this method, which is the ip_addr and B) to prepare a result to be returned from it in the test - if you don't do it, your test will blow up on that absence during the call.)

Vojta, this is going to be an annoying task but I'm afraid also necessary, because this goes perfectly along with our dev policy. We want to mock out anything sucking resources highly.

[bertllll]

Yeah, this seems like you will eventually have to mock the get_node_location fn in this test as I described at the place of the function declaration.

[bertllll]

I've gone further already and seen some use of the code. I think solving the problem by using this String type alone was a bad decision. The proper way to go by would be using an optional value because this value is optional.

It should be:

pub country_code_opt: Option<String>,

If not that, but I truly recommend that, you must make sure this field will be somehow consistent with what we do in other places. Please don't use an entirely empty string but put in the ZZ sentinel instead.

[bertllll]

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

[bertllll]

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]

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.

[bertllll]

Can you please move this modification step towards the newly initialized variable (without having it intercepted by another initialization which makes it less bright)?

[bertllll]

Continue neighborhood_database.rs can_get_mutable_root, ln. 409.

[bertllll]

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()
     }
}

[bertllll]

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?

[bertllll]

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.

[bertllll]

Here you can easily use this notation:

let last_update = subject.root().metadata.last_update;

It's because you know there is only one record at the moment, the root Node.

This is much better solution, also solving the problem with the left over todo!().

[bertllll]

Continue by test_ip_country_performance, ln. 123.

[bertllll]

Okay, I've just seen multiple tests whose structure is built up almost identically like this first one.

Therefore I'm putting forth refactoring it this way:

There always is this introducing block of lines

    let mut this_node = make_node_record(1234, true);
    this_node.inner.country_code = "AU".to_string();
    this_node.metadata.node_location_opt = Some(NodeLocation {
        country_code: "AU".to_string(),
        free_world_bit: true,
    });
    this_node.resign();

which I wanted to recommend to put inside an auxiliary function and reuse but while I was pondering over that,
I realized an interesting thing. None of those tests actually use their assertion section for direct assertions on these carefully injected values from the beginning. It means you do not have to do that at all in the first place.
In other words, the test doesn't care what the values are. Whatever they are they only must match as read before the act and after it. Therefore you can simply delete all the added lines and leave only let mut this_node = make_node_record(1234, true); in there.

[bertllll]

As stated previously. No need for the green lines.

[bertllll]

No need for the green lines.

[bertllll]

No need for the green lines.

[bertllll]

No need for the green lines.

[bertllll]

Please remove this "printing".

[bertllll]

Redundant "printing".

[bertllll]

I think you forgot to assert the free world bit.

[bertllll]

Please learn yourself to use the more revealing decoupled tuple with named inner args, not to refer to them by indexes. Here it is:

 let (country_code, free_world_bit) = pick_country_code_record(2222);

[bertllll]

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?