Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add segwit support to the BTC wallet #4568

Merged
merged 30 commits into from
Oct 8, 2020

Conversation

oscarguindzberg
Copy link
Contributor

This is just the BTC wallet.
Trade protocol keeps using legacy addresses.
This PR does not require a hardfork.

@ripcurlx
Copy link
Contributor

@oscarguindzberg If you run into Codacy issues please let me know upfront, so you don't need to create unnecessary workarounds. I just checked the current complaints and removed the pattern for comment intention as IntelliJ is doing it differently by default. The other remaining two are legit (formatting issue and unused import)
Bildschirmfoto 2020-09-29 um 09 46 10

@oscarguindzberg oscarguindzberg force-pushed the segwitWallet branch 3 times, most recently from f09d829 to 40d67ba Compare October 1, 2020 16:05
Copy link
Contributor

@chimp1984 chimp1984 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@oscarguindzberg oscarguindzberg force-pushed the segwitWallet branch 3 times, most recently from 778a7aa to d1e3e09 Compare October 5, 2020 15:49
@oscarguindzberg oscarguindzberg changed the title [WIP] Add segwit support to the BTC wallet Add segwit support to the BTC wallet Oct 5, 2020
@oscarguindzberg
Copy link
Contributor Author

  • Finished reviewing the source code to make sure this brings no bugs to the dao.
  • Tested DAO txs (compensation requests, voting, etc) and trading. I tested both pre-segwit and post-segwit nodes.
  • Removed the WIP tag from this PR title.

This PR is ready to be merged IMHO.

@oscarguindzberg
Copy link
Contributor Author

@ripcurlx @sqrrm @chimp1984 Just bringing you attention in case you are in charge of approving/merging this PR.

@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems you commented on outdated code.

@@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You seem to be right. In any case, it causes no harm.

@sqrrm sqrrm mentioned this pull request Oct 6, 2020
@oscarguindzberg oscarguindzberg changed the title Add segwit support to the BTC wallet [WIP] Add segwit support to the BTC wallet Oct 7, 2020
@oscarguindzberg
Copy link
Contributor Author

@ripcurlx Found a bug migrating encrypted wallets, added WIP tag. Will remove WIP once the bug is fixed.

@oscarguindzberg oscarguindzberg changed the base branch from master to release/v1.4.0 October 8, 2020 19:34
Copy link
Contributor

@chimp1984 chimp1984 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

See small suggestion in comment...

if (migratedWalletToSegwit.get()) {
startPeerGroup();
} else {
migratedWalletToSegwit.addListener((observable, oldValue, newValue) -> startPeerGroup());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add a if(newValue) to make it more clear/safe. As we change only once from false to true it would not matter but feels more correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed 261e0ec

@oscarguindzberg oscarguindzberg changed the title [WIP] Add segwit support to the BTC wallet Add segwit support to the BTC wallet Oct 8, 2020
Copy link
Contributor

@chimp1984 chimp1984 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@sqrrm sqrrm merged commit 35e0c34 into bisq-network:release/v1.4.0 Oct 8, 2020
@wiz
Copy link
Member

wiz commented Oct 9, 2020

post merge ACK - tested OK on mainnet using encrypted wallet for send and receive, and also using mixed segwit/legacy inputs/outputs all worked fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants