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

Limit max. nr. of PersistableNetworkPayload and ProtectedStorageEntries #3562

Merged
merged 6 commits into from Nov 5, 2019

Conversation

@chimp1984
Copy link
Contributor

chimp1984 commented Nov 5, 2019

To avoid that seed nodes get overloaded with requests for too many
PersistableNetworkPayload and ProtectedStorageEntry data we limit nr. of
entries to max 10000.

… to 10000

To avoid that seed nodes get overloaded with requests for too many
PersistableNetworkPayload and ProtectedStorageEntry data we limit nr. of
entries to max 10000.
@chimp1984 chimp1984 requested a review from ripcurlx as a code owner Nov 5, 2019
@chimp1984 chimp1984 referenced this pull request Nov 5, 2019
chimp1984 added 5 commits Nov 5, 2019
- Add log of size to GetBlocksResponse.toProtoNetworkEnvelope method
- Log in kb
We have an invalid Filter object in the live network (prob. some dev
made some mistake). This code helps so clean that up.
Copy link
Member

ripcurlx left a comment

utACK

@ripcurlx ripcurlx merged commit b976bec into bisq-network:master Nov 5, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@chimp1984 chimp1984 referenced this pull request Nov 5, 2019
.map(Map.Entry::getValue)
.collect(Collectors.toSet());
if (maxSize.get() <= 0) {
log.warn("The peer request caused too much ProtectedStorageEntry entries to get delivered. We limited the entries for the response to {} entries", MAX_ENTRIES);

This comment has been minimized.

Copy link
@julianknutsen

julianknutsen Nov 6, 2019

Contributor

Is the UpdateDataRequest the only way to retrieve items > 10,000? What if the total set is larger than what the Initial & Update can handle? Is there a periodic on as well to sync over time?

@chimp1984 chimp1984 deleted the chimp1984:limit-data-response-at-seed branch Nov 6, 2019
*
* @param protectedStorageEntry The entry to be removed
*/
public void removeInvalidProtectedStorageEntry(ProtectedStorageEntry protectedStorageEntry) {

This comment has been minimized.

Copy link
@julianknutsen

julianknutsen Nov 9, 2019

Contributor

This leads to data structure inconsistency in the event that two consumers listen for the same Payload type and one consumer calls into this function to remove the data. It also breaks encapsulation having the consumers know about the internals of the P2PDataStore. This isn't perfect now, anyway, but moving towards a model where P2PDataStore is private to P2PService may be a good goal.

It seems like a cleaner way to do the same thing would be to have the P2PDataStore validate each stored entry prior to becoming "active" or signaling listeners. The ProtectedStorageEntry and/or payload could implement an interface that would do this check and remove the data before it was every available for consumers.

Is that what you were thinking with the "generic validation method"? Doing this would dovetail well with my existing refactoring and I can probably try it out and see how it looks. I think it would give a lot more flexibility for the future if there are certain Entrys or Payloads that we need to purge but made it past prior validation. Here is just one example I found while testing:

// TESTCASE: validForAddOperation() should fail if Entry.receiversPubKey and Payload.ownerPubKey don't match
// XXXBUGXXX: The current code doesn't validate this mismatch, but it would create an added payload that could never
// be removed since the remove code requires Entry.receiversPubKey == Payload.ownerPubKey
@Test
public void isValidForAddOperation_EntryReceiverPayloadReceiverMismatch() throws NoSuchAlgorithmException, CryptoException {
KeyPair senderKeys = TestUtils.generateKeyPair();
KeyPair receiverKeys = TestUtils.generateKeyPair();
MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic());
ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, senderKeys.getPublic(), 1);
// should be assertFalse
Assert.assertTrue(protectedStorageEntry.isValidForAddOperation());
}

@ripcurlx ripcurlx referenced this pull request Nov 11, 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.