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 past fees #77

Merged
merged 5 commits into from
Apr 27, 2021
Merged

Fix past fees #77

merged 5 commits into from
Apr 27, 2021

Conversation

tromp
Copy link
Contributor

@tromp tromp commented Jan 24, 2021

@tromp tromp changed the title extend 40-bit fee restriction to entire past Fix past fees Jan 24, 2021
@DavidBurkett
Copy link
Contributor

@tromp Have you confirmed nobody has used a fee that large in the past? If you have, then I see no reason we can't get this accepted.

[community-level-explanation]: #community-level-explanation

The largest fee to appear on mainnet prior to Hark Fork 4 is 2.404 Grin. This means that extending the current 40-bit fee restriction to the pre-HF4 history preserves validity.
While this is technically a hard-forking change, it would only have any visible
Copy link
Member

Choose a reason for hiding this comment

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

this sentence "it would only have any visible" is unfinished

@phyro
Copy link
Member

phyro commented Apr 4, 2021

Should we move it to FCP and get it accepted ASAP so that we can also tackle the PR that implements this?

@j01tz
Copy link
Member

j01tz commented Apr 6, 2021

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 April 20.

Please ensure any comments are made before then!

Copy link
Member

@quentinlesceller quentinlesceller left a comment

Choose a reason for hiding this comment

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

Small format comments. Overall 👍

[summary]: #summary

Carry the restriction of fees, to 40 bits since HF4, back to all history.
Do not change headerversion despite the hard-forking nature.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Do not change headerversion despite the hard-forking nature.
Do not change `headerversion` despite the hard-forking nature.

or

Suggested change
Do not change headerversion despite the hard-forking nature.
Do not change header version despite the hard-forking nature.

## Motivation
[motivation]: #motivation

This makes the FeeFields methods fee() and fee\_shift() height independent,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This makes the FeeFields methods fee() and fee\_shift() height independent,
This makes the `FeeFields` methods `fee()` and `fee_shift()` height independent,

## References
[references]: #references

[1] [Grin github](https://github.com/mimblewimble/grin/blob/acba73bf40242f963d8ea1e7128dfdfde6fb8853/core/src/core/transaction.rs#L181-L188)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a different link name?

Copy link
Member

@j01tz j01tz left a comment

Choose a reason for hiding this comment

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

Looks good overall, few small typos

This makes the `FeeFields` methods `fee()` and `fee\_shift()` height independent,
as well as several other functions which end up calling them.
This results in nontrivial code and consensus model simplification,
with no downsise, which is always a good thing.
Copy link
Member

Choose a reason for hiding this comment

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

Should "downsise" -> "downside"?

## Motivation
[motivation]: #motivation

This makes the `FeeFields` methods `fee()` and `fee\_shift()` height independent,
Copy link
Member

Choose a reason for hiding this comment

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

Should fee\_shift() -> fee_shift()?

update README
@antiochp antiochp merged commit 709606d into mimblewimble:master Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants