rpc-api: scheduler status flag in session

[ghost]

This adds an additional status flag to the /session endpoint that allows consumers of the API to differentiate whether a currently running coinjoin is part of a schedule of coinjoins or if it is a "single coinjoin".

Behavior

Single CJ:

• coinjoin_in_process: true
• coinjoin_is_part_of_schedule: false

Schedule of CJs:

• coinjoin_in_process: true
• coinjoin_is_part_of_schedule: true

Reasoning

I'm aware that fundamentally there's no difference between a CJ that's part of a schedule or a single CJ. They're both just instantiating the Taker who takes care of the appropriate logic.
Still for no other reasons than UI, it can be important to be able to differentiate the two on the client side.
In Jam, for example, we have one flow that takes care of running a schedule of transactions and another for making single collaborative transactions (aka "single CJs").
While a schedule is running, we'd like to indicate that in the UI in a way so that the user knows that the current transaction is not just a single CJ.
That's were a flag like this comes in handy.
We could of course try to retrieve the schedule via GET /taker/schedule and depending on the response determine if a schedule is setup to run. But this would be a rather clumsy approach.

Implementation

I decided to keep the semantics of the existing coinjoin_in_process intact and add an additional flag that further qualifies whether the CJ is part of a larger schedule of CJs.
Another way to do this would've been to introduce two distinct flags: one single_coinjoin_in_process and the other schedule_in_process. But this would have suggested that there are different types of CJs which is not really true as discussed above. Also, it would've technically been a breaking change. Therefore, I decided to go with the slightly less obvious, but probably more correct way.

Let me know what you think! Any feedback welcome. If we could get that in, it would help us a lot making the UI for Jam more intuitive.

[dergigi]

Nice!

I like the non-invasive approach of adding a flag, this way we don't risk any regressions by removing or modifying the existing flags.

Looks good to me ✅

[AdamISZ]

My 2 sats:

I can see that this desire to query what is currently happening, could arise out of the situation that client-side code is either starting or restarting, not knowing the current state of the jmwalletd daemon; so it wants to know the state in detail, not just the current CJ_***_RUNNING. But I would disagree that a /taker/schedule query is more messy than adding an extra state field; it seems both logical and relatively easy to do, given that it is the most general solution to the problem that would include future unanticipated structures/behaviours.

If /taker/schedule looks messy because it's an additional query, I'd say it seems very natural to include the current schedule in the Response of the current /session query (and then not bother with a new /taker/schedule), when authorized.

(I see the existing CJ_***_RUNNING state field as being something that helps us gate actions, if we are doing X then we can do Y but we cannot do Z, that kind of thing. Not information to the client on internal events).

(It's probably orthogonal but - are we not using websockets at all? I put that in because I felt it was essential so that you could get pushed updates immediately without having to poll - /session is a good example of that, though the really obvious example is deposits or txs into the wallet).

[theborakompanioni]

If /taker/schedule looks messy because it's an additional query, I'd say it seems very natural to include the current schedule in the Response of the current /session query (and then not bother with a new /taker/schedule), when authorized.

That seems like a reasonable and nice solution. Is it rather straightforward to add the schedule to the response without, for example, including a schedule that is present but not currently running?

[ghost]

If /taker/schedule looks messy because it's an additional query, I'd say it seems very natural to include the current schedule in the Response of the current /session query (and then not bother with a new /taker/schedule), when authorized.

Sure I can do that!

(It's probably orthogonal but - are we not using websockets at all? I put that in because I felt it was essential so that you could get pushed updates immediately without having to poll - /session is a good example of that, though the really obvious example is deposits or txs into the wallet).

Good point we are using them and we should add something there as well. I did not think about that -- thanks for bringing it up. The socket currently sends a coinjoin_state flag which is either:

• 0: taker is running
• 1: maker is running
• 2: neither is running

@AdamISZ would you prefer adding a new schedule property to the websocket response (like suggested for the /session endpoint) or extending the coinjoin_state flag? I think I'd prefer going with sending the schedule over the socket. We could also use this for progress updates for the scheduler feature. Edit: I'm realizing though, that it might be tricky to trigger a websocket update whenever the schedule file updates. We'd need to watch the schedule file somehow. So maybe this is something we should do in a separate PR?

Is it rather straightforward to add the schedule to the response without, for example, including a schedule that is present but not currently running?

@theborakompanioni good point, we can't just rely on the existence of the schedule file. That is present even after a schedule completed successfully. Like in the current implementation, what we would need to do is something like:

if self.coinjoin_state == CJ_TAKER_RUNNING and self.tumbler_options is not None:
  # include schedule

[theborakompanioni]

@dnlggr Seeing that /session response has been refactored to include schedule instead of flag coinjoin_is_part_of_schedule, would you believe this is ready for reviewing/testing?

[ghost]

Yes definitely! I think I've addressed all feedback.

[AdamISZ]

Thanks @dnlggr . So a couple of things I see: first, to get the current schedule, I think you'd better check the value of self.taker.schedule in the JMWalletDaemon object. However you'd need to check if the self.taker object exists (see stop_taker which sets it to None), and that it is not aborted (self.taker.aborted boolean). If these conditions are fulfilled then you can return that object. It should be a list of lists.

Next, is the entry in the yaml only a single array instead of an array of arrays?

[ghost]

Applied your suggestions @AdamISZ.

As you said, communicating schedule changes via the websocket would be good as well. However, I'd prefer doing this another time in a separate PR to keep changes small and incremental.

[AdamISZ]

This setup looks correct to me. I did briefly wonder about self.taker.schedule being None but I don't think it can be.

I'll try to run some brief sanity check test, but assuming that passes, I'm happy to merge unless someone objects.

[AdamISZ]

OK, using curl I have tested successfully that this functions as intended:

• If taker is not running, "schedule": null is returned.
• If /session request is made without an Auth header, "schedule": null is returned.
• If /session request is made with a valid Auth header, the actual schedule is returned as a list of lists.

tACK, merging.