[DRAFT] Improve db max size and prune handling #3983
Conversation
WalkthroughThis PR systematically extends MetaData initialization across multiple domain classes to include configurable Changes
Sequence Diagram(s)sequenceDiagram
participant Deserializer as Protobuf Deserializer
participant MetaData as MetaData Constructor
participant Runtime as Runtime Map Storage
Deserializer->>MetaData: fromProto(ttl, priority, className, maxMapSize)
activate MetaData
Note over MetaData: Check MAX_MAP_SIZE_BY_CLASS_NAME runtime map
alt Found in runtime map
MetaData->>MetaData: Use runtime maxMapSize
else Check DEFAULT_MAX_MAP_SIZE_BY_CLASS_NAME defaults
alt Found in defaults
MetaData->>MetaData: Use default maxMapSize
else Use provided maxMapSize
MetaData->>MetaData: Use fallback + log warning
end
end
MetaData->>Runtime: putIfAbsent(className, maxMapSize)
MetaData-->>Deserializer: MetaData instance created
deactivate MetaData
sequenceDiagram
participant DataStorageService as DataStorageService.prunePersisted()
participant Pruner as Pruning Logic
participant Logger as Logging
DataStorageService->>Pruner: Build newMap from existing entries
Pruner->>Pruner: Track per-data-request distribution
Pruner->>Logger: Log distribution metrics
alt newMap size differs from original
Pruner->>DataStorageService: Replace map atomically
Logger->>Logger: Log prune result
else No changes needed
Pruner-->>DataStorageService: Return same object unchanged
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The PR includes significant logic refactoring in DataStorageService and MetaData (prunePersisted rewrite, new static maps, conditional updates, enhanced logging), balanced against ~25 repetitive MetaData constructor updates following a consistent pattern. The core changes demand careful review of conditional map updates, per-request distribution tracking, and deserialization fallback logic; the breadth and heterogeneous nature of affected files adds complexity. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
network/network/src/main/java/bisq/network/p2p/services/data/storage/DataStorageService.java (5)
82-99: Avoid stream side effects, preserve sort order, and reuse maxSizeSide‑effects in filter to count types are brittle; HashMap collector also loses the sorted order. Use groupingBy for counts and LinkedHashMap to retain the created-desc order; reuse the calculated maxSize for consistency.
- Map<String, AtomicInteger> numEntriesByDataRequest = new TreeMap<>(); - Map<ByteArray, T> newMap = map.entrySet().stream() - .filter(entry -> { - T dataRequest = entry.getValue(); - String dataRequestName = dataRequest.getClass().getSimpleName(); - numEntriesByDataRequest - .computeIfAbsent(dataRequestName, k -> new AtomicInteger(0)) - .incrementAndGet(); - if (dataRequest.isExpired()) { - // log.debug("{} is expired (was created: {})", storeKey, new Date(dataRequest.getCreated())); - return false; - } - return true; - }) - .sorted(Comparator.comparingLong((Map.Entry<ByteArray, T> e) -> e.getValue().getCreated()).reversed()) - .limit(getMaxMapSize()) - .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); + Map<String, Long> distribution = + map.values().stream() + .collect(Collectors.groupingBy(dr -> dr.getClass().getSimpleName(), + TreeMap::new, Collectors.counting())); + + Map<ByteArray, T> newMap = map.entrySet().stream() + .filter(e -> !e.getValue().isExpired()) + .sorted(Comparator.comparingLong((Map.Entry<ByteArray, T> e) -> e.getValue().getCreated()).reversed()) + .limit(maxSize) + .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue, + (a, b) -> a, LinkedHashMap::new));
100-103: Fix logging: condition always true and wording is misleadingThe sum check is always > 0 for non‑empty maps; and “Add/Remove distribution” doesn’t reflect what’s logged. If you keep this log, make it debug and rename.
- if (numEntriesByDataRequest.values().stream().mapToInt(AtomicInteger::get).sum() > 0) { - log.info("Add/Remove distribution for {}: {}", storeKey, numEntriesByDataRequest); - } + if (!distribution.isEmpty()) { + log.debug("Entry distribution (pre‑prune) for {}: {}", storeKey, distribution); + }
96-98: Use the same max value for logging and limitingLimit with the already computed maxSize to avoid a second lookup and potential drift.
- int originalSize = map.size(); - log.debug("Maybe pruning persisted data for {}: size={}, maxSize={}", storeKey, originalSize, maxSize); + int originalSize = map.size(); + log.debug("Maybe pruning persisted data for {}: size={}, maxSize={}", storeKey, originalSize, maxSize); ... - .limit(getMaxMapSize()) + .limit(maxSize)Also applies to: 79-81
128-135: Micro: avoid repeated getMaxMapSize calls and clarify placeholderCache the max and use a clearer log label.
- Map<ByteArray, T> map = persistableStore.getMap(); - int size = map.size(); - boolean isExceeding = size > getMaxMapSize(); + Map<ByteArray, T> map = persistableStore.getMap(); + int size = map.size(); + int max = getMaxMapSize(); + boolean isExceeding = size > max; if (isExceeding) { - log.debug("Max. map size reached for {}. map.size()={}, getMaxMapSize={}", - storeKey, size, getMaxMapSize()); + log.debug("Max. map size reached for {}. map.size()={}, maxMapSize={}", + storeKey, size, max); }
142-149: Thread-safe lookup is fine; remove dead field and consider cachingMethod is fine. However, the class field
protected Optional<Integer> maxMapSize = Optional.empty();isn’t used anymore—please remove it. Optionally cache the resolved value per instance if this is a hot path.network/network/src/main/java/bisq/network/p2p/services/data/storage/MetaData.java (2)
156-171: Proto path: noisy warning and minor simplificationThe double Optional check logs a warn for any unknown class name arriving over the wire; that can get noisy. Consider debug/info. Also, you can simplify lookups.
- this.maxMapSize = Optional.ofNullable(MAX_MAP_SIZE_BY_CLASS_NAME.get(className)).orElseGet(() -> - Optional.ofNullable(DEFAULT_MAX_MAP_SIZE_BY_CLASS_NAME.get(className)) - .orElse(maxMapSize)); - if (Optional.ofNullable(MAX_MAP_SIZE_BY_CLASS_NAME.get(className)).isEmpty()) { - if (Optional.ofNullable(DEFAULT_MAX_MAP_SIZE_BY_CLASS_NAME.get(className)).isEmpty()) { - log.warn("DEFAULT_MAX_MAP_SIZE_BY_CLASS_NAME has no value for {}", className); - } - } + Integer resolved = MAX_MAP_SIZE_BY_CLASS_NAME.get(className); + if (resolved == null) { + resolved = DEFAULT_MAX_MAP_SIZE_BY_CLASS_NAME.getOrDefault(className, maxMapSize); + if (!DEFAULT_MAX_MAP_SIZE_BY_CLASS_NAME.containsKey(className)) { + log.info("No default maxMapSize for {}; using proto value {}", className, maxMapSize); + } + } + this.maxMapSize = resolved;
59-117: Stringly‑typed keys risk driftThe defaults map relies on class name strings. If classes are renamed, defaults silently break. Consider centralizing class name constants (or using Class<?>.getSimpleName() suppliers) in one place to reduce typo/rename risk.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
bonded-roles/src/main/java/bisq/bonded_roles/market_price/AuthorizedMarketPriceData.java(2 hunks)bonded-roles/src/main/java/bisq/bonded_roles/registration/BondedRoleRegistrationRequest.java(1 hunks)bonded-roles/src/main/java/bisq/bonded_roles/release/ReleaseNotification.java(1 hunks)bonded-roles/src/main/java/bisq/bonded_roles/security_manager/alert/AuthorizedAlertData.java(1 hunks)bonded-roles/src/main/java/bisq/bonded_roles/security_manager/difficulty_adjustment/AuthorizedDifficultyAdjustmentData.java(2 hunks)bonded-roles/src/main/java/bisq/bonded_roles/security_manager/min_reputation_score/AuthorizedMinRequiredReputationScoreData.java(2 hunks)chat/src/main/java/bisq/chat/bisq_easy/open_trades/BisqEasyOpenTradeMessage.java(1 hunks)chat/src/main/java/bisq/chat/reactions/CommonPublicChatMessageReaction.java(1 hunks)chat/src/main/java/bisq/chat/reactions/TwoPartyPrivateChatMessageReaction.java(2 hunks)chat/src/main/java/bisq/chat/two_party/TwoPartyPrivateChatMessage.java(1 hunks)common/src/main/java/bisq/common/network/Address.java(1 hunks)network/network/src/main/java/bisq/network/p2p/services/confidential/ack/AckMessage.java(1 hunks)network/network/src/main/java/bisq/network/p2p/services/data/storage/DataStorageService.java(3 hunks)network/network/src/main/java/bisq/network/p2p/services/data/storage/MetaData.java(4 hunks)network/network/src/main/java/bisq/network/p2p/services/data/storage/auth/AuthenticatedDataStorageService.java(1 hunks)network/network/src/main/java/bisq/network/p2p/services/data/storage/mailbox/MailboxDataStorageService.java(1 hunks)trade/src/main/java/bisq/trade/bisq_easy/protocol/messages/BisqEasyTradeMessage.java(1 hunks)user/src/main/java/bisq/user/banned/BannedUserProfileData.java(1 hunks)user/src/main/java/bisq/user/reputation/data/AuthorizedAccountAgeData.java(2 hunks)user/src/main/java/bisq/user/reputation/data/AuthorizedBondedReputationData.java(2 hunks)user/src/main/java/bisq/user/reputation/data/AuthorizedProofOfBurnData.java(1 hunks)user/src/main/java/bisq/user/reputation/data/AuthorizedSignedWitnessData.java(2 hunks)user/src/main/java/bisq/user/reputation/data/AuthorizedTimestampData.java(2 hunks)user/src/main/java/bisq/user/reputation/requests/AuthorizeAccountAgeRequest.java(1 hunks)user/src/main/java/bisq/user/reputation/requests/AuthorizeSignedWitnessRequest.java(1 hunks)user/src/main/java/bisq/user/reputation/requests/AuthorizeTimestampRequest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
chat/src/main/java/bisq/chat/reactions/CommonPublicChatMessageReaction.java (1)
network/network/src/main/java/bisq/network/p2p/services/data/storage/MetaData.java (1)
Slf4j(37-204)
user/src/main/java/bisq/user/reputation/requests/AuthorizeTimestampRequest.java (1)
network/network/src/main/java/bisq/network/p2p/services/data/storage/MetaData.java (1)
Slf4j(37-204)
user/src/main/java/bisq/user/reputation/requests/AuthorizeSignedWitnessRequest.java (1)
network/network/src/main/java/bisq/network/p2p/services/data/storage/MetaData.java (1)
Slf4j(37-204)
bonded-roles/src/main/java/bisq/bonded_roles/registration/BondedRoleRegistrationRequest.java (1)
network/network/src/main/java/bisq/network/p2p/services/data/storage/MetaData.java (1)
Slf4j(37-204)
user/src/main/java/bisq/user/reputation/requests/AuthorizeAccountAgeRequest.java (2)
bonded-roles/src/main/java/bisq/bonded_roles/registration/BondedRoleRegistrationRequest.java (1)
Slf4j(39-125)network/network/src/main/java/bisq/network/p2p/services/data/storage/MetaData.java (1)
Slf4j(37-204)
network/network/src/main/java/bisq/network/p2p/services/data/storage/mailbox/MailboxDataStorageService.java (1)
common/src/main/java/bisq/common/formatter/DataSizeFormatter.java (1)
DataSizeFormatter(23-49)
network/network/src/main/java/bisq/network/p2p/services/data/storage/auth/AuthenticatedDataStorageService.java (1)
common/src/main/java/bisq/common/formatter/DataSizeFormatter.java (1)
DataSizeFormatter(23-49)
🔇 Additional comments (33)
chat/src/main/java/bisq/chat/two_party/TwoPartyPrivateChatMessage.java (1)
47-47: LGTM: Max map size increased for private chat messages.The increase from
MAX_MAP_SIZE_100toMAX_MAP_SIZE_1000aligns with the PR's objective to improve database max size handling. This 10x increase seems appropriate for two-party private chat messages.chat/src/main/java/bisq/chat/bisq_easy/open_trades/BisqEasyOpenTradeMessage.java (1)
71-71: Verify the appropriateness of MAX_MAP_SIZE_10_000 for trade messages.The 100x increase from
MAX_MAP_SIZE_100toMAX_MAP_SIZE_10_000is significantly larger than changes in other files. Given that trade messages are markedHIGH_PRIORITYand are critical to platform functionality, confirm that this limit aligns with expected concurrent trade volumes.user/src/main/java/bisq/user/banned/BannedUserProfileData.java (1)
36-36: LGTM: Wildcard import simplifies MetaData constant usage.The switch to wildcard static import is consistent with changes across the codebase and improves readability when multiple MetaData constants are used.
trade/src/main/java/bisq/trade/bisq_easy/protocol/messages/BisqEasyTradeMessage.java (2)
37-37: LGTM: MAX_MAP_SIZE_1000 added for trade message metadata.The addition of
MAX_MAP_SIZE_1000aligns with the PR's systematic metadata configuration improvements.
39-43: LGTM: Constructor formatting improved.The multi-line parameter formatting enhances readability with no semantic changes.
user/src/main/java/bisq/user/reputation/data/AuthorizedBondedReputationData.java (2)
39-39: LGTM: Wildcard import consistent with codebase pattern.The switch to wildcard static import aligns with similar changes across the PR.
49-49: Verify MAX_MAP_SIZE_50_000 is appropriate for bonded reputation data.This is the highest max map size value in the entire PR. Given that reputation data is HIGH_PRIORITY and has TTL_100_DAYS, confirm this limit aligns with expected reputation data volumes and growth patterns.
network/network/src/main/java/bisq/network/p2p/services/confidential/ack/AckMessage.java (1)
22-22: LGTM: Increased capacity for acknowledgment messages.The 100x increase to
MAX_MAP_SIZE_10_000provides adequate headroom for acknowledgment message volume, while theLOW_PRIORITYandTTL_2_DAYSsettings appropriately reflect the transient nature of acks.user/src/main/java/bisq/user/reputation/data/AuthorizedSignedWitnessData.java (2)
37-37: LGTM: Wildcard import simplifies constant references.Consistent with the import style changes across the codebase.
47-47: LGTM: MAX_MAP_SIZE_50_000 consistent with reputation data.Using the same max map size as
AuthorizedBondedReputationDataensures consistent capacity for reputation-related data structures.bonded-roles/src/main/java/bisq/bonded_roles/security_manager/alert/AuthorizedAlertData.java (2)
40-40: LGTM: Wildcard import improves readability.The switch to wildcard static import is appropriate given multiple MetaData constants are now used.
52-52: LGTM: maxMapSize aligns with defaults.The MAX_MAP_SIZE_1000 value correctly matches the DEFAULT_MAX_MAP_SIZE_BY_CLASS_NAME entry for AuthorizedAlertData.
user/src/main/java/bisq/user/reputation/requests/AuthorizeSignedWitnessRequest.java (2)
32-32: LGTM: Wildcard import is consistent.The wildcard static import aligns with the codebase-wide pattern in this PR.
40-40: LGTM: MetaData parameters are correct.The constructor correctly uses LOW_PRIORITY and MAX_MAP_SIZE_100, matching the DEFAULT_MAX_MAP_SIZE_BY_CLASS_NAME entry.
chat/src/main/java/bisq/chat/reactions/TwoPartyPrivateChatMessageReaction.java (1)
29-29: LGTM: Import change is consistent.user/src/main/java/bisq/user/reputation/requests/AuthorizeTimestampRequest.java (2)
32-32: LGTM: Wildcard import is appropriate.
40-40: LGTM: MetaData configuration is correct.The parameters align with DEFAULT_MAX_MAP_SIZE_BY_CLASS_NAME for AuthorizeTimestampRequest.
bonded-roles/src/main/java/bisq/bonded_roles/market_price/AuthorizedMarketPriceData.java (2)
41-41: LGTM: Import change is consistent with PR pattern.
52-52: LGTM: MetaData parameters are correct.The MAX_MAP_SIZE_100 aligns with DEFAULT_MAX_MAP_SIZE_BY_CLASS_NAME.
chat/src/main/java/bisq/chat/reactions/CommonPublicChatMessageReaction.java (2)
29-30: LGTM: Import change is consistent.
36-38: LGTM: Symmetry maintained correctly.The MAX_MAP_SIZE_10_000 correctly maintains symmetry with CommonPublicChatMessage and matches DEFAULT_MAX_MAP_SIZE_BY_CLASS_NAME.
network/network/src/main/java/bisq/network/p2p/services/data/storage/auth/AuthenticatedDataStorageService.java (1)
400-407: LGTM: Logging enhancement is useful.The updated log format now includes the storeKey and max map size, which are more useful for debugging than the inferred className. This aligns well with the PR's focus on max size handling.
bonded-roles/src/main/java/bisq/bonded_roles/registration/BondedRoleRegistrationRequest.java (2)
37-37: LGTM: Import change is consistent.
45-45: Verify priority downgrade from HIGH_PRIORITY to LOW_PRIORITY.The priority for BondedRoleRegistrationRequest changed from HIGH_PRIORITY to LOW_PRIORITY. This affects inventory request delivery order when size limits are exceeded. Please confirm this deprioritization is intentional, as registration requests might need timely processing.
bonded-roles/src/main/java/bisq/bonded_roles/security_manager/min_reputation_score/AuthorizedMinRequiredReputationScoreData.java (1)
37-37: LGTM! MetaData initialization updated consistently.The wildcard import and the addition of
MAX_MAP_SIZE_100to the MetaData constructor align with the PR's objective to support configurable maximum map sizes per data class.Also applies to: 48-48
user/src/main/java/bisq/user/reputation/requests/AuthorizeAccountAgeRequest.java (1)
32-32: LGTM! Request metadata properly configured.The addition of
LOW_PRIORITYandMAX_MAP_SIZE_100to the MetaData constructor is appropriate for request-type messages, aligning with similar patterns in other request classes (e.g., BondedRoleRegistrationRequest).Also applies to: 40-40
user/src/main/java/bisq/user/reputation/data/AuthorizedAccountAgeData.java (1)
37-37: LGTM! Reputation data metadata properly configured.The addition of
MAX_MAP_SIZE_50_000to the MetaData constructor is appropriate for reputation data classes that may accumulate many entries. The value aligns with the default map size configuration in MetaData.Also applies to: 47-47
user/src/main/java/bisq/user/reputation/data/AuthorizedTimestampData.java (1)
37-37: LGTM! Timestamp data metadata properly configured.The addition of
LOW_PRIORITYandMAX_MAP_SIZE_50_000to the MetaData constructor is appropriate. The lower priority is reasonable for timestamp data while the large map size accommodates the potential volume of timestamp entries.Also applies to: 48-48
bonded-roles/src/main/java/bisq/bonded_roles/security_manager/difficulty_adjustment/AuthorizedDifficultyAdjustmentData.java (1)
37-37: LGTM! Security manager data metadata properly configured.The addition of
MAX_MAP_SIZE_100to the MetaData constructor is appropriate for difficulty adjustment data, which typically has a limited number of entries. The HIGH_PRIORITY is suitable for security manager operations.Also applies to: 47-47
user/src/main/java/bisq/user/reputation/data/AuthorizedProofOfBurnData.java (1)
39-39: LGTM! Proof of burn data metadata properly configured.The addition of
MAX_MAP_SIZE_50_000to the MetaData constructor is appropriate for proof of burn reputation data. The large map size accommodates many potential reputation entries while the updated data size comment (107 bytes) helps document storage considerations.Also applies to: 42-42, 50-50
network/network/src/main/java/bisq/network/p2p/services/data/storage/mailbox/MailboxDataStorageService.java (1)
246-269: LGTM! Logging enhancement improves observability.The refactored logging now includes the max map size via
getMaxMapSize()and usesstoreKeydirectly as the identifier. This simplifies the code while providing better visibility into storage limits, which directly supports the PR's objective to handle max size and pruning.bonded-roles/src/main/java/bisq/bonded_roles/release/ReleaseNotification.java (1)
49-49: LGTM! Release notification metadata properly configured.The addition of
MAX_MAP_SIZE_100to the MetaData constructor is appropriate for release notifications, which typically have a small, controlled number of entries. The HIGH_PRIORITY ensures timely distribution of release information.network/network/src/main/java/bisq/network/p2p/services/data/storage/DataStorageService.java (1)
47-47: Visibility change: confirm exposure is intentionalChanging
storeKeyto protected widens the API surface. Confirm subclasses need direct access; otherwise prefer a protected accessor to keep the field final/private.
| // Metadata needs to be symmetric with TwoPartyPrivateChatMessage. | ||
| // MetaData is transient as it will be used indirectly by low level network classes. Only some low level network classes write the metaData to their protobuf representations. | ||
| private transient final MetaData metaData = new MetaData(TTL_30_DAYS, getClass().getSimpleName(), MAX_MAP_SIZE_100); | ||
| private transient final MetaData metaData = new MetaData(TTL_30_DAYS, LOW_PRIORITY, getClass().getSimpleName(), MAX_MAP_SIZE_10_000); |
There was a problem hiding this comment.
Inconsistent maxMapSize violates symmetry requirement.
The comment on Line 36 states "Metadata needs to be symmetric with TwoPartyPrivateChatMessage," but MAX_MAP_SIZE_10_000 is used here while the DEFAULT_MAX_MAP_SIZE_BY_CLASS_NAME in MetaData.java specifies MAX_MAP_SIZE_1000 for both TwoPartyPrivateChatMessage and TwoPartyPrivateChatMessageReaction. This 10x discrepancy breaks the intended symmetry.
Apply this diff to fix the inconsistency:
- private transient final MetaData metaData = new MetaData(TTL_30_DAYS, LOW_PRIORITY, getClass().getSimpleName(), MAX_MAP_SIZE_10_000);
+ private transient final MetaData metaData = new MetaData(TTL_30_DAYS, LOW_PRIORITY, getClass().getSimpleName(), MAX_MAP_SIZE_1000);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Metadata needs to be symmetric with TwoPartyPrivateChatMessage. | |
| // MetaData is transient as it will be used indirectly by low level network classes. Only some low level network classes write the metaData to their protobuf representations. | |
| private transient final MetaData metaData = new MetaData(TTL_30_DAYS, getClass().getSimpleName(), MAX_MAP_SIZE_100); | |
| private transient final MetaData metaData = new MetaData(TTL_30_DAYS, LOW_PRIORITY, getClass().getSimpleName(), MAX_MAP_SIZE_10_000); | |
| // Metadata needs to be symmetric with TwoPartyPrivateChatMessage. | |
| // MetaData is transient as it will be used indirectly by low level network classes. Only some low level network classes write the metaData to their protobuf representations. | |
| private transient final MetaData metaData = new MetaData(TTL_30_DAYS, LOW_PRIORITY, getClass().getSimpleName(), MAX_MAP_SIZE_1000); |
🤖 Prompt for AI Agents
In
chat/src/main/java/bisq/chat/reactions/TwoPartyPrivateChatMessageReaction.java
around lines 36-38, the MetaData is created with MAX_MAP_SIZE_10_000 which
breaks symmetry with TwoPartyPrivateChatMessage and
MetaData.DEFAULT_MAX_MAP_SIZE_BY_CLASS_NAME; replace MAX_MAP_SIZE_10_000 with
MAX_MAP_SIZE_1000 (or otherwise use the DEFAULT_MAX_MAP_SIZE_BY_CLASS_NAME
lookup for class defaults) so the maxMapSize matches the value used for
TwoPartyPrivateChatMessage.
| } else { | ||
| // I2P | ||
| NetworkDataValidation.validateText(host, 512); | ||
| NetworkDataValidation.validateText(host, 700); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the rationale for I2P host length limit of 700.
The increase from 512 to 700 for I2P address validation should be confirmed against I2P address specifications. Please verify:
- What is the actual maximum length for I2P addresses?
- Is 700 based on spec or empirical observations?
🌐 Web query:
What is the maximum length of an I2P address?
💡 Result:
Short answer:
- Standard I2P hostnames: maximum 67 characters total (including the ".i2p"). [1]
- Standard .b32.i2p addresses: 52 encoded characters + ".b32.i2p" = 60 characters total. [1][2]
- Extended/encrypted leaseset .b32.i2p addresses are 56 or more encoded characters (56+ + ".b32.i2p"). [1][3]
Sources: I2P naming docs and b32 encrypted spec. [1][3]
Correct I2P host validation limit to align with specification.
Standard I2P hostnames have a maximum of 67 characters total (including ".i2p"), and standard .b32.i2p addresses are 60 characters total, with extended versions reaching 56+ characters. The validation limit of 700 characters far exceeds I2P specifications and should be reduced to match the actual maximum I2P address length (approximately 67 characters for hostnames or 60 characters for b32 addresses, depending on the expected format).
🤖 Prompt for AI Agents
In common/src/main/java/bisq/common/network/Address.java around line 67, the
call NetworkDataValidation.validateText(host, 700) uses an incorrect
700-character limit for I2P hosts; change the limit to the correct maximum (use
67 to cover standard I2P hostnames including “.i2p” or adjust to 60 if you
intend to validate only .b32.i2p addresses), i.e., replace 700 with the chosen
max length and add a brief comment referencing the I2P spec for future
maintainers.
| public static final Map<String, Integer> MAX_MAP_SIZE_BY_CLASS_NAME = new HashMap<>(); | ||
|
|
There was a problem hiding this comment.
Make runtime map concurrent
MAX_MAP_SIZE_BY_CLASS_NAME is mutated from multiple threads (via constructor calls) and read elsewhere; HashMap is not thread‑safe. Use ConcurrentHashMap.
@@
-import java.util.AbstractMap;
-import java.util.HashMap;
+import java.util.AbstractMap;
import java.util.Map;
import java.util.Optional;
+import java.util.concurrent.ConcurrentHashMap;
@@
- public static final Map<String, Integer> MAX_MAP_SIZE_BY_CLASS_NAME = new HashMap<>();
+ public static final Map<String, Integer> MAX_MAP_SIZE_BY_CLASS_NAME = new ConcurrentHashMap<>();Also applies to: 28-31
🤖 Prompt for AI Agents
In
network/network/src/main/java/bisq/network/p2p/services/data/storage/MetaData.java
around lines 28-31 and 119-120, the static maps (including
MAX_MAP_SIZE_BY_CLASS_NAME) are declared as HashMap which is mutated from
multiple threads; replace HashMap with ConcurrentHashMap for thread-safe
concurrent access. Update the declarations to use
java.util.concurrent.ConcurrentHashMap and ensure imports reflect that change so
all reads/writes are safe without external synchronization.
HenrikJannsen
changed the title
Improve prune method. We want to avoid to take the maxMapSize from data we received from the network, as that data can have an older version of maxMapSize and is basically untrusted data. If we create the MetaData by constructor we have the metadata set from our code, thus we do not need a lookup but put the value into the lookup map.
HenrikJannsen
force-pushed
the
improve-db-max-size--and-prune-handling
branch
from
1ac7abc to
01de9b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
chat/src/main/java/bisq/chat/reactions/TwoPartyPrivateChatMessageReaction.java (1)
36-38: Inconsistent maxMapSize violates symmetry requirement.The comment states metadata needs to be symmetric with
TwoPartyPrivateChatMessage, butMAX_MAP_SIZE_10_000is used here whileMetaData.DEFAULT_MAX_MAP_SIZE_BY_CLASS_NAMEspecifiesMAX_MAP_SIZE_1000for both classes. This 10x discrepancy breaks the intended symmetry.
🧹 Nitpick comments (1)
user/src/main/java/bisq/user/banned/BannedUserProfileData.java (1)
36-36: Verify wildcard static import is intentional.Wildcard static imports (
import static MetaData.*;) are generally discouraged by Java style guides because they reduce code clarity and can introduce naming conflicts. This file currently uses onlyTTL_100_DAYSandHIGH_PRIORITY, so explicit imports would be clearer.However, if this change is part of a deliberate project-wide standardization effort (as the summary suggests), consistency across the codebase is a valid consideration. Please confirm this aligns with the project's coding standards.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
bonded-roles/src/main/java/bisq/bonded_roles/market_price/AuthorizedMarketPriceData.java(2 hunks)bonded-roles/src/main/java/bisq/bonded_roles/registration/BondedRoleRegistrationRequest.java(1 hunks)bonded-roles/src/main/java/bisq/bonded_roles/release/ReleaseNotification.java(1 hunks)bonded-roles/src/main/java/bisq/bonded_roles/security_manager/alert/AuthorizedAlertData.java(1 hunks)bonded-roles/src/main/java/bisq/bonded_roles/security_manager/difficulty_adjustment/AuthorizedDifficultyAdjustmentData.java(2 hunks)bonded-roles/src/main/java/bisq/bonded_roles/security_manager/min_reputation_score/AuthorizedMinRequiredReputationScoreData.java(2 hunks)chat/src/main/java/bisq/chat/bisq_easy/open_trades/BisqEasyOpenTradeMessage.java(1 hunks)chat/src/main/java/bisq/chat/reactions/CommonPublicChatMessageReaction.java(1 hunks)chat/src/main/java/bisq/chat/reactions/TwoPartyPrivateChatMessageReaction.java(2 hunks)chat/src/main/java/bisq/chat/two_party/TwoPartyPrivateChatMessage.java(1 hunks)network/network/src/main/java/bisq/network/p2p/services/confidential/ack/AckMessage.java(1 hunks)network/network/src/main/java/bisq/network/p2p/services/data/storage/DataStorageService.java(3 hunks)network/network/src/main/java/bisq/network/p2p/services/data/storage/MetaData.java(4 hunks)network/network/src/main/java/bisq/network/p2p/services/data/storage/auth/AuthenticatedDataStorageService.java(1 hunks)network/network/src/main/java/bisq/network/p2p/services/data/storage/mailbox/MailboxDataStorageService.java(1 hunks)trade/src/main/java/bisq/trade/bisq_easy/protocol/messages/BisqEasyTradeMessage.java(1 hunks)user/src/main/java/bisq/user/banned/BannedUserProfileData.java(1 hunks)user/src/main/java/bisq/user/reputation/data/AuthorizedAccountAgeData.java(2 hunks)user/src/main/java/bisq/user/reputation/data/AuthorizedBondedReputationData.java(2 hunks)user/src/main/java/bisq/user/reputation/data/AuthorizedProofOfBurnData.java(1 hunks)user/src/main/java/bisq/user/reputation/data/AuthorizedSignedWitnessData.java(2 hunks)user/src/main/java/bisq/user/reputation/data/AuthorizedTimestampData.java(2 hunks)user/src/main/java/bisq/user/reputation/requests/AuthorizeAccountAgeRequest.java(1 hunks)user/src/main/java/bisq/user/reputation/requests/AuthorizeSignedWitnessRequest.java(1 hunks)user/src/main/java/bisq/user/reputation/requests/AuthorizeTimestampRequest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- bonded-roles/src/main/java/bisq/bonded_roles/security_manager/alert/AuthorizedAlertData.java
- trade/src/main/java/bisq/trade/bisq_easy/protocol/messages/BisqEasyTradeMessage.java
- network/network/src/main/java/bisq/network/p2p/services/data/storage/auth/AuthenticatedDataStorageService.java
- user/src/main/java/bisq/user/reputation/requests/AuthorizeSignedWitnessRequest.java
- user/src/main/java/bisq/user/reputation/data/AuthorizedBondedReputationData.java
- user/src/main/java/bisq/user/reputation/data/AuthorizedSignedWitnessData.java
- network/network/src/main/java/bisq/network/p2p/services/data/storage/MetaData.java
- user/src/main/java/bisq/user/reputation/requests/AuthorizeTimestampRequest.java
- bonded-roles/src/main/java/bisq/bonded_roles/security_manager/min_reputation_score/AuthorizedMinRequiredReputationScoreData.java
- chat/src/main/java/bisq/chat/bisq_easy/open_trades/BisqEasyOpenTradeMessage.java
- user/src/main/java/bisq/user/reputation/data/AuthorizedProofOfBurnData.java
🧰 Additional context used
🧬 Code graph analysis (3)
network/network/src/main/java/bisq/network/p2p/services/data/storage/mailbox/MailboxDataStorageService.java (1)
common/src/main/java/bisq/common/formatter/DataSizeFormatter.java (1)
DataSizeFormatter(23-49)
bonded-roles/src/main/java/bisq/bonded_roles/registration/BondedRoleRegistrationRequest.java (1)
network/network/src/main/java/bisq/network/p2p/services/data/storage/MetaData.java (1)
Slf4j(37-204)
chat/src/main/java/bisq/chat/reactions/CommonPublicChatMessageReaction.java (1)
network/network/src/main/java/bisq/network/p2p/services/data/storage/MetaData.java (1)
Slf4j(37-204)
🔇 Additional comments (15)
network/network/src/main/java/bisq/network/p2p/services/confidential/ack/AckMessage.java (1)
11-22: LGTM!The MetaData initialization correctly adds
MAX_MAP_SIZE_10_000, which aligns with the default mapping defined inMetaData.DEFAULT_MAX_MAP_SIZE_BY_CLASS_NAMEforAckMessage.chat/src/main/java/bisq/chat/reactions/CommonPublicChatMessageReaction.java (1)
29-38: LGTM!The MetaData initialization correctly uses
MAX_MAP_SIZE_10_000, maintaining symmetry withCommonPublicChatMessageas both classes map to the same value inMetaData.DEFAULT_MAX_MAP_SIZE_BY_CLASS_NAME.user/src/main/java/bisq/user/reputation/data/AuthorizedAccountAgeData.java (1)
37-47: LGTM!The MetaData initialization correctly includes
MAX_MAP_SIZE_50_000, consistent with the default mapping inMetaData.DEFAULT_MAX_MAP_SIZE_BY_CLASS_NAMEforAuthorizedAccountAgeData.user/src/main/java/bisq/user/reputation/data/AuthorizedTimestampData.java (1)
37-48: LGTM!The MetaData initialization correctly adds
LOW_PRIORITYandMAX_MAP_SIZE_50_000, with the latter matching the default mapping inMetaData.DEFAULT_MAX_MAP_SIZE_BY_CLASS_NAMEforAuthorizedTimestampData.user/src/main/java/bisq/user/reputation/requests/AuthorizeAccountAgeRequest.java (1)
32-40: LGTM!The MetaData initialization correctly adds
LOW_PRIORITYwhile maintainingMAX_MAP_SIZE_100, which matches the default mapping inMetaData.DEFAULT_MAX_MAP_SIZE_BY_CLASS_NAMEforAuthorizeAccountAgeRequest.bonded-roles/src/main/java/bisq/bonded_roles/security_manager/difficulty_adjustment/AuthorizedDifficultyAdjustmentData.java (1)
37-47: LGTM!The MetaData initialization correctly adds
MAX_MAP_SIZE_100, which aligns with the default mapping inMetaData.DEFAULT_MAX_MAP_SIZE_BY_CLASS_NAMEforAuthorizedDifficultyAdjustmentData.bonded-roles/src/main/java/bisq/bonded_roles/release/ReleaseNotification.java (1)
38-49: LGTM!The MetaData initialization correctly adds
MAX_MAP_SIZE_100, consistent with the default mapping inMetaData.DEFAULT_MAX_MAP_SIZE_BY_CLASS_NAMEforReleaseNotification.bonded-roles/src/main/java/bisq/bonded_roles/market_price/AuthorizedMarketPriceData.java (1)
41-41: LGTM! MetaData initialization updated correctly.The wildcard static import and the addition of
MAX_MAP_SIZE_100parameter to the MetaData constructor align with the PR's objective to introduce configurable max map sizes.Also applies to: 52-52
bonded-roles/src/main/java/bisq/bonded_roles/registration/BondedRoleRegistrationRequest.java (1)
37-37: LGTM! Priority and max map size parameters added appropriately.The addition of
LOW_PRIORITYfor bonded role registration requests is sensible, as these are less time-sensitive than market data or active trade messages. TheMAX_MAP_SIZE_100limit is appropriate for the expected low volume of registration requests.Also applies to: 45-45
network/network/src/main/java/bisq/network/p2p/services/data/storage/mailbox/MailboxDataStorageService.java (1)
261-268: LGTM! Logging simplified and enhanced.The logging now directly uses
storeKeyinstead of deriving className from stream operations, which is more efficient. The addition ofgetMaxMapSize()provides useful context for monitoring storage limits.network/network/src/main/java/bisq/network/p2p/services/data/storage/DataStorageService.java (5)
47-47: LGTM! Visibility change enables subclass logging.Changing
storeKeytoprotectedallows subclasses likeMailboxDataStorageServiceto access it for logging purposes, which is appropriate and maintains encapsulation.
142-149: LGTM! Robust max map size resolution with proper fallbacks.The
getMaxMapSize()method implements a sensible three-tier fallback strategy: runtime map → static defaults → hardcoded constant. The warning when defaults are used helps identify configuration gaps.
128-140: LGTM! Enhanced logging for size monitoring.The improved logging provides better visibility into storage limits with the
storeKeyidentifier and actual vs. max size values. The additional warning at 20,000 entries helps identify unusually large maps.
32-36: LGTM! Import additions support the new functionality.The new imports (
AtomicIntegerfor distribution tracking andMAX_MAP_SIZE_10_000for fallback default) are necessary and properly used.
72-126: Now let me search the codebase to verify the actual implementation context:Let me search the codebase directly to understand the implementation:
Based on my verification, aggregate operations such as putAll and clear on ConcurrentHashMap are not atomic—concurrent retrievals may reflect insertion or removal of only some entries. This means the original review comment correctly identifies a real thread safety concern.
However, whether this concern represents an actual bug depends on the calling context. The script output shows that
prunePersistedis called during data loading inPersistenceClient.readAsync(), which typically happens during initialization. If the map is not accessed concurrently beforeprunePersistedcompletes and the data is applied viaapplyPersisted(), then the risk may be limited.To finalize the review, I need the calling context verified via codebase:
Question for you: Could you run the following to check when and under what threading conditions
prunePersistedis called?#!/bin/bash # Check DataStorageService class hierarchy and initialization rg -nP -A10 'class DataStorageService' --type=java rg -nP -B5 -A10 'readAsync' --type=java | head -100This will help determine if concurrent access to the map is actually possible during or after pruning.
| // Metadata needs to be symmetric with TwoPartyPrivateChatMessageReaction. | ||
| // MetaData is transient as it will be used indirectly by low level network classes. Only some low level network classes write the metaData to their protobuf representations. | ||
| private transient final MetaData metaData = new MetaData(TTL_30_DAYS, getClass().getSimpleName(), MAX_MAP_SIZE_100); | ||
| private transient final MetaData metaData = new MetaData(TTL_30_DAYS, getClass().getSimpleName(), MAX_MAP_SIZE_1000); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify symmetry with TwoPartyPrivateChatMessageReaction.
The comment on line 45 explicitly states that metadata must be symmetric with TwoPartyPrivateChatMessageReaction. Ensure that the related class also uses MAX_MAP_SIZE_1000 to maintain this symmetry requirement.
Run the following script to verify the MetaData configuration in the related class:
🏁 Script executed:
#!/bin/bash
# Description: Verify TwoPartyPrivateChatMessageReaction uses MAX_MAP_SIZE_1000 for symmetry
# Search for MetaData initialization in TwoPartyPrivateChatMessageReaction
rg -n -A2 -B2 'class TwoPartyPrivateChatMessageReaction' --type java
rg -n 'new MetaData.*MAX_MAP_SIZE' chat/src/main/java/bisq/chat/reactions/TwoPartyPrivateChatMessageReaction.javaLength of output: 956
Fix asymmetric MetaData configuration between related classes.
Verification found that TwoPartyPrivateChatMessage.java (line 47) uses MAX_MAP_SIZE_1000, while TwoPartyPrivateChatMessageReaction.java (line 38) uses MAX_MAP_SIZE_10_000. Both classes have explicit comments requiring symmetry. Update one of these values to match the other and maintain the required symmetry.
🤖 Prompt for AI Agents
In chat/src/main/java/bisq/chat/two_party/TwoPartyPrivateChatMessage.java around
line 47, the MetaData is configured with MAX_MAP_SIZE_1000 which is asymmetric
with TwoPartyPrivateChatMessageReaction (which uses MAX_MAP_SIZE_10_000); update
the MetaData instantiation to use MAX_MAP_SIZE_10_000 so both related classes
use the same map size constant, keeping the rest of the constructor parameters
unchanged.
1 participant
Summary by CodeRabbit
Refactor
Chores