Update full_stack_target to take an arbitrary config object
#2810
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2810 +/- ##
==========================================
- Coverage 89.18% 89.17% -0.01%
==========================================
Files 116 116
Lines 93252 93268 +16
Branches 93252 93268 +16
==========================================
+ Hits 83166 83171 +5
- Misses 7560 7570 +10
- Partials 2526 2527 +1 ☔ View full report in Codecov by Sentry. |
TheBlueMatt
force-pushed
the
2023-12-arbitrary-fuzz-config
branch
2 times, most recently
from
727887f
to
f693ff3
Compare
TheBlueMatt
force-pushed
the
2023-12-arbitrary-fuzz-config
branch
from
f693ff3
to
9a01841
Compare
|
Rebased. |
WalkthroughThe changes across the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 9
Configuration used: CodeRabbit UI
Files ignored due to filter (3)
- fuzz/Cargo.toml
- lightning-invoice/Cargo.toml
- lightning/Cargo.toml
Files selected for processing (25)
- ci/check-cfg-flags.py (1 hunks)
- fuzz/src/full_stack.rs (15 hunks)
- lightning/src/chain/chainmonitor.rs (1 hunks)
- lightning/src/chain/channelmonitor.rs (13 hunks)
- lightning/src/chain/onchaintx.rs (6 hunks)
- lightning/src/events/bump_transaction.rs (1 hunks)
- lightning/src/lib.rs (1 hunks)
- lightning/src/ln/channel.rs (3 hunks)
- lightning/src/ln/channelmanager.rs (22 hunks)
- lightning/src/ln/functional_test_utils.rs (5 hunks)
- lightning/src/ln/functional_tests.rs (4 hunks)
- lightning/src/ln/monitor_tests.rs (1 hunks)
- lightning/src/ln/outbound_payment.rs (5 hunks)
- lightning/src/ln/peer_handler.rs (2 hunks)
- lightning/src/onion_message/messenger.rs (2 hunks)
- lightning/src/routing/gossip.rs (2 hunks)
- lightning/src/routing/router.rs (8 hunks)
- lightning/src/routing/scoring.rs (7 hunks)
- lightning/src/routing/utxo.rs (1 hunks)
- lightning/src/sign/mod.rs (1 hunks)
- lightning/src/sync/debug_sync.rs (4 hunks)
- lightning/src/util/config.rs (4 hunks)
- lightning/src/util/indexed_map.rs (4 hunks)
- lightning/src/util/ser.rs (2 hunks)
- lightning/src/util/test_utils.rs (12 hunks)
Files not summarized due to errors (1)
- fuzz/src/full_stack.rs: Error: Message exceeds token limit
Files skipped from review due to trivial changes (3)
- lightning/src/chain/channelmonitor.rs
- lightning/src/ln/channel.rs
- lightning/src/util/test_utils.rs
Additional comments: 83
ci/check-cfg-flags.py (1)
- 16-17: The addition of the "ahash" feature condition in the
check_featurefunction is consistent with the existing structure and is correctly implemented.lightning/src/util/indexed_map.rs (2)
- 37-37: The replacement of
HashMap::new()withnew_hash_map()andHashMap::with_capacity(capacity)withhash_map_with_capacity(capacity)is consistent and follows the DRY principle by centralizing the creation of hash maps.Also applies to: 45-45
- 179-179: The replacement of
hash_map::VacantEntryandhash_map::OccupiedEntrywithVacantHashMapEntryandOccupiedHashMapEntryis consistent with the new import changes and maintains the clarity of the code.Also applies to: 188-188
lightning/src/lib.rs (2)
168-234: The conditional compilation logic for including the
ahashcrate and using different hash map and hash set implementations based on feature flags is complex. Ensure that the logic is thoroughly tested to prevent any issues due to incorrect feature flag combinations.222-234: The introduction of functions for creating hash maps and hash sets with specific capacities is a good practice, as it encapsulates the creation logic and allows for easy adjustments in the future.
lightning/src/sync/debug_sync.rs (1)
- 60-60: The use of
new_hash_map()for initializingHashMapinstances forLOCKS_HELD,locked_before, andLOCKSis consistent with the changes in other files and follows the DRY principle.Also applies to: 116-116, 125-125
lightning/src/routing/utxo.rs (1)
- 310-310: The change to use
new_hash_mapfor initializingchannelsandnodesin thePendingChecksstruct is consistent with the PR's objective to standardize the creation of these data structures. This should improve the maintainability of the code by using a single function to create hash maps, which could be beneficial if the initialization logic needs to be updated or extended in the future.lightning/src/events/bump_transaction.rs (2)
394-394: The change to use
new_hash_map()instead ofHashMap::new()is consistent with the PR's objective to standardize the creation of hash maps across the codebase. This change should be safe as long asnew_hash_map()is correctly implemented to return an emptyHashMap. However, ensure thatnew_hash_map()is indeed a drop-in replacement forHashMap::new()in terms of performance and behavior.392-396: The introduction of
new_hash_map()is used here to initializelocked_utxos. It's important to verify that thenew_hash_map()function has been defined and is accessible within this scope. Additionally, ensure that thenew_hash_map()function does not introduce any unexpected side effects that could affect the locking mechanism of UTXOs.Verification successful
The
new_hash_mapfunction is defined inlightning/src/lib.rsand is accessible within the scope ofbump_transaction.rssince it is marked aspub(crate), meaning it is public within the crate. The implementation ofnew_hash_mapis a simple wrapper aroundHashMap::new(), which should not introduce any unexpected side effects.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence and implementation of new_hash_map function. rg "fn new_hash_map" --type rustLength of output: 205
lightning/src/util/config.rs (4)
16-17: The addition of the
Readabletrait for fuzzing is a good practice as it allows fuzzers to generate a wide range of inputs to test the system's robustness. However, ensure that this trait is only available when thefuzzingfeature is enabled to prevent its use in production code.220-235: The implementation of
ReadableforChannelHandshakeConfiglooks correct and follows the pattern of reading each field in order. It's important to ensure that the types of each field implementReadablethemselves, which is a safe assumption given the standard types used here.346-361: Similarly, the implementation of
ReadableforChannelHandshakeLimitsis consistent with the previous implementation forChannelHandshakeConfig. It's good to see consistency in the approach to these implementations.834-847: The
Readableimplementation forUserConfigis also consistent with the otherReadableimplementations. It's crucial to ensure that theReadabletrait is not exposed in production as it can lead to security issues if external entities can influence configuration.lightning/src/onion_message/messenger.rs (2)
708-708: The use of
new_hash_map()is consistent with the PR's objective to standardize the creation of hash maps. This change is good as it abstracts the creation of aHashMapand can be easily modified in the future if needed.837-837: Again, the use of
new_hash_map()here is consistent with the PR's objective. It's good to see that the abstraction is being used in multiple places, which will make future changes to the hash map creation process easier.lightning/src/util/ser.rs (3)
- 752-752: The change to use
hash_map_with_capacity(len)in theimpl_for_map!macro forHashMapis noted. Ensure that thehash_map_with_capacityfunction is defined and correctly handles the capacity allocation.Verification successful
The output from the
rgcommand confirms that the functionhash_map_with_capacityis indeed defined within thelightning/src/lib.rsfile. This definition is consistent with the change noted in the review comment, where theimpl_for_map!macro forHashMapnow useshash_map_with_capacity(len).Given this information, the review comment is consistent with the codebase, as the function required by the macro is present and presumably handles the capacity allocation as intended.
* 774-774: The initialization of `HashSet` with `hash_set_with_capacity` using `cmp::min(len.0 as usize, MAX_BUF_SIZE / core::mem::size_of::())` is a good practice to avoid re-allocations. However, ensure that `MAX_BUF_SIZE` and `core::mem::size_of::()` are used correctly to prevent integer overflows and that `MAX_BUF_SIZE` is defined and appropriate for this context.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the function `hash_map_with_capacity` is defined and properly handles capacity. rg 'fn hash_map_with_capacity'Length of output: 226
Verification successful
Based on the script output,
MAX_BUF_SIZEis defined in theser.rsfile, and the usage ofcore::mem::size_of::<T>()is consistent with the context of settingHashSetcapacity. The use ofcmp::minin the capacity calculation is a safeguard against potential integer overflows by ensuring the capacity does not exceedMAX_BUF_SIZE. The review comment has been verified and the code appears to be using these constants correctly.* 774-774: The error handling for duplicate insertions in the `HashSet` is correct, returning `DecodeError::InvalidValue` if an insert fails. This ensures that the deserialization process fails fast on invalid data.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that MAX_BUF_SIZE is defined and check for potential integer overflow issues. rg 'MAX_BUF_SIZE' # Check for the correct usage of core::mem::size_of::<T>() in the context of HashSet capacity. ast-grep --lang rust --pattern $'core::mem::size_of::<$_>()'Length of output: 1338
lightning/src/chain/onchaintx.rs (6)
377-383: The use of
hash_map_with_capacityis introduced to initializepending_claim_requestsandclaimable_outpoints. This change seems to be part of a refactoring effort to standardize the creation of hash maps. Ensure that thehash_map_with_capacityfunction is correctly defined elsewhere and that it behaves as expected, especially with regards to memory allocation limits defined byMAX_ALLOC_SIZE.448-449: The initialization of
pending_claim_requestsandclaimable_outpointswithin theOnchainTxHandlerstruct has been updated to usenew_hash_map. This change is consistent with the refactoring pattern observed in the previous hunk. It's important to verify thatnew_hash_mapis properly defined and that all instances ofHashMap::newhave been replaced where intended.689-689: The removal of the reference operator
&beforeoutpointin the removal of claimable outpoints is a subtle but important change. This change should be correct as long as theremovemethod expects a borrowed reference, which is the usual case withHashMap. However, it's worth double-checking to ensure that this change does not introduce any issues with the types expected by theremovemethod.811-811: The introduction of a
debug_assert!with a conditional compilation check for fuzzing is a good practice to avoid panics during fuzz testing. However, it's important to ensure that this does not mask potential issues during normal execution. The use ofcfg!(fuzzing)should be verified to ensure it's defined correctly in the build configuration.Verification successful
The search results indicate that the
fuzzingconfiguration is indeed used throughout the codebase, particularly within conditional compilation attributes. The flag is used to differentiate behavior during fuzz testing from normal execution, as seen in various files includinglib.rs,config.rs,mod.rs, and others.The presence of the
fuzzingflag in thelib.rsfile suggests that it is a recognized compilation flag within the project. It is used in conjunction withcfg_attrandcfgto conditionally compile code for fuzzing. This confirms that thefuzzingflag is defined and used correctly in the build configuration.Given this information, the review comment about the introduction of a
debug_assert!with a conditional compilation check for fuzzing is consistent with the codebase's use of thefuzzingflag. The flag is used to ensure that certain assertions are only enabled during normal execution and not during fuzz testing, which is a common practice to avoid false positives when fuzzing.Based on the evidence from the search results, the use of
cfg!(fuzzing)in thedebug_assert!is verified to be defined correctly in the build configuration, and the review comment is consistent with the practices observed in the codebase.* 837-837: The initialization of `bump_candidates` using `new_hash_map` in the `update_claims_view` function is consistent with the refactoring pattern. As with previous instances, ensure that `new_hash_map` is defined and used correctly throughout the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the build configuration for the fuzzing flag rg "cfg!(fuzzing)" --type rustLength of output: 32
Script:
#!/bin/bash # Search for the definition of the 'fuzzing' feature flag in Cargo.toml files rg 'fuzzing' --glob 'Cargo.toml' # Search for the usage of the 'fuzzing' flag without the cfg! macro in case it's used differently rg 'fuzzing' --type rustLength of output: 17504
- 1023-1023: Similarly, the initialization of
bump_candidatesin thehandle_onchain_eventsfunction has been updated to usenew_hash_map. This change should be reviewed in the context of the entire function to ensure that it aligns with the intended behavior and thatnew_hash_mapis the appropriate choice here.lightning/src/chain/chainmonitor.rs (1)
- 422-422: The initialization of the
monitorsfield has been refactored to usenew_hash_map()instead ofHashMap::new(). This is a good practice for standardizing the creation of hash maps across the codebase. Ensure that thenew_hash_map()function is implemented correctly and performs as expected.Verification successful
The change to initialize the
monitorsfield usingnew_hash_map()has been verified and is consistent with the codebase. Thenew_hash_map()function is correctly implemented as a wrapper forHashMap::new()inlightning/src/lib.rs.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that new_hash_map is defined and check its implementation. rg "fn new_hash_map"Length of output: 193
fuzz/src/full_stack.rs (3)
52-56: The imports are well-organized and follow Rust's convention of grouping imports by the crate. This is good for readability and maintainability.
442-451: The function
do_testnow includes logic to parse aUserConfigfrom the input data. It's important to ensure that the input data is of sufficient length and properly formatted to avoid panics due to slicing errors or failed parsing. The current guard clause for data length at the beginning of the function is a good practice.1050-1050: The test code here is using a hardcoded hex string to simulate input data. This is a common practice in fuzz testing to provide a deterministic input for testing purposes. However, it's important to ensure that this input data is representative of real-world scenarios to make the fuzz testing effective.
lightning/src/sign/mod.rs (1)
- 353-353: The initialization of
output_sethas been changed to usehash_set_with_capacity. Ensure that this custom function is implemented correctly and provides the same or improved functionality over the standardHashSet::with_capacity. Also, verify that this change is consistent across the entire codebase.lightning/src/ln/outbound_payment.rs (5)
169-169: The replacement of
HashSet::new()withnew_hash_set()is consistent with the PR's objective to standardize the creation of hash sets. This change looks good and follows the DRY principle by using a dedicated function for initialization.184-184: The use of
new_hash_set()for initializingour_session_privsis in line with the changes made in the rest of the file. It's good to see consistency in the use of the new initialization function.193-193: Again, the initialization of
session_privswithnew_hash_set()is consistent with the other changes. This maintains the uniformity of the codebase.673-673: The change to initialize
pending_outbound_paymentswithnew_hash_map()is consistent with the PR's objective to standardize the creation of hash maps. This is a good refactor that improves the maintainability of the code.1266-1266: The initialization of
session_privswithnew_hash_set()here is consistent with the other changes in the file. It's good to see the new function being used throughout the file, which improves the readability and maintainability of the code.lightning/src/routing/gossip.rs (2)
1356-1357: The replacement of
HashMap::new()withnew_hash_map()is consistent with the PR's objective to standardize the creation of hash maps. This change should ensure that any custom logic or default configurations needed for hash maps are centralized in thenew_hash_map()function.1401-1402: The initialization of
last_rapid_gossip_sync_timestampwithNonerepresents a change in the default state ofNetworkGraph. This change should be verified to ensure that it aligns with the intended logic, especially since it was previously initialized with a variable that might have held a different value.lightning/src/ln/peer_handler.rs (2)
948-949: The replacement of direct
HashMap::new()calls withnew_hash_map()is consistent with the PR's objective to standardize the initialization of hash maps. This change should be checked for correctness in the context of thePeerManagerstruct initialization.1942-1942: The use of
new_hash_map()for initializingpeers_to_disconnectis in line with the changes made in the rest of the file. It's important to ensure thatnew_hash_map()is implemented correctly and that it does not introduce any performance regressions or unexpected behavior.lightning/src/ln/monitor_tests.rs (1)
- 2575-2575: The replacement of direct
HashMapinstantiation withnew_hash_map()is consistent with the PR's objective to standardize the creation of hash maps. Ensure thatnew_hash_map()is properly defined and used consistently across the codebase.Verification successful
The occurrences of
HashMap::new()found in the output are in test files, fuzzing files, and utility files. This suggests that the main codebase has been refactored to usenew_hash_map()as intended by the PR, and the remaining usages ofHashMap::new()are likely intentional and appropriate for their specific contexts. The PR's objective to standardize hash map creation in the main codebase appears to have been met.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that new_hash_map() is defined. rg --type rust 'fn new_hash_map' # Verify that new_hash_map() is used consistently instead of HashMap::new(). rg --type rust 'HashMap::new' --files-without-matchLength of output: 9757
Script:
#!/bin/bash # Check for occurrences of `HashMap::new` to see if there are any that should have been replaced by `new_hash_map()`. rg --type rust 'HashMap::new'Length of output: 1652
lightning/src/ln/functional_test_utils.rs (5)
588-588: The replacement of
HashMap::new()withnew_hash_map()seems consistent with the PR's objective to standardize the creation of hash maps. Ensure that thenew_hash_map()function is defined and behaves as expected.1018-1018: The use of
new_hash_map()here is consistent with the changes in the rest of the codebase. However, it's important to ensure that theassert!statement retains its intended functionality with the new map creation method.3011-3011: The introduction of
new_hash_set()is consistent with the PR's refactoring goals. The logic within theretainmethod appears to be unchanged, which is good for maintaining existing functionality.3076-3076: Again,
new_hash_set()is used here to replaceHashSet::new(). The subsequent logic seems to be intact. It's important to verify thatnew_hash_set()is properly defined and accessible in this context.3176-3176: The macro
get_chan_reestablish_msgsnow usesnew_hash_set()which aligns with the PR's refactoring strategy. Ensure that the macro's behavior is consistent with the new set creation method.lightning/src/routing/scoring.rs (7)
656-656: The replacement of
HashMap::new()withnew_hash_map()is consistent with the PR's objective to standardize the creation of hash maps. This change should be checked for correctness in the context of theProbabilisticScoringFeeParametersstruct.698-698: The change in the
clear_manual_penaltiesmethod reflects the same standardization as above. Ensure that thenew_hash_map()function is properly defined and accessible in this context.712-712: Again, the initialization of
manual_node_penaltieswithin thenewmethod ofProbabilisticScorerhas been updated to usenew_hash_map(). This change is consistent with the rest of the PR's changes and should be checked for correctness.822-822: The initialization of
channel_liquiditieswithnew_hash_map()in theProbabilisticScorerstruct is another instance of the same refactoring pattern. This change should be checked for correctness.1333-1333: The update to the retrieval of
penaltyfrommanual_node_penaltiesby removing the reference operator&is a change in how keys are accessed in the map. This should be checked to ensure that it does not affect the intended behavior, especially since Rust's HashMap allows for key access without a reference.1363-1363: Similarly, the change in the retrieval of
channel_liquiditiesby removing the reference operator&should be checked for correctness. This change is consistent with the previous one and should be verified to ensure it aligns with Rust's HashMap key access patterns.2076-2076: The introduction of
new_hash_map()in theReadableArgsimplementation forProbabilisticScoringDecayParametersis consistent with the other changes in this PR. Ensure that thenew_hash_map()function is properly defined and accessible in this context.lightning/src/routing/router.rs (8)
285-285: The replacement of
HashMap::new()withnew_hash_map()in theInFlightHtlcs::newfunction is consistent with the PR's objective to standardize hash map initialization.1938-1938: The use of
hash_map_with_capacityinstead ofHashMap::with_capacityis a good practice to encapsulate the creation logic, which can be beneficial for future modifications or optimizations.1957-1957: The initialization of
private_hop_key_cachewithhash_map_with_capacityis correctly using the sum of the lengths of the unblinded route hints as the initial capacity, which should optimize memory allocation.1978-1978: Replacing
HashMap::new()withhash_map_with_capacityfordistand usingnetwork_nodes.len()as the initial capacity is a good optimization, as it likely approximates the number of nodes that will be inserted.2019-2019: The
used_liquiditieshash map is also initialized withhash_map_with_capacity, using the length ofnetwork_nodesfor capacity. This change is consistent with the other refactoring efforts and should help with performance.2537-2537: The check for
first_channels.iter().any(...)is a good use of iterators to efficiently determine if any channel matches the condition. This is a clean and readable way to perform the check.2547-2547: The use of
and_thenandmapfor chaining option transformations is idiomatic Rust and is used correctly here to handle thenetwork_channelslookup and subsequent operation.2588-2588: The call to
sort_first_hop_channelswithin theif let Some(first_channels)block is a good example of conditional logic that only executes if thefirst_hop_targetscontains the target. This ensures that the sorting only happens when necessary.lightning/src/ln/functional_tests.rs (2)
3322-3322: The replacement of
HashSet::new()withnew_hash_set()is consistent with the refactoring effort described in the summary. Ensure thatnew_hash_set()is properly defined and accessible in this context.7336-7336: The change from
HashMap::new()tonew_hash_map()is consistent with the refactoring effort. Ensure thatnew_hash_map()is properly defined and accessible in this context.lightning/src/ln/channelmanager.rs (22)
2421-2428: The replacement of direct
Mutex::new(HashSet::new())andMutex::new(HashMap::new())calls withnew_hash_set()andnew_hash_map()is a good practice for maintainability and consistency. It abstracts the creation logic and allows for future optimizations or changes in one place.2440-2440: The use of
new_hash_map()for initializingper_peer_stateunder aFairRwLockis consistent with the other changes and promotes the same benefits of maintainability and potential optimizations.3673-3673: The introduction of
hash_map_with_capacity(first_hops.len())is a performance optimization that pre-allocates space for the expected number of elements, reducing the need for rehashing as elements are inserted.4224-4224: Using
new_hash_map()to initializeforward_htlcsbefore swapping it withself.forward_htlcsis a logical change that aligns with the rest of the refactoring. However, ensure that themem::swapoperation is still valid and that theforward_htlcsMutex guard is dropped immediately after the swap to avoid potential deadlocks.Verification successful
The review comment is concerned with ensuring that the Mutex guard is dropped immediately after the
mem::swapoperation to avoid potential deadlocks. The provided script output shows that themem::swapis called within a block, and since the Mutex guard is not assigned to a variable, it will be dropped at the end of the statement, which is the behavior desired to prevent deadlocks. This confirms that the implementation is correct regarding the Mutex guard's scope and its release.* 5919-5919: The code snippet provided does not show any changes, but it is related to the `outpoint_to_peer` map which was mentioned earlier in the summary. Without the context of what changed, it's difficult to provide a specific comment. If this is meant to show the usage of `outpoint_to_peer`, then it seems to be correctly accessing the map.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the Mutex guard is dropped immediately after the swap to avoid deadlocks. rg --type rust --context 5 'mem::swap\(&mut forward_htlcs, &mut self.forward_htlcs.lock\(\).unwrap\(\)\);'Length of output: 980
8970-8970: The initialization of
channel_by_idandinbound_channel_request_by_idwithnew_hash_map()is consistent with the refactoring pattern and is approved.10014-10014: The use of
new_hash_map()for initializingpending_outbound_payments_no_retryis consistent with the refactoring pattern. Ensure that the rest of the code correctly handles the new structure.10042-10042: The conditional initialization of
in_flight_monitor_updateswithnew_hash_map()is a good practice to avoid unnecessary memory allocation. However, ensure that theis_none()check is the desired logic and that there are no cases where an existing map should be used or modified instead.Verification successful
Based on the context provided by the script output, the
in_flight_monitor_updatesvariable is correctly initialized withnew_hash_map()only when necessary, which is when there are updates to be inserted. The lazy initialization pattern usingis_none()check before the assignment is a standard practice to avoid unnecessary memory allocation. There are no indications from the provided context that an existing map should be used or modified instead. The logic appears to be correct as per the current usage shown.* 10278-10281: Pre-allocating hash maps and sets with `hash_set_with_capacity()` and `hash_map_with_capacity()` based on `channel_count` is a performance optimization. Ensure that the `cmp::min` logic is correct and that it does not under-allocate for the expected number of channels.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that there are no cases where an existing in_flight_monitor_updates map should be used or modified. rg --type rust --context 5 'in_flight_monitor_updates.is_none\(\)'Length of output: 990
10368-10368: The use of
new_hash_map()for initializingby_id_mapwithin a match arm is consistent with the refactoring pattern and is approved.10413-10413: The use of
hash_map_with_capacity()for initializingforward_htlcswith a pre-allocated capacity is a performance optimization. Ensure that thecmp::minlogic is correct and that it does not under-allocate for the expected number of HTLCs.10439-10439: The initialization of
inbound_channel_request_by_idwithnew_hash_map()is consistent with the refactoring pattern and is approved.10450-10453: The use of
hash_map_with_capacity()for initializingper_peer_statewith a pre-allocated capacity is a performance optimization. Ensure that thecmp::minlogic and theMAX_ALLOC_SIZEconstant are correctly used to avoid over-allocation or under-allocation.Verification successful
The verification process has confirmed that the
cmp::minlogic and theMAX_ALLOC_SIZEconstant are correctly used in thehash_map_with_capacityfunction call for initializingper_peer_statein thechannelmanager.rsfile. The code snippet provided in the output shows the intended performance optimization is in place.* 10487-10487: The use of `hash_map_with_capacity()` for initializing `pending_inbound_payments` with a pre-allocated capacity is a performance optimization. Ensure that the `cmp::min` logic is correct and that it does not under-allocate for the expected number of payments.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the cmp::min logic and the MAX_ALLOC_SIZE constant are correctly used. rg --type rust --context 5 'hash_map_with_capacity\(cmp::min\(peer_count as usize, MAX_ALLOC_SIZE/mem::size_of::<\(PublicKey, Mutex<PeerState<SP>>\)>())\)'Length of output: 458
Script:
#!/bin/bash # Correcting the regex pattern by escaping special characters to verify the usage of cmp::min and MAX_ALLOC_SIZE. rg --type rust --context 5 'hash_map_with_capacity\(cmp::min\(peer_count as usize, MAX_ALLOC_SIZE\/mem::size_of::<\(PublicKey, Mutex<PeerState<SP>>\)>())\)'Length of output: 421
Script:
#!/bin/bash # Simplifying the regex pattern to avoid parse errors and verify the usage of cmp::min and MAX_ALLOC_SIZE. rg --type rust --context 5 'hash_map_with_capacity\(cmp::min'Length of output: 11405
Verification successful
The output from the script has provided the relevant line of code from
channelmanager.rsthat useshash_map_with_capacity()for initializing various hash maps, includingpending_inbound_payments. The line of interest is:let mut pending_inbound_payments: HashMap<PaymentHash, PendingInboundPayment> = hash_map_with_capacity(cmp::min(pending_inbound_payment_count as usize, MAX_ALLOC_SIZE/(3*32)));This line shows that the
cmp::minfunction is used to determine the smaller of two values: thepending_inbound_payment_countcast to ausize, andMAX_ALLOC_SIZE/(3*32). This logic ensures that the capacity of the hash map is not pre-allocated to be larger than necessary, which is a performance optimization to avoid excessive memory allocation.The
MAX_ALLOC_SIZE/(3*32)part of the expression seems to be a calculation to prevent over-allocation based on a maximum allocation size constant (MAX_ALLOC_SIZE) and the size of the elements that will be stored in the hash map (assuming eachPaymentHashandPendingInboundPaymenttakes up3*32bytes).Given this information, the
cmp::minlogic appears to be correctly implemented to allocate enough space for the expected number of inbound payments without exceeding a maximum memory allocation threshold. The specific numbers used in the calculation would be based on the expected size of the data structures and the constraints of the system.* 10496-10496: The use of `hash_map_with_capacity()` for initializing `pending_outbound_payments_compat` with a pre-allocated capacity is a performance optimization. Ensure that the `cmp::min` logic is correct and that it does not under-allocate for the expected number of payments.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the cmp::min logic correctly allocates enough space for the expected number of inbound payments. rg --type rust --context 5 'hash_map_with_capacity\(cmp::min\(pending_inbound_payment_count as usize, MAX_ALLOC_SIZE/\(3*32\)\)\)'Length of output: 132
Script:
#!/bin/bash # Attempt a broader search for the usage of `hash_map_with_capacity()` in the `channelmanager.rs` file. rg --type rust 'hash_map_with_capacity' lightning/src/ln/channelmanager.rsLength of output: 1099
Verification successful
The output from the script shows that the
hash_map_with_capacity()function is used withcmp::minto determine the initial capacity of theHashMapforpending_outbound_payments_compat. The capacity is set to the minimum ofpending_outbound_payments_count_compat as usizeandMAX_ALLOC_SIZE/32. This is a safeguard to prevent over-allocation while also ensuring that there is enough space allocated for the expected number of outbound payments.To further verify the correctness of this logic, we would need to understand the typical values of
pending_outbound_payments_count_compatand the value ofMAX_ALLOC_SIZE. However, based on the context provided, it seems that the logic is designed to balance performance with memory safety by usingcmp::minto set a reasonable upper limit on the initial capacity of theHashMap.* 10510-10510: The initialization of `pending_intercepted_htlcs` with `Some(new_hash_map())` is consistent with the refactoring pattern and is approved. Ensure that the `Some` wrapper is the intended logic and that there are no cases where this should be `None`.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the cmp::min logic correctly allocates enough space for the expected number of outbound payments. rg --type rust --context 5 'hash_map_with_capacity\(cmp::min\(pending_outbound_payments_count_compat as usize, MAX_ALLOC_SIZE/32\)\)'Length of output: 1019
Verification successful
Based on the search results, the initialization of
pending_intercepted_htlcswithSome(new_hash_map())is consistent with the rest of the code. TheOptiontype is used intentionally, and there are mechanisms to handle bothSomeandNonestates. The use ofSomefor initialization aligns with the expected logic in the codebase.* 10553-10553: The use of `new_hash_map()` for initializing `outbounds` within a conditional block is consistent with the refactoring pattern. Ensure that the logic for handling `pending_outbound_payments_no_retry` is correct and that the `unwrap().drain()` operation is safe.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the Some wrapper is the intended logic for pending_intercepted_htlcs. rg --type rust --context 5 'let mut pending_intercepted_htlcs: Option<HashMap<InterceptId, PendingAddHTLCInfo>> = Some\(new_hash_map\(\)\);'Length of output: 1133
Script:
#!/bin/bash # Search for any assignments to `pending_intercepted_htlcs` that might set it to None or rely on it being None. rg --type rust 'pending_intercepted_htlcs' lightning/src/ln/channelmanager.rsLength of output: 1211
Verification successful
The conditional logic in
channelmanager.rsensures thatpending_outbound_payments_no_retry.unwrap().drain()is called only whenpending_outbound_payments_no_retryis notNone, which makes theunwrap()call safe. Thedrain()operation is correctly used to move the contents into a new hash map, which is then assigned topending_outbound_payments. This aligns with the refactoring pattern and the logic appears to be correctly implemented.* 10660-10660: The use of `new_hash_map()` for initializing a new `peer_state` within the `or_insert_with` closure is consistent with the refactoring pattern and is approved. Ensure that the `or_insert_with` logic is correct and that it is the desired behavior to create a new `peer_state` if one does not exist.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the logic for handling pending_outbound_payments_no_retry is correct. rg --type rust --context 5 'pending_outbound_payments_no_retry.unwrap\(\).drain\(\)'Length of output: 985
Verification successful
The verification process has confirmed that the use of
or_insert_withfor initializing a newpeer_statewithin theper_peer_statemap is correct. This ensures that a newpeer_stateis created if one does not exist for the givencounterparty_id, which aligns with the standard Rust practice for handling absent keys in a map.* 10845-10845: The use of `hash_map_with_capacity()` for initializing `claimable_payments` with a pre-allocated capacity is a performance optimization. Ensure that the `claimable_htlcs_list.len()` is the correct basis for the capacity and that the error handling for mismatched lengths is appropriate.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the or_insert_with logic is correct for creating a new peer_state. rg --type rust --context 5 'per_peer_state.entry\(counterparty_id\).or_insert_with\(\|\)'Length of output: 91
Script:
#!/bin/bash # Broaden the search to find the usage of `or_insert_with` related to `per_peer_state`. rg --type rust 'per_peer_state.*\.or_insert_with'Length of output: 172
Script:
#!/bin/bash # If the above does not yield results, find occurrences of `per_peer_state` to understand its usage. rg --type rust 'per_peer_state'Length of output: 25970
Verification successful
Based on the provided context, the use of
claimable_htlcs_list.len()as the capacity forclaimable_paymentsis justified by the subsequent check that ensurespurposes.len()matchesclaimable_htlcs_list.len(). If they do not match, an error is returned, which is appropriate error handling for mismatched lengths. Therefore, the review comment is consistent with the codebase.* 10918-10918: The use of `new_hash_set()` for initializing `outbound_scid_aliases` is consistent with the refactoring pattern and is approved. Ensure that the rest of the code correctly handles the new structure.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the claimable_htlcs_list.len() is the correct basis for the capacity. rg --type rust --context 5 'hash_map_with_capacity\(claimable_htlcs_list.len\(\)\)'Length of output: 950
11023-11023: The code snippet provided does not show any changes, but it is related to the
per_peer_statemap which was mentioned earlier in the summary. Without the context of what changed, it's difficult to provide a specific comment. If this is meant to show the usage ofper_peer_state, then it seems to be correctly accessing the map.12451-12451: The use of
new_hash_map()for initializingforward_htlcsand inserting dummy HTLCs is consistent with the refactoring pattern and is approved. Ensure that the dummy HTLCs are correctly used for testing or example purposes.
| use lightning::routing::gossip::{P2PGossipSync, NetworkGraph}; | ||
| use lightning::routing::utxo::UtxoLookup; | ||
| use lightning::routing::router::{InFlightHtlcs, PaymentParameters, Route, RouteParameters, Router}; | ||
| use lightning::util::config::{UserConfig, MaxDustHTLCExposure}; | ||
| use lightning::util::config::UserConfig; | ||
| use lightning::util::errors::APIError; | ||
| use lightning::util::test_channel_signer::{TestChannelSigner, EnforcementState}; | ||
| use lightning::util::logger::Logger; | ||
| use lightning::util::ser::{ReadableArgs, Writeable}; | ||
| use lightning::util::ser::{Readable, ReadableArgs, Writeable}; | ||
|
|
||
| use crate::utils::test_logger; | ||
| use crate::utils::test_persister::TestPersister; |
There was a problem hiding this comment.
Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [3-3]
The comments across these lines are consistent in providing examples of input data formats. While they are helpful, the recommendation to document these examples outside of the code still stands. Additionally, it's important to ensure that these comments are kept up-to-date with any changes to the input data format to prevent confusion.
Also applies to: 4-4, 5-5, 6-6, 7-7, 8-8, 9-9, 10-10, 11-11, 12-12, 13-13, 14-14
| @@ -2532,9 +2532,9 @@ where L::Target: Logger { | |||
| let mut aggregate_path_contribution_msat = path_value_msat; | |||
|
|
|||
| for (idx, (hop, prev_hop_id)) in hop_iter.zip(prev_hop_iter).enumerate() { | |||
| let target = private_hop_key_cache.get(&prev_hop_id).unwrap(); | |||
| let target = private_hop_key_cache.get(prev_hop_id).unwrap(); | |||
There was a problem hiding this comment.
Accessing the private_hop_key_cache with unwrap() assumes that the entry exists. It would be safer to handle the potential None case explicitly to avoid panics at runtime.
- let target = private_hop_key_cache.get(prev_hop_id).unwrap();
+ let target = private_hop_key_cache.get(prev_hop_id).expect("Expected previous hop ID to exist in private_hop_key_cache");Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| let target = private_hop_key_cache.get(prev_hop_id).unwrap(); | |
| let target = private_hop_key_cache.get(prev_hop_id).expect("Expected previous hop ID to exist in private_hop_key_cache"); |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- lightning/src/ln/functional_tests.rs (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- lightning/src/ln/functional_tests.rs
|
This needs to land so we can keep fuzzing, but it doesn't need to land for a specific release. |
TheBlueMatt
force-pushed
the
2023-12-arbitrary-fuzz-config
branch
2 times, most recently
from
fd0ef78
to
dfa113e
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (3)
fuzz/Cargo.tomlis excluded by:!**/*.tomllightning-invoice/Cargo.tomlis excluded by:!**/*.tomllightning/Cargo.tomlis excluded by:!**/*.toml
Files selected for processing (25)
- ci/check-cfg-flags.py (1 hunks)
- fuzz/src/full_stack.rs (15 hunks)
- lightning/src/chain/chainmonitor.rs (1 hunks)
- lightning/src/chain/channelmonitor.rs (13 hunks)
- lightning/src/chain/onchaintx.rs (6 hunks)
- lightning/src/events/bump_transaction.rs (1 hunks)
- lightning/src/lib.rs (1 hunks)
- lightning/src/ln/channel.rs (3 hunks)
- lightning/src/ln/channelmanager.rs (22 hunks)
- lightning/src/ln/functional_test_utils.rs (5 hunks)
- lightning/src/ln/functional_tests.rs (5 hunks)
- lightning/src/ln/monitor_tests.rs (1 hunks)
- lightning/src/ln/outbound_payment.rs (5 hunks)
- lightning/src/ln/peer_handler.rs (2 hunks)
- lightning/src/onion_message/messenger.rs (2 hunks)
- lightning/src/routing/gossip.rs (2 hunks)
- lightning/src/routing/router.rs (8 hunks)
- lightning/src/routing/scoring.rs (7 hunks)
- lightning/src/routing/utxo.rs (1 hunks)
- lightning/src/sign/mod.rs (1 hunks)
- lightning/src/sync/debug_sync.rs (4 hunks)
- lightning/src/util/config.rs (4 hunks)
- lightning/src/util/indexed_map.rs (4 hunks)
- lightning/src/util/ser.rs (2 hunks)
- lightning/src/util/test_utils.rs (12 hunks)
Files not summarized due to errors (1)
- fuzz/src/full_stack.rs: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (22)
- ci/check-cfg-flags.py
- lightning/src/chain/chainmonitor.rs
- lightning/src/chain/channelmonitor.rs
- lightning/src/chain/onchaintx.rs
- lightning/src/events/bump_transaction.rs
- lightning/src/lib.rs
- lightning/src/ln/channel.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/functional_test_utils.rs
- lightning/src/ln/monitor_tests.rs
- lightning/src/ln/outbound_payment.rs
- lightning/src/ln/peer_handler.rs
- lightning/src/onion_message/messenger.rs
- lightning/src/routing/gossip.rs
- lightning/src/routing/router.rs
- lightning/src/routing/scoring.rs
- lightning/src/routing/utxo.rs
- lightning/src/sign/mod.rs
- lightning/src/sync/debug_sync.rs
- lightning/src/util/config.rs
- lightning/src/util/ser.rs
- lightning/src/util/test_utils.rs
Additional comments: 11
lightning/src/util/indexed_map.rs (5)
- 3-3: The previous comment regarding the wildcard import is still valid.
- 37-37: The change from
HashMap::new()tonew_hash_map()is consistent with the PR's objective to abstract the hash map implementation.- 45-45: The change from
HashMap::with_capacity()tohash_map_with_capacity()is consistent with the PR's objective to abstract the hash map implementation.- 179-179: The update to
VacantEntryto useVacantHashMapEntryis consistent with the PR's objective to abstract the hash map implementation.- 188-188: The update to
OccupiedEntryto useOccupiedHashMapEntryis consistent with the PR's objective to abstract the hash map implementation.fuzz/src/full_stack.rs (2)
- 451-451: The configuration for the test is read from the input data, which allows for dynamic test setups. This is a good practice for fuzz testing as it increases the variety of tests that can be run automatically.
let config: UserConfig = if let Ok(config) = Readable::read(&mut data) { config } else { return; };
- 1050-1050: The test logger is being used correctly to capture log entries during the test. This is useful for debugging and ensuring that the test outputs can be analyzed after the test runs.
let logger = Arc::new(TrackingLogger { lines: Mutex::new(HashMap::new()) });lightning/src/ln/functional_tests.rs (4)
- 3322-3322: The change from
HashSet::new()tonew_hash_set()is consistent with the PR's objective to standardize the creation of hash sets.- 5399-5403: The variable name correction and the change from
HashSet::new()tonew_hash_set()are appropriate and align with the PR's objective to standardize the creation of hash sets.- 5425-5429: The change from
HashSet::new()tonew_hash_set()is consistent with the PR's objective to standardize the creation of hash sets.- 7336-7336: The change from
HashMap::new()tonew_hash_map()is consistent with the PR's objective to standardize the creation of hash maps.
|
LGTM but CI is failing |
TheBlueMatt
force-pushed
the
2023-12-arbitrary-fuzz-config
branch
from
dfa113e
to
9ca367f
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to path filters (3)
fuzz/Cargo.tomlis excluded by:!**/*.tomllightning-invoice/Cargo.tomlis excluded by:!**/*.tomllightning/Cargo.tomlis excluded by:!**/*.toml
Files selected for processing (25)
- ci/check-cfg-flags.py (1 hunks)
- fuzz/src/full_stack.rs (15 hunks)
- lightning/src/chain/chainmonitor.rs (1 hunks)
- lightning/src/chain/channelmonitor.rs (13 hunks)
- lightning/src/chain/onchaintx.rs (6 hunks)
- lightning/src/events/bump_transaction.rs (1 hunks)
- lightning/src/lib.rs (1 hunks)
- lightning/src/ln/channel.rs (3 hunks)
- lightning/src/ln/channelmanager.rs (22 hunks)
- lightning/src/ln/functional_test_utils.rs (5 hunks)
- lightning/src/ln/functional_tests.rs (5 hunks)
- lightning/src/ln/monitor_tests.rs (1 hunks)
- lightning/src/ln/outbound_payment.rs (5 hunks)
- lightning/src/ln/peer_handler.rs (2 hunks)
- lightning/src/onion_message/messenger.rs (2 hunks)
- lightning/src/routing/gossip.rs (2 hunks)
- lightning/src/routing/router.rs (8 hunks)
- lightning/src/routing/scoring.rs (7 hunks)
- lightning/src/routing/utxo.rs (1 hunks)
- lightning/src/sign/mod.rs (1 hunks)
- lightning/src/sync/debug_sync.rs (4 hunks)
- lightning/src/util/config.rs (4 hunks)
- lightning/src/util/indexed_map.rs (4 hunks)
- lightning/src/util/ser.rs (2 hunks)
- lightning/src/util/test_utils.rs (12 hunks)
Files not summarized due to errors (1)
- fuzz/src/full_stack.rs: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (22)
- ci/check-cfg-flags.py
- lightning/src/chain/chainmonitor.rs
- lightning/src/chain/channelmonitor.rs
- lightning/src/chain/onchaintx.rs
- lightning/src/events/bump_transaction.rs
- lightning/src/lib.rs
- lightning/src/ln/channel.rs
- lightning/src/ln/functional_test_utils.rs
- lightning/src/ln/functional_tests.rs
- lightning/src/ln/monitor_tests.rs
- lightning/src/ln/outbound_payment.rs
- lightning/src/ln/peer_handler.rs
- lightning/src/onion_message/messenger.rs
- lightning/src/routing/gossip.rs
- lightning/src/routing/router.rs
- lightning/src/routing/scoring.rs
- lightning/src/routing/utxo.rs
- lightning/src/sign/mod.rs
- lightning/src/sync/debug_sync.rs
- lightning/src/util/config.rs
- lightning/src/util/ser.rs
- lightning/src/util/test_utils.rs
Additional comments: 27
lightning/src/util/indexed_map.rs (4)
- 37-37: The change from
HashMap::new()tonew_hash_map()is good as it abstracts the specific map implementation, which can be beneficial for testing or future changes to the underlying data structure.- 45-45: Similarly, replacing
HashMap::with_capacity(capacity)withhash_map_with_capacity(capacity)is a positive change for the same reasons as the previous comment.- 179-179: The change from
hash_map::VacantEntrytoVacantHashMapEntryis consistent with the abstraction of the underlying hash map implementation. Ensure that the newVacantHashMapEntrytype is correctly defined and behaves as expected.Verification successful
The search results from
lightning/src/lib.rsreveal thatVacantHashMapEntryis a type alias within thelightningcodebase. It is conditionally defined based on the compilation feature flags. The alias points to different types ofVacantEntrydepending on whether thehashbrownfeature is enabled and whether it is in a fuzzing context:
- Without the
hashbrownfeature, it aliases tostd::collections::hash_map::VacantEntry.- With the
hashbrownfeature, it aliases tohashbrown::hash_map::VacantEntrywith a default hash builder.- With both
hashbrownfeature and fuzzing, it aliases tohashbrown::hash_map::VacantEntrywith aRandomState.This conditional compilation ensures that the
VacantHashMapEntrytype is consistent with the underlying hash map implementation being used in the build. TheVacantEntrytype from both the standard library andhashbrowncrate is a well-defined and expected part of a hash map's API, which handles the case when an entry is vacant (i.e., no value is associated with the key in the map).Given this information, we can confirm that the change from
hash_map::VacantEntrytoVacantHashMapEntryin theindexed_map.rsfile is consistent with the abstraction of the underlying hash map implementation, and theVacantHashMapEntrytype is correctly defined and behaves as expected within the context of the codebase's conditional compilation strategy.* 188-188: The update from `hash_map::OccupiedEntry` to `OccupiedHashMapEntry` is also consistent with the abstraction of the underlying hash map implementation. Similar to the previous comment, ensure that `OccupiedHashMapEntry` is correctly defined and behaves as expected.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that VacantHashMapEntry is defined and has the expected methods and properties. rg "struct VacantHashMapEntry"Length of output: 32
Script:
#!/bin/bash # Broaden the search to find the definition or alias of VacantHashMapEntry, and check for use statements that might bring it into scope. rg "VacantHashMapEntry"Length of output: 316
Script:
#!/bin/bash # Check if VacantHashMapEntry is brought into scope from an external crate. rg "use .*VacantHashMapEntry"Length of output: 31
Script:
#!/bin/bash # Look at the lines in `lightning/src/lib.rs` where `VacantHashMapEntry` is mentioned to understand its nature. rg "VacantHashMapEntry" lightning/src/lib.rs -C 3Length of output: 890
Verification successful
The search results indicate that
OccupiedHashMapEntryis indeed present in the codebase, but it seems to be a type alias or re-export defined inlightning/src/lib.rs. To understand whatOccupiedHashMapEntryis an alias for and to verify its properties and methods, we should look at the lines inlib.rswhere it is defined.
The
OccupiedHashMapEntrytype is defined inlightning/src/lib.rsand is an alias forstd::collections::hash_map::OccupiedEntryin the standard library'sHashMapimplementation. Additionally, there are conditional compilations for different versions of theOccupiedHashMapEntry, one using thehashbrowncrate with aDefaultHashBuilderand another with aRandomState.This confirms that the
OccupiedHashMapEntryis correctly defined and is expected to behave as the standardOccupiedEntryfrom the Rust standard library or thehashbrowncrate, depending on the compilation context.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that OccupiedHashMapEntry is defined and has the expected methods and properties. rg "struct OccupiedHashMapEntry"Length of output: 34
Script:
#!/bin/bash # Search for the definition of OccupiedHashMapEntry using ast-grep ast-grep --lang rust --pattern 'struct OccupiedHashMapEntry { $$$ }'Length of output: 70
Script:
#!/bin/bash # Search for any type aliases or re-exports that might point to the actual definition of OccupiedHashMapEntry rg "type OccupiedHashMapEntry"Length of output: 248
Script:
#!/bin/bash # Look at the lines in lib.rs where OccupiedHashMapEntry is defined to understand what it is an alias for rg "type OccupiedHashMapEntry" lightning/src/lib.rs -C 3Length of output: 1067
fuzz/src/full_stack.rs (1)
- 442-451: The changes in lines 442-451 address a previously raised concern about slicing the input data without checking its length. The added check ensures that there is at least 32 bytes of data before attempting to create a
SecretKeyfrom the slice, which prevents potential panics due to slicing with an out-of-bounds index.lightning/src/ln/channelmanager.rs (22)
- 2427-2434: The replacement of direct
HashSetandHashMapinstantiations withnew_hash_set()andnew_hash_map()is consistent with the PR's objective to standardize the creation of these data structures. This should not affect the logic but could impact memory allocation and performance.- 2446-2446: The use of
new_hash_map()forper_peer_stateis in line with the refactoring pattern observed in the rest of the file.- 3680-3680: The introduction of
hash_map_with_capacityis a good practice to pre-allocate memory based on expected size, which can improve performance.- 4231-4231: Using
new_hash_map()and then swapping withself.forward_htlcsis a thread-safe way to handle theforward_htlcsmap. Ensure that this change is reflected whereverforward_htlcsis used.Verification successful
The output from the script shows consistent use of
new_hash_map()for initializingforward_htlcsacross the codebase. TheMutexis used to ensure thread safety when accessing or modifyingforward_htlcs. The changes made toforward_htlcsare in line with the PR objectives and comments, which aim to standardize the initialization of hash maps and ensure thread safety.* 5926-5926: The code here is unchanged, but it's worth noting that the comment suggests future refactoring. Ensure that the `outpoint_to_peer` map is still required and that the comment is still relevant.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the new_hash_map usage for forward_htlcs is consistent across the codebase rg --type rust "forward_htlcs" --context 5Length of output: 30348
Verification successful
The
outpoint_to_peermap is still required in the codebase, as evidenced by its usage in various parts of the code, including tests that verify its state during channel operations. The comments regarding future refactoring suggest that it may become redundant once certain changes are made, but these changes have not yet been implemented. Therefore, the comment about future refactoring remains relevant.* 8977-8978: The initialization of `channel_by_id` and `inbound_channel_request_by_id` with `new_hash_map()` is consistent with the refactoring pattern. This is a good practice for maintainability. * 10023-10023: The use of `new_hash_map()` for `pending_outbound_payments_no_retry` is consistent with the refactoring pattern and improves the readability of the code. * 10051-10051: The conditional initialization of `in_flight_monitor_updates` with `new_hash_map()` is a good practice to avoid unnecessary memory allocation. * 10287-10290: Pre-allocating memory for hash maps and sets with `hash_map_with_capacity` and `hash_set_with_capacity` is a performance optimization that should be beneficial given the context. * 10377-10377: The use of `new_hash_map()` for `by_id_map` is consistent with the refactoring pattern and improves the readability of the code. * 10422-10422: Pre-allocating memory for `forward_htlcs` with `hash_map_with_capacity` is a performance optimization that should be beneficial given the context. * 10448-10448: The initialization of `inbound_channel_request_by_id` with `new_hash_map()` is consistent with the refactoring pattern and improves the readability of the code. * 10459-10462: Pre-allocating memory for `per_peer_state` with `hash_map_with_capacity` is a performance optimization that should be beneficial given the context. * 10496-10496: Pre-allocating memory for `pending_inbound_payments` with `hash_map_with_capacity` is a performance optimization that should be beneficial given the context. * 10505-10505: Pre-allocating memory for `pending_outbound_payments_compat` with `hash_map_with_capacity` is a performance optimization that should be beneficial given the context. * 10519-10519: The conditional initialization of various hash maps with `new_hash_map()` is consistent with the refactoring pattern and improves the readability of the code. * 10562-10562: The use of `new_hash_map()` for `outbounds` is consistent with the refactoring pattern and improves the readability of the code. * 10669-10669: The use of `new_hash_map()` within `or_insert_with` is a good practice to avoid unnecessary memory allocation unless it's needed. * 10854-10854: Pre-allocating memory for `claimable_payments` with `hash_map_with_capacity` is a performance optimization that should be beneficial given the context. * 10927-10927: The use of `new_hash_set()` for `outbound_scid_aliases` is consistent with the refactoring pattern and improves the readability of the code. * 11032-11032: This code segment is unchanged, but it's worth noting that the comment suggests future refactoring. Ensure that the `per_peer_state` map is still required and that the comment is still relevant.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if the outpoint_to_peer map is still required and if there are any plans to refactor it as suggested by the comment. rg --type rust "outpoint_to_peer" --context 5Length of output: 28755
Verification successful
Based on the search results, there are no explicit comments in the codebase suggesting future refactoring plans for the
per_peer_statemap. The map is currently in use and serves its purpose within the system as indicated by the comments and code structure.* 12460-12460: The use of `new_hash_map()` for `forward_htlcs` in a test context is consistent with the refactoring pattern and improves the readability of the code.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if the per_peer_state map is still required and if there are any plans to refactor it as suggested by the comment. rg --type rust "per_peer_state" --context 5Length of output: 149828
Script:
#!/bin/bash # Search for comments related to `per_peer_state` that might suggest refactoring plans. rg --type rust "//" -A 5 "per_peer_state"Length of output: 94
Script:
#!/bin/bash # Corrected search for comments related to `per_peer_state` within the files. rg --type rust "//.*per_peer_state" -A 5Length of output: 5297
TheBlueMatt
force-pushed
the
2023-12-arbitrary-fuzz-config
branch
from
9ca367f
to
a1a6f52
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to path filters (4)
bench/Cargo.tomlis excluded by:!**/*.tomlfuzz/Cargo.tomlis excluded by:!**/*.tomllightning-invoice/Cargo.tomlis excluded by:!**/*.tomllightning/Cargo.tomlis excluded by:!**/*.toml
Files selected for processing (25)
- ci/check-cfg-flags.py (1 hunks)
- fuzz/src/full_stack.rs (15 hunks)
- lightning/src/chain/chainmonitor.rs (1 hunks)
- lightning/src/chain/channelmonitor.rs (13 hunks)
- lightning/src/chain/onchaintx.rs (6 hunks)
- lightning/src/events/bump_transaction.rs (1 hunks)
- lightning/src/lib.rs (1 hunks)
- lightning/src/ln/channel.rs (3 hunks)
- lightning/src/ln/channelmanager.rs (22 hunks)
- lightning/src/ln/functional_test_utils.rs (5 hunks)
- lightning/src/ln/functional_tests.rs (5 hunks)
- lightning/src/ln/monitor_tests.rs (1 hunks)
- lightning/src/ln/outbound_payment.rs (5 hunks)
- lightning/src/ln/peer_handler.rs (2 hunks)
- lightning/src/onion_message/messenger.rs (2 hunks)
- lightning/src/routing/gossip.rs (2 hunks)
- lightning/src/routing/router.rs (8 hunks)
- lightning/src/routing/scoring.rs (7 hunks)
- lightning/src/routing/utxo.rs (1 hunks)
- lightning/src/sign/mod.rs (1 hunks)
- lightning/src/sync/debug_sync.rs (4 hunks)
- lightning/src/util/config.rs (4 hunks)
- lightning/src/util/indexed_map.rs (4 hunks)
- lightning/src/util/ser.rs (2 hunks)
- lightning/src/util/test_utils.rs (12 hunks)
Files not summarized due to errors (1)
- fuzz/src/full_stack.rs: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (22)
- ci/check-cfg-flags.py
- lightning/src/chain/chainmonitor.rs
- lightning/src/chain/channelmonitor.rs
- lightning/src/chain/onchaintx.rs
- lightning/src/events/bump_transaction.rs
- lightning/src/lib.rs
- lightning/src/ln/channel.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/functional_test_utils.rs
- lightning/src/ln/functional_tests.rs
- lightning/src/ln/monitor_tests.rs
- lightning/src/ln/outbound_payment.rs
- lightning/src/ln/peer_handler.rs
- lightning/src/onion_message/messenger.rs
- lightning/src/routing/gossip.rs
- lightning/src/routing/router.rs
- lightning/src/routing/scoring.rs
- lightning/src/routing/utxo.rs
- lightning/src/sign/mod.rs
- lightning/src/util/config.rs
- lightning/src/util/ser.rs
- lightning/src/util/test_utils.rs
Additional comments: 12
lightning/src/util/indexed_map.rs (4)
- 37-37: The change from
HashMap::new()tonew_hash_map()suggests a custom implementation for hash map creation. Ensure thatnew_hash_map()is thoroughly tested and benchmarked againstHashMap::new()to avoid performance regressions.- 45-45: Similar to the previous comment, the change from
HashMap::with_capacity(capacity)tohash_map_with_capacity(capacity)should be verified for performance implications, especially since this method is often used for optimization purposes.- 179-179: The change from
hash_map::VacantEntrytoVacantHashMapEntryabstracts away the specific hash map entry type. Ensure thatVacantHashMapEntrymaintains the same API contract and performance characteristics as the original.- 188-188: The same verification for
OccupiedHashMapEntryas forVacantHashMapEntryapplies here. Confirm that the abstraction does not introduce any unexpected behavior or performance issues.lightning/src/sync/debug_sync.rs (3)
- 60-60: The replacement of
HashMap::new()withnew_hash_map()in the thread-localLOCKS_HELDis noted. As with the previous file, ensure thatnew_hash_map()is properly tested and does not introduce performance regressions.- 116-116: The change to
new_hash_map()in thelocked_beforefield initialization withinLockMetadatastruct should be verified for performance and correctness, similar to previous instances.- 125-125: The introduction of
new_hash_map()in theLOCKS_INITcall_once lambda should be verified to ensure that the initialization logic is correct and does not introduce thread-safety issues.fuzz/src/full_stack.rs (5)
- 442-451: The previous comment regarding the potential panic when slicing
dataforSecretKeycreation has been addressed with a length check.- 451-451: The configuration object
UserConfigis now being read from the input data. Ensure that the input data format is well-documented and that theReadabletrait implementation forUserConfigis robust against malformed input.- 788-788: The previous comment about documenting the input data format outside of the code is still valid. Ensure that this documentation is created and maintained.
- 798-798: The previous comment about documenting the input data format outside of the code is still valid. Ensure that this documentation is created and maintained.
- 846-846: The previous comment about documenting the input data format outside of the code is still valid. Ensure that this documentation is created and maintained.
TheBlueMatt
force-pushed
the
2023-12-arbitrary-fuzz-config
branch
from
a1a6f52
to
d521777
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (4)
bench/Cargo.tomlis excluded by:!**/*.tomlfuzz/Cargo.tomlis excluded by:!**/*.tomllightning-invoice/Cargo.tomlis excluded by:!**/*.tomllightning/Cargo.tomlis excluded by:!**/*.toml
Files selected for processing (26)
- .github/workflows/build.yml (1 hunks)
- ci/check-cfg-flags.py (1 hunks)
- fuzz/src/full_stack.rs (15 hunks)
- lightning/src/chain/chainmonitor.rs (4 hunks)
- lightning/src/chain/channelmonitor.rs (13 hunks)
- lightning/src/chain/onchaintx.rs (6 hunks)
- lightning/src/events/bump_transaction.rs (1 hunks)
- lightning/src/lib.rs (1 hunks)
- lightning/src/ln/channel.rs (3 hunks)
- lightning/src/ln/channelmanager.rs (23 hunks)
- lightning/src/ln/functional_test_utils.rs (5 hunks)
- lightning/src/ln/functional_tests.rs (5 hunks)
- lightning/src/ln/monitor_tests.rs (1 hunks)
- lightning/src/ln/outbound_payment.rs (5 hunks)
- lightning/src/ln/peer_handler.rs (2 hunks)
- lightning/src/onion_message/messenger.rs (2 hunks)
- lightning/src/routing/gossip.rs (2 hunks)
- lightning/src/routing/router.rs (8 hunks)
- lightning/src/routing/scoring.rs (7 hunks)
- lightning/src/routing/utxo.rs (1 hunks)
- lightning/src/sign/mod.rs (1 hunks)
- lightning/src/sync/debug_sync.rs (4 hunks)
- lightning/src/util/config.rs (4 hunks)
- lightning/src/util/indexed_map.rs (4 hunks)
- lightning/src/util/ser.rs (2 hunks)
- lightning/src/util/test_utils.rs (12 hunks)
Files not summarized due to errors (1)
- fuzz/src/full_stack.rs: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (21)
- ci/check-cfg-flags.py
- lightning/src/chain/chainmonitor.rs
- lightning/src/chain/channelmonitor.rs
- lightning/src/chain/onchaintx.rs
- lightning/src/events/bump_transaction.rs
- lightning/src/lib.rs
- lightning/src/ln/channel.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/functional_test_utils.rs
- lightning/src/ln/functional_tests.rs
- lightning/src/ln/monitor_tests.rs
- lightning/src/ln/outbound_payment.rs
- lightning/src/ln/peer_handler.rs
- lightning/src/onion_message/messenger.rs
- lightning/src/routing/gossip.rs
- lightning/src/routing/router.rs
- lightning/src/routing/utxo.rs
- lightning/src/sign/mod.rs
- lightning/src/util/config.rs
- lightning/src/util/ser.rs
- lightning/src/util/test_utils.rs
Additional comments: 23
lightning/src/util/indexed_map.rs (5)
- 3-3: The previous comment regarding the wildcard import
use crate::prelude::*;is still valid.- 37-37: The use of
new_hash_map()instead ofHashMap::new()is consistent with the PR's objective to abstract theHashMaptype.- 45-45: The use of
hash_map_with_capacity(capacity)instead ofHashMap::with_capacity(capacity)is consistent with the PR's objective to abstract theHashMaptype.- 179-179: The change from
hash_map::VacantEntrytoVacantHashMapEntryis consistent with the PR's objective to abstract theHashMaptype.- 188-188: The change from
hash_map::OccupiedEntrytoOccupiedHashMapEntryis consistent with the PR's objective to abstract theHashMaptype..github/workflows/build.yml (1)
- 113-113: The addition of the
ahashfeature to thecargo testcommand is consistent with the PR's objective to enable different hashing algorithms.lightning/src/sync/debug_sync.rs (4)
- 17-17: The previous comment regarding the wildcard import
use crate::prelude::*;is still valid.- 60-60: The use of
new_hash_map()instead ofHashMap::new()is consistent with the PR's objective to abstract theHashMaptype.- 116-116: The use of
new_hash_map()instead ofHashMap::new()within theLockMetadatastruct is consistent with the PR's objective to abstract theHashMaptype.- 125-125: The initialization of
LOCKSwithnew_hash_map()is consistent with the PR's objective to abstract theHashMaptype.fuzz/src/full_stack.rs (6)
- 442-451: The previous comment about checking the length of
databefore slicing it forSecretKeycreation has been addressed with a length check on line 443.- 451-451: The configuration object
UserConfigis now being read from the input data, which aligns with the PR's objective to enablefull_stack_targetto read an arbitrary configuration object.- 788-788: The comment block at line 788 provides an example of the input data format. Since this is a test file and the comment is providing context for the test data, it seems appropriate to keep it inline for easy reference by developers working on or debugging the tests.
- 798-798: The comment block at line 798 is similar to the one at line 788, providing an example of the input data format. The same reasoning applies here; it is useful for context within the test file.
- 846-846: The comment block at line 846, like the previous ones, provides an example of the input data format. It remains useful for context and should be kept inline in the test file.
- 1050-1050: The hardcoded hex string used for simulating a fuzzing input is still present. This is acceptable in the context of a fuzzing test, where specific byte sequences are used to simulate various scenarios. However, it's important to ensure that these strings are well-documented and, if they represent specific test cases, that they are kept up-to-date with the test scenarios they are meant to simulate.
lightning/src/routing/scoring.rs (7)
- 656-656: The change from
HashMap::new()tonew_hash_map()formanual_node_penaltiesis consistent with the PR's objective to standardize hash map creation.- 698-698: Replacing
HashMap::new()withnew_hash_map()in theclear_manual_penaltiesmethod aligns with the refactoring pattern and is approved.- 712-712: The initialization of
manual_node_penaltieswithnew_hash_map()in this context is also consistent with the PR's refactoring goals.- 822-822: The use of
new_hash_map()for initializingchannel_liquiditiesis in line with the PR's aim to standardize the creation of hash maps.- 1333-1333: The retrieval of
manual_node_penaltiesusinggetis correct and does not alter the logic of penalty application.- 1363-1363: The access pattern for
channel_liquiditiesremains unchanged, and the use ofnew_hash_map()does not affect the existing logic.- 2076-2076: The deserialization of
channel_liquiditiesusingnew_hash_map()is appropriate and maintains the consistency of the refactoring effort.
TheBlueMatt
force-pushed
the
2023-12-arbitrary-fuzz-config
branch
from
3d3d1f6
to
6902370
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (4)
bench/Cargo.tomlis excluded by:!**/*.tomlfuzz/Cargo.tomlis excluded by:!**/*.tomllightning-invoice/Cargo.tomlis excluded by:!**/*.tomllightning/Cargo.tomlis excluded by:!**/*.toml
Files selected for processing (26)
- .github/workflows/build.yml (3 hunks)
- ci/check-cfg-flags.py (1 hunks)
- fuzz/src/full_stack.rs (15 hunks)
- lightning/src/chain/chainmonitor.rs (4 hunks)
- lightning/src/chain/channelmonitor.rs (13 hunks)
- lightning/src/chain/onchaintx.rs (6 hunks)
- lightning/src/events/bump_transaction.rs (1 hunks)
- lightning/src/lib.rs (1 hunks)
- lightning/src/ln/channel.rs (3 hunks)
- lightning/src/ln/channelmanager.rs (24 hunks)
- lightning/src/ln/functional_test_utils.rs (5 hunks)
- lightning/src/ln/functional_tests.rs (5 hunks)
- lightning/src/ln/monitor_tests.rs (1 hunks)
- lightning/src/ln/outbound_payment.rs (5 hunks)
- lightning/src/ln/peer_handler.rs (2 hunks)
- lightning/src/onion_message/messenger.rs (2 hunks)
- lightning/src/routing/gossip.rs (2 hunks)
- lightning/src/routing/router.rs (8 hunks)
- lightning/src/routing/scoring.rs (7 hunks)
- lightning/src/routing/utxo.rs (1 hunks)
- lightning/src/sign/mod.rs (1 hunks)
- lightning/src/sync/debug_sync.rs (4 hunks)
- lightning/src/util/config.rs (4 hunks)
- lightning/src/util/indexed_map.rs (4 hunks)
- lightning/src/util/ser.rs (2 hunks)
- lightning/src/util/test_utils.rs (12 hunks)
Files not summarized due to errors (1)
- fuzz/src/full_stack.rs: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (23)
- .github/workflows/build.yml
- ci/check-cfg-flags.py
- lightning/src/chain/chainmonitor.rs
- lightning/src/chain/channelmonitor.rs
- lightning/src/chain/onchaintx.rs
- lightning/src/events/bump_transaction.rs
- lightning/src/lib.rs
- lightning/src/ln/channel.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/functional_test_utils.rs
- lightning/src/ln/functional_tests.rs
- lightning/src/ln/monitor_tests.rs
- lightning/src/ln/outbound_payment.rs
- lightning/src/ln/peer_handler.rs
- lightning/src/onion_message/messenger.rs
- lightning/src/routing/gossip.rs
- lightning/src/routing/router.rs
- lightning/src/routing/scoring.rs
- lightning/src/routing/utxo.rs
- lightning/src/sign/mod.rs
- lightning/src/util/config.rs
- lightning/src/util/ser.rs
- lightning/src/util/test_utils.rs
Additional comments: 15
lightning/src/util/indexed_map.rs (5)
- 3-3: The previous comment regarding the wildcard import
use crate::prelude::*;is still valid.- 37-37: The change from
HashMap::new()tonew_hash_map()is consistent with the PR's objective to abstract theHashMaptype.- 45-45: The change from
HashMap::with_capacity(capacity)tohash_map_with_capacity(capacity)aligns with the PR's objective and ensures consistency in map creation.- 179-179: The change in the
VacantEntrystruct to useVacantHashMapEntryinstead ofhash_map::VacantEntryis consistent with the PR's objective to support different underlying hash map implementations.- 188-188: The change in the
OccupiedEntrystruct to useOccupiedHashMapEntryinstead ofhash_map::OccupiedEntryis consistent with the PR's objective to support different underlying hash map implementations.lightning/src/sync/debug_sync.rs (4)
- 17-17: The previous comment regarding the wildcard import
use crate::prelude::*;is still valid.- 60-60: The change to initialize
LOCKS_HELDwithnew_hash_map()instead ofHashMap::new()is consistent with the PR's objective to abstract theHashMaptype.- 116-116: The change to initialize
locked_beforewithnew_hash_map()instead ofHashMap::new()is consistent with the PR's objective to abstract theHashMaptype.- 125-125: The change to initialize
LOCKSwithnew_hash_map()instead ofHashMap::new()is consistent with the PR's objective to abstract theHashMaptype.fuzz/src/full_stack.rs (6)
- 442-451: The changes made to the
do_testfunction include a check for the length ofdatabefore attempting to slice it for theSecretKey. This is a good addition for preventing panics due to slicing with an out-of-bounds index.- 451-451: The configuration object
UserConfigis now being read from thedataslice. Ensure that theReadabletrait implementation forUserConfigproperly handles arbitrary data without causing panics or other issues.- 788-788: The comment block at line 788 provides an example of the input data format. As previously noted, this should be documented outside of the code for better visibility and maintainability.
- 798-798: The comment block at line 798 is similar to the one at line 788, providing an example of the input data format. The recommendation to document this outside of the code still stands.
- 846-846: The comment block at line 846 also provides an example of the input data format. The same recommendation to document this information in a more formal and discoverable manner applies here as well.
- 1050-1050: The hardcoded hex string used for simulating a fuzzing input is still present. It's recommended to refactor this to use a more dynamic approach for generating test cases.
TheBlueMatt
force-pushed
the
2023-12-arbitrary-fuzz-config
branch
from
6902370
to
2cab60e
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to path filters (4)
bench/Cargo.tomlis excluded by:!**/*.tomlfuzz/Cargo.tomlis excluded by:!**/*.tomllightning-invoice/Cargo.tomlis excluded by:!**/*.tomllightning/Cargo.tomlis excluded by:!**/*.toml
Files selected for processing (26)
- .github/workflows/build.yml (3 hunks)
- ci/check-cfg-flags.py (1 hunks)
- fuzz/src/full_stack.rs (15 hunks)
- lightning/src/chain/chainmonitor.rs (4 hunks)
- lightning/src/chain/channelmonitor.rs (13 hunks)
- lightning/src/chain/onchaintx.rs (6 hunks)
- lightning/src/events/bump_transaction.rs (1 hunks)
- lightning/src/lib.rs (1 hunks)
- lightning/src/ln/channel.rs (3 hunks)
- lightning/src/ln/channelmanager.rs (24 hunks)
- lightning/src/ln/functional_test_utils.rs (5 hunks)
- lightning/src/ln/functional_tests.rs (5 hunks)
- lightning/src/ln/monitor_tests.rs (1 hunks)
- lightning/src/ln/outbound_payment.rs (5 hunks)
- lightning/src/ln/peer_handler.rs (2 hunks)
- lightning/src/onion_message/messenger.rs (2 hunks)
- lightning/src/routing/gossip.rs (2 hunks)
- lightning/src/routing/router.rs (8 hunks)
- lightning/src/routing/scoring.rs (7 hunks)
- lightning/src/routing/utxo.rs (1 hunks)
- lightning/src/sign/mod.rs (1 hunks)
- lightning/src/sync/debug_sync.rs (4 hunks)
- lightning/src/util/config.rs (4 hunks)
- lightning/src/util/indexed_map.rs (4 hunks)
- lightning/src/util/ser.rs (2 hunks)
- lightning/src/util/test_utils.rs (12 hunks)
Files not summarized due to errors (1)
- fuzz/src/full_stack.rs: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (22)
- .github/workflows/build.yml
- ci/check-cfg-flags.py
- lightning/src/chain/chainmonitor.rs
- lightning/src/chain/channelmonitor.rs
- lightning/src/chain/onchaintx.rs
- lightning/src/events/bump_transaction.rs
- lightning/src/lib.rs
- lightning/src/ln/channel.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/functional_test_utils.rs
- lightning/src/ln/functional_tests.rs
- lightning/src/ln/monitor_tests.rs
- lightning/src/ln/outbound_payment.rs
- lightning/src/ln/peer_handler.rs
- lightning/src/onion_message/messenger.rs
- lightning/src/routing/gossip.rs
- lightning/src/routing/router.rs
- lightning/src/routing/utxo.rs
- lightning/src/sign/mod.rs
- lightning/src/util/config.rs
- lightning/src/util/ser.rs
- lightning/src/util/test_utils.rs
Additional comments: 19
lightning/src/util/indexed_map.rs (4)
- 37-37: The change from
HashMap::new()tonew_hash_map()in thenew()method is approved as it abstracts the specific map implementation, which could be beneficial for future flexibility or optimizations.- 45-45: The change from
HashMap::with_capacity()tohash_map_with_capacity()in thewith_capacity()method is approved for the same reasons as the change in thenew()method.- 179-179: The change in the
VacantEntrystruct to useVacantHashMapEntryinstead ofhash_map::VacantEntryis approved as it aligns with the abstraction of the underlying map implementation.- 188-188: Similarly, the change in the
OccupiedEntrystruct to useOccupiedHashMapEntryinstead ofhash_map::OccupiedEntryis approved for consistency with theVacantEntrychanges.lightning/src/sync/debug_sync.rs (3)
- 60-60: The initialization of
LOCKS_HELDusingnew_hash_map()instead ofHashMap::new()is approved for the same reasons as in indexed_map.rs.- 116-116: The change to initialize
locked_beforewithnew_hash_map()is consistent with the other changes and is approved.- 125-125: The initialization of
LOCKSwithinLOCKS_INIT.call_once()usingnew_hash_map()is approved, maintaining consistency with the other changes.fuzz/src/full_stack.rs (5)
- 442-451: The previous comment about checking the length of
databefore slicing it forSecretKeycreation has been addressed with a length check on line 443.- 788-788: The previous comment about documenting the input data format outside of the code is still valid. The comment block at line 788 should be moved to a more formal documentation.
- 798-798: The previous comment about documenting the input data format outside of the code is still valid. The comment block at line 798 should be moved to a more formal documentation.
- 846-846: The previous comment about documenting the input data format outside of the code is still valid. The comment block at line 846 should be moved to a more formal documentation.
- 1050-1050: The hardcoded hex string used for simulating a fuzzing input is still present. Consider refactoring this to use a more dynamic approach for generating test cases.
lightning/src/routing/scoring.rs (7)
- 656-656: The replacement of
HashMap::new()withnew_hash_map()in the initialization ofmanual_node_penaltiesis consistent with the PR's objective to standardize the creation of hash maps.- 698-698: The change in
clear_manual_penaltiesmethod to usenew_hash_map()aligns with the refactoring pattern and is a straightforward substitution.- 712-712: Again, the use of
new_hash_map()for initializingmanual_node_penaltieswithin a different context is consistent with the rest of the changes and follows the PR's theme.- 822-822: The initialization of
channel_liquiditieswithnew_hash_map()is another instance of the refactoring pattern applied in this PR. It's a simple replacement and maintains consistency.- 1333-1333: The access pattern for
manual_node_penaltiesremains unchanged, which is good as only the initialization method has been refactored. No action is needed here.- 1363-1363: The access to
channel_liquiditiesusing.get()is unaffected by the change in initialization method. This ensures that the existing logic is preserved.- 2076-2076: The introduction of
new_hash_map()in thedecodemethod forchannel_liquiditiesis consistent with the changes made elsewhere in the file. It's important to ensure that the customnew_hash_map()function is compatible with the deserialization process.Verification successful
The
new_hash_mapfunction is defined inlightning/src/lib.rsand simply returns a new instance ofHashMapusingHashMap::new(). There is no custom implementation of theReadabletrait forHashMapfound in the codebase. Therefore, the use ofnew_hash_mapin thedecodemethod forchannel_liquiditiesshould be compatible with the deserialization process.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the new_hash_map function is compatible with the deserialization process. # This script assumes that the new_hash_map function is defined in a file that can be located by its name. fd new_hash_map.rs | xargs cat | grep -A 10 -B 10 "fn new_hash_map" # Additionally, check for any custom deserialization logic that might be affected by this change. rg "impl Readable for HashMap" --type rustLength of output: 114
Script:
#!/bin/bash # Search for the definition of new_hash_map function across the entire codebase. rg "fn new_hash_map" --type rust # Search for any implementation of the Readable trait for HashMap without specifying the file type. rg "impl Readable for HashMap"Length of output: 238
TheBlueMatt
force-pushed
the
2023-12-arbitrary-fuzz-config
branch
from
2cab60e
to
1f56738
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (4)
bench/Cargo.tomlis excluded by:!**/*.tomlfuzz/Cargo.tomlis excluded by:!**/*.tomllightning-invoice/Cargo.tomlis excluded by:!**/*.tomllightning/Cargo.tomlis excluded by:!**/*.toml
Files selected for processing (26)
- .github/workflows/build.yml (3 hunks)
- ci/check-cfg-flags.py (1 hunks)
- fuzz/src/full_stack.rs (15 hunks)
- lightning/src/chain/chainmonitor.rs (4 hunks)
- lightning/src/chain/channelmonitor.rs (13 hunks)
- lightning/src/chain/onchaintx.rs (6 hunks)
- lightning/src/events/bump_transaction.rs (1 hunks)
- lightning/src/lib.rs (1 hunks)
- lightning/src/ln/channel.rs (3 hunks)
- lightning/src/ln/channelmanager.rs (24 hunks)
- lightning/src/ln/functional_test_utils.rs (5 hunks)
- lightning/src/ln/functional_tests.rs (5 hunks)
- lightning/src/ln/monitor_tests.rs (1 hunks)
- lightning/src/ln/outbound_payment.rs (5 hunks)
- lightning/src/ln/peer_handler.rs (2 hunks)
- lightning/src/onion_message/messenger.rs (2 hunks)
- lightning/src/routing/gossip.rs (2 hunks)
- lightning/src/routing/router.rs (8 hunks)
- lightning/src/routing/scoring.rs (7 hunks)
- lightning/src/routing/utxo.rs (1 hunks)
- lightning/src/sign/mod.rs (1 hunks)
- lightning/src/sync/debug_sync.rs (4 hunks)
- lightning/src/util/config.rs (4 hunks)
- lightning/src/util/indexed_map.rs (4 hunks)
- lightning/src/util/ser.rs (2 hunks)
- lightning/src/util/test_utils.rs (12 hunks)
Files not summarized due to errors (1)
- fuzz/src/full_stack.rs: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (23)
- .github/workflows/build.yml
- ci/check-cfg-flags.py
- lightning/src/chain/chainmonitor.rs
- lightning/src/chain/channelmonitor.rs
- lightning/src/chain/onchaintx.rs
- lightning/src/events/bump_transaction.rs
- lightning/src/lib.rs
- lightning/src/ln/channel.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/functional_test_utils.rs
- lightning/src/ln/functional_tests.rs
- lightning/src/ln/monitor_tests.rs
- lightning/src/ln/outbound_payment.rs
- lightning/src/ln/peer_handler.rs
- lightning/src/onion_message/messenger.rs
- lightning/src/routing/gossip.rs
- lightning/src/routing/router.rs
- lightning/src/routing/utxo.rs
- lightning/src/sign/mod.rs
- lightning/src/sync/debug_sync.rs
- lightning/src/util/config.rs
- lightning/src/util/ser.rs
- lightning/src/util/test_utils.rs
Additional comments: 17
lightning/src/util/indexed_map.rs (4)
- 37-37: The
new()function now usesnew_hash_map()instead ofHashMap::new(). This change abstracts away the specific hash map implementation, which is good for modularity and flexibility. Ensure that thenew_hash_map()function is properly tested to handle the creation of hash maps as expected.- 45-45: The
with_capacity()function now useshash_map_with_capacity()instead ofHashMap::with_capacity(). Similar to thenew()function, this change abstracts the hash map creation and should be tested to ensure that the capacity is set correctly in the new hash maps.- 179-179: The
VacantEntrystruct now usesVacantHashMapEntryinstead ofhash_map::VacantEntry. This change is consistent with the abstraction of the hash map implementation. Verify thatVacantHashMapEntrymaintains the same contract and behavior as the previous implementation to avoid any unexpected behavior.- 188-188: The
OccupiedEntrystruct now usesOccupiedHashMapEntryinstead ofhash_map::OccupiedEntry. As withVacantEntry, ensure thatOccupiedHashMapEntryis a suitable replacement and that it behaves as expected in all scenarios whereOccupiedEntryis used.fuzz/src/full_stack.rs (6)
- 442-451: The changes made to the
do_testfunction include a check for the length ofdatabefore attempting to slice it for theSecretKey. This is a good addition for preventing potential panics due to slicing with an out-of-bounds index.- 451-451: The configuration object
UserConfigis now being read from thedataslice. Ensure that theReadabletrait implementation forUserConfigproperly handles any potential errors and that thedataslice contains a correctly formattedUserConfigobject.- 788-788: The comment block at line 788 provides an example of the input data format. As previously noted, this should be documented outside of the code for better visibility and maintainability.
- 798-798: The comment block at line 798 is similar to the one at line 788, providing an example of the input data format. The recommendation to document this outside of the code still stands.
- 846-846: The comment block at line 846 also provides an example of the input data format. The same recommendation to document this information in a more formal and discoverable manner applies here.
- 1050-1050: The hardcoded hex string used for simulating a fuzzing input is still present. Consider refactoring this to use a more dynamic approach for generating test cases.
lightning/src/routing/scoring.rs (7)
- 656-656: The replacement of
HashMap::new()withnew_hash_map()is consistent with the PR's objective to standardize the creation of hash maps. Ensure thatnew_hash_map()is properly defined and imported in this context.- 698-698: The use of
new_hash_map()inclear_manual_penaltiesmethod is appropriate for resetting themanual_node_penalties. Verify that thenew_hash_mapfunction is available in the scope and that its behavior is as expected.- 712-712: Again, the use of
new_hash_map()is consistent with the PR's objectives. Confirm thatnew_hash_map()is correctly implemented and that it does not introduce any unexpected behavior.- 822-822: The initialization of
channel_liquiditieswithnew_hash_map()should be checked for correctness and consistency with the rest of the codebase.- 1333-1333: The retrieval of
manual_node_penaltiesusinggetis standard practice. Ensure that themanual_node_penaltiesmap is correctly populated before this point to avoid logic errors.- 1363-1363: The use of
unwrap_orwithChannelLiquidity::new(Duration::ZERO)is a safe default. However, confirm that this default value is appropriate for all cases wherechannel_liquiditiesmight not have an entry forscid.- 2076-2076: The deserialization of
channel_liquiditiesusingnew_hash_map()followed byread_tlv_fields!macro is a critical operation. Ensure that the macro and thenew_hash_map()function work together without issues, and that the deserialization process is robust.
TheBlueMatt
force-pushed
the
2023-12-arbitrary-fuzz-config
branch
from
1f56738
to
3cf6d0a
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to path filters (4)
bench/Cargo.tomlis excluded by:!**/*.tomlfuzz/Cargo.tomlis excluded by:!**/*.tomllightning-invoice/Cargo.tomlis excluded by:!**/*.tomllightning/Cargo.tomlis excluded by:!**/*.toml
Files selected for processing (26)
- .github/workflows/build.yml (3 hunks)
- ci/check-cfg-flags.py (1 hunks)
- fuzz/src/full_stack.rs (15 hunks)
- lightning/src/chain/chainmonitor.rs (4 hunks)
- lightning/src/chain/channelmonitor.rs (13 hunks)
- lightning/src/chain/onchaintx.rs (6 hunks)
- lightning/src/events/bump_transaction.rs (1 hunks)
- lightning/src/lib.rs (1 hunks)
- lightning/src/ln/channel.rs (3 hunks)
- lightning/src/ln/channelmanager.rs (24 hunks)
- lightning/src/ln/functional_test_utils.rs (5 hunks)
- lightning/src/ln/functional_tests.rs (5 hunks)
- lightning/src/ln/monitor_tests.rs (1 hunks)
- lightning/src/ln/outbound_payment.rs (5 hunks)
- lightning/src/ln/peer_handler.rs (2 hunks)
- lightning/src/onion_message/messenger.rs (2 hunks)
- lightning/src/routing/gossip.rs (2 hunks)
- lightning/src/routing/router.rs (8 hunks)
- lightning/src/routing/scoring.rs (7 hunks)
- lightning/src/routing/utxo.rs (1 hunks)
- lightning/src/sign/mod.rs (1 hunks)
- lightning/src/sync/debug_sync.rs (4 hunks)
- lightning/src/util/config.rs (4 hunks)
- lightning/src/util/indexed_map.rs (4 hunks)
- lightning/src/util/ser.rs (2 hunks)
- lightning/src/util/test_utils.rs (12 hunks)
Files not summarized due to errors (1)
- fuzz/src/full_stack.rs: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (23)
- .github/workflows/build.yml
- ci/check-cfg-flags.py
- lightning/src/chain/chainmonitor.rs
- lightning/src/chain/channelmonitor.rs
- lightning/src/chain/onchaintx.rs
- lightning/src/events/bump_transaction.rs
- lightning/src/lib.rs
- lightning/src/ln/channel.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/functional_test_utils.rs
- lightning/src/ln/functional_tests.rs
- lightning/src/ln/monitor_tests.rs
- lightning/src/ln/outbound_payment.rs
- lightning/src/ln/peer_handler.rs
- lightning/src/onion_message/messenger.rs
- lightning/src/routing/gossip.rs
- lightning/src/routing/router.rs
- lightning/src/routing/scoring.rs
- lightning/src/routing/utxo.rs
- lightning/src/sign/mod.rs
- lightning/src/util/config.rs
- lightning/src/util/ser.rs
- lightning/src/util/test_utils.rs
Additional comments: 9
lightning/src/util/indexed_map.rs (4)
- 37-37: The change from
HashMap::new()tonew_hash_map()suggests a move towards a more standardized way of creating hash maps. Ensure that thenew_hash_mapfunction is implemented correctly and that it does not introduce any performance regressions or unexpected behavior.- 45-45: Similar to the previous comment, the change from
HashMap::with_capacity(capacity)tohash_map_with_capacity(capacity)should be verified for correctness and performance implications.- 179-179: The change in the type of
underlying_entryfromhash_map::VacantEntrytoVacantHashMapEntryindicates an abstraction over the underlying hash map implementation. Verify that the newVacantHashMapEntrytype maintains the expected behavior and interfaces.- 188-188: As with the
VacantEntry, theOccupiedEntry'sunderlying_entrytype change toOccupiedHashMapEntryshould be verified for compatibility and correctness.lightning/src/sync/debug_sync.rs (3)
- 60-60: The initialization of
LOCKS_HELDhas been changed to usenew_hash_map(). As with the previous file, ensure that thenew_hash_mapfunction is correctly implemented and does not introduce any regressions.- 116-116: The
locked_beforefield inLockMetadatanow usesnew_hash_map()for initialization. Verify that this change is consistent with the intended use of theLockMetadatastructure and that thenew_hash_mapfunction is suitable for this context.- 125-125: The initialization of
LOCKSwithin theLOCKS_INITcall_once lambda has been changed to usenew_hash_map(). Ensure that this change does not affect the thread safety and initialization guarantees provided bycall_once.fuzz/src/full_stack.rs (2)
- 442-451: The changes in the
do_testfunction include a check for the minimum length ofdatabefore proceeding, which is a good practice to avoid panics due to slicing errors. The use ofReadable::readto deserialize aUserConfigfromdatais also a safe operation assuming theReadabletrait is implemented correctly forUserConfig.- 442-451: The
do_testfunction has been updated to include a length check for the input data and to deserialize aUserConfigobject. This is a good practice to ensure that the input data is of a sufficient length before attempting to slice it and that the configuration is correctly deserialized before use.
|
The fuzz build is still failing. |
TheBlueMatt
force-pushed
the
2023-12-arbitrary-fuzz-config
branch
2 times, most recently
from
1e8d804
to
cf2a175
Compare
|
Rebased and fixed CI. |
TheBlueMatt
force-pushed
the
2023-12-arbitrary-fuzz-config
branch
2 times, most recently
from
15a0900
to
d0465b6
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to path filters (4)
bench/Cargo.tomlis excluded by:!**/*.tomlfuzz/Cargo.tomlis excluded by:!**/*.tomllightning-invoice/Cargo.tomlis excluded by:!**/*.tomllightning/Cargo.tomlis excluded by:!**/*.toml
Files selected for processing (26)
- .github/workflows/build.yml (3 hunks)
- ci/check-cfg-flags.py (1 hunks)
- fuzz/src/full_stack.rs (15 hunks)
- lightning/src/chain/chainmonitor.rs (4 hunks)
- lightning/src/chain/channelmonitor.rs (13 hunks)
- lightning/src/chain/onchaintx.rs (6 hunks)
- lightning/src/events/bump_transaction.rs (1 hunks)
- lightning/src/lib.rs (1 hunks)
- lightning/src/ln/channel.rs (3 hunks)
- lightning/src/ln/channelmanager.rs (25 hunks)
- lightning/src/ln/functional_test_utils.rs (5 hunks)
- lightning/src/ln/functional_tests.rs (5 hunks)
- lightning/src/ln/monitor_tests.rs (1 hunks)
- lightning/src/ln/outbound_payment.rs (5 hunks)
- lightning/src/ln/peer_handler.rs (2 hunks)
- lightning/src/onion_message/messenger.rs (2 hunks)
- lightning/src/routing/gossip.rs (2 hunks)
- lightning/src/routing/router.rs (8 hunks)
- lightning/src/routing/scoring.rs (7 hunks)
- lightning/src/routing/utxo.rs (1 hunks)
- lightning/src/sign/mod.rs (1 hunks)
- lightning/src/sync/debug_sync.rs (4 hunks)
- lightning/src/util/config.rs (4 hunks)
- lightning/src/util/indexed_map.rs (4 hunks)
- lightning/src/util/ser.rs (2 hunks)
- lightning/src/util/test_utils.rs (12 hunks)
Files not summarized due to errors (1)
- fuzz/src/full_stack.rs: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (22)
- .github/workflows/build.yml
- ci/check-cfg-flags.py
- lightning/src/chain/chainmonitor.rs
- lightning/src/chain/channelmonitor.rs
- lightning/src/chain/onchaintx.rs
- lightning/src/events/bump_transaction.rs
- lightning/src/lib.rs
- lightning/src/ln/channel.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/functional_test_utils.rs
- lightning/src/ln/functional_tests.rs
- lightning/src/ln/monitor_tests.rs
- lightning/src/ln/outbound_payment.rs
- lightning/src/ln/peer_handler.rs
- lightning/src/onion_message/messenger.rs
- lightning/src/routing/gossip.rs
- lightning/src/routing/router.rs
- lightning/src/routing/utxo.rs
- lightning/src/sign/mod.rs
- lightning/src/util/config.rs
- lightning/src/util/ser.rs
- lightning/src/util/test_utils.rs
Additional comments: 15
lightning/src/util/indexed_map.rs (4)
- 37-37: Replacing
HashMap::new()withnew_hash_map()abstracts the specific hash map implementation used. This change enhances modularity and allows for easier adjustments to the hash map's behavior or characteristics globally. Ensure that thenew_hash_map()function is implemented correctly and efficiently to avoid any performance regressions.- 45-45: The change from
HashMap::with_capacity(capacity)tohash_map_with_capacity(capacity)is consistent with the abstraction strategy for hash map creation. This approach allows for pre-allocating space in the underlying data structure, potentially improving performance for known large datasets. Verify thathash_map_with_capacityis implemented to leverage the capacity hint effectively.- 179-179: The
VacantEntrystruct'sunderlying_entryfield type change from a specifichash_map::VacantEntryto a more abstractVacantHashMapEntryaligns with the overall goal of abstracting away from concrete hash map implementations. This change should facilitate easier swaps or modifications to the underlying hash map logic. Confirm thatVacantHashMapEntryprovides a compatible API and maintains the expected behavior.- 188-188: Similar to
VacantEntry, the modification inOccupiedEntryto useOccupiedHashMapEntryinstead of a specifichash_map::OccupiedEntrytype supports the abstraction of hash map details. This consistency in abstracting entry types is crucial for maintaining a uniform approach throughout the module. Ensure thatOccupiedHashMapEntryadequately mirrors the functionality and performance characteristics of the original entry type.lightning/src/sync/debug_sync.rs (3)
- 60-60: The initialization of
LOCKS_HELDwithnew_hash_map()instead ofHashMap::new()is consistent with the abstraction approach seen inindexed_map.rs. This change allows for a centralized way to modify the hash map behavior across the codebase. Ensure thatnew_hash_map()is optimized for the expected usage patterns indebug_sync.rs.- 116-116: Using
new_hash_map()for initializinglocked_beforewithinLockMetadata::new()maintains the abstraction layer for hash map creation. This consistency in using a custom function for hash map instantiation is crucial for modular and maintainable code. Verify that the performance and functionality ofnew_hash_map()meet the requirements for lock metadata management.- 125-125: The call to
new_hash_map()within theLOCKS_INITinitialization block further demonstrates the consistent application of the hash map abstraction strategy throughout thedebug_sync.rsfile. This approach simplifies potential future changes to the hash map implementation used by the locking mechanism. Confirm that the use ofnew_hash_map()here does not introduce any performance or functionality regressions.fuzz/src/full_stack.rs (2)
- 441-450: The changes made to the
do_testfunction, specifically the handling of theSecretKey::from_slicecall, correctly address the previously raised concern about ensuring the input data slice is of the correct length before attempting to create aSecretKey. This change prevents potential panics due to incorrect slice lengths, improving the robustness of the fuzz testing setup.- 441-451: The introduction of a conditional check for the length of
databefore attempting to extract aSecretKeyand the subsequent configuration parsing is a good practice for ensuring data integrity before processing. This approach enhances error handling by gracefully exiting the function if the data does not meet the expected criteria, thereby avoiding potential runtime errors.lightning/src/routing/scoring.rs (6)
- 653-659: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [656-698]
The change from
HashMap::new()tonew_hash_map()for initializingmanual_node_penaltiesinProbabilisticScoringFeeParametersandProbabilisticScoringDecayParametersis consistent with the summary provided. This standardizes the creation of hash maps across the codebase, potentially allowing for future optimizations or custom behavior in hash map creation.
- 712-712: The same change from
HashMap::new()tonew_hash_map()for initializingmanual_node_penaltiesin thezero_penaltymethod ofProbabilisticScoringFeeParametersis also consistent and aligns with the objective of standardizing hash map creation.- 822-822: The introduction of
new_hash_map()in theProbabilisticScorerconstructor for initializingchannel_liquiditiesaligns with the changes mentioned and the overall goal of standardizing hash map creation across the codebase.- 1333-1333: The use of
new_hash_map()for re-initializingmanual_node_penaltieswithin theProbabilisticScoringFeeParametersstruct when clearing manual penalties is consistent with the changes described and further supports the standardization effort.- 1363-1363: The use of
new_hash_map()for initializingchannel_liquiditiesin theProbabilisticScoreraligns with the described changes and the goal of standardizing hash map creation. This change ensures consistency across the codebase.- 2076-2076: The re-initialization of
channel_liquiditiesusingnew_hash_map()in the deserialization process ofProbabilisticScoreris consistent with the changes described. This ensures that the standardization of hash map creation is maintained even when objects are deserialized.
| } | ||
|
|
||
| #[inline] | ||
| pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) { | ||
| pub fn do_test(mut data: &[u8], logger: &Arc<dyn Logger>) { | ||
| if data.len() < 32 { return; } | ||
|
|
||
| let our_network_key = match SecretKey::from_slice(&data[..32]) { | ||
| Ok(key) => key, | ||
| Err(e) => return, | ||
| }; | ||
| data = &data[32..]; | ||
|
|
||
| let config: UserConfig = if let Ok(config) = Readable::read(&mut data) { config } else { return; }; | ||
|
|
||
| let input = Arc::new(InputData { | ||
| data: data.to_vec(), | ||
| read_pos: AtomicUsize::new(0), |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [441-1049]
The do_test function is quite lengthy and handles multiple responsibilities, from setting up the testing environment to processing various types of input data for fuzz testing. While the changes made within this function are appropriate and improve its functionality, consider refactoring it into smaller, more manageable functions. This could enhance readability, maintainability, and make it easier to identify and isolate specific behaviors for testing or further modification.
| @@ -14,7 +14,7 @@ use std::sync::Condvar as StdCondvar; | |||
|
|
|||
| pub use std::sync::WaitTimeoutResult; | |||
|
|
|||
| use crate::prelude::HashMap; | |||
| use crate::prelude::*; | |||
There was a problem hiding this comment.
The switch to a wildcard import from crate::prelude::* in debug_sync.rs mirrors the change observed in indexed_map.rs. While this can simplify imports by reducing verbosity, it also risks namespace pollution and obscures the origin of imported entities. Consider using explicit imports for better code clarity and to prevent potential conflicts.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- fuzz/src/peer_crypt.rs (1 hunks)
Additional comments: 2
fuzz/src/peer_crypt.rs (2)
- 86-86: The change from
buf.copy_from_slice(&get_slice!(len as usize + 16))tobuf[..len as usize + 16].copy_from_slice(&get_slice!(len as usize + 16))improves the clarity and precision of the slice operation by explicitly defining the slice range before copying. This modification ensures that only the intended portion ofbufis overwritten, which could prevent potential out-of-bounds errors or unintended data manipulation. This change aligns with best practices for handling slices in Rust, enhancing code safety and maintainability.- 83-89: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-92]
Overall, the file
peer_crypt.rsdemonstrates a careful approach to handling cryptographic operations and buffer management. The use of macros for slice access (get_slice!) and the structured error handling (early returns on errors) contribute to the code's robustness. However, it's crucial to ensure comprehensive testing, especially given the cryptographic context and the potential for subtle bugs. The use ofunsafeinpeer_crypt_runis justified by the need to interface with C code but requires careful attention to buffer bounds and data validity to prevent security vulnerabilities.
Because of txid conflicts, a txid can match both a local and remote commitment transaction when fuzzing. Thus, we disable this assertion when fuzzing.
As we've added more and more configuration parameters which change our behavior, we're increasingly missing coverage in the general `full_stack_target` fuzzer. Sadly, a reachable `unwrap` slipped in uncaught that should have been stopped by the `full_stack_target`. Here, we update the `full_stack_target` fuzzer to read a full config object to substantially increase coverage.
In the next commit we'll bump the `hashbrown` version, which no longer randomizes its hasher by default. Thus, we'll need to call a different constructor in no-std builds from std builds. Here we do a quick prefactor to use wrappers for constructors instead of calling the tables directly to make the version bump changeset smaller.
In the next commit we'll bump the `hashbrown` version, which no longer randomizes its hasher by default. Thus, we'll need to call a different constructor in no-std builds from std builds. Here we do a quick prefactor to use wrappers for `FromIterator` constructors instead of calling the tables directly to make the version bump changeset smaller.
While this isn't expected to materially improve performance, it does get us ahash 0.8, which allows us to reduce fuzzing randomness, making our fuzzers much happier. Sadly, by default `ahash` no longer tries to autodetect a randomness source, so we cannot simply rely on `hashbrown` to do randomization for us, but rather have to also explicitly depend on `ahash`.
This has apparently been broken for quite some time...I guess `peer_crypt_target` doesn't get much CPU anymore.
TheBlueMatt
force-pushed
the
2023-12-arbitrary-fuzz-config
branch
from
bd85396
to
7377cc9
Compare
|
Ugh, had to rebase again. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files ignored due to path filters (4)
bench/Cargo.tomlis excluded by:!**/*.tomlfuzz/Cargo.tomlis excluded by:!**/*.tomllightning-invoice/Cargo.tomlis excluded by:!**/*.tomllightning/Cargo.tomlis excluded by:!**/*.toml
Files selected for processing (27)
- .github/workflows/build.yml (3 hunks)
- ci/check-cfg-flags.py (1 hunks)
- fuzz/src/full_stack.rs (15 hunks)
- fuzz/src/peer_crypt.rs (1 hunks)
- lightning/src/chain/chainmonitor.rs (4 hunks)
- lightning/src/chain/channelmonitor.rs (13 hunks)
- lightning/src/chain/onchaintx.rs (6 hunks)
- lightning/src/events/bump_transaction.rs (1 hunks)
- lightning/src/lib.rs (1 hunks)
- lightning/src/ln/channel.rs (3 hunks)
- lightning/src/ln/channelmanager.rs (25 hunks)
- lightning/src/ln/functional_test_utils.rs (5 hunks)
- lightning/src/ln/functional_tests.rs (5 hunks)
- lightning/src/ln/monitor_tests.rs (1 hunks)
- lightning/src/ln/outbound_payment.rs (5 hunks)
- lightning/src/ln/peer_handler.rs (2 hunks)
- lightning/src/onion_message/messenger.rs (2 hunks)
- lightning/src/routing/gossip.rs (2 hunks)
- lightning/src/routing/router.rs (8 hunks)
- lightning/src/routing/scoring.rs (7 hunks)
- lightning/src/routing/utxo.rs (1 hunks)
- lightning/src/sign/mod.rs (1 hunks)
- lightning/src/sync/debug_sync.rs (4 hunks)
- lightning/src/util/config.rs (4 hunks)
- lightning/src/util/indexed_map.rs (4 hunks)
- lightning/src/util/ser.rs (2 hunks)
- lightning/src/util/test_utils.rs (12 hunks)
Files not summarized due to errors (1)
- fuzz/src/full_stack.rs: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (25)
- .github/workflows/build.yml
- ci/check-cfg-flags.py
- fuzz/src/peer_crypt.rs
- lightning/src/chain/chainmonitor.rs
- lightning/src/chain/channelmonitor.rs
- lightning/src/chain/onchaintx.rs
- lightning/src/events/bump_transaction.rs
- lightning/src/lib.rs
- lightning/src/ln/channel.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/functional_test_utils.rs
- lightning/src/ln/functional_tests.rs
- lightning/src/ln/monitor_tests.rs
- lightning/src/ln/outbound_payment.rs
- lightning/src/ln/peer_handler.rs
- lightning/src/onion_message/messenger.rs
- lightning/src/routing/gossip.rs
- lightning/src/routing/router.rs
- lightning/src/routing/scoring.rs
- lightning/src/routing/utxo.rs
- lightning/src/sign/mod.rs
- lightning/src/sync/debug_sync.rs
- lightning/src/util/config.rs
- lightning/src/util/ser.rs
- lightning/src/util/test_utils.rs
Additional comments: 4
lightning/src/util/indexed_map.rs (4)
- 37-37: The change to use
new_hash_map()for creating a newIndexedMapinstance is a good practice for abstracting the specific hash map implementation. This allows for easier changes in the future if the underlying hash map needs to be swapped or configured differently.- 45-45: Using
hash_map_with_capacity(capacity)for initializingIndexedMapwith a specific capacity is consistent with the abstraction goal. This ensures that the capacity optimizations are preserved while abstracting the underlying hash map implementation.- 179-179: The modification to use
VacantHashMapEntryinVacantEntrystruct abstracts the underlying vacant entry representation. This is a positive change towards making theIndexedMapmore flexible and less tied to a specific hash map implementation.- 188-188: Similarly, the change to use
OccupiedHashMapEntryinOccupiedEntrystruct aligns with the abstraction goals of the PR. It ensures that theIndexedMapcan be adapted more easily to different hash map implementations without significant refactoring.
| @@ -1,6 +1,6 @@ | |||
| //! This module has a map which can be iterated in a deterministic order. See the [`IndexedMap`]. | |||
|
|
|||
| use crate::prelude::{HashMap, hash_map}; | |||
| use crate::prelude::*; | |||
There was a problem hiding this comment.
The use of a wildcard import from crate::prelude::* introduces potential namespace pollution and makes it harder to identify where certain types or functions are coming from. Consider explicitly importing only the necessary entities to maintain clarity and avoid potential conflicts.
| pub fn do_test(mut data: &[u8], logger: &Arc<dyn Logger>) { | ||
| if data.len() < 32 { return; } | ||
|
|
||
| let our_network_key = match SecretKey::from_slice(&data[..32]) { | ||
| Ok(key) => key, | ||
| Err(e) => return, | ||
| }; | ||
| data = &data[32..]; | ||
|
|
||
| let config: UserConfig = if let Ok(config) = Readable::read(&mut data) { config } else { return; }; |
There was a problem hiding this comment.
The do_test function's initial check for data.len() < 32 is a good safeguard to ensure there's enough data for creating a SecretKey. However, this check should be accompanied by a log or error message for better debugging and understanding why the test might be exiting early in certain cases.
Consider adding a log statement before returning to provide insight into the early exit condition:
if data.len() < 32 {
logger.log("Data length is less than 32 bytes, exiting do_test early.");
return;
}
There was a problem hiding this comment.
This seems like it would require a massive factor increase to go through all the valid config/code path combinations?
There was a problem hiding this comment.
@arik-so, you've raised an important point about the complexity and computational cost associated with testing all valid configuration and code path combinations through fuzz testing. Fuzz testing is indeed a powerful method to automatically generate a wide range of inputs to explore different execution paths in the codebase, aiming to uncover bugs that manual testing might not detect.
The goal of fuzz testing is not necessarily to exhaustively test every single combination of configurations and code paths—such an approach would indeed require a massive increase in resources. Instead, fuzz testing focuses on maximizing code coverage and uncovering bugs with a feasible number of test cases. Techniques like code coverage analysis are often used alongside fuzz testing to identify untested paths, guiding the fuzzing process towards areas of the code that might need more attention.
In practice, this means that while we strive to cover as many scenarios as possible, we also prioritize practicality and efficiency, ensuring that the testing process remains manageable and focused on areas most likely to contain bugs. This balance allows us to explore a broad range of configurations and code paths without the need for an exhaustive and potentially infeasible testing effort.
| }; | ||
| data = &data[32..]; | ||
|
|
||
| let config: UserConfig = if let Ok(config) = Readable::read(&mut data) { config } else { return; }; |
There was a problem hiding this comment.
The configuration object deserialization from data using Readable::read(&mut data) without a subsequent length check before proceeding with further operations could potentially lead to unexpected behavior if data does not contain enough bytes for the operations that follow.
It's recommended to add a check after deserializing config to ensure data has enough remaining bytes for subsequent operations, similar to the initial length check:
if data.len() < expected_minimum_length {
logger.log("Not enough data remaining after deserializing config, exiting do_test early.");
return;
}Note: expected_minimum_length should be determined based on the minimum expected length of data needed for the operations following the config deserialization.
| @@ -782,8 +784,7 @@ | |||
| // Writing new code generating transactions and see a new failure ? Don't forget to add input for the FuzzEstimator ! | |||
|
|
|||
| // 0100000000000000000000000000000000000000000000000000000000000000 - our network key | |||
| // 00000000 - fee_proportional_millionths | |||
| // 01 - announce_channels_publicly | |||
| // 0000000000900000000000000000640001000000000001ffff0000000000000000ffffffffffffffffffffffffffffffff0000000000000000ffffffffffffffff000000ffffffff00ffff1a000400010000020400000000040200000a08ffffffffffffffff0001000000 - config | |||
There was a problem hiding this comment.
The comment explaining the configuration object's hex representation is helpful for understanding the test setup. However, it's placed far from the actual usage of the configuration object, making it less effective for readers trying to understand the code.
Consider moving this comment closer to where the configuration object is actually used or deserialized to improve readability and comprehension.
| @@ -793,7 +794,7 @@ | |||
| // 030012 - inbound read from peer id 0 of len 18 | |||
| // 0010 03000000000000000000000000000000 - message header indicating message length 16 | |||
| // 030020 - inbound read from peer id 0 of len 32 | |||
| // 0010 00021aaa 0008aaaaaaaaaaaa9aaa 03000000000000000000000000000000 - init message (type 16) with static_remotekey required and other bits optional and mac | |||
| // 0010 00021aaa 0008aaa20aaa2a0a9aaa 03000000000000000000000000000000 - init message (type 16) with static_remotekey required, no channel_type/anchors/taproot, and other bits optional and mac | |||
There was a problem hiding this comment.
The detailed breakdown of the message handling process, including the noise act and message types, is valuable for understanding the test flow. However, this information is somewhat disconnected from the actual code execution, making it challenging to follow.
To improve clarity, consider adding inline comments within the code at relevant points to directly associate these explanations with the corresponding operations.
| @@ -841,7 +842,7 @@ | |||
| // 030112 - inbound read from peer id 1 of len 18 | |||
| // 0010 01000000000000000000000000000000 - message header indicating message length 16 | |||
| // 030120 - inbound read from peer id 1 of len 32 | |||
| // 0010 00021aaa 0008aaaaaaaaaaaa9aaa 01000000000000000000000000000000 - init message (type 16) with static_remotekey required and other bits optional and mac | |||
| // 0010 00021aaa 0008aaa20aaa2a0a9aaa 01000000000000000000000000000000 - init message (type 16) with static_remotekey required, no channel_type/anchors/taproot, and other bits optional and mac | |||
There was a problem hiding this comment.
The explanation of the test sequence involving peer connections and channel creation is crucial for understanding the test's purpose. However, the detailed hex values and operations are described in a comment block far from their actual execution points in the code.
Incorporate brief comments within the code to map these operations to their descriptions in the comment block, enhancing readability and traceability of the test sequence.
| @@ -1059,7 +1046,7 @@ | |||
| // - client now fails the HTLC backwards as it was unable to extract the payment preimage (CHECK 9 duplicate and CHECK 10) | |||
|
|
|||
| let logger = Arc::new(TrackingLogger { lines: Mutex::new(HashMap::new()) }); | |||
| super::do_test(&<Vec<u8>>::from_hex("01000000000000000000000000000000000000000000000000000000000000000000000001000300000000000000000000000000000000000000000000000000000000000000020300320003000000000000000000000000000000000000000000000000000000000000000203000000000000000000000000000000030012001003000000000000000000000000000000030020001000021aaa0008aaaaaaaaaaaa9aaa030000000000000000000000000000000300120147030000000000000000000000000000000300fe00206fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000ff4f00f805273c1b203bb5ebf8436bfde57b3be8c2f5e95d9491dbb181909679000000000000c35000000000000000000000000000000162ffffffffffffffff00000000000002220000000000000000000000fd000601e3030000000000000000000000000000000000000000000000000000000000000001030000000000000000000000000000000000000000000000000000000000000002030000000000000000000000000000000000000000000000000000000000000003030000000000000000000000000000000000000000000000000000000000000004030059030000000000000000000000000000000000000000000000000000000000000005020900000000000000000000000000000000000000000000000000000000000000010000010210000300000000000000000000000000000000fd0300120084030000000000000000000000000000000300940022ff4f00f805273c1b203bb5ebf8436bfde57b3be8c2f5e95d9491dbb1819096793d00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000210100000000000000000000000000000000000000000000000000000000000000030000000000000000000000000000000c005e020000000100000000000000000000000000000000000000000000000000000000000000000000000000ffffffff0150c3000000000000220020ae00000000000000000000000000000000000000000000000000000000000000000000000c00000c00000c00000c00000c00000c00000c00000c00000c00000c00000c00000c000003001200430300000000000000000000000000000003005300243d0000000000000000000000000000000000000000000000000000000000000002080000000000000000000000000000000000000000000000000000000000000003000000000000000000000000000000010301320003000000000000000000000000000000000000000000000000000000000000000703000000000000000000000000000000030142000302000000000000000000000000000000000000000000000000000000000000000300000000000000000000000000000003000000000000000000000000000000030112001001000000000000000000000000000000030120001000021aaa0008aaaaaaaaaaaa9aaa01000000000000000000000000000000050103020000000000000000000000000000000000000000000000000000000000000000c3500003e800fd0301120112010000000000000000000000000000000301ff00210000000000000000000000000000000000000000000000000000000000000e05000000000000016200000000004c4b4000000000000003e800000000000003e80000000203f000050300000000000000000000000000000000000000000000000000000000000001000300000000000000000000000000000000000000000000000000000000000002000300000000000000000000000000000000000000000000000000000000000003000300000000000000000000000000000000000000000000000000000000000004000300000000000000000000000000000000000000000000000000000000000005000266000000000000000000000000000003012300000000000000000000000000000000000000010000000000000000000000000000000a00fd00fd03011200620100000000000000000000000000000003017200233a00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000007c0001000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000b03011200430100000000000000000000000000000003015300243a000000000000000000000000000000000000000000000000000000000000000267000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000003001205ac030000000000000000000000000000000300ff00803d0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000003e80ff00000000000000000000000000000000000000000000000000000000000000000003f00003000000000000000000000000000000000000000000000000000000000000055511020203e80401a0060800000e00000100000a00000000000000000000000000000000000000000000000000000000000000ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300c1ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffab000000000000000000000000000000000000000000000000000000000000000300000000000000000000000000000000fd03001200640300000000000000000000000000000003007400843d000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000030010000000000000000000000000000000000000000000000000000000000000000000300000000000000000000000000000003001200630300000000000000000000000000000003007300853d000000000000000000000000000000000000000000000000000000000000000900000000000000000000000000000000000000000000000000000000000000020b00000000000000000000000000000000000000000000000000000000000000030000000000000000000000000000000700fd00fd03011200640100000000000000000000000000000003017400843a00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000006a000100000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000003011200630100000000000000000000000000000003017300853a00000000000000000000000000000000000000000000000000000000000000660000000000000000000000000000000000000000000000000000000000000002640000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000030112004a0100000000000000000000000000000003015a00823a000000000000000000000000000000000000000000000000000000000000000000000000000000ff008888888888888888888888888888888888888888888888888888888888880100000000000000000000000000000003011200640100000000000000000000000000000003017400843a000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000010000100000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000003011200630100000000000000000000000000000003017300853a0000000000000000000000000000000000000000000000000000000000000067000000000000000000000000000000000000000000000000000000000000000265000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000003001205ac030000000000000000000000000000000300ff00803d0000000000000000000000000000000000000000000000000000000000000000000000000000010000000000003e80ff00000000000000000000000000000000000000000000000000000000000000000003f00003000000000000000000000000000000000000000000000000000000000000055511020203e80401a0060800000e00000100000a00000000000000000000000000000000000000000000000000000000000000ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300c1ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffab000000000000000000000000000000000000000000000000000000000000000300000000000000000000000000000000fd03001200630300000000000000000000000000000003007300853d000000000000000000000000000000000000000000000000000000000000000800000000000000000000000000000000000000000000000000000000000000020a000000000000000000000000000000000000000000000000000000000000000300000000000000000000000000000003001200640300000000000000000000000000000003007400843d0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000c3010000000000000000000000000000000000000000000000000000000000000000000300000000000000000000000000000003001200630300000000000000000000000000000003007300853d000000000000000000000000000000000000000000000000000000000000000b00000000000000000000000000000000000000000000000000000000000000020d00000000000000000000000000000000000000000000000000000000000000030000000000000000000000000000000700fd00fd03011200640100000000000000000000000000000003017400843a000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000039000100000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000003011200630100000000000000000000000000000003017300853a00000000000000000000000000000000000000000000000000000000000000640000000000000000000000000000000000000000000000000000000000000002700000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000030112002c0100000000000000000000000000000003013c00833a00000000000000000000000000000000000000000000000000000000000000000000000000000100000100000000000000000000000000000003011200640100000000000000000000000000000003017400843a000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000039000100000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000003011200630100000000000000000000000000000003017300853a000000000000000000000000000000000000000000000000000000000000006500000000000000000000000000000000000000000000000000000000000000027100000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000703001200630300000000000000000000000000000003007300853d000000000000000000000000000000000000000000000000000000000000000a00000000000000000000000000000000000000000000000000000000000000020c000000000000000000000000000000000000000000000000000000000000000300000000000000000000000000000003001200640300000000000000000000000000000003007400843d000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000032010000000000000000000000000000000000000000000000000000000000000000000300000000000000000000000000000003001205ac030000000000000000000000000000000300ff00803d00000000000000000000000000000000000000000000000000000000000000000000000000000200000000000b0838ff00000000000000000000000000000000000000000000000000000000000000000003f0000300000000000000000000000000000000000000000000000000000000000005551202030927c00401a0060800000e00000100000a00000000000000000000000000000000000000000000000000000000000000ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300c1ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff53000000000000000000000000000000000000000000000000000000000000000300000000000000000000000000000000fd03001200a4030000000000000000000000000000000300b400843d00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000007501000000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000006705000000000000000000000000000000000000000000000000000000000000060300000000000000000000000000000003001200630300000000000000000000000000000003007300853d000000000000000000000000000000000000000000000000000000000000000d00000000000000000000000000000000000000000000000000000000000000020f00000000000000000000000000000000000000000000000000000000000000030000000000000000000000000000000700fd00fd0c007d02000000013a000000000000000000000000000000000000000000000000000000000000000000000000000000800258020000000000002200204b0000000000000000000000000000000000000000000000000000000000000014c00000000000001600142800000000000000000000000000000000000000050000200c005e0200000001730000000000000000000000000000000000000000000000000000000000000000000000000000000001a701000000000000220020b200000000000000000000000000000000000000000000000000000000000000000000000c00000c00000c00000c00000c000007").unwrap(), &(Arc::clone(&logger) as Arc<dyn Logger>)); | |||
| super::do_test(&<Vec<u8>>::from_hex("01000000000000000000000000000000000000000000000000000000000000000000000000900000000000000000640001000000000001ffff0000000000000000ffffffffffffffffffffffffffffffff0000000000000000ffffffffffffffff000000ffffffff00ffff1a000400010000020400000000040200000a08ffffffffffffffff0001000000000300000000000000000000000000000000000000000000000000000000000000020300320003000000000000000000000000000000000000000000000000000000000000000203000000000000000000000000000000030012001003000000000000000000000000000000030020001000021aaa0008aaa20aaa2a0a9aaa030000000000000000000000000000000300120147030000000000000000000000000000000300fe00206fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000ff4f00f805273c1b203bb5ebf8436bfde57b3be8c2f5e95d9491dbb181909679000000000000c35000000000000000000000000000000162ffffffffffffffff00000000000002220000000000000000000000fd000601e3030000000000000000000000000000000000000000000000000000000000000001030000000000000000000000000000000000000000000000000000000000000002030000000000000000000000000000000000000000000000000000000000000003030000000000000000000000000000000000000000000000000000000000000004030059030000000000000000000000000000000000000000000000000000000000000005020900000000000000000000000000000000000000000000000000000000000000010000010210000300000000000000000000000000000000fd0300120084030000000000000000000000000000000300940022ff4f00f805273c1b203bb5ebf8436bfde57b3be8c2f5e95d9491dbb1819096793d00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000210100000000000000000000000000000000000000000000000000000000000000030000000000000000000000000000000c005e020000000100000000000000000000000000000000000000000000000000000000000000000000000000ffffffff0150c3000000000000220020ae00000000000000000000000000000000000000000000000000000000000000000000000c00000c00000c00000c00000c00000c00000c00000c00000c00000c00000c00000c000003001200430300000000000000000000000000000003005300243d0000000000000000000000000000000000000000000000000000000000000002080000000000000000000000000000000000000000000000000000000000000003000000000000000000000000000000010301320003000000000000000000000000000000000000000000000000000000000000000703000000000000000000000000000000030142000302000000000000000000000000000000000000000000000000000000000000000300000000000000000000000000000003000000000000000000000000000000030112001001000000000000000000000000000000030120001000021aaa0008aaa20aaa2a0a9aaa01000000000000000000000000000000050103020000000000000000000000000000000000000000000000000000000000000000c3500003e800fd0301120112010000000000000000000000000000000301ff00210000000000000000000000000000000000000000000000000000000000000e05000000000000016200000000004c4b4000000000000003e800000000000003e80000000203f000050300000000000000000000000000000000000000000000000000000000000001000300000000000000000000000000000000000000000000000000000000000002000300000000000000000000000000000000000000000000000000000000000003000300000000000000000000000000000000000000000000000000000000000004000300000000000000000000000000000000000000000000000000000000000005000266000000000000000000000000000003012300000000000000000000000000000000000000010000000000000000000000000000000a03011200620100000000000000000000000000000003017200233a00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000007c0001000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000b03011200430100000000000000000000000000000003015300243a000000000000000000000000000000000000000000000000000000000000000267000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000003001205ac030000000000000000000000000000000300ff00803d0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000003e80ff00000000000000000000000000000000000000000000000000000000000000000003f00003000000000000000000000000000000000000000000000000000000000000055511020203e80401a0060800000e00000100000a00000000000000000000000000000000000000000000000000000000000000ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300c1ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffab000000000000000000000000000000000000000000000000000000000000000300000000000000000000000000000003001200640300000000000000000000000000000003007400843d000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000030010000000000000000000000000000000000000000000000000000000000000000000300000000000000000000000000000003001200630300000000000000000000000000000003007300853d000000000000000000000000000000000000000000000000000000000000000900000000000000000000000000000000000000000000000000000000000000020b00000000000000000000000000000000000000000000000000000000000000030000000000000000000000000000000703011200640100000000000000000000000000000003017400843a00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000006a000100000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000003011200630100000000000000000000000000000003017300853a00000000000000000000000000000000000000000000000000000000000000660000000000000000000000000000000000000000000000000000000000000002640000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000030112004a0100000000000000000000000000000003015a00823a000000000000000000000000000000000000000000000000000000000000000000000000000000ff008888888888888888888888888888888888888888888888888888888888880100000000000000000000000000000003011200640100000000000000000000000000000003017400843a000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000010000100000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000003011200630100000000000000000000000000000003017300853a0000000000000000000000000000000000000000000000000000000000000067000000000000000000000000000000000000000000000000000000000000000265000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000003001205ac030000000000000000000000000000000300ff00803d0000000000000000000000000000000000000000000000000000000000000000000000000000010000000000003e80ff00000000000000000000000000000000000000000000000000000000000000000003f00003000000000000000000000000000000000000000000000000000000000000055511020203e80401a0060800000e00000100000a00000000000000000000000000000000000000000000000000000000000000ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300c1ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffab000000000000000000000000000000000000000000000000000000000000000300000000000000000000000000000003001200630300000000000000000000000000000003007300853d000000000000000000000000000000000000000000000000000000000000000800000000000000000000000000000000000000000000000000000000000000020a000000000000000000000000000000000000000000000000000000000000000300000000000000000000000000000003001200640300000000000000000000000000000003007400843d0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000c3010000000000000000000000000000000000000000000000000000000000000000000300000000000000000000000000000003001200630300000000000000000000000000000003007300853d000000000000000000000000000000000000000000000000000000000000000b00000000000000000000000000000000000000000000000000000000000000020d00000000000000000000000000000000000000000000000000000000000000030000000000000000000000000000000703011200640100000000000000000000000000000003017400843a000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000039000100000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000003011200630100000000000000000000000000000003017300853a00000000000000000000000000000000000000000000000000000000000000640000000000000000000000000000000000000000000000000000000000000002700000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000030112002c0100000000000000000000000000000003013c00833a00000000000000000000000000000000000000000000000000000000000000000000000000000100000100000000000000000000000000000003011200640100000000000000000000000000000003017400843a000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000039000100000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000003011200630100000000000000000000000000000003017300853a000000000000000000000000000000000000000000000000000000000000006500000000000000000000000000000000000000000000000000000000000000027100000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000703001200630300000000000000000000000000000003007300853d000000000000000000000000000000000000000000000000000000000000000a00000000000000000000000000000000000000000000000000000000000000020c000000000000000000000000000000000000000000000000000000000000000300000000000000000000000000000003001200640300000000000000000000000000000003007400843d000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000032010000000000000000000000000000000000000000000000000000000000000000000300000000000000000000000000000003001205ac030000000000000000000000000000000300ff00803d00000000000000000000000000000000000000000000000000000000000000000000000000000200000000000b0838ff00000000000000000000000000000000000000000000000000000000000000000003f0000300000000000000000000000000000000000000000000000000000000000005551202030927c00401a0060800000e00000100000a00000000000000000000000000000000000000000000000000000000000000ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff0300c1ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff53000000000000000000000000000000000000000000000000000000000000000300000000000000000000000000000003001200a4030000000000000000000000000000000300b400843d00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000007501000000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000006705000000000000000000000000000000000000000000000000000000000000060300000000000000000000000000000003001200630300000000000000000000000000000003007300853d000000000000000000000000000000000000000000000000000000000000000d00000000000000000000000000000000000000000000000000000000000000020f0000000000000000000000000000000000000000000000000000000000000003000000000000000000000000000000070c007d02000000013a000000000000000000000000000000000000000000000000000000000000000000000000000000800258020000000000002200204b0000000000000000000000000000000000000000000000000000000000000014c00000000000001600142800000000000000000000000000000000000000050000200c005e0200000001730000000000000000000000000000000000000000000000000000000000000000000000000000000001a701000000000000220020b200000000000000000000000000000000000000000000000000000000000000000000000c00000c00000c00000c00000c000007").unwrap(), &(Arc::clone(&logger) as Arc<dyn Logger>)); | |||
There was a problem hiding this comment.
The test function test_no_existing_test_breakage is well-documented with comments explaining each step of the test sequence. This is good practice for complex tests. However, there's a missed opportunity to use assertions more effectively to validate intermediate states and not just the final outcomes.
Consider adding assertions throughout the test to validate the state after each significant operation, such as after sending or receiving messages, to ensure the test's behavior is as expected at each step.
| @@ -3172,7 +3172,11 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> { | |||
| (htlc, htlc_source.as_ref().map(|htlc_source| htlc_source.as_ref())) | |||
| ), logger); | |||
| } else { | |||
| debug_assert!(false, "We should have per-commitment option for any recognized old commitment txn"); | |||
| // Our fuzzers aren't contrained by pesky things like valid signatures, so can | |||
| pub fn do_test(mut data: &[u8], logger: &Arc<dyn Logger>) { | ||
| if data.len() < 32 { return; } | ||
|
|
||
| let our_network_key = match SecretKey::from_slice(&data[..32]) { | ||
| Ok(key) => key, | ||
| Err(e) => return, | ||
| }; | ||
| data = &data[32..]; | ||
|
|
||
| let config: UserConfig = if let Ok(config) = Readable::read(&mut data) { config } else { return; }; |
There was a problem hiding this comment.
This seems like it would require a massive factor increase to go through all the valid config/code path combinations?
| @@ -5402,11 +5402,11 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno | |||
|
|
|||
| let as_events = nodes[0].node.get_and_clear_pending_events(); | |||
| assert_eq!(as_events.len(), if announce_latest { 10 } else { 6 }); | |||
| let mut as_failds = new_hash_set(); | |||
| let mut as_faileds = new_hash_set(); | |||
There was a problem hiding this comment.
if you're already doing a typo fix, might as well call them a_failures or, better yet (imo), failures_a.
| @@ -83,7 +83,7 @@ pub fn do_test(data: &[u8]) { | |||
| Ok(len) => len, | |||
| Err(_) => return, | |||
| }; | |||
| buf.copy_from_slice(&get_slice!(len as usize + 16)); | |||
| buf[..len as usize + 16].copy_from_slice(&get_slice!(len as usize + 16)); | |||
There was a problem hiding this comment.
+ 16 might benefit from a comment?
There was a problem hiding this comment.
Meh, its a fuzzer, it'll complain if its wrong :)
|
Gonna go ahead and land this, though its gonna have silent merge conflicts I'll fix immediately with the spelling comment above. |
v0.0.123 - May 08, 2024 - "BOLT12 Dust Sweeping" API Updates =========== * To reduce risk of force-closures and improve HTLC reliability the default dust exposure limit has been increased to `MaxDustHTLCExposure::FeeRateMultiplier(10_000)`. Users with existing channels might want to consider using `ChannelManager::update_channel_config` to apply the new default (lightningdevkit#3045). * `ChainMonitor::archive_fully_resolved_channel_monitors` is now provided to remove from memory `ChannelMonitor`s that have been fully resolved on-chain and are now not needed. It uses the new `Persist::archive_persisted_channel` to inform the storage layer that such a monitor should be archived (lightningdevkit#2964). * An `OutputSweeper` is now provided which will automatically sweep `SpendableOutputDescriptor`s, retrying until the sweep confirms (lightningdevkit#2825). * After initiating an outbound channel, a peer disconnection no longer results in immediate channel closure. Rather, if the peer is reconnected before the channel times out LDK will automatically retry opening it (lightningdevkit#2725). * `PaymentPurpose` now has separate variants for BOLT12 payments, which include fields from the `invoice_request` as well as the `OfferId` (lightningdevkit#2970). * `ChannelDetails` now includes a list of in-flight HTLCs (lightningdevkit#2442). * `Event::PaymentForwarded` now includes `skimmed_fee_msat` (lightningdevkit#2858). * The `hashbrown` dependency has been upgraded and the use of `ahash` as the no-std hash table hash function has been removed. As a consequence, LDK's `Hash{Map,Set}`s no longer feature several constructors when LDK is built with no-std; see the `util::hash_tables` module instead. On platforms that `getrandom` supports, setting the `possiblyrandom/getrandom` feature flag will ensure hash tables are resistant to HashDoS attacks, though the `possiblyrandom` crate should detect most common platforms (lightningdevkit#2810, lightningdevkit#2891). * `ChannelMonitor`-originated requests to the `ChannelSigner` can now fail and be retried using `ChannelMonitor::signer_unblocked` (lightningdevkit#2816). * `SpendableOutputDescriptor::to_psbt_input` now includes the `witness_script` where available as well as new proprietary data which can be used to re-derive some spending keys from the base key (lightningdevkit#2761, lightningdevkit#3004). * `OutPoint::to_channel_id` has been removed in favor of `ChannelId::v1_from_funding_outpoint` in preparation for v2 channels with a different `ChannelId` derivation scheme (lightningdevkit#2797). * `PeerManager::get_peer_node_ids` has been replaced with `list_peers` and `peer_by_node_id`, which provide more details (lightningdevkit#2905). * `Bolt11Invoice::get_payee_pub_key` is now provided (lightningdevkit#2909). * `Default[Message]Router` now take an `entropy_source` argument (lightningdevkit#2847). * `ClosureReason::HTLCsTimedOut` has been separated out from `ClosureReason::HolderForceClosed` as it is the most common case (lightningdevkit#2887). * `ClosureReason::CooperativeClosure` is now split into `{Counterparty,Locally}Initiated` variants (lightningdevkit#2863). * `Event::ChannelPending::channel_type` is now provided (lightningdevkit#2872). * `PaymentForwarded::{prev,next}_user_channel_id` are now provided (lightningdevkit#2924). * Channel init messages have been refactored towards V2 channels (lightningdevkit#2871). * `BumpTransactionEvent` now contains the channel and counterparty (lightningdevkit#2873). * `util::scid_utils` is now public, with some trivial utilities to examine short channel ids (lightningdevkit#2694). * `DirectedChannelInfo::{source,target}` are now public (lightningdevkit#2870). * Bounds in `lightning-background-processor` were simplified by using `AChannelManager` (lightningdevkit#2963). * The `Persist` impl for `KVStore` no longer requires `Sized`, allowing for the use of `dyn KVStore` as `Persist` (lightningdevkit#2883, lightningdevkit#2976). * `From<PaymentPreimage>` is now implemented for `PaymentHash` (lightningdevkit#2918). * `NodeId::from_slice` is now provided (lightningdevkit#2942). * `ChannelManager` deserialization may now fail with `DangerousValue` when LDK's persistence API was violated (lightningdevkit#2974). Bug Fixes ========= * Excess fees on counterparty commitment transactions are now included in the dust exposure calculation. This lines behavior up with some cases where transaction fees can be burnt, making them effectively dust exposure (lightningdevkit#3045). * `Future`s used as an `std::...::Future` could grow in size unbounded if it was never woken. For those not using async persistence and using the async `lightning-background-processor`, this could cause a memory leak in the `ChainMonitor` (lightningdevkit#2894). * Inbound channel requests that fail in `ChannelManager::accept_inbound_channel` would previously have stalled from the peer's perspective as no `error` message was sent (lightningdevkit#2953). * Blinded path construction has been tuned to select paths more likely to succeed, improving BOLT12 payment reliability (lightningdevkit#2911, lightningdevkit#2912). * After a reorg, `lightning-transaction-sync` could have failed to follow a transaction that LDK needed information about (lightningdevkit#2946). * `RecipientOnionFields`' `custom_tlvs` are now propagated to recipients when paying with blinded paths (lightningdevkit#2975). * `Event::ChannelClosed` is now properly generated and peers are properly notified for all channels that as a part of a batch channel open fail to be funded (lightningdevkit#3029). * In cases where user event processing is substantially delayed such that we complete multiple round-trips with our peers before a `PaymentSent` event is handled and then restart without persisting the `ChannelManager` after having persisted a `ChannelMonitor[Update]`, on startup we may have `Err`d trying to deserialize the `ChannelManager` (lightningdevkit#3021). * If a peer has relatively high latency, `PeerManager` may have failed to establish a connection (lightningdevkit#2993). * `ChannelUpdate` messages broadcasted for our own channel closures are now slightly more robust (lightningdevkit#2731). * Deserializing malformed BOLT11 invoices may have resulted in an integer overflow panic in debug builds (lightningdevkit#3032). * In exceedingly rare cases (no cases of this are known), LDK may have created an invalid serialization for a `ChannelManager` (lightningdevkit#2998). * Message processing latency handling BOLT12 payments has been reduced (lightningdevkit#2881). * Latency in processing `Event::SpendableOutputs` may be reduced (lightningdevkit#3033). Node Compatibility ================== * LDK's blinded paths were inconsistent with other implementations in several ways, which have been addressed (lightningdevkit#2856, lightningdevkit#2936, lightningdevkit#2945). * LDK's messaging blinded paths now support the latest features which some nodes may begin relying on soon (lightningdevkit#2961). * LDK's BOLT12 structs have been updated to support some last-minute changes to the spec (lightningdevkit#3017, lightningdevkit#3018). * CLN v24.02 requires the `gossip_queries` feature for all peers, however LDK by default does not set it for those not using a `P2PGossipSync` (e.g. those using RGS). This change was reverted in CLN v24.02.2 however for now LDK always sets the `gossip_queries` feature. This change is expected to be reverted in a future LDK release (lightningdevkit#2959). Security ======== 0.0.123 fixes a denial-of-service vulnerability which we believe to be reachable from untrusted input when parsing invalid BOLT11 invoices containing non-ASCII characters. * BOLT11 invoices with non-ASCII characters in the human-readable-part may cause an out-of-bounds read attempt leading to a panic (lightningdevkit#3054). Note that all BOLT11 invoices containing non-ASCII characters are invalid. In total, this release features 150 files changed, 19307 insertions, 6306 deletions in 360 commits since 0.0.121 from 17 authors, in alphabetical order: * Arik Sosman * Duncan Dean * Elias Rohrer * Evan Feenstra * Jeffrey Czyz * Keyue Bao * Matt Corallo * Orbital * Sergi Delgado Segura * Valentine Wallace * Willem Van Lint * Wilmer Paulino * benthecarman * jbesraa * olegkubrakov * optout * shaavan
Built on #2808 and #2809, this updates thefull_stack_targetto read in an arbitrary config object, which ultimately was able to reproduce #2804 rather trivially.