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

[Splicing] Clone for ChannelContext #3332

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

optout21
Copy link
Contributor

@optout21 optout21 commented Sep 23, 2024

Cloning of a channel -- more precisely of ChannelContext -- is needed for Splicing, this is a preparation (may be needed for dual funding RBF as well)

Simple #[derive(Clone)) is not sufficient, as the channel signer struct cannot be cloned, ultimately due to AtomicUSize and Secp256k1 (in KeysManager). So instead, a field-by-field cloning is done, with the a few exceptions.

@optout21 optout21 marked this pull request as ready for review September 25, 2024 19:04
Copy link

codecov bot commented Sep 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.00%. Comparing base (ba3d4ff) to head (1cdce16).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3332      +/-   ##
==========================================
+ Coverage   89.25%   90.00%   +0.74%     
==========================================
  Files         130      130              
  Lines      106959   111140    +4181     
  Branches   106959   111140    +4181     
==========================================
+ Hits        95464   100029    +4565     
+ Misses       8706     8343     -363     
+ Partials     2789     2768      -21     

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

@jkczyz jkczyz self-requested a review September 26, 2024 02:39
@optout21 optout21 marked this pull request as draft October 3, 2024 18:09
@jkczyz
Copy link
Contributor

jkczyz commented Oct 3, 2024

Discussed briefly offline. This may not be needed if we split ChannelContext to avoid duplicating some fields.

@optout21
Copy link
Contributor Author

optout21 commented Oct 3, 2024

This change request assumes that the whole ChannelContext struct is duplicated, which may not be the best solution. I put this on hold for now (back to draft, description updated).

@optout21 optout21 marked this pull request as ready for review November 26, 2024 14:13
@jkczyz
Copy link
Contributor

jkczyz commented Nov 27, 2024

Not sure if cloning is the best solution. But, regardless, since the channel signer is a Deref can't we just add a Clone trait bound?

@optout21
Copy link
Contributor Author

There are 4 Mutex fields, used for testing/fuzzing: I placed them under refcounting (`Arc), so they can be cloned.

The only non-cloneable field is the signer. The signer could also be placed under Arc, but I find that unjustified complexity and inefficiency for this infrequent use case.

@optout21
Copy link
Contributor Author

optout21 commented Dec 5, 2024

Still up for discussion, different approaches being discussed, to get rid of per-field cloning:

  • change inside Signer so that it is clonable (it is not due to atomic counters)
  • in relation with the channel phase refactor Channel phase refactor, wrap channel phase, store wrapped phase in map #3418, possible move signer out of channel context to the channel wrapper, so it doesn't have to be cloned. It would affect methods which uses the signer (to be moved 'out' as well or pass the signer)
  • place the signer behind Arc (Arc<Mutex>?) -- not preferred, unnecessary complexity/inefficiency

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

Successfully merging this pull request may close these issues.

2 participants