persister: Expose method to read ChannelMonitors from disk by valentinewallace · Pull Request #863 · lightningdevkit/rust-lightning

valentinewallace · 2021-04-02T18:43:25Z

Tested w/ the sample.

codecov · 2021-04-02T18:59:18Z

Codecov Report

Merging #863 (94aa47b) into main (df732f4) will increase coverage by 1.89%.
The diff coverage is 87.09%.

@@ Coverage Diff @@## main #863 +/- ##
==========================================
+ Coverage 90.59% 92.49% +1.89% 
==========================================
Files 51 51 Lines 27195 32604 +5409 ==========================================
+ Hits 24638 30157 +5519 + Misses 2557 2447 -110

Impacted Files	Coverage Δ
lightning-persister/src/lib.rs	`94.95% <87.09%> (-0.66%)`	⬇️
background-processor/src/lib.rs	`98.73% <0.00%> (-1.27%)`	⬇️
lightning/src/routing/router.rs	`96.87% <0.00%> (-0.10%)`	⬇️
lightning/src/ln/reorg_tests.rs	`99.63% <0.00%> (+0.06%)`	⬆️
lightning/src/util/test_utils.rs	`83.23% <0.00%> (+0.28%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`96.07% <0.00%> (+0.85%)`	⬆️
lightning/src/ln/functional_tests.rs	`98.47% <0.00%> (+1.58%)`	⬆️
lightning/src/ln/channel.rs	`91.87% <0.00%> (+4.74%)`	⬆️
lightning/src/ln/channelmanager.rs	`89.25% <0.00%> (+6.02%)`	⬆️

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 df732f4...94aa47b. Read the comment docs.

jkczyz · 2021-04-07T15:55:43Z

+	{
+ let path = self.path_to_monitor_data();
+ if !Path::new(&path).exists() {
+ return Ok(HashMap::new());


Should this be a NotFound error? That way caller's can create the directory if it does not exist.

Hmm, isn't it more likely its just the first startup and thus there's no monitors?

I was thinking that the caller would handle creating the directory. But I guess that FilesystemPersister creates it on demand, so probably ok then. Wasn't sure if we want to give the user insight into this (e.g., they instantiate a persister expecting to find data but don't because the path was wrong).

valentinewallace · 2021-04-02T18:43:25Z

Tested w/ the sample.

codecov · 2021-04-02T18:59:18Z

Codecov Report

Merging #863 (94aa47b) into main (df732f4) will increase coverage by 1.89%.
The diff coverage is 87.09%.

@@ Coverage Diff @@## main #863 +/- ##
==========================================
+ Coverage 90.59% 92.49% +1.89% 
==========================================
Files 51 51 Lines 27195 32604 +5409 ==========================================
+ Hits 24638 30157 +5519 + Misses 2557 2447 -110

Impacted Files	Coverage Δ
lightning-persister/src/lib.rs	`94.95% <87.09%> (-0.66%)`	⬇️
background-processor/src/lib.rs	`98.73% <0.00%> (-1.27%)`	⬇️
lightning/src/routing/router.rs	`96.87% <0.00%> (-0.10%)`	⬇️
lightning/src/ln/reorg_tests.rs	`99.63% <0.00%> (+0.06%)`	⬆️
lightning/src/util/test_utils.rs	`83.23% <0.00%> (+0.28%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`96.07% <0.00%> (+0.85%)`	⬆️
lightning/src/ln/functional_tests.rs	`98.47% <0.00%> (+1.58%)`	⬆️
lightning/src/ln/channel.rs	`91.87% <0.00%> (+4.74%)`	⬆️
lightning/src/ln/channelmanager.rs	`89.25% <0.00%> (+6.02%)`	⬆️

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 df732f4...94aa47b. Read the comment docs.

jkczyz · 2021-04-07T15:55:43Z

+	{
+ let path = self.path_to_monitor_data();
+ if !Path::new(&path).exists() {
+ return Ok(HashMap::new());


Should this be a NotFound error? That way caller's can create the directory if it does not exist.

Hmm, isn't it more likely its just the first startup and thus there's no monitors?

I was thinking that the caller would handle creating the directory. But I guess that FilesystemPersister creates it on demand, so probably ok then. Wasn't sure if we want to give the user insight into this (e.g., they instantiate a persister expecting to find data but don't because the path was wrong).

valentinewallace · 2021-04-02T18:43:25Z

Tested w/ the sample.

codecov · 2021-04-02T18:59:18Z

Codecov Report

Merging #863 (94aa47b) into main (df732f4) will increase coverage by 1.89%.
The diff coverage is 87.09%.

@@ Coverage Diff @@## main #863 +/- ##
==========================================
+ Coverage 90.59% 92.49% +1.89% 
==========================================
Files 51 51 Lines 27195 32604 +5409 ==========================================
+ Hits 24638 30157 +5519 + Misses 2557 2447 -110

Impacted Files	Coverage Δ
lightning-persister/src/lib.rs	`94.95% <87.09%> (-0.66%)`	⬇️
background-processor/src/lib.rs	`98.73% <0.00%> (-1.27%)`	⬇️
lightning/src/routing/router.rs	`96.87% <0.00%> (-0.10%)`	⬇️
lightning/src/ln/reorg_tests.rs	`99.63% <0.00%> (+0.06%)`	⬆️
lightning/src/util/test_utils.rs	`83.23% <0.00%> (+0.28%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`96.07% <0.00%> (+0.85%)`	⬆️
lightning/src/ln/functional_tests.rs	`98.47% <0.00%> (+1.58%)`	⬆️
lightning/src/ln/channel.rs	`91.87% <0.00%> (+4.74%)`	⬆️
lightning/src/ln/channelmanager.rs	`89.25% <0.00%> (+6.02%)`	⬆️

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 df732f4...94aa47b. Read the comment docs.

jkczyz · 2021-04-07T15:55:43Z

+	{
+ let path = self.path_to_monitor_data();
+ if !Path::new(&path).exists() {
+ return Ok(HashMap::new());


Should this be a NotFound error? That way caller's can create the directory if it does not exist.

Hmm, isn't it more likely its just the first startup and thus there's no monitors?

I was thinking that the caller would handle creating the directory. But I guess that FilesystemPersister creates it on demand, so probably ok then. Wasn't sure if we want to give the user insight into this (e.g., they instantiate a persister expecting to find data but don't because the path was wrong).

valentinewallace · 2021-04-02T18:43:25Z

Tested w/ the sample.

codecov · 2021-04-02T18:59:18Z

Codecov Report

Merging #863 (94aa47b) into main (df732f4) will increase coverage by 1.89%.
The diff coverage is 87.09%.

@@ Coverage Diff @@## main #863 +/- ##
==========================================
+ Coverage 90.59% 92.49% +1.89% 
==========================================
Files 51 51 Lines 27195 32604 +5409 ==========================================
+ Hits 24638 30157 +5519 + Misses 2557 2447 -110

Impacted Files	Coverage Δ
lightning-persister/src/lib.rs	`94.95% <87.09%> (-0.66%)`	⬇️
background-processor/src/lib.rs	`98.73% <0.00%> (-1.27%)`	⬇️
lightning/src/routing/router.rs	`96.87% <0.00%> (-0.10%)`	⬇️
lightning/src/ln/reorg_tests.rs	`99.63% <0.00%> (+0.06%)`	⬆️
lightning/src/util/test_utils.rs	`83.23% <0.00%> (+0.28%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`96.07% <0.00%> (+0.85%)`	⬆️
lightning/src/ln/functional_tests.rs	`98.47% <0.00%> (+1.58%)`	⬆️
lightning/src/ln/channel.rs	`91.87% <0.00%> (+4.74%)`	⬆️
lightning/src/ln/channelmanager.rs	`89.25% <0.00%> (+6.02%)`	⬆️

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 df732f4...94aa47b. Read the comment docs.

jkczyz · 2021-04-07T15:55:43Z

+	{
+ let path = self.path_to_monitor_data();
+ if !Path::new(&path).exists() {
+ return Ok(HashMap::new());


Should this be a NotFound error? That way caller's can create the directory if it does not exist.

Hmm, isn't it more likely its just the first startup and thus there's no monitors?

I was thinking that the caller would handle creating the directory. But I guess that FilesystemPersister creates it on demand, so probably ok then. Wasn't sure if we want to give the user insight into this (e.g., they instantiate a persister expecting to find data but don't because the path was wrong).

valentinewallace · 2021-04-02T18:43:25Z

Tested w/ the sample.

codecov · 2021-04-02T18:59:18Z

Codecov Report

Merging #863 (94aa47b) into main (df732f4) will increase coverage by 1.89%.
The diff coverage is 87.09%.

@@ Coverage Diff @@## main #863 +/- ##
==========================================
+ Coverage 90.59% 92.49% +1.89% 
==========================================
Files 51 51 Lines 27195 32604 +5409 ==========================================
+ Hits 24638 30157 +5519 + Misses 2557 2447 -110

Impacted Files	Coverage Δ
lightning-persister/src/lib.rs	`94.95% <87.09%> (-0.66%)`	⬇️
background-processor/src/lib.rs	`98.73% <0.00%> (-1.27%)`	⬇️
lightning/src/routing/router.rs	`96.87% <0.00%> (-0.10%)`	⬇️
lightning/src/ln/reorg_tests.rs	`99.63% <0.00%> (+0.06%)`	⬆️
lightning/src/util/test_utils.rs	`83.23% <0.00%> (+0.28%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`96.07% <0.00%> (+0.85%)`	⬆️
lightning/src/ln/functional_tests.rs	`98.47% <0.00%> (+1.58%)`	⬆️
lightning/src/ln/channel.rs	`91.87% <0.00%> (+4.74%)`	⬆️
lightning/src/ln/channelmanager.rs	`89.25% <0.00%> (+6.02%)`	⬆️

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 df732f4...94aa47b. Read the comment docs.

jkczyz · 2021-04-07T15:55:43Z

+	{
+ let path = self.path_to_monitor_data();
+ if !Path::new(&path).exists() {
+ return Ok(HashMap::new());


Should this be a NotFound error? That way caller's can create the directory if it does not exist.

Hmm, isn't it more likely its just the first startup and thus there's no monitors?

I was thinking that the caller would handle creating the directory. But I guess that FilesystemPersister creates it on demand, so probably ok then. Wasn't sure if we want to give the user insight into this (e.g., they instantiate a persister expecting to find data but don't because the path was wrong).

valentinewallace · 2021-04-02T18:43:25Z

Tested w/ the sample.

codecov · 2021-04-02T18:59:18Z

Codecov Report

Merging #863 (94aa47b) into main (df732f4) will increase coverage by 1.89%.
The diff coverage is 87.09%.

@@ Coverage Diff @@## main #863 +/- ##
==========================================
+ Coverage 90.59% 92.49% +1.89% 
==========================================
Files 51 51 Lines 27195 32604 +5409 ==========================================
+ Hits 24638 30157 +5519 + Misses 2557 2447 -110

Impacted Files	Coverage Δ
lightning-persister/src/lib.rs	`94.95% <87.09%> (-0.66%)`	⬇️
background-processor/src/lib.rs	`98.73% <0.00%> (-1.27%)`	⬇️
lightning/src/routing/router.rs	`96.87% <0.00%> (-0.10%)`	⬇️
lightning/src/ln/reorg_tests.rs	`99.63% <0.00%> (+0.06%)`	⬆️
lightning/src/util/test_utils.rs	`83.23% <0.00%> (+0.28%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`96.07% <0.00%> (+0.85%)`	⬆️
lightning/src/ln/functional_tests.rs	`98.47% <0.00%> (+1.58%)`	⬆️
lightning/src/ln/channel.rs	`91.87% <0.00%> (+4.74%)`	⬆️
lightning/src/ln/channelmanager.rs	`89.25% <0.00%> (+6.02%)`	⬆️

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 df732f4...94aa47b. Read the comment docs.

jkczyz · 2021-04-07T15:55:43Z

+	{
+ let path = self.path_to_monitor_data();
+ if !Path::new(&path).exists() {
+ return Ok(HashMap::new());


Should this be a NotFound error? That way caller's can create the directory if it does not exist.

Hmm, isn't it more likely its just the first startup and thus there's no monitors?

I was thinking that the caller would handle creating the directory. But I guess that FilesystemPersister creates it on demand, so probably ok then. Wasn't sure if we want to give the user insight into this (e.g., they instantiate a persister expecting to find data but don't because the path was wrong).

valentinewallace · 2021-04-02T18:43:25Z

Tested w/ the sample.

codecov · 2021-04-02T18:59:18Z

Codecov Report

Merging #863 (94aa47b) into main (df732f4) will increase coverage by 1.89%.
The diff coverage is 87.09%.

@@ Coverage Diff @@## main #863 +/- ##
==========================================
+ Coverage 90.59% 92.49% +1.89% 
==========================================
Files 51 51 Lines 27195 32604 +5409 ==========================================
+ Hits 24638 30157 +5519 + Misses 2557 2447 -110

Impacted Files	Coverage Δ
lightning-persister/src/lib.rs	`94.95% <87.09%> (-0.66%)`	⬇️
background-processor/src/lib.rs	`98.73% <0.00%> (-1.27%)`	⬇️
lightning/src/routing/router.rs	`96.87% <0.00%> (-0.10%)`	⬇️
lightning/src/ln/reorg_tests.rs	`99.63% <0.00%> (+0.06%)`	⬆️
lightning/src/util/test_utils.rs	`83.23% <0.00%> (+0.28%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`96.07% <0.00%> (+0.85%)`	⬆️
lightning/src/ln/functional_tests.rs	`98.47% <0.00%> (+1.58%)`	⬆️
lightning/src/ln/channel.rs	`91.87% <0.00%> (+4.74%)`	⬆️
lightning/src/ln/channelmanager.rs	`89.25% <0.00%> (+6.02%)`	⬆️

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 df732f4...94aa47b. Read the comment docs.

jkczyz · 2021-04-07T15:55:43Z

+	{
+ let path = self.path_to_monitor_data();
+ if !Path::new(&path).exists() {
+ return Ok(HashMap::new());


Should this be a NotFound error? That way caller's can create the directory if it does not exist.

Hmm, isn't it more likely its just the first startup and thus there's no monitors?

I was thinking that the caller would handle creating the directory. But I guess that FilesystemPersister creates it on demand, so probably ok then. Wasn't sure if we want to give the user insight into this (e.g., they instantiate a persister expecting to find data but don't because the path was wrong).

valentinewallace · 2021-04-02T18:43:25Z

Tested w/ the sample.

codecov · 2021-04-02T18:59:18Z

Codecov Report

Merging #863 (94aa47b) into main (df732f4) will increase coverage by 1.89%.
The diff coverage is 87.09%.

@@ Coverage Diff @@## main #863 +/- ##
==========================================
+ Coverage 90.59% 92.49% +1.89% 
==========================================
Files 51 51 Lines 27195 32604 +5409 ==========================================
+ Hits 24638 30157 +5519 + Misses 2557 2447 -110

Impacted Files	Coverage Δ
lightning-persister/src/lib.rs	`94.95% <87.09%> (-0.66%)`	⬇️
background-processor/src/lib.rs	`98.73% <0.00%> (-1.27%)`	⬇️
lightning/src/routing/router.rs	`96.87% <0.00%> (-0.10%)`	⬇️
lightning/src/ln/reorg_tests.rs	`99.63% <0.00%> (+0.06%)`	⬆️
lightning/src/util/test_utils.rs	`83.23% <0.00%> (+0.28%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`96.07% <0.00%> (+0.85%)`	⬆️
lightning/src/ln/functional_tests.rs	`98.47% <0.00%> (+1.58%)`	⬆️
lightning/src/ln/channel.rs	`91.87% <0.00%> (+4.74%)`	⬆️
lightning/src/ln/channelmanager.rs	`89.25% <0.00%> (+6.02%)`	⬆️

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 df732f4...94aa47b. Read the comment docs.

jkczyz · 2021-04-07T15:55:43Z

+	{
+ let path = self.path_to_monitor_data();
+ if !Path::new(&path).exists() {
+ return Ok(HashMap::new());


Should this be a NotFound error? That way caller's can create the directory if it does not exist.

Hmm, isn't it more likely its just the first startup and thus there's no monitors?

I was thinking that the caller would handle creating the directory. But I guess that FilesystemPersister creates it on demand, so probably ok then. Wasn't sure if we want to give the user insight into this (e.g., they instantiate a persister expecting to find data but don't because the path was wrong).

valentinewallace mentioned this pull request Apr 2, 2021
Sample v0 lightningdevkit/ldk-sample#4
Merged
3 tasks

TheBlueMatt approved these changes Apr 5, 2021
View reviewed changes

Comment threadlightning-persister/src/lib.rs Outdated

TheBlueMatt added this to the 0.0.14 milestone Apr 5, 2021

jkczyz reviewed Apr 7, 2021
View reviewed changes

valentinewallace force-pushed the expose-read-chanmons-from-disk branch from 34e7c66 to bcd1345Compare April 9, 2021 14:23

persister: Expose method to read ChannelMonitors from disk
94aa47b

valentinewallace force-pushed the expose-read-chanmons-from-disk branch from bcd1345 to 94aa47bCompare April 9, 2021 15:19

TheBlueMatt approved these changes Apr 9, 2021
View reviewed changes

jkczyz approved these changes Apr 9, 2021
View reviewed changes

TheBlueMatt merged commit 24c1cfe into lightningdevkit:mainApr 9, 2021