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

(8/8) Persist changes to ProtectedStoragePayload objects implementing PersistablePayload #3640

Merged
merged 26 commits into from
Nov 26, 2019

Commits on Nov 19, 2019

  1. [PR COMMENTS] Make maxSequenceNumberBeforePurge final

    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.
    julianknutsen committed Nov 19, 2019
    Configuration menu
    Copy the full SHA
    617585d View commit details
    Browse the repository at this point in the history
  2. [TESTS] Clean up 'Analyze Code' warnings

    Remove unused imports and clean up some access modifiers now that
    the final test structure is complete
    julianknutsen committed Nov 19, 2019
    Configuration menu
    Copy the full SHA
    3bd67ba View commit details
    Browse the repository at this point in the history
  3. [REFACTOR] HashMapListener::onAdded/onRemoved

    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.
    julianknutsen committed Nov 19, 2019
    Configuration menu
    Copy the full SHA
    b281566 View commit details
    Browse the repository at this point in the history
  4. [REFACTOR] removeFromMapAndDataStore can operate on Collections

    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.
    julianknutsen committed Nov 19, 2019
    Configuration menu
    Copy the full SHA
    4f08588 View commit details
    Browse the repository at this point in the history
  5. Change removeFromMapAndDataStore to signal listeners at the end in a …

    …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.
    julianknutsen committed Nov 19, 2019
    Configuration menu
    Copy the full SHA
    489b25a View commit details
    Browse the repository at this point in the history
  6. Update removeExpiredEntries to remove all items 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.
    julianknutsen committed Nov 19, 2019
    Configuration menu
    Copy the full SHA
    eae641e View commit details
    Browse the repository at this point in the history
  7. ProposalService::onProtectedDataRemoved signals listeners once on bat…

    …ch removes
    
    bisq-network#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 bisq-network#3148 and those interfaces will be removed in
    the next patch.
    julianknutsen committed Nov 19, 2019
    Configuration menu
    Copy the full SHA
    a50e59f View commit details
    Browse the repository at this point in the history
  8. Remove HashmapChangedListener::onBatch operations

    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.
    julianknutsen committed Nov 19, 2019
    Configuration menu
    Copy the full SHA
    a8139f3 View commit details
    Browse the repository at this point in the history
  9. [TESTS] Regression test for bisq-network#3629

    Write a test that shows the incorrect behavior for bisq-network#3629, the hashmap
    is rebuilt from disk using the 20-byte key instead of the 32-byte key.
    julianknutsen committed Nov 19, 2019
    Configuration menu
    Copy the full SHA
    849155a View commit details
    Browse the repository at this point in the history

Commits on Nov 21, 2019

  1. [BUGFIX] Reconstruct HashMap using 32-byte key

    Addresses the first half of bisq-network#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.
    julianknutsen committed Nov 21, 2019
    Configuration menu
    Copy the full SHA
    e212240 View commit details
    Browse the repository at this point in the history
  2. [BUGFIX] Use 32-byte key in requestData path

    Addresses the second half of bisq-network#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 bisq-network#3629
    julianknutsen committed Nov 21, 2019
    Configuration menu
    Copy the full SHA
    455f7d2 View commit details
    Browse the repository at this point in the history
  3. [DEAD CODE] Remove getProtectedDataStoreMap

    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 committed Nov 21, 2019
    Configuration menu
    Copy the full SHA
    793e84d View commit details
    Browse the repository at this point in the history
  4. [TESTS] Allow tests to validate SequenceNumberMap write separately

    In order to implement remove-before-add behavior, we need a way to
    verify that the SequenceNumberMap was the only item updated.
    julianknutsen committed Nov 21, 2019
    Configuration menu
    Copy the full SHA
    526aee5 View commit details
    Browse the repository at this point in the history
  5. Implement remove-before-add message sequence behavior

    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.
    julianknutsen committed Nov 21, 2019
    Configuration menu
    Copy the full SHA
    372c26d View commit details
    Browse the repository at this point in the history

Commits on Nov 22, 2019

  1. [TESTS] Allow remove() verification to be more flexible

    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.
    julianknutsen committed Nov 22, 2019
    Configuration menu
    Copy the full SHA
    931c1f4 View commit details
    Browse the repository at this point in the history
  2. Broadcast remove-before-add messages to P2P network

    In order to aid in propagation of remove() messages, broadcast them
    in the event the remove is seen before the add.
    julianknutsen committed Nov 22, 2019
    Configuration menu
    Copy the full SHA
    0472ffc View commit details
    Browse the repository at this point in the history
  3. [TESTS] Clean up remove verification helpers

    Now that there are cases where the SequenceNumberMap and Broadcast
    are called, but no other internal state is updated, the existing helper
    functions conflate too many decisions. Remove them in favor of explicitly
    defining each state change expected.
    julianknutsen committed Nov 22, 2019
    Configuration menu
    Copy the full SHA
    6e2ea6e View commit details
    Browse the repository at this point in the history

Commits on Nov 25, 2019

  1. [BUGFIX] Fix duplicate sequence number use case (startup)

    Fix a bug introduced in d484617 that
    did not properly handle a valid use case for duplicate sequence numbers.
    
    For in-memory-only ProtectedStoragePayloads, the client nodes need a way
    to reconstruct the Payloads after startup from peer and seed nodes. This
    involves sending a ProtectedStorageEntry with a sequence number that
    is equal to the last one the client had already seen.
    
    This patch adds tests to confirm the bug and fix as well as the changes
    necessary to allow adding of Payloads that were previously seen, but
    removed during a restart.
    julianknutsen committed Nov 25, 2019
    Configuration menu
    Copy the full SHA
    3d571c4 View commit details
    Browse the repository at this point in the history
  2. Clean up AtomicBoolean usage in FileManager

    Although the code was correct, it was hard to understand the relationship
    between the to-be-written object and the savePending flag.
    
    Trade two dependent atomics for one and comment the code to make it more
    clear for the next reader.
    julianknutsen committed Nov 25, 2019
    Configuration menu
    Copy the full SHA
    2208003 View commit details
    Browse the repository at this point in the history
  3. Configuration menu
    Copy the full SHA
    1895802 View commit details
    Browse the repository at this point in the history
  4. [BUGFIX] Shorter delay values not taking precedence

    Fix a bug in the FileManager where a saveLater called with a low delay
    won't execute until the delay specified by a previous saveLater call.
    
    The trade off here is the execution of a task that returns early vs.
    losing the requested delay.
    julianknutsen committed Nov 25, 2019
    Configuration menu
    Copy the full SHA
    d4d2f26 View commit details
    Browse the repository at this point in the history
  5. [REFACTOR] Inline saveNowInternal

    Only one caller after deadcode removal.
    julianknutsen committed Nov 25, 2019
    Configuration menu
    Copy the full SHA
    685824b View commit details
    Browse the repository at this point in the history

Commits on Nov 26, 2019

  1. [TESTS] Introduce MapStoreServiceFake

    Now that we want to make changes to the MapStoreService,
    it isn't sufficient to have a Fake of the ProtectedDataStoreService.
    
    Tests now use a REAL ProtectedDataStoreService and a FAKE MapStoreService
    to exercise more of the production code and allow future testing of
    changes to MapStoreService.
    julianknutsen committed Nov 26, 2019
    Configuration menu
    Copy the full SHA
    66f71e5 View commit details
    Browse the repository at this point in the history
  2. Persist changes to ProtectedStorageEntrys

    With the addition of ProtectedStorageEntrys, there are now persistable
    maps that have different payloads and the same keys. In the
    ProtectedDataStoreService case, the value is the ProtectedStorageEntry
    which has a createdTimeStamp, sequenceNumber, and signature that can
    all change, but still contain an identical payload.
    
    Previously, the service was only updating the on-disk representation on
    the first object and never again. So, when it was recreated from disk it
    would not have any of the updated metadata. This was just copied from the
    append-only implementation where the value was the Payload
    which was immutable.
    
    This hasn't caused any issues to this point, but it causes strange behavior
    such as always receiving seqNr==1 items from seednodes on startup. It
    is good practice to keep the in-memory objects and on-disk objects in
    sync and removes an unexpected failure in future dev work that expects
    the same behavior as the append-only on-disk objects.
    julianknutsen committed Nov 26, 2019
    Configuration menu
    Copy the full SHA
    f3faf4b View commit details
    Browse the repository at this point in the history
  3. [DEADCODE] Remove protectedDataStoreListener

    There were no users.
    julianknutsen committed Nov 26, 2019
    Configuration menu
    Copy the full SHA
    3503fe3 View commit details
    Browse the repository at this point in the history
  4. Configuration menu
    Copy the full SHA
    44a11a0 View commit details
    Browse the repository at this point in the history