-
Notifications
You must be signed in to change notification settings - Fork 492
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
Select coins for round abstraction #9123
Select coins for round abstraction #9123
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could extract all the coins selection code to some class (lets call it: CoinSelector
) which implements and ICoinSelector
and implement your own if you want, that would give you a lot more power and flexibility.
|
||
namespace WalletWasabi.Blockchain.TransactionOutputs; | ||
|
||
public interface ISmartCoin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an ICoin
+ AnonymitySet. I mean, a NBitcoin::ICoin
already has and TxOut
(Amount, ScriptPubKey) and an OutPoint
(TransactionId, Index). The only think tit doesn't have is an "anonymityset" (that shoulc be called AnonymityScore because we are abandoming the concept)
Btw, ScriptType
is not really necessary because it can be extracted from the scriptPubKey.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an ICoin + AnonymitySet. I mean, a NBitcoin::ICoin already has and TxOut (Amount, ScriptPubKey) and an OutPoint (TransactionId, Index).
It is not, because (as you already said yourself) ICoin
+ AnonymitySet contains ScriptPubKey
instead of ScriptType
. The ScriptPubKey
itself is not needed in the coin selection algorithm.
@@ -472,11 +475,17 @@ public static FeeRate GetSanityFeeRate(this MemPoolInfo me) | |||
output.Value + feeRate.GetFee(output.ScriptPubKey.EstimateOutputVsize()); | |||
|
|||
public static Money EffectiveValue(this Coin coin, FeeRate feeRate, CoordinationFeeRate coordinationFeeRate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we could receive an ICoin
instead of a Coin
because in that way nothing needs to be changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 95ebc9a.
@@ -66,6 +66,7 @@ public record RoundParameters | |||
public TimeSpan TransactionSigningTimeout { get; init; } | |||
public TimeSpan BlameInputRegistrationTimeout { get; init; } | |||
|
|||
public ImmutableSortedSet<ScriptType> AllowedInputScriptTypes => AllowedInputTypes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in c49b791.
@@ -739,7 +740,7 @@ private void LogCoinJoinSummary(ImmutableArray<AliceClient> registeredAliceClien | |||
|
|||
var sw2 = Stopwatch.StartNew(); | |||
foreach (var group in orderedAllowedCoins | |||
.Except(new[] { coin }) | |||
.Except(new TCoin[] { coin }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assumes TCoin
is Equatable when it is not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in fc098d6.
@@ -6,7 +6,7 @@ | |||
|
|||
namespace WalletWasabi.WabiSabi.Backend.Rounds; | |||
|
|||
public record RoundParameters | |||
public record RoundParameters : IUtxoSelectionParameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please try hard to avoid adding interfaces (we almost never use them). If you want you can add a new UtxoSelectionParameters
containing the required properties and that can be build from a round parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 64f4744.
This is definitely a good idea. I will probably do it in a following pull request. |
It was not possible for me to re-take the work on this so, I think the best would be to go with this version as it is and then I will go back to it when I find the time. Please fix the conflict and I will merge it. |
307aa54
to
2bf98f4
Compare
@lontivero Is there anything I can to get this PR merged? |
Yes, please answer my questions about the nullability. Is there any problem changing that? Where the is a |
What questions do you mean? I cannot see any questions. |
{ | ||
var netFee = feeRate.GetFee(coin.ScriptPubKey.EstimateInputVsize()); | ||
var coordFee = coordinationFeeRate.GetFee(coin.Amount); | ||
var netFee = feeRate.GetFee(scriptType.EstimateInputVsize()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if scriptType
is null? Why is it nullable?
scriptPubKey.TryGetScriptType() switch | ||
scriptPubKey.TryGetScriptType().EstimateInputVsize(); | ||
|
||
public static int EstimateInputVsize(this ScriptType? scriptType) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be nullable.
Sorry, they were "pending". |
Please see 9db433e. Do you think it solves the issue? |
@@ -522,6 +534,6 @@ public static byte[] ExtractKeyId(this Script scriptPubKey) | |||
} | |||
} | |||
|
|||
return null; | |||
throw new NotImplementedException($"Unsupported script type."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be NotSupportedException
but I can see this is not the only place this exception is used so, it's okay, we have to change it everywhere.
The content is mergeable as it is, just fix solve the conflict and we are ready. |
9db433e
to
729efd1
Compare
Done.
|
This PR broke wallet load.
@kiminuo is working on to fix, if he fails to do it quickly, then this PR gets reverted and need to be sorted out later. |
No need to revert. #9615 fixes it. |
This PR allows for implementation of Wabisabi client library which will be used by Trezor.