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

Not allow bitcoinj to auto connect to localhost when localhost was not detected by client #3783

Merged

Conversation

@ripcurlx
Copy link
Member

ripcurlx commented Dec 12, 2019

This fixes the problem if the local bitcoin core node is not detected by our client,
but bitcoinj is able to connect to it because of the auto connect to localhost behavior.
In that case the minimum required nodes to broadcast a transaction will be 4 (provided nodes settings),
but bitcoinj will only connect to one node. The requirement of 4 nodes will be never fulfilled and
the transaction never broadcasted.

Testing

As this is a bug based on the default localhost connect behavior of bitcoinj it is difficult to simulate on regtest.
To simulate this behavior on mainnet you could try following steps:

  • Use current master
  • Skip the initial check for a localhost node in BisqSetup step 3
  • Go to network settings and you'll see only one node is connected but the minimum peers for broadcast is still set to 4
  • Checkout this PR and skip the initial check for localhost again
  • Go to network settings and you'll see that the client connects to the provided nodes and will be able to broadcast transactions
…t detected by client

This fixes the problem if the local bitcoin core node is not detected by our client,
but bitcoinj is able to connect to it because of the auto connect to localhost behavior.
In that case the minimum required nodes to broadcast a transaction will be 4 (provided nodes settings),
but bitcoinj will only connect to one node. The requirement of 4 nodes will be never fulfilled and
the transaction never broadcasted.
@ripcurlx

This comment has been minimized.

Copy link
Member Author

ripcurlx commented Dec 12, 2019

I found this issue when I experienced a trade failure myself where the logs showed Waiting for 4 peers required for broadcast, we have 1 ... which was never fulfilled as bitcoinj only connected to my local core node.

Copy link
Contributor

chimp1984 left a comment

utACK

@julianknutsen

This comment has been minimized.

Copy link
Contributor

julianknutsen commented Dec 12, 2019

This would only fail for users that run full bitcoind nodes locally, right?

What is the workaround if a user encounters this? restart?

Is waiting for the Jan release sufficient or is this a hotfix candidate?

@ripcurlx

This comment has been minimized.

Copy link
Member Author

ripcurlx commented Dec 12, 2019

This would only fail for users that run full bitcoind nodes locally, right?

Yes

What is the workaround if a user encounters this? restart?

Should solve the problem also if you have a transaction already in limbo. Unfortunately for me it wasn't solved with a restart and after a SPV resync the transaction was removed by bitcoinj.

Is waiting for the Jan release sufficient or is this a hotfix candidate?

I don't think this is something new, so I think it should be fine to ship it in the next release.

@chimp1984

This comment has been minimized.

Copy link
Contributor

chimp1984 commented Dec 13, 2019

Is waiting for the Jan release sufficient or is this a hotfix candidate?

I think that happens only in edge cases. At startup we connect to localhost (BisqSetup.checkIfLocalHostNodeIsRunning) on the default port and if that succeeds we set a flag to use local Bitcoin Core. There seems to be cases where that check fails even a Bitcoin Core is funning at localhost and we should find out what causes that. I could imagine some race conditions when both Bisq and Bitcoin Core are start up at same time could cause that as well if Bitcoin Core is syncing (this should be detected as well as BitcoinJ cannot sync reliably then).

@ripcurlx ripcurlx mentioned this pull request Dec 13, 2019
@sqrrm
sqrrm approved these changes Dec 16, 2019
Copy link
Member

sqrrm left a comment

utACK

@sqrrm sqrrm merged commit 65d3dd3 into bisq-network:master Dec 16, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ripcurlx ripcurlx added this to the v1.2.5 milestone Dec 17, 2019
@ripcurlx ripcurlx deleted the ripcurlx:only-connect-to-localhost-when-detected branch Dec 18, 2019
@sqrrm sqrrm mentioned this pull request Jan 13, 2020
@ripcurlx ripcurlx mentioned this pull request Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.