add `Fallback` getter that returns `Address` #2023

futurepaul · 2023-02-08T15:18:48Z

closes #882

I wasn't sure the exact right strategy and wrote this before seeing the most recent comment on this issue, so feel free to close if this is the wrong direction.

A few notes:

I don't know of a way to impl Into because the Fallback on its own isn't enough to create an Address, you also need a network.
I wasn't sure what to do about returning a Result (and what errors that would require) because this is data that's already been successfully parsed and theoretically isn't wrong. Therefore I'm using unwraps which feels wrong, so would appreciate some input on that.
There's a potentially deeper fix here by using the bitcoin lib's WitnessVersion, Script, and hash types inside Fallback (there's even a todo on Fallback about using better types)
I'm not exactly sure why I didn't end up having to clone even though I take a reference, but I'm not going to look a gift horse in the mouth.
I convert Currency::Simnet to Network::Regtest, not sure if that's correct

jkczyz · 2023-02-09T19:03:43Z

Thanks for putting this together!

closes #882

I wasn't sure the exact right strategy and wrote this before seeing the most recent comment on this issue, so feel free to close if this is the wrong direction.

Chatted with @TheBlueMatt. From the perspective from Invoice interface, this looks like the direction we want.

A few notes:

I don't know of a way to impl Into because the Fallback on its own isn't enough to create an Address, you also need a network.

You could implement it for a tuple (&Fallback, Currency), but seems like we'd rather not use Into anyhow.

I wasn't sure what to do about returning a Result (and what errors that would require) because this is data that's already been successfully parsed and theoretically isn't wrong. Therefore I'm using unwraps which feels wrong, so would appreciate some input on that.

There's a potentially deeper fix here by using the bitcoin lib's WitnessVersion, Script, and hash types inside Fallback (there's even a todo on Fallback about using better types)

For these two points, yeah, it would better if we use better types. Could you update the enum variants to use them?

I'm not exactly sure why I didn't end up having to clone even though I take a reference, but I'm not going to look a gift horse in the mouth.

Clone wasn't need but the bytes are eventually copied, so somewhat similar.

I convert Currency::Simnet to Network::Regtest, not sure if that's correct

This seems right. Not sure how an LND-specific concept got into here lol. We inherited this code. 😛

lightning-invoice/src/lib.rs

lightning-invoice/src/utils.rs

futurepaul · 2023-02-09T23:48:31Z

Down to one unwrap in the address creation, so that's nice. Can do a Vec<Result<Address>> if that's feels better

codecov-commenter · 2023-02-10T00:17:11Z

Codecov Report

Patch coverage: 74.07% and project coverage change: +0.23 🎉

Comparison is base (56146e7) 91.04% compared to head (61db97d) 91.28%.

📣 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    #2023      +/-   ##
==========================================
+ Coverage   91.04%   91.28%   +0.23%     
==========================================
  Files          99      102       +3     
  Lines       51722    49994    -1728     
  Branches    51722    49994    -1728     
==========================================
- Hits        47091    45635    -1456     
+ Misses       4631     4359     -272

Impacted Files	Coverage Δ
lightning-invoice/src/ser.rs	`92.43% <0.00%> (+0.24%)`	⬆️
lightning-invoice/src/lib.rs	`79.91% <68.42%> (-6.41%)`	⬇️
lightning-invoice/src/de.rs	`75.81% <100.00%> (-6.58%)`	⬇️

... and 99 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-invoice/src/lib.rs

jkczyz

Sorry about the delay. Travel go the best of me.

lightning-invoice/src/ser.rs

lightning-invoice/src/de.rs

jkczyz · 2023-02-13T16:17:45Z

lightning-invoice/src/de.rs

 				if bytes.len() != 20 {
 					return Err(ParseError::InvalidPubKeyHashLength);
 				}
-				//TODO: refactor once const generics are available
-				let mut pkh = [0u8; 20];
-				pkh.copy_from_slice(&bytes);
+				let pkh = match PubkeyHash::from_slice(&bytes) {
+					Ok(pkh) => pkh,
+					Err(_) => return Err(ParseError::InvalidPubKeyHash),
+				};


from_slice will check the length, so we can remove our explicit check in favor of the new one. Probably could have the match pattern on the exact error variant. That way we won't need to add a new error variant and we'll fail to compile if new variants are added upstream.

nice, thank you this worked out great

jkczyz · 2023-02-13T16:17:56Z

lightning-invoice/src/de.rs

 				if bytes.len() != 20 {
 					return Err(ParseError::InvalidScriptHashLength);
 				}
-				let mut sh = [0u8; 20];
-				sh.copy_from_slice(&bytes);
+				let sh = match ScriptHash::from_slice(&bytes) {
+					Ok(sh) => sh,
+					Err(_) => return Err(ParseError::InvalidScriptHash),
+				};


Likewise here.

yeah I got to get rid of all three errors I added!

lightning-invoice/src/lib.rs

TheBlueMatt · 2023-03-20T16:45:05Z

Any update on addressing the review feedback @futurepaul?

futurepaul · 2023-03-22T16:49:56Z

Sorry for the long delay, will be able to get back to this in about a week

futurepaul · 2023-04-04T17:12:07Z

okay @jkczyz @TheBlueMatt this addresses the review feedback

TheBlueMatt

Nice!

lightning-invoice/src/de.rs

lightning-invoice/src/lib.rs

TheBlueMatt · 2023-04-04T21:02:21Z

Please feel free to squash down your fixups so that each commit stands on its own and doesn't fix issues introduced in previous commits in the same PR. THen we'll get another reviewer here.

futurepaul · 2023-04-04T21:35:10Z

couldn't think of a clean way to separate out this work because the use of the bitcoin types directly impacted how the getter is written so hope it's okay I squashed down to one commit

jkczyz · 2023-04-06T02:35:21Z

couldn't think of a clean way to separate out this work because the use of the bitcoin types directly impacted how the getter is written so hope it's okay I squashed down to one commit

FWIW, you could add fallback_addresses in a second commit since the first one would have all the types changed.

jkczyz

Just some nits that I missed but otherwise looks good.

lightning-invoice/src/de.rs

lightning-invoice/src/lib.rs

0.0.115 - Apr 24, 2023 - "Rebroadcast the Bugfixes" API Updates =========== * The MSRV of the main LDK crates has been increased to 1.48 (lightningdevkit#2107). * Attempting to claim an un-expired payment on a channel which has closed no longer fails. The expiry time of payments is exposed via `PaymentClaimable::claim_deadline` (lightningdevkit#2148). * `payment_metadata` is now supported in `Invoice` deserialization, sending, and receiving (via a new `RecipientOnionFields` struct) (lightningdevkit#2139, lightningdevkit#2127). * `Event::PaymentFailed` now exposes a failure reason (lightningdevkit#2142). * BOLT12 messages now support stateless generation and validation (lightningdevkit#1989). * The `NetworkGraph` is now pruned of stale data after RGS processing (lightningdevkit#2161). * Max inbound HTLCs in-flight can be changed in the handshake config (lightningdevkit#2138). * `lightning-transaction-sync` feature `esplora-async-https` was added (lightningdevkit#2085). * A `ChannelPending` event is now emitted after the initial handshake (lightningdevkit#2098). * `PaymentForwarded::outbound_amount_forwarded_msat` was added (lightningdevkit#2136). * `ChannelManager::list_channels_by_counterparty` was added (lightningdevkit#2079). * `ChannelDetails::feerate_sat_per_1000_weight` was added (lightningdevkit#2094). * `Invoice::fallback_addresses` was added to fetch `bitcoin` types (lightningdevkit#2023). * The offer/refund description is now exposed in `Invoice{,Request}` (lightningdevkit#2206). Backwards Compatibility ======================= * Payments sent with the legacy `*_with_route` methods on LDK 0.0.115+ will no longer be retryable via the LDK 0.0.114- `retry_payment` method (lightningdevkit#2139). * `Event::PaymentPathFailed::retry` was removed and will always be `None` for payments initiated on 0.0.115 which fail on an earlier version (lightningdevkit#2063). * `Route`s and `PaymentParameters` with blinded path information will not be readable on prior versions of LDK. Such objects are not currently constructed by LDK, but may be when processing BOLT12 data in a coming release (lightningdevkit#2146). * Providing `ChannelMonitorUpdate`s generated by LDK 0.0.115 to a `ChannelMonitor` on 0.0.114 or before may panic (lightningdevkit#2059). Note that this is in general unsupported, and included here only for completeness. Bug Fixes ========= * Fixed a case where `process_events_async` may `poll` a `Future` which has already completed (lightningdevkit#2081). * Fixed deserialization of `u16` arrays. This bug may have previously corrupted the historical buckets in a `ProbabilisticScorer`. Users relying on the historical buckets may wish to wipe their scorer on upgrade to remove corrupt data rather than waiting on it to decay (lightningdevkit#2191). * The `process_events_async` task is now `Send` and can thus be polled on a multi-threaded runtime (lightningdevkit#2199). * Fixed a missing macro export causing `impl_writeable_tlv_based_enum{,_upgradable}` calls to not compile (lightningdevkit#2091). * Fixed compilation of `lightning-invoice` with both `no-std` and serde (lightningdevkit#2187) * Fix an issue where the `background-processor` would not wake when a `ChannelMonitorUpdate` completed asynchronously, causing delays (lightningdevkit#2090). * Fix an issue where `process_events_async` would exit immediately (lightningdevkit#2145). * `Router` calls from the `ChannelManager` now call `find_route_with_id` rather than `find_route`, as was intended and described in the API (lightningdevkit#2092). * Ensure `process_events_async` always exits if any sleep future returns true, not just if all sleep futures repeatedly return true (lightningdevkit#2145). * `channel_update` messages no longer set the disable bit unless the peer has been disconnected for some time. This should resolve cases where channels are disabled for extended periods of time (lightningdevkit#2198). * We no longer remove CLN nodes from the network graph for violating the BOLT spec in some cases after failing to pay through them (lightningdevkit#2220). * Fixed a debug assertion which may panic under heavy load (lightningdevkit#2172). * `CounterpartyForceClosed::peer_msg` is now wrapped in UntrustedString (lightningdevkit#2114) * Fixed a potential deadlock in `funding_transaction_generated` (lightningdevkit#2158). Security ======== * Transaction re-broadcasting is now substantially more aggressive, including a new regular rebroadcast feature called on a timer from the `background-processor` or from `ChainMonitor::rebroadcast_pending_claims`. This should substantially increase transaction confirmation reliability without relying on downstream `TransactionBroadcaster` implementations for rebroadcasting (lightningdevkit#2203, lightningdevkit#2205, lightningdevkit#2208). * Implemented the changes from BOLT PRs lightningdevkit#1031, lightningdevkit#1032, and lightningdevkit#1040 which resolve a privacy vulnerability which allows an intermediate node on the path to discover the final destination for a payment (lightningdevkit#2062). In total, this release features 110 files changed, 11928 insertions, 6368 deletions in 215 commits from 21 authors, in alphabetical order: * Advait * Alan Cohen * Alec Chen * Allan Douglas R. de Oliveira * Arik Sosman * Elias Rohrer * Evan Feenstra * Jeffrey Czyz * John Cantrell * Lucas Soriano del Pino * Marc Tyndel * Matt Corallo * Paul Miller * Steven * Steven Williamson * Steven Zhao * Tony Giorgio * Valentine Wallace * Wilmer Paulino * benthecarman * munjesi

jkczyz self-requested a review February 8, 2023 15:42

TheBlueMatt assigned TheBlueMatt and jkczyz Feb 8, 2023

jkczyz reviewed Feb 9, 2023

View reviewed changes

lightning-invoice/src/lib.rs Outdated Show resolved Hide resolved

lightning-invoice/src/utils.rs Outdated Show resolved Hide resolved

benthecarman reviewed Feb 12, 2023

View reviewed changes

lightning-invoice/src/lib.rs Outdated Show resolved Hide resolved

jkczyz reviewed Feb 13, 2023

View reviewed changes

TheBlueMatt reviewed Apr 4, 2023

View reviewed changes

lightning-invoice/src/de.rs Outdated Show resolved Hide resolved

lightning-invoice/src/lib.rs Outdated Show resolved Hide resolved

futurepaul force-pushed the fallback-to-address branch from 61db97d to faf1e23 Compare April 4, 2023 21:34

jkczyz previously approved these changes Apr 6, 2023

View reviewed changes

jkczyz reviewed Apr 6, 2023

View reviewed changes

lightning-invoice/src/de.rs Outdated Show resolved Hide resolved

lightning-invoice/src/lib.rs Outdated Show resolved Hide resolved

lightning-invoice/src/lib.rs Outdated Show resolved Hide resolved

Fallback: add Address getter and use bitcoin types

cf0a90b

futurepaul dismissed jkczyz’s stale review via cf0a90b April 6, 2023 15:49

futurepaul force-pushed the fallback-to-address branch from faf1e23 to cf0a90b Compare April 6, 2023 15:49

TheBlueMatt approved these changes Apr 7, 2023

View reviewed changes

jkczyz approved these changes Apr 7, 2023

View reviewed changes

jkczyz merged commit 1ceb41e into lightningdevkit:main Apr 7, 2023

optout21 mentioned this pull request Apr 18, 2024

Add splicing feature flag #3001

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `Fallback` getter that returns `Address` #2023

add `Fallback` getter that returns `Address` #2023

futurepaul commented Feb 8, 2023

jkczyz commented Feb 9, 2023

futurepaul commented Feb 9, 2023 •

edited

Loading

codecov-commenter commented Feb 10, 2023 •

edited

Loading

jkczyz left a comment

jkczyz Feb 13, 2023

futurepaul Apr 4, 2023

jkczyz Feb 13, 2023

futurepaul Apr 4, 2023

TheBlueMatt commented Mar 20, 2023

futurepaul commented Mar 22, 2023

futurepaul commented Apr 4, 2023

TheBlueMatt left a comment

TheBlueMatt commented Apr 4, 2023

futurepaul commented Apr 4, 2023

jkczyz commented Apr 6, 2023

jkczyz left a comment

add Fallback getter that returns Address #2023

add Fallback getter that returns Address #2023

Conversation

futurepaul commented Feb 8, 2023

jkczyz commented Feb 9, 2023

futurepaul commented Feb 9, 2023 • edited Loading

codecov-commenter commented Feb 10, 2023 • edited Loading

Codecov Report

jkczyz left a comment

Choose a reason for hiding this comment

jkczyz Feb 13, 2023

Choose a reason for hiding this comment

futurepaul Apr 4, 2023

Choose a reason for hiding this comment

jkczyz Feb 13, 2023

Choose a reason for hiding this comment

futurepaul Apr 4, 2023

Choose a reason for hiding this comment

TheBlueMatt commented Mar 20, 2023

futurepaul commented Mar 22, 2023

futurepaul commented Apr 4, 2023

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Apr 4, 2023

futurepaul commented Apr 4, 2023

jkczyz commented Apr 6, 2023

jkczyz left a comment

Choose a reason for hiding this comment

add `Fallback` getter that returns `Address` #2023

add `Fallback` getter that returns `Address` #2023

futurepaul commented Feb 9, 2023 •

edited

Loading

codecov-commenter commented Feb 10, 2023 •

edited

Loading