Add filter support on network level #5038
Conversation
p2PService.getP2PDataStorage().getAppendOnlyDataStoreMap(). p2PService.getP2PDataStorage().getAppendOnlyDataStoreMap() iterates over all services including the historical data store service. It used the getMap method which should not be used at historical data store service as it is not clear if the live data or all data should be accessed.
HistoricalDataStoreService.getMap is called. HistoricalDataStoreService.getMap should not be used by domain clients but rather the custom methods getMapOfAllData, getMapOfLiveData or getMapSinceVersion. As we have not removed the calls from ProposalService and other domains we return getMapOfAllData() instead of the live map. This was prevented earlier for performance reasons. It is more safe thought to return in case of an illegal access all data instead of live data only.
CloseConnectionMessage as it would trigger a wrong log on the peers side, that he got banned.
The numbers did not match up from delivered response size and items as we did not count in the overhead of the ProtectedStorageEntry (pub key+sig) and did estimate the size with taking only first item and multiplying it. A measurement resulted in 20 ms costs for the exact calculation (toProtoMessage().getSerializedSize() has some costs). I guess that is acceptable to get correct metrics.
chimp1984
force-pushed
the
add-filter-support-on-network-level
branch
from
9837bde to
818c797
Compare
p2p/src/main/java/bisq/network/p2p/peers/getdata/RequestDataHandler.java
Show resolved
Hide resolved
| // added at v1.5.5 | ||
| // In contrast to bannedNodeAddress those addresses cannot access the network but are rejected immediately. | ||
| // Should be only used in case of dos attacks | ||
| private final Set<String> nodeAddressesBannedFromNetwork; |
There was a problem hiding this comment.
Naming is confusing since there is a bannedNodeAddress and a nodeAddressesBannedFromNetwork, better to have more descriptive names.
There was a problem hiding this comment.
I think nodeAddressesBannedFromNetwork is pretty clear (and long ;-)) no? The bannedNodeAddress is not great but renaming it would require a rename if the protobuf field as well to avoid that those fields get out of sync and was not sure if thats worth it. Also not clear for a good alternative name.
There was a problem hiding this comment.
Renamed it to nodeAddressesBannedFromTrading
There was a problem hiding this comment.
Much better this way, no need for comments since code is self describing. Agreed that it was the bannedNodeAddress that needed a name change.
|
This is dangerous because if you accidentally filter out all nodes the entire P2P network would go down immediately and no way to fix it - maybe include some protection for that |
You have to add a list of onions, we cannot even know all nodes... And the filter can be ignored by a prog arg (--ignoreDevMsg). |
Adds a new field to the filter window for banning nodes on the network level. The existing one is only for banning nodes from trading.
Based on #5031
Starts at commit 15b8f64