talaia-labs / rust-teos

The Eye of Satoshi - Lightning Watchtower
https://talaia.watch
MIT License
128 stars 62 forks source link

Add a log lines for datadir, conf file and options #202

Closed sr-gi closed 1 year ago

sr-gi commented 1 year ago

Many newcomers, especially for SoB, are struggling with where the teos data directory is located. It may be helpful to add log entries that state:

This is mainly what bitcoind does, so we can mimic it. Here's an example from my local regtest node:

2023-03-07T22:12:14Z Default data directory /Users/sergi/Library/Application Support/Bitcoin
2023-03-07T22:12:14Z Using data directory /Users/sergi/Library/Application Support/Bitcoin/regtest
2023-03-07T22:12:14Z Config file: /Users/sergi/Library/Application Support/Bitcoin/bitcoin.conf
2023-03-07T22:12:14Z Config file arg: dnsseed="0"
2023-03-07T22:12:14Z Config file arg: regtest="1"
2023-03-07T22:12:14Z Config file arg: rpcpassword=****
2023-03-07T22:12:14Z Config file arg: rpcuser=****
2023-03-07T22:12:14Z Config file arg: server="1"

These are INFO entries AFAICT.

anmode commented 1 year ago

Could you assign this issue to me please? I got the solution. Hopefully generate pR soon.

sr-gi commented 1 year ago

Hey @anmode, aren't you already working on 2 PR atm?

I'd rather you finish those first and then consider working on this one if you're still up for it

anmode commented 1 year ago

Hey @anmode, aren't you already working on 2 PR atm?

I'd rather you finish those first and then consider working on this one if you're still up for it

But those are completed from myside..I address all the suggestions. Please could you review them so if any fix is required I'll do that!

sr-gi commented 1 year ago

They are not. You had pending suggestions for both, and I left additional ones earlier today.

Work on getting those ready to merge and then pick more if you like.

anmode commented 1 year ago

They are not. You had pending suggestions for both, and I left additional ones earlier today.

Work on getting those ready to merge and then pick more if you like.

Yeahh!! Sorry i Recently see your next message!! Okay I'll complete them first thanks!

anipaul2 commented 1 year ago

Hello @sr-gi, could you please assign this issue to me? I want to work on this one. I am participating in sob this year and want to work on the idea accountable and non-accountable modes. We just had a chat in discord some minutes ago.

anipaul2 commented 1 year ago

@sr-gi Do I need to make a log file at the root of the project writing the relevant paths mentioned or I'm approaching it in the wrong way?

anipaul2 commented 1 year ago

For eg:-

Default data directory: E:\rust-teos\teos Using data directory: E:\rust-teos\teos\main.rs Config file: E:\rust-teos\teos\config.rs Config file arg: rpcport=8814

sr-gi commented 1 year ago

@sr-gi Do I need to make a log file at the root of the project writing the relevant paths mentioned or I'm approaching it in the wrong way?

The code is already logging via the log::{info, debug, error, ...}! macros. You just need to add the relevant lines in main.rs for the items pointed out in the issue.

So basically understand how main deals with the data_dir, how defaults are set, and when are they updated. For updated defaults log the corresponding lines.

anipaul2 commented 1 year ago

If you're free can we just have a 5 minutes meet call?

anipaul2 commented 1 year ago

@sr-gi Do I need to make a log file at the root of the project writing the relevant paths mentioned or I'm approaching it in the wrong way?

The code is already logging via the log::{info, debug, error, ...}! macros. You just need to add the relevant lines in main.rs for the items pointed out in the issue.

So basically understand how main deals with the data_dir, how defaults are set, and when are they updated. For updated defaults log the corresponding lines.

main.rs file and config.rs file right?

anipaul2 commented 1 year ago

@sr-gi I understood what I have to do, just have some small doubts on main.rs. Can we have a short call on meet?

sr-gi commented 1 year ago

@sr-gi I understood what I have to do, just have some small doubts on main.rs. Can we have a short call on meet?

I cannot arrange a call today. Just ask your questions here or in Discord.

anipaul2 commented 1 year ago

@sr-gi I understood what I have to do, just have some small doubts on main.rs. Can we have a short call on meet?

I cannot arrange a call today. Just ask your questions here or in Discord.

In main.rs, I have to change this code for default one and the one we are using?

if is_default { log::info!("Loading default configuration") } else { log::info!("Loading configuration from file") }

sr-gi commented 1 year ago

@sr-gi I understood what I have to do, just have some small doubts on main.rs. Can we have a short call on meet?

I cannot arrange a call today. Just ask your questions here or in Discord.

In main.rs, I have to change this code for default one and the one we are using?

if is_default { log::info!("Loading default configuration") } else { log::info!("Loading configuration from file") }

Not really. You need to log the results of path_network (for the data directory that is being used), config_file_path (for where the config file is located. You'll need to create that one, since now it is not stored in a variable but directly passed to config::from_file).

There's two missing things:

Those two I'll leave it to you to figure out how to implement them.

anipaul2 commented 1 year ago

Okay. Thank you for the clarification.

void-ness commented 1 year ago

@sr-gi I understood what I have to do, just have some small doubts on main.rs. Can we have a short call on meet?

I cannot arrange a call today. Just ask your questions here or in Discord.

In main.rs, I have to change this code for default one and the one we are using? if is_default { log::info!("Loading default configuration") } else { log::info!("Loading configuration from file") }

Not really. You need to log the results of path_network (for the data directory that is being used), config_file_path (for where the config file is located. You'll need to create that one, since now it is not stored in a variable but directly passed to config::from_file).

There's two missing things:

  • Log the default datadir path
  • Log every option that is not the default.

Those two I'll leave it to you to figure out how to implement them.

Shouldn't we log the path instead for the data directory that is being used?

let path = config::data_dir_absolute_path(opt.data_dir.clone());

path_netowork joins the unnecessary information of btc_network with our data directory.

let path_network = path.join(conf.btc_network.clone());
sr-gi commented 1 year ago

@sr-gi I understood what I have to do, just have some small doubts on main.rs. Can we have a short call on meet?

I cannot arrange a call today. Just ask your questions here or in Discord.

In main.rs, I have to change this code for default one and the one we are using? if is_default { log::info!("Loading default configuration") } else { log::info!("Loading configuration from file") }

Not really. You need to log the results of path_network (for the data directory that is being used), config_file_path (for where the config file is located. You'll need to create that one, since now it is not stored in a variable but directly passed to config::from_file). There's two missing things:

  • Log the default datadir path
  • Log every option that is not the default.

Those two I'll leave it to you to figure out how to implement them.

Shouldn't we log the path instead for the data directory that is being used?

let path = config::data_dir_absolute_path(opt.data_dir.clone());

path_netowork joins the unnecessary information of btc_network with our data directory.

let path_network = path.join(conf.btc_network.clone());

Not really. The btc_network folder under the data dir is the actual folder where the data related to that network is stored. It follows the same pattern as for bitcoind:

> ls .teos
ca-key.pem     client-key.pem main           server-key.pem teos.toml
ca.pem         client.pem     regtest        server.pem
> ls .teos/regtest
teos_db.sql3
> ls .teos/main
onion_v3_sk  teos_db.sql3
void-ness commented 1 year ago

Not really. The btc_network folder under the data dir is the actual folder where the data related to that network is stored. It follows the same pattern as for bitcoind:

> ls .teos
ca-key.pem     client-key.pem main           server-key.pem teos.toml
ca.pem         client.pem     regtest        server.pem
> ls .teos/regtest
teos_db.sql3
> ls .teos/main
onion_v3_sk  teos_db.sql3

Apologies. I misunderstood the role of data_dir. I assumed it is used to refer to the parent folder of conf file.

anipaul2 commented 1 year ago

@sr-gi I understood what I have to do, just have some small doubts on main.rs. Can we have a short call on meet?

I cannot arrange a call today. Just ask your questions here or in Discord.

In main.rs, I have to change this code for default one and the one we are using? if is_default { log::info!("Loading default configuration") } else { log::info!("Loading configuration from file") }

Not really. You need to log the results of path_network (for the data directory that is being used), config_file_path (for where the config file is located. You'll need to create that one, since now it is not stored in a variable but directly passed to config::from_file).

There's two missing things:

  • Log the default datadir path
  • Log every option that is not the default.

Those two I'll leave it to you to figure out how to implement them.

I tried to log the results of path network and config_file_path but it shows the same error again and again.

Pathbuf doesn't implement std::fmt::Display

sr-gi commented 1 year ago

@anipaul2 check how Display works in rust.

e.g: https://doc.rust-lang.org/rust-by-example/hello/print/print_display.html

anipaul2 commented 1 year ago

@anipaul2 check how Display works in rust.

e.g: https://doc.rust-lang.org/rust-by-example/hello/print/print_display.html

test case all passed and working for datadir, conf but not able to execute options which are not default. Do I need to iterate for all of the non default ones and log them out?

anipaul2 commented 1 year ago

@anipaul2 check how Display works in rust. e.g: https://doc.rust-lang.org/rust-by-example/hello/print/print_display.html

test case all passed and working for datadir, conf but not able to execute options which are not default. Do I need to iterate for all of the non default ones and log them out?

like this

if !is_default { log::info!("Using the following options:"); if opt.data_dir != config::

void-ness commented 1 year ago

@anipaul2 check how Display works in rust. e.g: https://doc.rust-lang.org/rust-by-example/hello/print/print_display.html

test case all passed and working for datadir, conf but not able to execute options which are not default. Do I need to iterate for all of the non default ones and log them out?

like this

if !is_default { log::info!("Using the following options:"); if opt.data_dir != config::

I wonder if we can use something like below in the config module under the patch_with_options function. For eg

if options.btc_rpc_user.is_some() {
            log::info!(
                "Config file arg: btc_rpc_user={:?}",
                &options.btc_rpc_user.clone().unwrap()
            );
            self.btc_rpc_user = options.btc_rpc_user.unwrap();
        }

The only drawback over here is that, this approach will let us log only the changed options passed by the user via the command line. It won't be displaying options by comparing the config file with default values.

anipaul2 commented 1 year ago

@anipaul2 check how Display works in rust. e.g: https://doc.rust-lang.org/rust-by-example/hello/print/print_display.html

test case all passed and working for datadir, conf but not able to execute options which are not default. Do I need to iterate for all of the non default ones and log them out?

like this if !is_default { log::info!("Using the following options:"); if opt.data_dir != config::

I wonder if we can use something like below in the config module under the patch_with_options function. For eg

if options.btc_rpc_user.is_some() {
            log::info!(
                "Config file arg: btc_rpc_user={:?}",
                &options.btc_rpc_user.clone().unwrap()
            );
            self.btc_rpc_user = options.btc_rpc_user.unwrap();
        }

The only drawback over here is that, this approach will let us log only the changed options passed by the user via the command line. It won't be displaying options by comparing the config file with default values.

Nothing has to be changed in config.rs file for this issue. It's all in main.

sr-gi commented 1 year ago

@anipaul2 check how Display works in rust. e.g: https://doc.rust-lang.org/rust-by-example/hello/print/print_display.html

test case all passed and working for datadir, conf but not able to execute options which are not default. Do I need to iterate for all of the non default ones and log them out?

like this if !is_default { log::info!("Using the following options:"); if opt.data_dir != config::

I wonder if we can use something like below in the config module under the patch_with_options function. For eg

if options.btc_rpc_user.is_some() {
            log::info!(
                "Config file arg: btc_rpc_user={:?}",
                &options.btc_rpc_user.clone().unwrap()
            );
            self.btc_rpc_user = options.btc_rpc_user.unwrap();
        }

The only drawback over here is that, this approach will let us log only the changed options passed by the user via the command line. It won't be displaying options by comparing the config file with default values.

I think the best approach would go on the lines of comparing the loaded Config with its default value, which is defined in config.rs. If any item differed from the default, then it may be logged. Notice there may be some expectations here, such as btc_rpc_port which defaults to zero, but just as a temporary value given it depends on the btc_network.