Use WabiSabi package #10126

lontivero · 2023-02-16T23:49:36Z

Remove all the code for WabiSabi and use the nuget package.

molnard · 2023-03-21T13:46:25Z

This is the next step before the daemon.

Finish WabiSabi library distribution to NuGet https://github.com/zkSNACKs/NWabiSabi
Fix this PR

molnard

I thought it will be just adding usings everywhere and code removal. Something is going on here. Why new files were required?

molnard · 2023-03-24T10:49:01Z

WalletWasabi/JsonConverters/ReflectionUtils.cs

+
+public static class ReflectionUtils
+{
+	public static T? CreateInstance<T>(object[] args) =>


What is this?

molnard · 2023-03-24T10:51:20Z

CF issues...

lontivero · 2023-03-24T16:58:09Z

The new files are JsonSerializers. We need to serialize/deserialize the messages which have inaccessible constructors and that were using JsonConstructor for deserialization. But now those types are not accessible anymore because they are in a different project so, we have to create the serializers.

yahiheb · 2023-03-25T23:54:33Z

CF issues...

0aa36f7 Fixed the CodeFactor issues.
c67e6e7 Renamed a variable.
1d64be2 Fixed deepsource issues, but there is only one left.

molnard · 2023-03-27T10:53:35Z

we have to create the serializers

Will the consumer of the library, need to do the same?

inaccessible constructors

What if we make those accessible?

lontivero · 2023-03-27T11:24:51Z

Will the consumer of the library, need to do the same?

Yes ofc. How could we know how they want to serialize these things?

What if we make those accessible?

In that case we could avoid the usage of reflection (good) at the cost of ruining the library API (bad). I would prefer to keep the API well design as it is.

MaxHillebrand

tested 1d64be2

after input registration started, but coinjoin is successful, but txid not available

2023-03-27 13:55:55.793 [24] WARNING	CoinJoinClient.CreateRegisterAndConfirmCoinsAsync (452)	Newtonsoft.Json.JsonException: Property 'T' was expected.
   at WalletWasabi.WabiSabi.Crypto.Serialization.JsonReaderExtensions.ReadProperty[T](JsonReader reader, JsonSerializer serializer, String name) in WalletWasabi/WabiSabi/Crypto/Serialization/JsonReaderExtensions.cs:line 19
   at WalletWasabi.WabiSabi.Crypto.Serialization.MacJsonConverter.ReadJson(JsonReader reader, Type objectType, MAC existingValue, Boolean hasExistingValue, JsonSerializer serializer) in WalletWasabi/WabiSabi/Crypto/Serialization/MacConverter.cs:line 16
   at Newtonsoft.Json.JsonConverter`1.ReadJson(JsonReader reader, Type objectType, Object existingValue, JsonSerializer serializer)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.DeserializeConvertable(JsonConverter converter, JsonReader reader, Type objectType, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.PopulateList(IList list, JsonReader reader, JsonArrayContract contract, JsonProperty containerProperty, String id)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateList(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, Object existingValue, String id)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateValueInternal(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.ResolvePropertyAndCreatorValues(JsonObjectContract contract, JsonProperty containerProperty, JsonReader reader, Type objectType)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObjectUsingCreatorWithParameters(JsonReader reader, JsonObjectContract contract, JsonProperty containerProperty, ObjectConstructor`1 creator, String id)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObject(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateValueInternal(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.ResolvePropertyAndCreatorValues(JsonObjectContract contract, JsonProperty containerProperty, JsonReader reader, Type objectType)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObjectUsingCreatorWithParameters(JsonReader reader, JsonObjectContract contract, JsonProperty containerProperty, ObjectConstructor`1 creator, String id)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObject(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateValueInternal(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.Deserialize(JsonReader reader, Type objectType, Boolean checkAdditionalContent)
   at Newtonsoft.Json.JsonSerializer.DeserializeInternal(JsonReader reader, Type objectType)
   at Newtonsoft.Json.JsonConvert.DeserializeObject(String value, Type type, JsonSerializerSettings settings)
   at Newtonsoft.Json.JsonConvert.DeserializeObject[T](String value, JsonSerializerSettings settings)
   at WalletWasabi.WabiSabi.Client.WabiSabiHttpApiClient.Deserialize[TResponse](String jsonString) in WalletWasabi/WabiSabi/Client/WabiSabiHttpApiClient.cs:line 151
   at WalletWasabi.WabiSabi.Client.WabiSabiHttpApiClient.SendAndReceiveAsync[TRequest,TResponse](RemoteAction action, TRequest request, CancellationToken cancellationToken, Nullable`1 retryTimeout) in WalletWasabi/WabiSabi/Client/WabiSabiHttpApiClient.cs:line 141
   at WalletWasabi.WabiSabi.Client.ArenaClient.RegisterInputAsync(uint256 roundId, OutPoint outPoint, OwnershipProof ownershipProof, CancellationToken cancellationToken) in WalletWasabi/WabiSabi/Client/ArenaClient.cs:line 45
   at WalletWasabi.WabiSabi.Client.AliceClient.RegisterInputAsync(RoundState roundState, ArenaClient arenaClient, SmartCoin coin, IKeyChain keyChain, CancellationToken cancellationToken) in WalletWasabi/WabiSabi/Client/AliceClient.cs:line 101
   at WalletWasabi.WabiSabi.Client.AliceClient.CreateRegisterAndConfirmInputAsync(RoundState roundState, ArenaClient arenaClient, SmartCoin coin, IKeyChain keyChain, RoundStateUpdater roundStatusUpdater, CancellationToken unregisterCancellationToken, CancellationToken registrationCancellationToken, CancellationToken confirmationCancellationToken) in WalletWasabi/WabiSabi/Client/AliceClient.cs:line 73
   at WalletWasabi.WabiSabi.Client.CoinJoinClient.<>c__DisplayClass55_0.<<CreateRegisterAndConfirmCoinsAsync>g__RegisterInputAsync|0>d.MoveNext() in WalletWasabi/WabiSabi/Client/CoinJoinClient.cs:line 381

2023-03-27 13:59:39.038 [45] INFO	CoinJoinClient.StartRoundAsync (268)	Round (02856becc95a6be8e3d6edb7146a6f9e5b9c4e627d4915028051f17a90618812): Broadcasted. Coinjoin TxId: (Not available)

lontivero · 2023-03-27T16:02:40Z

@MaxHillebrand you can try again if you want. This time it will work.

MaxHillebrand

tACK 55dc44e

coinjoin works without smells in the logs.

molnard

Reflection is slow and we tried to avoid it. Using it every time when we are deserializing an object is not OK.

Can you find any other option?
How others are doing these kinds of things?

molnard

cACK. Before I can merge this I need these kinds of tests for every related converter. It is OK if you check them in combination as I did here.

#10393

kiminuo · 2023-03-30T15:46:02Z

WalletWasabi/WabiSabi/Models/OutputRegistrationRequest.cs

 using Microsoft.AspNetCore.Mvc.ModelBinding.Validation;
+using WabiSabi.CredentialRequesting;


I wonder if it would make sense to you to have it like this:

Suggested change

using WabiSabi.CredentialRequesting;

using ZkSnacks.WabiSabi.CredentialRequesting;

Putting the organization in the namespace is quite common. I'm not totally sure if I like it or not but it would make it clear who is behind that package and if we ever have more packages we would just have ZkSnacks.SomethingElse.

I don't have a strong opinion on this but maybe somebody has.

Also there is https://github.com/zkSNACKs/NWabiSabi but the namespace is WabiSabi. Should it be NWabisabi?

No. I called the repo NWabiSabi because there is another repo called WabiSabi that contains the specs, the wabisabi white paper and the research from Yuval.

ok

I think that https://github.com/zkSNACKs/NWabiSabi should be a pinned repo here on Github given its importance.

lontivero · 2023-03-30T15:56:01Z

The new converters here are for those types that were moved to the WabiSabi package which represent cryptographic elements like zkproofs, pedersen commitments, key material and similar. This means that it would be enough to have one single bit different to make coinjoins impossible. In other words, if you can participate successfully in a coinjoin that means that the serialization/deserialization works okay.

Testing is the most important verification for this kind of PRs while UTs for compatibility are a bad idea. For example, your test in #10393 verifies that the serializer still does it as before but for this version that is not only insufficient but also wrong because the serializer is now more strict and doesn't accept whatever the server sends.

In the current version, the server uses ~~snake~~ camel casing while in your test the encoded json uses pascal casing so, that means that the expected json is one that the server would have never sent. That can create a false sense of confidence, what I think is not good.

kiminuo · 2023-03-30T16:03:45Z

WalletWasabi/Tor/Control/TorControlClientFactory.cs

@@ -28,7 +28,7 @@ public class TorControlClientFactory

 	public TorControlClientFactory(IRandom? random = null)
 	{
-		Random = random ?? InsecureRandom.Instance;
+		Random = random ?? new UnsecureRandom();


I'm confused about this. Where is UnsecureRandom defined?

I think that unsecure is not an English word. Insecure or Unsecured (with the d at the end) are imho English words.

it was the easiest thing to do because InsecureRandom is now in WabiSabi package and it didn't sound good to tight Tor code to WabiSabi. Anyway, we can use it anyway, the only thing that we have to do is to replace the IRandom by a WasabiRandom and that should be pretty much all.

WalletWasabi/Helpers/Guard.cs

WalletWasabi/Crypto/StrobeProtocol/StrobeHasher.cs

WalletWasabi/Crypto/Extensions.cs

molnard · 2023-03-31T14:47:24Z

In the current version, the server uses ~~snake~~ camel casing while in your test the encoded json uses pascal casing so, that means that the expected json is one that the server would have never sent.

And after we deploy this PR to the backend?

Testing is the most important verification for this kind of PRs while UTs for compatibility are a bad idea.

Trust the compatibility check to manual testing? The stakes are very high here. In SerializationTests.cs I see hex-converted data used for testing. Why that was added before?

molnard

I did a CJ with this PR - it was working.

lontivero · 2023-03-31T15:41:32Z

Ok. I will deploy it in testing now and test it with old and new client.

lontivero · 2023-03-31T16:39:24Z

Ok. It didn't work. Let me review everything again.

kiminuo · 2023-03-31T17:32:20Z

(I was thinking about this and it feels like it would be easier to review this PR if we just created a new project in this repo to see all changes immediately (mainly to see ALL changes) as a git diff and then as a second step, we would create the nuget package. I would feel much more confident in the process and less afraid of bugs.)

lontivero · 2023-03-31T17:42:27Z

It's done. Ive created the compatibility test and verified this works on all combinations of client versions and coordinator versions. I've undone the stricter rule that i introduced and went back to "case insensitive" deserializers as NewtonSoft does.

lontivero · 2023-03-31T17:46:02Z

The CI is compiling a commit that is not part of this PR. wtf.

…soft does

molnard

tACK

pull-request-size bot added the size/XXL label Feb 16, 2023

lontivero force-pushed the use-wabisabi-package branch from 52ad69d to 7ba48cf Compare February 17, 2023 00:09

lontivero force-pushed the use-wabisabi-package branch from cf9a4e1 to aaa5154 Compare March 24, 2023 04:54

molnard requested changes Mar 24, 2023

View reviewed changes

MaxHillebrand requested changes Mar 27, 2023

View reviewed changes

MaxHillebrand previously approved these changes Mar 27, 2023

View reviewed changes

molnard requested changes Mar 28, 2023

View reviewed changes

molnard requested changes Mar 30, 2023

View reviewed changes

kiminuo reviewed Mar 30, 2023

View reviewed changes

WalletWasabi/Helpers/Guard.cs Show resolved Hide resolved

kiminuo reviewed Mar 30, 2023

View reviewed changes

WalletWasabi/Crypto/StrobeProtocol/StrobeHasher.cs Outdated Show resolved Hide resolved

kiminuo reviewed Mar 30, 2023

View reviewed changes

WalletWasabi/Crypto/Extensions.cs Show resolved Hide resolved

lontivero dismissed MaxHillebrand’s stale review via 1f8ba0c March 30, 2023 16:57

lontivero force-pushed the use-wabisabi-package branch 2 times, most recently from 1f8ba0c to c3df6a6 Compare March 30, 2023 17:36

molnard mentioned this pull request Mar 31, 2023

Add tests for serializations #10393

Closed

molnard previously approved these changes Mar 31, 2023

View reviewed changes

lontivero marked this pull request as draft March 31, 2023 16:39

lontivero dismissed molnard’s stale review via 8f94312 March 31, 2023 17:09

lontivero marked this pull request as ready for review March 31, 2023 17:39

lontivero mentioned this pull request Mar 31, 2023

Use wabisabi package #10409

Closed

lontivero and others added 9 commits March 31, 2023 15:01

Use WabiSabi package

2057c66

Clean JsonConverters

ec9fa86

Update WabiSabi reference

40a060d

Fix CodeFactor issues

1a45b02

Fix naming rule violation

a6eb745

Fix some deepsource issues

f4ff552

Remove unused code

ca17e17

Add compatibility test and case insensitive deserialization as newton…

6540c6e

…soft does

Update CoinJoinCoinSelector.cs

f7d831f

lontivero force-pushed the use-wabisabi-package branch from 683f589 to f7d831f Compare March 31, 2023 18:05

lontivero changed the title ~~[WIP] Use WabiSabi package~~ Use WabiSabi package Mar 31, 2023

molnard approved these changes Mar 31, 2023

View reviewed changes

lontivero merged commit 90d3aaa into zkSNACKs:master Mar 31, 2023
5 of 7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use WabiSabi package #10126

Use WabiSabi package #10126

lontivero commented Feb 16, 2023

molnard commented Mar 21, 2023 •

edited

molnard left a comment

molnard Mar 24, 2023

molnard commented Mar 24, 2023

lontivero commented Mar 24, 2023

yahiheb commented Mar 25, 2023

molnard commented Mar 27, 2023

lontivero commented Mar 27, 2023

MaxHillebrand left a comment •

edited

lontivero commented Mar 27, 2023

MaxHillebrand left a comment

molnard left a comment

molnard left a comment

kiminuo Mar 30, 2023

kiminuo Mar 30, 2023

lontivero Mar 30, 2023

kiminuo Mar 31, 2023

lontivero commented Mar 30, 2023 •

edited

kiminuo Mar 30, 2023

lontivero Mar 30, 2023

molnard commented Mar 31, 2023

molnard left a comment

lontivero commented Mar 31, 2023

lontivero commented Mar 31, 2023

kiminuo commented Mar 31, 2023

lontivero commented Mar 31, 2023

lontivero commented Mar 31, 2023

molnard left a comment

		using Microsoft.AspNetCore.Mvc.ModelBinding.Validation;
		using WabiSabi.CredentialRequesting;

	using WabiSabi.CredentialRequesting;
	using ZkSnacks.WabiSabi.CredentialRequesting;

Use WabiSabi package #10126

Use WabiSabi package #10126

Conversation

lontivero commented Feb 16, 2023

molnard commented Mar 21, 2023 • edited

molnard left a comment

Choose a reason for hiding this comment

molnard Mar 24, 2023

Choose a reason for hiding this comment

molnard commented Mar 24, 2023

lontivero commented Mar 24, 2023

yahiheb commented Mar 25, 2023

molnard commented Mar 27, 2023

lontivero commented Mar 27, 2023

MaxHillebrand left a comment • edited

Choose a reason for hiding this comment

lontivero commented Mar 27, 2023

MaxHillebrand left a comment

Choose a reason for hiding this comment

molnard left a comment

Choose a reason for hiding this comment

molnard left a comment

Choose a reason for hiding this comment

kiminuo Mar 30, 2023

Choose a reason for hiding this comment

kiminuo Mar 30, 2023

Choose a reason for hiding this comment

lontivero Mar 30, 2023

Choose a reason for hiding this comment

kiminuo Mar 31, 2023

Choose a reason for hiding this comment

lontivero commented Mar 30, 2023 • edited

kiminuo Mar 30, 2023

Choose a reason for hiding this comment

lontivero Mar 30, 2023

Choose a reason for hiding this comment

molnard commented Mar 31, 2023

molnard left a comment

Choose a reason for hiding this comment

lontivero commented Mar 31, 2023

lontivero commented Mar 31, 2023

kiminuo commented Mar 31, 2023

lontivero commented Mar 31, 2023

lontivero commented Mar 31, 2023

molnard left a comment

Choose a reason for hiding this comment

molnard commented Mar 21, 2023 •

edited

MaxHillebrand left a comment •

edited

lontivero commented Mar 30, 2023 •

edited