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

Define and implement block rewards #1768

Closed
wants to merge 6 commits into from

Conversation

ferencdg
Copy link
Contributor

fixes: #246

@@ -0,0 +1,87 @@
/*******************************************************************************
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add 2 more tests: missing signatures and missing validators

@@ -148,13 +148,6 @@ public string isInvalidReason (in Block block, Engine engine, Height prev_height
if (only_coinbase)
return "Block: Must contain other transactions than Coinbase";

auto expected_cb_txs = getCoinbaseTX(block.txs,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

point 5
#246 (comment)

@@ -214,10 +214,6 @@ unittest
{
createAndExpectNewBlock(Height(block_idx));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

point 5
#246 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean a node can be fed blocks with wrong CB txs? Do we just trust the peer when we are catching up?

Copy link
Contributor Author

@ferencdg ferencdg Mar 11, 2021

Choose a reason for hiding this comment

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

Does this mean a node can be fed blocks with wrong CB txs? Do we just trust the peer when we are catching up?

We are still checking if at least 50% of the validators signed the block, so not just trusting only 1 peer.

------ background

However, Mathias described an attack before

  1. in 2020, lets say we have 20 validators, 2 are malicious, 8 are lazy(they don't sign blocks), and 10 are well-behaving. Those 10 well-behaving is able to create a block in 2020.
  2. in 2022, 8 validators(from 2020) sell their private keys(6 previously lazy ones and 2 previously well-behaving one are the sellers).
  3. now those 2 malicious validators from 2020 along with the newly bought private keys can dupe a 'catching up' a validator.

Our block reward(based on validators signing blocks) will try to minimize the number of lazy validators, so let's in 2020 we didn't have 8 lazy validators, just 2, so the block was signed by 6+10=16 validators... With block reward in mind the person in 2022 have to buy more private keys, because we will also change our catchup logic at some point to the following one:

each validator needs to accept the block with the MOST signatures at a specific height

----- end of background

so if we implement all above, then a catching up validator can only be duped, if someone can create a block with let's say 60%-70% of all signatures


Returns:
List of expected Coinbase TXs
One or zero Coinbase TX
Copy link
Contributor Author

@ferencdg ferencdg Mar 11, 2021

Choose a reason for hiding this comment

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

explanation
0cd1c01#diff-d25eeef12241e794638a748cea9cc8e7b124f1d61ccdaa962f7fb64ed3c66cb6R725

actually, even before my change we had numerous asserts that this function only returns 1 or less elements in the array

@codecov
Copy link

codecov bot commented Mar 11, 2021

Codecov Report

Merging #1768 (da52927) into v0.x.x (6f78c68) will increase coverage by 0.67%.
The diff coverage is 96.48%.

❗ Current head da52927 differs from pull request most recent head 3055b73. Consider uploading reports for the commit 3055b73 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           v0.x.x    #1768      +/-   ##
==========================================
+ Coverage   88.05%   88.72%   +0.67%     
==========================================
  Files         144      147       +3     
  Lines       13869    13825      -44     
==========================================
+ Hits        12212    12266      +54     
+ Misses       1657     1559      -98     
Flag Coverage Δ
integration 42.36% <ø> (+4.47%) ⬆️
unittests 88.50% <96.48%> (+0.75%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
source/agora/common/Set.d 90.90% <ø> (-0.40%) ⬇️
source/agora/consensus/validation/Block.d 95.75% <ø> (-0.12%) ⬇️
source/agora/test/Base.d 76.66% <ø> (+2.81%) ⬆️
source/agora/test/Fee.d 95.65% <ø> (-0.10%) ⬇️
source/agora/test/MissingPreImageDetection.d 93.33% <ø> (+0.31%) ⬆️
source/agora/test/ValidatorRecurringEnrollment.d 92.30% <ø> (-2.14%) ⬇️
source/agora/utils/Log.d 63.46% <50.00%> (-8.10%) ⬇️
source/agora/consensus/EnrollmentManager.d 95.06% <75.00%> (-0.78%) ⬇️
source/agora/common/Amount.d 97.46% <80.00%> (-0.61%) ⬇️
source/agora/node/Ledger.d 94.13% <91.66%> (+<0.01%) ⬆️
... and 102 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 6f78c68...3055b73. Read the comment docs.

@ferencdg
Copy link
Contributor Author

ready for review - squashed some commits and cleaned up the code

Copy link
Contributor

@AndrejMitrovic AndrejMitrovic left a comment

Choose a reason for hiding this comment

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

Just a few comments.

I'm not sure whether using floating point is a good idea, there were reservations against using it in the past.

@@ -86,6 +86,13 @@ public struct Amount
return ret;
}

/// Non-throwing version of toString
public string toStringNT () const @safe nothrow
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed? Commit doesn't explain it.

Copy link
Contributor Author

@ferencdg ferencdg Mar 15, 2021

Choose a reason for hiding this comment

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

Why is this needed? Commit doesn't explain it.

I had some 'nothrow' functions from which I had to print out the 'Amount', I will modify the comment to make it more obvious that this 'non-throwing' version should mainly be used for debugging.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not used anywhere and stands out, I'd remove it.
If you need to debug you can use the scope (failure) assert(0); hack.

block signatures which is the basis of the block reward.
3. Each validator will be rewarded based on how many blocks they signed
and the reward will NOT be propotional to how much coin they staked.
4 The block reward is divided between the validators and the Foundation.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Foundation/Commons Budget?


mixin AddLogger!();

public class BlockRewardCalc
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs /// or ddoc is not generated.

public Amount[Point] getEachValidatorReward (in Height height, ushort[Point] validator_stakes,
out Amount remainder, double comply_perc, uint indent = 0) @safe const nothrow
{
auto indentStr = ' '.repeat(indent).array;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will cause an allocation even if trace logging is disabled. Could perhaps turn it into a function?

Copy link
Contributor Author

@ferencdg ferencdg Mar 17, 2021

Choose a reason for hiding this comment

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

This will cause an allocation even if trace logging is disabled. Could perhaps turn it into a function?

hmm, Ocean's format function(called by log.trace) is defined as

public void format (Args...) (Level level, cstring fmt, Args args) @safe

so it cannot just lazily evaluate the argument.

the only thing I can think of is

class IndentClass(int indentation)
{
    void toString (scope FormatterSink sink)
    {
        sink(' '.repeat(indent).array);
    }
}

and then create an instance of this class on the stack(or just declare it as struct), and then pass that instance to log.trace :)

do you think it is worth the effort?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's overkill then. Let's fix it later if it's not a big deal.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Amount gcd_validator_reward = total_validator_reward;

ulong total_stakes;
foreach (ref stake; validator_stakes.byValue())
Copy link
Contributor

Choose a reason for hiding this comment

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

const ref?

min_validator_reward.div(total_stakes);

Amount actual_total_validator_reward;
foreach (pkey_stake; validator_stakes.byKeyValue())
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe also const ref here?

remainder.mustSub(actual_total_validator_reward);
foreach (amount_point_pair; each_validator_rewards.byKeyValue())
log.trace(indentStr ~ "{} -> {}", amount_point_pair.key, amount_point_pair.value);
log.trace(indentStr ~ "Remainder is: {}", remainder.toStringNT());
Copy link
Contributor

Choose a reason for hiding this comment

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

Can use comma instead of concatenation.

out Amount remainder, double comply_perc, uint indent = 0) @safe const nothrow
{
auto indentStr = ' '.repeat(indent).array;
log.trace(indentStr ~ "Calculating each validator reward");
Copy link
Contributor

Choose a reason for hiding this comment

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

Can use comma instead of concatenation.

@@ -1,4 +1,4 @@
/*******************************************************************************
/*******************************************************************************
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops

}
catch (Exception e) {
log.error("unable to read block at height: {}", height);
return keys;
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this just return null then?

@ferencdg
Copy link
Contributor Author

ferencdg commented Mar 15, 2021

Just a few comments.

I'm not sure whether using floating point is a good idea, there were reservations against using it in the past.

Do you by any chance remember why floating point was not recommended? The only thing I could think is

  1. double is kind of a waste, it takes 64 bytes
  2. some cpu's use different floating point units and precision might differ -> it is not an issue in our case as I think 'double' type is standardized, so arithmetic on them should be the same on every fpu(vs 'real' type in dlang)
  3. precision loss: 0.1 has a finite decimal representation, but infinete binary representation(0.00011001100110011011....) -> we don't care about this

the exponential function that I use for 'block reward reduction' returns a flotaing point value, so the only think I could do is represent the input(comply percentage) to that exponential function in decimal(I just multiply it by 10.000 let's say), so I can pass around that input parameter to different functions. However when I use it as the input in the exponential function, I still have to convert it back to floating point. (I don't think we want to create our own integer based exponential function like 2^x and then try to somehow fit the return value between 0 - 100 so it can be used as reduction percentage)

@ferencdg ferencdg force-pushed the block-reward branch 2 times, most recently from 13fa65a to f7ca780 Compare March 17, 2021 01:59
@ferencdg
Copy link
Contributor Author

thanks for the review @AndrejMitrovic, if anyone else has a comment, please let me know, so this PR could be merged this sprint

public string toStringNT () const @safe nothrow
{
try return this.toString();
catch (Exception e) {return "";}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this return string should indicate the exception.

Copy link
Contributor Author

@ferencdg ferencdg Mar 18, 2021

Choose a reason for hiding this comment

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

I was thinking returning null, but that is probably a bad idea: it would require the call site to do extra checks. So as you suggested I will just return some error message.

@ferencdg ferencdg force-pushed the block-reward branch 2 times, most recently from f7875c4 to e27b616 Compare March 23, 2021 02:36
@ferencdg
Copy link
Contributor Author

rebased, let me know if any more changes needed

This is because we want to give enough time validators to share
block signatures which is the basis of the block reward.
3. Each validator will be rewarded based on how many blocks they signed
and the reward will NOT be propotional to how much coin they staked.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite clearly see why it shouldn't be proportional? Is this explained anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I remember correctly it was not my decision, but let me know if I am mistaken (@Geod24). A validator who has X * 40k BOA can and will run multiple Agora instances to maximimze its reward, so I agree with you on this.

Copy link
Collaborator

@Geod24 Geod24 left a comment

Choose a reason for hiding this comment

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

Did a first pass.

@@ -79,6 +79,12 @@ public struct Set (T)
this.put(key);
}

/// Returns true if the set has the element e, otherwise returns false
public auto opBinary (string op)(in T e) nothrow const pure @safe @nogc if (op == "in")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Type ctor (const, inout, shared, etc...) always come first.
Also it's a template so you don't need to specify all attributes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure I am following this:
'Also it's a template so you don't need to specify all attributes.'

are those attributes stripped after the template instantiation? or are we expecting

return e in _set;

might throw for some instantiation?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean that you can remove @safe because the compiler will check the body, see that it is @safe, and mark it @safe itself without you needing to do anything.

@@ -86,6 +86,13 @@ public struct Amount
return ret;
}

/// Non-throwing version of toString
public string toStringNT () const @safe nothrow
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not used anywhere and stands out, I'd remove it.
If you need to debug you can use the scope (failure) assert(0); hack.


*******************************************************************************/

module agora.test.Common;
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's what Base is for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'
it's not used anywhere and stands out, I'd remove it.
If you need to debug you can use the scope (failure) assert(0); hack.
'

it is used in BlockRewardCalc.getEachValidatorReward

Copy link
Contributor Author

@ferencdg ferencdg Mar 24, 2021

Choose a reason for hiding this comment

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

That's what Base is for.

my plan was to put the higher level classes like Validator/EnrollmentManager to a new file as the module description suggests:

Common classes and utilities shared between the network tests.
    Classes and utilities in this module are considered higher level than
    in Base.d.

Base.d is already a monster with 2000+ lines of code. I can move those classes to Base.d though

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just make them public in one of the test modules and import them in the other test files. Otherwise common will become the next monster.

Copy link
Contributor Author

@ferencdg ferencdg Jul 6, 2021

Choose a reason for hiding this comment

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

I think

  1. It would be hard to find that class if we just put it randomly into some of the test files from where we use it.
  2. Someone already moved those classes into Base.d anyways in recent v0.x.x branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case can you rebase please.

// EnrollmentManager that might or migh not reveal preimage based on the value
// current value of an atomic shared variable
version (unittest)
package class MissingPreImageEM : EnrollmentManager
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a good idea to make them reusable, but those classes have a few issues. One of them being public static shared UTXOSet utxo_set;

Copy link
Contributor Author

@ferencdg ferencdg Mar 24, 2021

Choose a reason for hiding this comment

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

I have an other PR that tries to solve that problem:
#1817 (comment)
any comment on that comment?

I do need to reuse those classes for block reward.


*******************************************************************************/

module agora.consensus.BlockRewardCalc;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
module agora.consensus.BlockRewardCalc;
module agora.consensus.Rewards;

Let's keep it simple and spellable

Comment on lines 64 to 65
///
private alias CurrentPeriodTup = Tuple!(ulong, "period", bool, "is_period_end");
Copy link
Collaborator

Choose a reason for hiding this comment

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

A struct would also work :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just wondering if struct would have any benefit over Tuples or if we should refine our coding guidelines. Tuples have some nice methods like expand, translate... Any suggestion on when to use one over the other?

min_validator_reward.div(total_stakes);

Amount actual_total_validator_reward;
foreach (const ref pkey_stake; validator_stakes.byKeyValue())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the complicated pattern when you can just do:

foreach (key; const ref value; validator_stakes)

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got a nasty DLang bug when tried:
source/agora/consensus/BlockRewardCalc.d(146,9): Error: _aaApply2 is not nothrow
source/agora/consensus/BlockRewardCalc.d(128,26): Error: nothrow function agora.consensus.BlockRewardCalc.BlockRewardCalc.getEachValidatorReward may throw

to reproduce

void f() nothrow
{
    int[string] m;
    foreach(const ref k, v; m)
    {
    }
}
int testmain()
{
    f();
    return 0;
}

Comment on lines 166 to 167
auto validator1 = Point.fromString("0xab4f6f6e85b8d0d38f5d5798a4bdc4dd444c8909c8a5389d3bb209a186105110");
auto validator2 = Point.fromString("0xab4f6f6e85b8d0d38f5d5798a4bdc4dd444c8909c8a5389d3bb209a186105111");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use well-known KPs


***************************************************************************/

public Amount getReducedValidatorReward (in Height height, double comply_perc, uint indent = 0) @safe const nothrow
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let me tell you a floating point joke:
A man has a computer problem. He decides to use floating point. Now he has 2.00000000001 problems.
Usually, we use an integer with a fixed precision for this kind of use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ehhmm, I tried to explain it here:
#1768 (comment)

any comment on that?


if (reduce_perc == 100)
return total_validator_reward;
total_validator_reward.div(100);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about the remainder ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

remainder goes to the Commons Budget and calculated by: getExtraCommonsBudgetReward

@ferencdg
Copy link
Contributor Author

ferencdg commented Mar 25, 2021

thanks for the review @Geod24, I fixed what I could, but for the following ones I had suggestions/questions:

#1768 (comment)
#1768 (comment)
#1768 (comment)
#1768 (comment)
#1768 (comment)
#1768 (comment)

If I could get some comment on those that you disagree, then I might be able to finish this task this Sprint.

@ferencdg
Copy link
Contributor Author

ferencdg commented Apr 6, 2021

rebase #3

@ferencdg
Copy link
Contributor Author

rebase #4

Copy link
Contributor

@hewison-chris hewison-chris left a comment

Choose a reason for hiding this comment

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

I will continue the review after reading the Rewards section of the white paper.

else
return this.keymap.index_to_key[height][index];
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Would prefer this in a separate commit (or with the changes it is created for) as it is unrelated to the other changes in this commit.


*******************************************************************************/

module agora.test.Common;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just make them public in one of the test modules and import them in the other test files. Otherwise common will become the next monster.

@@ -787,11 +929,6 @@ public class Ledger
return "Internal error: Could not calculate random seed at current height";
}

auto expected_cb_txs = this.getCoinbaseTX(block.txs, block.header.missing_validators);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ferencdg
Copy link
Contributor Author

rebase #5

@hewison-chris
Copy link
Contributor

Closing as will continue in new PR #2415

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.

Define and implement block rewards
5 participants