Now that we have introduced remove-before-add, we need a way
to validate that the SequenceNumberMap was written, but nothing
else. Add this feature to the validation path.
It is possible to receive a RemoveData or RemoveMailboxData message
before the relevant AddData, but the current code does not handle
it.
This results in internal state updates and signal handler's being called
when an Add is received with a lower sequence number than a previously
seen Remove.
Minor test validation changes to allow tests to specify that only the
SequenceNumberMap should be written during an operation.
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
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.
Now that the only user of this interface has been removed, go ahead
and delete it. This is a partial revert of
f5d75c4f60 that includes the code that was
added into ProposalService that subscribed to the P2PDataStore.
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.
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.
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.
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.
The remove code checks to ensure these fields match, but the add code
never did. This could lead to a situation where a MailboxStoragePayload
could be added, but never removed.
Previously, the expire path, the remove path, and the onDisconnect
all used separate logic for updating the map, signaling listeners, and
removing PersistablePaylod objects from the data store. This led to a
bug where the onDisconnect path did not update the protectedDataStore.
Combine the three code paths to ensure that the same state is updated
regardless of the context.
The code to remove expired Entrys in the onDisconnect path was not
correctly removing the Entry from the protectedDataStore.
This patch adds a test that failed and fixes the bug.
* All of this work is done on the UserThread so there is no need to
clone the map.
* ArrayList objects are faster to iterate than HashSets and the data is
guaranteed to be unique since the source is a ConcurrentHashMap
* Finding all items to remove first, then removing them all is an easier
to read code pattern instead of removing during iteration.
It is currently possible to construct a valid Payload object
that implements both the ProtectedStoragePayload and
PersistableNetworkPayload interfaces even though this combination is
invalid.
Instead of depending on future reviewers to catch an error, assert that
ProtectedStoragePayloads and PersistableNetworkPayloads are incompatible
as objects inside a ProtectedStorageEntry.
This allows cleanup of removeExpiredEntries that branched on this
behavior.
The old PeerManagerTest was located under network/p2p/routing, which is
no longer the correct location. Additionally, it was outdated so I
just removed it and added a new file under network/p2p/peers containing
tests for checkMaxConnections.
All test callers now just ask the TestState for a SavedTestState instead
of SavedTestState ctor. This makes more sense with the object
relationship since SavedTestState is only used internally to TestState.
One monolithic test was useful when it was under development to reduce
code churn, but now that the tests are complete it is easier to find
and run a specific test when separated into separate test files.
This also fixes a downside of Enclosed.class that didn't allow individual
tests to be run in intellij.
Now that the unit tests cover all of the per-Entry validation,
the tests that create specific configuration of ProtectedStorageEntry
and ProtectedMailboxStorageEntry objects can be removed in favor
of mockable Entrys.
Using mocks prior to this patch was impossible due to the relationship
between the Entry objects and the P2PDataStorage helper functions. Now
that the objects are properly abstracted and tested, real unit tests
can be written for the P2PDataStore module.
This patch leaves the tests and adds an @Ignore so the reviewer can see
which unit test now supersedes the integration test.
Use a more compact version of string formatting
in log messages
Rename isMetadataEquals to matchesRelevantPubKey
which is more descriptive of the actual check
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 #3556
Make the remove validation more robust by asserting that the
correct remove message is broadcast. This will provide a better
safety net when combining the remove functions.
Let the objects compare their metadata instead of doing it for them. This
allows for actual unit testing and paves the way for deduplicating the
remove code paths.
This patch also removes an unnecessary check around comparing the hash
of the stored data to the new data's hash. That check can't fail since
the hash was a requirement for the map lookup in the first place.
The current check verifies that the stored Payload.ownerPubKey == stored Entry.ownerPubKey.
This is the same check that was done when the item was originally added
and there is no reason to do it again.
This mailbox-only check can now exist inside the object for which it
belongs. This makes it easier to test and moves closer to allowing
the deduplication of the remove() methods.
Move the signature checks into the objects to clean up the calling code
and make it more testable.
The testing now has to take real hashes so some work was done in the fixtures
to create valid hashable objects.
Now that the objects can answer questions about valid conditions
for add/remove, ask them directly.
This also pushes the logging down into the ProtectedStorageEntry and
ProtectedMailboxStorageEntry and cleans up the message.
Method bodies are copied from P2PDataStore to separate refactoring
efforts and behavior changes.
Identified a bug where a ProtectedMailboxStorageEntry mailbox entry
could be added, but never removed.
The code around validating MailboxStoragePayloads is subtle when
a MailboxStoragePayload is wrapped in a ProtectedStorageEntry. Add tests
to document the current behavior.
The custom code to verify the refreshTTLMessage's signature and update
an entry isn't necessary. Just have the code construct an updated
ProtectedStorageEntry from the existing and new data, verify it,
and add it to the map.
This also allows the removal of the ProtectedStorageEntry APIs
that modify internal state.
The original test would take over 5 seconds. Allow tests to set the number
of required entries before purge to a lower value so the tests
can run faster with the same confidence.
Add tests for removing expired entries and optionally purging
the sequence number map. Now possible since these tests have
control over time with the ClockFake.
The remove validation needed to be improved since deletes through
the expire path don't signal HashMap listeners or write sequence numbers.
Reduces non-deterministic failures of the refreshTTL tests that resulted
from the uncontrollable System.currentTimeMillis().
Now, all tests have extremely fine control over the elapsed time between
calls which makes the current and future tests much better.
