Creating fresh destination scripts for every channel in KeysManager #2174
Conversation
vss96
force-pushed
the
Fresh-DestScripts
branch
from
52a983d to
4e52db2
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2174 +/- ##
==========================================
- Coverage 91.57% 91.56% -0.01%
==========================================
Files 104 104
Lines 51782 51802 +20
Branches 51782 51802 +20
==========================================
+ Hits 47417 47434 +17
- Misses 4365 4368 +3 ☔ View full report in Codecov by Sentry. |
| } | ||
|
|
||
| fn get_shutdown_scriptpubkey(&self) -> ShutdownScript { | ||
| ShutdownScript::new_p2wpkh_from_pubkey(self.shutdown_pubkey.clone()) | ||
| ShutdownScript::new_p2wpkh_from_pubkey(self.shutdown_pubkey.public_key.clone()) |
There was a problem hiding this comment.
Should we also make this different per channel too?
There was a problem hiding this comment.
Will deriving from the existing shutdown_pubkey at some fixed index and creating a shutdown script work?
There was a problem hiding this comment.
I'd think we just use the same derivation logic (effectively merging destination and shutdown scripts).
vss96
force-pushed
the
Fresh-DestScripts
branch
from
4e52db2 to
9e0d1e6
Compare
Change order for methods in the impl block for NodeSigner for KeysManger. Add channel_keys_id to the methods implementing KeyProvider's get_destination_script.
vss96
force-pushed
the
Fresh-DestScripts
branch
from
9e0d1e6 to
733aee3
Compare
| @@ -1087,6 +1093,7 @@ impl KeysManager { | |||
|
|
|||
| channel_master_key, | |||
| channel_child_index: AtomicUsize::new(0), | |||
| disallow_downgrades: true, | |||
There was a problem hiding this comment.
This needs to be a parameter/configurable.
| let derivation_idx = if self.disallow_downgrades { | ||
| u32::from_be_bytes(channel_keys_id[0..4].try_into().unwrap()) | ||
| } else { | ||
| 3 |
There was a problem hiding this comment.
Destination key was 1 on current git, so 3 is a totally different key.
| let derivation_idx = if output.script_pubkey == self.destination_script { | ||
| 1 | ||
| } else { | ||
| } else if Some(&output.script_pubkey) == destination_script.as_ref() { |
There was a problem hiding this comment.
I'm not convinced the logic here is right, the destination script in the output should always be what get_destination_script gives us, so we need to use the derivation idx as calculated there, not just always use the channel_keys_id one here.
|
Looks like the Signerprovider has changed 😞 |
|
Indeed, sorry about that. It hasn't changed a lot in practice, just renames and some splits. The rebase may be a bit annoying but its all the same code, really, so should be pretty doable. |
|
@vss96 are you still working on this? Was hoping to try to get this in before next release |
|
@benthecarman if that's the case and you want to take this ahead please do so. I have been stuck on this for a while and I don't know if I can get it in before the next release. |
|
Any desire to pick this back up @vss96? |
|
When we do this we should also make the |
Yes! let me try to rebase and fix the conflicts 😪 |
|
Closing as abandoned. |
Continuing from #1312. This pr is to address #1139.