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

Affiliate server integration #10091

Merged
merged 59 commits into from Feb 22, 2023
Merged

Conversation

lontivero
Copy link
Collaborator

Continuation of #10055

@lontivero lontivero mentioned this pull request Feb 9, 2023
@kiminuo
Copy link
Collaborator

kiminuo commented Feb 10, 2023

Maybe the following commits would be useful: https://github.com/kiminuo/WalletWasabi/commits/affiliate-server-review

@kiminuo
Copy link
Collaborator

kiminuo commented Feb 15, 2023

74992a2 is great work.

@kiminuo in order to be sure there were not concurrency problems in the CoinJoinRequestsUpdater I moved all the code that is effectively an extension of Arena to a new class called AffiliateDataCollector. That class is the one that subscribes to Arena's events and keeps track of the data elements that collects from Arena. The events risen by Arena are all synchronous and are all wrapped around the Arena's AsyncLock so, no race condition is possible there.

Sounds good to me.

AffiliateDataCollector has an outbox to communicate asynchronously with the world (CoinJoinRequestsUpdater) , it is a channel were it writes two notifications: RoundEnded (when a round switches to Ended state) and RoundBuiltTransaction (when the round has a transaction that can be signed by participatnts).

Because RoundData is accessed by AffiliateDataCollector only, which is an extension of Arena and it is never accessed concurrently, all its ConcurrentBug<Tuple<,>> were removed.

Please take a look at it. Specially attention to these two:

* [Separate concurrent-safe code from non-concurrent-safe code](https://github.com/zkSNACKs/WalletWasabi/pull/10091/commits/aa73eda97aaefb6ca906af13130f5f5bb34bcf99)

* [Complete separation of concurrent code](https://github.com/zkSNACKs/WalletWasabi/pull/10091/commits/1d055823e316d182fd884cdff199a24a80999255)

I have not found an issue but the build is broken for me now.

@lontivero
Copy link
Collaborator Author

@kiminuo I found a race condition thanks to the UTs. I fixed it just by wrapping access to the RunningAffiliateServers around a lock. Please take a look, if you come with a better solution we can improve that. Otherwise I think we can go with this version as it is.
WDYT?

@kiminuo
Copy link
Collaborator

kiminuo commented Feb 16, 2023

@kiminuo I found a race condition thanks to the UTs. I fixed it just by wrapping access to the RunningAffiliateServers around a lock. Please take a look, if you come with a better solution we can improve that. Otherwise I think we can go with this version as it is. WDYT?

Good catch. I proposed the same appoach in the original PR (maybe in a different place). However, yes, it looks ok.

btw: We use this pattern avoid using lock (..) { ... }s and rather use immutable collections pattern in multiple places in the codebase and ... some probably are races and some will likely turn into races once code is updated later on. The lock (..)s are much easier to review and harder to mess up and that's why the use of them makes sense to me.

@kiminuo
Copy link
Collaborator

kiminuo commented Feb 16, 2023

Otherwise I think we can go with this version as it is.
WDYT?

I believe so.

@lontivero
Copy link
Collaborator Author

lontivero commented Feb 17, 2023

About renames

The concepts of coinjoin request and payment request were renamed as coinjoin notification to make it more generic. The idea is that Wasabi coordinator notifies about new ready to sign coinjoins to the affiliate servers.

Endpoint rename

The get_coinjoin_request endpoint was renamed as notify_coinjoin

Payloads change

The only change is in the request payload header where the title field sends "coinjoin notification" instead of the previous "payment request". See:

{
  "body": {  
     // Nothing changed here
  },
  "header": {
    "title": "coinjoin notification",
    "version": 1
  }
}

On the response side the field coinjoin_request was renames as affiliate_data. Example:

{ "affiliate_data":"010203040506" }

Ping @onvej-sl @kiminuo @Kukks @hynek-jina


Update: affiliationFlag was also renamed as affiliationId

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

Successfully merging this pull request may close these issues.

None yet

5 participants