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.

Sign up for GitHub

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

Jump to bottom

Fix Musig2RoundFinalizeError caused by concurrency problem #493

Open
wants to merge 37 commits into
base: develop
Choose a base branch
from
Open

Fix Musig2RoundFinalizeError caused by concurrency problem #493

wants to merge 37 commits into from

Conversation

chenyukang
Copy link
Collaborator

@chenyukang chenyukang commented Jan 19, 2025 •
edited
Loading

Fixes #480
Fixes #475

When there are multiple RemoveTlcFail happened at the same time, node A's TLC with RemoveTlcFail may in RemoveWaitAck, node B's corresponding TLC may already be removed from memory, so Musig2RoundFinalizeError will be triggered.

In this PR, we keep RemoveTlcFail in TLC list and will remove it when the number exceeds 2 in any direction of the channel.

@chenyukang chenyukang force-pushed the yukang-fix-480-keep-fail-tlc branch from 49fd486 to 8fcd2f0 Compare January 22, 2025 06:55
@chenyukang chenyukang changed the title Fix Musig2RoundFinalizeError caused by currency problem Fix Musig2RoundFinalizeError caused by concurrency problem Jan 22, 2025
@chenyukang chenyukang force-pushed the yukang-fix-480-keep-fail-tlc branch from 8fcd2f0 to e21cfda Compare January 22, 2025 07:03
@codecov-commenter
Copy link

codecov-commenter commented Jan 22, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 67.76860% with 39 lines in your changes missing coverage. Please review.

Project coverage is 50.60%. Comparing base (2f12f40) to head (89c1af4).

Files with missing lines Patch % Lines
src/fiber/types.rs 0.00% 21 Missing ⚠️
src/fiber/channel.rs 85.55% 13 Missing ⚠️
src/fiber/network.rs 33.33% 2 Missing ⚠️
src/rpc/channel.rs 0.00% 2 Missing ⚠️
src/fiber/history.rs 80.00% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop     #493      +/-   ##
===========================================
+ Coverage    50.53%   50.60%   +0.06%     
===========================================
  Files           49       49              
  Lines        31878    31978     +100     
===========================================
+ Hits         16110    16182      +72     
- Misses       15768    15796      +28
Flag Coverage Δ
unittests 50.60% <67.76%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

contrun
contrun reviewed Jan 22, 2025
View reviewed changes
src/fiber/channel.rs Show resolved Hide resolved
src/fiber/channel.rs Outdated Show resolved Hide resolved
src/fiber/tests/test_utils.rs
unexpected_events.insert(event.to_string());
}

let unexpected_events = Arc::new(TokioRwLock::new(unexpected_events));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think using OutputPort in ractor::port::output - Rust and OutputPortSubscriber in ractor::port::output - Rust for current event_receiver/event_sender would be better, because OutputPort/OutputPortSubscriber natively supports broadcasting (send events to multiple subscribers) and filtering out uninterested events (creating a subscriber to subscribe only unexpected events is easy). Of course, using OutputPort/OutputPortSubscriber instead of mpsc::Sender<NetworkServiceEvent>/mpsc::Receiver<NetworkServiceEvent> requires much more work.

src/fiber/channel.rs Show resolved Hide resolved
@chenyukang chenyukang force-pushed the yukang-fix-480-keep-fail-tlc branch from 2405eb8 to 6f436fb Compare January 22, 2025 16:44
quake
quake reviewed Jan 23, 2025
View reviewed changes
src/fiber/channel.rs Outdated Show resolved Hide resolved
src/fiber/channel.rs Outdated Show resolved Hide resolved
src/fiber/channel.rs Outdated Show resolved Hide resolved
src/fiber/history.rs Outdated Show resolved Hide resolved
@chenyukang chenyukang force-pushed the yukang-fix-480-keep-fail-tlc branch from bd6bd8b to 53a264e Compare January 23, 2025 08:43
@chenyukang chenyukang force-pushed the yukang-fix-480-keep-fail-tlc branch 2 times, most recently from c5c77b0 to dd5c346 Compare January 23, 2025 09:17
@chenyukang chenyukang force-pushed the yukang-fix-480-keep-fail-tlc branch from dd5c346 to 07d054d Compare January 23, 2025 09:18
@chenyukang chenyukang force-pushed the yukang-fix-480-keep-fail-tlc branch from 433a54a to 89fb81c Compare January 24, 2025 04:29
@chenyukang chenyukang force-pushed the yukang-fix-480-keep-fail-tlc branch 2 times, most recently from bb4c1bb to 77555e5 Compare January 25, 2025 02:05
@chenyukang chenyukang force-pushed the yukang-fix-480-keep-fail-tlc branch from 77555e5 to d665a40 Compare January 25, 2025 02:09
@chenyukang chenyukang force-pushed the yukang-fix-480-keep-fail-tlc branch from 2804cc0 to 77577ab Compare January 26, 2025 01:51
@chenyukang chenyukang force-pushed the yukang-fix-480-keep-fail-tlc branch from 89c1af4 to 8390836 Compare January 26, 2025 03:22
@gpBlockchain
Copy link
Contributor

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@contrun contrun contrun left review comments

@quake quake quake left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@chenyukang @codecov-commenter @gpBlockchain @quake @contrun