rpc: verify address is for correct net

[torkelrogstad]

trafficstars

Change Description

Verify that the addresses we're decoding when sending coins onchain are for the correct network. Without this check we'll convert the users addresses to their equivalent on other networks, which is a gross violation of the principle of least astonishment.

Steps to Test

Prior to this commit:

1. Run a node on regtest
2. Issue either a sendmany or sendcoins RPC, with a mainnet or testnet address
3. Observe that the address is accepted, and converted into a regtest address.

After this commit, step 2 fails.

Pull Request Checklist

Testing

• [ ] Your PR passes all CI checks.
• [ ] Tests covering the positive and negative (error paths) are included.
• [ ] Bug fixes contain tests triggering the bug to prevent regressions. There doesn't seem to be any tests for this, at least not based on a quick code search. Would be happy to implement if I'm wrong.

Code Style and Documentation

• [x] The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
• [x] Commits follow the Ideal Git Commit Structure.
• [x] ~Any new logging statements use an appropriate subsystem and logging level.~
• [x] There is a change description in the release notes, or [skip ci] in the commit message for small changes.

📝 Please see our Contribution Guidelines for further guidance.

[torkelrogstad]

Or, to fix the underlying assumption that (because it takes the net params as the second parameter) DecodeAddress actually checks that the address belongs to the correct network, we should perhaps add the check there?

Do you mean changing btcutil? That sounds like a pretty big behavioral change in how DecodeAddress works

[guggero]

Do you mean changing btcutil? That sounds like a pretty big behavioral change in how DecodeAddress works

Yes, maybe it's too big of a change. But maybe it would be worth adding a comment in the btcutil library that for legacy addresses the network isn't checked.

[torkelrogstad]

Found another place, in chancloser

https://github.com/lightningnetwork/lnd/blob/f13399bc494d9ad78c4ecbc081e027ee3b41485f/lnwallet/chancloser/chancloser.go#L710-L728

For the sake of returning errors that look the same in all places, and ergonomics, would it be an idea to introduce a new wallet in btcutil? Filed an issue in btcd, figured that was a better place to discuss: https://github.com/btcsuite/btcd/issues/1846

[guggero]

Looking at the code, this seems to only be possible with non-Segwit (non-bech32) addresses in the first place.

Oops, I was wrong there. I read the if chaincfg.IsBech32SegwitPrefix(prefix) { incorrectly. It doesn't check that the prefix is for the given network, it just checks if it's any valid prefix...

I like your idea of adding DecodeAddressForNet in btcd. Since we'll be bumping the dependency to that project anyway, I'd rather use that method everywhere in lnd (after the PR there was merged) than add the IsForNet check everywhere.

[lightninglabs-deploy]

@torkelrogstad, remember to re-request review from reviewers when ready

[lightninglabs-deploy]

@torkelrogstad, remember to re-request review from reviewers when ready

[lightninglabs-deploy]

@torkelrogstad, remember to re-request review from reviewers when ready

[guggero]

Replaced by/included in https://github.com/lightningnetwork/lnd/pull/7689, original commit author credits are preserved.