Correct docs and marginally expand OMNameResolver
#3900
Conversation
We decided against implementing `OMNameResolver` using completion callbacks, but the documentation on `new_best_block` was never updated to match.
|
👋 Thanks for assigning @tnull as a reviewer! |
| /// (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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
👋 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.
TheBlueMatt
force-pushed
the
2025-06-bad-docs
branch
from
84fdc85 to
072675b
Compare
|
✅ Added second reviewer: @tankyleo |
TheBlueMatt
force-pushed
the
2025-06-bad-docs
branch
from
d4ff34e to
bb8af47
Compare
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
force-pushed
the
2025-06-bad-docs
branch
from
bb8af47 to
1855184
Compare
|
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), |
See rust-bitcoin/bitcoin-payment-instructions#6 for the use of the new methods, but its all pretty straightforward.