Skip to content
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

Check chain hash for channel announcement and update #2230

Merged

Conversation

freddiekrugerrand
Copy link
Contributor

Fixes #2228

Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

LGTM with one small nit.

@@ -2311,6 +2325,15 @@ pub(crate) mod tests {
Ok(_) => panic!(),
Err(e) => assert_eq!(e.err, "Channel announcement node had a channel with itself")
};

// Test that channel announcements with the wrong chain hash are ignored.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Could we just include why it's the wrong chain hash here? I got a little confused with it in isolation but remembered we were dealing with testnet in these gossip tests.

"testnet vs mainnet" or something similar.

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 77.77% and project coverage change: -0.01 ⚠️

Comparison is base (c182567) 91.56% compared to head (0a43123) 91.56%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2230      +/-   ##
==========================================
- Coverage   91.56%   91.56%   -0.01%     
==========================================
  Files         104      104              
  Lines       51749    51767      +18     
  Branches    51749    51767      +18     
==========================================
+ Hits        47384    47398      +14     
- Misses       4365     4369       +4     
Impacted Files Coverage Δ
lightning/src/routing/gossip.rs 89.77% <77.77%> (-0.15%) ⬇️

... and 5 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@benthecarman
Copy link
Contributor

This could also be done for rapid gossip sync updates

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.

Thanks for taking this on!

lightning/src/routing/gossip.rs Outdated Show resolved Hide resolved
@tnull
Copy link
Contributor

tnull commented Apr 26, 2023

This could also be done for rapid gossip sync updates

Catching unmatching RGS updates are the primary intention, but I think they are already covered by checking in update_channel_intern?

@freddiekrugerrand
Copy link
Contributor Author

This could also be done for rapid gossip sync updates

Took a look at processing.rs to make sure RGS is fully covered:

  • update_channel_unsigned: calls update_channel_intern which has check
  • add_channel_from_partial_announcement: no chain_hash provided

Wondering whether we should add a top level check here in RGS so that we can exit early if we have the wrong hash? Would need to add get_genesis_hash (or check_genesis_hash) to NetworkGraph.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Nice!

In a followup, we should also (finally) add support for the init genesis block hash field by fetching the genesis block hash from the ChannelMessageHandler (ie ChannelManager) in PeerManager, then checking that value against what we receive in init message(s) and send it in ours.

@TheBlueMatt
Copy link
Collaborator

Wondering whether we should add a top level check here in RGS so that we can exit early if we have the wrong hash? Would need to add get_genesis_hash (or check_genesis_hash) to NetworkGraph.

Yes, yes we should.

@TheBlueMatt TheBlueMatt merged commit 88c63e9 into lightningdevkit:main Apr 27, 2023
14 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.

Fail to apply gossip update for invalid chain hash
6 participants