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.

Sign up for GitHub

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

Jump to bottom

fix: fix merge mining proxy pool mining #3814

Merged
merged 3 commits into from
Mar 9, 2022
Merged

fix: fix merge mining proxy pool mining #3814

merged 3 commits into from
Mar 9, 2022

Conversation

hansieodendaal
Copy link
Contributor

Description

Fixed merge mining proxy pool mining:

  • XMRig/merge_mininig_proxy error: 'required field "blocktemplate_blob" not found'
  • config file was not read correctly;
  • global config exited prematurely ;
  • round-robin for monerod_url - start at the next entry in the list when encountering connection errors;
  • updated cucumber integration test.

Motivation and Context

When used with Monero mainnet and pool mining, XMRig reported many errors and exited after mining for a while:

  • XMRig/merge_mininig_proxy had an issue with pool mining: 
    [2022-02-01 23:56:05.773]  origin   submitted to origin daemon (174/748)  diff 110685 vs. 311330
[2022-02-01 23:56:06.135]  cpu      accepted (859/61) diff 40000 (361 ms)
[2022-02-01 23:56:06.517] [127.0.0.1:7878] required field "blocktemplate_blob" not found
  • Default merge mining proxy config on windows was wrong
  • When monerod_url had an error new connection attempts were always started at the first entry instead of at the last used entry

How Has This Been Tested?

Cucumber tests for merge mining
System-level tests

@hansieodendaal hansieodendaal force-pushed the ho_fix_merge_mining_proxy_pool_mining branch from 6bf8057 to c8b1fce Compare February 14, 2022 09:42
stringhandler
stringhandler reviewed Feb 17, 2022
View reviewed changes
Copy link
Collaborator

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

I'm not convinced this is an improvement. The fixes to the config file are good, but the other changes, specifically the one I highlighted, do not seem to make any difference to the existing code

applications/tari_merge_mining_proxy/src/proxy.rs Outdated
warn!(target: LOG_TARGET, "Monerod server unavailable: {:?}", uri);
continue;
},
let current_url = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will always be "".to_string() because of the above return statement, which makes the extra code here and the find below unnecessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good pickup, I will fix the logic error here. If we have a troublesome monerod we do not want to connect to it straight away again even if it is possible; much better to try the next server in the list. self.last_available_server is zeroed if async fn proxy_request_to_monerod returns an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@hansieodendaal hansieodendaal force-pushed the ho_fix_merge_mining_proxy_pool_mining branch 2 times, most recently from 0d941d9 to bfb07e8 Compare February 23, 2022 02:47
@hansieodendaal
Fix merge mining proxy pool mining
d8a7aab 
Fixed merge mining proxy pool mining:
- config file was not read correctly;
- global config exited prematurely ;
- round robin for monerod_url - start at the next entry in the list when
  encountering connection errors instead of always starting at the beginning;
- updated cucumber integration test.
@hansieodendaal
Review comments
5d57059
@hansieodendaal hansieodendaal force-pushed the ho_fix_merge_mining_proxy_pool_mining branch from bfb07e8 to 5d57059 Compare March 8, 2022 09:18
@aviator-app aviator-app bot merged commit 407160c into tari-project:development Mar 9, 2022
@hansieodendaal hansieodendaal deleted the ho_fix_merge_mining_proxy_pool_mining branch March 9, 2022 10:34
sdbondi added a commit to Cifko/tari that referenced this pull request Mar 14, 2022
@sdbondi
Merge branch 'development' into pr/3820
26a4841 
* development: (54 commits)
  fix(block-sync): use avg latency to determine slow sync peer for block sync (tari-project#3912)
  fix: fix merge mining proxy pool mining (tari-project#3814)
  revert: remove use of blocking tasks for DHT db (reverts tari-project#3887) (tari-project#3901)
  chore: add license info missing from some crates (tari-project#3892)
  fix(core): correctly filter pruned sync peers for block sync (tari-project#3902)
  ci: revert bors squash merge (tari-project#3900)
  fix: update metadata size calculation to use FixedSet.iter()
  docs(rfc): deep links structure convention - deep links is use (tari-project#3897)
  ci: use squash merge for bors (tari-project#3896)
  change struct
  fmt
  chore: fix npm dependencies
  feat: update FFI client user agent string
  fix: update wallet logging config
  fix completed tx index
  ignore clippy
  code review
  fmt
  fix: add bound for number of console_wallet notifications
  remove TODOs
  ...
sdbondi added a commit to sdbondi/tari that referenced this pull request Mar 15, 2022
@sdbondi
Merge branch 'development' into rfc-consensus-encoding-and-hashing
9a6f7ce 
* development: (118 commits)
  chore: clean up providing seed words from LibWallet (tari-project#3906)
  chore: move tari_script into its own crate (tari-project#3909)
  fix(consensus): check blockchain version within valid range (tari-project#3916)
  ci: fix missing npm deps and add javascript ci (tari-project#3910)
  refactor: use clap as a commands parser (tari-project#3867)
  chore: use git tagged tari_utilities and tari-crypto deps (tari-project#3913)
  fix: aligned tables left (tari-project#3899)
  ci: fix vue build
  v0.29.0
  feat!: add recovery byte to output features (tari-project#3727)
  add ffi ci check (tari-project#3915)
  fix(block-sync): use avg latency to determine slow sync peer for block sync (tari-project#3912)
  fix: fix merge mining proxy pool mining (tari-project#3814)
  revert: remove use of blocking tasks for DHT db (reverts tari-project#3887) (tari-project#3901)
  chore: add license info missing from some crates (tari-project#3892)
  fix(core): correctly filter pruned sync peers for block sync (tari-project#3902)
  ci: revert bors squash merge (tari-project#3900)
  fix: update metadata size calculation to use FixedSet.iter()
  docs(rfc): deep links structure convention - deep links is use (tari-project#3897)
  ci: use squash merge for bors (tari-project#3896)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philipr-za philipr-za philipr-za left review comments

@delta1 delta1 delta1 left review comments

@stringhandler stringhandler stringhandler approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hansieodendaal @delta1 @stringhandler @philipr-za