[bisq-network/bisq] Add segwit support to the BTC wallet (#4568)

[chimp1984]

@chimp1984 requested changes on this pull request.



> @@ -584,23 +584,13 @@ public AddressEntry getOrCreateAddressEntry(String offerId, AddressEntry.Context
             if (emptyAvailableAddressEntry.isPresent()) {
                 return addressEntryList.swapAvailableToAddressEntryWithOfferId(emptyAvailableAddressEntry.get(), context, offerId);
             } else {
-                AddressEntry entry = new AddressEntry(wallet.freshReceiveKey(), context, offerId);
+                AddressEntry entry = new AddressEntry(wallet.freshReceiveKey(), context, offerId, true);

Is that never called from a trade protocol entry? e.g. the one for the multisig.

> @@ -601,6 +603,9 @@ public Transaction takerSignsDepositTx(boolean takerIsSeller,
             for (int i = 0; i < buyerInputs.size(); i++) {
                 TransactionInput transactionInput = makersDepositTx.getInputs().get(i);
                 depositTx.addInput(getTransactionInput(depositTx, getMakersScriptSigProgram(transactionInput), buyerInputs.get(i)));
+                if (!TransactionWitness.EMPTY.equals(transactionInput.getWitness())) {
+                    depositTx.getInput(depositTx.getInputs().size()-1).setWitness(transactionInput.getWitness());

Instead of `depositTx.getInputs().size()-1)` I would suggest to make a local var from the `getTransactionInput(depositTx, getMakersScriptSigProgram(transactionInput), buyerInputs.get(i))` we add earlier so its more clear. Maybe even do the check before adding, technically no difference but somehow more clean if the input gets completely defined before added.

> @@ -601,6 +603,9 @@ public Transaction takerSignsDepositTx(boolean takerIsSeller,
             for (int i = 0; i < buyerInputs.size(); i++) {
                 TransactionInput transactionInput = makersDepositTx.getInputs().get(i);
                 depositTx.addInput(getTransactionInput(depositTx, getMakersScriptSigProgram(transactionInput), buyerInputs.get(i)));
+                if (!TransactionWitness.EMPTY.equals(transactionInput.getWitness())) {
+                    depositTx.getInput(depositTx.getInputs().size()-1).setWitness(transactionInput.getWitness());

As we do not support now segwit in the trade process, is that change needed at all? I dont see a risk to have it there but maybe better to leave all that out until we start adding segwit support to the trade protocol? Not sure if you started already in more pleaces to add support which do not interfere (as we discussed it is our strategy to add as much as possible without activating segwit in trade protocol...), so if you have started that process already all find, otherwise if its a single change it would be maybe more clear to start after the release with that process... 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/4568#pullrequestreview-502272340
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20201005/0f7ea6a0/attachment.html>