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

Support future removal of redundant per-HTLC signatures in CMUs #2101

Merged
merged 2 commits into from
Apr 6, 2023

Conversation

TheBlueMatt
Copy link
Collaborator

ChannelMonitorUpdates are our most size-sensitive objects - they are the minimal objects which need to be written to disk on each commitment update. Thus, we should be careful to ensure we don't pack too much extraneous information into each one.

Here we add future support for removing the per-HTLC explicit Option<Signature> in holder commitment tx updates, which are redundant with the HolderCommitmentTransaction.

While we cannot remove them entirely as previous versions rely on them, adding support for filling in the in-memory structures from the redundant fields will let us remove them in a future version.

@TheBlueMatt TheBlueMatt changed the title tSupport future removal of redundant per-HTLC signatures in CMUs Support future removal of redundant per-HTLC signatures in CMUs Mar 13, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 13, 2023

Codecov Report

Patch coverage: 98.63% and project coverage change: +0.48 🎉

Comparison is base (23c1b46) 91.14% compared to head (1b90060) 91.63%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2101      +/-   ##
==========================================
+ Coverage   91.14%   91.63%   +0.48%     
==========================================
  Files         101      101              
  Lines       48888    55401    +6513     
  Branches    48888    55401    +6513     
==========================================
+ Hits        44561    50767    +6206     
- Misses       4327     4634     +307     
Impacted Files Coverage Δ
lightning/src/util/ser_macros.rs 92.74% <ø> (+0.66%) ⬆️
lightning/src/chain/channelmonitor.rs 94.49% <98.00%> (+0.72%) ⬆️
lightning/src/ln/chan_utils.rs 94.75% <100.00%> (+0.99%) ⬆️
lightning/src/ln/channel.rs 90.50% <100.00%> (+1.13%) ⬆️
lightning/src/ln/channelmanager.rs 92.13% <100.00%> (+3.26%) ⬆️

... and 37 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved
lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved
@@ -493,6 +493,9 @@ impl_writeable_tlv_based_enum_upgradable!(OnchainEvent,
pub(crate) enum ChannelMonitorUpdateStep {
LatestHolderCommitmentTXInfo {
commitment_tx: HolderCommitmentTransaction,
/// Note that the signature is redundant, and can be loaded from the `commitment_tx` as of
/// LDK 0.0.115. After some time it should be set to `None` to reduce the size of the
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we be more specific with "after some time"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, not really? We haven't really decided at what interval we can break backwards compat for different parts of the API. I'd imagine for this we could consider doing it in two years - having monitor updates lying around isn't all that common anyway, but two years is probably about as big a gap as we'd ever want to reasonably support.

@TheBlueMatt TheBlueMatt force-pushed the 2023-03-one-less-sig branch 2 times, most recently from b0367ec to 2e869df Compare March 20, 2023 23:27
`vec_type` is confusing - it is happy to have a missing entry,
"reading" an empty `Vec` instead, but always writes something,
making a serialization round-trip different.

This is a problem for writing a new `Vec` which is
backwards-incompatible, but only if filled in. In that case we'd
really like the same read behavior, but not write anything if the
`Vec` is empty. Here we introduce such semantics via a new
`optional_vec` TLV format.
@TheBlueMatt TheBlueMatt force-pushed the 2023-03-one-less-sig branch from 2e869df to 6f7b3c2 Compare March 20, 2023 23:29
Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

CI is not happy:

error[E0599]: no method named `possibly_matches_output` found for enum `HTLCSource` in the current scope
    --> lightning/src/chain/channelmonitor.rs:2204:27
     |
2204 |                     debug_assert!(source.possibly_matches_output(htlc));
     |                                          ^^^^^^^^^^^^^^^^^^^^^^^ method not found in `HTLCSource`
     |
    ::: lightning/src/ln/channelmanager.rs:268:1
     |
268  | pub(crate) enum HTLCSource {
     | -------------------------- method `possibly_matches_output` not found for this enum

lightning/src/ln/channel.rs Show resolved Hide resolved
Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

LGTM after squash

`ChannelMonitorUpdate`s are our most size-sensitive objects - they
are the minimal objects which need to be written to disk on each
commitment update. Thus, we should be careful to ensure we don't
pack too much extraneous information into each one.

Here we add future support for removing the per-HTLC explicit
`Option<Signature>` and `HTLCInCommitmentUpdate` for non-dust HTLCs
in holder commitment tx updates, which are redundant with the
`HolderCommitmentTransaction`.

While we cannot remove them entirely as previous versions rely on
them, adding support for filling in the in-memory structures from
the redundant fields will let us remove them in a future version.

We also add test-only generation logic to test the new derivation.
@TheBlueMatt TheBlueMatt force-pushed the 2023-03-one-less-sig branch from 1b90060 to b72f6b1 Compare March 24, 2023 19:02
@TheBlueMatt
Copy link
Collaborator Author

Squashed without further changes.

@TheBlueMatt
Copy link
Collaborator Author

CI apparently failed to download electrs, I kicked it:


error: failed to run custom build command for `electrsd v0.22.2`

Caused by:
  process didn't exit successfully: `/Users/runner/work/rust-lightning/rust-lightning/lightning-transaction-sync/target/debug/build/electrsd-c06f0f4896161acd/build-script-build` (exit status: 101)
  --- stdout
  filename:electrs_macos_esplora_a33e97e1a1fc63fa9c20a116bb92579bbf43b254.zip version:esplora_a33e97e1a1fc63fa9c20a116bb92579bbf43b254 hash:2d5ff149e8a2482d3658e9b386830dfc40c8fbd7c175ca7cbac58240a9505bcd

  --- stderr
  [/Users/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/electrsd-0.22.2/build.rs:29] &download_filename = "electrs_macos_esplora_a33e97e1a1fc63fa9c20a116bb92579bbf43b254.zip"
  [/Users/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/electrsd-0.22.2/build.rs:37] &destination_filename = "/Users/runner/work/rust-lightning/rust-lightning/lightning-transaction-sync/target/debug/build/electrsd-eb25646f46bc0701/out/electrs/electrs_macos_esplora_a33e97e1a1fc63fa9c20a116bb92579bbf43b254/electrs"
  thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Transport(Transport { kind: ConnectionFailed, message: Some("tls connection init failed"), url: Some(Url { scheme: "https", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("github.com")), port: None, path: "/RCasatta/electrsd/releases/download/electrs_releases/electrs_macos_esplora_a33e97e1a1fc63fa9c20a116bb92579bbf43b254.zip", query: None, fragment: None }), source: Some(Custom { kind: InvalidData, error: InvalidCertificateData("invalid peer certificate: CertExpired") }) })', /Users/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/electrsd-0.22.2/build.rs:50:14
  stack backtrace:
     0: rust_begin_unwind
               at /rustc/8460ca823e8367a30dda430efda790588b8c84d3/library/std/src/panicking.rs:575:5
     1: core::panicking::panic_fmt
               at /rustc/8460ca823e8367a30dda430efda790588b8c84d3/library/core/src/panicking.rs:64:14
     2: core::result::unwrap_failed
               at /rustc/8460ca823e8367a30dda430efda790588b8c84d3/library/core/src/result.rs:1790:5
     3: core::result::Result<T,E>::unwrap
               at /rustc/8460ca823e8367a30dda430efda790588b8c84d3/library/core/src/result.rs:1112:23
     4: build_script_build::main
               at ./build.rs:48:21
     5: core::ops::function::FnOnce::call_once
               at /rustc/8460ca823e8367a30dda430efda790588b8c84d3/library/core/src/ops/function.rs:250:5
  note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
warning: build failed, waiting for other jobs to finish...

Copy link
Contributor

@alecchendev alecchendev left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -2160,7 +2165,53 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
/// is important that any clones of this channel monitor (including remote clones) by kept
/// up-to-date as our holder commitment transaction is updated.
/// Panics if set_on_holder_tx_csv has never been called.
fn provide_latest_holder_commitment_tx(&mut self, holder_commitment_tx: HolderCommitmentTransaction, htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>, claimed_htlcs: &[(SentHTLCId, PaymentPreimage)]) -> Result<(), &'static str> {
fn provide_latest_holder_commitment_tx(&mut self, holder_commitment_tx: HolderCommitmentTransaction, mut htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>, claimed_htlcs: &[(SentHTLCId, PaymentPreimage)], nondust_htlc_sources: Vec<HTLCSource>) -> Result<(), &'static str> {
if htlc_outputs.iter().any(|(_, s, _)| s.is_some()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Comprehension check: do dust HTLCs not have signatures because there wouldn't be a point since they have no output to ever spend/enforce on-chain?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct. There's simply nothing to sign.

Comment on lines +3132 to +3138
let mut separate_nondust_htlc_sources = false;
#[cfg(all(feature = "std", any(test, fuzzing)))] {
use core::hash::{BuildHasher, Hasher};
// Get a random value using the only std API to do so - the DefaultHasher
let rand_val = std::collections::hash_map::RandomState::new().build_hasher().finish();
separate_nondust_htlc_sources = rand_val % 2 == 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I get how this would work well with fuzzing, but for normal tests would you just have to run them a couple times to be confident the new implementation got tested? I guess this may be a temporary solution for testing so it's not a big deal?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, we have a bit of randomness in tests which avoids having to run every test five times but ensures over time we will hit any issues.

@TheBlueMatt TheBlueMatt merged commit ba13499 into lightningdevkit:main Apr 6, 2023
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.

5 participants