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

(3/6) [BUGFIX] #3629 (Duplicate payloads w/ 20-byte and 32-byte keys) #3636

Merged
merged 12 commits into from Nov 26, 2019

Conversation

@julianknutsen
Copy link
Contributor

julianknutsen commented Nov 19, 2019

New commits start at 849155a

fixes #3629

Hotfix Needed: No

There was a bug when reconstructing the in-memory HashMap for ProtectedStoragePayload objects implementing the PersistablePayload object. This had two related bugs:

  1. The in-memory map would end up with duplicate Payloads if a node rebuilt a ProtectedStorageEntry from the 20-byte key, then received the 32-byte key from the seednode during initialization or from an update on the P2P network.

  2. An initializing node would resync known payloads from the seednode since the Payload existed with both the 20-byte and 32-byte key.

This PR cleans up the usages so that all outward users see the 32-byte key and all internal users can use the existing 20-byte key for persistence. This design is much safer than transitioning all persistent users to 32-byte keys and writing on-disk upgrade code.

All external listeners of Payloads don't look at the key. So, although they will receive 2 onAdded() calls instead of 1, they do the right thing by excluding the second payload since it is identical. I don't think there is any reason to publish a hotfix for this.

Upgrade behavior

New Seednode <--> Old Client
The seednode will only have 1 32-byte key for each unique payload, but the client node may still have a 20-byte key that it will send as a known key. The seednode will ignore this known key, send back the 32-byte key Payload and the client node will update its map with the 32-byte key. This mimics existing behavior.

Old Seednode <--> New Client
The seednode will have 1 20-byte key and 1 32-byte key for the same unique payload. The client will only have the 32-byte key and will send it is a known key. The seednode will transmit the 20-byte Payload back to the client who will ignore it.

New Seednode <--> New Client
Both the seednode and the client will only have 1 32-byte key for each unique payload. The client will include its 32-byte key as a known key which will match the key in the seednode map. No duplicate Payload objects will be transmitted.

Instead of using a subclass that overwrites a value, utilize Guice
to inject the real value of 10000 in the app and let the tests overwrite
it with their own.
Remove unused imports and clean up some access modifiers now that
the final test structure is complete
Previously, this interface was called each time an item was changed. This
required listeners to understand performance implications of multiple
adds or removes in a short time span.

Instead, give each listener the ability to process a list of added or
removed entrys which can help them avoid performance issues.

This patch is just a refactor. Each listener is called once for each
ProtectedStorageEntry. Future patches will change this.
Minor performance overhead for constructing MapEntry and Collections
of one element, but keeps the code cleaner and all removes can still
use the same logic to remove from map, delete from data store, signal
listeners, etc.

The MapEntry type is used instead of Pair since it will require less
operations when this is eventually used in the removeExpiredEntries path.
…batch

All current users still call this one-at-a-time. But, it gives the ability
for the expire code path to remove in a batch.
This will cause HashMapChangedListeners to receive just one onRemoved()
call for the expire work instead of multiple onRemoved() calls for each
item.

This required a bit of updating for the remove validation in tests so
that it correctly compares onRemoved with multiple items.
…ch removes

#3143 identified an issue that tempProposals listeners were being
signaled once for each item that was removed during the P2PDataStore
operation that expired old TempProposal objects. Some of the listeners
are very expensive (ProposalListPresentation::updateLists()) which results
in large UI performance issues.

Now that the infrastructure is in place to receive updates from the
P2PDataStore in a batch, the ProposalService can apply all of the removes
received from the P2PDataStore at once. This results in only 1 onChanged()
callback for each listener.

The end result is that updateLists() is only called once and the performance
problems are reduced.

This removes the need for #3148 and those interfaces will be removed in
the next patch.
Now that the only user of this interface has been removed, go ahead
and delete it. This is a partial revert of
f5d75c4 that includes the code that was
added into ProposalService that subscribed to the P2PDataStore.
Write a test that shows the incorrect behavior for #3629, the hashmap
is rebuilt from disk using the 20-byte key instead of the 32-byte key.
@julianknutsen julianknutsen requested a review from ripcurlx as a code owner Nov 19, 2019
@julianknutsen julianknutsen mentioned this pull request Nov 19, 2019
@julianknutsen julianknutsen changed the title [BUGFIX] #3629 (Duplicate payloads w/ 20-byte and 32-byte keys) (3/5) [BUGFIX] #3629 (Duplicate payloads w/ 20-byte and 32-byte keys) Nov 20, 2019
Copy link
Member

freimair left a comment

I might be missing something here so here are some control questions to foster my understanding:

Upgrade behavior

New Seednode <--> Old Client
The seednode will only have 1 32-byte key for each unique payload, but the client node may still have a 20-byte key that it will send as a known key. The seednode will ignore this known key, send back the 32-byte key Payload and the client node will update its map with the 32-byte key. This mimics existing behavior.

  • So in the real world the old client will send roughly 2MB of 20-byte known keys to the new seed node.
  • The new seed node ignores all of them and respond with all data (close to our single-message size limit of 10MB)? (since it only received 20-byte keys and no 32-byte key)
  • Does the old client memorize the 32-byte keys? after restart?

Old Seednode <--> New Client
The seednode will have 1 20-byte key and 1 32-byte key for the same unique payload. The client will only have the 32-byte key and will send it is a known key. The seednode will transmit the 20-byte Payload back to the client who will ignore it.

  • why does the old seednode have 32-byte keys?
  • the client sends only 32-byte keys for its known payloads, so the seed node will send all 20-byte addressed payloads to the client (roughly 10MB of data) to every client.
  • every time a client connects. Because new clients will not memorize the 20-byte keys?
@julianknutsen

This comment has been minimized.

Copy link
Contributor Author

julianknutsen commented Nov 20, 2019

I might be missing something here so here are some control questions to foster my understanding:

Upgrade behavior

New Seednode <--> Old Client
The seednode will only have 1 32-byte key for each unique payload, but the client node may still have a 20-byte key that it will send as a known key. The seednode will ignore this known key, send back the 32-byte key Payload and the client node will update its map with the 32-byte key. This mimics existing behavior.

This behavior is only for ProtectedStoragePayload objects implementing PersistablePayload. For now, this is just TempProposalPayload.

  • So in the real world the old client will send roughly 2MB of 20-byte known keys to the new seed node.

No, the client sends roughly 1kb of unnecessary 20-byte keys at startup for the TempProposalPayload objects.

  • The new seed node ignores all of them and respond with all data (close to our single-message size limit of 10MB)? (since it only received 20-byte keys and no 32-byte key)

No, the seednode sends back around 50kb of duplicate TempProposal data in GetDataResponse.

  • Does the old client memorize the 32-byte keys? after restart?

No, old clients use the 20-byte key for persistence. So after restart all TempProposalPayload objects
are reconstructed with 20-byte keys. It only "works" because the seed nodes send 32-byte keys at
startup that the client nodes use to update their internal map.

Old Seednode <--> New Client
The seednode will have 1 20-byte key and 1 32-byte key for the same unique payload. The client will only have the 32-byte key and will send it is a known key. The seednode will transmit the 20-byte Payload back to the client who will ignore it.

  • why does the old seednode have 32-byte keys?

During initial creation of TempProposalPayload and during MyProposalListService::rePublishMyProposalsOnceWellConnected() the owners send them to the network. If these are "new", the seednode will save it as a 20-byte key in the ProtectedDataStoreService, but also keep it in-memory with the 32-bye key like the other ProtectedStoragePayloads. This allows it to give the 32-byte keys to new nodes on startup.

  • the client sends only 32-byte keys for its known payloads, so the seed node will send all 20-byte addressed payloads to the client (roughly 10MB of data) to every client.
  • every time a client connects. Because new clients will not memorize the 20-byte keys?

It isn't that the new clients won't memorize it, it is that they only have in-memory representations using 32-byte keys now. So, until the seednode is updated to ONLY have 32-byte keys and NOT have 20-byte keys in the internal map, the seednodes will still send back the 20-byte key payloads. It isn't harmful because the duplicate paylaods sent back are ignored from the new client.

Addresses the first half of #3629 by ensuring that the reconstructed
HashMap always has the 32-byte key for each payload.

It turns out, the TempProposalStore persists the ProtectedStorageEntrys
on-disk as a List and doesn't persist the key at all. Then, on
reconstruction, it creates the 20-byte key for its internal map.

The fix is to update the TempProposalStore to use the 32-byte key instead.
This means that all writes, reads, and reconstrution of the TempProposalStore
uses the 32-byte key which matches perfectly with the in-memory map
of the P2PDataStorage that expects 32-byte keys.

Important to note that until all seednodes receive this update, nodes
will continue to have both the 20-byte and 32-byte keys in their HashMap.
Addresses the second half of #3629 by using the HashMap, not the
protectedDataStore to generate the known keys in the requestData path.

This won't have any bandwidth reduction until all seednodes have the
update and only have the 32-byte key in their HashMap.

fixes #3629
The only user has been migrated to getMap(). Delete it so future
development doesn't have the same 20-byte vs 32-byte key issue.
@julianknutsen julianknutsen force-pushed the julianknutsen:bugfix-3629-20-byte branch from 1e648a7 to 793e84d Nov 21, 2019
@julianknutsen

This comment has been minimized.

Copy link
Contributor Author

julianknutsen commented Nov 21, 2019

After a deeper investigation of the persistence code, I found that TempProposals are stored to disk as a List and the Map.Entry<ByteArray, ProtectedStorageEntry> is actually reconstructed in-memory. So, I've updated e212240 accordingly with a much cleaner patch that just changes the 20-byte key to a 32-byte key.

@ripcurlx

This comment has been minimized.

Copy link
Member

ripcurlx commented Nov 21, 2019

@freimair Can you please review the changes and ACK if ok?

Copy link
Member

ripcurlx left a comment

utACK

Copy link
Member

freimair left a comment

Ack.

Now is the time to do the change, as only TempProposalPayload and the deprecated TradeStatistics are affected by the change. But it will generate additional network load if we are not quick in updating the seednodes.

@julianknutsen

This comment has been minimized.

Copy link
Contributor Author

julianknutsen commented Nov 21, 2019

I updated the bug as well but wanted to include it here.

After more investigation, 455f7d2 also fixes an additional issue where we were retransmitting every non-persistable ProtectedStorageEntry in the GetUpdatedDataRequest path. By using the in-memory map instead of the protectedDataStore, we now stop transmitting duplicates. Here was the breakdown of the duplicate transmissions before that patch. This likely also caused additional bandwidth when seednodes were trying to sync with each other since they would be resending everything.

#################################################################
Received 724 instances
RefundAgent: 1 (915 B)
Filter: 2 (5.7 KB)
TempProposalPayload: 56 (33.9 KB)
MailboxStoragePayload: 457 (2.0 MB)
Alert: 1 (826 B)
Mediator: 2 (1.8 KB)
OfferPayload: 205 (249.6 KB)
################################################################# 
@freimair freimair mentioned this pull request Nov 21, 2019
@julianknutsen julianknutsen changed the title (3/5) [BUGFIX] #3629 (Duplicate payloads w/ 20-byte and 32-byte keys) (3/6) [BUGFIX] #3629 (Duplicate payloads w/ 20-byte and 32-byte keys) Nov 22, 2019
@ripcurlx ripcurlx merged commit 793e84d into bisq-network:master Nov 26, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@julianknutsen julianknutsen deleted the julianknutsen:bugfix-3629-20-byte branch Nov 26, 2019
@ripcurlx ripcurlx mentioned this pull request Dec 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.