[bisq-network/bitcoinj] [WIP] bisq's bitcoinj fork changes audit (#20)

Oscar Guindzberg notifications at github.com
Fri Jan 4 18:00:55 UTC 2019


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

Here are the conclussions:

- Already implemented on https://github.com/oscarguindzberg/bitcoinj/commits/bisq_0.14.7-review
  - https://github.com/oscarguindzberg/bitcoinj/commit/bcafe50f2c07d746cf5c3a159f39361597c766bd 
    - Commit made by mistake (getActiveKeychain method added where getActiveKeyChain was there with capital C)
    - Action: Reverted commit.
  - https://github.com/oscarguindzberg/bitcoinj/commit/b2e11ecb5af665256c4bf60d5d119e740a49f190
    - 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.      
  - Extra logging
    - https://github.com/oscarguindzberg/bitcoinj/commit/fc7c58765a7a0f438223023be5a169aab3fec4b5  
    - https://github.com/oscarguindzberg/bitcoinj/commit/eeabb880cdd0bd6683156219f0656407d49fa20e 
    - https://github.com/oscarguindzberg/bitcoinj/commit/befee2f6f1d2e6d2a4a1d4f98f86e507a16c3ba2 
    - Action: Discussed with Manfred and decided to keep those commits for debugging purposes.
  - https://github.com/oscarguindzberg/bitcoinj/commit/f9bc2f776e36c53d335b908e49f58d432d6ab1f1
    - Manfred suggested not to use Jeff Garzik's dns seed.
    - Action: Keep commit. I also removed Bloq's dns seed https://github.com/oscarguindzberg/bitcoinj/commit/b3b3b6457458f513766aeb0d70ead8237686fe87 
  - PeerDiscovery
    - https://github.com/oscarguindzberg/bitcoinj/commit/57928fc95750d8441bce385c0ff32b6965ed3fb7
      - Seed nodes update.
      - Action: Keep commit. I re-updated seed nodes on Dec 2018: https://github.com/oscarguindzberg/bitcoinj/commit/524cbc63bc8878119fc0e75231397f65384adcc0 
    - https://github.com/oscarguindzberg/bitcoinj/commit/9d040713325980d42366aad4d853dd78ed33d541
      - 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. 
  - https://github.com/oscarguindzberg/bitcoinj/commit/09ae4b1398d3c4ced4d64b2b35fabd3d678d0e56 
    - This is required to be able to create both btc and bisq wallets.
    - Action: Keep commit.
  - https://github.com/oscarguindzberg/bitcoinj/commit/eed7f70f291788426caa93c5f6ced0f23036c1eb 
    - Style/clean-up/formatting changes should be avoided so it is easier to import new code from upstream.
    - Action: Revert commit.
  - https://github.com/oscarguindzberg/bitcoinj/commit/1c36ff94c0cb2b800964317565f562b0c397e74d 
    - 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: https://github.com/oscarguindzberg/bisq/commit/06f47d84caeeef1c973e8902050989a0e91a1784 
  - Fix failing tests on bisq's bitcoinj branch 
    - See https://github.com/bisq-network/bitcoinj/issues/19 
    - PeerTest fails
      - Fails because of https://github.com/oscarguindzberg/bitcoinj/commit/f76a9daac3ba119057b5e663a8f185334c5fbeec
      - 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: https://github.com/oscarguindzberg/bitcoinj/commit/ec029d843127512f6a86cabc5e1406c45295e714
    - https://github.com/oscarguindzberg/bitcoinj/commit/9b999c8eca130a4fc73db155dc37507436735dab
      - Tests fail without the "Ignore" because https://github.com/oscarguindzberg/bitcoinj/commit/1c36ff94c0cb2b800964317565f562b0c397e74d 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. 
  - https://github.com/oscarguindzberg/bitcoinj/commit/05c64329e30021f5357c9cb4c95cf1847ede1e2c
    - Manfred could not remeber why this was necessary.
    - Action: Revert the commit. See if the reason for change will pop up again later.
  - https://github.com/oscarguindzberg/bitcoinj/commit/7cb2f4e57f92bcc44d3164d59219476ec7fd64f6
    - Set monetaryFormat to BTC
    - Action: Keep commit. 
  - jdk jce workarounds 
    - https://github.com/oscarguindzberg/bitcoinj/commit/d47a3d44b33618934e2bbdf191c70f01bff72589
      - Action: Replace by a more comprehensive solution https://github.com/bitcoinj/bitcoinj/commit/b9c2b61712b84f0de78c732b7d3561aeba39ae2a
    - https://github.com/oscarguindzberg/bitcoinj/commit/aace35dcd71f3e4e6649c599e51d5dd25c3c5028
      - 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
      - https://github.com/bisq-network/bitcoinj/pull/8
      - https://github.com/bitcoinj/bitcoinj/pull/1679 
      - https://github.com/bitcoinj/bitcoinj/pull/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 https://github.com/bisq-network/bitcoinj/issues/17
  - https://github.com/oscarguindzberg/bitcoinj/commit/ec28f6b2ef5df8deb8a128567b63ade08e480bec 
    - Wallet: Add printAllPubKeysAsHex()
    - I evaluated implementing that outside bitcoinj, but it can not be done without doing other changes.
    - Action: Keep commit. 
  - https://github.com/oscarguindzberg/bitcoinj/commit/006821a05326b3d1b81edeec73239e033dafd952#diff-b5705bce28d8b7c33a69b5c5c42ca901
    - 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. 
  - https://github.com/oscarguindzberg/bitcoinj/commit/5b35c8412d547b2e2dbd0e77b22c75081875b0ff
    - Dash specific change
    - Action: Keep commit, could be removed once libdohj is removed.  


-- 
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/bitcoinj/issues/20
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20190104/b59a63c6/attachment-0001.html>


More information about the bisq-github mailing list