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

WIP: Synchronize sending auth #1560

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

AdamISZ
Copy link
Member

@AdamISZ AdamISZ commented Sep 26, 2023

No description provided.

@AdamISZ
Copy link
Member Author

AdamISZ commented Sep 26, 2023

This bears some explaining.

For context:

Taker Maker
!fill (with commitment) -> bans if commitment seen, and broadcasts
<- !pubkey
!auth ->
end of phase 1 <- !ioauth (with utxos) broadcasts commitment after sending

(Tech note: "broadcasts" actually means transferring to other makers, who then do a !pubmsg, as this disguises which makers participated in transactions).

So, the early "broadcasts" in the first row is a situation where: a taker comes along and "blatantly" uses the same commitment as they used in some earlier transaction. This would have been persisted in the file that each maker keeps, because it would have been broadcast from an earlier transaction (see the bottom row). So they would in theory not succeed, but get stopped at this point.

The problem is that this relies on some vague concept of synchrony.
If there are two Makers, M1 and M2, and M1 goes through all of the above conversation before M2 receives the !fill on the first row, then the broadcast event in the bottom row for M1 will result in M2 adding the commitment to its database before it sees the commitment in the !fill message from the taker. Thus they will ban it and not continue with the transaction.

I suspect that this scenario is now happening a lot more often than it used to (albeit it's not a total disaster: 1/ there is no money loss and 2/ there is no privacy loss here, there is just the potential loss of access to makers who have bad latency; the banning by these slow participants is not going to affect fast participants, who would have already sent !pubkey and will then not ban/block the transactions themselves).

It seems like the worst scenario is just: there is 1 out of 10 makers who is really fast, and all other 9 have a multi-second delay. Here the transaction is blocked unnecessarily, because all 9 will refuse to continue after !fill.

The solution thus proposed in this PR is just to block continuation of the conversation until everyone has received !fill, or give up after a timeout of 60s if some makers do not reply with !pubkey. This can make some transactions relatively slower than they should be. Also, the added state tracking is a bit messy, but, not sure that's a central question here (the state tracking in daemon_protocol.py is already messy and really should be overhauled anyway).

I think this isn't very obvious, there are a number of ways to look at it, so I'm all ears :)

@AdamISZ
Copy link
Member Author

AdamISZ commented Sep 26, 2023

Marked WIP because obviously this kind of thing needs extensive testing and, just as much, we need to reason about whether this is the best solution we can come up with.

@kristapsk
Copy link
Member

Needs rebase.

Prior to this commit, in scenarios of very big latency differentials
between maker bots, it was possible for one maker to complete the
conversation up to !ioauth and broadcast the commitment, before another
maker had even received the !fill message. This causes incorrect local
blacklisting of commitments for that (and any other slow) maker.
After this commit, the taker does not continue Phase1 with the !auth
message, until either (a) all of the makers have sent their !pubkey
message (indicating that they have already seen the commitment in the
!fill message), or (b) a timeout of 60 seconds (after which any maker
that has not sent the !pubkey is ignored for the rest of the
conversation).
@AdamISZ AdamISZ force-pushed the synchronize-sending-auth branch from 23720ff to dad3bb6 Compare November 22, 2023 14:50
@AdamISZ
Copy link
Member Author

AdamISZ commented Nov 22, 2023

Rebased on master dad3bb6

@AdamISZ
Copy link
Member Author

AdamISZ commented Dec 1, 2023

This needs some review from other people.

Would be a great exercise for someone trying to understand the in-depth mechanics of Joinmarket better, I think!

@kristapsk kristapsk added the protocol related to the communication between maker / taker label Dec 2, 2023
@kristapsk
Copy link
Member

Concept ACK, logic seems correct to me.

Could some test be added that always fails without these changes and always succeeds with them?

self.send_all_auth()

@taker_only
def send_all_auth(self, force:bool =False) -> None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
def send_all_auth(self, force:bool =False) -> None:
def send_all_auth(self, force: bool = False) -> None:

@AdamISZ
Copy link
Member Author

AdamISZ commented Dec 16, 2023

Could some test be added that always fails without these changes and always succeeds with them?

You're right; the logic of this is sufficiently non-trivial that it requires some kind of testing, though it'll be some work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
protocol related to the communication between maker / taker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants