Session support as described in issue #634 #714
Conversation
|
Hi, I'm not sure how to fix the conflict with the TestWallet class in test/commontest.py. I added support for passing the password to the wallet constructor, but there seems to be a conflict when this test runs. |
joinmarket/support.py
Outdated
| f.close() | ||
|
|
||
| def update_session_tx_confirmed (sessionname,i_tx): | ||
| sessionfilename = os.path.join(script_dir,'../sessions/'+sessionname) |
There was a problem hiding this comment.
Crashed here on a first test run due to sessionname not being set (it's 'None'). The help message says it's optional, so I guess can be fixed various ways, e.g. don't call this fn if sessionname option not chosen, or something similar. It's currently called unconditionally in TumblerThread.confirm_callback()
|
Tested: run on regtest without any adverse maker behaviour. Added sessionname as per minor bug above, test passed OK. Second test: with malicious makers that fail to respond with small probability (one way of testing restart). Test passed OK, but a few things worthy of considering: On restarting, might want to give users help about how, for example, restarting with I notice the wallet utxo printout goes to mixdepth 11 by default now, can imagine why that might be, but need to investigate. Will need to do tests that explicitly cover the case of quit before confirm, so far only doing fast confirm tests with regtest. Another tricky case (and relatively common) is commitment sourcing failure. Overall it's looking promising in terms of achieving the intended goal. @chris-belcher @adlai @AlexCato and anyone else, your input would be greatly appreciated. |
|
(There are several more trivial points like : squashing before commits - I'm not super strict about that personally, but it's better not to at least not have 'remove debug' type of commits. there are also really minor formatting things like spaces in function arguments, but I'm again not that bothered, maybe others are.) |
|
The changes you've made to wallet should not be there. I know where it comes from - the 'Wallet' object having to receive a password interactively is a pain. I've been working on improving this kind of thing in the joinmarket-clientserver repo (this and a number of similar things) - but only by removing it from the Wallet class so that the necessary interactivity can be defined by callers (see the joinmarket-qt gui e.g.). But putting a password in command line -> If you're restricting yourself to a command line script (as you are here), then just leave it as interactive even on restart. (Edit: technical correction, I don't think it's that the changes you've made to Wallet are wrong, it's that I think it needs to be called interactively as per the above security comment). |
joinmarket/support.py
Outdated
| with open(sessionfilename, "w") as sources: | ||
| for line in lines: | ||
| log.debug('line: '+line+'\n') | ||
| if re.match(r'^["]next_tx["][:][ ][0-9]',line): |
There was a problem hiding this comment.
General comment about this regexp matching approach: it should be far simpler to just json.loads() and then do the updates/changes purely in Python, then write back with json.dumps()
|
There is a bug in handling the case of quit after broadcast but before confirm: If a user quits before confirmation has occurred, I noticed this when trying quitting after unconfirm (I set the block generation time to 20 seconds in the regtest setup). I was seeing I haven't thought through how one would fix it in this version of the tumbler, I presume moving the update to the unconfirm_callback although one would have to think through the details. |
|
An interesting example of the above scenario; user quit on unconfirm of sweep, then restart and generate a block, get: which makes sense since the sweep already happened, there are no coins in the mixdepth. However, on restart again the problem self-corrects since the confirm callback now has been called and next_tx has been updated. |
tumbler.py
Outdated
| self.confirm_callback, pushed_destaddr, | ||
| self.timeout_callback) | ||
| with self.lockcond: | ||
| self.lockcond.wait() |
|
Tested 'addrask' functionality: appears to be fine. |
|
Note the code here. This is a way to set the mixdepth balance that's used to calculate the amount fraction, at the start of each mixdepth, only. So for example, if the amount fraction is 10%, it will be 10% of the amount when starting to process that mixdepth. If you restart the tumbler, This is not the end of the world, but there could be cases where it causes problems, and in any case it fails to create the intended amount distribution. And in case it does cause a problem that problem will be persistent since on restarting a second time the same situation occurs. I addressed this distribution preservation issue here although of course it doesn't carry across really. |
|
Hi thanks for coming through quickly on this. I will review it within the next few hours. I am also willing to do any tests you need, but then someone will eventually have to read the code of the tests. I will probably give you 1% of the bounty for being the escrow/mediator. That's usually what the mediator I use at Rein gets. You may deserve more, but at least it's something for the principle. I'm not in a super rush, as long as things are smoothly moving forward with a predictable timeframe, then it's fine with me. |
|
For the case of quitting before confirm, I thought this should handle this: fcc6b7b#diff-e5ebbf3237752f0c78e5834760582020R386 I put a call to new function "update_session_tx_pushed" right before the tx is broadcasted, and this saves the info about the unconfirmed tx to the sessions file for later waiting here: fcc6b7b#diff-7dca4cc17829f93a2517b5a8dd3ef018R373 but I will test again Also note: next_tx is the tx index and the line saying "that was tx 1 out of 9" is the number (index+1). So that's what it should say when the first tx is still unconfirmed. I think I just have to fix one thing with incrementing next_tx though |
|
I now increment the next_tx variable right after the confirm and before it is used in the tx loop. Also fixed some minor things you mentioned, and one subtle thing: I now put the pushed_tx info before the next_tx in the session file, so that it does not stay marked as unconfirmed right after processing the next_tx line and before removing the unconfirmed tx info. Will fully test tomorrow, but I think this last commit should work. |
|
For the wallet password, I can see some cases where it can be passed safely on the command line, such as in ram disk os or by disabling bash history. Alternatively, you can pass a password file from which you read the password. I think it would be useful to automate the password passing, such as in a script that keeps restarting the tumbler if it fails (many times I have liquidity or no response problems that get resolved when I restart the tumbler). Maybe there's some command line tools to simulate a user passing an argument to a program, but would be easier if it was built in. |
|
Looked at this for the first time and have two comments/requests about moving code around. Updating sessions is only a tumbler feature and has nothing to do with sendpayment, electrum plugin, bitcoin-qt integration or any other use of the taker class. So I believe the new field Also I think the functions It was a good idea to use the balance of each mixdepth to figure out where the previous run stalled. Finally, please squish your commits together into one (or multiple if it makes logical sense). Here is the tutorial I used in case it helps http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html |
|
ok I isolated the changes to the tumbler script and squashed the commits into 1, but did a force push (didn't know how to resolve it otherwise), so now I only have 1 commit in my history. |
|
Forgot to test that last change. Will do now. |
fix bugs with -D option and sessionname path fix path join remove debug line fix tumbler bug rebased session support
|
ok I think it is tested enough now so someone can merge or test further. Thanks. |
See the issue here: #634