-
Notifications
You must be signed in to change notification settings - Fork 247
fix(mm_proxy): update reserved_offset after inserting merge mining tag #7660
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
base: development
Are you sure you want to change the base?
fix(mm_proxy): update reserved_offset after inserting merge mining tag #7660
Conversation
|
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
Summary of ChangesHello @HashVault, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an inaccuracy in the merge mining proxy's block template response. Previously, when the proxy inserted a 35-byte merge mining tag into the Monero coinbase transaction's extra field, it failed to update the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly addresses an issue where the reserved_offset was not updated after the merge mining tag was inserted into the Monero coinbase transaction. The fix introduces a constant for the tag size and adds it to the original offset, which is the right approach. My review includes one suggestion for future improvement regarding the location of the new constant to enhance long-term maintainability by colocating it with the related logic in the tari_core crate.
| /// Size in bytes of the merge mining tag that is inserted into the Monero coinbase transaction's | ||
| /// extra field. When this tag is inserted at the beginning of the extra field, all subsequent | ||
| /// data (including the pool nonce reserved area) is shifted by this amount, so the `reserved_offset` | ||
| /// must be adjusted accordingly. | ||
| const MERGE_MINING_TAG_SIZE: u64 = 35; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While defining MERGE_MINING_TAG_SIZE here with a clear comment is a good improvement, this constant is tightly coupled to the implementation of the merge mining tag creation logic, which resides in the tari_core::proof_of_work::monero_rx module.
To improve maintainability and establish a single source of truth, it would be ideal to define this constant within the monero_rx module and export it for use here. This would prevent potential inconsistencies if the tag structure were to change in the future.
Since modifying tari_core might be out of scope for this PR, this is acceptable for now, but please consider creating a follow-up task to address this.
The merge mining proxy inserts a merge mining tag at the beginning of the Monero coinbase transaction's extra field, which shifts all subsequent data by 35 bytes. However, the `reserved_offset` field in the response was not being updated to reflect this shift. This caused mining pools to have to calculate the correct nonce position themselves instead of using the provided `reserved_offset` value. This fix adjusts `reserved_offset` by adding the merge mining tag size (35 bytes) after modifying the block template.
3d10fc1 to
5dea6b0
Compare
Description
The merge mining proxy inserts a merge mining tag at the beginning of the Monero coinbase transaction's extra field, which shifts all subsequent data by 35 bytes. However, the
reserved_offsetfield in the response was not being updated to reflect this shift.This caused mining pools to have to calculate the correct nonce position themselves instead of using the provided
reserved_offsetvalue.This fix adjusts
reserved_offsetby adding the merge mining tag size (35 bytes) after modifying the block template.Motivation and Context
Mining pools that use the
reserved_offsetvalue from the proxy response were receiving an incorrect offset (e.g., 131) when the actual position in the modified blob was different (e.g., 166). This forced pools to calculate the correct position themselves by searching for the nonce pattern in the blob.How Has This Been Tested?
Empirical testing the offset mismatch: daemon reported 131, actual position was 166, difference = 35 bytes.
What process can a PR reviewer use to test or verify this change?
Breaking Changes