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

[GH-736] Beneficiary reward calculation #798

Merged
merged 91 commits into from Dec 12, 2018

Conversation

@d-velev
Copy link
Member

d-velev commented Nov 22, 2018

closes #736
Each generation's fees are stored in a cache, whenever the block that applies the reward for that generation is processed, that cache entry is cleared. The cache should also be persisted but I'll leave that for the persistence issue.

grzegorz225 and others added some commits Aug 10, 2018

Merge pull request #779 from aeternity/GH-778
[GH-778] Add dependencies to release notes
Merge pull request #781 from aeternity/GH-731
[GH-731] Adjusted base58 encoding/decoding delimiter
[GH-755] Adjusted ChannelStateOnChain structure
- adjusted serialization
- adjusted mutal close check
- added identifier value list encoders

Artur64 and others added some commits Nov 26, 2018

Merge pull request #799 from aeternity/GH-758
[GH-758] ChannelMutalClose + ChannelSettle adjustments
Merge pull request #800 from aeternity/GH-795
[GH-795] Added response TTL to oracle_response_tx
@thepiwo

This comment has been minimized.

Copy link
Collaborator

thepiwo commented on docs/release-notes/release-notes-0.2.0.md in fd0b492 Nov 26, 2018

no need to show the function, just say with outdated ttl are removed.

@thepiwo

This comment has been minimized.

Copy link
Collaborator

thepiwo commented on fd0b492 Nov 26, 2018

looks great so far, thank you!

cytadela8 and others added some commits Nov 26, 2018

@grzegorz225
Copy link
Contributor

grzegorz225 left a comment

Looks good. Fork handling while calculating rewards needs to be introduced.

@@ -61,20 +60,21 @@ defmodule Aecore.Chain.Chainstate do
end

@spec calculate_and_validate_chain_state(
list(),
KeyBlock.t() | MicroBlock.t(),
Chainstate.t(),
non_neg_integer()
) :: {:ok, Chainstate.t()} | {:error, String.t()}
def calculate_and_validate_chain_state(block, chainstate, block_height) do

This comment has been minimized.

@grzegorz225

grzegorz225 Nov 27, 2018

Contributor

You can remove the case statement by matching the type of a block in the function definition:
calculate_and_validate_chain_state(%KeyBlock{}, chainstate, block_height) ...
calculate_and_validate_chain_state(%MicroBlock{}, chainstate, block_height) ...

case miner do
@genesis_miner ->
chainstate
defp calculate_miner_reward_chain_state(chainstate, block_height) do

This comment has been minimized.

@grzegorz225

grzegorz225 Nov 27, 2018

Contributor

If we have a fork like:

           [Gen2a] - ... - [Gen#{GovernanceConstants.beneficiary_reward_lock_time()+1}a] 
           //
[Gen1]--
            \\
            [Gen2b] - ... - [Gen#{GovernanceConstants.beneficiary_reward_lock_time()+1}b]

The reward calculation will apply wrong rewards on one of the branches. This situation could happen if a group of miners decides to build blocks on top of the mallicoius chain instead of building on top the most difficult chain. This could lead to miscalculations in the main chain which could invalidate the the main chain and lead to double spend attacks.

Make sure on what fork the block reward was calculated.

This comment has been minimized.

@d-velev

d-velev Nov 28, 2018

Author Member

I know, we need to handle forks generally, not only when calculating rewards. Currently the check for new blocks in add_validated_block is if top_height <= new_block_height, we should be comparing difficulties not height. Haven't thought about a way but I won't do it here...

}

if top_height < height do
if top_height <= height && !block_is_genesis do

This comment has been minimized.

@grzegorz225

grzegorz225 Nov 27, 2018

Contributor

Are we validating if the new block is linked to the genesis block?

chainstate
defp calculate_miner_reward_chain_state(chainstate, block_height) do
if block_height > GovernanceConstants.beneficiary_reward_lock_time() do
current_generation_beneficiary =

This comment has been minimized.

@grzegorz225

grzegorz225 Nov 27, 2018

Contributor

@cytadela8
https://github.com/aeternity/elixir-node/pull/798/files#diff-76c70afbd7cea50d60f10c30ae67b41cR555
We are querying the beneficiary pub key.

We are not checking whether we are applying the rewards for the correct fork (see comment above)

@thepiwo thepiwo removed the blocked label Nov 27, 2018

@thepiwo

This comment has been minimized.

Copy link
Collaborator

thepiwo commented Nov 27, 2018

I assume this is unblocked now

d-velev and others added some commits Nov 28, 2018

@thepiwo

This comment has been minimized.

Copy link
Collaborator

thepiwo commented Nov 29, 2018

@d-velev whats the status of this? will you merge master-mg in here?

@d-velev

This comment has been minimized.

Copy link
Member Author

d-velev commented Nov 29, 2018

@thepiwo I have to move the sleep before adding micro blocks, I'll get this ready to merge in an hour or so

thepiwo and others added some commits Nov 29, 2018

Merge pull request #806 from aeternity/GH-803
[GH-803] Added release notes for 0.2.0
Merge pull request #808 from aeternity/GH-807
added disclaimer in readme and updated version
@thepiwo

This comment has been minimized.

Copy link
Collaborator

thepiwo commented Dec 3, 2018

@grzegorz225 @DanielaIvanova could you review?

@thepiwo

thepiwo approved these changes Dec 3, 2018

Copy link
Collaborator

thepiwo left a comment

should be good for now

seems deprecated

@thepiwo thepiwo merged commit b8a2a0d into master-ng Dec 12, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
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.