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

Add mediation support #3221

Merged
merged 181 commits into from Sep 9, 2019

Conversation

@chimp1984
Copy link
Contributor

commented Sep 6, 2019

No description provided.

chimp1984 added 30 commits Aug 30, 2019
- Do not use protobuf inheritance for Arbitrator and Mediator as it
would break backward compatibility (and protobuf inheritance sucks
anyway)
- Add common base class for ArbitratorService and MediatorService
- Add common base class for ArbitratorManager and MediatorManager
- With cmd+D one can open the mediator regisration in account screen.
For arbitrator its cmd+R
- Before release set new keys from maintainer who manages keys
- We changed handling of arbitrator selection with v0.9 so the
supported arbitrators in the offer is not used anymore. As we
enforced v1.2 a while back for trading we can be sure no pre v0.9
clients are used anymore and we can remove the optional code part.
- As we do not use supported arbitraors in offer anymore since v0.9
we can remove that.
As we do not use the supported arbitratos from offer since v0.9 we can
remove that check.
We do not use those fields anymore. We still need to keep the fields
not nullable as old clients have the check still.
We got in dev testing sometimes an empty protobuf Alert. Might be
caused from protobuf copatibility issues during development but not
100% clear.
As it causes an exception and corrupted user db file we prefer to set
it to null.
This is not used anymore. Currently we would get an exception in the
trade but with follow up changes we will fix that...

Mediator handling and selection will be done the same way like
arbitrator. The current mediator handling was a relict from earlier
partial support for mediators which never got completed. As still a
null check is in place we need to ensure backward compatibility.
We do not use arbitratorNodeAddresses and mediatorNodeAddresses anymore
but as there is a null check we still need to keep the field ans set it
to an empty arrayList.
We want to use the same selection algorithm for mediators as for
arbitrators, so we make ArbitratorSelection generic.
We add MEDIATOR_ADDRESS as extraMap entry to TradeStatistics2 to be
able to track number of trades with specific mediators.

ExtraMap is used to add new data to existing protobuf definitions which
is supported also by not updated clients. Adding a new protobuf field
would only be supported by new clients. As mediator support is a new
feature we could add a new field but to keep it in the same style like
arbitrator we prefer to use the map here as well.
…OpenOffer

WIP for supporting mediator selection the same way like arbitrators.
We can ensure that all users are post v0.9 so we can remove the nullable
 support.
chimp1984 added 3 commits Sep 9, 2019
…bisq into add-mediation-support

� Conflicts:
�	core/src/main/resources/i18n/displayStrings.properties
# Conflicts:
#	common/src/main/java/bisq/common/app/Capability.java
#	core/src/main/java/bisq/core/setup/CoreNetworkCapabilities.java
If mediator or arbitrator are doing a custom payout, we auto-fill
counterpart field with remaining amount, so he does not need to
calculate.
@chimp1984

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

@ripcurlx Text changes are applied.

@ripcurlx ripcurlx merged commit d55114e into bisq-network:release/v1.1.6 Sep 9, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@chimp1984 chimp1984 deleted the chimp1984:add-mediation-support branch Sep 11, 2019
@ripcurlx

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Bildschirmfoto 2019-09-11 um 10 05 19
@chimp1984 Not critical, but I would remove the Need help... in the forth step to prevent unnecessary mediation cases (there shouldn't be any issues at that point that require mediation)

@ripcurlx

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Although it will only be for 10 days as a transitional period that the mediation system will be activated, I think it will cause too much confusion if we have the mediation text in the the "Need help..." and within the popup, but open an arbitration case afterwards.
Bildschirmfoto 2019-09-11 um 10 47 25
Bildschirmfoto 2019-09-11 um 10 53 45

@ripcurlx

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

I don't think we have to get that far to deactivate the mediation tab in the support section as well, but within the trade I think it creates too much confusion.
Bildschirmfoto 2019-09-11 um 11 07 46

@ripcurlx

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Bildschirmfoto 2019-09-11 um 10 05 19
@chimp1984 Not critical, but I would remove the Need help... in the forth step to prevent unnecessary mediation cases (there shouldn't be any issues at that point that require mediation)

Actually we need to remove it here, as within arbitration it looks to the arbitrator as if he/she is able to change the output when closing the case, but actually it doesn't have any effect as the multisig was already signed and broadcasted.

@ripcurlx

This comment was marked as resolved.

Copy link
Member

commented Sep 11, 2019

Scenario

Buyer opens dispute with old client version and receives payout
--Given Alice as buyer (maker), Bob as seller (taker), and arbitrator are online
--And Alice is running an old client version (v1.1.5)
--And Bob is running the current client release version (1.1.6)
--And Alice has published a buy offer

I've set private static final Date REQUIRE_UPDATE_DATE = Utilities.getUTCDate(2019, GregorianCalendar.SEPTEMBER, 12); and private static final Date MEDIATION_ACTIVATED_DATE = Utilities.getUTCDate(2019, GregorianCalendar.SEPTEMBER, 12);. This should be the case when we release 1.1.6 and mediation is not activated and no update is required for trading yet.

When Bob tries to take the offer from Alice, Alice sees following error message
Bildschirmfoto 2019-09-11 um 14 13 42

java.lang.NullPointerException: mediator must not be null
	at com.google.common.base.Preconditions.checkNotNull(Preconditions.java:787)
	at bisq.core.trade.Trade.setMediatorNodeAddress(Trade.java:702)
	at bisq.core.trade.protocol.tasks.maker.MakerProcessPayDepositRequest.run(MakerProcessPayDepositRequest.java:80)
	at bisq.common.taskrunner.TaskRunner.next(TaskRunner.java:69)
	at bisq.common.taskrunner.TaskRunner.run(TaskRunner.java:60)
	at bisq.core.trade.protocol.BuyerAsMakerProtocol.handleTakeOfferRequest(BuyerAsMakerProtocol.java:143)
	at bisq.core.trade.BuyerAsMakerTrade.handleTakeOfferRequest(BuyerAsMakerTrade.java:104)
	at bisq.core.trade.TradeManager.handlePayDepositRequest(TradeManager.java:346)
	at bisq.core.trade.TradeManager.lambda$new$0(TradeManager.java:180)
	at bisq.network.p2p.P2PService.lambda$onMessage$8(P2PService.java:410)
	at java.base/java.util.concurrent.CopyOnWriteArrayList.forEach(CopyOnWriteArrayList.java:804)
	at java.base/java.util.concurrent.CopyOnWriteArraySet.forEach(CopyOnWriteArraySet.java:425)
	at bisq.network.p2p.P2PService.onMessage(P2PService.java:409)
	at bisq.network.p2p.network.NetworkNode.lambda$onMessage$4(NetworkNode.java:344)
	at java.base/java.util.Spliterators$ArraySpliterator.forEachRemaining(Spliterators.java:948)
	at java.base/java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:658)
	at bisq.network.p2p.network.NetworkNode.onMessage(NetworkNode.java:344)
	at bisq.network.p2p.network.Connection.lambda$onMessage$4(Connection.java:428)
	at java.base/java.util.concurrent.CopyOnWriteArrayList.forEach(CopyOnWriteArrayList.java:804)
	at java.base/java.util.concurrent.CopyOnWriteArraySet.forEach(CopyOnWriteArraySet.java:425)
	at bisq.network.p2p.network.Connection.lambda$onMessage$5(Connection.java:428)
	at com.sun.javafx.application.PlatformImpl.lambda$runLater$10(PlatformImpl.java:428)
	at java.base/java.security.AccessController.doPrivileged(Native Method)
	at com.sun.javafx.application.PlatformImpl.lambda$runLater$11(PlatformImpl.java:427)
	at com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java:96)

I was surprised that we had some mediator code in our old version already 🤔
During the time of creating the offer there was already a mediator and an arbitrator registered. When the mediator from the offer taker is is checked against the local selected mediators it returns null. That is because it tries to check for localhost:9223 in my case, but it isn't an accepted mediator for Alice. Alice has only the arbitrator localhost:9222 as mediator selected. So I have the feeling we might run into some issues during this first grace period until updating is required, if mediators are already registered during that time.

@chimp1984

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

@ripcurlx @mpolavieja
Did you register the mediator from the same app as the arbitrator? This is required for the update period as otherwise the mediator will be unknown to the old client (old clients had mirrored mediator with arbitrator as it was only prepared code but never implemented - so we need to ensure both have same onion address). Once we have enforced update mediators and arbitrators can (should) be diff. apps (nodes).

I could not reproduce the issue with the correct setup but could reproduce it with diff. arbitrator and mediator. If it was not documented in the setup please update the doc.

@ripcurlx

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

@ripcurlx @mpolavieja
Did you register the mediator from the same app as the arbitrator? This is required for the update period as otherwise the mediator will be unknown to the old client (old clients had mirrored mediator with arbitrator as it was only prepared code but never implemented - so we need to ensure both have same onion address). Once we have enforced update mediators and arbitrators can (should) be diff. apps (nodes).

I could not reproduce the issue with the correct setup but could reproduce it with diff. arbitrator and mediator. If it was not documented in the setup please update the doc.

No, I have used a different data directory for arbitrator and mediator. That explains the problem with the different address. So we'll switch to the real mediators after the enforced update requirement, correct? Didn't read your comment properly ;-)

@ripcurlx

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Mediator mediator = checkNotNull(user.getAcceptedMediatorByAddress(mediatorNodeAddress),
"user.getAcceptedArbitratorByAddress(arbitratorNodeAddress) must not be null");

Here the error message is wrong. Should be user.getAcceptedMediatorByAddress(mediatorNodeAddress) must not be null

I recognized this error when I had the wrong setup with separate arbitrator and mediator

Copy link
Member

left a comment

ACK

I have tried to break it but only managed to break it by creating a mediation case and then reverting to an older version for one of the traders. Should we handle that case? If we do nothing I think it will be up to the arbitrator to manually create payouts from the look of it, that's still acceptable I think.

@chimp1984

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

We should state in the release notes very clearly tha downgrading is not supporting and might break existing data structures. Downgrading is basically never supported and tested in Bisq.

@chimp1984

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

Here the error message is wrong. Should be user.getAcceptedMediatorByAddress(mediatorNodeAddress) must not be null

Fixed in next bug-fix PR

@chimp1984 chimp1984 referenced this pull request Sep 11, 2019
@devinbileck

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

While testing different variations of upgrade scenarios, I encountered a NullPointer exception. This may not be directly related to mediation (I have yet to confirm), but was encountered while testing it.

Preconditions

  • Offers of old clients permitted, mediation deactivated:
    OfferRestrictions.REQUIRE_UPDATE_DATE = 25.9
    MediationManager.MEDIATION_ACTIVATED_DATE = 30.9
  • Alice and Bob running old version (v1.1.5).

Steps to reproduce

  • Bob takes offer, BTC seller as taker.
  • Alice confirms payment started.
  • Alice updates and starts her client.

Result
Alice encounters the following NPE on starting her client.
Notes:

  • Did not encounter the error upon restarting the client subsequent times.
  • Same error is encountered when Bob updates and starts his client.
Sep-10 23:42:31.753 [JavaFX Application Thread] ERROR b.c.s.CommonSetup: Uncaught Exception from thread JavaFX Application Thread
Sep-10 23:42:31.754 [JavaFX Application Thread] ERROR b.c.s.CommonSetup: throwableMessage= null
Sep-10 23:42:31.754 [JavaFX Application Thread] ERROR b.c.s.CommonSetup: throwableClass= class java.lang.NullPointerException
Sep-10 23:42:31.759 [JavaFX Application Thread] ERROR b.c.s.CommonSetup: Stack trace:
java.lang.NullPointerException
	at bisq.core.btc.wallet.WalletService.getTransaction(WalletService.java:610)
	at bisq.core.btc.wallet.WalletService.getTransaction(WalletService.java:615)
	at bisq.core.trade.Trade.getDepositTx(Trade.java:607)
	at bisq.desktop.main.portfolio.pendingtrades.PendingTradesDataModel.doSelectItem(PendingTradesDataModel.java:403)
	at bisq.desktop.main.portfolio.pendingtrades.PendingTradesDataModel.selectBestItem(PendingTradesDataModel.java:379)
	at bisq.desktop.main.portfolio.pendingtrades.PendingTradesDataModel.onListChanged(PendingTradesDataModel.java:374)
	at bisq.desktop.main.portfolio.pendingtrades.PendingTradesDataModel.activate(PendingTradesDataModel.java:151)
	at bisq.desktop.common.model.ActivatableDataModel._activate(ActivatableDataModel.java:28)
	at bisq.desktop.common.model.ActivatableWithDataModel._activate(ActivatableWithDataModel.java:28)
	at bisq.desktop.common.view.ActivatableViewAndModel.lambda$prepareInitialize$0(ActivatableViewAndModel.java:42)
	at com.sun.javafx.binding.ExpressionHelper$SingleChange.fireValueChangedEvent(ExpressionHelper.java:181)
	at com.sun.javafx.binding.ExpressionHelper.fireValueChangedEvent(ExpressionHelper.java:80)
	at javafx.beans.property.ReadOnlyObjectPropertyBase.fireValueChangedEvent(ReadOnlyObjectPropertyBase.java:74)
	at javafx.beans.property.ReadOnlyObjectWrapper.fireValueChangedEvent(ReadOnlyObjectWrapper.java:102)
	at javafx.scene.Node$ReadOnlyObjectWrapperManualFire.fireSuperValueChangedEvent(Node.java:1054)
	at javafx.scene.Node.invalidatedScenes(Node.java:1114)
	at javafx.scene.Node.setScenes(Node.java:1152)
	at javafx.scene.Parent.scenesChanged(Parent.java:769)
	at javafx.scene.Node.invalidatedScenes(Node.java:1076)
	at javafx.scene.Node.setScenes(Node.java:1152)
	at javafx.scene.Parent$2.onChanged(Parent.java:369)
	at com.sun.javafx.collections.TrackableObservableList.lambda$new$0(TrackableObservableList.java:45)
	at com.sun.javafx.collections.ListListenerHelper$Generic.fireValueChangedEvent(ListListenerHelper.java:329)
	at com.sun.javafx.collections.ListListenerHelper.fireValueChangedEvent(ListListenerHelper.java:73)
	at javafx.collections.ObservableListBase.fireChange(ObservableListBase.java:233)
	at javafx.collections.ListChangeBuilder.commit(ListChangeBuilder.java:482)
	at javafx.collections.ListChangeBuilder.endChange(ListChangeBuilder.java:541)
	at javafx.collections.ObservableListBase.endChange(ObservableListBase.java:205)
	at javafx.collections.ModifiableObservableListBase.add(ModifiableObservableListBase.java:155)
	at com.sun.javafx.collections.VetoableListDecorator.add(VetoableListDecorator.java:320)
	at com.jfoenix.skins.JFXTabPaneSkin.addTabContentHolder(JFXTabPaneSkin.java:228)
	at com.jfoenix.skins.JFXTabPaneSkin.<init>(JFXTabPaneSkin.java:100)
	at com.jfoenix.controls.JFXTabPane.createDefaultSkin(JFXTabPane.java:67)
	at javafx.scene.control.Control.doProcessCSS(Control.java:897)
	at javafx.scene.control.Control.access$000(Control.java:83)
	at javafx.scene.control.Control$1.doProcessCSS(Control.java:89)
	at com.sun.javafx.scene.control.ControlHelper.processCSSImpl(ControlHelper.java:67)
	at com.sun.javafx.scene.NodeHelper.processCSS(NodeHelper.java:145)
	at javafx.scene.Node.processCSS(Node.java:9529)
	at javafx.scene.Node.processCSS(Node.java:9522)
	at javafx.scene.Node.processCSS(Node.java:9522)
	at javafx.scene.Node.processCSS(Node.java:9522)
	at javafx.scene.Node.processCSS(Node.java:9522)
	at javafx.scene.Scene.doCSSPass(Scene.java:569)
	at javafx.scene.Scene.access$3400(Scene.java:172)
	at javafx.scene.Scene$ScenePulseListener.pulse(Scene.java:2477)
	at com.sun.javafx.tk.Toolkit.lambda$runPulse$2(Toolkit.java:412)
	at java.base/java.security.AccessController.doPrivileged(Native Method)
	at com.sun.javafx.tk.Toolkit.runPulse(Toolkit.java:411)
	at com.sun.javafx.tk.Toolkit.firePulse(Toolkit.java:438)
	at com.sun.javafx.tk.quantum.QuantumToolkit.pulse(QuantumToolkit.java:519)
	at com.sun.javafx.tk.quantum.QuantumToolkit.pulse(QuantumToolkit.java:499)
	at com.sun.javafx.tk.quantum.QuantumToolkit.pulseFromQueue(QuantumToolkit.java:492)
	at com.sun.javafx.tk.quantum.QuantumToolkit.lambda$runToolkit$11(QuantumToolkit.java:320)
	at com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java:96)
	at com.sun.glass.ui.win.WinApplication._runLoop(Native Method)
	at com.sun.glass.ui.win.WinApplication.lambda$runLoop$3(WinApplication.java:174)
	at java.base/java.lang.Thread.run(Thread.java:844)
@devinbileck

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Also while testing the above mentioned upgrade scenarios, I am seeing the following after they update, not sure if it is a concern:

Sep-10 23:52:42.719 [JavaFX Application Thread] ERROR bisq.common.proto.ProtoUtil: Invalid value for enum MediationResultState: PB_ERROR_MEDIATION_RESULT
Sep-10 23:52:42.720 [JavaFX Application Thread] ERROR bisq.common.proto.ProtoUtil: We try to lookup for an enum entry with name 'UNDEFINED' and use that if available, otherwise the enum is null. enum=null
@ripcurlx

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Bildschirmfoto 2019-09-11 um 10 05 19
@chimp1984 Not critical, but I would remove the Need help... in the forth step to prevent unnecessary mediation cases (there shouldn't be any issues at that point that require mediation)

@chimp1984 This is still open.

@ripcurlx

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Although it will only be for 10 days as a transitional period that the mediation system will be activated, I think it will cause too much confusion if we have the mediation text in the the "Need help..." and within the popup, but open an arbitration case afterwards.
Bildschirmfoto 2019-09-11 um 10 47 25
Bildschirmfoto 2019-09-11 um 10 53 45

@chimp1984 This is still open.

@ripcurlx

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Bildschirmfoto 2019-09-11 um 21 14 39

I think after a mediation has been closed there should be a message in the final message that the user has to go to the trade again and accept or reject the suggestion made by the mediator. Otherwise people don't know what to do next.
@ripcurlx

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Bildschirmfoto 2019-09-11 um 21 16 40

After accepting or rejecting a mediation the button still shows `Accept or Reject`. It should be greyed out with `You have accepted` or `You have rejected`
@ripcurlx

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Also while testing the above mentioned upgrade scenarios, I am seeing the following after they update, not sure if it is a concern:

Sep-10 23:52:42.719 [JavaFX Application Thread] ERROR bisq.common.proto.ProtoUtil: Invalid value for enum MediationResultState: PB_ERROR_MEDIATION_RESULT
Sep-10 23:52:42.720 [JavaFX Application Thread] ERROR bisq.common.proto.ProtoUtil: We try to lookup for an enum entry with name 'UNDEFINED' and use that if available, otherwise the enum is null. enum=null

@devinbileck I followed the process described by you (documented in https://bisq.ontestpad.com/script/101#63/2/), but didn't get the exception.
The only thing I see as well is the error logs mentioned above.

@devinbileck

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

@ripcurlx Now I cannot reproduce the NPE either ;(

@ripcurlx ripcurlx referenced this pull request Sep 13, 2019
@ripcurlx ripcurlx referenced this pull request Oct 7, 2019
@sqrrm sqrrm referenced this pull request Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.