Skip to content
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

fix: read scorer #942

Merged
merged 1 commit into from
Jul 13, 2023
Merged

fix: read scorer #942

merged 1 commit into from
Jul 13, 2023

Conversation

bonomat
Copy link
Contributor

@bonomat bonomat commented Jul 12, 2023

With this patch we don't read a historically stored scrorer from disk. It's not ideal, but at least users running into panics when loading the scorer will be able to run the app again

Workaround for #797
fix #797

@bonomat bonomat requested a review from holzeis July 12, 2023 15:48
Copy link
Contributor

@holzeis holzeis left a comment

Choose a reason for hiding this comment

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

@bonomat great, if that fixes the issue 🙂

As we have checked my scorer file was anyways empty, so I guess it doesn't hurt not reading the file from disk?

Are there any big disadvantages of recreating the scorer from scratch every time?

crates/ln-dlc-node/src/disk.rs Outdated Show resolved Hide resolved
@bonomat
Copy link
Contributor Author

bonomat commented Jul 12, 2023

Are there any big disadvantages of recreating the scorer from scratch every time?

I guess we could run without reading from disc. This means, we will not learn from past payments and only keep an in-memory picture of the network.

@bonomat bonomat requested a review from klochowicz July 12, 2023 18:12
@bonomat bonomat marked this pull request as ready for review July 12, 2023 18:12
@bonomat
Copy link
Contributor Author

bonomat commented Jul 12, 2023

@ reviewers :

  1. The first commit fixed the problem for me
  2. The second commit might be the better fix but I can't test it because it does not panic anymore.

If we go for the second, let's squash the commits.
I propose to roll out a new version with this asap, because you really can't use the app if this happens to you.

Copy link
Contributor

@klochowicz klochowicz left a comment

Choose a reason for hiding this comment

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

I would go for solution 2 - it does not panic anymore, and we're logging potential problems, without sweeping them completely under the rug.

On iOS it seems like we can run into a problem where we can't read the scorer file anymore. Reading will then just panic. Here we catch the panic and delete the broken file which allows us to continue.
@holzeis
Copy link
Contributor

holzeis commented Jul 13, 2023

bors r+

@holzeis
Copy link
Contributor

holzeis commented Jul 13, 2023

I propose to roll out a new version with this asap, because you really can't use the app if this happens to you.

@bonomat fyi: https://github.com/get10101/10101/actions/runs/5540939930/jobs/10113681545

@bors
Copy link
Contributor

bors bot commented Jul 13, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 4f8caaa into main Jul 13, 2023
7 checks passed
@bors bors bot deleted the fix/scorer branch July 13, 2023 08:49
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.

Panic on reading scorer from disk
3 participants