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

Remove CLI option --restart from tumbler #1546

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AdamISZ
Copy link
Member

@AdamISZ AdamISZ commented Sep 1, 2023

Prior to this commit, the --restart option was still present in the help documentation of tumbler.py, however this functionality has been removed from the algorithm (restarting now just means "running again", since the algorithm now functions the same way independent of the location of the coins in mixdepths). Moreover, running with this option could lead to crash conditions.
After this commit, the --restart option is removed from the help text, and no longer executed in code. Further, a slight change is made to the wording of the tumblerguide.md documentation, section "Restarting", to make the situation clearer.

@AdamISZ
Copy link
Member Author

AdamISZ commented Sep 1, 2023

Implemented the most obvious test on regtest; ran the tumbler and Ctrl-C after a few transactions. Then ran again and the process completed successfully.

Prior to this commit, the --restart option was still present in the help
documentation of tumbler.py, however this functionality has been removed
from the algorithm (restarting now just means "running again", since the
algorithm now functions the same way independent of the location of the
coins in mixdepths). Moreover, running with this option could lead to
crash conditions.
After this commit, the --restart option is removed from the help text,
and no longer executed in code. Further, a slight change is made to the
wording of the tumblerguide.md documentation, section "Restarting", to
make the situation clearer.
@AdamISZ AdamISZ force-pushed the tumbler-no-restart-option branch from 5e325e1 to 0ea722d Compare September 1, 2023 15:24
@PulpCattel
Copy link
Member

Following from the telegram discussion.

As we discussed, there are two obvious ways to do a "restart":

  1. The "just run again" way. Where the user can start the tumbler again with the same options but a lower mixdepth count. This should always be supported by definition. It's a new run.
  2. Pass the pre-existing schedule, which should be under /logs in our JM folder, or possibly somewhere else if the user passed a custom schedule to begin with. We already have a --schedulefile in tumbler.py, and supporting custom schedules is IMHO something that we definitely want to do. (BTW, the --schedulefile option also mentions a restart. Which makes a lot of sense to me even without the --restart option.)

I was under the impression that both ways are supported by JM for "free". Meaning, as said, 1. by definition, and 2. because it's a schedule like any other. Am I mistaken?

BTW, I initially thought the restart option was pointless because of option 2., i.e., by being already implicit in the schedule file, we do not need a special case for it.

Assuming what I said above is the case, I think we should mention in the restarting section of the doc that it's also possible to reuse the schedule for restart. My personal opinion (biased for sure, I've played with schedules for a long time) is that using a pre-existing schedule is cleaner (and possibly cheaper?), so that would be my preferential recommendation for a user. There are still trade-offs, like if the user received or made payments before restarting, the schedule will be out of date, possibly causing issues. Nonetheless, the general principle seems better to me. Something like "if it crashed, just run the tumbler with --schedulefile /home/user/.joinmarket/logs/tumble.schedule".
I'm also generally in favor of letting the user know what a schedule is, and how to use it when needed (after all, that schedule is touching his money), but I see how the complexity might scare people away, or cost them troubles.

@AdamISZ
Copy link
Member Author

AdamISZ commented Sep 3, 2023

My personal opinion (biased for sure, I've played with schedules for a long time) is that using a pre-existing schedule is cleaner (and possibly cheaper?), so that would be my preferential recommendation for a user.

I can definitely see that. You correctly perceived my intention in going the schedule route, and I'm glad that it works. As you point out, it's still possible to footgun, in either approach.

Assuming what I said above is the case, I think we should mention in the restarting section of the doc that it's also possible to reuse the schedule for restart. (snip) Something like "if it crashed, just run the tumbler with --schedulefile .. (snip)

Yes after this and the preceding discussions, it's now very clear that the restarting section of the doc needs to be substantially improved. I guess I'll try to do that and update the doc shortly.

We should be clear in this discussion though, that you're advocating for --schedulefile and not for --restart, right (which is principally what ought to already have been removed and this PR was cleaning that up .. I think honestly I just forgot to do this follow up PR, I kind of vaguely mentioned it in #1324 .

I was quite enthusiastic about originally creating the schedule file approach for this reason (flexibility, especially for power users), and while it had some meaningful limitations (see #429 , not quite what I think, but what I do totally understand is that even if users can edit such a file, it doesn't mean that most of them will want to, or feel comfortable doing it), it's good that it exists.

I'm also generally in favor of letting the user know what a schedule is,

Yes. Note the tumblerguide doc does currently address this quite a bit by showing examples and referring to it. Feel free to suggest further additions to the doc to make that clearer.

I think we're in vague agreement.

@PulpCattel
Copy link
Member

PulpCattel commented Sep 3, 2023

We should be clear in this discussion though, that you're advocating for --schedulefile and not for --restart

Yes. I guess --restart could in theory be kept just as a convenience (basically an alias) for --schedule with the default schedule file in /logs. But I have no problem removing it and it might actually be clearer without it.

while it had some meaningful limitations (see #429 , not quite what I think, but what I do totally understand is that even if users can edit such a file, it doesn't mean that most of them will want to, or feel comfortable doing it), it's good that it exists.

Absolutely, I think it's great to have it. I think labels already would go a long way. As I see it, this is a case where the schedule looks much more complicated/intimidating than it really is. I mean, it's literally just a few rows of a few values each, and most of these values (if not all really) are trivial to understand (and a taker should already be familiar with them). I'm fairly confident our users can handle it no problem.
That being said, I still agree with you that most users won't have to change it manually (especially if the tumbler does its job well).

Yes. Note the tumblerguide doc does currently address this quite a bit by showing examples and referring to it. Feel free to suggest further additions to the doc to make that clearer.

Sorry, the point I was trying to make was referred to what you said on telegram about what's the easiest/safest approach for user. So I was trying to say that I don't mind telling user "hey, if you want to restart the tumbler you can use the schedule" and that I don't think we should spend too much time hiding the schedule from the users in an attempt to make things easier.

@PulpCattel
Copy link
Member

Linking some closely related discussion in #1352, in particular #1352 (comment)

@kristapsk
Copy link
Member

Needs rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants