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.

Sign up for GitHub

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

Jump to bottom

Fixfees #3481

Merged
merged 27 commits into from
Nov 26, 2020
Merged

Fixfees #3481

merged 27 commits into from
Nov 26, 2020

Conversation

tromp
Copy link
Contributor

@tromp tromp commented Oct 24, 2020

name: fixfees
about: implement fixfees RFC
title: Fix Fees
labels: consensus-breaking, must-have
assignees: @antiochp @yeastplume @quentinlesceller

@tromp tromp mentioned this pull request Oct 27, 2020
Merged
antiochp
antiochp reviewed Nov 17, 2020
View reviewed changes
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.

👍 Looks good to me.

Couple of really minor formatting comments.

One question about our bucketing/aggregation logic and how fee_shift and fee_rate interacts with this. I'm not clear how this should work when bucketing dependent transactions together.

core/src/core/transaction.rs Outdated Show resolved Hide resolved
core/src/global.rs Outdated Show resolved Hide resolved
pool/src/pool.rs Show resolved Hide resolved
@antiochp
Copy link
Member

Do we need to handle FeeFields differently per and post HF4 (via header version or height)?
I don't see anything conditional on version or height.

@tromp
Copy link
Contributor Author

tromp commented Nov 17, 2020

Do we need to handle FeeFields differently per and post HF4 (via header version or height)?
I don't see anything conditional on version or height.

Yes, FeeFields::fee() should be version dependent. I need to ponder how best to provide it with the header version.

@antiochp
Copy link
Member

antiochp commented Nov 17, 2020 •
edited
Loading

Yes, FeeFields::fee() should be version dependent. I need to ponder how best to provide it with the header version.

I'm thinking about this as well.

Edit: If we make FeeFields an enum with two different variants I think we can achieve what we want here.
That way FeeFields::fee() is variant dependent with two different sets of rules. One for "legacy" fees and another for post-HF4 fee fields.
The enum variant is determined during deserialization based on header version (block header for blocks, current chain head for transactions).

@DavidBurkett DavidBurkett mentioned this pull request Nov 19, 2020
Closed
@antiochp antiochp added this to the 5.0.0 milestone Nov 24, 2020
@lehnberg lehnberg mentioned this pull request Nov 24, 2020
Closed
@antiochp antiochp changed the title [WIP] Fixfees Fixfees Nov 24, 2020
@antiochp antiochp mentioned this pull request Nov 25, 2020
Open
5 tasks
antiochp
antiochp approved these changes Nov 25, 2020
View reviewed changes
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.

I'm 👍.
I'd like another pair of eyes on this before we merge though.

@antiochp antiochp changed the title Fixfees [DNM] Fixfees Nov 26, 2020
antiochp
antiochp approved these changes Nov 26, 2020
View reviewed changes
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.

Passing height into accept_fee() looks 👍

j01tz
j01tz approved these changes Nov 26, 2020
View reviewed changes
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 but wasn't able to spend too much time thinking through further possible corner cases and did not get a chance to manually test. Will be good to test this change extensively with the beta release. 👍

@antiochp antiochp changed the title [DNM] Fixfees Fixfees Nov 26, 2020
jaspervdm
jaspervdm reviewed Nov 26, 2020
View reviewed changes
Copy link
Contributor

@jaspervdm jaspervdm 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, I just have one question.

core/src/core/transaction.rs Show resolved Hide resolved
jaspervdm
jaspervdm approved these changes Nov 26, 2020
View reviewed changes
Copy link
Contributor

@jaspervdm jaspervdm left a comment

Choose a reason for hiding this comment

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

👍 to merge assuming CI completes succesfully.

quentinlesceller
quentinlesceller reviewed Nov 26, 2020
View reviewed changes
Copy link
Member

@quentinlesceller quentinlesceller left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Error mixed up with grin-wallet repo please ignore.

@jaspervdm jaspervdm merged commit 48efb69 into mimblewimble:master Nov 26, 2020
@antiochp antiochp mentioned this pull request Nov 26, 2020
Closed
bayk added a commit to mwcproject/mwc-node that referenced this pull request Jun 11, 2024
@bayk
grin v5.3 (0022) Fixfees (mimblewimble#3481)
566151f 
See explanation: https://github.com/mimblewimble/grin-rfcs/blob/master/text/0017-fix-fees.md

                 * add FeeFields type

                 * use FeeFields with ::zero and try_into().unwrap()

                 * fixed tests

                 * avoid 0 accept_base_fee

                 * add aggregate_fee_fields method for transaction

                 * implement std::fmt::Display trait for FeeFields

                 * make base_fee argument non-optional in libtx::mod::tx_fee

                 * add global and thread local accept_fee_base; use to simplify tests

                 * set unusually high fee base for a change

                 * revert to optional base fee argument; default coming from either grin-{server,wallet}.toml

                 * remove optional base fee argument; can be set with global::set_local_accept_fee_base instead

                 * define constant global::DEFAULT_ACCEPT_FEE_BASE and set global value

                 * add Transaction::accept_fee() method and use

                 * Manual (de)ser impl on FeeFields

                 * fix comment bug

                 * Serialize FeeFields as int in tx

                 * allow feefields: u32:into() for tests

                 * try adding height args everywhere

                 * make FeeFields shift/fee methods height dependent

                 * prior to hf4 feefield testing

                 * rename selected fee_fields back to fee for serialization compatibility

                 * fix test_fee_fields test, merge conflict, and doctest use of obsolete fee_fields

                 * make accept_fee height dependent

                 * Accept any u64 in FeeFields deser
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@quentinlesceller quentinlesceller quentinlesceller approved these changes

@j01tz j01tz j01tz approved these changes

@antiochp antiochp antiochp approved these changes

@jaspervdm jaspervdm jaspervdm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@tromp @antiochp @jaspervdm @quentinlesceller @j01tz