-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Redact deposittxid from tradestatistics #3911
Redact deposittxid from tradestatistics #3911
Conversation
Thinking about it, I think we should do it date based, as it will delay the activation of this feature for roughly 30 more days and it would also help us to get rid of unnecessary support cases by users using old versions that don't include the patches from v1.2.4+. Will force push a change in a couple of minutes. |
Thank you for working on this critical issue and doing the forced update - will #3894 be a part of this forced update as well? |
cd269b5
to
b297a69
Compare
I'll probably add the fix for #3894 to this PR as well, as they both need a forced update process to prevent any backwards compatibility issues. |
Avoiding duplicate trade stats is good but is it really a breaking issue? Perhaps any duplicates should be removed if they have a deposittxid set for trades made after the cutoff date. Seed nodes would be upgraded by the cutoff date and only a few users would still be running on version < 1.2.5. They might have duplicate entries but I don't think that would cause harm and once they upgrade they would also remove the bad duplicates. I think it's good to avoid forced upgrades as much as possible. |
True, that could be done. I'll have a look on how to clean up duplicates in that case. |
As we don't have any arbitrators connected to the network anymore it shouldn't possible to use arbitration anymore
b297a69
to
1d9fda7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
Starting with the release after v1.2.6 the deposit transaction ID will not be added anymore to the trade statistics object. If you use the
--dumpStatistics
option in this PR the deposit transaction ID will not be stored in the JSON file anymore.Of course until we activate this feature with the next release after v1.2.6 (we need to set the minimum required version for trading to 1.2.6 before e.g. v1.2.7 release is published) a sophisticated actor can manipulate the source code and still access this information.
This staged release is necessary to prevent duplicate trade statistics objects because of how the hash for it is generated.
What needs to be discussed is, if we want to make it version update dependent or if we want to set a date and force all to upgrade to v1.2.6 a couple days after the release.
Fixes #3893.