feat: add onchain address validation for network-specific addresses

[Camillarhi]

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]

👋 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]

🔔 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!

[tnull]

@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]

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.

[tnull]

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.

[Camillarhi]

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

[tnull]

Yes please go ahead.

[tnull]

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

[Camillarhi]

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.