-
Notifications
You must be signed in to change notification settings - Fork 412
fuzz: Add LSPS message decoder fuzzing #3849
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fuzz: Add LSPS message decoder fuzzing #3849
Conversation
👋 Thanks for assigning @TheBlueMatt 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.
One comment, otherwise lgtm
fuzz/src/lsps_message.rs
Outdated
let msg_router = | ||
Arc::new(DefaultMessageRouter::new(network_graph.clone(), Arc::clone(&keys_manager))); | ||
let chain_source = Arc::new(TestChainSource::new(Network::Bitcoin)); | ||
let kv_store = Arc::new(FilesystemStore::new("persister".into())); |
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.
Can we use a test persister rather than something that could write to the fs in a fuzz target?
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.
I guess we should just be able to use the in-memory lightning::util::test_utils::TestStore
for this?
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.
LGTM, mod one nit and the pending question regarding the KVStore
.
Great start for lightning-liquidity
fuzzing!
fuzz/src/lsps_message.rs
Outdated
let seed = sha256::Hash::hash(b"lsps-message-seed").to_byte_array(); | ||
let keys_manager = Arc::new(KeysManager::new(&seed, now.as_secs(), now.subsec_nanos())); | ||
let router = Arc::new(DefaultRouter::new( | ||
network_graph.clone(), |
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.
nit: Given we're about to enable a lint enforcing to use Arc::clone
vs. arc.clone()
in #3851, let's switch all of these to Arc::clone
pls.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3849 +/- ##
==========================================
+ Coverage 89.89% 89.90% +0.01%
==========================================
Files 164 164
Lines 132022 132022
Branches 132022 132022
==========================================
+ Hits 118681 118695 +14
+ Misses 10649 10640 -9
+ Partials 2692 2687 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
LGTM, feel free to squash, I think.
d8176d1
to
039af53
Compare
thanks for the review! squashed and pushed |
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.
LGTM
Fixes #3515
The fuzz test tests the
LiquidityManager
's ability to parse and handle arbitrary LSPS message data safely.