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

CI migration: translation-proxy MG to Integration Test #1208

Closed
plebhash opened this issue Oct 11, 2024 · 4 comments · Fixed by #1262 or #1298
Closed

CI migration: translation-proxy MG to Integration Test #1208

plebhash opened this issue Oct 11, 2024 · 4 comments · Fixed by #1262 or #1298

Comments

@plebhash
Copy link
Collaborator

plebhash commented Oct 11, 2024

The translation-proxy MG test displays undeterministic behavior on CI.

The MG test does the following:

  • launch a tProxy
  • launch a SV1 CPU miner
  • injects messages to tProxy while mocking an upstream

tProxy will establish a channel with the mocked upstream with some predefined difficulty target (via OpenExtendedMiningChannel)

once tProxy receives a NewExtendedMiningJob, it forwards it to the CPU miner (as Sv1 messages)

the lack of determinism comes from the fact that two different things could happen after NewExtendedMiningJob is sent:

  • maybe CPU miner finds a share that is valid under the Channel difficulty target, therefore tProxy sends SubmitSharesExtended to its upstream
  • maybe tProxy finished its difficulty adjustment (by looking at SV1 shares submitted by the CPU miner), and it needs to change the difficulty target of the Channel via UpdateChannel

it's impossible for us to know beforehand the nominal_hash_rate of the CPU miner (since it depends on the available system resources)

as a consequence, we often see the following error on the current MG CI:

WRONG MESSAGE TYPE expected: 27 received: 22
  • 27 is decimal for 0x1b, which means SubmitSharesExtended
  • 22 is decimal for 0x16, which means UpdateChannel

(ref: Message Types Spec)

that is because the MG test is fixed in a way that after sending NewExtendedMiningJob, it expects ONLY SubmitSharesExtended, so whenever the difficulty adjustment finishes BEFORE a valid share is found, CI breaks

#1059 was meant to add this kind of functionality to MG, which would allow it to assert that EITHER one specific message OR another arrived. Unfortunately, the trade-offs on the engineering effort for implementing this feature on MG didn't seem appealing.

hopefully the Integration Test framework will give us enough flexibility to avoid this kind of problem

@plebhash
Copy link
Collaborator Author

plebhash commented Oct 11, 2024

a comment about mocking roles here:

even though the MG test is mocking an upstream, we could theoretically achieve this kind of test by connecting tProxy to a Pool, which would do the same things the mock is doing:

  • SetupConnection/.Success
  • OpenExtendedMiningChannel/.Success
  • NewExtendedMiningJob

so (differently from #1207) there's no strong reason to use a mock here, aside for simplicity.

so far I don't really have a strong opinion on whether we should do this integration test with a mock or a real upstream.

the only pro-mock argument I can think of is: maybe launching a real TP+Pool could make the test execution slower than necessary?

but yeah, not a strong opinion.

@plebhash

This comment was marked as outdated.

@plebhash plebhash moved this from Todo 📝 to In Progress 🏗️ in SV2 Roadmap 🛣️ Nov 28, 2024
@plebhash
Copy link
Collaborator Author

plebhash commented Dec 3, 2024

this tProxy CI migration is currently blocked by the fact that shares are undeterministic, which makes it hard to determine the appropriate timing for the Integration Test runtime to halt

the current naive approach is to sleep for a pre-determined period of time, which could mean that a share was found or not

we have two alternatives:

  • premined shares (which I personally don't like)
  • wait/block until a share is found, instead of waiting for a fixed amount of time

the second approach is described on #1282 and potentially solved via #1283

@plebhash
Copy link
Collaborator Author

plebhash commented Dec 10, 2024

here's a commit based on #1284 that would potentially unblock PR #1262

plebhash@45542e0


note: do not cherry pick this, we still need to remove unnecessary JDC logic (already pointed out on PR review above) and then do proper testing

note: plz ignore #1283, it was a suboptimal solution

plebhash added a commit to plebhash/stratum that referenced this issue Dec 10, 2024
WARNING: this commit is just to illustrate how to unblock stratum-mining#1208

DO NOT CHERRY PICK, we still need to fix unnecessary JDC logic
@plebhash plebhash moved this from In Progress 🏗️ to Blocked ⛔ in SV2 Roadmap 🛣️ Dec 11, 2024
@plebhash plebhash moved this from Blocked ⛔ to In Progress 🏗️ in SV2 Roadmap 🛣️ Dec 12, 2024
@github-project-automation github-project-automation bot moved this from In Progress 🏗️ to Done ✅ in SV2 Roadmap 🛣️ Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done ✅
2 participants