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

Accept old trade statistic object #3933

Conversation

ripcurlx
Copy link
Contributor

@ripcurlx ripcurlx commented Feb 3, 2020

The code didn't handle before the use case of new trade statistic objects
created by two old clients. This change make it independent of the cut off date
and allows us at a later point to update all trade statistics objects with
depositTxId value of null.

The code didn't handle before the use case of new trade statistic objects
created by two old clients. This change make it independent of the cut off date
and allows us at a later point to update all trade statistics objects with
depositTxId value of null.
@ripcurlx ripcurlx force-pushed the accept-tradestatistic-objects-before-cut-off-date branch from 4992762 to 2ad279f Compare February 3, 2020 10:38
@ripcurlx
Copy link
Contributor Author

ripcurlx commented Feb 3, 2020

Just to make the different use cases more clear:

  1. Old clients
    As they still have their local trade statistics they will still see the trade statistics.
  • If a new trade is added and both parties are on the old version the trade will show up in the trade statistics for the clients.
  • If a new trade is added and the client receives the trade statistics object from the seednode (or any other v1.2.6 node) it won't be shown in the trade statistics as it is seen as invalid trade statistics object (trade deposit tx id is null).
  1. New clients v1.2.6+
    Nothing changes for the new clients. They will see trades created by old clients and trades by new clients.

Seednodes:
Starting with v1.2.6 they will load the trade statistic object in memory and set the deposit tx id of the trade statistics objects to null (also for all old trades). We should update the seednodes first before releasing, so they are not missing out any trades by new clients which would be seen as invalid by the old seednodes.

This prevents unnecessary load on the seednodes from old clients
loading all trade statistic objects on startup as they interpret
the null value as empty string
@ripcurlx
Copy link
Contributor Author

ripcurlx commented Feb 3, 2020

As it doesn't give more privacy to set deposit transaction ids of old trade statistics to null (can be retrieved by data stores of old client binaries) and only would cause additional load for our seednodes I've reverted this change. Still as old clients will parse null to an empty string causes them to load trades by new clients (v1.2.6) to reload on restart as empty string != null.

@@ -217,7 +218,7 @@ public static TradeStatistics2 fromProto(protobuf.TradeStatistics2 proto) {
proto.getTradePrice(),
proto.getTradeAmount(),
proto.getTradeDate(),
null, // We don't want to expose this anymore
ProtoUtil.stringOrNullFromProto(proto.getDepositTxId()),
Copy link
Member

Choose a reason for hiding this comment

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

This would leave all new trades from older clients still exposed. That's perhaps the safest way to do it. Was hoping it would be possible to keep setting it to null for new version and just not serve the old version anything. Not sure if that would break things though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you only get the additionally privacy if you update or if you trade with a peer that has already updated. I had to do it like that to prevent the unnecessary load from old clients.

Copy link
Member

@sqrrm sqrrm left a comment

Choose a reason for hiding this comment

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

ACK

Works as planned and the deposittx is of course exposed as long as users don't upgrade. Nothing can be done about that.

@sqrrm sqrrm merged commit cd74571 into bisq-network:master Feb 4, 2020
@sqrrm
Copy link
Member

sqrrm commented Feb 4, 2020

I verified that the old version did cause delivery of a lot of tradestats objects and this version doesn't.

@ripcurlx ripcurlx added the is:priority PR or issue marked with this label is up for compensation label Feb 5, 2020
@ripcurlx ripcurlx deleted the accept-tradestatistic-objects-before-cut-off-date branch July 16, 2021 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is:priority PR or issue marked with this label is up for compensation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants