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

[FireProofing] Don't allow to burn more than 30% of coins effective value in CJs #10739

Closed
wants to merge 6 commits into from

Conversation

adamPetho
Copy link
Collaborator

First action item we could think of for fixing #10675

Idea:

Client should not register small coins that are loosing more than 30% of their effective values.
This check is only useful in high fee enviroment.

0.3 was from the top of my head. Feel free to suggest other values.

MaxHillebrand
MaxHillebrand previously approved these changes May 19, 2023
Copy link
Collaborator

@MaxHillebrand MaxHillebrand left a comment

Choose a reason for hiding this comment

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

cACK

Copy link
Collaborator

@turbolay turbolay left a comment

Choose a reason for hiding this comment

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

The condition is the inverse of what it should be.

Co-authored-by: Turbolay <turbolay@zksnacks.com>
@BTCparadigm
Copy link
Collaborator

BTCparadigm commented May 20, 2023

I had a call about this with @turbolay and I understand the idea of trying to improve the situation for both input and output side at the same time, but I believe they should be considered separately, because different considerations apply.

The implementation in this PR considers the script type for calculating the minimum, which would mean that Segwit v0 and Taproot inputs of X denomination would be allowed in at different fee rates. This brings unnecessary complications compared to using the most conservative (Segwit v0 input) basis of the 30% calculation for the minimum output amount.

I believe @turbolay said he would create a PR for just the output side fix now, as its more urgent.
Let's think of input side sanity check improvements as a different but related thing, which sould be incuded for this same release.

@BTCparadigm
Copy link
Collaborator

It seems that I misunderstood, that this PR would be applying the same 30% logic to both input and output side.
I still wonder if script-type agnostic approach for the input side calculation would be better, to avoid opening the liquidity of certain denomination at different times for different script types.

Calculations

If you use only Taproot Input cost as the basis for the 30%:
image

If you use only SegWit v0 Input cost as the basis for the 30%:
image

If the numbers suggested in #10675 would get accepted for the output side, and this PR would be merged to mitigate the input side, it should quarantee that outputs coming from round 1 would be allowed to remix in round 2, assuming fee rate stays the same or go down. But there's a problem if fees rise, as the first rounds output would not be able to participate in the round 2.
This is why I started considering the implications of "tipping the scale" a bit.

@lontivero lontivero added the affiliate Could affect affiliates compatibility label May 24, 2023
@nopara73
Copy link
Contributor

nopara73 commented May 31, 2023

To limit minimum input amount to be registered because of network congestion is misguided: if the input can pay the network fees for itself, then the input is good. The key metric to consider here is the total registrable amount.

You're falling into a mental trap by fixating on a single input, let's say 5000 satoshi. Then you postulate that the input creation fee is 4999 satoshi. This leaves you with a 1 satoshi credential, leading you to believe the system is nonsensical. In this specific scenario, you're correct, it is nonsensical. But not due to the individual input amounts, it's because the total registrable amount is only 5000 satoshi.

However, if your total registrable amount was 1 BTC, then registering a 5000 sat input at a 4999 sat input creation fee to receive a 1 credential is entirely rational for you!!!

@nopara73 nopara73 closed this May 31, 2023
@adamPetho adamPetho removed the affiliate Could affect affiliates compatibility label May 31, 2023
@turbolay
Copy link
Collaborator

However, if your total registrable amount was 1 BTC, then registering a 5000 sat input at a 4999 sat input creation fee to receive a 1 credential is entirely rational for you!!!

It still doesn't seem rational for me. And I don't see the relation between total amount and cost of registering one.

@nopara73
Copy link
Contributor

nopara73 commented Jun 1, 2023

In the coinjoin you gained 1 sat. Now consider the opportunity cost. What would happen with the 5000 UTXO if you don't spend it in a coinjoin?

  1. Never spent: you lost 1 sat.
  2. Spent cheaper: you gained X sat.
  3. Spent more expensive: you lost Y sat.

Since coinjoins tend to have lower time preference than everyday transactions: Y > X.

@adamPetho
Copy link
Collaborator Author

adamPetho commented Jun 1, 2023

A change like this in IsEconomicallyFeasible method would make sense?
Edit: No, not at all. This is stupid.

private static bool IsEconomicallyFeasible<TCoin>(TCoin coin, UtxoSelectionParameters parameters)
	where TCoin : ISmartCoin
	=> coin.EffectiveValue(parameters.MiningFeeRate).Satoshi > (parameters.AllowedOutputAmounts.Min.Satoshi + Math.Max(ScriptType.P2WPKH.EstimateOutputVsize(), ScriptType.Taproot.EstimateOutputVsize()) * parameters.MiningFeeRate);

I'm aiming to implement the first bullet point of this comment.

WDYT?

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

Successfully merging this pull request may close these issues.

None yet

6 participants