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

[WIP] Use bisq's bitcoinj 0.15.1 #2772

Closed

Conversation

oscarguindzberg
Copy link
Contributor

No description provided.

@ManfredKarrer ManfredKarrer removed the request for review from cbeams April 23, 2019 23:12
@ManfredKarrer ManfredKarrer changed the title Use bisq's bitcoinj 0.15.1 [WIP] Use bisq's bitcoinj 0.15.1 Apr 23, 2019
@ManfredKarrer
Copy link
Contributor

I added WIP so that it does not get merged accidentally. I will not have time soon for review and it will require a solid review. We also need to deplay soon versions with the security improvements against the scammer and i prefer to not add a big change like that in that period. Sorry for that delay, looking forward to get it in but unfortunately a bit bad timing atm....

@nopara73
Copy link

I added WIP so that it does not get merged accidentally.

Just a heads up: GitHub added Draft PRs those prevent merging. (Although you cannot downgrade a PR to be Draft, you have to start out with that.)

@ManfredKarrer
Copy link
Contributor

Just a heads up: GitHub added Draft PRs those prevent merging. (Although you cannot downgrade a PR to be Draft, you have to start out with that.)

Thanks. We are using that once the PR maker knows that it cannot be merged soon. In that case @oscarguindzberg did not had the information about the current priority changes so I needed to add the WIP afterwards...

Copy link
Contributor

@ManfredKarrer ManfredKarrer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

I leave it to @ripcurlx when to merge as it require a profound test cycle for all wallet use cases and DAO tx use cases.

@ManfredKarrer ManfredKarrer changed the title [WIP] Use bisq's bitcoinj 0.15.1 Use bisq's bitcoinj 0.15.1 May 14, 2019
@devinbileck
Copy link
Member

I ran into the following error when launching a seednode (as a DAO full node) on regtest. It appears to be caused by updating to guava 27. We need to update btcd-cli4j to use guava 27 as well.

May-17 16:15:57.793 [RpcService] ERROR b.c.d.n.f.RpcService: java.lang.NoSuchMethodError: com.google.common.util.concurrent.Futures.addCallback(Lcom/google/common/util/concurrent/ListenableFuture;Lcom/google/common/util/concurrent/FutureCallback;)V 
java.lang.NoSuchMethodError: com.google.common.util.concurrent.Futures.addCallback(Lcom/google/common/util/concurrent/ListenableFuture;Lcom/google/common/util/concurrent/FutureCallback;)V
    at com.neemre.btcdcli4j.daemon.BtcdDaemonImpl.startMonitors(BtcdDaemonImpl.java:213)
    at com.neemre.btcdcli4j.daemon.BtcdDaemonImpl.<init>(BtcdDaemonImpl.java:53)
    at bisq.core.dao.node.full.RpcService.lambda$setup$2(RpcService.java:136)
    at com.google.common.util.concurrent.TrustedListenableFutureTask$TrustedFutureInterruptibleTask.runInterruptibly(TrustedListenableFutureTask.java:125)
    at com.google.common.util.concurrent.InterruptibleTask.run(InterruptibleTask.java:57)
    at com.google.common.util.concurrent.TrustedListenableFutureTask.run(TrustedListenableFutureTask.java:78)
    at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1135)
    at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
    at java.base/java.lang.Thread.run(Thread.java:844)
May-17 16:15:57.795 [SeedNodeMain] ERROR b.c.d.n.f.FullNode: An error occurred: Error=bisq.core.dao.node.full.RpcException: java.lang.NoSuchMethodError: com.google.common.util.concurrent.Futures.addCallback(Lcom/google/common/util/concurrent/ListenableFuture;Lcom/google/common/util/concurrent/FutureCallback;)V 
bisq.core.dao.node.full.RpcException: java.lang.NoSuchMethodError: com.google.common.util.concurrent.Futures.addCallback(Lcom/google/common/util/concurrent/ListenableFuture;Lcom/google/common/util/concurrent/FutureCallback;)V
    at bisq.core.dao.node.full.RpcService.lambda$setup$2(RpcService.java:153)
    at com.google.common.util.concurrent.TrustedListenableFutureTask$TrustedFutureInterruptibleTask.runInterruptibly(TrustedListenableFutureTask.java:125)
    at com.google.common.util.concurrent.InterruptibleTask.run(InterruptibleTask.java:57)
    at com.google.common.util.concurrent.TrustedListenableFutureTask.run(TrustedListenableFutureTask.java:78)
    at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1135)
    at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
    at java.base/java.lang.Thread.run(Thread.java:844)
Caused by: java.lang.NoSuchMethodError: com.google.common.util.concurrent.Futures.addCallback(Lcom/google/common/util/concurrent/ListenableFuture;Lcom/google/common/util/concurrent/FutureCallback;)V
    at com.neemre.btcdcli4j.daemon.BtcdDaemonImpl.startMonitors(BtcdDaemonImpl.java:213)
    at com.neemre.btcdcli4j.daemon.BtcdDaemonImpl.<init>(BtcdDaemonImpl.java:53)
    at bisq.core.dao.node.full.RpcService.lambda$setup$2(RpcService.java:136)
    ... 6 more
May-17 16:15:57.824 [SeedNodeMain] ERROR b.c.a.m.AppSetupWithP2PAndDAO: An error occurred: Error=bisq.core.dao.node.full.RpcException: java.lang.NoSuchMethodError: com.google.common.util.concurrent.Futures.addCallback(Lcom/google/common/util/concurrent/ListenableFuture;Lcom/google/common/util/concurrent/FutureCallback;)V

@oscarguindzberg
Copy link
Contributor Author

I just:

  • rebased from master
  • Excluded guava dependency from btcd-cli4j

Once bisq-network/btcd-cli4j#4 is merged I can update this pull request to use the latest btcd-cli4j version

@ripcurlx
Copy link
Contributor

I just:

  • rebased from master
  • Excluded guava dependency from btcd-cli4j

Once bisq-network/btcd-cli4j#4 is merged I can update this pull request to use the latest btcd-cli4j version

@oscarguindzberg  Do you want to update the PR with the latest btcd-cli4j version before the merge?

@oscarguindzberg
Copy link
Contributor Author

@ripcurlx Just updated the PR to use the latest btcd-cli4j. I also did a rebase from master

@ripcurlx ripcurlx changed the title Use bisq's bitcoinj 0.15.1 [WIP] Use bisq's bitcoinj 0.15.1 Jul 15, 2019
@wiz
Copy link
Contributor

wiz commented Aug 25, 2019

It's been a few months, what's the status of this PR?

@devinbileck
Copy link
Member

It was/is mostly ready, aside from thorough testing. It is stalled because we no longer have a bitcoinj maintainer and would be a risk to merge without one.

@wiz
Copy link
Contributor

wiz commented Aug 27, 2019

I agree it seems a significant risk with no immediate benefit, even for the SegWit use case there are many other issues to consider such as how Bisq P2P clients that don't support SegWit wouldn't be able to verify those transactions - I guess it would hard-fork the Bisq P2P network ?

@yoshimo
Copy link

yoshimo commented Aug 27, 2019

I agree it seems a significant risk with no immediate benefit, even for the SegWit use case there are many other issues to consider such as how Bisq P2P clients that don't support SegWit wouldn't be able to verify those transactions - I guess it would hard-fork the Bisq P2P network ?

I don't know if it is possible to see the version distribution, but even if we can't there is a significant difference between an updated library , implementing segwit and actually using segwit and its new adress-formats.
You can do it in stages and wait a certain time until enough users probably updated their client.

@bodymindarts
Copy link

bitcoinj has had some patch releases in the meantime (0.15.2, 0.15.3) I have created a branch that rebases the bisq specific changes to bitcoinj ontop of the 0.15.3 release.
I suggest using that version as the bases for this PR.

I am going to now try and rebase this PR ontop of bisq master and apply the 0.15.3-bisq bitcoinj version from my branch to hopeful move this work along.

@ripcurlx
Copy link
Contributor

As we decided to close PRs with no activity > 30 days, I'll close this PR for now. Feel free to reopen it when it is ready to be merged again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants