Fix shutdown save peers utreexo

[lucad70]

Added code to save good utreexo peers to a anchors.json file

[lucad70]

I have seen the errors on linting and build_docker. I am looking into it.

[JoseSK999]

I think the build docker error is unrelated, cc @Davidson-Souza

[lucad70]

To fix linting, do I need to use the nightly rust?

[JoseSK999]

Yes, you can run just fmt

[lucad70]

A doubt that I had when writting the code was if I needed to save every utreexo peers or just "good" peers. I opted for just the "good" ones.

[Davidson-Souza]

A doubt that I had when writting the code was if I needed to save every utreexo peers or just "good" peers. I opted for just the "good" ones.

The idea is to save only the ones we are connected to, so they are by definition good

[lucad70]

Is there anything I should improve/modify before merging?

[jaoleal]

I was looking deeper on floresta-wire and looks like we already have the functionality of saving peers in disk...

pub fn dump_peers(&self, datadir: &str) -> std::io::Result<()> {
        let peers: Vec<_> = self
            .addresses
            .values()
            .cloned()
            .map(Into::<DiskLocalAddress>::into)
            .collect::<Vec<_>>();
        let peers = serde_json::to_string(&peers);
        if let Ok(peers) = peers {
            std::fs::write(datadir.to_owned() + "/peers.json", peers)?;
        }
        Ok(())
    }

Maybe the work here can be hooking this to do what dump_utreexo_peers and save_utreexo_peers do... additionally making docstrings for this function

[Davidson-Souza]

I was looking deeper on floresta-wire and looks like we already have the functionality of saving peers in disk...

pub fn dump_peers(&self, datadir: &str) -> std::io::Result<()> {
        let peers: Vec<_> = self
            .addresses
            .values()
            .cloned()
            .map(Into::<DiskLocalAddress>::into)
            .collect::<Vec<_>>();
        let peers = serde_json::to_string(&peers);
        if let Ok(peers) = peers {
            std::fs::write(datadir.to_owned() + "/peers.json", peers)?;
        }
        Ok(())
    }

Maybe the work here can be hooking this to do what dump_utreexo_peers and save_utreexo_peers do... additionally making docstrings for this function

This is our address database, it saves all known address. The functionality we are discussing here is saving only peers we are currently connected.

[jaoleal]

Yes, i can see this...

maybe i expressed my idea poorly. we could do a nested functionality with this function, saving the addresses and our best peers... I understand if you disagree with this because of organization.

[lucad70]

I did the method here, but I haven't find a better way to verify other than starting the node and shutting it down. My question now is should I nest both features or not? Or rather, should I first commit and push the modified version before trying to nest these functionalities?

[lucad70]

An unrelated question: Is the peer id=0 always "my running node"?

[Davidson-Souza]

I did the method here, but I haven't find a better way to verify other than starting the node and shutting it down. My question now is should I nest both features or not? Or rather, should I first commit and push the modified version before trying to nest these functionalities?

They should be independent. One is called periodically, the other should only be called on shutdown.

An unrelated question: Is the peer id=0 always "my running node"?

Not sure if I get your question. RunningNode is a state we can be in (out of: ChainSelector and SyncNode) not something about our peers

[lucad70]

I did the method here, but I haven't find a better way to verify other than starting the node and shutting it down. My question now is should I nest both features or not? Or rather, should I first commit and push the modified version before trying to nest these functionalities?

They should be independent. One is called periodically, the other should only be called on shutdown.

If i go to node.rs when the node reads the kill_signal it calls the method .shutdown().await

The .shutdown() is as follows:

    pub(crate) async fn shutdown(&mut self) {
        info!("Shutting down node");
        for peer in self.peer_ids.iter() {
            try_and_log!(self.send_to_peer(*peer, NodeRequest::Shutdown).await);
        }
        try_and_log!(self.save_utreexo_peers());
        try_and_log!(self.save_peers());
        try_and_log!(self.chain.flush());
    }

The self.save_utreexo_peers()was the one I added aiming to save the connected utreexo peers at the moment of shutdown.

However, the self.save_peers() from the address manager was already running on shutdown. So from the suggestion made by @jaoleal I was wondering if I should make a method to call both methods or if this organization is alright at the moment.

[Davidson-Souza]

The way you already implemented is what I had in mind. No need to change

[lucad70]

I'll push the commits so you can test and review it.

[lucad70]

@Davidson-Souza I realized that as I used .unwrap() to modify the type of peers_id u32 to usize, if I shutdown my node before connecting to any peers, even though I added the if clause to check for peers, it unwraps a None value and the shutdown panics. I am looking into possible solutions.

It also made me realize I need to add a functional test to check for this behavior of panicking during shutdown. What do you think?

[Davidson-Souza]

ACK 3e70ae9.