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

feat: proposer reward #922

Merged

Conversation

@zhangsoledad
Copy link
Member

commented May 31, 2019

Break Changes

consensus

  1. earliest transaction proposer get 40% of the transaction fee as a reward.
  2. block reward finalized after proposal window close.
  3. enforce one-input one-output one-witness on cellbase.

database

  1. block_ext.txs_verified -> block_ext.verified
  2. txs_fees persistence

Enhancement

  1. lazy static always success script for test
  2. introduce AsCapacity make Capacity interface more flexible
@nervos-bot

This comment has been minimized.

Copy link

commented May 31, 2019

@yangby-cryptape is assigned as the chief reviewer

@nervos-bot

This comment was marked as outdated.

Copy link

commented May 31, 2019

CI status of the fork branch is Build Status

Show resolved Hide resolved spec/src/lib.rs Outdated

@zhangsoledad zhangsoledad force-pushed the zhangsoledad:zhangsoledad/proposer_reward branch 7 times, most recently from be4904d to e60b072 May 31, 2019

@zhangsoledad zhangsoledad marked this pull request as ready for review Jun 4, 2019

@zhangsoledad zhangsoledad requested a review from nervosnetwork/ckb-code-review as a code owner Jun 4, 2019

@zhangsoledad zhangsoledad force-pushed the zhangsoledad:zhangsoledad/proposer_reward branch from efd2d29 to 81154f1 Jun 4, 2019

@zhangsoledad zhangsoledad force-pushed the zhangsoledad:zhangsoledad/proposer_reward branch 3 times, most recently from 23fad2e to 5f0e81a Jun 4, 2019

@codecov

This comment was marked as outdated.

Copy link

commented Jun 5, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (develop@8c6bbce). Click here to learn what that means.
The diff coverage is 76.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##             develop    #922   +/-   ##
=========================================
  Coverage           ?   58.4%           
=========================================
  Files              ?     174           
  Lines              ?   17597           
  Branches           ?       0           
=========================================
  Hits               ?   10277           
  Misses             ?    7320           
  Partials           ?       0
Impacted Files Coverage Δ
core/src/extras.rs 90.69% <ø> (ø)
test/src/specs/mining/size_limit.rs 0% <0%> (ø)
core/src/block.rs 85.03% <0%> (ø)
test/src/specs/mining/foundation.rs 0% <0%> (ø)
verification/src/tests/dummy.rs 0% <0%> (ø)
chain/src/tests/util.rs 100% <100%> (ø)
shared/tx-pool-executor/src/tx_pool_executor.rs 87.95% <100%> (ø)
chain/src/chain.rs 70.6% <100%> (ø)
chain/src/tests/reward.rs 100% <100%> (ø)
core/src/header.rs 70.55% <100%> (ø)
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c6bbce...d9ceb2d. Read the comment docs.

@zhangsoledad zhangsoledad force-pushed the zhangsoledad:zhangsoledad/proposer_reward branch 2 times, most recently from 4f2fe6c to 7215c33 Jun 6, 2019

Show resolved Hide resolved spec/src/lib.rs Outdated
@yangby-cryptape
Copy link
Contributor

left a comment

  • My knowledge, though not too much, but I spent a lot of time on this PR, and I didn't find any critical and obvious bugs.

  • Be limited by poor knowledge about this feature, I think there is still need at least one more folks who know this knowledge well to review again.

    Since I learned this feature from this PR, I think I can not distinguish some mistakes in details.

@zhangsoledad zhangsoledad force-pushed the zhangsoledad:zhangsoledad/proposer_reward branch 2 times, most recently from b0f16db to d9ceb2d Jun 6, 2019

@doitian doitian self-assigned this Jun 10, 2019

@doitian doitian self-requested a review Jun 10, 2019

@doitian

This comment was marked as resolved.

Copy link
Member

commented Jun 10, 2019

Plan to review RewardCalculator tomorrow morning. Need more focus.

Show resolved Hide resolved util/reward-calculator/src/lib.rs Outdated
Show resolved Hide resolved util/reward-calculator/src/lib.rs
Show resolved Hide resolved util/reward-calculator/src/lib.rs Outdated
.map_err(Into::into)
}

pub fn proposal_reward(

This comment has been minimized.

Copy link
@doitian

doitian Jun 11, 2019

Member

Some notes for other reviewers:

The proposal window is (2, 10).

  • target block is t
  • current block is t + 11
  • parent block is t + 10
  • commit_start is t + 2, which is the earliest block which can commit txs in target block's proposals.
  • proposal_start is t - 8, which is the earliest block which proposals are competing with the target block.
let commit_start = cmp::max(block_number.saturating_sub(proposal_window.length()), 2);
let proposal_start = cmp::max(commit_start.saturating_sub(proposal_window.start()), 1);

let mut proposal_table = BTreeMap::new();

This comment has been minimized.

Copy link
@doitian

doitian Jun 11, 2019

Member

I propose a simpler algorithm below.

We only need to keep a single HashSet, which tracks the proposal ids that have already been proposed in previous blocks. This can avoid repeat 8 flat_maps.

We name the HashSet as proposed, which stores txs already proposed before the target block.

The loop starts from parent block, backward to commit_start. For each block bn:

  • Find proposals of bn - 10, add the proposals to proposed unless bn - 10 == target.
  • For each tx in block bn, if tx is not in proposed but in target's proposal, sum the proposal reward.

This comment has been minimized.

Copy link
@doitian

doitian Jun 11, 2019

Member

Alternatively, the proposed ids can be removed from target_proposals, no need to maintain another hash set.

@doitian

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

The whole PR looks good to me, the only change I requested is:

(4, 10) should be defined as consensus parameters or constants.

The others are just refactoring suggestions, they have lower priority and we can improve them later.

Show resolved Hide resolved spec/src/lib.rs Outdated

@zhangsoledad zhangsoledad force-pushed the zhangsoledad:zhangsoledad/proposer_reward branch 3 times, most recently from 7cc291e to d3fa274 Jun 11, 2019

@doitian

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

This diagram demonstrates how the reward is finalized.

proposal_rewards

PDF Version

Show resolved Hide resolved spec/src/consensus.rs Outdated

@zhangsoledad zhangsoledad force-pushed the zhangsoledad:zhangsoledad/proposer_reward branch from 48eba6d to f154714 Jun 12, 2019

@zhangsoledad zhangsoledad requested a review from doitian Jun 12, 2019

zhangsoledad added some commits May 30, 2019

@zhangsoledad zhangsoledad force-pushed the zhangsoledad:zhangsoledad/proposer_reward branch from f154714 to 1687674 Jun 12, 2019

@zhangsoledad zhangsoledad force-pushed the zhangsoledad:zhangsoledad/proposer_reward branch from 5efb0e2 to 7c593f7 Jun 12, 2019

@zhangsoledad zhangsoledad merged commit 0a72d86 into nervosnetwork:develop Jun 12, 2019

7 checks passed

Nervos CI Build Passed via Travis
Details
Nervos Integration Build Passed via Travis
Details
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
nervosnetwork.ckb Build #20190612.39 succeeded
Details
nervosnetwork.ckb (IntegrationTest) IntegrationTest succeeded
Details
nervosnetwork.ckb (UnitTest) UnitTest succeeded
Details

@zhangsoledad zhangsoledad deleted the zhangsoledad:zhangsoledad/proposer_reward branch Jun 12, 2019

This was referenced Jun 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.