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

feat: add miner input processing #6016

Merged

Conversation

hansieodendaal
Copy link
Contributor

@hansieodendaal hansieodendaal commented Dec 6, 2023

Description

Added miner input processing to the minotari miner and merge mining proxy. The miner wallet address, base node gRPC address and basic gRPC connection will be verified.

Motivation and Context

Users did not have a good user experience
See #5929

How Has This Been Tested?

System-level testing

What process can a PR reviewer use to test or verify this change?

Code walk-through
System-level testing

Breaking Changes

  • None
  • Requires data directory on base node to be deleted
  • Requires hard fork
  • Other - Please specify

Copy link

github-actions bot commented Dec 6, 2023

Test Results (CI)

1 260 tests   1 259 ✔️  14m 56s ⏱️
     39 suites         0 💤
       1 files           1

For more details on these failures, see this check.

Results for commit c3200cd.

♻️ This comment has been updated with latest results.

@ghpbot-tari-project ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Dec 6, 2023
@ghpbot-tari-project ghpbot-tari-project added the P-conflicts Process - The PR has merge conflicts that need to be resolved label Dec 6, 2023
Copy link

github-actions bot commented Dec 6, 2023

Test Results (Integration tests)

  2 files  +  2    1 errors  2 suites  +2   1h 0m 51s ⏱️ + 1h 0m 51s
14 tests +14  11 ✔️ +11  0 💤 ±0  3 +3 
20 runs  +20  11 ✔️ +11  0 💤 ±0  9 +9 

For more details on these parsing errors and failures, see this check.

Results for commit c3200cd. ± Comparison against base commit 0bc62b4.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@SWvheerden SWvheerden left a comment

Choose a reason for hiding this comment

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

There are a lof of places where this is done:

println!("{}", msg);
error!(target: LOG_TARGET, "{}", msg);

or similar. If the log config is setup correctly, which it is by default, this will result in double logging to the terminal. We should only use println! where absolutely required by the app for example where there is user input required. Or we wish to print out a enact message to him. General logging stuff should not be printout out to terminal.

Added miner input processing to the minotari miner and merge mining proxy.
The miner wallet address, base node gRPC address and basic gRPC connection
will be verified.
@SWvheerden
Copy link
Collaborator

SWvheerden commented Dec 7, 2023

I tested this locally, works as intended.

but this is why I don't like the double logs, it looks like its broken and looks unprofessional:

Initializing logging according to "pie/esmeralda/config/miner/log4rs.yml"

Please enter 'wallet-payment-address' ('quit' or 'exit' to quit) : 7c12ddd61cbf5d8d518f8c61dd86f00d483ef795cf66d496f
Please enter 'wallet-payment-address' ('quit' or 'exit' to quit) : 7c12ddd61cbf5d8d518f8c61dd86f00d483ef795cf66d496fba83086d8834019be

Please enter 'base-node-grpc-address' ('quit' or 'exit' to quit) : /ip4/127.0.0.1/tcp/18142

👛 Connecting to base node at http://127.0.0.1:18142
13:50 INFO  👛 Connecting to base node at http://127.0.0.1:18142
13:50 INFO  ⛏ Miner 09 reported 0.33MH/s with total 3.33MH/s over 10 threads. Height: 3026. Target: 200257119)
13:50 INFO  ⛏ Miner 05 reported 0.33MH/s with total 3.33MH/s over 10 threads. Height: 3026. Target: 200257119)

It prints most of the stuff double. The beauty of log4rs is that you can customize what is showed on terminal, if you don't want to see all of this, you can remove it. On default we choose what the user must see, but its not forced on the user.

}

/// Read base_node_socket_address arg or prompt for input
pub fn base_node_socket_address(
Copy link
Collaborator

Choose a reason for hiding this comment

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

a note here, we need to decide if this is fine or not.

Currently, the miner chooses the default address as 127.0.0.1:{Network port}
If you want a non interactive deploy, you need to set the correct default address in the config. If not the miner will block on user prompt.

@hansieodendaal
Copy link
Contributor Author

hansieodendaal commented Dec 7, 2023

Changing all the double prints:

Base node not running (miner):

5:53 ERROR Could not connect to base node
15:53 ERROR Is the base node's gRPC running? Try running it with `--enable-grpc` or enable it in the config.
15:53 ERROR Fatal error: ExitError { exit_code: GrpcError, details: Some("TLS connection error: transport error") }
15:53 ERROR Exiting with code: GrpcError
error: process didn't exit successfully: `target\release\minotari_miner.exe` (exit code: 105)

Base node not running (merge mining proxy):

15:54 ERROR Could not connect to base node
15:54 ERROR Is the base node's gRPC running? Try running it with `--enable-grpc` or enable it in the config.
15:54 ERROR Fatal error: TLS connection error: transport error
Error: TLS connection error: transport error
error: process didn't exit successfully: `target\release\minotari_merge_mining_proxy.exe` (exit code: 1)

Base node gRPC methods not enabled (miner):

15:54 ERROR Base node not responding to gRPC requests: 'get_tip_info'
15:54 ERROR Are the base node's gRPC mining methods denied in its 'config.toml'? Please ensure these methods are commented out:
  'grpc_server_deny_methods': "get_new_block_template", "get_tip_info", "get_new_block", "submit_block"

15:54 ERROR Fatal error: ExitError { exit_code: GrpcError, details: Some("Base node not responding to gRPC requests: 'get_tip_info'") }
15:54 ERROR Exiting with code: GrpcError
error: process didn't exit successfully: `target\release\minotari_miner.exe` (exit code: 105)

Base node gRPC methods not enabled (merge mining proxy):

15:54 ERROR Base node not responding to gRPC requests: 'get_tip_info'
15:54 ERROR Are the base node's gRPC mining methods denied in its 'config.toml'? Please ensure these methods are commented out:
  'grpc_server_deny_methods': "get_new_block_template", "get_tip_info", "get_new_block", "submit_block"

15:54 ERROR Fatal error: Base node not responding to gRPC requests: 'get_tip_info'
Error: Base node not responding to gRPC requests: 'get_tip_info'
error: process didn't exit successfully: `target\release\minotari_merge_mining_proxy.exe` (exit code: 1)

@ghpbot-tari-project ghpbot-tari-project removed the P-conflicts Process - The PR has merge conflicts that need to be resolved label Dec 7, 2023
SWvheerden
SWvheerden previously approved these changes Dec 8, 2023
applications/minotari_miner/src/run_miner.rs Outdated Show resolved Hide resolved
applications/minotari_miner/src/run_miner.rs Outdated Show resolved Hide resolved
@ghpbot-tari-project ghpbot-tari-project removed P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Dec 8, 2023
@SWvheerden SWvheerden merged commit 26f5b60 into tari-project:development Dec 8, 2023
7 of 12 checks passed
@hansieodendaal hansieodendaal deleted the ho_parse_miner_inputs branch December 8, 2023 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants