[RFC] wallet: Always prefer bech32(m) change by default

MarcoFalke commented at 9:07 am on December 10, 2021: member

This function was added in commit 95941396fff81f60d75fc8cca70716b26efe820e.

However, now that taproot was added, I think it might be worth unconditionally trying bech32(m).

Reminder:

If the user picked the change type, this patch won’t change anything.
If the user left all settings unchanged or explicitly set address type to bech32(m), the change will already be bech32(m) for all code paths.
If there is any bech32(m) output, the change will already be bech32(m)

This patch will only change behaviour if the user picked p2sh-witness as address type and there are no bech32(m) outputs. However, it seems inconsistent to special-case this one for a questionable privacy benefit, given that there are already potential privacy leaks with the other settings. Moreover, any potential privacy leak seems questionable, since p2sh-witness will later be revealed to be a wrapped witness spend on-chain. So I think we might as well use native witness to at least save the user some fees.

wallet: Always prefer bech32(m) change by default [ci skip] [skip ci] a2ffb3537b

MarcoFalke added the label Brainstorming on Dec 10, 2021

MarcoFalke added the label Wallet on Dec 10, 2021

MarcoFalke added the label Privacy on Dec 10, 2021

MarcoFalke commented at 9:08 am on December 10, 2021: member

Obviously this will fail tests and need docs fixed up. I’ll do that unless there are NACKs.

MarcoFalke commented at 11:05 am on December 10, 2021: member

(un?) related: According to https://transactionfee.info/charts/inputs-types-by-count/ about 80% of spends today are using the witness stack.

theStack commented at 0:11 am on December 14, 2021: member

Light concept ACK

Talking about potential privacy loss, isn’t the main concern here to consider how hard it is to deduce which one is the change output? (https://en.bitcoin.it/wiki/Privacy#Change_address_detection) For the mentioned special scenario “p2sh-witness as address type and there are no bech32(m) outputs.” and a simple spend to a P2SH(-wrapped) destination:

On master, the change address is also P2SH(-wrapped) -> Perfect, both outputs are equally likely to be change.
On the PR branch, the change address is bech32(m). Is this slightly worse? (Or even better, as analyzers assume the change address type usually matches the input type and would wrongly deduce that the bech32(m) output is actually the destination?)

Just thinking out loud, would be very interested to hear comments about this.

MarcoFalke commented at 7:46 am on December 14, 2021: member

On master, the change address is also P2SH(-wrapped) -> Perfect, both outputs are equally likely to be change.

Not sure this is necessarily true. If the payment is unwrapped to be non-witness and our change is unwrapped to be witness (which is likely because ~80% of all spends are using the witness stack and also P2SH seems to be exclusively used for wrapping a witness program), then this can be combined with other leaks to determine the change address.

On the PR branch, the change address is bech32(m). Is this slightly worse? (Or even better, as analyzers assume the change address type usually matches the input type and would wrongly deduce that the bech32(m) output is actually the destination?)

As you say it might be slightly worse or slightly better, depending on the context. After all those are heuristics.

MarcoFalke commented at 7:54 am on December 14, 2021: member

An alternative approach I could think of would be to treat a wallet that can provide bech32m addresses as a taproot wallet. A wallet that can provide bech32 as a wpkh wallet.

0if tx_has_any_tr_output and wallet_has_tr:
1 return bech32m
2if tx_has_any_wpkh and wallet_has_wpkh:
3 return bech32
4if wallet_has_tr:
5 return bech32m
6if wallet_has_wpkh:
7 return bech32

Obviously this will break again once we add output script descriptor support for v2 witness programs.

Happy to implement this nonetheless if reviewers think it is useful.

in src/wallet/wallet.cpp:1947 in a2ffb3537b

1959-        }
1960+    if (GetScriptPubKeyMan(OutputType::BECH32M, true)) {
1961+        return OutputType::BECH32M;
1962+    } else if (GetScriptPubKeyMan(OutputType::BECH32, true)) {
1963+        return OutputType::BECH32;
1964     }

benthecarman commented at 9:20 pm on December 14, 2021:

doesn’t this need

0
1else {
2  return m_default_address_type;
3}

MarcoFalke commented at 9:26 pm on December 14, 2021:

Yes, and it is already in the code after the comment // else use m_default_address_type for change.

benthecarman commented at 9:27 pm on December 14, 2021:

Oh I see, was outside of the preview of github

achow101 commented at 6:23 pm on December 15, 2021: member

Instead of always using bech32(m), I feel like we should just do the inverse of the current implementation - prefer bech32(m) and use p2sh-segwit if there is a p2sh-segwit output.

MarcoFalke commented at 7:05 pm on December 15, 2021: member

prefer bech32(m) and use p2sh-segwit if there is a p2sh-segwit output.

I don’t think it is possible to see whether a p2sh is segwit or not. While it is likely that a p2sh is segwit, I don’t see a benefit to pick p2sh-segwit change over native segwit change (potentially wpkh). (See OP for explanation).

achow101 commented at 7:12 pm on December 15, 2021: member

I don’t think it is possible to see whether a p2sh is segwit or not.

p2sh-segwit if there are any p2sh outputs, rather.

While it is likely that a p2sh is segwit, I don’t see a benefit to pick p2sh-segwit change over native segwit change (potentially wpkh). (See OP for explanation).

I disagree that there is little or no benefit. Given that the majority of p2sh inputs are nested segwit, it is likely that when those outputs are eventually spent, they will be revealed to be segwit as well, so there is less information to be able to determine which is change. Furthermore, the change output would be less likely to be identified before the outputs are spent.

MarcoFalke commented at 7:29 pm on December 15, 2021: member

Ok, will try to propose an alternative

MarcoFalke closed this on Dec 15, 2021

MarcoFalke deleted the branch on Dec 15, 2021

MarcoFalke commented at 8:17 pm on December 15, 2021: member

Done in https://github.com/bitcoin/bitcoin/pull/23789

DrahtBot locked this on Dec 15, 2022

[RFC] wallet: Always prefer bech32(m) change by default #23731