Stop slashing commitments with "forced errors" by lrubiorod · Pull Request #1267 · witnet/witnet-rust

lrubiorod · 2020-05-28T15:40:47Z

This PR adds the next logic:

If you commit a RadonError you can not be slashed
If you commit a RadonError and you are in consensus, you will earn the wits reward
If you commit more RadonErrors than normal values (truths) in an epoch, you will not earn reputation.

lrubiorod · 2020-06-03T10:50:51Z

wallet/src/repository/wallet/mod.rs


            // Set not `in_consensus` reveals
-            for pkh in &tally.slashed_witnesses {
+            // TODO: Review that part for wallet guys (now out of consensus is no the same that slashed)


@mariocao, @aesedepece could you review that part?

seems ok... maybe it could have some indirect consequence in the way non-conensus reveals are shown in Sheikah

perfect, but in that case it could be solved in a little fix in the future if we notice that

lrubiorod · 2020-06-03T10:51:20Z

src/cli/node/json_rpc_client.rs

                            None => format!(""),
                            Some(tally) => {
-                                if tally.slashed_witnesses.contains(&pkh) {
+                                // TODO: Need review by tmpolaczyk


@tmpolaczyk could you review that part?

This condition should be "if the witness was not rewarded". But now there are 3 possible outcomes, so we should re-think the dataRequestReport method.

The current behavior is:

Honest: "+reward"

Liar: red text "-0"

I propose:

Honest: "+reward"

Liar: red text "-0"

Error: red text "+reward"

This way will be relatively simple to implement but it may be difficult for users to understand the difference, unless we explain all the consensus logic in dataRequestReport --help.

So yeah, we should add an additional case to this if. Will see if I can write a commit later.

we consider an error if it is out of consensus and an error, so it should not have a reward. And I think we have to include -collateral for the liars?

An error in consensus should be considered as honest

And I think we have to include -collateral for the liars?

Currently, dataRequestReport never shows -collateral when you lose the collateral, but that would make sense.

Proposal number 2:

Honest: "+reward"

Liar: red text "-collateral"

Error: "+0"

Proposal number 3:

Honest: "+reward" "+rep"

Liar: red text "-collateral"

Error: "+reward"

Although I'm not sure if it is possible to get the amount of reputation that has been rewarded to witnesses.

data_structures/src/transaction.rs

node/src/actors/chain_manager/mod.rs

validations/src/validations.rs

data_structures/src/data_request.rs

aesedepece · 2020-06-03T13:56:57Z

data_structures/src/data_request.rs

+        let collateral = if dr_output.collateral == 0 {
+            collateral_minimum
+        } else {
+            dr_output.collateral
+        };


Suggested change

let collateral = if dr_output.collateral == 0 {

collateral_minimum

} else {

dr_output.collateral

};

let collateral = max(collateral_minimum, dr_output.collateral);

That it is a different approach. We consider that if there are not a collateral value, we put the collateral minimum. But if the requester specify a quantity for collateral, and is less than minimum I think we shouldnt change and wait for the data request error in validation.

Maybe someone put 50_000 because he thinks is in wits and need a big amount of collateral, it is better that the request will be never included, and the requester notice his own error than publish the request with a very low value of collateral

data_structures/src/data_request.rs

data_structures/src/error.rs

validations/src/validations.rs

aesedepece · 2020-06-03T14:22:19Z

validations/src/validations.rs

+    let collateral = if dr_state.data_request.collateral == 0 {
+        collateral_minimum
+    } else {
+        dr_state.data_request.collateral
+    };


Suggested change

let collateral = if dr_state.data_request.collateral == 0 {

collateral_minimum

} else {

dr_state.data_request.collateral

};

let collateral = max(dr_state.data_request.collateral, collateral_minimum);

same as before

tmpolaczyk · 2020-06-03T15:30:38Z

node/src/actors/chain_manager/mod.rs

+    for (pkh, result) in rep_info {
+        if result.lies > 0 {
+            liars.push((pkh, result.lies));
+        // TODO: Decide which percentage would be fair enough


The behavior implemented in this PR is to only give reputation to nodes whose number of errors is smaller than the number of "honests".

This opens an attack vector where an attacker can flood the network with requests that always return errors, and nobody will be able to earn reputation except of course the attacker nodes, which will avoid participating in their own requests and will slowly accumulate all the reputation.

This attack need several wits to pay the rewards, because witness will earn the wits unless they do not earn reputation. The other option, give reputation to error_committers could produce incentives for node operators to commit errors without the risk to lose reputation, but also wining it

Currently if one identity commits one error and one "honest", it will not be rewarded any reputation. Maybe we can change the > into >= to be less aggressive.

Should we open an issue to further assess which percentage would be fair enough?

data_structures/src/data_request.rs

data_structures/src/error.rs

data_structures/src/radon_report.rs

data_structures/src/transaction.rs

validations/src/tests.rs

validations/src/validations.rs

mariocao · 2020-06-08T13:33:41Z

wallet/src/repository/wallet/mod.rs


            // Set not `in_consensus` reveals
-            for pkh in &tally.slashed_witnesses {
+            // TODO: Review that part for wallet guys (now out of consensus is no the same that slashed)


seems ok... maybe it could have some indirect consequence in the way non-conensus reveals are shown in Sheikah

mariocao · 2020-06-08T13:34:53Z

wallet/src/repository/wallet/mod.rs

+            // TODO: Review that part for wallet guys (now out of consensus is no the same that slashed)
+            for pkh in &tally.out_of_consensus {
                let liar = reveals.get_mut(&pkh).cloned();
                if let Some(mut reveal) = liar {


Suggested change

if let Some(mut reveal) = liar {

if let Some(mut reveal) = outlier {

Now they are not considered dishonest but outliers to the consensus.

data_structures/src/data_request.rs

tmpolaczyk · 2020-06-08T16:28:16Z

node/src/actors/chain_manager/mining.rs

            let reveals_count = dr_state.info.reveals.len();
-            let honests_count = commits_count - ta_tx.slashed_witnesses.len();
+
+            let (liars_count, errors_count) = calculate_liars_and_errors_count_from_tally(&ta_tx);


This operation is only needed to calculate the extra_tally_fee. If we could refactor this function from accepting tally_transactions: &[TallyTransaction] into tally_transactions_and_fees: &[(TallyTransaction, u64)] we could skip this calculation.

But we would need to add that information to create_tally_transactions, and I'm not sure if this change will be small enough to be included in this PR :P

uff, I want to merge as soon as possible, maybe we could open an issue to optimize some functions later

That's fine

validations/src/tests.rs

tmpolaczyk · 2020-06-09T10:25:43Z

validations/src/validations.rs

    reports_len: usize,
 ) -> RadonReport<RadonTypes> {
+    // This TallyMetadata would be included in case of Error Result, in that case, anyone has to be
+    // classified as a lier, but as an error


Suggested change

// classified as a lier, but as an error

// classified not as a liar, but as an error

validations/src/validations.rs

tmpolaczyk

Investigate issue with random_source.json:

[2020-06-09T11:20:38Z ERROR witnet_node::actors::chain_manager::mining] Error trying to mine a block: Invalid witness reward found: 1000001000. Expected value: 2000001000

Logs:
pr-1282-fail.zip

lrubiorod · 2020-06-09T14:03:30Z

Investigate issue with random_source.json:

[2020-06-09T11:20:38Z ERROR witnet_node::actors::chain_manager::mining] Error trying to mine a block: Invalid witness reward found: 1000001000. Expected value: 2000001000

Logs:
pr-1282-fail.zip

Fixed

lrubiorod force-pushed the no_slash branch from 9814ac0 to 4f00ec4 Compare June 1, 2020 16:38

aesedepece changed the title ~~no slash errors~~ Stop slashing commitments with "forced errors" Jun 2, 2020

lrubiorod force-pushed the no_slash branch from 4f00ec4 to f543c97 Compare June 3, 2020 10:40

lrubiorod marked this pull request as ready for review June 3, 2020 10:41

lrubiorod force-pushed the no_slash branch from f543c97 to e860332 Compare June 3, 2020 10:41

lrubiorod commented Jun 3, 2020

View reviewed changes

lrubiorod force-pushed the no_slash branch from e860332 to 35502cb Compare June 3, 2020 10:54

tmpolaczyk reviewed Jun 3, 2020

View reviewed changes

data_structures/src/transaction.rs Outdated Show resolved Hide resolved

tmpolaczyk reviewed Jun 3, 2020

View reviewed changes

node/src/actors/chain_manager/mod.rs Outdated Show resolved Hide resolved

tmpolaczyk reviewed Jun 3, 2020

View reviewed changes

validations/src/validations.rs Outdated Show resolved Hide resolved

tmpolaczyk reviewed Jun 3, 2020

View reviewed changes

data_structures/src/data_request.rs Outdated Show resolved Hide resolved

lrubiorod force-pushed the no_slash branch from 35502cb to 0acc033 Compare June 3, 2020 13:51

aesedepece requested changes Jun 3, 2020

View reviewed changes

tmpolaczyk reviewed Jun 3, 2020

View reviewed changes

data_structures/src/data_request.rs Outdated Show resolved Hide resolved

lrubiorod force-pushed the no_slash branch from 0acc033 to b2d90bd Compare June 3, 2020 17:19

lrubiorod requested review from aesedepece and tmpolaczyk June 4, 2020 08:32

lrubiorod force-pushed the no_slash branch from b2d90bd to 3bc151c Compare June 5, 2020 13:33