Skip to content

Improve logging of pathfinding behavior #2011

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

Closed
wants to merge 28 commits into from
Closed
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
9073b8d
first commit
Sharmalm Feb 4, 2023
02dddb4
second commit
Sharmalm Feb 4, 2023
0312c2b
original
Sharmalm Feb 5, 2023
3cdb8ed
first logging
Sharmalm Feb 5, 2023
b6ac4ec
Merge remote-tracking branch 'origin/main'
Sharmalm Feb 5, 2023
cfe611a
fifth commit
Sharmalm Feb 5, 2023
5bd7708
third commit
Sharmalm Feb 5, 2023
87eef3f
fourth commit
Sharmalm Feb 9, 2023
bb6c7f6
Merge branch 'lightningdevkit:main' into main
Sharmalm Feb 9, 2023
aef2aa6
Merge branch 'lightningdevkit:main' into main
Sharmalm Feb 11, 2023
4a2c289
sixth commit
Sharmalm Feb 11, 2023
a8781ca
seventh commit
Sharmalm Feb 11, 2023
0e5e8a6
eight commit
Sharmalm Feb 14, 2023
3240053
Merge branch 'lightningdevkit:main' into main
Sharmalm Feb 14, 2023
b913098
Merge branch 'lightningdevkit:main' into main
Sharmalm Feb 15, 2023
8d2a85d
ninth
Sharmalm Feb 15, 2023
fe93ed8
Merge branch 'lightningdevkit:main' into main
Sharmalm Feb 20, 2023
6da0e06
access hashmap
Sharmalm Feb 20, 2023
78dbb77
Merge remote-tracking branch 'origin/main'
Sharmalm Feb 20, 2023
0be98f5
Merge branch 'lightningdevkit:main' into main
Sharmalm Feb 24, 2023
c830123
Merge branch 'lightningdevkit:main' into main
Sharmalm Mar 1, 2023
9bc34a1
improving hashmap conditions
Sharmalm Mar 1, 2023
1969bb7
Merge remote-tracking branch 'refs/remotes/origin/main'
Sharmalm Mar 1, 2023
7febd5b
Merge branch 'lightningdevkit:main' into main
Sharmalm Mar 4, 2023
4364e18
correction
Sharmalm Mar 4, 2023
34d21dc
Merge branch 'lightningdevkit:main' into main
Sharmalm Mar 16, 2023
592f615
test function
Sharmalm Mar 16, 2023
c633d31
Merge remote-tracking branch 'refs/remotes/origin/main'
Sharmalm Mar 16, 2023
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
80 changes: 79 additions & 1 deletion lightning/src/routing/router.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1296,6 +1296,25 @@ where L::Target: Logger {
// around again with a higher amount.
if !contributes_sufficient_value || exceeds_max_path_length ||
exceeds_cltv_delta_limit || payment_failed_on_this_channel {

let mut is_first_hop = true;
for (key , _channel) in &first_hop_targets {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need to loop here, you can just do first_hop_targets.get(&$dest_node_id) with an if let Some... or match. That should also let you drop the is_first_hop variable.

if key == &$dest_node_id {
if is_first_hop && !contributes_sufficient_value {
log_trace!(logger, "First Hop {} is excluded due to insufficient value", short_channel_id);

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can drop the blank lines here, and note that some lines have whitespace at the end, which would be nice to remove. Finally, note that some lines are indented by spaces followed by tabs, which should be replaced by all tabs. All of these issues will be highlighted by doing git show or git diff-tree -U3 locally, depending on your terminal config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks

} if is_first_hop && exceeds_max_path_length {
log_trace!(logger, "First Hop {} is excluded due to candidate hop excluded max path length", short_channel_id);

} if is_first_hop && exceeds_cltv_delta_limit {
log_trace!(logger, "First Hop {} is excluded beacause it exceed the maximum total cltv expiry limit" , short_channel_id);

} if is_first_hop && payment_failed_on_this_channel {
log_trace!(logger, "First Hop {} is excluded beacause it was failed previously" , short_channel_id);
}
is_first_hop = false;
} // commit
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does // commit mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, it means nothing. I write // commit in the line where I update the code.

}
// Path isn't useful, ignore it and move on.
} else if may_overpay_to_meet_path_minimum_msat {
hit_minimum_limit = true;
Expand Down Expand Up @@ -3813,7 +3832,66 @@ mod tests {
assert_eq!(total_amount_paid_msat, 50_000);
}
}


#[test]
fn test_my_logging_function() { // commit
let (secp_ctx, network_graph, gossip_sync, _, logger) = build_graph();
let (our_privkey, our_id, privkeys, nodes) = get_nodes(&secp_ctx);
let scorer = ln_test_utils::TestScorer::with_penalty(0);
let keys_manager = ln_test_utils::TestKeysInterface::new(&[0u8; 32], Network::Testnet);
let random_seed_bytes = keys_manager.get_secure_random_bytes();
let payment_params = PaymentParameters::from_node_id(nodes[2], 42);
let logger = crate::util::test_utils::TestLogger::new();

update_channel(&gossip_sync, &secp_ctx, &our_privkey, UnsignedChannelUpdate {
chain_hash: genesis_block(Network::Testnet).header.block_hash(),
short_channel_id: 1,
timestamp: 2,
flags: 0,
cltv_expiry_delta: 0,
htlc_minimum_msat: 0,
htlc_maximum_msat: 100_000,
fee_base_msat: 1_000_000,
fee_proportional_millionths: 0,
excess_data: Vec::new()
});
update_channel(&gossip_sync, &secp_ctx, &privkeys[0], UnsignedChannelUpdate {
chain_hash: genesis_block(Network::Testnet).header.block_hash(),
short_channel_id: 3,
timestamp: 2,
flags: 0,
cltv_expiry_delta: 0,
htlc_minimum_msat: 0,
htlc_maximum_msat: 50_000,
fee_base_msat: 0,
fee_proportional_millionths: 0,
excess_data: Vec::new()
});

// we can make four test for different condition or all condition in diffrent tests.
let is_first_hop = true;
let contributes_sufficient_value = false ; // for contributes_suffiecient value , understand where this value came from. what is contribute_sufficent value.

let exceeds_max_path_length = false;
let exceeds_cltv_delta_limit = false;
let payment_failed_on_this_channel = false;

// For this test to run I want short_channel_id , dest_node_id .
let route = get_route(&our_id, &payment_params, &network_graph.read_only(), None, 49_000, 42, &logger, &scorer, &random_seed_bytes).unwrap();

// Retrieve the log message from TestLogger
let lines = logger.lines.lock().unwrap();
let mut log_output = String::new();
for ((module, file), line) in &*lines {
Copy link
Collaborator

@TheBlueMatt TheBlueMatt Mar 20, 2023

Choose a reason for hiding this comment

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

You don't have to iterate over the lines here, you can just do logger.assert_log_contains("routing:;router", "First Hop 3 is excluded due to insufficient value", 1);

log_output.push_str(&format!("{} [{}] {}: {}\n", line, module, line, file));
}
// Check for expected messages in the log output
assert!(log_output.contains("First Hop 3 is excluded due to insufficient value")); //
// assert_log_contains(log_ouptuts , 1);
}



#[test]
fn simple_mpp_route_test() {
let (secp_ctx, network_graph, gossip_sync, _, logger) = build_graph();
Expand Down