feat: add merge mining proxy support for p2pool #6474
Conversation
ghpbot-tari-project
added
the
CR-insufficient_context
Test Results (Integration tests)36 tests 36 ✅ 16m 3s ⏱️ Results for commit 650f6dc. ♻️ This comment has been updated with latest results. |
ghpbot-tari-project
added
P-acks_required
Test Results (CI) 2 files 86 suites 24m 11s ⏱️ For more details on these failures, see this check. Results for commit 650f6dc. ♻️ This comment has been updated with latest results. |
applications/minotari_merge_mining_proxy/src/block_template_protocol.rs
Outdated
Show resolved
Hide resolved
applications/minotari_merge_mining_proxy/src/block_template_protocol.rs
Outdated
Show resolved
Hide resolved
applications/minotari_merge_mining_proxy/src/block_template_protocol.rs
Outdated
Show resolved
Hide resolved
…ri into st-randomx-merge-mining
applications/minotari_merge_mining_proxy/src/block_template_protocol.rs
Outdated
Show resolved
Hide resolved
| }, | ||
| ); | ||
| b | ||
| }, |
There was a problem hiding this comment.
We are missing handing this error
| }, | |
| }, | |
| Err(MmProxyError::FailedPreconditionBlockLostRetry) => { | |
| debug!( | |
| target: LOG_TARGET, | |
| "Chain tip has progressed past template height {}. Fetching a new block template (try {}).", | |
| height, loop_count | |
| ); | |
| continue; | |
| }, |
| None => { | ||
| let new_template = match self.get_new_block_template().await { | ||
| Ok(val) => val, | ||
| let (_new_template, block_template_with_coinbase, height) = self |
There was a problem hiding this comment.
| let (_new_template, block_template_with_coinbase, height) = self | |
| let (new_template, block_template_with_coinbase, height) = self |
There was a problem hiding this comment.
this field is not used, but I have removed it now
| .cloned() | ||
| .ok_or_else(|| MmProxyError::GrpcResponseMissingField("miner_data"))?; | ||
|
|
||
| (add_monero_data(block, monero_mining_data.clone(), miner_data)?, height) |
There was a problem hiding this comment.
Should new_template as calculated earlier not be used for non-pools? I am not sure if miner data pulled form the block will be the same. In fn add_monero_data( parameter template_data: NewBlockTemplateData is fed in and not pulled from tari_block_result: grpc::GetNewBlockResult. With the suggestion below we know the code will behave the same.
| (add_monero_data(block, monero_mining_data.clone(), miner_data)?, height) | |
| match self.p2pool_client.as_mut() { | |
| Some(p2pool_client) => { | |
| (add_monero_data(block, monero_mining_data.clone(), miner_data)?, height) | |
| }, | |
| None => { | |
| (add_monero_data(block, monero_mining_data.clone(), new_template)?, height) | |
| }, | |
| } |
There was a problem hiding this comment.
I'm not sure I understand what you mean here. I think the mmproxy caching can be simplified even for non pools, but I unfortunately don't have the time to do this now
| // TODO: I don't know why this is so complicated, but I've extracted it into it's own method to hide the ugliness | ||
| async fn get_block_template_in_unnecessarily_complicated_manner( |
There was a problem hiding this comment.
| // TODO: I don't know why this is so complicated, but I've extracted it into it's own method to hide the ugliness | |
| async fn get_block_template_in_unnecessarily_complicated_manner( | |
| async fn get_block_template_from_cache_or_new( |
| let new_template = match self.get_new_block_template().await { | ||
| Ok(val) => val, | ||
| let (_new_template, block_template_with_coinbase, height) = self | ||
| .get_block_template_in_unnecessarily_complicated_manner( |
There was a problem hiding this comment.
| .get_block_template_in_unnecessarily_complicated_manner( | |
| .get_block_template_from_cache_or_new( |
| let block_result = client.get_new_block(GetNewBlockRequest::default()).await?.into_inner(); | ||
| block_result | ||
| .block | ||
| .ok_or_else(|| MmProxyError::FailedToGetBlockTemplate("block result".to_string()))? |
There was a problem hiding this comment.
I think pool mining could also benefit using the cached templates
There was a problem hiding this comment.
pool mining has a new template every second, so I don't think that is true
| b.get(merge_mining_hash.as_ref()).map(|item| { | ||
| trace!( | ||
| target: LOG_TARGET, | ||
| "Retrieving block template at height #{} with merge mining hash: {:?}", | ||
| item.data.clone().template.new_block_template.header.unwrap_or_default().height, | ||
| hex::encode(merge_mining_hash.as_ref()) | ||
| ); | ||
| item.data.clone() | ||
| }) |
There was a problem hiding this comment.
I would leave in the the trace log please
| @@ -325,7 +307,6 @@ impl BlockTemplateDataBuilder { | |||
| tari_difficulty, | |||
| tari_merge_mining_hash, | |||
| aux_chain_hashes: self.aux_chain_hashes, | |||
| new_block_template, | |||
There was a problem hiding this comment.
I would leave refactoring this (removing new_block_template and using tari_block instead) for a next PR when we can do ample tests to verify that the change is good. We had very-hard-to-find sync issues between when the template is requested from the base node and when the block is finally populated by the base node, of state changing midway.
Co-authored-by: SW van Heerden <swvheerden@gmail.com>
…otocol.rs Co-authored-by: SW van Heerden <swvheerden@gmail.com>
Description --- Sha-P2Pool must be integrated into universe to help users earn more by mining in a pool. P2pool should be able to enable/disable from the app. Motivation and Context --- How Has This Been Tested? --- https://github.com/user-attachments/assets/7fa09375-d5e3-4e3f-b990-497dc38612c0 What process can a PR reviewer use to test or verify this change? --- See tests Breaking Changes --- - [x] None - [ ] Requires data directory on base node to be deleted - [ ] Requires hard fork - [ ] Other - Please specify **NOTE:** This PR must wait for the following changes to be merged: - tari-project/sha-p2pool#31 - tari-project/tari#6474 - tari-project/sha-p2pool#32 --------- Co-authored-by: stringhandler <stringhandler@protonmail.com> Co-authored-by: C.Lee Taylor <47312074+leet4tari@users.noreply.github.com> Co-authored-by: shan <47271333+shanimal08@users.noreply.github.com> Co-authored-by: Marcin Papież <mmpapiez@gmail.com> Co-authored-by: Misieq01 <38589417+Misieq01@users.noreply.github.com> Co-authored-by: Erika <87762061+NovaT82@users.noreply.github.com> Co-authored-by: stringhandler <stringhandler@gmail.com> Co-authored-by: Bertok Richard <richardb@Bertok-MacBook-Pro.local>
Adds merge mining support for p2pool