-
Notifications
You must be signed in to change notification settings - Fork 677
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/microblock fees #3144
Fix/microblock fees #3144
Conversation
…nt's block's evaluated epoch is 2.1, we start using the child block's reported streamed tx fees to determine the parent's reward. We need to use the parent block's evaluated epoch, since a stream cannot cross an epoch boundary.
…eward's parent block's evaluated epoch
…fee behavior, the new fee behavior (by way of the new get-block-info? fields), and the transition gating
Codecov Report
@@ Coverage Diff @@
## feat/btc-stx-exchange-rate #3144 +/- ##
==============================================================
+ Coverage 83.99% 84.25% +0.26%
==============================================================
Files 268 268
Lines 211702 212573 +871
==============================================================
+ Hits 177812 179108 +1296
+ Misses 33890 33465 -425
Continue to review full report at Codecov.
|
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.
thanks for finding this.. one comment
@@ -1015,6 +1022,7 @@ impl StacksChainState { | |||
/// Returns a list of payments to make to each address -- miners and user-support burners. | |||
pub fn find_mature_miner_rewards( | |||
clarity_tx: &mut ClarityTx, | |||
sortdb_conn: &Connection, |
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.
can unit tests for this function be used to test the miner reward calculations?
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.
They are, albeit indirectly, via test_get_block_info_v210
and test_get_block_info_v210_no_microblocks
. Each block and microblock stream has a unique set of transaction fees, and the test verifies that these fees are awarded accordingly.
…confirm that we have 0 tx fees from them stored in the MinerReward structs
On the topic of microblock fee rewards, I think there's another bug (though not a realizable one, because user burn support is disabled) in the handling of poisoned rewards: Per the comment, the supporting users should get 0 rewards, not a portion of the coinbase. |
Yup; might as well fix that too since it can't be hit in production anyway. |
…lock fee reporting (not a consensus bug since user burn supports are disabled)
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.
LGTM!
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.
lgtm
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This fixes #3140 by considering the evaluated epoch of a matured miner reward's block's parent. If the parent block was evaluated in epoch 2.1, then the correct 40/60 fee split begins to occur between the parent and child blocks' miners (as it should have been). Before this happens, the buggy behavior described in #3140 continues to happen.
Do not merge until #3142 is merged, since it depends on code in that branch.