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.

Sign up for GitHub

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

Jump to bottom

Support invoice expiry over a year #1273

Merged
merged 1 commit into from
Jan 26, 2022
Merged

Support invoice expiry over a year #1273

merged 1 commit into from
Jan 26, 2022

Conversation

jkczyz
Copy link
Contributor

@jkczyz jkczyz commented Jan 21, 2022

The lightning-invoice crate represents timestamps as Duration since the UNIX epoch rather than a SystemTime. Therefore, internal calculations are in terms of u64-based Durations. This allows for relaxing the one year maximum expiry.

Fixes #1235

@codecov-commenter
Copy link

codecov-commenter commented Jan 22, 2022 •
edited
Loading

Codecov Report

Merging #1273 (6002032) into main (d741fb1) will increase coverage by 0.31%.
The diff coverage is 90.90%.

❗ Current head 6002032 differs from pull request most recent head cdf670c. Consider uploading reports for the commit cdf670c to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1273      +/-   ##
==========================================
+ Coverage   90.40%   90.72%   +0.31%     
==========================================
  Files          70       71       +1     
  Lines       38118    39381    +1263     
==========================================
+ Hits        34462    35728    +1266     
+ Misses       3656     3653       -3
Impacted Files Coverage Δ
lightning-invoice/src/lib.rs 89.58% <90.00%> (+2.08%) ⬆️
lightning-invoice/src/de.rs 81.06% <100.00%> (+0.33%) ⬆️
lightning/src/ln/functional_tests.rs 97.28% <0.00%> (-0.09%) ⬇️
lightning/src/util/invoice.rs 91.66% <0.00%> (ø)
lightning-background-processor/src/lib.rs 93.29% <0.00%> (+0.31%) ⬆️
lightning/src/ln/channelmanager.rs 84.98% <0.00%> (+0.72%) ⬆️
lightning/src/ln/channel.rs 90.23% <0.00%> (+0.88%) ⬆️
lightning/src/ln/functional_test_utils.rs 96.92% <0.00%> (+1.63%) ⬆️
lightning-invoice/src/utils.rs 86.15% <0.00%> (+1.94%) ⬆️
... and 2 more

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

@TheBlueMatt TheBlueMatt added this to the 0.0.105 milestone Jan 22, 2022
TheBlueMatt
TheBlueMatt reviewed Jan 22, 2022
View reviewed changes
lightning-invoice/src/lib.rs Outdated
/// Allow the expiry time to be up to one year. Since this reduces the range of possible timestamps
/// it should be rather low as long as we still have to support 32bit time representations
const MAX_EXPIRY_TIME: u64 = 60 * 60 * 24 * 356;
/// The maximum expiry allowed, represented as a [`Duration`] since the invoice timestamp.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why have a max expiry at all? Can we just update the expiry-check logic to do a saturating add?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea!

@jkczyz jkczyz force-pushed the 2022-01-invoice-expiry branch 2 times, most recently from 62235d4 to 5f61a12 Compare January 24, 2022 06:00
@TheBlueMatt
Copy link
Collaborator

I assume the serialization of PositiveTimestamp needs updating?

impl ToBase32 for PositiveTimestamp {
	fn write_base32<W: WriteBase32>(&self, writer: &mut W) -> Result<(), <W as WriteBase32>::Err> {
		// FIXME: use writer for int encoding
		writer.write(
			&try_stretch(encode_int_be_base32(self.as_unix_timestamp()), 7)
				.expect("Can't be longer due than 7 u5s due to timestamp bounds")
		)
	}
}

@jkczyz jkczyz force-pushed the 2022-01-invoice-expiry branch 2 times, most recently from da6b69e to 6002032 Compare January 24, 2022 22:58
devrandom
devrandom reviewed Jan 25, 2022
View reviewed changes
@jkczyz
Copy link
Contributor Author

jkczyz commented Jan 25, 2022

I assume the serialization of PositiveTimestamp needs updating?

impl ToBase32 for PositiveTimestamp {
	fn write_base32<W: WriteBase32>(&self, writer: &mut W) -> Result<(), <W as WriteBase32>::Err> {
		// FIXME: use writer for int encoding
		writer.write(
			&try_stretch(encode_int_be_base32(self.as_unix_timestamp()), 7)
				.expect("Can't be longer due than 7 u5s due to timestamp bounds")
		)
	}
}

Discussed offline. BOLT 11 defines timestamps as 35 bits, so updated the PR yesterday to enforce this instead.

TheBlueMatt
TheBlueMatt reviewed Jan 25, 2022
View reviewed changes
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM, feel free to squash.

lightning-invoice/src/lib.rs Outdated
Ok(t) => self.tagged_fields.push(TaggedField::ExpiryTime(t)),
Err(e) => self.error = Some(e),
};
self.tagged_fields.push(TaggedField::ExpiryTime(ExpiryTime::from_duration(expiry_time)));
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: when you squash, can you make this a tab not space (it was already spaces before).

lightning-invoice/src/lib.rs Outdated
/// Create a new `PositiveTimestamp` from a unix timestamp in the Range
/// `0...SYSTEM_TIME_MAX_UNIX_TIMESTAMP - MAX_EXPIRY_TIME`, otherwise return a
/// `CreationError::TimestampOutOfBounds`.
/// Creates a `PositiveTimestamp` from a unix timestamp in the range `0...MAX_TIMESTAMP`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you're gonna reference MAX_TIMESTAMP in the docs it should be pub.

@jkczyz
Support invoice expiry over a year
3b14a76 
The lightning-invoice crate represents timestamps as Duration since the
UNIX epoch rather than a SystemTime. Therefore, internal calculations
are in terms of u64-based Durations. This allows for relaxing the one
year maximum expiry.
@jkczyz jkczyz force-pushed the 2022-01-invoice-expiry branch from cdf670c to 3b14a76 Compare January 25, 2022 21:34
TheBlueMatt
TheBlueMatt approved these changes Jan 25, 2022
View reviewed changes
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

ACK 3b14a76

@devrandom
Copy link
Member

ACK

@valentinewallace valentinewallace merged commit f49662c into lightningdevkit:main Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devrandom devrandom devrandom approved these changes

@TheBlueMatt TheBlueMatt TheBlueMatt approved these changes

@valentinewallace valentinewallace valentinewallace approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.0.105
Development

Successfully merging this pull request may close these issues.

Drop one-year limit on invoice expiry
5 participants
@jkczyz @codecov-commenter @TheBlueMatt @devrandom @valentinewallace