Skip to content

persist: Persist ChannelMonitors in their own directory. #806

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

valentinewallace
Copy link
Contributor

Slightly nicer to work with for loading monitors from disk.

@jkczyz jkczyz self-requested a review February 26, 2021 16:38
@codecov
Copy link

codecov bot commented Feb 26, 2021

Codecov Report

Merging #806 (b7b6bd9) into main (e77b160) will decrease coverage by 0.04%.
The diff coverage is 94.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #806      +/-   ##
==========================================
- Coverage   91.04%   91.00%   -0.05%     
==========================================
  Files          48       48              
  Lines       25480    25483       +3     
==========================================
- Hits        23199    23190       -9     
- Misses       2281     2293      +12     
Impacted Files Coverage Δ
lightning-persister/src/lib.rs 92.52% <92.30%> (+0.29%) ⬆️
lightning-persister/src/util.rs 95.77% <100.00%> (-0.06%) ⬇️
lightning/src/ln/functional_tests.rs 96.92% <0.00%> (-0.21%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e77b160...bfaec71. Read the comment docs.

@TheBlueMatt TheBlueMatt added this to the 0.0.13 milestone Feb 26, 2021
@valentinewallace valentinewallace force-pushed the monitor-data-persistence-path branch from 7be8bbf to bfaec71 Compare February 26, 2021 17:39
Copy link

@ariard ariard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review ACK bfaec71

Could be cool to display somewhere in the doc what's the expected dir-tree should look like (e.g a tree cmd-like output)

your_path
└── channel
    ├── manager
    └── monitors

@TheBlueMatt TheBlueMatt merged commit beb88e6 into lightningdevkit:main Feb 28, 2021
Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Post-merge ACK.

Code Review ACK bfaec71

Could be cool to display somewhere in the doc what's the expected dir-tree should look like (e.g a tree cmd-like output)

your_path
└── channel
    ├── manager
    └── monitors

I think it's actually suppose to be:

your_path
 ├── manager
 └── monitors
     └── channel

But currently this is not enforced because FilesystemPersister::persist_manager can be given any directory by the user. We could enforce this by making it non-pub and have a pub method return a closure calling it with the appropriate path.

Comment on lines 104 to 135
Result<HashMap<OutPoint, ChannelMonitor<Keys::Signer>>, ChannelMonitorUpdateErr> {
if let Err(_) = fs::create_dir_all(&self.path_to_channel_data) {
return Err(ChannelMonitorUpdateErr::PermanentFailure);
}
let mut res = HashMap::new();
for file_option in fs::read_dir(&self.path_to_channel_data).unwrap() {
let file = file_option.unwrap();
let owned_file_name = file.file_name();
let filename = owned_file_name.to_str();
if !filename.is_some() || !filename.unwrap().is_ascii() || filename.unwrap().len() < 65 {
if let Err(_) = fs::create_dir_all(self.path_to_monitor_data()) {
return Err(ChannelMonitorUpdateErr::PermanentFailure);
}
let mut res = HashMap::new();
for file_option in fs::read_dir(self.path_to_monitor_data()).unwrap() {
let file = file_option.unwrap();
let owned_file_name = file.file_name();
let filename = owned_file_name.to_str();
if !filename.is_some() || !filename.unwrap().is_ascii() || filename.unwrap().len() < 65 {
return Err(ChannelMonitorUpdateErr::PermanentFailure);
}

let txid = Txid::from_hex(filename.unwrap().split_at(64).0);
if txid.is_err() { return Err(ChannelMonitorUpdateErr::PermanentFailure); }
let txid = Txid::from_hex(filename.unwrap().split_at(64).0);
if txid.is_err() { return Err(ChannelMonitorUpdateErr::PermanentFailure); }

let index = filename.unwrap().split_at(65).1.split('.').next().unwrap().parse();
if index.is_err() { return Err(ChannelMonitorUpdateErr::PermanentFailure); }
let index = filename.unwrap().split_at(65).1.split('.').next().unwrap().parse();
if index.is_err() { return Err(ChannelMonitorUpdateErr::PermanentFailure); }

let contents = fs::read(&file.path());
if contents.is_err() { return Err(ChannelMonitorUpdateErr::PermanentFailure); }
let contents = fs::read(&file.path());
if contents.is_err() { return Err(ChannelMonitorUpdateErr::PermanentFailure); }

if let Ok((_, loaded_monitor)) =
<(BlockHash, ChannelMonitor<Keys::Signer>)>::read(&mut Cursor::new(&contents.unwrap()), keys) {
res.insert(OutPoint { txid: txid.unwrap(), index: index.unwrap() }, loaded_monitor);
} else {
return Err(ChannelMonitorUpdateErr::PermanentFailure);
if let Ok((_, loaded_monitor)) =
<(BlockHash, ChannelMonitor<Keys::Signer>)>::read(&mut Cursor::new(&contents.unwrap()), keys) {
res.insert(OutPoint { txid: txid.unwrap(), index: index.unwrap() }, loaded_monitor);
} else {
return Err(ChannelMonitorUpdateErr::PermanentFailure);
}
}
Ok(res)
}
Ok(res)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation looks off by one. I've started following these conventions when wrapping long method signatures:

https://github.com/rust-dev-tools/fmt-rfcs/blob/master/guide/items.md

With an extra line for the opening brace, you can reduce the indent to how it was before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants