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.

Sign up for GitHub

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

(6/6) Clean up technical debt in P2PDataStorage and ProtectedStorageEntry objects #3747

Merged
merged 51 commits into from
Dec 9, 2019
Merged

(6/6) Clean up technical debt in P2PDataStorage and ProtectedStorageEntry objects #3747

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
5fcd18c
[TESTS] Add tests of requestData
julianknutsen Nov 20, 2019
1e814d9
[REFACTOR] Introduce buildGetDataRequest variants
julianknutsen Nov 20, 2019
a927ed4
[TESTS] Add tests of new RequestData APIs
julianknutsen Nov 20, 2019
daffe6d
[TESTS] Add tests of GetDataRequestHandler
julianknutsen Nov 21, 2019
944b3ff
[REFACTOR] Introduce buildGetDataResponse
julianknutsen Nov 21, 2019
8208f78
[REFACTOR] Extract connectionInfo String
julianknutsen Nov 21, 2019
5402155
[REFACTOR] Extract getDataResponse logging
julianknutsen Nov 21, 2019
a6e8868
[REFACTOR] Extract truncation logging
julianknutsen Nov 21, 2019
dafc762
[REFACTOR] Pass peerCapabilities into buildGetDataResponse
julianknutsen Nov 21, 2019
5630b35
[TESTS] Unit tests of buildGetDataResponse
julianknutsen Nov 21, 2019
caf946d
Remove redundant HashSet lookups in filter functions
julianknutsen Nov 21, 2019
703a9a0
[REFACTOR] Move required capabilities log
julianknutsen Nov 21, 2019
3aaf8a2
[REFACTOR] Inline capability check for ProtectedStorageEntries
julianknutsen Nov 21, 2019
4c5d818
[REFACTOR] Inline filtering functions
julianknutsen Nov 21, 2019
e767340
[REFACTOR] Remove duplication in filtering functions
julianknutsen Nov 21, 2019
00128d9
[BUGFIX] Fix off-by-one in truncation logic
julianknutsen Nov 21, 2019
c7bce9e
[TESTS] Add test of RequestDataHandler::onMessage
julianknutsen Nov 21, 2019
873271c
[REFACTOR] Introduce processGetDataResponse
julianknutsen Nov 21, 2019
690b980
[TESTS] Make verify() functions more flexible
julianknutsen Nov 21, 2019
a34488b
[TESTS] Add unit tests for processGetDataResponse
julianknutsen Nov 21, 2019
3d6e9fb
Remove static from initialRequestApplied
julianknutsen Nov 21, 2019
f92893b
[TESTS] Write synchronization integration tests
julianknutsen Nov 21, 2019
5db1285
[REFACTOR] Clean up processGetDataResponse
julianknutsen Nov 22, 2019
ecae31e
[RENAME] LazyProcessedPayload to ProcessOncePersistableNetworkPayload
julianknutsen Nov 22, 2019
a0fae12
Remove @Nullable around persistableNetworkPayloadSet
julianknutsen Nov 22, 2019
c503bcb
Remove @Nullable around supportedCapabilities in GetDataResponse
julianknutsen Nov 22, 2019
b1a06fe
Remove @Nullable around supportedCapabilities in PreliminaryGetDataRe…
julianknutsen Nov 22, 2019
4fe19ae
[DEADCODE] Remove old request handler tests
julianknutsen Nov 22, 2019
0649323
Make addPersistableNetworkPayloadFromInitialRequest private
julianknutsen Nov 23, 2019
56a7661
[REFACTOR] Clean up ClientAPI for addPersistableNetworkPayload
julianknutsen Nov 22, 2019
0e6b1a2
[REFACTOR] Clean up ClientAPI for addProtectedStorageEntry
julianknutsen Nov 22, 2019
77413c9
[REFACTOR] Clean up ClientAPI for remove
julianknutsen Nov 22, 2019
9f69134
[REFACTOR] Clean up ClientAPI for refreshTTL
julianknutsen Nov 22, 2019
bfdb8f5
Make isDataOwner a private policy decision in BroadcastHandler
julianknutsen Nov 23, 2019
4dc4532
Remove isDataOwner from P2PDataStorage
julianknutsen Nov 23, 2019
6ff8756
[REFACTOR] inline broadcast() private function
julianknutsen Nov 22, 2019
5a174d5
[REFACTOR] inline broadcastProtectedStorageEntry() private function
julianknutsen Nov 22, 2019
1bd450b
[REFACTOR] inline maybeAddToRemoveAddOncePayloads() private function
julianknutsen Nov 22, 2019
17f4b70
[TESTS] Clean up mockito never() and eq(null) usages
julianknutsen Nov 23, 2019
b6b0026
Remove ProtectedStorageEntry::updateSignature
julianknutsen Nov 23, 2019
24ecfc7
Remove ProtectedStorageEntry::maybeAdjustCreationTimeStamp
julianknutsen Nov 23, 2019
0c67608
@NotNull ProtectedStorageEntry::ownerPubKey
julianknutsen Nov 23, 2019
76e8c57
@NotNull ProtectedStorageEntry::protectedStoragePayload
julianknutsen Nov 23, 2019
104984c
@NotNull MailboxStoragePayload::senderPubKeyForAddOperation
julianknutsen Nov 23, 2019
01a7f79
Make CHECK_TTL_INTERVAL_SEC final
julianknutsen Dec 4, 2019
c38ff9b
s/networkPayload/protectedStoragePayload
julianknutsen Dec 5, 2019
688405b
[TESTS] Make onDisconnect tests more robust
julianknutsen Dec 5, 2019
df2e4cc
Refactor P2PDataStorage::onDisconnect
julianknutsen Dec 5, 2019
b166009
Remove expire optimization in onDisconnect
julianknutsen Dec 5, 2019
7b8d346
Remove filter for ExpirablePayload
julianknutsen Dec 5, 2019
e8c8225
[PR COMMENTS] Fix comment typo
julianknutsen Dec 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 45 additions & 40 deletions p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,46 +440,51 @@ public void onConnection(Connection connection) {

@Override
public void onDisconnect(CloseConnectionReason closeConnectionReason, Connection connection) {
if (connection.hasPeersNodeAddress() && !closeConnectionReason.isIntended) {
map.values()
.forEach(protectedStorageEntry -> {
ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload();
if (protectedStoragePayload instanceof ExpirablePayload && protectedStoragePayload instanceof RequiresOwnerIsOnlinePayload) {
NodeAddress ownerNodeAddress = ((RequiresOwnerIsOnlinePayload) protectedStoragePayload).getOwnerNodeAddress();
if (connection.getPeersNodeAddressOptional().isPresent() &&
ownerNodeAddress.equals(connection.getPeersNodeAddressOptional().get())) {
// We have a RequiresLiveOwnerData data object with the node address of the
// disconnected peer. We remove that data from our map.

// Check if we have the data (e.g. OfferPayload)
ByteArray hashOfPayload = get32ByteHashAsByteArray(protectedStoragePayload);
boolean containsKey = map.containsKey(hashOfPayload);
if (containsKey) {
log.debug("We remove the data as the data owner got disconnected with " +
"closeConnectionReason=" + closeConnectionReason);

// We only set the data back by half of the TTL and remove the data only if is has
// expired after that back dating.
// We might get connection drops which are not caused by the node going offline, so
// we give more tolerance with that approach, giving the node the change to
// refresh the TTL with a refresh message.
// We observed those issues during stress tests, but it might have been caused by the
// test set up (many nodes/connections over 1 router)
// TODO investigate what causes the disconnections.
// Usually the are: SOCKET_TIMEOUT ,TERMINATED (EOFException)
protectedStorageEntry.backDate();
if (protectedStorageEntry.isExpired(this.clock)) {
log.info("We found an expired data entry which we have already back dated. " +
"We remove the protectedStoragePayload:\n\t" + Utilities.toTruncatedString(protectedStorageEntry.getProtectedStoragePayload(), 100));
removeFromMapAndDataStore(protectedStorageEntry, hashOfPayload);
}
} else {
log.debug("Remove data ignored as we don't have an entry for that data.");
}
}
}
});
}
if (closeConnectionReason.isIntended)
return;

if (!connection.getPeersNodeAddressOptional().isPresent())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why you prefer using directly getPeersNodeAddressOptional().isPresent() in comparison using hasPeerNodeAddress() ? It is used mixed in the original code as well, so we might use one way for the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish there was a better reason than this one, but I used this pattern because it reduces the number of lint errors from IDEA. If you don't have an isPresent() call prior to a get() it will highlight the word and raise an alert. You can suppress it, but that is just more code to add.

The other benefit is that mocking is bit less error-prone because you just need to mock out the Optional and not the wrapper class that has the hasPeerNodeAddress() function.

return;

NodeAddress peersNodeAddress = connection.getPeersNodeAddressOptional().get();

// Retrieve all the eligible payloads based on the node that disconnected
ArrayList<Map.Entry<ByteArray, ProtectedStorageEntry>> toBackDate =
map.entrySet().stream()
.filter(entry -> entry.getValue().getProtectedStoragePayload() instanceof ExpirablePayload)
.filter(entry -> entry.getValue().getProtectedStoragePayload() instanceof RequiresOwnerIsOnlinePayload)
.filter(entry -> ((RequiresOwnerIsOnlinePayload) entry.getValue().getProtectedStoragePayload()).getOwnerNodeAddress().equals(peersNodeAddress))
.collect(Collectors.toCollection(ArrayList::new));

// Backdate each payload
toBackDate.forEach(mapEntry -> {
// We only set the data back by half of the TTL and remove the data only if is has
// expired after that back dating.
// We might get connection drops which are not caused by the node going offline, so
// we give more tolerance with that approach, giving the node the change to
julianknutsen marked this conversation as resolved.
Show resolved Hide resolved
// refresh the TTL with a refresh message.
// We observed those issues during stress tests, but it might have been caused by the
// test set up (many nodes/connections over 1 router)
// TODO investigate what causes the disconnections.
// Usually the are: SOCKET_TIMEOUT ,TERMINATED (EOFException)
log.debug("We remove the data as the data owner got disconnected with " +
"closeConnectionReason=" + closeConnectionReason);
mapEntry.getValue().backDate();
});

// Remove each backdated payload that is now expired
ArrayList<Map.Entry<ByteArray, ProtectedStorageEntry>> toRemoveList =
toBackDate.stream().filter(mapEntry -> mapEntry.getValue().isExpired(this.clock))
.collect(Collectors.toCollection(ArrayList::new));

toRemoveList.forEach(toRemoveItem -> {
log.debug("We found an expired data entry. We remove the protectedData:\n\t" +
Utilities.toTruncatedString(toRemoveItem.getValue()));
});
removeFromMapAndDataStore(toRemoveList);

if (sequenceNumberMap.size() > this.maxSequenceNumberMapSizeBeforePurge)
sequenceNumberMap.setMap(getPurgedSequenceNumberMap(sequenceNumberMap.getMap()));
}

@Override
Expand Down
21 changes: 7 additions & 14 deletions p2p/src/test/java/bisq/network/p2p/storage/P2PDataStoreDisconnectTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,7 @@ public void setUp() {
// TESTCASE: Bad peer info
@Test
public void peerConnectionUnknown() throws CryptoException, NoSuchAlgorithmException {
when(this.mockedConnection.hasPeersNodeAddress()).thenReturn(false);

when(this.mockedConnection.getPeersNodeAddressOptional()).thenReturn(Optional.empty());
ProtectedStorageEntry protectedStorageEntry = populateTestState(testState, 2);

SavedTestState beforeState = this.testState.saveTestState(protectedStorageEntry);
Expand All @@ -100,8 +99,7 @@ public void peerConnectionUnknown() throws CryptoException, NoSuchAlgorithmExcep
// TESTCASE: Intended disconnects don't trigger expiration
@Test
public void connectionClosedIntended() throws CryptoException, NoSuchAlgorithmException {
when(this.mockedConnection.hasPeersNodeAddress()).thenReturn(true);

when(this.mockedConnection.getPeersNodeAddressOptional()).thenReturn(Optional.of(getTestNodeAddress()));
ProtectedStorageEntry protectedStorageEntry = populateTestState(testState, 2);

SavedTestState beforeState = this.testState.saveTestState(protectedStorageEntry);
Expand All @@ -114,8 +112,7 @@ public void connectionClosedIntended() throws CryptoException, NoSuchAlgorithmEx
// TESTCASE: Peer NodeAddress unknown
@Test
public void connectionClosedSkipsItemsPeerInfoBadState() throws NoSuchAlgorithmException, CryptoException {
when(this.mockedConnection.hasPeersNodeAddress()).thenReturn(true);
when(mockedConnection.getPeersNodeAddressOptional()).thenReturn(Optional.empty());
when(this.mockedConnection.getPeersNodeAddressOptional()).thenReturn(Optional.empty());

ProtectedStorageEntry protectedStorageEntry = populateTestState(testState, 2);

Expand All @@ -129,8 +126,7 @@ public void connectionClosedSkipsItemsPeerInfoBadState() throws NoSuchAlgorithmE
// TESTCASE: Unintended disconnects reduce the TTL for entrys that match disconnected peer
@Test
public void connectionClosedReduceTTL() throws NoSuchAlgorithmException, CryptoException {
when(this.mockedConnection.hasPeersNodeAddress()).thenReturn(true);
when(mockedConnection.getPeersNodeAddressOptional()).thenReturn(Optional.of(TestState.getTestNodeAddress()));
when(this.mockedConnection.getPeersNodeAddressOptional()).thenReturn(Optional.of(getTestNodeAddress()));

ProtectedStorageEntry protectedStorageEntry = populateTestState(testState, TimeUnit.DAYS.toMillis(90));

Expand All @@ -144,8 +140,7 @@ public void connectionClosedReduceTTL() throws NoSuchAlgorithmException, CryptoE
// TESTCASE: Unintended disconnects don't reduce TTL for entrys that are not from disconnected peer
@Test
public void connectionClosedSkipsItemsNotFromPeer() throws NoSuchAlgorithmException, CryptoException {
when(this.mockedConnection.hasPeersNodeAddress()).thenReturn(true);
when(mockedConnection.getPeersNodeAddressOptional()).thenReturn(Optional.of(new NodeAddress("notTestNode", 2020)));
when(this.mockedConnection.getPeersNodeAddressOptional()).thenReturn(Optional.of(new NodeAddress("notTestNode", 2020)));

ProtectedStorageEntry protectedStorageEntry = populateTestState(testState, 2);

Expand All @@ -159,8 +154,7 @@ public void connectionClosedSkipsItemsNotFromPeer() throws NoSuchAlgorithmExcept
// TESTCASE: Unintended disconnects expire entrys that match disconnected peer and TTL is low enough for expire
@Test
public void connectionClosedReduceTTLAndExpireItemsFromPeer() throws NoSuchAlgorithmException, CryptoException {
when(this.mockedConnection.hasPeersNodeAddress()).thenReturn(true);
when(mockedConnection.getPeersNodeAddressOptional()).thenReturn(Optional.of(TestState.getTestNodeAddress()));
when(this.mockedConnection.getPeersNodeAddressOptional()).thenReturn(Optional.of(getTestNodeAddress()));

ProtectedStorageEntry protectedStorageEntry = populateTestState(testState, 2);

Expand Down Expand Up @@ -188,8 +182,7 @@ private ExpirablePersistentProtectedStoragePayloadStub(PublicKey ownerPubKey) {
}
}

when(this.mockedConnection.hasPeersNodeAddress()).thenReturn(true);
when(mockedConnection.getPeersNodeAddressOptional()).thenReturn(Optional.of(TestState.getTestNodeAddress()));
when(this.mockedConnection.getPeersNodeAddressOptional()).thenReturn(Optional.of(getTestNodeAddress()));

KeyPair ownerKeys = TestUtils.generateKeyPair();
ProtectedStoragePayload protectedStoragePayload =
Expand Down