bisq-network / bitcoinj Goto Github PK

I have rebased the bisq specific commits on to 0.15.3 https://github.com/bodymindarts/bitcoinj/tree/bisq_0.15.3 and would like to contribute it back to this repo.

As I do not have push access I cannot create a new branch and cannot open a PR against a branch that doesn't exist.

Perhaps someone with access could do the following:

git clone https://github.com/bitcoinj/bitcoinj.git && cd bitcoinj
git remote add bisq https://github.com/bisq-network/bitcoinj.git
git checkout v0.15.3
git checkout -b bisq_0.15.3
git push -u bisq bisq_0.15.3

Then I can open a PR against that branch to add the bisq specific commits.

In progress

• Merge to bisq project PR for using the latest bitcoinj 0.14.7 version with improvements on connection handling based on audit (useless if we migrate to bitcoinj 0.15)
  • Audit: #30 (completed)
  • Fixes: #31 (completed)
  • PR to use that in bisq bisq-network/bisq#2618 (merging pending)
• Migration to bitcoinj 0.15
  • Oscar reviewed bitcoinj 0.15 release release notes https://bitcoinj.github.io/release-notes and everything looks ok.
  • Rebase bisq’s bitcoinj from bitcoinj 0.15.1 (completed)
    • https://github.com/bisq-network/bitcoinj/commits/bisq_0.15.1 (completed).
    • All commits added by me have a description linking to the commit on the 0.14.7 branch that
      originated it.
    • Many commits were combined.
    • Many commits were already included on upstream bitcoinj 0.15.1
  • Use bisq's bitcoinj 0.15 in bisq (merging pending)
    • bisq-network/bisq#2772
• Deprecated transaction confidence / broadcasting problem research
  • bisq bug: bisq-network/bisq#2639
  • bitcoinj upstream issue: bitcoinj#1769 (waiting for Andreas Schildbach feedback)
  • Tests to prove the problem on bitcoinj upstream: https://github.com/oscarguindzberg/bitcoinj/commits/tx-confidence-cache
  • Found out setting confidence to null in Transaction.unCache() is the best short term solution for bisq’s bitcoinj (upstream bitcoinj probably needs a more comprehensive solution). bisq’s bitcoinj 0.14.7 and 0.15.1 are already updated to include that fix
  • PR for Bisq to use the latest version of bitcoinj 0.14.7 and 0.15.1 (merging pending)
    • bisq-network/bisq#2772 now uses bitcoinj 0.15.1 that includes the fix
    • bisq-network/bisq#2618 now uses bitcoinj 0.14.7 that includes the fix
• PeerGroup.handlePeerDeath() + BlockingClient.socket.close() called twice
  • Upstream bug bitcoinj#1765
  • Someone has to look at the bug again and confirm whether the bug described matches reality and then implement a solution.
  • I guess this is not causing big problems for Bisq.
  • The analysis is based on bitcoinj 0.14 but I guess it is still valid for bitcoinj 0.15.
• "Restore from seed" on "wallet template" upstream bitcoinj project is broken.
  • Upstream bug: bitcoinj#1704
  • Waiting for someone to fix the bug.
  • It is important for Bisq that "restore from seed" works on "wallet template" because it is a benchmark on how bisq "restore from seed" should work.
• Path issue with serialized wallets migrated from pre-0.15 to 0.15
  • Upstream bug bitcoinj#1812
  • A workaround was implemented on bisq by using BisqKeyChainFactory and isqKeyChainGroupStructure, but a generic upstream solution would be better.

TODO (i.e. not started yet)

• Review bisq usage of bitcoinj
• Segwit support
  • Support sending/receiving tp/from "bc1" addresses once bitcoinj 0.15 usage is merged.
• Improve multi-wallet support in bitcoinj
  • Spin off from #18
  • See bitcoinj mailing list - Design doc: separating confidence out from transaction objects https://groups.google.com/forum/#!topic/bitcoinj/KsEWEnSdbN0
  • See bitcoinj#877
  • See bitcoinj#1553
• Run tor over nio in bitcoinj
  • Florian should know a lot about this.
• Implement tor / onion address support in bitcoinj upstream.
• Improve re-broadcasting of bitcoinj txs
  • Rebroadcasting occurs just at app restart, there should be some rebroadcasting while the app is running. E.g. when peers have no broadcasted it back after 45 seconds and/or when tx is not mined after 1 hour.
  • See bitcoinj mailing list https://groups.google.com/forum/#!topic/bitcoinj/lkK9N3NhoPY
  • See “reject” msg removal in bitcoin core mailing list https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-March/016701.html
• Fix BitcoinJ bloom filters broken privacy
  • https://groups.google.com/forum/#!topic/bitcoinj/vodUFpdmskg
  • bisq-network/bisq#1062
  • https://www.coindesk.com/spv-support-billion-bitcoin-users-sizing-scaling-claim
  • bitcoin/bitcoin#11863

• Commits to audit
  • 659f3c1
• How it works now
  • PeerGroup: When "addr" msg is received, add a new peer to the "inactives" collection.
  • PeerGroup property addPeersFromAddressMessage can be used to disable the feature.
  • bisq/bitcoinj node never sends “getaddr” msgs.
  • bisq/bitcoinj node receives from time to time unsolicited “addr” msgs from peers.
  • bisq sets addPeersFromAddressMessage to false (disables the feature) if preferences.getBitcoinNodes() is not empty.
• upstream bitcoinj support for addr/getaddr messages
  • upstream bitcoinj does not implement this feature
    • Peer class can send "getaddr" msg and process incoming "addr" msg, but the feature is not used.
    • This is a missing feature on bitcoinj.
  • Related work on upstream bitcoinj
    • https://groups.google.com/d/msg/bitcoinj/3Y93LIDfJaM/SOYnpM9hlkYJ
    • bitcoinj#2
  • New conversation I started: https://groups.google.com/forum/#!topic/bitcoinj/W3gkXT7Hk2M
• Bitcoin core support: https://en.bitcoin.it/wiki/Satoshi_Client_Node_Discovery
• Comments by Dan (commit author)
  • I modified bitcoinj to listen to the addr p2p messages and add nodes dynamically. I was rather surprised mainline bitcoinj didn't already do that since it is standard bitcoind functionality.
  • This was in context of enabling bitcoinj p2p traffic to work over Tor, including DNS lookups which was a particularly tricky aspect.
  • btc nodes come and go, so hard-coded seeds become unavailable over time. thus unreliable.
    • Oscar comment: it's true, but same non critical issue is present on every app that uses bitcoinj.
  • DNS provided nodes are a tiny subset of total bitcoin p2p network, and using only them is a point of centralization. dns maintainers become gatekeepers.
    • Oscar comment: but same non critical issue is present on every app that uses bitcoinj.
  • DNS over Tor is slow and some record-sets from bitcoin DNS servers are too large, which result in empty result set. (if memory serves, sipa's dns node was one such.) In prior testing, we would sometimes have issues with few connections being made to p2p network, and then it never corrects itself back up to the target 8 or 10. Using the addr message fixed that issue as I recall.
    • Oscar comment: Dan has a point here. Since Bisq uses by default the curated btc nodes harcoded in the bisq code instead of dns discovery, this is not as critical as it could be.
  • Bitcoin DNS servers do not provide .onion addresses. So addr messages are best way to become informed of nodes running behind Tor.
    • Oscar comment: Why is important to connect to btc nodes running behind tor?.
  • It is a way to bootstrap p2p connections from a local peer, eg bitcoind on same machine or subnet, even if DNS is being flaky/slow over Tor.
    • Oscar comment: If a user is connected to a local bitcoind, she probably does not want to connect to any other bitcoin node for privacy/trust issues.
  • It's a part of the bitcoin p2p protocol. Not implementing it in bitcoinj seems incomplete at best.
    • Oscar comment: agree.
  • It was not hard to do. low hanging fruit.
    • Oscar comment: See "Implementation problems/limitations" section.
• Implementation problems/limitations.
  • If a user wants to connect just to the Bisq curated node list in BtcNodes.java for privacy issues, this mechanism might have her connected to other nodes. This could probably be solved by improving when PeerGroup.setAddPeersFromAddressMessage() is set to true.
  • getaddr messages are never sent, so bitcoinj depends on the unsolicited "addr" msgs peers send from time to time.
  • An attacker could send a million "addr" msgs with fake peers causing a DoS on the bitcoinj node.
  • Received addresses are not persisted.
  • Full support of peer discovery using getaddr/addr msgs might be difficult to implement. See upstream bitcoinj attempts to do it detailed above.
• Actions:
  • Disable "add peers from addr message" custom bitcoinj feature when Bisq curated node list in BtcNodes.java is used: bisq-network/bisq#2591
  • Add a link to this audit in bitcoinj source code: #29

Based on log analysis, we believe that bitcoinj's lack of support for v2 Bitcoin transactions [1] may have led to some of the timeout issues we've seen recently.

Apparently, support has been implemented, but has not yet been officially released in the canonical bitcoinj project.

• Merge this support into our own fork
• Upgrade to it in bisq-network/exchange
• Test it
• Tag v0.14.5.1 of bisq-network/bitcoinj
• Upgrade to v0.14.5.1 of bisq-network/bitcoinj in v0.6.3 of bisq-network/exchange

[1] https://github.com/bitcoin/bips/blob/master/bip-0068.mediawiki

We need the block time when a tx is confirmed.
The code in Transaction.setBlockAppearance does not set the updatedAt if it was set before.
As Wallet.commit sets the updatedAt the update never happes.

See: https://groups.google.com/d/msg/bitcoinj/05TJbxAwKXs/D6CYV1byAgAJ

The problem

bisq has to be built using jdk 10. bitcoinj has to be built using jdk 8.

Details

bitcoinj 0.14.4+ uses sun.misc.Cleaner.
sun.* packages should not be used directly as a rule of thumb.
bitcoinj uses them to fix some windows specific issue.
Using sun.* seemed to be the lesser evil.
The thing is sun.misc.Cleaner was removed in jdk 9.
bitcoinj does not compile if you use jdk 9, 10 or 11.

The problem was discussed on several bitcoinj mailing list msgs and github issues. The most relevant discussion happened here: bitcoinj#1477

Andreas Schildbach eventually removed the windows specific hack, which eliminates the need to use sun.misc.Cleaner (he thinks the latest versions of windows may not reproduce the bug)
Fix:
bitcoinj@1700ec0
Test:
bitcoinj@dbc4c35
Andreas Schildbach did the change on bitcoinj master (not the bitcoinj fork we use)
So, bitcoinj master can be built using jdk 8, 9, 10 or 11.
Nobody confirmed yet that without the hack, bitcoinj does not reproduce the bug on windows.
Even if someones confirm she is running bitcoinj on windows without problems, that does not mean it will run on every windows environment.

The feature that has the windows problem is marked to be refactored in bitcoinj
https://github.com/bitcoinj/bitcoinj/blob/master/core/src/main/java/org/bitcoinj/store/SPVBlockStore.java#L33

How to solve it

Current solution

• Devs install both jdk8 and jdk10. Tell intellij to use jdk8 for bitcoinj and jdk10 for bisq. Use a bash script to switch jdk versions when the dev needs to run maven/gradle on the command line.
  • Pros
    • Oscar has the "hack" to switch jdks set up, so it is not a big issue for him to use this "hack".
  • Cons
    • Other devs coding both bisq and bitcoinj need to be aware of this hack and adapt it to their OS if they don't run on Mac.

Other possible solutions

• cherry pick bitcoinj@1700ec0 to bisq’s bitcoinj fork.
  • Pros
    • Easy to implement.
    • Devs coding both bisq and bitcoinj don't have to switch jdks - jdk 10 is ok for both projects.
  • Cons
    • Risk of bisq users having problems when running on windows (Risk could be partially reduced if we test on Windows).
• Devs cherry pick bitcoinj@1700ec0 fix locally. Devs use git stash and other git hacks to avoid pushing that change.
  • Pros
    • Easy to implement.
    • Devs coding both bisq and bitcoinj don't have to switch jdks - jdk 10 is ok for both projects.
  • Cons
    • It is a nuisance to make sure some local changes are not included every time you commit.
• Invest some time to refactor the bitcoinj feature that has the windows problem.
  • Pros
    • Solve the problem for good.
    • Devs coding both bisq and bitcoinj don't have to switch jdks - jdk 10 is ok for both projects.
  • Cons
    • May require a considerable amount of time to do the refactor.

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 #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
    • #8
    • bitcoinj#1679
    • 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 #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 bisq-network/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 #18 and #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
    • #11
    • 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

Overview

• Changes done on bisq's bitcoinj just support onion v2 addresses.
• Most of the code needed to support tor/onion services is in https://github.com/bisq-network/bisq (out of scope of this audit)
• Upstream tor support in bitcoinj was removed because it was not well maintained and caused problems and there were no volunteers to maintain it (bitcoinj#1314)
• Tor support could be re-enabled upstream by contributing changes done on bisq's bitcoinj and on https://github.com/bisq-network/bisq
• Tor support upstream could provide
  • Connect to peers running behind an onion address
  • Resolve DNS over tor
  • Connect via tor to clearnet peers
  • A mechanism to start a local tor client (orchid or another tor client)

Commit review

• PeerGroup.addAddress(): don't increase max connections if address already exist 011fe40
  • No changes suggested.
  • Created PR upstream bitcoinj#1741
• Catch PeerDiscoveryException during getPeers call. avoids stack trace when all DNS lookups timeout 834f57a
  • No changes suggested.
  • Created PR upstream bitcoinj#1742
• PeerAddress hashCode() and equals(): add hostname and refactor 0988148
  • No changes suggested.
• Changes to support connecting to .onion addresses 9f09a89
  • See #24

These tests fail on bisq's bitcoinj fork and need to be a) fixed or b) ignored and documented.

PeerTest

• Fails because of oscarguindzberg@f76a9da
• writeTarget.writeBytes() is used to check the connection was in fact closed. Implementation before bisq change used to throw an exception if connection was already closed.

DeterministicKeyChainTest

• Ignored here oscarguindzberg@7712e3a
• Fails becauses protobuf java was upgraded from 2.6.1 to 3.2.0 here oscarguindzberg@1c36ff9

I have not looked closer but that might be a critical issue we should get merged to our fork:

Transaction can serialize and deserialize according to Segwit (BIP144)
bitcoinj@b9c983e

@cbeams @ripcurlx Maybe we should try to get that into the next release as well.

• I found a couple of problems that are not related to changes done by bisq on bitcoinj. Here they are:
  • On Bisq project: Different timeouts should be used for socket connection and version message exchange.
    • Use different timeouts because version message exchange with the peer by definition happens after connecting the socket to the peer.
    • See bisq-network/bisq#2609
  • PeerGroup.handlePeerDeath() is called twice for the same peer.
    • The problem is the peer will be added twice to the inactives collection. See: inactives.offer(address)
    • I could reproduce this by running bisq when bitcoinj fails to connect to an onion peer
    • I see 2 “Peer died” on the log for the same peer, which means handlePeerDeath() has been invoked twice.
    • Action
      • As a temporary workaround: PeerGroup.handlePeerDeath(): check for duplicates before calling inactives.offer(address). See oscarguindzberg@2d7c24c
      • Report upstream bitcoinj#1765
• Commit review
  • PeerGroup: Add check to not duplicate peers
    • Commit: 91ab651
    • Manfred’s comment: can be reproduced if u connect to provided bisq nodes and disconnect/reconnect several times. u will get duplicated nodes without that fix.
    • Oscar’s comments:
      • Changes on triggerConnectionsJob
        • addrToTry was just removed from inactives and is being re-added, so there is no chance for duplication here, so no need to check for isAlreadyAdded(). This change probably caused a positive effect because it prevents re-adding a peer if it was duplicated already.
        • Action: Revert change. See oscarguindzberg@b8d1534
      • isAlreadyAdded() method
        • I understand PeerAddress.equals() is not trustworthy since PeerAddress.equals() includes comparison of “time” field which could be different if informed by different peers in “addr” msgs.
        • Problems
          • It just compares by hostname, 2 peers could be run on the same host and different port.
          • PeerAddress could use addr (InetAddress) instead of hostname so the comparison will not work on this case.
        • Actions:
          • Replace usages of PeerAddress.equals() by PeerAddress.equalsIgnoringMetadata()
          • See
            • oscarguindzberg@e06453c
            • oscarguindzberg@3834e1f
            • oscarguindzberg@a1ed4be
      • Change on addInactive()
        • backoffMap.containsKey(peerAddress) already checks for peer existence, there is no need to add an extra check to see if the peer is part of the inactives collection.
        • Action: Revert change. See oscarguindzberg@26889a8
  • PeerGroup: Limit number of Bitcoin network peers at re-connect after connection loss
    • Commit: 3f1382d
    • I could reproduce the bug running the code without the fix.
    • Not sure what is the reason for the bug, but the fix for sure does not cause any harm.
    • Action: keep the commit, just use wrapper methods, see oscarguindzberg@f36a1ea
  • PeerGroup: Add additional check isRunning() at handlePeerDeath to avoid errors at shutDown when using BlockingClient
    • Commit: f76d341
    • The idea is ok, but the implementation is wrong: When the PeerGroup is stopped, first peers are stopped and then vRunning is set to false, specially in BlockingClientManager case, so isRunning() is going to return true during shutdown.
    • Action: Revert change and use upstream solution instead: PeerGroup.stopAsync() set downloadPeer to null before channels are stoped. So, there is no download peer replacement on handlePeerDeath(). See oscarguindzberg@630662f
  • Do not close socket if it is already closed. (Should be called: Do not write to socket if it is already closed)
    • Commit: b90bce9
      • The “if” to not write the socket if the connection is ok. the catch code kind of duplicates the “if”.
      • Action: Remove the special catch, add an “else” statement printing a warning “writing to a closed socket”. See oscarguindzberg@4baf363
• Extra
  • Testing Bisq using all the changes suggested on this audit, I still find bitcoinj errors on the log when the Bisq node is being shut down. I think the problem is the Tor node is shutdown before bitcoinj is told to shutdown, then connection to btc peers fail.
• PR containing the fixes: #31