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

fix: fix mining chunk cache #636

Merged
merged 4 commits into from
Oct 28, 2024
Merged

fix: fix mining chunk cache #636

merged 4 commits into from
Oct 28, 2024

Conversation

JamesPiechota
Copy link
Collaborator

@JamesPiechota JamesPiechota commented Oct 25, 2024

Copy link
Collaborator

@shizzard shizzard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No major problems with the code (didn't test it though)


case NewCacheLimit == State#state.chunk_cache_limit of
case PartitionCacheLimit == State#state.chunk_cache_limit of
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These parts of the code are easily extracted into sharp functions like

maybe_update_cache_limit(PartitionCacheLimit, #state{chunk_cache_limit = PartitionCacheLimit} = S0) -> S0;
maybe_update_cache_limit(PartitionCacheLimit, S0) ->
    ...

This will greatly reduce the nesting within a function and reduce function scope.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! 1a8a2ce

false -> ok
end,
GarbageCollectionFrequency = 4 * VDFQueueLimit * 1000,

GCRef =
case State#state.gc_frequency_ms == undefined of
Copy link
Collaborator

@shizzard shizzard Oct 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic is not clear here.
We're checking if the gc_frequency_ms is defined in the process state, but not checking if the gc_process_ref is set (the actual marker that the timer is set here). So there are a possible scenario where we will have two timers set at the same time.
I would recommend saving the actual timer reference in the state and extract the timer initialization into a separate function. The example may be found in the P3 ledger implementation (handle_continue(?msg_continue_start_shutdown_timer, ...) @ ar_p3_ledger.erl line 431):

  • ensure you cancel the existing timer if the timer exists and still running;
  • ensure to put the unique marker into timer message (this is already done here);

When hitting the timer message handler, just do the related work and ensure to call the extracted timer initialization function once again, that will do all the needed things.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it - Done! cb46fb0

@JamesPiechota JamesPiechota marked this pull request as ready for review October 28, 2024 01:05
@JamesPiechota JamesPiechota changed the title WIP fix: fix mining chunk cache Oct 28, 2024
mining cache now tracks sub-chunks stored and assumes difficulty
0 sub-chunks are the size of a full chunk. Also allow user
to specify mining cache size in MiB rather than chunks
@JamesPiechota JamesPiechota merged commit 1a2e4f7 into master Oct 28, 2024
67 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants