Skip to content

Move spendable output descriptors key behind signer interface #562

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

Closed

Conversation

ariard
Copy link

@ariard ariard commented Mar 24, 2020

Build on top of #559 + #560 + #561.

Once merged, we don't have hot private keys anymore in OnchainTxHandler/ChannelMonitor :)

@@ -469,6 +472,18 @@ impl ChannelKeys for InMemoryChannelKeys {
Ok(secp_ctx.sign(&sighash, &self.funding_key))
}

fn sign_delayed_transaction<T: secp256k1::Signing>(&self, spend_tx: &mut Transaction, input: usize, witness_script: &Script, amount: u64, per_commitment_point: &PublicKey, secp_ctx: &Secp256k1<T>) {
Copy link
Member

Choose a reason for hiding this comment

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

the witness_script could be derived here rather than passed in, if I'm not mistaken

@@ -469,6 +472,18 @@ impl ChannelKeys for InMemoryChannelKeys {
Ok(secp_ctx.sign(&sighash, &self.funding_key))
}

fn sign_delayed_transaction<T: secp256k1::Signing>(&self, spend_tx: &mut Transaction, input: usize, witness_script: &Script, amount: u64, per_commitment_point: &PublicKey, secp_ctx: &Secp256k1<T>) {
if let Ok(htlc_key) = chan_utils::derive_private_key(&secp_ctx, &per_commitment_point, &self.delayed_payment_base_key) {
Copy link
Member

Choose a reason for hiding this comment

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

failing here likely indicates a serious internal error, so should propagate the error to the caller

@@ -484,6 +488,19 @@ impl ChannelKeys for InMemoryChannelKeys {
}
}

fn sign_payment_transaction<T: secp256k1::Signing>(&self, spend_tx: &mut Transaction, input: usize, amount: u64, per_commitment_point: &PublicKey, network: Network, secp_ctx: &Secp256k1<T>) {
Copy link
Member

Choose a reason for hiding this comment

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

see two comments on sign_delayed_transaction

@ariard
Copy link
Author

ariard commented Apr 25, 2020

Rebased on top of #561+#562+#598

@codecov
Copy link

codecov bot commented Apr 25, 2020

Codecov Report

Merging #562 into master will increase coverage by 0.00%.
The diff coverage is 95.86%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master     #562    +/-   ##
========================================
  Coverage   91.10%   91.10%            
========================================
  Files          34       34            
  Lines       20447    20565   +118     
========================================
+ Hits        18628    18736   +108     
- Misses       1819     1829    +10     
Impacted Files Coverage Δ
lightning/src/ln/functional_test_utils.rs 94.63% <ø> (ø)
lightning/src/ln/onchaintx.rs 94.05% <91.39%> (-0.82%) ⬇️
lightning/src/ln/channelmonitor.rs 95.58% <96.42%> (+0.08%) ⬆️
lightning/src/chain/keysinterface.rs 97.42% <100.00%> (+0.44%) ⬆️
lightning/src/ln/chan_utils.rs 97.19% <100.00%> (ø)
lightning/src/ln/functional_tests.rs 97.05% <100.00%> (-0.10%) ⬇️
lightning/src/util/enforcing_trait_impls.rs 100.00% <100.00%> (ø)
lightning/src/util/ser_macros.rs 97.27% <0.00%> (+0.68%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12e2a81...7a23c0c. Read the comment docs.

Antoine Riard added 10 commits April 27, 2020 19:02
…TxCache

Used in next commits to avoid passing script between ChannelMonitor
and OnchainTxHandler. ChannelMonitor duplicata will be removed
in future commits.
As we can't predict if any and which revoked commitment tx is
going to appear onchain we have by design to cache all htlc information
to regenerate htlc script if needed.
As we cache more and more transaction elements in OnchainTxHandler
we should dry up completly InputMaterial until them being replaced
directly by InputDescriptor
By moving script generation inside OnchainTxHandler, we may dry-up
further ChannelMonitor in next commits.
OnchainTxHandler

By moving script generation inside OnchainTxHandler, we may dry-up
further ChannelMonitor in next commits
Add sign_delayed_transaction in ChanSigner to be able to spend
SpendableOutputDescriptor in test framework.
Add sign_payment_transaction in ChanSigner to be able to spend
SpendableOutputDescriptor in test framework
@ariard ariard force-pushed the 2020-03-remove-seckey-chanmon branch from 81bc09a to 7a23c0c Compare April 27, 2020 23:02
@ariard
Copy link
Author

ariard commented May 28, 2020

Closed by #610

@ariard ariard closed this May 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants