Use bisq's bitcoinj 0.15.8 - september 2020 #4504

oscarguindzberg · 2020-09-09T00:15:25Z

Adapt Bisq to bitcoinj 0.15.8 (see https://github.com/bisq-network/bitcoinj/commits/bisq_0.15.8)
That is a pre-requisite for segwit support.
This PR replaces #4270

oscarguindzberg · 2020-09-09T00:32:24Z

I see codacy suggested a couple of changes, I will fix them ASAP

chimp1984 · 2020-09-09T00:59:19Z

core/src/main/java/bisq/core/btc/setup/BisqKeyChainGroupStructure.java

+    }
+
+    @Override
+    public ImmutableList<ChildNumber> accountPathFor(Script.ScriptType outputScriptType) {


Aren't there more script types (MultiSig) to check?

Even though Bisq uses multisig (eg for payout tx signing), bisq does not use a multisig keychain.
A multisig keychain (MarriedKeyChain in bitcoinj) makes sense when you want to repeatedly generate new multisig addresses for the same group of people.

Ok, thanks. Did not see the full context.

chimp1984 · 2020-09-09T01:00:52Z

core/src/main/java/bisq/core/btc/setup/WalletsSetup.java

@@ -172,7 +172,6 @@ public WalletsSetup(RegTestHost regTestHost,

        btcWalletFileName = "bisq_" + config.baseCurrencyNetwork.getCurrencyCode() + ".wallet";
        params = Config.baseCurrencyNetworkParameters();
-        PeerGroup.setIgnoreHttpSeeds(true);


Are httpSeed still used by default in BitcoinJ? If so I would prefer to still make sure to not use http seeds as they have some privacy issues.

http seeds are not used by default in bitcoinj and are not used in bisq as is.
Having said that, I had another look at this and found they might be used if a bisq dev activates a certain feature of bitcoinj that is not being used today. So, I will add PeerGroup.setIgnoreHttpSeeds(true) again.

chimp1984 · 2020-09-09T01:03:23Z

core/src/main/java/bisq/core/btc/setup/WalletConfig.java

@@ -495,6 +485,8 @@ private Wallet loadWallet(boolean shouldReplayWallet, File walletFile, boolean i
    }

    protected Wallet createWallet(boolean isBsqWallet) {
+        // Change preferredOutputScriptType of btc wallet to P2WPKH to start using segwit
+        Script.ScriptType preferredOutputScriptType = isBsqWallet ? Script.ScriptType.P2PKH : Script.ScriptType.P2PKH;


I guess the else case should be P2WPKH

That will be true when we implement segwit.
For now, this is just migration to bitcoinj 0.15
The code is a placeholder for the code that is coming soon (see the comment in the previous line).
I will update the code to:
Script.ScriptType preferredOutputScriptType = Script.ScriptType.P2PKH;
To avoid confusions until segwit is implemented.

Ah missed the comment.

chimp1984 · 2020-09-09T02:57:26Z

@ripcurlx why it got flagged incorrectly (altcoin)?

ripcurlx · 2020-09-09T13:26:49Z

@ripcurlx why it got flagged incorrectly (altcoin)?

It touches the assets module.

oscarguindzberg · 2020-09-09T17:16:55Z

I fixed codacy suggestions, but 2 of them. They say I should avoid calling toString() on a String object, but actually Sha256Hash.toString() is being called.

chimp1984 · 2020-09-09T17:18:53Z

I fixed codacy suggestions, but 2 of them. They say I should avoid calling toString() on a String object, but actually Sha256Hash.toString() is being called.

Its in TransactionsListItem.java trade.getDepositTx().getTxId().toString()

chimp1984

preliminary utACK
But I would like to do a more in depth review in a few days. So better wait for after release with merge and another ACK as its high risk area with wallet code changes.

ripcurlx · 2020-09-15T11:33:37Z

@oscarguindzberg Could you please have a look at the two unnecessary toString calls Codacy is complaining about, so this PR is mergeable? Thanks!

chimp1984

utACK

sqrrm

Tested in regtest and wallet conversion seems to have worked fine. Sending coins work fine, BSQ parsing looks correct.

What I haven't done is completely understood the changes to WalletConfig.java. The rest looks ok to me.

sqrrm · 2020-09-17T09:44:13Z

core/src/main/java/bisq/core/btc/model/AddressEntryList.java

@@ -105,9 +107,9 @@ public void onWalletReady(Wallet wallet) {
        if (!entrySet.isEmpty()) {
            Set<AddressEntry> toBeRemoved = new HashSet<>();
            entrySet.forEach(addressEntry -> {
-                DeterministicKey keyFromPubHash = (DeterministicKey) wallet.findKeyFromPubHash(addressEntry.getPubKeyHash());
+                DeterministicKey keyFromPubHash = (DeterministicKey) wallet.findKeyFromPubKeyHash(addressEntry.getPubKeyHash(), Script.ScriptType.P2PKH);


Prefer to break lines at 120 characters.

sqrrm · 2020-09-17T11:42:42Z

gradle/witness/gradle-witness.gradle

@@ -68,22 +68,23 @@ dependencyVerification {
        'net.glxn:qrgen:c85d9d8512d91e8ad11fe56259a7825bd50ce0245447e236cf168d1b17591882',
        'net.jcip:jcip-annotations:be5805392060c71474bf6c9a67a099471274d30b83eef84bfc4e0889a4f1dcc0',
        'net.sf.jopt-simple:jopt-simple:df26cc58f235f477db07f753ba5a3ab243ebe5789d9f89ecf68dd62ea9a66c28',
-        'network.bisq.btcd-cli4j:btcd-cli4j-core:203156fc63dc1202774de9818e4f21149549f79b25d356b08bb0c784be40c0e8',
-        'network.bisq.btcd-cli4j:btcd-cli4j-daemon:0a2783a851add6e3d8ae899ade48c041b250bfac64b6a4c5f6380ebcdbbe6848',
+        'network.bisq.btcd-cli4j:btcd-cli4j-core:4634b39de93764c4609e295e254e8c3b1427ba24febf43352f4f315029c5b1b3',


Upgraded btcd-cli4j, why was this necessary?

Kind of long story...
A year ago I upgraded some Bisq dependencies to match bitcoinj 0.15.1 dependencies. eg upgraded guava from 20 to 27.
Then, when bitcoinj 0.15.8 came out with guava 28.2 , I upgraded Bisq to use that guava version.
None of those changes were merged yet, we are merging it as part of this PR.

Some time ago this code was merged into the btcd-cli4j repo bisq-network/btcd-cli4j#4 . And then you reverted part of the change bisq-network/btcd-cli4j#8.

So, I am not actually upgrading btcd-cli4j but downgrading it to the version that declares a dependency on guava 27.

In any case, bisq's gradle resolves not to include the guava version declared in btcd-cli4j but the version declared on its own build.gradle file.

To make things clear a new btcd-cli4j version declaring a dependency to guava 28.2-jre could be created, then we could upgrade to that btcd-cli4j version in bisq.

chimp1984

utACK

oscarguindzberg · 2020-09-17T20:53:04Z

@ripcurlx the two "unnecessary toString() calls Codacy is complaining about" seem like a Codacy bug to me. toString() is being called on Sha256Hash instances, not on String instances.

oscarguindzberg · 2020-09-17T21:25:20Z

@sqrrm @ripcurlx I think this PR is ready for merging. Please, let me know if there is something I did not answer/fix.

oscarguindzberg · 2020-09-17T21:43:24Z

@sqrrm I re-requested review from you, I guess that is necessary to merge the PR since I pushed 3 new commits with a bugfix and code styling changes.

sqrrm

ACK

Waiting for @ripcurlx to test this before merging as well. Also need a resolution to the codacy complaints.

chimp1984

utACK

chimp1984 · 2020-09-18T16:25:49Z

@sqrrm @ripcurlx The codady complains are false positives. getTxId() returns a ShaHash and toString() is correct on that. Seems a bug in codacy. Please ignore that and merge or deactivate that rule.

sqrrm · 2020-09-18T16:29:40Z

Impossible to ignore, it blocks merging. So either we're changing codacy rules or we write bad code to get around codacy. It's a pity that's it's so annoying.

Make it clear P2PKH is used for both BSQ and BTC wallets until segwit is implemented.

…ue)." This reverts commit 9be6a9d.

oscarguindzberg · 2020-09-18T21:35:39Z

I just signed my commits. I had to push again. I guess the PR requires approval again.

chimp1984 · 2020-09-19T00:07:01Z

Impossible to ignore, it blocks merging. So either we're changing codacy rules or we write bad code to get around codacy. It's a pity that's it's so annoying.

Thats the real dnager with that too strict and inflexible robot. We bow down and do things we would not do otherwise (for instance i use less exception handling now as it enforces me to write custom exceptions which is often boilerplate, leading at the end to less safe code). We are the masters not codacy.

chimp1984

NACK (to last commit bowing down to stupid codacy bot)

I think this is really wrong that we start to write less readible code because of that stupid bot. @ripcurlx Please deactivate codacy until we find a more flexible solution. I find this inacceptable that we cannot override clear mistakes or bad settings in codacy.

oscarguindzberg · 2020-09-19T03:14:36Z

@chimp1984 The last commit (83a1e2f) does not seem wrong or less readable to me, it is just another way to do the same thing. Please, please, don't NACK this PR... it was so hard to be ready to merge. Let's fix codacy in another issue/PR.

chimp1984

utACK

chimp1984 · 2020-09-19T04:11:28Z

Ok, I just got too many times annoyed by a tool which seems to makes us slaves.

cd2357 · 2020-09-19T12:19:11Z

Impossible to ignore, it blocks merging. So either we're changing codacy rules or we write bad code to get around codacy. It's a pity that's it's so annoying.

@sqrrm are you sure?

I played around with Codacy and it can be ignored when merging:

sqrrm · 2020-09-19T12:25:20Z

I guess admins can override codacy, but I'm not an admin so I can't.

cd2357 · 2020-09-19T12:35:34Z

Hmm.

Alternatively, Codacy can be removed from the "branch protection" settings.

In that case, the Codacy checks are only informative and merging is possible without admin permissions.

Which seems like a sane setup.

cd2357 · 2020-09-19T12:39:19Z

Other checks can still be enforced though, like successful travis builds.

sqrrm · 2020-09-19T12:58:08Z

@cd2357 making them recommendations rather than enforced sounds like the more flexible and sane thing to do, all things considered. Thanks for looking that up, @ripcurlx could you make codacy optional? I think it could save some headaches.

boring-cyborg bot added in:altcoins is:no-priority PR or issue marked with this label is not up for compensation right now labels Sep 9, 2020

chimp1984 suggested changes Sep 9, 2020

View reviewed changes

ripcurlx added the is:priority PR or issue marked with this label is up for compensation label Sep 9, 2020

chimp1984 previously approved these changes Sep 9, 2020

View reviewed changes

oscarguindzberg mentioned this pull request Sep 9, 2020

Extract btc to segwit addresses #4509

Merged

oscarguindzberg dismissed chimp1984’s stale review via 44bf0c3 September 14, 2020 16:23

oscarguindzberg force-pushed the bitcoinj_0_15_8_sep_2020 branch from 63dbbbd to 44bf0c3 Compare September 14, 2020 16:23

oscarguindzberg force-pushed the bitcoinj_0_15_8_sep_2020 branch 2 times, most recently from f8e202e to 36219de Compare September 16, 2020 16:17

chimp1984 previously approved these changes Sep 16, 2020

View reviewed changes

sqrrm reviewed Sep 17, 2020

View reviewed changes

oscarguindzberg dismissed chimp1984’s stale review via 7bdae1b September 17, 2020 19:35

chimp1984 previously approved these changes Sep 17, 2020

View reviewed changes

oscarguindzberg dismissed chimp1984’s stale review via 78c1a57 September 17, 2020 20:49

oscarguindzberg requested a review from sqrrm September 17, 2020 21:40

sqrrm previously approved these changes Sep 18, 2020

View reviewed changes

chimp1984 previously approved these changes Sep 18, 2020

View reviewed changes

oscarguindzberg dismissed chimp1984’s stale review via 83a1e2f September 18, 2020 18:24

oscarguindzberg added 11 commits September 18, 2020 18:29

Use bitcoinj 0.15.8 (commit 0d7762b)

d0b00fb

Make it clear P2PKH is used always used

4de39fc

Make it clear P2PKH is used for both BSQ and BTC wallets until segwit is implemented.

Use bitcoinj 0.15.8 (commit 0d98efb)

daaacec

Revert "bitcoinj 0.15: Remove call to PeerGroup.setIgnoreHttpSeeds(tr…

02559f2

…ue)." This reverts commit 9be6a9d.

Code polishing to make Codacy happy

f342c89

Code polishing to make Codacy happier

f1738f0

Remove okhttp and okio dependencies

0d25ca1

bugfix: When creating a new BSQ wallet, use the BTC wallet seed

24da468

Code quality: Use {} in if oneliners

b89e9e1

Break lines at 120 characters

9a7439b

ok codacy, you win

fc70c11

oscarguindzberg dismissed sqrrm’s stale review via fc70c11 September 18, 2020 21:31

oscarguindzberg force-pushed the bitcoinj_0_15_8_sep_2020 branch from 83a1e2f to fc70c11 Compare September 18, 2020 21:31

sqrrm approved these changes Sep 18, 2020

View reviewed changes

chimp1984 suggested changes Sep 19, 2020

View reviewed changes

chimp1984 approved these changes Sep 19, 2020

View reviewed changes

sqrrm merged commit 9735a85 into bisq-network:master Sep 19, 2020

oscarguindzberg mentioned this pull request Sep 21, 2020

For cycle 17 bisq-network/compensation#662

Closed

sqrrm mentioned this pull request Sep 22, 2020

For Cycle 17 bisq-network/compensation#667

Closed

cd2357 mentioned this pull request Sep 22, 2020

Upgrade JavaFX to v15 #4242

Closed

ripcurlx added this to the v1.4.0 milestone Sep 30, 2020

Use bisq's bitcoinj 0.15.8 - september 2020 #4504

Use bisq's bitcoinj 0.15.8 - september 2020 #4504

Conversation

oscarguindzberg commented Sep 9, 2020

oscarguindzberg commented Sep 9, 2020

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

chimp1984 commented Sep 9, 2020

ripcurlx commented Sep 9, 2020

oscarguindzberg commented Sep 9, 2020

chimp1984 commented Sep 9, 2020

chimp1984 left a comment • edited Loading

Choose a reason for hiding this comment

ripcurlx commented Sep 15, 2020

chimp1984 left a comment

Choose a reason for hiding this comment

sqrrm left a 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 Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

chimp1984 left a comment

Choose a reason for hiding this comment

oscarguindzberg commented Sep 17, 2020

oscarguindzberg commented Sep 17, 2020

oscarguindzberg commented Sep 17, 2020 • edited Loading

sqrrm left a comment

Choose a reason for hiding this comment

chimp1984 left a comment

Choose a reason for hiding this comment

chimp1984 commented Sep 18, 2020

sqrrm commented Sep 18, 2020

oscarguindzberg commented Sep 18, 2020

chimp1984 commented Sep 19, 2020

chimp1984 left a comment • edited Loading

Choose a reason for hiding this comment

oscarguindzberg commented Sep 19, 2020

chimp1984 left a comment

Choose a reason for hiding this comment

chimp1984 commented Sep 19, 2020

cd2357 commented Sep 19, 2020

sqrrm commented Sep 19, 2020

cd2357 commented Sep 19, 2020

cd2357 commented Sep 19, 2020

sqrrm commented Sep 19, 2020

chimp1984 left a comment •

edited

Loading

oscarguindzberg Sep 17, 2020 •

edited

Loading

oscarguindzberg commented Sep 17, 2020 •

edited

Loading

chimp1984 left a comment •

edited

Loading