Skip to content

Commit

Permalink
[BUGFIX] Reconstruct HashMap using 32-byte key
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@julianknutsen
julianknutsen committed Nov 21, 2019
1 parent 849155a commit e212240
Show file tree
Hide file tree
Showing 4 changed files with 12 additions and 27 deletions.
2 changes: 1 addition & 1 deletion core/src/main/java/bisq/core/dao/governance/proposal/storage/temp/TempProposalStore.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public class TempProposalStore implements PersistableEnvelope {
///////////////////////////////////////////////////////////////////////////////////////////

private TempProposalStore(List<ProtectedStorageEntry> list) {
list.forEach(entry -> map.put(P2PDataStorage.getCompactHashAsByteArray(entry.getProtectedStoragePayload()), entry));
list.forEach(entry -> map.put(P2PDataStorage.get32ByteHashAsByteArray(entry.getProtectedStoragePayload()), entry));
}

public Message toProtoMessage() {
Expand Down
14 changes: 2 additions & 12 deletions p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -423,8 +423,7 @@ public boolean addProtectedStorageEntry(ProtectedStorageEntry protectedStorageEn

// Persist ProtectedStorageEntrys carrying PersistablePayload payloads and signal listeners on changes
if (protectedStoragePayload instanceof PersistablePayload) {
ByteArray compactHash = P2PDataStorage.getCompactHashAsByteArray(protectedStoragePayload);
ProtectedStorageEntry previous = protectedDataStoreService.putIfAbsent(compactHash, protectedStorageEntry);
ProtectedStorageEntry previous = protectedDataStoreService.putIfAbsent(hashOfPayload, protectedStorageEntry);
if (previous == null)
protectedDataStoreListeners.forEach(e -> e.onAdded(protectedStorageEntry));
}
Expand Down Expand Up @@ -663,8 +662,7 @@ private void removeFromMapAndDataStore(

ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload();
if (protectedStoragePayload instanceof PersistablePayload) {
ByteArray compactHash = getCompactHashAsByteArray(protectedStoragePayload);
ProtectedStorageEntry previous = protectedDataStoreService.remove(compactHash, protectedStorageEntry);
ProtectedStorageEntry previous = protectedDataStoreService.remove(hashOfPayload, protectedStorageEntry);
if (previous != null) {
protectedDataStoreListeners.forEach(e -> e.onRemoved(protectedStorageEntry));
} else {
Expand Down Expand Up @@ -714,14 +712,6 @@ public static ByteArray get32ByteHashAsByteArray(NetworkPayload data) {
return new ByteArray(P2PDataStorage.get32ByteHash(data));
}

public static ByteArray getCompactHashAsByteArray(ProtectedStoragePayload protectedStoragePayload) {
return new ByteArray(getCompactHash(protectedStoragePayload));
}

private static byte[] getCompactHash(ProtectedStoragePayload protectedStoragePayload) {
return Hash.getSha256Ripemd160hash(protectedStoragePayload.toProtoMessage().toByteArray());
}

// Get a new map with entries older than PURGE_AGE_DAYS purged from the given map.
private Map<ByteArray, MapValue> getPurgedSequenceNumberMap(Map<ByteArray, MapValue> persisted) {
Map<ByteArray, MapValue> purged = new HashMap<>();
Expand Down
8 changes: 3 additions & 5 deletions p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -575,8 +575,7 @@ protected Class<ProtectedStorageEntry> getEntryClass() {

// Tests that just apply to PersistablePayload objects

// XXXBUG_3629XXX: Persisted ProtectedStorageEntries are saved to disk via their 20-byte hash. This causes
// the internal hash map to be reloaded with the 20-byte key instead of the 32-byte key.
// TESTCASE: Ensure the HashMap is the same before and after a restart
@Test
public void addProtectedStorageEntry_afterReadFromResourcesWithDuplicate_3629RegressionTest() {
ProtectedStorageEntry protectedStorageEntry = this.getProtectedStorageEntryForAdd(1);
Expand All @@ -585,9 +584,8 @@ public void addProtectedStorageEntry_afterReadFromResourcesWithDuplicate_3629Reg
Map<P2PDataStorage.ByteArray, ProtectedStorageEntry> beforeRestart = this.testState.mockedStorage.getMap();

this.testState.simulateRestart();

// Should be equal
Assert.assertNotEquals(beforeRestart, this.testState.mockedStorage.getMap());

Assert.assertEquals(beforeRestart, this.testState.mockedStorage.getMap());
}
}

Expand Down
15 changes: 6 additions & 9 deletions p2p/src/test/java/bisq/network/p2p/storage/TestState.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,6 @@ void verifyProtectedStorageAdd(SavedTestState beforeState,
boolean expectedStateChange,
boolean expectedIsDataOwner) {
P2PDataStorage.ByteArray hashMapHash = P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload());
P2PDataStorage.ByteArray storageHash = P2PDataStorage.getCompactHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload());

if (expectedStateChange) {
Assert.assertEquals(protectedStorageEntry, this.mockedStorage.getMap().get(hashMapHash));
Expand All @@ -225,10 +224,10 @@ void verifyProtectedStorageAdd(SavedTestState beforeState,
// TODO: Should the behavior be identical between this and the HashMap listeners?
// TODO: Do we want ot overwrite stale values in order to persist updated sequence numbers and timestamps?
if (protectedStorageEntry.getProtectedStoragePayload() instanceof PersistablePayload && beforeState.protectedStorageEntryBeforeOpDataStoreMap == null) {
Assert.assertEquals(protectedStorageEntry, this.mockedStorage.getProtectedDataStoreMap().get(storageHash));
Assert.assertEquals(protectedStorageEntry, this.mockedStorage.getProtectedDataStoreMap().get(hashMapHash));
verify(this.protectedDataStoreListener).onAdded(protectedStorageEntry);
} else {
Assert.assertEquals(beforeState.protectedStorageEntryBeforeOpDataStoreMap, this.mockedStorage.getProtectedDataStoreMap().get(storageHash));
Assert.assertEquals(beforeState.protectedStorageEntryBeforeOpDataStoreMap, this.mockedStorage.getProtectedDataStoreMap().get(hashMapHash));
verify(this.protectedDataStoreListener, never()).onAdded(protectedStorageEntry);
}

Expand All @@ -245,7 +244,7 @@ void verifyProtectedStorageAdd(SavedTestState beforeState,
this.verifySequenceNumberMapWriteContains(P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()), protectedStorageEntry.getSequenceNumber());
} else {
Assert.assertEquals(beforeState.protectedStorageEntryBeforeOp, this.mockedStorage.getMap().get(hashMapHash));
Assert.assertEquals(beforeState.protectedStorageEntryBeforeOpDataStoreMap, this.mockedStorage.getProtectedDataStoreMap().get(storageHash));
Assert.assertEquals(beforeState.protectedStorageEntryBeforeOpDataStoreMap, this.mockedStorage.getProtectedDataStoreMap().get(hashMapHash));

verify(this.mockBroadcaster, never()).broadcast(any(BroadcastMessage.class), any(NodeAddress.class), any(BroadcastHandler.Listener.class), anyBoolean());

Expand Down Expand Up @@ -294,13 +293,12 @@ void verifyProtectedStorageRemove(SavedTestState beforeState,

protectedStorageEntries.forEach(protectedStorageEntry -> {
P2PDataStorage.ByteArray hashMapHash = P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload());
P2PDataStorage.ByteArray storageHash = P2PDataStorage.getCompactHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload());

if (expectedStateChange) {
Assert.assertNull(this.mockedStorage.getMap().get(hashMapHash));

if (protectedStorageEntry.getProtectedStoragePayload() instanceof PersistablePayload) {
Assert.assertNull(this.mockedStorage.getProtectedDataStoreMap().get(storageHash));
Assert.assertNull(this.mockedStorage.getProtectedDataStoreMap().get(hashMapHash));

verify(this.protectedDataStoreListener).onRemoved(protectedStorageEntry);
}
Expand Down Expand Up @@ -420,11 +418,10 @@ private SavedTestState(TestState testState, PersistableNetworkPayload persistabl
private SavedTestState(TestState testState, ProtectedStorageEntry protectedStorageEntry) {
this(testState);

P2PDataStorage.ByteArray storageHash = P2PDataStorage.getCompactHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload());
this.protectedStorageEntryBeforeOpDataStoreMap = testState.mockedStorage.getProtectedDataStoreMap().get(storageHash);

P2PDataStorage.ByteArray hashMapHash = P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload());
this.protectedStorageEntryBeforeOp = testState.mockedStorage.getMap().get(hashMapHash);
this.protectedStorageEntryBeforeOpDataStoreMap = testState.mockedStorage.getProtectedDataStoreMap().get(hashMapHash);


this.creationTimestampBeforeUpdate = (this.protectedStorageEntryBeforeOp != null) ? this.protectedStorageEntryBeforeOp.getCreationTimeStamp() : 0;
}
Expand Down

0 comments on commit e212240

Please sign in to comment.