block-core / blockcore

Open source .NET Core Bitcoin based blockchain node in C#
https://www.blockcore.net
219 stars 90 forks source link

[BUG] - BTC Wallet sending from a segwit address, the "change" address is non-segwit. #288

Open MudDev opened 3 years ago

MudDev commented 3 years ago

Using the blockcore UI I sent BTC to segwit address in my blockcore wallet, then sent a transaction to another wallet, the change address went to a non-segwit change address.

sondreb commented 3 years ago

The question is whether the UI should by default use segwit address if enabled on the network, or not?

MudDev commented 3 years ago

Yeah, my node is fully synced, so by default it's using segwit. I think when the node is doing an IBD you see it using legacy until it passes a certain block, then the UI switches it automatically.

dangershony commented 3 years ago

This is the code that will decide if change is segwit or not

 var context = new TransactionBuildContext(this.Network)
            {
                AccountReference = new WalletAccountReference(walletname, accountname),
                Shuffle = true, // We shuffle transaction outputs by default as it's better for anonymity.
                WalletPassword = Password,
                Recipients = recipients,
                UseSegwitChangeAddress = recipients[0].ScriptPubKey.IsScriptType(ScriptType.Witness),
                TransactionFee = new Money(this.Fee, MoneyUnit.BTC)
            };

https://github.com/block-core/blockcore/blob/master/src/Features/Blockcore.Features.Wallet/UI/Pages/WalletSend.razor#L247

So the question is did you send to segwit recipient?

MudDev commented 3 years ago

Ahh, The recipient was not segwit (I think) The address didn't start with bc1....

dangershony commented 3 years ago

This is interesting then, what if the users wants change always in segwit output. We should look at the input then not at the recipient. I suppose we have the input information.

MudDev commented 3 years ago

Edit: Opps...

Yeah, I think we should always use segwit, because the fee's are much better, is there another reason not to?

dangershony commented 3 years ago

Sure I think not, but we have to check segwit is activated first I think.