Segwit for the trade protocol #4612

oscarguindzberg · 2020-10-09T22:36:49Z

Use segwit for trade protocol transactions.

chimp1984 · 2020-10-21T23:43:38Z

core/src/main/java/bisq/core/btc/wallet/TradeWalletService.java

@@ -957,7 +1013,15 @@ public Transaction traderSignAndFinalizeDisputedPayoutTx(byte[] depositTxSeriali

        // take care of sorting!
        Script redeemScript = get2of3MultiSigRedeemScript(buyerPubKey, sellerPubKey, arbitratorPubKey);
-        Sha256Hash sigHash = payoutTx.hashForSignature(0, redeemScript, Transaction.SigHash.ALL, false);
+        Sha256Hash sigHash;
+        boolean hashedMultiSigOutputIsSegwit = !ScriptPattern.isP2SH(hashedMultiSigOutput.getScriptPubKey());


Maybe better to use "hashedMultiSigOutputIsLegacy" to avoid the 2x negation. Makes it easier to read...

chimp1984 · 2020-10-21T23:47:04Z

core/src/main/java/bisq/core/btc/wallet/TradeWalletService.java

+        // Since there is no way to check migration to bitcoinj 0.15,
+        // Config.SEGWIT_TRADE_PROTOCOL_ACTIVE could be checked instead.
+        // Actually, given the serialized tx is used just as the connected tx of an input,
+        // we don't need the the connected tx's witness data.


duplicate "the the"

Isn't the input always from the users own wallet (not the peers input)? If so I don't understand the above comment. Then this code is only execute anyway if the user runs the new version and segwit could be used.

duplicate "the the"

fixed

Isn't the input always from the users own wallet (not the peers input)? If so I don't understand the above comment. Then this code is only execute anyway if the user runs the new version and segwit could be used.

When creating the deposit tx, traders will have a tx with an input from her wallet and an input from the counterparty. Traders collaborate creating the deposit tx. They send RawTransactionInput over p2p. So, if you are trading with a node that was not upgraded to bitcoinj 0.15 and RawTransactionInput.parentTransaction is serialized with segwit data, the other node won't be able to parse RawTransactionInput.parentTransaction correctly.
Extra comments:

RawTransactionInput.parentTransaction could be replaced with RawTransactionInput.parentTransactionHash since the tx is only used to obtain its hash.

Since we upgraded the trade protocol version, it is not possible to take a trade from a not-upgraded node, but better be on the safe side. This comment made probably more sense for the previous release.

I will update the comment to make it more clear.

chimp1984 · 2020-10-21T23:52:11Z

common/src/main/java/bisq/common/app/Version.java

    // VERSION = 0.5.0 -> TRADE_PROTOCOL_VERSION = 1
    // Version 1.2.2 -> TRADE_PROTOCOL_VERSION = 2
-    public static final int TRADE_PROTOCOL_VERSION = 2;
+    // Version 1.4.1 -> TRADE_PROTOCOL_VERSION = 3


We will be on 1.4.2 or more likely 1.5.0

chimp1984 · 2020-10-22T00:31:30Z

core/src/main/java/bisq/core/btc/wallet/TradeWalletService.java

-        if (scriptProgram.length == 0) {
-            throw new TransactionVerificationException("Inputs from maker not signed.");
-        }
-
        return scriptProgram;


Variable is redundant now. Also does not throw exception anymore

This all started with scriptProgram.length == 0 being valid for a segwit input.
I just removed the method and moved its content to where it was being used, i.e. takerSignsDepositTx().
See takerSignsDepositTx():

byte[] makersScriptSigProgram = makersInput.getScriptSig().getProgram(); if (makersScriptSigProgram.length == 0 && TransactionWitness.EMPTY.equals(makersInput.getWitness())) { throw new TransactionVerificationException("Inputs from maker not signed."); }

See 7eb808d

Yes, I saw. But now the method does not throw TransactionVerificationException anymore.

I saw you removed the method. Good so...

chimp1984 · 2020-10-22T00:34:34Z

core/src/main/java/bisq/core/btc/wallet/TradeWalletService.java

-            Script scriptCode = new ScriptBuilder().data(
-                    ScriptBuilder.createOutputScript(LegacyAddress.fromKey(transaction.getParams(), sigKey)).getProgram())
-                    .build();
+            Script scriptCode = ScriptBuilder.createP2PKHOutputScript(sigKey);


Is createP2PKHOutputScript correct? Would expect a segwit method...

Yes, it is correct.
I know it sounds weird.
To sign a P2WPKH input you have to serialize the tx with a specific format, hash it and then sign the hash.
That specific format includes something called the scriptCode which should be: OP_DUP OP_HASH160 pubKeyHash OP_EQUALVERIFY OP_CHECKSIG. That is the same script format used in P2PKH outputs.
Detailed info: https://github.com/bitcoin/bips/blob/master/bip-0143.mediawiki

Ok. Maybe a comment would be good with backgorund info.

chimp1984 · 2020-10-22T00:38:59Z

core/src/main/java/bisq/core/trade/protocol/tasks/buyer/BuyerSignsDelayedPayoutTx.java

+            NetworkParameters params = btcWalletService.getParams();
+            Transaction preparedDepositTx = new Transaction(params, processModel.getPreparedDepositTx());
+            Coin delayedPayoutTxInputValue = preparedDepositTx.getOutput(0).getValue();
+


Why not take the input from preparedDelayedPayoutTx? If that is not possible, I would prefer to pass over the deposit tx to the signDelayedPayoutTx method and do the deail work in the TradeWalletService to keep the transaction domain more isolated there.

In bitcoin (the protocol) inputs do not include the amount.
In bitcoinj (the library) you can connect a TransactionInput to its related TransactionOutput and then ask the TransactionInput its value. But it is only possible if you connected both of them.
I moved the code to obtain the input value to TradeWalletService.

oscarguindzberg · 2020-10-22T15:23:14Z

I deleted Config.SEGWIT_TRADE_PROTOCOL_ACTIVE

oscarguindzberg · 2020-10-22T15:23:45Z

@chimp1984 I addressed all your comments.

chimp1984 · 2020-10-22T15:42:52Z

core/src/main/java/bisq/core/btc/wallet/TradeWalletService.java

-        // Actually, given the serialized tx is used just as the connected tx of an input,
-        // we don't need the connected tx's witness data.
+        // bitcoinSerialize(false) is used just in case the serialized tx is parsed by a bisq node still using
+        // bitcoinj 0.14. The serialized tx is just used to obtain its hash, so the witness data is not relevant.
        return new RawTransactionInput(input.getOutpoint().getIndex(),


getRawInputFromTransactionInput is used only by methods for creating the deposit tx. With the trade protocol update we do not have mixed cases of segwit/legacy here. Or do I miss something?

yes, you are right, just fixed the comment to explain that.

chimp1984 · 2020-10-22T15:43:59Z

core/src/main/java/bisq/core/btc/wallet/TradeWalletService.java

-        if (scriptProgram.length == 0) {
-            throw new TransactionVerificationException("Inputs from maker not signed.");
-        }
-
        return scriptProgram;


I saw you removed the method. Good so...

wiz · 2020-10-22T15:47:43Z

is this ready to begin testing?

chimp1984 · 2020-10-22T15:56:32Z

@wiz Not really. You could test it locally but not with mainnet with real users....
We merge first all pending PRs to master, then create a release branch and merge segwit into it. Its a trade protocol update so master must not use that.

oscarguindzberg · 2020-10-22T17:42:34Z

@wiz I suggest you wait until I remove the WIP tag.

chimp1984

utACK

oscarguindzberg · 2020-10-26T23:41:42Z

I just squashed commits and polished commit messages, but I had not updated the codebase.

This reverts commit b8f5c6e.

This reverts commit e49c565.

oscarguindzberg · 2020-11-05T14:57:33Z

WIP tag removed
Pointed PR to release/v1.5.0

chimp1984

utACK

oscarguindzberg force-pushed the segwit branch 6 times, most recently from a44285d to aae2d25 Compare October 21, 2020 18:56

oscarguindzberg changed the base branch from release/v1.4.0 to master October 21, 2020 18:59

chimp1984 suggested changes Oct 22, 2020

View reviewed changes

oscarguindzberg force-pushed the segwit branch from 911ed77 to 1d79dfb Compare October 22, 2020 15:19

chimp1984 reviewed Oct 22, 2020

View reviewed changes

chimp1984 previously approved these changes Oct 22, 2020

View reviewed changes

oscarguindzberg dismissed chimp1984’s stale review via 46c7d9e October 26, 2020 22:18

oscarguindzberg force-pushed the segwit branch 3 times, most recently from 86a35b9 to 19bdf79 Compare October 26, 2020 23:21

oscarguindzberg mentioned this pull request Oct 26, 2020

Segwit fee estimation #4710

Merged

oscarguindzberg force-pushed the segwit branch from 19bdf79 to 0dda9d4 Compare October 28, 2020 21:09

oscarguindzberg added 7 commits November 5, 2020 11:47

Use bitcoinj 0.15.8 (commit fcec3da)

ef97daa

BtcWalletService: Use segwit addresses

161e220

TradeWalletService use P2WSH

06e5091

Revert "Construct dummy outputs with LegacyAddress"

4a05b6d

This reverts commit b8f5c6e.

Explain why bitcoinSerialize(false) is used

22ba9a8

Create the scriptCode the right way

3585dc9

Deal with P2WPKH has empty scriptSig

953a5f0

oscarguindzberg added 3 commits November 5, 2020 11:47

Revert "Validate AddressEntry.segwit"

d1620c4

This reverts commit e49c565.

Set TRADE_PROTOCOL_VERSION to 3

a3d7c71

Remove unused imports

dffa251

oscarguindzberg force-pushed the segwit branch from 0dda9d4 to dffa251 Compare November 5, 2020 14:48

oscarguindzberg changed the base branch from master to release/v1.5.0 November 5, 2020 14:49

oscarguindzberg changed the title ~~[WIP] Segwit for the trade protocol~~ Segwit for the trade protocol Nov 5, 2020

chimp1984 approved these changes Nov 5, 2020

View reviewed changes

ripcurlx merged commit 35018b0 into bisq-network:release/v1.5.0 Nov 5, 2020

ripcurlx added this to the v1.5.0 milestone Nov 9, 2020

oscarguindzberg mentioned this pull request Nov 16, 2020

For cycle 19 bisq-network/compensation#710

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Segwit for the trade protocol #4612

Segwit for the trade protocol #4612

oscarguindzberg commented Oct 9, 2020 •

edited

Loading

chimp1984 Oct 21, 2020

oscarguindzberg Oct 22, 2020

chimp1984 Oct 21, 2020

chimp1984 Oct 21, 2020

oscarguindzberg Oct 22, 2020

oscarguindzberg Oct 22, 2020

oscarguindzberg Oct 22, 2020

chimp1984 Oct 21, 2020

oscarguindzberg Oct 22, 2020

chimp1984 Oct 22, 2020

oscarguindzberg Oct 22, 2020

chimp1984 Oct 22, 2020

chimp1984 Oct 22, 2020

chimp1984 Oct 22, 2020

oscarguindzberg Oct 22, 2020

chimp1984 Oct 22, 2020

oscarguindzberg Oct 22, 2020

chimp1984 Oct 22, 2020

oscarguindzberg Oct 22, 2020

oscarguindzberg commented Oct 22, 2020

oscarguindzberg commented Oct 22, 2020

chimp1984 Oct 22, 2020

oscarguindzberg Oct 22, 2020

chimp1984 Oct 22, 2020

wiz commented Oct 22, 2020

chimp1984 commented Oct 22, 2020 •

edited

Loading

oscarguindzberg commented Oct 22, 2020

chimp1984 left a comment

oscarguindzberg commented Oct 26, 2020

oscarguindzberg commented Nov 5, 2020

chimp1984 left a comment

Segwit for the trade protocol #4612

Segwit for the trade protocol #4612

Conversation

oscarguindzberg commented Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oscarguindzberg commented Oct 22, 2020

oscarguindzberg commented Oct 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wiz commented Oct 22, 2020

chimp1984 commented Oct 22, 2020 • edited Loading

oscarguindzberg commented Oct 22, 2020

chimp1984 left a comment

Choose a reason for hiding this comment

oscarguindzberg commented Oct 26, 2020

oscarguindzberg commented Nov 5, 2020

chimp1984 left a comment

Choose a reason for hiding this comment

oscarguindzberg commented Oct 9, 2020 •

edited

Loading

chimp1984 commented Oct 22, 2020 •

edited

Loading