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 proxy monerod resiliency #6637

Conversation

hansieodendaal
Copy link
Contributor

@hansieodendaal hansieodendaal commented Oct 17, 2024

Description

Added resiliency to the merge mining proxy monerod connection:

  • At startup if the monero.fail is not available
  • With the default list of monerod servers (it is not necessary anymore to populate the list in config.toml or from the command line)
  • When qualifying the starting list of monerod servers
  • When getting the fully qualified monerod URL to send requests to during runtime

Motivation and Context

All the things addressed in this PR were handled inadequately.

How Has This Been Tested?

  • Added unit tests.
  • System-level testing:
    • Start the merge mining proxy using the default config. [PASSED]
    • Start the merge mining proxy where use_dynamic_fail_data = false. [PASSED]
    • Start the merge mining proxy where use_dynamic_fail_data = true but monero_fail_url is invalid. [PASSED]
    • Start the merge mining proxy where use_dynamic_fail_data = false with 5 entries in the monerod_url list but where the first 4 entries are invalid. [PASSED]
    • Perform a merge mining proxy + monerod stress test by issuing json rpc commands to the proxy that needs responses from the monerod server, testing multiple access paths into match inner.handle(&method_name, request).await {, all of this while merge mining is taking place. [PASSED]

Stress test results

method: get_height, tick: 1.00s
  1: method: get_height; time now: 07:41:12.966; duration: 354.83ms, response length: 124
  2: method: get_height; time now: 07:41:14.976; duration: 367.71ms, response length: 124
  3: method: get_height; time now: 07:41:16.974; duration: 360.16ms, response length: 124
  4: method: get_height; time now: 07:41:18.976; duration: 354.45ms, response length: 124
  5: method: get_height; time now: 07:41:20.990; duration: 373.91ms, response length: 124
  ...
method: get_version, tick: 1.00s
  1: method: get_version; time now: 07:41:22.963; duration: 355.97ms, response length: 686
  2: method: get_version; time now: 07:41:25.003; duration: 378.83ms, response length: 686
  3: method: get_version; time now: 07:41:26.978; duration: 369.92ms, response length: 686
  4: method: get_version; time now: 07:41:28.978; duration: 362.31ms, response length: 686
  5: method: get_version; time now: 07:41:30.979; duration: 358.23ms, response length: 686
  ...
method: get_block_template, tick: 1.00s
  1: method: get_block_template; time now: 07:41:33.023; duration: 408.98ms, response length: 1490
  2: method: get_block_template; time now: 07:41:35.007; duration: 388.51ms, response length: 1490
  3: method: get_block_template; time now: 07:41:37.002; duration: 388.86ms, response length: 1490
  4: method: get_block_template; time now: 07:41:39.005; duration: 386.01ms, response length: 1490
  5: method: get_block_template; time now: 07:41:41.018; duration: 410.48ms, response length: 1490
  ...

image

image

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

  • Code review
  • System-level testing as above

Breaking Changes

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

Added resiliency to the merge mining proxy monerod connection.
Copy link

github-actions bot commented Oct 17, 2024

Test Results (CI)

    2 files     86 suites   23m 54s ⏱️
1 339 tests 1 339 ✅ 0 💤 0 ❌
2 677 runs  2 677 ✅ 0 💤 0 ❌

Results for commit 81f423b.

♻️ 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 Oct 17, 2024
Copy link

github-actions bot commented Oct 17, 2024

Test Results (Integration tests)

 2 files  + 2  11 suites  +11   24m 37s ⏱️ + 24m 37s
36 tests +36  34 ✅ +34  0 💤 ±0  2 ❌ +2 
38 runs  +38  36 ✅ +36  0 💤 ±0  2 ❌ +2 

For more details on these failures, see this check.

Results for commit 81f423b. ± Comparison against base commit c6cbbc1.

♻️ This comment has been updated with latest results.

@SWvheerden SWvheerden merged commit c51ba7a into tari-project:development Oct 18, 2024
21 of 23 checks passed
SWvheerden pushed a commit that referenced this pull request Oct 18, 2024
Description
---
Add merge mining proxy monerod stress test code used to perform stress
tests for #6637.

Motivation and Context
---
The tests may need to be performed again in the future.

How Has This Been Tested?
---
Tested as part of #6637.

What process can a PR reviewer use to test or verify this change?
---
Code review.

<!-- Checklist -->
<!-- 1. Is the title of your PR in the form that would make nice release
notes? The title, excluding the conventional commit
tag, will be included exactly as is in the CHANGELOG, so please think
about it carefully. -->


Breaking Changes
---

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

<!-- Does this include a breaking change? If so, include this line as a
footer -->
<!-- BREAKING CHANGE: Description what the user should do, e.g. delete a
database, resync the chain -->
sdbondi added a commit to sdbondi/tari that referenced this pull request Oct 18, 2024
* development:
  test: add merge mining proxy monerod stress test code (tari-project#6639)
  test: add DammSum known-answer test (tari-project#6638)
  feat: add proxy monerod resiliency (tari-project#6637)
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 P-reviews_required Process - Requires a review from a lead maintainer to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants