Bisq's bitcoinj general changes audit

[oscarguindzberg]

I reviewed the changes done by bisq's to bitcoinj.

Here are the conclusions:

Already reviewed

Fixes implemented on https://github.com/bisq-network/bitcoinj/tree/bisq_0.14.7

• oscarguindzberg@bcafe50
  • Commit made by mistake (getActiveKeychain method added where getActiveKeyChain was there with capital C)
  • Action: Reverted commit.
• oscarguindzberg@b2e11ec
  • No longer needed, since now p2p alert messages are logged with log.debug(). "log" level does not reach bisq log because only "info" level and above is printed.
  • Action: Revert commit.
• Style rules
  • oscarguindzberg@eed7f70
  • oscarguindzberg@76cf53e
  • oscarguindzberg@da9faae
  • oscarguindzberg@36b3e25
  • oscarguindzberg@f31daee
  • oscarguindzberg@3be463f
  • Style/clean-up/formatting changes should be avoided so it is easier to import new code from upstream.
  • Action: Revert commit.
• Disable unnecessary logging
  • oscarguindzberg@bdc84b5
  • oscarguindzberg@fd3a526
  • Action: Discussed with Manfred and decided to keep those commits to avoid showing log messages that might scare out the user but in fact are not real problems.
• Extra logging
  • oscarguindzberg@fc7c587
  • oscarguindzberg@eeabb88
  • oscarguindzberg@befee2f
  • Action: Discussed with Manfred and decided to keep those commits for debugging purposes.
• oscarguindzberg@f9bc2f7
  • Manfred suggested not to use Jeff Garzik's dns seed.
  • Action: Keep commit. I also removed Bloq's dns seed oscarguindzberg@b3b3b64
• PeerDiscovery
  • oscarguindzberg@57928fc
    • Seed nodes update.
    • Action: Keep commit. I re-updated seed nodes on Dec 2018: oscarguindzberg@524cbc6
  • oscarguindzberg@9d04071
    • Manfred comment: Http seeds have some privacy issues (peter todd discussed that on old btc mailing list with mike hearn - dns caches so it makes logging on server less effective, with a http seed you can easier log users IPs). We don't nor need http seeds, and prefer to not add a privacy vulnerability without reason for its need.
    • Action: Keep commit.
• oscarguindzberg@09ae4b1
  • This is required to be able to create both btc and bisq wallets.
  • Action: Keep commit.
• oscarguindzberg@1c36ff9
  • As a rule of thumb, changes to bitcoinj should be avoided so it is easier to import new code from upstream.
  • This change could be replaced by the use of exclude for bitcoinj dependencies in bisq’s build.gradle
  • Action: Revert the commit. Update build.gradle: oscarguindzberg/bisq@06f47d8
• Fix failing tests on bisq's bitcoinj branch
  • See Failing tests #19
  • PeerTest fails
    • Fails because of oscarguindzberg@f76a9da
    • writeTarget.writeBytes() is used to check the connection in fact was closed
    • Implementation before the commit used to throw an exception if connection is closed().
    • Action: Add "Ignore" to the test until review of connection handling is done: oscarguindzberg@ec029d8
  • oscarguindzberg@9b999c8
    • Tests fail without the "Ignore" because oscarguindzberg@1c36ff9 updated the protobuf version from 2.6.1 to 3.2.0.
    • Since that commit is going to be reverted, there is no need to "Ignore" the tests.
    • Action: Revert commit.
• oscarguindzberg@05c6432
  • Manfred could not remeber why this was necessary.
  • Action: Revert the commit. See if the reason for change will pop up again later.
• oscarguindzberg@7cb2f4e
  • Set monetaryFormat to BTC
  • Action: Keep commit.
• jdk jce workarounds
  • oscarguindzberg@d47a3d4
    • Action: Replace by a more comprehensive solution bitcoinj@b9c2b61
  • oscarguindzberg@aace35d
    • Not needed. See https://stackoverflow.com/questions/1179672/how-to-avoid-installing-unlimited-strength-jce-policy-files-when-deploying-an . jdk 9 should not have problems with DRM.
    • Action: Revert commit.
  • Useful links
    • https://github.com/bitcoinj/bitcoinj/blob/828036cb4a52263972887ad7e1f2c4cb32d72ccd/core/src/main/java/org/bitcoinj/crypto/DRMWorkaround.java
    • Fix travis build #8
    • DRMWorkaround: JceSecurity.isRestricted remove final modifier bitcoinj/bitcoinj#1679
    • DRMWorkaround: Skip workaround on OpenJDK. bitcoinj/bitcoinj#1680
    • https://stackoverflow.com/questions/1179672/how-to-avoid-installing-unlimited-strength-jce-policy-files-when-deploying-an
• jdk8 vs jdk10 problem
  • That is worth its own issue jdk8 vs jdk 10 #17
• oscarguindzberg@ec28f6b
  • Wallet: Add printAllPubKeysAsHex()
  • I evaluated implementing that outside bitcoinj, but it can not be done without doing other changes.
  • Action: Keep commit.
• oscarguindzberg@006821a
  • Wallet.toString(): Include includeLookahead keys
  • Why show Wallet.toString() result to users? Manfred: with cmd+j users can see wallet data, Wallet.toString() is used. It's helpful for debugging and techie users who what to see raw wallet data.
  • Why print keys that have not been shown to users? Manfred: helps for some debugging cases. Some addresses are reserved for the trade, so those are not used in a tx yet but get the flag or a trade purpose (e.g. payout, multisig,….). Also bisq does not allow (yet) in the ui to display more then 1 unused address. Sometimes there is need for getting more unused addresses and then the cmd+j wallet tool can be used to find other unused addresses. thought we should remove that limitation in the receive funds screen and allow users to “create” (display) more unused addresses. there is a GH issue for that.  
  • Action: Keep commit.
• oscarguindzberg@5b35c84
  • Dash specific change
  • Action: Revert commit, not needed anymore now libdohj is removed (See Remove libdohj dependency bisq#2368).
• oscarguindzberg@05b62a5
  • Risk analysis prevents adding to the wallet (and displaying on the UI) unconfirmed txs that will probably never be confirmed (there is still a chance they get confirmed).
  • isStandard() check on bitcoinj is in WIP stage and has not been updated as core was updated. These are some checks that are not implemented / wrong:
    • Non-dust segwit output value should be 294 satoshis (as opposed to 546).
    • These checks are missing: check only one OP_RETURN output is present, check tx max size, check script sig max size.
  • To compare bitcoinj and bitcoin core implementations, see bitcoinj's RiskAnalysis and bitcoin core’s validation.cpp AcceptToMemoryPoolWorker and isStandardTx().
  • This commit excludes OP_RETURN outputs from the MIN_ANALYSIS_NONDUST_OUTPUT check.
  • Manfred discussed the change with Andreas Schildbach: https://groups.google.com/forum/#!searchin/bitcoinj/Manfred$20Karrer|sort:date/bitcoinj/0hHDQvoCc_o/6YYI4woyAwAJ
  • The commit is correct and solves part of the problem (the part of the problem that constantly annoys bisq users because BSQ txs use OP_RETURN)
  • Action: Don't adapt bitcoinj to match bitcoin core. Keep the commit as is.
• Count confidence relative to latest block height.
  • Problem: One transaction can belong to multiple wallets so it would count the depth twice for each block. The problem used to be reproduced on bisq since it uses we use bsq+btc wallets.
  • Solution: Count depth as current block height - seen block height + 1
  • Commits
    • oscarguindzberg@ba5e10f
    • oscarguindzberg@5ed2576
    • oscarguindzberg@6ebcdd5
  • See Clone transaction before adding to wallet #18 and [WIP] Clone transaction before committing to wallet #21 for discussions on more comprehensive solutions.
  • Action: Squash and keep the commits.
• Fix issue: Transaction.updatedAt is not set at new block
  • oscarguindzberg@fbf4c7e
  • This commit changes the semantic of updatedAt
  • See
    • https://groups.google.com/forum/#!msg/bitcoinj/05TJbxAwKXs/eeCJeuvJDwAJ
    • https://groups.google.com/forum/#!topic/bitcoinj/Sb42044BGkg
    • Transaction.updatedAt is not set at new block #11
    • Transaction.updatedAt is not set at new block bitcoinj/bitcoinj#1507
    • https://github.com/bitcoinj/bitcoinj/blob/d953ced801ef425de8ac4fe14447255bc5e383f8/core/src/main/java/org/bitcoinj/core/Transaction.java#L136
  • Action: Revert commit and create Transaction.includedInBestChainAt variable
  • Transaction.includedInBestChainAt source code
    • oscarguindzberg@cc774c5
    • oscarguindzberg/bisq@2f324c4

Still to be reviewed

• tor/onion services changes
  • oscarguindzberg@4bb73f8
  • oscarguindzberg@0424c08
• Peer discovery
  • oscarguindzberg@b0290ad
• Fee calculation changes
  • oscarguindzberg@7dd1bfd
• connection handling changes
  • oscarguindzberg@91ab651
  • oscarguindzberg@84ac33a
  • oscarguindzberg@66f4441
  • oscarguindzberg@f76a9da