Skip to content

Correct docs and marginally expand OMNameResolver #3900

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

Merged
merged 4 commits into from
Jul 2, 2025

Conversation

TheBlueMatt
Copy link
Collaborator

See rust-bitcoin/bitcoin-payment-instructions#6 for the use of the new methods, but its all pretty straightforward.

We decided against implementing `OMNameResolver` using completion
callbacks, but the documentation on `new_best_block` was never
updated to match.
@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jun 29, 2025

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@tnull tnull requested review from tnull and removed request for joostjager June 30, 2025 07:56
Copy link
Contributor

@tnull tnull 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 unhappy, otherwise LGTM

/// (at least until [`Self::new_best_block`] is called).
///
/// If possible, you should prefer [`Self::new`] so that providing stale proofs is not
/// possible, however in no-std environments where there is some trust in the resolver used and
Copy link
Contributor

Choose a reason for hiding this comment

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

Given over how many hoops we're jumping in different places, I increasingly think we should lean on a standardized TimeProvider trait across LDK. This would allow us provide a very simple flow without introducing more assumptions/complications for 99% of users that can use the time feature. And anybody on WASM/no-std could plug-in their own time source.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would be nice if Rust WASM weren't a trainwreck...In this case, though we aren't just using it for time, but its really rather a timer-tick to give us CPU time to expire old offers. A TimeProvider trait could clean up some of the time logic internally, but it wouldn't materially change the public API.

@ldk-reviews-bot
Copy link

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

In `bitcoin-payment-instructions` we can figure out when a lookup
has timed out by looking at if the `Future` was dropped. However,
while we can clean up internal state in
`bitcoin-payment-instructions` we'd leave stale entries in
`OMNameResolver` (at least until they time out). Instead, here we
add a method to explicitly time out a lookup.
While all `OMNameResolver` users should at least use the block time
to avoid accepting stale DNSSEC proofs, in some cases its annoying
to pipe that data through and time may not be available (e.g. in a
no-std environment).

Here we enable such use by exposing an additional constructor and
disabling expiry validation when it is used.
@ldk-reviews-bot ldk-reviews-bot requested a review from tankyleo July 1, 2025 07:26
@ldk-reviews-bot
Copy link

✅ Added second reviewer: @tankyleo

tnull
tnull previously approved these changes Jul 1, 2025
In cases where we're built with the `std` feature, we can assume
that `SystemTime` works, so we should use it to validate that
DNSSEC proofs we receive are currently valid, even if we don't have
block time data.
@TheBlueMatt
Copy link
Collaborator Author

Updated docs and fn name only:

$ git diff-tree -U4 bb8af4760 18551845f
diff --git a/lightning/src/onion_message/dns_resolution.rs b/lightning/src/onion_message/dns_resolution.rs
index a33a883ee4..e927d645ec 100644
--- a/lightning/src/onion_message/dns_resolution.rs
+++ b/lightning/src/onion_message/dns_resolution.rs
@@ -329,18 +329,18 @@ impl OMNameResolver {
 		}
 	}

 	/// Builds a new [`OMNameResolver`] which will not validate the time limits on DNSSEC proofs
-	/// (at least until [`Self::new_best_block`] is called).
+	/// (for builds without the "std" feature and until [`Self::new_best_block`] is called).
 	///
 	/// If possible, you should prefer [`Self::new`] so that providing stale proofs is not
 	/// possible, however in no-std environments where there is some trust in the resolver used and
 	/// no time source is available, this may be acceptable.
 	///
 	/// Note that not calling [`Self::new_best_block`] will result in requests not timing out and
 	/// unresolved requests leaking memory. You must instead call
 	/// [`Self::expire_pending_resolution`] as unresolved requests expire.
-	pub fn new_without_expiry_validation() -> Self {
+	pub fn new_without_no_std_expiry_validation() -> Self {
 		Self {
 			pending_resolves: Mutex::new(new_hash_map()),
 			latest_block_time: AtomicUsize::new(0),
 			latest_block_height: AtomicUsize::new(0),

@tankyleo tankyleo requested a review from tnull July 1, 2025 21:52
@tnull tnull merged commit 257ebad into lightningdevkit:main Jul 2, 2025
27 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants