Skip to content

Misc (mostly-)feature cleanups #3250

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

Merged
merged 5 commits into from
Aug 21, 2024
Merged

Misc (mostly-)feature cleanups #3250

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1c83e61
Remove note about `std`/`no-std` scorer serialization compat
TheBlueMatt Aug 16, 2024
d89a487
Simplify `Instant` mocking in outbound payments
TheBlueMatt Aug 16, 2024
75a2089
Drop `std::Error` impl for `PeerHandleError`
TheBlueMatt Aug 17, 2024
5412784
Reduce `feature = std` flags in `gossip.rs` somewhat
TheBlueMatt Aug 17, 2024
11ab302
Remove uneccessary `std` bounds on many tests
TheBlueMatt Aug 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 12 additions & 16 deletions lightning/src/routing/gossip.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,6 @@ where U::Target: UtxoLookup, L::Target: Logger
{
network_graph: G,
utxo_lookup: RwLock<Option<U>>,
#[cfg(feature = "std")]
full_syncs_requested: AtomicUsize,
pending_events: Mutex<Vec<MessageSendEvent>>,
logger: L,
Expand All @@ -325,7 +324,6 @@ where U::Target: UtxoLookup, L::Target: Logger
pub fn new(network_graph: G, utxo_lookup: Option<U>, logger: L) -> Self {
P2PGossipSync {
network_graph,
#[cfg(feature = "std")]
full_syncs_requested: AtomicUsize::new(0),
utxo_lookup: RwLock::new(utxo_lookup),
pending_events: Mutex::new(vec![]),
Expand All @@ -348,10 +346,8 @@ where U::Target: UtxoLookup, L::Target: Logger
&self.network_graph
}

#[cfg(feature = "std")]
/// Returns true when a full routing table sync should be performed with a peer.
fn should_request_full_sync(&self, _node_id: &PublicKey) -> bool {
//TODO: Determine whether to request a full sync based on the network map.
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to remove this todo?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, I don't think we will/can ever make such a decision - instead we need the spec to include a set reconciliation protocol.

fn should_request_full_sync(&self) -> bool {
const FULL_SYNCS_TO_REQUEST: usize = 5;
if self.full_syncs_requested.load(Ordering::Acquire) < FULL_SYNCS_TO_REQUEST {
self.full_syncs_requested.fetch_add(1, Ordering::AcqRel);
Expand Down Expand Up @@ -619,10 +615,12 @@ where U::Target: UtxoLookup, L::Target: Logger
// For no-std builds, we bury our head in the sand and do a full sync on each connection.
#[allow(unused_mut, unused_assignments)]
let mut gossip_start_time = 0;
#[allow(unused)]
let should_sync = self.should_request_full_sync();
Comment on lines +618 to +619
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I don't see why this got moved and allow(unused)'d?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed the std bounds on should_request_full_sync, and in order to make it not-unused, I moved this out of the std block below. But that also means that in no-std builds this value is unused. In theory we should keep the bounds as they were, but I liked the diff stats 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, I had a similar issue once. Hate this aspect of conditional compilation

#[cfg(feature = "std")]
{
gossip_start_time = SystemTime::now().duration_since(UNIX_EPOCH).expect("Time must be > 1970").as_secs();
if self.should_request_full_sync(&their_node_id) {
if should_sync {
gossip_start_time -= 60 * 60 * 24 * 7 * 2; // 2 weeks ago
} else {
gossip_start_time -= 60 * 60; // an hour ago
Expand Down Expand Up @@ -2452,18 +2450,16 @@ pub(crate) mod tests {
}

#[test]
#[cfg(feature = "std")]
fn request_full_sync_finite_times() {
let network_graph = create_network_graph();
let (secp_ctx, gossip_sync) = create_gossip_sync(&network_graph);
let node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&<Vec<u8>>::from_hex("0202020202020202020202020202020202020202020202020202020202020202").unwrap()[..]).unwrap());

assert!(gossip_sync.should_request_full_sync(&node_id));
assert!(gossip_sync.should_request_full_sync(&node_id));
assert!(gossip_sync.should_request_full_sync(&node_id));
assert!(gossip_sync.should_request_full_sync(&node_id));
assert!(gossip_sync.should_request_full_sync(&node_id));
assert!(!gossip_sync.should_request_full_sync(&node_id));
let (_, gossip_sync) = create_gossip_sync(&network_graph);

assert!(gossip_sync.should_request_full_sync());
assert!(gossip_sync.should_request_full_sync());
assert!(gossip_sync.should_request_full_sync());
assert!(gossip_sync.should_request_full_sync());
assert!(gossip_sync.should_request_full_sync());
assert!(!gossip_sync.should_request_full_sync());
}

pub(crate) fn get_signed_node_announcement<F: Fn(&mut UnsignedNodeAnnouncement)>(f: F, node_key: &SecretKey, secp_ctx: &Secp256k1<secp256k1::All>) -> NodeAnnouncement {
Expand Down