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

Don't use honest_makers in case: mempool conflict #572

Merged
merged 1 commit into from
May 11, 2020

Conversation

AdamISZ
Copy link
Member

@AdamISZ AdamISZ commented May 8, 2020

Prior to this commit, if a tumbler coinjoin negotiation
failed in Phase 2, then the retry as per the logic in
taker_utils.tumbler_taker_finished_update would always
attempt to retry the transaction with those counterparties
that returned valid !sig responses. However this ignored
the case that all the counterparties responded validly,
but there was a mempool conflict in the created transaction.
After this commit, if it is detected that all counterparties
responded, it is assumed that a mempool conflict or similar
occurred with the transaction, and therefore it is better
to fallback to a schedule tweak and choose randomly again,
not to fix the counterparty set (which is likely to result
in failing again).

Prior to this commit, if a tumbler coinjoin negotiation
failed in Phase 2, then the retry as per the logic in
taker_utils.tumbler_taker_finished_update would always
attempt to retry the transaction with those counterparties
that returned valid !sig responses. However this ignored
the case that all the counterparties responded validly,
but there was a mempool conflict in the created transaction.
After this commit, if it is detected that all counterparties
responded, it is assumed that a mempool conflict or similar
occurred with the transaction, and therefore it is better
to fallback to a schedule tweak and choose randomly again,
not to fix the counterparty set (which is likely to result
in failing again).
@chris-belcher
Copy link
Contributor

chris-belcher commented May 9, 2020

Even though there's a lot of lines changed, actually most of the code has been intended and put inside an else: block. The new code is just two lines checking whether the nonrespondents list is empty. So utACK.

@AdamISZ
Copy link
Member Author

AdamISZ commented May 9, 2020

Yes. I have done some tests with it, and the logic change is very simple.

@AdamISZ
Copy link
Member Author

AdamISZ commented May 9, 2020

It is actually a rather needed change, as any txn-mempool-conflict errors in multi-join schedule results in just repeated failures, using up commitments.
What I actually spent a lot of time doing was trying to understand what exactly might cause this kind of scenario, even with default JM code. I actually didn't fully succeed, except clearly it can happen if people spend out of a pre-0.6.0 JM wallet while the yg is running (while after #359 that wallet change is seen and so conflicts won't happen). So this is my best hypothesis.

Meanwhile we obviously can't trust makers to do anything in particular, so this change is needed in any case.

@AdamISZ
Copy link
Member Author

AdamISZ commented May 11, 2020

There also seems no point waiting further on this. I have tested it as per above and as noted by @chris-belcher , the change is very simple (not to mention, this could fix a material degradation in user experience, depending on market conditions). Merging.

@AdamISZ AdamISZ merged commit 66bef00 into master May 11, 2020
@AdamISZ AdamISZ deleted the tumbler-txconflict branch June 10, 2020 15:20
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.

2 participants