feat: add onchain address validation for network-specific addresses #519

Camillarhi · 2025-04-04T23:46:10Z

Addresses need to be validated against the target network before use to ensure correctness
and prevent potential loss of funds.

This commit introduces a function to validate addresses based on the target network:

Implements a function to validate addresses based on the network.
Adds unit tests to verify the correctness of the validation logic.

This change enhances the robustness of address handling and mitigates risks associated with invalid or mismatched addresses.

Fixes #386

ldk-reviews-bot · 2025-04-04T23:46:13Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

ldk-reviews-bot · 2025-04-07T00:00:14Z

🔔 1st Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

enigbe

Hi @Camillarhi,

Thanks for picking this. I reviewed this over the weekend and from my understanding of the issue, the expectation is to expose functionality for the bindings targets, so that they can validate addresses against the network.

We use UniFFI to generate the bindings targeting ldk-node, defining interfaces in here. Your approach uses an already exposed interface OnchainPayment. You can update the interface to add parse_and_validate_address.

@tnull is this enough of a middle ground or do you have other ideas? Would it be preferable to adapt rust-bitcoin's Address and implement the validation on the wrapper?

tnull

Thanks for looking into this!

src/error.rs

src/payment/onchain.rs

tnull · 2025-04-07T11:42:20Z

@tnull is this enough of a middle ground or do you have other ideas? Would it be preferable to adapt rust-bitcoin's Address and implement the validation on the wrapper?

Yeah, honestly, I'm a bit on the fence. rust-bitcoin's enforced validation via types is a bit cumbersome, and generally something I don't want to force down our user's throats, especially in bindings targets where it may be even less idiomatic. Maybe just making sure to validate the addresses before using them is the way to go, we just have to make sure we're not missing any codepaths.

tnull

Ah, seems CI is also unhappy due to formatting issues. Please make sure to run cargo fmt before committing.

enigbe · 2025-04-08T09:54:57Z

Yeah, honestly, I'm a bit on the fence. rust-bitcoin's enforced validation via types is a bit cumbersome, and generally something I don't want to force down our user's throats, especially in bindings targets where it may be even less idiomatic. Maybe just making sure to validate the addresses before using them is the way to go, we just have to make sure we're not missing any codepaths.

Thanks for clarifying this.

src/error.rs

src/main.rs

src/payment/onchain.rs

tnull · 2025-04-10T11:06:09Z

src/payment/onchain.rs

+	pub fn parse_and_validate_address(
+		&self, network: Network, address: &Address,
+	) -> Result<Address, Error> {
+		Address::<NetworkUnchecked>::from_str(address.to_string().as_str())


This is kind of annoying, but I guess there is no other way to do this if we don't want to take an UncheckedAddress type ourselves, so fine to leave as is I guess.

Alright , do I go ahead and expose this functionality for the binding targets?

Yes please go ahead.

src/payment/onchain.rs

tnull

Seems CI is still failing, please let me know when this is ready for another round of review.

Camillarhi · 2025-04-22T15:12:50Z

Seems CI is still failing, please let me know when this is ready for another round of review.

Hello @tnull , this is ready for another review, though the CI is still failing

tnull

Excuse the delay here. Basically looks good, just a few minor comments.

tests/integration_tests_rust.rs

src/wallet/mod.rs

tnull

Please excuse the delay here!

LGTM!

Please squash the fixup commits into the feature commits to cleanup the commit history. Feel free to address the nit below while squashing.

Note it's usually prerable to keep a clean commit history at all times, and to clearly mark the fixup commits as you go. This link is a good starting point for some guidance on how to write commit messages: https://cbea.ms/git-commit/

src/wallet/mod.rs

Camillarhi · 2025-05-12T08:15:12Z

Please excuse the delay here!

LGTM!

Please squash the fixup commits into the feature commits to cleanup the commit history. Feel free to address the nit below while squashing.

Note it's usually prerable to keep a clean commit history at all times, and to clearly mark the fixup commits as you go. This link is a good starting point for some guidance on how to write commit messages: https://cbea.ms/git-commit/

Thanks, I will get on this and tag you as soon as it's done

Camillarhi · 2025-05-13T23:15:03Z

Please excuse the delay here!

LGTM!

Please squash the fixup commits into the feature commits to cleanup the commit history. Feel free to address the nit below while squashing.

Note it's usually prerable to keep a clean commit history at all times, and to clearly mark the fixup commits as you go. This link is a good starting point for some guidance on how to write commit messages: https://cbea.ms/git-commit/

Hello @tnull , I have squashed the commits and cleaned up the commit history.

Validate onchain addresses match their network type before processing transactions. This prevents sending funds to invalid addresses by: - Checking address network match expected network - Providing clear error messages for mismatches The validation is now integrated into Wallet::send_to_address flow

ldk-reviews-bot requested a review from jkczyz April 4, 2025 23:56

enigbe reviewed Apr 7, 2025

View reviewed changes

tnull requested review from tnull and removed request for jkczyz April 7, 2025 07:58

tnull reviewed Apr 7, 2025

View reviewed changes

src/error.rs Outdated Show resolved Hide resolved

src/payment/onchain.rs Outdated Show resolved Hide resolved

tnull reviewed Apr 7, 2025

View reviewed changes

tnull reviewed Apr 10, 2025

View reviewed changes

tnull reviewed Apr 17, 2025

View reviewed changes

Camillarhi force-pushed the validate-onchain-address branch from 63a6895 to 8273f07 Compare April 28, 2025 20:54

Camillarhi mentioned this pull request Apr 29, 2025

test_cln is flaky now #527

Open

tnull reviewed Apr 29, 2025

View reviewed changes

tests/integration_tests_rust.rs Show resolved Hide resolved

tests/integration_tests_rust.rs Outdated Show resolved Hide resolved

src/wallet/mod.rs Outdated Show resolved Hide resolved

tnull reviewed May 12, 2025

View reviewed changes

src/wallet/mod.rs Outdated Show resolved Hide resolved

Camillarhi force-pushed the validate-onchain-address branch 2 times, most recently from 1f16658 to 5030750 Compare May 13, 2025 23:12

Camillarhi force-pushed the validate-onchain-address branch from 5030750 to 94b32c9 Compare May 14, 2025 08:46

tnull approved these changes May 14, 2025

View reviewed changes

tnull merged commit 5586b69 into lightningdevkit:main May 14, 2025
15 checks passed

feat: add onchain address validation for network-specific addresses #519

feat: add onchain address validation for network-specific addresses #519

Conversation

Camillarhi commented Apr 4, 2025

Uh oh!

ldk-reviews-bot commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Apr 7, 2025

Uh oh!

enigbe left a comment

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tnull commented Apr 7, 2025

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

enigbe commented Apr 8, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tnull Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

Camillarhi Apr 13, 2025

Choose a reason for hiding this comment

Uh oh!

tnull Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Camillarhi commented Apr 22, 2025

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Camillarhi commented May 12, 2025

Uh oh!

Camillarhi commented May 13, 2025

Uh oh!

Uh oh!

Uh oh!

ldk-reviews-bot commented Apr 4, 2025 •

edited

Loading