Store creator hash prefixes in a separate lookup file #27731

zenparsing · 2025-02-19T13:03:20Z

Resolves brave/brave-browser#43994

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

atuchin-m · 2025-02-28T14:33:26Z

components/brave_rewards/core/engine/hash_prefix_iterator.h

+
+// A random-access iterator over prefixes stored in an uncompressed prefix list,
+// suitable for binary search.
+class HashPrefixIterator {


Hm, do we really need this thing?
If it's only to support std::binary_search, we can just compare [0.. count-1] indexes + a custom compare function instead.

This is a rename (and cleanup) of an existing class currently used by PrefixListReader.

O, thanks, I missed that part.
I see it uses to make sanity check that the first few prefixes are in order. That code can be moved to UpdatePrefixes.
If we can rid off it completely, it will be great.

atuchin-m · 2025-02-28T14:37:45Z

components/brave_rewards/core/engine/hash_prefix_store.cc

+  if (!reader.ReadU32LittleEndian(parts.prefix_size)) {
+    return std::nullopt;
+  }
+  parts.prefixes = reader.remaining_span();


I believe we need to verify size here. Otherwise we can get an overflow in case the file is broken (i.e. with a sudden shutdown)

See IsValidFile for span-length validation.

atuchin-m · 2025-02-28T14:45:28Z

ios/browser/api/brave_rewards/brave_rewards_api.mm

+  static scoped_refptr<base::SequencedTaskRunner> CreateTaskRunner() {
+    return base::ThreadPool::CreateSequencedTaskRunner(
+        {base::MayBlock(), base::TaskPriority::USER_VISIBLE,
+         base::TaskShutdownBehavior::BLOCK_SHUTDOWN});


maybe SKIP_SHUTDOWN?
Also does it make sense to reuse one of the current task runner to simplify things?

Updated to use SKIP_ON_SHUTDOWN. I will look into the possibility of reusing a task runner.

atuchin-m · 2025-02-28T15:07:13Z

components/brave_rewards/core/engine/hash_prefix_store.h

+  HashPrefixStore& operator=(const HashPrefixStore&) = delete;
+
+  // Opens the hash prefix file, if not already open.
+  bool Open();


Let's move internal methods to the private section.

This method is intentionally public. It's used by the unit tests for instance, but in principle could be used by a client to manually control opening/closing.

atuchin-m · 2025-02-28T15:16:28Z

components/brave_rewards/core/mojom/rewards_database.mojom

+  uint32 prefix_size;
+};
+
+interface HashPrefixStore {


Do we use actually use this interface for IPC? If not why just not use sequenceBound?

I considered passing this interface to the Rewards engine, but decided to leave that for a future improvement. In the meantime, it is used to run the interface on a different thread (via RemoteWorker). However, if you aren't comfortable with that approach, I can use SequenceBound instead.

github-actions · 2025-02-28T15:19:46Z

[puLL-Merge] - brave/brave-core@27731

Description

This PR implements a new storage system for creator hash prefixes in Brave Rewards, moving them from a SQLite database table to a dedicated memory-mapped file. This change aims to improve performance and reduce memory usage by keeping the hash prefixes in a more efficient format that can be searched directly without database queries.

Changes

Changes

HashPrefixStore Implementation:

Added new HashPrefixStore class for managing creator hash prefixes in a memory-mapped file
Implements file format versioning and validation
Provides methods for updating and searching prefixes

Database Migration:

Added migration v41 to remove the old publisher_prefix_list table
Updated state migration to v15 to reset server publisher list timestamp
Updated database schema version

Hash Prefix Iterator:

Replaced PrefixIterator with new HashPrefixIterator implementation
Maintains compatibility with STL algorithms while working with memory-mapped data

Integration Changes:

Modified RewardsServiceImpl to use new HashPrefixStore
Updated Publisher class to use new prefix store methods
Added new Mojo interfaces for hash prefix operations

iOS Support:

Added iOS-specific implementation of the new storage system
Updated BraveRewardsAPI to handle the new creator prefix store

sequenceDiagram
    participant Client
    participant RewardsServiceImpl
    participant HashPrefixStore
    participant MemoryMappedFile

    Client->>RewardsServiceImpl: UpdateCreatorPrefixStore(data)
    RewardsServiceImpl->>HashPrefixStore: UpdatePrefixes(prefixes, size)
    HashPrefixStore->>HashPrefixStore: Close()
    HashPrefixStore->>HashPrefixStore: WriteFileHeader()
    HashPrefixStore->>HashPrefixStore: WritePrefixes()
    HashPrefixStore-->>RewardsServiceImpl: success
    RewardsServiceImpl-->>Client: success

    Client->>RewardsServiceImpl: ContainsPrefix(value)
    RewardsServiceImpl->>HashPrefixStore: ContainsPrefix(value)
    HashPrefixStore->>MemoryMappedFile: Open()
    HashPrefixStore->>HashPrefixStore: BinarySearch()
    HashPrefixStore-->>RewardsServiceImpl: result
    RewardsServiceImpl-->>Client: result

Security Hotspots

File Format Validation: The code validates file headers and content in ParseFile(), but carefully review the validation logic for potential edge cases or overflow conditions.
Memory Mapping: The implementation uses memory-mapped files which could expose memory if not properly handled. Ensure proper error handling around file mapping operations.

emerick

++rewards

emerick · 2025-02-28T16:44:31Z

components/brave_rewards/content/rewards_service_impl.cc

@@ -236,12 +237,14 @@ void DeferCallback(base::Location location, Callback callback, Args&&... args) {
 // read comment about file pathes at src\base\files\file_path.h
 #if BUILDFLAG(IS_WIN)
 const base::FilePath::StringType kDiagnosticLogPath(L"Rewards.log");
+const base::FilePath::StringType kCreatorPrefixStore(L"RewardsCreators.db");


Can we keep these in alphabetical order?

github-actions bot assigned zenparsing Feb 19, 2025

zenparsing force-pushed the ksmith-prefix-list-v2 branch 6 times, most recently from 11de286 to a327bb5 Compare February 25, 2025 20:42

zenparsing force-pushed the ksmith-prefix-list-v2 branch 3 times, most recently from 9f6e4ff to f730894 Compare February 28, 2025 12:47

zenparsing marked this pull request as ready for review February 28, 2025 14:22

zenparsing requested review from a team as code owners February 28, 2025 14:22

zenparsing requested a review from atuchin-m February 28, 2025 14:23

atuchin-m reviewed Feb 28, 2025

View reviewed changes

zenparsing force-pushed the ksmith-prefix-list-v2 branch from f730894 to a9d51f7 Compare February 28, 2025 15:18

github-actions bot added the puLL-Merge label Feb 28, 2025

emerick approved these changes Feb 28, 2025

View reviewed changes

[Rewards] Store creator hash prefixes in a separate lookup file

0cb3a99

zenparsing force-pushed the ksmith-prefix-list-v2 branch from a9d51f7 to 0cb3a99 Compare February 28, 2025 17:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store creator hash prefixes in a separate lookup file #27731

Store creator hash prefixes in a separate lookup file #27731

zenparsing commented Feb 19, 2025

atuchin-m Feb 28, 2025

zenparsing Feb 28, 2025

atuchin-m Feb 28, 2025

atuchin-m Feb 28, 2025

zenparsing Feb 28, 2025

atuchin-m Feb 28, 2025

zenparsing Feb 28, 2025

atuchin-m Feb 28, 2025

zenparsing Feb 28, 2025

atuchin-m Feb 28, 2025

zenparsing Feb 28, 2025

github-actions bot commented Feb 28, 2025

Changes

emerick left a comment

emerick Feb 28, 2025

zenparsing Feb 28, 2025

Store creator hash prefixes in a separate lookup file #27731

Are you sure you want to change the base?

Store creator hash prefixes in a separate lookup file #27731

Conversation

zenparsing commented Feb 19, 2025

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 28, 2025

Description

Changes

Security Hotspots

emerick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment