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: reject pending offers #1763

Merged
merged 5 commits into from
Jan 2, 2024
Merged

fix: reject pending offers #1763

merged 5 commits into from
Jan 2, 2024

Conversation

bonomat
Copy link
Contributor

@bonomat bonomat commented Dec 15, 2023

I was able to reject an old unacceptable sub channel offer with this, however, I was then in a broken state. See here: #1761

@bonomat bonomat force-pushed the fix/reject-outdated-offers branch from 450eea6 to 04ef4ce Compare December 15, 2023 15:30
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.

LGTM 👍

I was able to reject an old unacceptable sub channel offer with this, however, I was then in a broken state. See here: #1761

I think your guess that a key was not successfully backed up is very likely for why you ended up in the broken state.

runtime.spawn({
let mut iteration_count = 0;

while !node
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 is this really needed. It think the channel status is derived from the shadow channel in the database.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I failed once to accept the order because I could not send a message to the coordinator to accept the offer because I was not connected :/

Copy link
Contributor

@holzeis holzeis Dec 15, 2023

Choose a reason for hiding this comment

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

🙃 But then it's not related to the track_channel_status but to the sync_position_to_subchannel_state

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are completely right, this might have happened in a rebase. I'll fix it.

@bonomat bonomat force-pushed the fix/reject-outdated-offers branch 2 times, most recently from df38f76 to f2a273b Compare December 16, 2023 19:09
@bonomat bonomat added this pull request to the merge queue Dec 18, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 18, 2023
if let Err(e) = node.sync_position_with_subchannel_state().await {
tracing::error!("Failed to sync position with subchannel state. Error: {e:#}");
}
runtime.spawn({
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 Could that be potentially dangerous as it's not blocking?

What if the user is opening / closing a position in the meantime?

@holzeis
Copy link
Contributor

holzeis commented Dec 21, 2023

@bonomat is there anything left on this PR or can we get it merged?

@bonomat
Copy link
Contributor Author

bonomat commented Dec 29, 2023

@bonomat is there anything left on this PR or can we get it merged?

I thought I had pushed the fix for your last comment 🙈

I'm using spawn_blocking now.

In `track_channel_status` we might need a connection to the coordinator. This is not always guaranteed because we spawn the connection task async as well. Hence, we wait here up to 1 minute. If we still haven't connected we give up and continue nevertheless. The below function call _might_ fail in certain cases but it's not the end of the world as we will retry simply when the user restarts the app the next time.
Hopefully by then the coordinator is reachable
This patch adds two things:
1. we check if we have any pending subchannel offers which need to be handled. We check if the subchannel offer is still valid, and if so, we accept, otherwise we reject.
2. if we fail accepting a subchannel offer, we reject it. This is needed so that we can recover in case of an offer which can't be accepted. The user is then invited to simply try again.
@bonomat bonomat force-pushed the fix/reject-outdated-offers branch from f2a273b to abbe1bd Compare December 29, 2023 19:47
@bonomat bonomat added this pull request to the merge queue Jan 2, 2024
@bonomat
Copy link
Contributor Author

bonomat commented Jan 2, 2024

@holzeis : I'm going ahead and merge this with my last fix.

Merged via the queue into main with commit d594a73 Jan 2, 2024
9 checks passed
@bonomat bonomat deleted the fix/reject-outdated-offers branch January 2, 2024 07:35
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.

2 participants