core: protect epoch checksum with its own mutex #2977
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JoeGruffins
reviewed
Sep 18, 2024
There was a problem hiding this comment.
Looks good but trying it out all of my cancels fail.
2024-09-18 08:07:17.441 [INF] CORE: Cancel order d208ee4fa7f536ffb2025bf6c1b31d1fbe9af6c506d3332fb70478ab3cac7e24 targeting order 7f0eec29050cd41ed20ce16aa5aec94c08bfb4cf6aeb8ef60b9c483478c078ab at 127.0.0.1:17273 has been placed
2024-09-18 08:07:17.441 [DBG] CORE: notify: |POKE| (order) Cancelling order - A cancel order has been submitted for order {{{order|7f0eec29050cd41ed20ce16aa5aec94c08bfb4cf6aeb8ef60b9c483478c078ab}}} - Order: 7f0eec29050cd41ed20ce16aa5aec94c08bfb4cf6aeb8ef60b9c483478c078ab
....
2024-09-18 08:07:30.000 [DBG] CORE: Received preimage request for order d208ee4fa7f536ffb2025bf6c1b31d1fbe9af6c506d3332fb70478ab3cac7e24 with known commitment 753dae1f98bd897467f8754912c82533b36612288f448f9dbefb472085a35fe7
2024-09-18 08:07:30.000 [DBG] CORE: async processPreimageRequest for d208ee4fa7f536ffb2025bf6c1b31d1fbe9af6c506d3332fb70478ab3cac7e24 succeeded
2024-09-18 08:07:30.000 [ERR] CORE[wss://127.0.0.1:17273/ws]: No handler found for response: {"type":2,"id":8886,"payload":{"result":null,"error":{"code":45,"message":"preimage hash f655dda0147785bf309308f63fe2c50a548f5ccecd864b91932db04823f23069 does not match order commitment 753dae1f98bd897467f8754912c82533b36612288f448f9dbefb472085a35fe7"}},"sig":""}
2024-09-18 08:07:30.011 [DBG] CORE: Score changed at 127.0.0.1:17273. New score is 13 / 60, tier = 10, penalties = 0
2024-09-18 08:07:30.011 [WRN] CORE: Deleting failed cancel order d208ee4fa7f536ffb2025bf6c1b31d1fbe9af6c506d3332fb70478ab3cac7e24 that targeted trade order 7f0eec29050cd41ed20ce16aa5aec94c08bfb4cf6aeb8ef60b9c483478c078ab
2024-09-18 08:07:30.013 [WRN] CORE: notify: |WARNING| (order) Failed cancel - Cancel order for order 7f0eec29050cd41ed20ce16aa5aec94c08bfb4cf6aeb8ef60b9c483478c078ab failed and is now deleted. - Order: 7f0eec29050cd41ed20ce16aa5aec94c08bfb4cf6aeb8ef60b9c483478c078ab
Comment on lines
4002
to
+4012
| tracker.mtx.RLock() | ||
| if !bytes.Equal(firstCSum, tracker.csum) { | ||
| csum := tracker.csum | ||
| tracker.mtx.RUnlock() | ||
| if !bytes.Equal(firstCSum, csum) { | ||
| t.Fatalf( | ||
| "[handlePreimageRequest] csum was changed, exp: %s, got: %s", | ||
| firstCSum, | ||
| tracker.csum, | ||
| csum, | ||
| ) | ||
| } | ||
| tracker.mtx.RUnlock() | ||
|
|
There was a problem hiding this comment.
Seems this one should also be holding the new tracker.csumMtx instead
Comment on lines
4073
to
+4076
| tracker.mtx.RLock() | ||
| if !bytes.Equal(csum, tracker.csum) { | ||
| checkSum := tracker.csum | ||
| tracker.mtx.RUnlock() | ||
| if !bytes.Equal(csum, checkSum) { |
There was a problem hiding this comment.
Should hold the csum mutex?
Comment on lines
4157
to
+4160
| tracker.mtx.RLock() | ||
| if !bytes.Equal(commitCSum, tracker.cancel.csum) { | ||
| cancelCsum := tracker.cancelCsum | ||
| tracker.mtx.RUnlock() | ||
| if !bytes.Equal(commitCSum, cancelCsum) { |
There was a problem hiding this comment.
Should hold the csum mutex?
Comment on lines
4247
to
+4249
| tracker.mtx.RLock() | ||
| if !bytes.Equal(firstCSum, tracker.cancel.csum) { | ||
| cancelCsum := tracker.cancelCsum | ||
| tracker.mtx.RUnlock() |
There was a problem hiding this comment.
Should hold the csum mutex instead?
Comment on lines
4333
to
+4336
| tracker.mtx.RLock() | ||
| if !bytes.Equal(csum, tracker.cancel.csum) { | ||
| cancelCsum := tracker.cancelCsum | ||
| tracker.mtx.RUnlock() | ||
| if !bytes.Equal(csum, cancelCsum) { |
There was a problem hiding this comment.
Should hold the csum mutex?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
trackedTrade.csumfield was protected under the very broadtrackedTrade.mtx. This created lock contention when preimage requests came in for cancel orders while we had long running ticks, sinceacceptCsumhad to lock that mutex. With this change, we now protect the checksum fields for trades and cancels under a different mutex.Additionally, there were two places where the
trackedTrade.cancelfield was being read unsafely. A solution for that is implemented.Closes #2964
Supercedes #2973