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: interop-jdc-change-upstream MG to Integration Test #1207

Open
plebhash opened this issue Oct 11, 2024 · 7 comments
Open

CI migration: interop-jdc-change-upstream MG to Integration Test #1207

plebhash opened this issue Oct 11, 2024 · 7 comments

Comments

@plebhash
Copy link
Collaborator

plebhash commented Oct 11, 2024

as part of #1121 a good first MG test to migrate is interop-jdc-change-upstream because:

  • it covers many roles (including JD)
  • it requires the usage of a mock, which we still don't have in the integration test framework

this will help us continue shaping up the integration test framework, as a continuation of #1122

@plebhash
Copy link
Collaborator Author

plebhash commented Oct 11, 2024

to elaborate a bit more on the requirement of a mock:

this test is essentially making sure that JDC has a sane implementation for the fallback mechanism, which is one of the major selling points of JD:

assuming a JDS+Pool have already acknowledged some specific job, Pool rejecting shares must trigger JDC to switch upstreams

now, how do we get a Pool to reject shares that are supposed to be valid? it wouldn't really make sense to add that kind of functionality to SRI Pool implementation, which is supposed to be sane

therefore, we need a way to mock a role by having something that establishes a connection and sends custom messages

@plebhash
Copy link
Collaborator Author

plebhash commented Oct 11, 2024

I did some experimentation where I tried modifying the Sniffer struct to give it the ability to inject messages, but this modification proved not to be trivial

I wonder if an easier approach could consist of the following:

we add a new Mock struct that does not relay messages... in fact, it doesn't even need to have both upstream+downstream connections, as one single connection would suffice

Mock would keep a queue record with all messages it receives form its single connection, and allow the user to assert against them (much like Sniffer)

but it would also have a method that would allow the user to inject some custom message into the connection

@jbesraa
Copy link
Contributor

jbesraa commented Oct 15, 2024

I did some experimentation where I tried modifying the Sniffer struct to give it the ability to inject messages, but this modification proved not to be trivial

I wonder if an easier approach could consist of the following:

we add a new Mock struct that does not relay messages... in fact, it doesn't even need to have both upstream+downstream connections, as one single connection would suffice

Mock would keep a queue record with all messages it receives form its single connection, and allow the user to assert against them (much like Sniffer)

but it would also have a method that would allow the user to inject some custom message into the connection

Yea I also think it need to be something a bit different than the Sniffer. It would need to be a "role" that can act as either upstream or downstream. As mentioned, we would also need to define some functionality to tell it "when you receive message X return message Y".
This issue requires also a bit of work in the testing framework setup, allowing us to start JDS and JDC in test env. As we already done #1095 and #993, this should not be a huge deal.

Anyway, please assign me here as I already started looking into this.

@jbesraa
Copy link
Contributor

jbesraa commented Oct 18, 2024

OK, I take back my previous comment. It ended up being more straightforward to implement in the Sniffer than I initially thought. There is a draft here 961caa7.

@plebhash
Copy link
Collaborator Author

currently blocked by #1245

@jbesraa
Copy link
Contributor

jbesraa commented Nov 25, 2024

I was able to successfully test this today but some of the futures in the code keep hanging and the test is not exiting for that reason. The problematic task is roles/roles-utils/network-helpers/src/noise_connection_tokio.rs L72 (recv_task) ( I guess the send_task will be an issue as well once recv_task is handled.

In a way we could do some mingle wingle and try to resolve this from the testing side, but I think this is a technical debt in the roles code and I wonder if we shouldnt just start to refactor that already, at least the roles_utils ..

In regards to the actual functionality, I am not convinced the JDC is actually falling-back successfully. Well, it does fallback and send handshake/setup connection to next pool in vec!, but something is off with the other roles/machines(miner). I do not think there connection is stable to allow this switch. The miner could be still sending shares to JDC while JDC didn't yet receive SubmitSharesError for previous submissions and we get into some kind of weird loop/behavior. The current functionality in JDC is a double nested loop(which is really not needed, not sure why it is there) and on a fallback, one loop is break and it calls all the task_handlers.abort that it knows about, but it does not know about the Connection::new we do in the testing. Anyway, it is a lot of fun.

@plebhash
Copy link
Collaborator Author

currently blocked by #1266

@plebhash plebhash moved this from Todo 📝 to Blocked ⛔ in SV2 Roadmap 🛣️ Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Blocked ⛔
Development

No branches or pull requests

2 participants