Refactor BestBlock to expose inner fields
#2923
Conversation
|
Note Reviews PausedUse the following commands to manage reviews:
WalkthroughThe recent updates involve a significant simplification in how 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 pause |
![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: 14
Configuration used: CodeRabbit UI
Files selected for processing (7)
- lightning-background-processor/src/lib.rs (1 hunks)
- lightning/src/chain/channelmonitor.rs (14 hunks)
- lightning/src/chain/mod.rs (2 hunks)
- lightning/src/ln/channel.rs (2 hunks)
- lightning/src/ln/channelmanager.rs (38 hunks)
- lightning/src/ln/functional_tests.rs (7 hunks)
- lightning/src/ln/onion_route_tests.rs (3 hunks)
Additional comments: 61
lightning/src/chain/mod.rs (2)
- 34-39: The changes to the
BestBlockstruct make theblock_hashandheightfields publicly accessible and derive theDebugtrait. This aligns with Rust's idiomatic practices for simplicity and directness, as mentioned in the PR objectives. The documentation comments added to the fields are also a good practice for clarity.However, it's important to ensure that making these fields public does not inadvertently allow for their modification in contexts where it might lead to inconsistencies or bugs. Given Rust's ownership and borrowing rules, direct field access is generally safe, but careful consideration should be given to the broader implications in the project's context.
- 34-39: The removal of getter methods for
block_hashandheightfrom theBestBlockstruct, as mentioned in the AI-generated summary, further simplifies the codebase by reducing unnecessary abstraction. This change should be carefully reviewed in the context of the entire project to ensure it does not introduce any unintended side effects, especially in parts of the code that previously relied on these methods. It's also important to update any documentation or examples that reference the old methods to prevent confusion.lightning/src/ln/onion_route_tests.rs (5)
- 1317-1323: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [132-132]
The test
test_fee_failuresuses a hardcoded value forforwarding_fee_base_msat. Consider using a constant or a configuration parameter to make this value more flexible and clear.
- 1317-1323: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [132-132]
The test
test_onion_failureuses a hardcoded value forour_htlc_minimum_msatin the node configuration. Consider defining this value as a constant or retrieving it from a configuration object for better clarity and maintainability.
- 1317-1323: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [132-132]
In
test_overshoot_final_cltv, consider adding comments to explain the significance of the CLTV values chosen for the test, especially why specific values are used to simulate the overshoot scenario.
- 1317-1323: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [132-132]
The function
do_test_fail_htlc_backwards_with_reasonuses a macro to generate tests for different failure reasons. Ensure that all relevant failure reasons are covered by the tests and consider adding comments to explain each failure scenario for clarity.
- 1317-1323: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [132-132]
In
test_phantom_onion_hmac_failure, consider adding more detailed comments to explain the steps involved in simulating the HMAC failure, especially how the onion packet is manipulated to induce the failure.lightning/src/chain/channelmonitor.rs (14)
- 390-390: The change to directly access the
heightfield of theBestBlockstruct aligns with Rust's idiomatic practices and the PR's objectives. Looks good!- 1080-1081: Directly accessing the
block_hashandheightfields for serialization purposes is efficient and simplifies the code. The use ofto_be_bytes()forheightis appropriate for ensuring the correct byte order.- 2276-2276: Directly accessing the
heightfield in this conditional check is appropriate and aligns with the PR's objectives. The logic remains correct.- 2677-2677: Updating the function call to use the directly accessed
heightfield simplifies the code and aligns with the PR's objectives. Correct usage.- 2724-2725: The updates to function calls using directly accessed fields maintain correctness and simplify the code. Consistent with the PR's objectives.
- 2739-2739: Using directly accessed fields for
PackageTemplateconstruction is efficient and simplifies the code. Correct and consistent with the PR's objectives.- 2756-2756: Updating the function call to use the directly accessed
heightfield simplifies the code and maintains correctness. Consistent with the PR's objectives.- 2780-2780: The updates to function calls using directly accessed fields maintain correctness and simplify the code. Consistent with the PR's objectives.
- 3596-3600: The logic for updating the
best_blockfield based on new block information is correct and efficient. Direct field access for comparison and assignment is appropriate and aligns with the PR's objectives.- 3745-3745: The logic for updating the
best_blockfield based on height comparison is correct. Direct field access for comparison and assignment is appropriate and aligns with the PR's objectives.- 3777-3777: The use of
debug_assert!to ensurebest_block.heightis not less thanconf_heightis appropriate and helps maintain invariants in debug builds. Correct usage.- 3868-3869: The updates to function calls using directly accessed fields and a provided
conf_heightmaintain correctness and simplify the code. Consistent with the PR's objectives.- 4020-4020: Introducing a macro that uses the directly accessed
heightfield simplifies repetitive logic and maintains correctness. Efficient and consistent with the PR's objectives.- 4619-4619: Constructing a
ChannelMonitorwith thebest_blockfield is correct and efficient. The logic aligns with the PR's objectives and simplifies the code.lightning/src/ln/channel.rs (2)
- 4089-4089: The change from
best_block.height()tobest_block.heightcorrectly aligns with the PR's objective to makeheightdirectly accessible. This simplifies the code and adheres to Rust's idiomatic practices of direct field access for publicly accessible fields. Ensure that theBestBlockstruct'sheightfield is indeed public and that all instances ofbest_block.height()have been updated across the codebase to prevent inconsistencies.- 5478-5478: Similar to the previous comment, this change from
best_block.height()tobest_block.heightis consistent with the PR's objective and Rust's idiomatic practices. It's important to verify that theheightfield is accessible where this change is made and to ensure that there are no remaining calls to the old getter method that might lead to compilation errors or inconsistencies within the project.lightning/src/ln/channelmanager.rs (38)
- 2212-2212: Direct access to the
heightfield ofbest_blockis correctly implemented, aligning with Rust's idiomatic practices. However, consider adding error handling for lock acquisition to gracefully handle potential lock poisoning.- 2512-2512: Direct access to the
heightfield is correctly implemented. Remember to consider error handling for lock acquisition.- 2590-2590: Direct access to the
heightfield is correctly implemented. Error handling for lock acquisition should be considered.- 2629-2629: Direct access to the
heightfield is correctly implemented. Consider error handling for lock acquisition.- 2662-2662: Direct access to the
heightfield is correctly implemented. Error handling for lock acquisition should be considered.- 2692-2692: Direct access to the
heightfield is correctly implemented. Consider error handling for lock acquisition.- 3202-3202: Direct access to the
heightfield is correctly implemented. Consider error handling for lock acquisition.- 3279-3279: Direct access to the
heightfield is correctly implemented. Consider error handling for lock acquisition.- 3539-3539: Direct access to the
heightfield is correctly implemented. Consider error handling for lock acquisition.- 3550-3550: Direct access to the
heightfield is correctly implemented. Consider error handling for lock acquisition.- 3561-3561: Direct access to the
heightfield is correctly implemented. Consider error handling for lock acquisition.- 3570-3570: Direct access to the
heightfield is correctly implemented. Consider error handling for lock acquisition.- 3580-3580: Direct access to the
heightfield is correctly implemented. Consider error handling for lock acquisition.- 3637-3637: Direct access to the
heightfield is correctly implemented. Consider error handling for lock acquisition.- 3652-3652: Direct access to the
heightfield is correctly implemented. Consider error handling for lock acquisition.- 3664-3664: Direct access to the
heightfield is correctly implemented. Consider error handling for lock acquisition.- 3942-3942: Direct access to the
heightfield is correctly implemented. Consider error handling for lock acquisition.- 4366-4366: Direct access to the
heightfield is correctly implemented. Consider error handling for lock acquisition.- 4563-4563: Direct access to the
heightfield is correctly implemented. Consider error handling for lock acquisition.- 4701-4701: Direct access to the
heightfield is correctly implemented. Consider error handling for lock acquisition.- 4755-4755: Direct access to the
heightfield is correctly implemented. Consider error handling for lock acquisition.- 5226-5226: Direct access to the
heightfield is correctly implemented. Consider error handling for lock acquisition.- 5557-5557: Direct access to the
heightfield is correctly implemented. Consider error handling for lock acquisition.- 6103-6103: Direct access to the
heightfield is correctly implemented. Consider error handling for lock acquisition.- 6178-6178: Direct access to the
heightfield is correctly implemented. Consider error handling for lock acquisition.- 6280-6280: Direct access to the
heightfield is correctly implemented. Consider error handling for lock acquisition.- 7173-7173: Direct access to the
heightfield is correctly implemented. Consider error handling for lock acquisition.- 8071-8071: Direct access to the
heightfield is correctly implemented. Consider error handling for lock acquisition.- 8090-8090: Direct access to the
heightfield is correctly implemented. Consider error handling for lock acquisition.- 8120-8120: Direct access to the
heightfield is correctly implemented. Consider error handling for lock acquisition.- 8368-8370: Asserting the
block_hashandheightagainst expected values is a critical operation for blockchain consistency. The implementation looks correct, but ensure thorough testing around these assertions to prevent potential chain sync issues.- 8385-8387: Asserting the
block_hashandheightduring block disconnection is crucial for maintaining chain order. The implementation appears correct, but extensive testing is recommended to ensure robustness.- 8421-8421: Handling chain events based on the
heightis implemented correctly. Consider thorough testing to ensure accurate event handling across different blockchain states.- 9121-9121: The check for unfunded channels based on
best_block_heightis correctly implemented. Ensure that this logic aligns with the intended behavior for managing channel states.- 10063-10064: Serializing the
heightandblock_hashofbest_blockis correctly implemented. Ensure data integrity and compatibility for serialization and deserialization processes.- 12346-12346: The use of
current_heightin payment failure checks is correctly implemented. Ensure comprehensive testing for edge cases in payment processing.- 12366-12366: The check for payment processing based on
current_heightis correctly implemented. Comprehensive testing is recommended to validate payment handling logic.- 12379-12379: The implementation for creating pending HTLC info based on
current_heightis correct. Thorough testing is advised to ensure accurate HTLC processing.
| // Ensure the payment fails with the expected error. | ||
| let mut error_data = recv_value_msat.to_be_bytes().to_vec(); | ||
| error_data.extend_from_slice( | ||
| &nodes[0].node.best_block.read().unwrap().height().to_be_bytes(), | ||
| &nodes[0].node.best_block.read().unwrap().height.to_be_bytes(), | ||
| ); | ||
| let mut fail_conditions = PaymentFailedConditions::new() | ||
| .blamed_scid(phantom_scid) |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [132-132]
The function do_test_onion_failure_stale_channel_update modifies channel configurations during the test. Ensure that these modifications are correctly reverted at the end of the test to prevent side effects on subsequent tests.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [132-132]
In test_always_create_tlv_format_onion_payloads, add assertions to explicitly check the format of the onion payloads created, ensuring they are indeed in TLV format as expected by the test's objective.
| @@ -1401,7 +1401,7 @@ fn test_fee_spike_violation_fails_htlc() { | |||
| let secp_ctx = Secp256k1::new(); | |||
| let session_priv = SecretKey::from_slice(&[42; 32]).expect("RNG is bad!"); | |||
|
|
|||
| let cur_height = nodes[1].node.best_block.read().unwrap().height() + 1; | |||
| let cur_height = nodes[1].node.best_block.read().unwrap().height + 1; | |||
There was a problem hiding this comment.
Direct access to the height field of best_block aligns with Rust's idiomatic practices and the PR's objectives. However, consider reviewing the use of .unwrap() for error handling, as it may cause a panic if the lock is poisoned. Exploring more resilient error handling strategies could improve robustness.
| @@ -1599,7 +1599,7 @@ fn test_chan_reserve_violation_inbound_htlc_outbound_channel() { | |||
| // Need to manually create the update_add_htlc message to go around the channel reserve check in send_htlc() | |||
| let secp_ctx = Secp256k1::new(); | |||
| let session_priv = SecretKey::from_slice(&[42; 32]).unwrap(); | |||
| let cur_height = nodes[1].node.best_block.read().unwrap().height() + 1; | |||
| let cur_height = nodes[1].node.best_block.read().unwrap().height + 1; | |||
There was a problem hiding this comment.
This change is consistent with the PR's objectives. As mentioned previously, consider revisiting the use of .unwrap() for error handling to enhance the code's robustness and prevent potential runtime panics.
| @@ -1778,7 +1778,7 @@ fn test_chan_reserve_violation_inbound_htlc_inbound_chan() { | |||
| // Need to manually create the update_add_htlc message to go around the channel reserve check in send_htlc() | |||
| let secp_ctx = Secp256k1::new(); | |||
| let session_priv = SecretKey::from_slice(&[42; 32]).unwrap(); | |||
| let cur_height = nodes[0].node.best_block.read().unwrap().height() + 1; | |||
| let cur_height = nodes[0].node.best_block.read().unwrap().height + 1; | |||
There was a problem hiding this comment.
As with previous hunks, this change aligns with the PR's objectives. Again, consider evaluating the use of .unwrap() for error handling to ensure the code's resilience against potential errors.
| @@ -3503,7 +3503,7 @@ fn fail_backward_pending_htlc_upon_channel_failure() { | |||
|
|
|||
| let secp_ctx = Secp256k1::new(); | |||
| let session_priv = SecretKey::from_slice(&[42; 32]).unwrap(); | |||
| let current_height = nodes[1].node.best_block.read().unwrap().height() + 1; | |||
| let current_height = nodes[1].node.best_block.read().unwrap().height + 1; | |||
There was a problem hiding this comment.
This change is consistent with the PR's objectives. The repeated use of .unwrap() for error handling across these changes suggests a broader review of error handling strategies could be beneficial to the project's overall robustness.
| @@ -6488,7 +6488,7 @@ fn test_update_add_htlc_bolt2_receiver_check_max_htlc_limit() { | |||
| get_route_and_payment_hash!(nodes[0], nodes[1], 1000); | |||
| route.paths[0].hops[0].fee_msat = send_amt; | |||
| let session_priv = SecretKey::from_slice(&[42; 32]).unwrap(); | |||
| let cur_height = nodes[0].node.best_block.read().unwrap().height() + 1; | |||
| let cur_height = nodes[0].node.best_block.read().unwrap().height + 1; | |||
There was a problem hiding this comment.
This change is consistent with the PR's objectives. As previously mentioned, consider a comprehensive review of error handling practices, especially regarding the use of .unwrap(), to enhance code safety and maintainability.
| @@ -10085,7 +10085,7 @@ fn test_non_final_funding_tx() { | |||
| let accept_channel_message = get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id()); | |||
| nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), &accept_channel_message); | |||
|
|
|||
| let best_height = nodes[0].node.best_block.read().unwrap().height(); | |||
| let best_height = nodes[0].node.best_block.read().unwrap().height; | |||
There was a problem hiding this comment.
This change is consistent with the PR's objectives. The pattern of using .unwrap() for error handling is prevalent throughout these changes, reinforcing the suggestion to review and possibly enhance error handling strategies across the affected code.
| @@ -10130,7 +10130,7 @@ fn test_non_final_funding_tx_within_headroom() { | |||
| let accept_channel_message = get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id()); | |||
| nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), &accept_channel_message); | |||
|
|
|||
| let best_height = nodes[0].node.best_block.read().unwrap().height(); | |||
| let best_height = nodes[0].node.best_block.read().unwrap().height; | |||
There was a problem hiding this comment.
This change is consistent with the PR's objectives. The consistent use of .unwrap() for error handling across these changes highlights the importance of reviewing error handling practices to ensure the code's resilience and robustness.
| let prev_blockhash = node.best_block.block_hash; | ||
| let height = node.best_block.height + 1; |
There was a problem hiding this comment.
The code attempts to confirm a transaction by incrementing the block height and updating the best block. However, there's a potential issue with the way prev_blockhash is used to create a new block header. Specifically, the create_dummy_header function is not provided in the code snippet, making it unclear how the previous block hash is being used to generate a new block header. This could potentially lead to incorrect block chain simulation if not handled properly.
Consider ensuring that create_dummy_header correctly uses prev_blockhash to simulate a realistic blockchain environment. If create_dummy_header is part of external code not shown here, verify its implementation to ensure it aligns with blockchain principles.
The code attempts to confirm a transaction by incrementing the block height and updating the best block. However, there's a potential issue with the way prev_blockhash is used to create a new block header. Specifically, the create_dummy_header function is not provided in the code snippet, making it unclear how the previous block hash is being used to generate a new block header. This could potentially lead to incorrect block chain simulation if not handled properly.
Consider ensuring that create_dummy_header correctly uses prev_blockhash to simulate a realistic blockchain environment. If create_dummy_header is part of external code not shown here, verify its implementation to ensure it aligns with blockchain principles.
The code attempts to confirm a transaction by incrementing the block height and updating the best block. However, there's a potential issue with the way prev_blockhash is used to create a new block header. Specifically, the create_dummy_header function is not provided in the code snippet, making it unclear how the previous block hash is being used to generate a new block header. This could potentially lead to incorrect block chain simulation if not handled properly.
Consider ensuring that create_dummy_header correctly uses prev_blockhash to simulate a realistic blockchain environment. If create_dummy_header is part of external code not shown here, verify its implementation to ensure it aligns with blockchain principles.
The code attempts to confirm a transaction by incrementing the block height and updating the best block. However, there's a potential issue with the way prev_blockhash is used to create a new block header. Specifically, the create_dummy_header function is not provided in the code snippet, making it unclear how the previous block hash is being used to generate a new block header. This could potentially lead to incorrect block chain simulation if not handled properly.
Consider ensuring that create_dummy_header correctly uses prev_blockhash to simulate a realistic blockchain environment. If create_dummy_header is part of external code not shown here, verify its implementation to ensure it aligns with blockchain principles.
The code attempts to confirm a transaction by incrementing the block height and updating the best block. However, there's a potential issue with the way prev_blockhash is used to create a new block header. Specifically, the create_dummy_header function is not provided in the code snippet, making it unclear how the previous block hash is being used to generate a new block header. This could potentially lead to incorrect block chain simulation if not handled properly.
Consider ensuring that create_dummy_header correctly uses prev_blockhash to simulate a realistic blockchain environment. If create_dummy_header is part of external code not shown here, verify its implementation to ensure it aligns with blockchain principles.
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2923 +/- ##
==========================================
- Coverage 89.15% 89.14% -0.02%
==========================================
Files 117 117
Lines 94868 94866 -2
Branches 94868 94866 -2
==========================================
- Hits 84584 84569 -15
- Misses 7808 7814 +6
- Partials 2476 2483 +7 ☔ View full report in Codecov by Sentry. |
|
@coderabbitai pause pleeeeeease |
|
@tnull: Understood, I'm pausing the automated reviews as requested. If you need anything else, feel free to let me know. Actions performedReviews paused. |
.. which is more idiomatic Rust, and easier to handle in (downstream) bindings.
tnull
force-pushed
the
2024-03-improve-best-block
branch
from
5e4b2b0 to
007a5db
Compare
I don't have a strong opinion, but I think the reason why the fields were private is because changing only one would result in an invalid block. Then again, there is nothing stopping you from creating an invalid block in the first place with |
Mh, interesting, that's a noteworthy point. Although, if we really want to protect sensitive fields, we probably want to start to establish some project-wide guidelines? Just having it on |
Yeah, at very least Seems I had made the change almost three years ago (5610ca1), but it was for an internal concern about race conditions. And the code itself could have modified the fields, so not sure how useful using private fields really was. More generally, I haven't put a ton of thought into it, but I try to avoid
Hard to say. I imagine we don't have too many structs using |
AFAIU the concern being fixed there is that we wanted to be able to access height + hash in one lock, rather than accessing them separately, not that height and hash need to be protected. |
Yeah, exactly. The fact that |
Previously,
BestBlockwouldn't just expose the innerblock_hash/heightfields but would provide accessor methods. As the former is more idiomatic Rust and it would allow us not to introduce a newtype wrapper for it in LDK Node, we refactor it to just expose the inner fields.Moreover, we have
Debugderived for it.