rust-lightning icon indicating copy to clipboard operation
rust-lightning copied to clipboard

[Splicing] Clone for ChannelContext

Open optout21 opened this issue 1 year ago • 3 comments

Update: This assumes that the whole ChannelContext is duplicated, which may not be the case in the final solution.

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

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 avatar Sep 23 '24 21:09 optout21

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Sep 25 '24 19:09 codecov[bot]

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

jkczyz avatar Oct 03 '24 18:10 jkczyz

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 avatar Oct 03 '24 18:10 optout21

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?

jkczyz avatar Nov 27 '24 17:11 jkczyz

There are 4 Mutex fields, used for testing/fuzzing: I placed them under refcounting (`Arc<Mutex>), 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 avatar Nov 29 '24 05:11 optout21

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 #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

optout21 avatar Dec 05 '24 18:12 optout21

Rebased

optout21 avatar Jan 07 '25 14:01 optout21

Superseded by #3592 and #3604.

jkczyz avatar Feb 19 '25 21:02 jkczyz