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

fix-fees #63

Merged
merged 18 commits into from
Oct 26, 2020
Merged

fix-fees #63

merged 18 commits into from
Oct 26, 2020

Conversation

tromp
Copy link
Contributor

@tromp tromp commented Sep 6, 2020

@tromp
Copy link
Contributor Author

tromp commented Sep 6, 2020

Recalibrated weight based fees.

The Transaction::weight\_as\_block shall be multiplied by a base fee.
This will not be hardcoded, but configurable in grin-server.log.
The already present accept\_fee\_base parameter appears suitable for this, as
there as no reason to use different fees for relay and mempool acceptance. Its
Copy link
Member

Choose a reason for hiding this comment

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

there as no reason -> there is no reason

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

The minimum relay fee of a transaction shall be proportional to Transaction::weight\_as\_block,
which uses weights of BLOCK\_INPUT\_WEIGHT = 1, BLOCK\_OUTPUT\_WEIGHT = 21, and BLOCK\_KERNEL\_WEIGHT = 3,
which correspond to the nearest multiple of 32 bytes that it takes to serialize.
Formerly, we used Transaction::weight\_as\_block,
Copy link
Member

Choose a reason for hiding this comment

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

did you mean something else than Transaction::weight\_as\_block here? This one has weights 1, 21, 3 as mentioned above.

@phyro
Copy link
Member

phyro commented Sep 7, 2020

I like the 1 grin cent fee idea 👍

I'm not sure this is a part of this RFC, but it is connected. I wonder if the BLOCK_INPUT_WEIGHT = 1, BLOCK_OUTPUT_WEIGHT = 21, and BLOCK_KERNEL_WEIGHT = 3 numbers should be updated. Specifically the output weight. At the time of creation of an output, we don't really know whether the outputs will get spent/prunned, so in the worst case scenario we should assume they won't (bloat attack).

Doubling the BLOCK_OUTPUT_WEIGHT = 42, then we will decrease the number of txs in a block which can make the worst case scenario only ~half as bad. It also makes the attacker pay twice as much. I'm not sure that's enough to stop the spam attacks though. The issue is also that this means that it comes at a higher cost for regular users.

If we put it to some extreme e.g. we increase the output weight by 10 times so BLOCK_OUTPUT_WEIGHT=210 then the attacker needs to pay 10 times more, but maybe we can actually help the 'fair' users. If we make an input subtract -9*21 from fees, then the user will only really pay the weight of 21 per output, but it comes in the effect when the output is spent which is in a future transaction. This means that the outputs are encouraged to be spent while at the same time, you make it harder to bloat the chain. Of course the down side is that it goes to ~94 txs per block which is ~1.5 tx/s.

@tromp
Copy link
Contributor Author

tromp commented Sep 7, 2020

We don't change BLOCK_OUTPUT_WEIGHT because it must correspond to the size when serialized.
If you want to make outputs more costly, you can just raise the base fee.
If you want to further limit the number of outputs in a block, you can lower the block weight limit.
But BLOCK_OUTPUT_WEIGHT must be 21 when Bulletproofs take about 21*32 bytes.

@phyro
Copy link
Member

phyro commented Sep 7, 2020

I always wondered where the 21 came from, makes sense now. Ah yes, forgot about lowering the block weight limit scenario, I think it achieves what I described as an issue. There does seem to be a difference if inputs contribute to negative fee though because in the scheme I described you overpay the fees at creation to gain back your fees at spending. It might introduce new issues when blocks are full and the miners prefer the txs with less inputs so probably not viable in practice...

@Paouky
Copy link
Contributor

Paouky commented Sep 7, 2020

@antiochp
Copy link
Member

antiochp commented Sep 7, 2020

👍 I like the simplicity of this.

I think we have convinced ourselves that the current incentive structure around negative fees (rebates?) for inputs is ineffective at best.

@lehnberg lehnberg changed the title initial version of fees RFC fix-fees Sep 7, 2020
@lehnberg lehnberg added the node dev Related to node dev team label Sep 7, 2020
@pkariz
Copy link

pkariz commented Sep 7, 2020

Great job on the RFC 👍 If i understand correctly the 21 in 3 are based on the size they add (eg. kernel = ~100bytes, 100/32 = ~3).
2 questions:

  1. I feel like kernel should be punishable more because it creates a permanent size increase, unlike the output which is probably going to be spent in the future and therefore removed. Would it makes sense to additionally penalize the permanent data (kernels in this case)?
  2. Input removes an utxo (so i would see it as -21, but i also don't want to have negative number), so what's the logic behind having input = 1 instead of 0?

@tromp
Copy link
Contributor Author

tromp commented Sep 7, 2020

This proposal aims to take the arbitrariness out of the weights, by making them miner incentive compatible. Never spent outputs cause as much of a permanent size increase as kernels. Worse in fact, because the UTXO set needs efficient random lookup in a running node, while kernels are only accessed sequentially. Weighing them by their size seems the most fair and is the only way to make them miner incentive compatible.

  1. The logic is that BLOCK_INPUT_WEIGHT == 1 because an input takes about 32 bytes to describe (its commitment), and a unit of weight corresponds to 32 byte of size.

@DavidBurkett
Copy link
Contributor

How do we want to implement this in a backward compatible way? My suggestion:

  1. Update grin-wallet to use the new higher fees now.
  2. Nodes follow the old mempool acceptance rules until the hardfork occurs

The general idea gets a 👍from me, though I do have concerns with raising the fees too high. We must keep in mind that setting fees too high could discourage transacting, which limits our anonymity set.

Possible alternative or additional idea:
Another approach we could take to limit spam could be to limit txs (and similarly blocks) to something like 2 outputs per input, which would at least give us warning signs when a spam attack is about to happen.

A rule like that only matters while blocks are empty though. Full blocks and an active fee market is necessary to prevent spam effectively.

@tromp
Copy link
Contributor Author

tromp commented Sep 8, 2020

I thought all wallets and nodes could switch to the new minimum relay fees at HF4.
If you then build a tx just before HF4, you may find it not relayed due to insufficient fees.
That could be avoided by making wallet switch for tx building up to a day earlier.

Grin has dropped about 6x in price since early months, so worries that a 2.5x fee increase is too much seem misplaced.

Trying to limit spam by anything other than fees feels wrong to me. Arbitrary rules like limiting #outputs / # inputs would cause friction to legitimate uses as much as to spam.

@DavidBurkett
Copy link
Contributor

worries that a 2.5x fee increase is too much seem misplaced.

For standard 2 output txs, fees are currently anywhere between 0.001 (eg. https://grinscan.net/block/863452#k3) and 0.008. That same 0.001 fee transaction would now have a fee of 0.032, a 32x increase.

A worst-case 1->2 transaction had a fee of 0.008, but would now have a fee of 0.0235, a ~2.9x fee increase.

So, it's not quite as low as you were thinking. My comment was merely meant as a warning though that increasing by too much could negatively affect privacy. I'm fine with the increase, but it's just something we should be aware of.

@tromp
Copy link
Contributor Author

tromp commented Sep 8, 2020

Currently, fees can be 0. So any proposal that makes the fee always positive is an infinite increase in fees. But we're talking about an increase from 1 milligrin to 32 milligrin. So some unusual transactions go from negligible frees to quite low fees. Seems not very relevant to the discussion.

While the fees for outputs go up 2.5x, the fees for a whole tx indeed go up by some more, around 3x.
Still only compensates for the price drop by about half...

@DavidBurkett
Copy link
Contributor

Those txs are quite common, not unusual. I pulled that from the most recent block at the time I wrote that comment.

Btw, I think you're mistaken about 0 fee txs. There's a 0.001 grin minimum if I understand the logic correctly. The wallet won't use a fee less than that anyway.

Anyhow, once again, I'm not trying to block this proposal. Quite the opposite, I sincerely welcome it. I've argued since early 2019 that negative weight inputs create bad incentives, and I never liked that our mempool criteria differed from consensus weighting logic. I was just offering some points for others to think about while they review and consider this RFC and any other future changes to the base fee rate.

@DavidBurkett
Copy link
Contributor

I thought all wallets and nodes could switch to the new minimum relay fees at HF4.
If you then build a tx just before HF4, you may find it not relayed due to insufficient fees.
That could be avoided by making wallet switch for tx building up to a day earlier.

Can you document this in the RFC?

DavidBurkett
DavidBurkett previously approved these changes Sep 9, 2020
@antiochp
Copy link
Member

We currently use the full 64 bits for fee.

If we split this out (for example)-

  • 40 bits for fee
  • 8 bits for fee factor
  • 16 bits remaining

The softfork mechanism is basically signalled/specified in these 16 bits?

i.e. A softfork would propose setting a specific bit to 1 and tx invalid beyond specific block height otherwise?

Or does FEE_FACTOR_BITS come into play in subsequent softforks? Does this size change over time?

@tromp
Copy link
Contributor Author

tromp commented Sep 25, 2020

The softfork mechanism is basically signalled/specified in these 16 bits?

Yes, we'd still need to work out the specifics of signalling and activation.

i.e. A softfork would propose setting a specific bit to 1 and tx invalid beyond specific block height otherwise?

A softfork could use k bits and have various restrictions for the 2^k possible values.
It could be signaled with one of these k bits, or with yet another bit. The activation height is unknown beforehand with miner signaling.

Or does FEE_FACTOR_BITS come into play in subsequent softforks? Does this size change over time?

Normally it wouldn't. But since the fee factor restriction is outside the consensus model, one could decide to reduce FEE_FACTOR_BITS and repurpose the freed up bits.

@antiochp
Copy link
Member

I actually like the idea of stealing some of these bits to allow for a possible softfork mechanism.

I think this is a larger conversation though - we now know we can do softforks this way, the question is do we want to?
And that's not a fee question, but a softfork question.

@tromp
Copy link
Contributor Author

tromp commented Sep 29, 2020

We can decide later whether we want to. But at this time we can enable that choice. Which has no adverse effect on the fee proposal. We surely don't need more than 40 bits for the fee. And 8 bits for fee factor seems quite generous. So we either force the remaining bits to be 0, preventing a future soft-fork mechanism, or we allow arbitrary values of these bits, enabling it.

@DavidBurkett
Copy link
Contributor

A softfork could use k bits and have various restrictions for the 2^k possible values.

So we have roughly 2 bytes worth of data we can commit to for a future softfork? That doesn't seem very useful at all. It's useful for signaling a future hardfork maybe, but tx signaling isn't nearly as well studied as miner signaling, and doesn't seem nearly as safe.

If we actually want to support softforks, we should use those 2 bytes to commit to the size of the kernel so we can add arbitrary data. Otherwise, we're just wasting 2 bytes and may as well just make fee + factor a total of 6 bytes.

@tromp
Copy link
Contributor Author

tromp commented Sep 29, 2020

we should use those 2 bytes to commit to the size of the kernel so we can add arbitrary data.

That opens a new can of worms, and deserves to be in a separate RFC.
I think introduction of new kernels with additional fields warrants a hard fork;
that's exactly what we have enum KernelFeatures for.
These few bits are for cases where we wants to restrict the validity of existing kernel types with a fee field, either at the consensus or at p2p level.

@DavidBurkett
Copy link
Contributor

These few bits are for cases where we wants to restrict the validity of existing kernel types with a fee field, either at the consensus or at p2p level.

okay, I can come up with a few scenarios where we might want to have a few extra bits to limit stem-phase aggregation and such.

@tromp tromp marked this pull request as draft October 4, 2020 13:18
@antiochp
Copy link
Member

antiochp commented Oct 6, 2020

In line with our governance process, this RFC can be considered being in Final Comment Period, with a disposition to merge in two weeks time, on Oct 8.

Please ensure any comments are made before then!

We are resetting the Final Comment Period due to changes in the RFC proposal, specifically the revised fee shift details.

This RFC can be considered being in Final Comment Period, with a disposition to merge in two weeks time, on Oct 20.

@antiochp antiochp added in FCP Currently in Final Comment Period and removed in FCP Currently in Final Comment Period labels Oct 6, 2020
Copy link
Member

@antiochp antiochp left a comment

Choose a reason for hiding this comment

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

Some minor comments but 👍.
I like the fee_shift idea.

@tromp tromp marked this pull request as ready for review October 13, 2020 21:23
Copy link
Member

@antiochp antiochp left a comment

Choose a reason for hiding this comment

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

This is looking really solid for final HF.

Copy link
Contributor

@lehnberg lehnberg left a comment

Choose a reason for hiding this comment

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

Mainly reviewed from structure and formatting point of view. Content readable and looks good @tromp, see minor nitpick formatting suggestions.

Approving as I see this is a clear improvement over what we have, but cannot say whether this is the optimal approach or not.

@lehnberg lehnberg merged commit 9b3a26c into mimblewimble:master Oct 26, 2020
@lehnberg
Copy link
Contributor

In line with our governance process, this proposal has now been accepted as RFC#0017.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in FCP Currently in Final Comment Period node dev Related to node dev team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants