[bisq-network/bisq] Test and Refactor P2PDataStorage Synchronization Path (#3667)

Julian Knutsen notifications at github.com
Sat Nov 23 17:47:39 UTC 2019 

Starts 62f6bac

Submitting as a draft since it touches the `GetData` logic and it may be a good idea to let this bake on master if the code freeze is close.  The only bug that was fixed was around truncating entries and it isn't needed in a hotfix.

But, the patch stack is only getting larger and earlier eyes on the changes will limit code churn later.

## Motivation
The GetData synchronization sequence was currently untested and the code was tangled between the connection request handling logic and the `P2PDataStorage` update logic. In order to implement a new feature (**_replay removes during synchronization_**) I needed a way to verify current behavior and write tests for the changes I am making.

## Changes
This PR moves the `GetDataRequest`->`GetDataResponse`->`ProcessDataResponse` logic into the
P2PDataStorage where it can operate locally on the various data structures that need to be accessed
and updated. This has a few benefits:

- Easier testing: tests don't need to mock `Future` and `Connection` to validate behavior
- Public APIs can be removed that allow operations on internal data structures in favor of letting
  the P2PDataStorage handle the adds/removes/updates.
- Optimizations can be handled internally to the `P2PDataStorage` limiting external knowledge of implementation details
- Conceptually, the synchronization logic is all about synchronizing two separate `P2PDataStorage` instances so letting the actual objects build and apply the synchronization messages makes more intuitive sense.

## Tests
Full unit test for each individual stage now exists as well as an integration test that creates two separate `P2PDataStorage` instances, runs the synchronization logic, and verifies they are synchronized. This type of "multiple node" testing will be used to validate the new feature development and will limit errors in development.

## Maintainability fixes
Since this patch tests the `GetData` messages sent back and forth, there was an opportunity to clean up the guarantees around null vs. empty w.r.t. sets and Capabilities. I think this is a good pattern moving forward to limit accidental NullPointerExceptions when we can do something less brittle.






You can view, comment on, or merge this pull request online at:

  https://github.com/bisq-network/bisq/pull/3667

-- Commit Summary --

  * [PR COMMENTS] Make maxSequenceNumberBeforePurge final
  * [TESTS] Clean up 'Analyze Code' warnings
  * [REFACTOR] HashMapListener::onAdded/onRemoved
  * [REFACTOR] removeFromMapAndDataStore can operate on Collections
  * Change removeFromMapAndDataStore to signal listeners at the end in a batch
  * Update removeExpiredEntries to remove all items in a batch
  * ProposalService::onProtectedDataRemoved signals listeners once on batch removes
  * Remove HashmapChangedListener::onBatch operations
  * [TESTS] Regression test for #3629
  * [BUGFIX] Reconstruct HashMap using 32-byte key
  * [BUGFIX] Use 32-byte key in requestData path
  * [DEAD CODE] Remove getProtectedDataStoreMap
  * [TESTS] Allow tests to validate SequenceNumberMap write separately
  * Implement remove-before-add message sequence behavior
  * [TESTS] Allow remove() verification to be more flexible
  * Broadcast remove-before-add messages to P2P network
  * [TESTS] Clean up remove verification helpers
  * [TESTS] Introduce MapStoreServiceFake
  * Persist changes to ProtectedStorageEntrys
  * [DEADCODE] Remove protectedDataStoreListener
  * [DEADCODE] Remove unused methods in ProtectedDataStoreService
  * [BUGFIX] Fix duplicate sequence number use case (startup)
  * [TESTS] Add tests of requestData
  * [REFACTOR] Introduce buildGetDataRequest variants
  * [TESTS] Add tests of new RequestData APIs
  * [TESTS] Add tests of GetDataRequestHandler
  * [REFACTOR] Introduce buildGetDataResponse
  * [REFACTOR] Extract connectionInfo String
  * [REFACTOR] Extract getDataResponse logging
  * [REFACTOR] Extract truncation logging
  * [REFACTOR] Pass peerCapabilities into buildGetDataResponse
  * [TESTS] Unit tests of buildGetDataResponse
  * Remove redundant HashSet lookups in filter functions
  * [REFACTOR] Move required capabilities log
  * [REFACTOR] Inline capability check for ProtectedStorageEntries
  * [REFACTOR] Inline filtering functions
  * [REFACTOR] Remove duplication in filtering functions
  * [BUGFIX] Fix off-by-one in truncation logic
  * [TESTS] Add test of RequestDataHandler::onMessage
  * [REFACTOR] Introduce processGetDataResponse
  * [TESTS] Make verify() functions more flexible
  * [TESTS] Add unit tests for processGetDataResponse
  * Remove static from initialRequestApplied
  * [TESTS] Write synchronization integration tests
  * [REFACTOR] Clean up processGetDataResponse
  * [RENAME] LazyProcessedPayload to ProcessOncePersistableNetworkPayload
  * Remove @Nullable around persistableNetworkPayloadSet
  * Remove @Nullable around supportedCapabilities in GetDataResponse
  * Remove @Nullable around supportedCapabilities in PreliminaryGetDataRequest
  * [DEADCODE] Remove old request handler tests

-- File Changes --

    M core/src/main/java/bisq/core/account/sign/SignedWitness.java (4)
    M core/src/main/java/bisq/core/account/witness/AccountAgeWitness.java (4)
    M core/src/main/java/bisq/core/alert/AlertManager.java (32)
    M core/src/main/java/bisq/core/dao/governance/proposal/ProposalListPresentation.java (49)
    M core/src/main/java/bisq/core/dao/governance/proposal/ProposalService.java (65)
    M core/src/main/java/bisq/core/dao/governance/proposal/storage/temp/TempProposalPayload.java (4)
    M core/src/main/java/bisq/core/dao/governance/proposal/storage/temp/TempProposalStore.java (2)
    M core/src/main/java/bisq/core/filter/FilterManager.java (31)
    M core/src/main/java/bisq/core/offer/OfferBookService.java (37)
    M core/src/main/java/bisq/core/support/dispute/agent/DisputeAgentManager.java (23)
    M core/src/main/java/bisq/core/trade/statistics/TradeStatistics.java (4)
    M core/src/main/java/bisq/core/trade/statistics/TradeStatistics2.java (4)
    A core/src/test/java/bisq/core/dao/governance/proposal/ProposalServiceP2PDataStorageListenerTest.java (127)
    M monitor/src/main/java/bisq/monitor/metric/P2PMarketStats.java (19)
    M monitor/src/main/java/bisq/monitor/metric/P2PSeedNodeSnapshot.java (19)
    M p2p/src/main/java/bisq/network/p2p/P2PModule.java (1)
    M p2p/src/main/java/bisq/network/p2p/P2PService.java (13)
    M p2p/src/main/java/bisq/network/p2p/peers/getdata/GetDataRequestHandler.java (122)
    M p2p/src/main/java/bisq/network/p2p/peers/getdata/RequestDataHandler.java (92)
    M p2p/src/main/java/bisq/network/p2p/peers/getdata/messages/GetDataResponse.java (35)
    M p2p/src/main/java/bisq/network/p2p/peers/getdata/messages/PreliminaryGetDataRequest.java (21)
    M p2p/src/main/java/bisq/network/p2p/storage/HashMapChangedListener.java (14)
    M p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java (336)
    R p2p/src/main/java/bisq/network/p2p/storage/payload/ProcessOncePersistableNetworkPayload.java (7)
    M p2p/src/main/java/bisq/network/p2p/storage/persistence/MapStoreService.java (5)
    D p2p/src/main/java/bisq/network/p2p/storage/persistence/ProtectedDataStoreListener.java (26)
    M p2p/src/main/java/bisq/network/p2p/storage/persistence/ProtectedDataStoreService.java (16)
    A p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageBuildGetDataResponseTest.java (481)
    M p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageClientAPITest.java (14)
    A p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageGetDataIntegrationTest.java (193)
    M p2p/src/test/java/bisq/network/p2p/storage/P2PDataStoragePersistableNetworkPayloadTest.java (5)
    A p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProcessGetDataResponse.java (246)
    M p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java (128)
    M p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRemoveExpiredTest.java (32)
    A p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRequestDataTest.java (175)
    M p2p/src/test/java/bisq/network/p2p/storage/P2PDataStoreDisconnectTest.java (7)
    M p2p/src/test/java/bisq/network/p2p/storage/TestState.java (259)
    R p2p/src/test/java/bisq/network/p2p/storage/mocks/MapStoreServiceFake.java (43)
    M p2p/src/test/java/bisq/network/p2p/storage/mocks/PersistableNetworkPayloadStub.java (12)
    M p2p/src/test/java/bisq/network/p2p/storage/mocks/ProtectedStoragePayloadStub.java (2)

-- Patch Links --

https://github.com/bisq-network/bisq/pull/3667.patch
https://github.com/bisq-network/bisq/pull/3667.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/3667
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20191123/8aa7e43e/attachment-0001.html>

More information about the bisq-github mailing list