Skip to content

Move justice transactions signature behind signer interface #560

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.

@ariard ariard changed the title Move justice tranactions signature behind signer interface Move justice transactions signature behind signer interface Mar 24, 2020
@ariard ariard force-pushed the 2020-03-move-remote-txn-chansigner branch from 3fa527a to 2f041ee Compare April 25, 2020 07:13
@ariard
Copy link
Author

ariard commented Apr 25, 2020

Rebased on top of #598

@codecov
Copy link

codecov bot commented Apr 25, 2020

Codecov Report

Merging #560 into master will decrease coverage by 0.02%.
The diff coverage is 93.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #560      +/-   ##
==========================================
- Coverage   91.10%   91.08%   -0.03%     
==========================================
  Files          34       34              
  Lines       20447    20520      +73     
==========================================
+ Hits        18628    18690      +62     
- Misses       1819     1830      +11     
Impacted Files Coverage Δ
lightning/src/ln/onchaintx.rs 94.04% <90.12%> (-0.83%) ⬇️
lightning/src/chain/keysinterface.rs 97.11% <100.00%> (+0.13%) ⬆️
lightning/src/ln/chan_utils.rs 97.19% <100.00%> (ø)
lightning/src/ln/channelmonitor.rs 95.62% <100.00%> (+0.12%) ⬆️
lightning/src/util/enforcing_trait_impls.rs 100.00% <100.00%> (ø)
lightning/src/ln/functional_tests.rs 97.04% <0.00%> (-0.10%) ⬇️

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...abf18a1. Read the comment docs.

@TheBlueMatt
Copy link
Collaborator

I'm really not a fan of the direction this is going in terms of OnchainTxHandler. More caching in OnchainTxHandler doesn't feel like the solution - its bad enough with the local_commitment and prev_local_commitment having almost entirely duplicated data but doing the same for remote commitment txn seems wrong. Its not that expensive to copy the data, and we shouldn't ever be in a state where we're tracking 10 different copies of remote HTLCs.

Antoine Riard added 5 commits April 27, 2020 19:00
…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.
@ariard ariard force-pushed the 2020-03-move-remote-txn-chansigner branch from 2f041ee to abf18a1 Compare April 27, 2020 23:01
@ariard
Copy link
Author

ariard commented May 28, 2020

Closed by #610

1 similar comment
@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