-
Notifications
You must be signed in to change notification settings - Fork 418
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
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! |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
👋 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.
84fdc85
to
072675b
Compare
✅ Added second reviewer: @tankyleo |
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.
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.