(2/8) [BUGFIX] Don't try and remove() if addMailboxData() fails #3556
Conversation
| keyRing.getSignatureKeyPair(), | ||
| receiversPublicKey); | ||
| boolean removeResult = p2PDataStorage.removeMailboxData(removeProtectedMailboxStorageEntry, networkNode.getNodeAddress(), true); | ||
| log.error("Unexpected state: adding mailbox message that already exists. removeMailboxData result=" + removeResult); | ||
| } |
There was a problem hiding this comment.
If the result from addProtectedStorageEntry is false we have not added data to the map or the sequence nr map. From my understanding I think the remove is pointless as there is nothing to remove anyway in case of a failed add. But maybe I am missing something. Do you see any case where this would make sense? Beside that your code looks correct to me. So if you agree that the whole result=false path is pointless, I would suggest to remove it instead.
There was a problem hiding this comment.
After more testing, it is even simpler than that. Even if the add() did update some state and still return false (which isn't the case), only the receiver of a mailbox entry can remove it so the sender shouldn't try.
This brings up one of my TODO list items for this module which is the fact that concurrent calls through the client API (getProtectedStorageEntry) can result in interleaved sequence numbers that cause unexpected failures. Moving to a contract where client API calls never fail due to stale sequence numbers will make the system easier to reason about. This could be done by synchronizing the client API paths to not hand out duplicate seq numbers.
I've added multiple tests in the normal add() and mailbox add() paths to document the current behavior to ensure my future patches don't change it.
Now that all the code is abstracted and tested, the remove() and removeMailboxData() functions are identical. Combine them and update callers appropriately. Now, any caller don't need to know the difference and it removes the sharp edge originally found in bisq-network#3556
julianknutsen
changed the title
julianknutsen
force-pushed
the
fix-remove-bug
branch
from
0de96a9 to
00a1b45
Compare
|
Ok. I've rebased this branch with master now to see how it looks. In my opinion, this is much easier to review than the wall of merges before and it will have a clean history once it is goes through the merge-commit path. The downside is the prior files are gone that the code review comments referenced, but the comments are still visible so the reviewers can verify the changes were actually implemented. Let me know what you think and if this is a good path moving forward. |
Fix a bug where remove() was called in the addMailboxData() failure path. 1. Sender's can't remove mailbox entries. Only the receiver can remove it so even if the previous add() failed and left partial state, the remove() can never succeed. 2. Even if the sender could remove, this path used remove() instead of removeMailboxData() so it wouldn't have succeed anyway. This patch cleans up the failure path as well as adds a precondition for the remove() function to ensure future callers don't use them for ProtectedMailboxStorageEntrys.
julianknutsen
force-pushed
the
fix-remove-bug
branch
from
00a1b45 to
de5ffd4
Compare
julianknutsen
changed the title
|
Absolutely right. I've updated the commit text and PR title to reflect the actual changes after PR review and comments. Thanks! |
Now that all the code is abstracted and tested, the remove() and removeMailboxData() functions are identical. Combine them and update callers appropriately. Now, any caller don't need to know the difference and it removes the sharp edge originally found in bisq-network#3556
Fix a bug where remove() was called instead of removeMailboxData() and
guard against future callers doing the same thing.
Additionally, fix a potential bug where the sequence number for the
remove was not incremented leading to unexpected add after remove behavior
documented in P2PDataStorageTest::addProtectectedStorageEntry_afterRemoveSameSeqNr().
For mailbox messages, this wouldn't actually be a problem due to the
AddOncePayload behavior, but all removes now increase the sequence number
making refactoring and analysis easier.