Attestations refactoring #3806

stanislavlyalin · 2022-08-19T11:09:55Z

Overview

This PR resolves some review issues for PR #3791 which is already merged.

Please make sure that this PR:

is at most 200 lines of code (excluding tests),
meets RChain development coding standards,
includes tests for all added features,
has a reviewer assigned,
has all commits signed.

Bors cheat-sheet:

bors r+ runs integration tests and merges the PR (if it's approved),
bors try runs integration tests for the PR,
bors delegate+ enables non-maintainer PR authors to run the above.

… constants 2 and 3

stanislavlyalin · 2022-08-19T19:17:43Z

sdk/src/main/scala/coop/rchain/sdk/casper/Stake.scala

@@ -0,0 +1,14 @@
+package coop.rchain.sdk.casper


I'm not sure where is the best place to locate this functions. My motivation was to place it to sdk project as common project and in casper subfolder as stake is part of Casper algorithm.

I think it can be in package object in casper folder. This is general logic which is used in many places.

Also it makes sense to add extensions/aliases for bonds map, when instead of full stake it is Map[Validator, Long]. This one can be a bit tricky because you have to account situation with equal stake. If you use conversion to Set you might lose some stake, I encountered this once. So just want to warn.

This package should be called consensus because it's not Casper specific. Supermajority of stake is theoretical requirement for consensus safety.

stanislavlyalin · 2022-08-19T19:22:17Z

block-storage/src/main/scala/coop/rchain/blockstorage/dag/Finalizer.scala

@@ -130,7 +131,7 @@ final case class Finalizer[M, S](msgMap: Map[M, Message[M, S]]) {
    // Total stake
    val totalStake = bondsMap.values.toSeq.sum
    // Calculate if 2/3 of stake supporting next layer
-    fullPartitionStake.toDouble / totalStake > 2d / 3
+    Stake.prevails(fullPartitionStake, totalStake)


In previous version greater (>) comparsion operator and floating-point calculations were used . More correct is to use greater or equal (>=) operator and integer calculations.

I was wrong. As shown in the article Byzantine_fault (I was guided by the Russian version), the stake must be strictly greater than 2/3.

stanislavlyalin · 2022-08-19T19:24:16Z

sdk/src/main/scala/coop/rchain/sdk/casper/Stake.scala

+  /**
+    * Check if stake prevails threshold of 2/3
+    */
+  def prevails(stake: Long, totalStake: Long): Boolean = 3 * stake >= 2 * totalStake


I'm not satisfied with this name but I couldn't find out more appropriate name. Anyway for me such functions is better then using magic constants directly.

This is a isSupermajority. I think you are right to use >= here.

While majority is > 50, it is not enough for Byzantine settings https://en.wikipedia.org/wiki/Byzantine_fault. So to be sure that in non trusted environment some decision is made, we have to rely on supermajority, which is 2/3.

Imagine large value for the stake with many validators. In that case Long (int64) values can overflow and calculation will be false. That's the reason why I used floats.

This is a isSupermajority. I think you are right to use >= here.

PTAL at this. I guess > is more correct so I was wrong.

stanislavlyalin · 2022-08-19T19:25:54Z

casper/src/test/scala/coop/rchain/casper/helper/TestNode.scala

-      preState          <- MultiParentCasper.getPreStateForNewBlock[F]
-      preState          <- MultiParentCasper.getPreStateForNewBlock[F]


There was duplicated lines of code. I guess it was just mistake so I removed one of them.

stanislavlyalin · 2022-08-19T19:28:02Z

casper/src/test/scala/coop/rchain/casper/helper/TestNode.scala

-                            preState.preStateHash,
-                            preState.justifications.map(_.blockHash),
-                            preState.fringeBondsMap,
+                            preState,
                            preState.rejectedDeploys,


Here rejectedDeploys field is used, while the Proposer uses fringeRejectedDeploys. I just want to pay attention. Is this correct?

It should be fringeRejectedDeploys as in Proposer, you are spot on.

nzpr · 2022-08-20T18:19:25Z

casper/src/main/scala/coop/rchain/casper/blocks/proposer/Proposer.scala

+        deploys <- for {
+                    dummy <- dummyDeployOpt.traverse {
+                              case (privateKey, term) =>
+                                val deployData = ConstructDeploy.sourceDeployNow(
+                                  source = term,
+                                  sec = privateKey,
+                                  vabn = nextBlockNum - 1,
+                                  shardId = shardId
+                                )
+                                BlockDagStorage[F].addDeploy(deployData).as(List(deployData.sig))
+                            }
+                  } yield Option(pooledOk).filter(_.nonEmpty).orElse(dummy).getOrElse(List.empty)


This will add dummy deploy into deploy storage even if it is not required.
Maybe you can use OptionT instead of Option and declare dummy as a suspended value (as in my initial code).

nzpr · 2022-08-20T18:21:32Z

casper/src/main/scala/coop/rchain/casper/blocks/proposer/Proposer.scala

-        }
-        hasDeploys = (b: BlockMessage) => b.state.systemDeploys.nonEmpty || b.state.deploys.nonEmpty
+        dag         <- BlockDagStorage[F].getRepresentation
+        seen        = (hash: BlockHash) => dag.dagMessageState.msgMap(hash).seen


You can write this as dag.dagMessageState.msgMap(_: BlockHash).seen

tgrospic · 2022-08-22T09:07:24Z

sdk/src/main/scala/coop/rchain/sdk/casper/Stake.scala

@@ -0,0 +1,14 @@
+package coop.rchain.sdk.casper


This package should be called consensus because it's not Casper specific. Supermajority of stake is theoretical requirement for consensus safety.

tgrospic · 2022-08-22T09:13:27Z

sdk/src/main/scala/coop/rchain/sdk/casper/Stake.scala

+  /**
+    * Check if stake prevails threshold of 2/3
+    */
+  def prevails(stake: Long, totalStake: Long): Boolean = 3 * stake >= 2 * totalStake


Imagine large value for the stake with many validators. In that case Long (int64) values can overflow and calculation will be false. That's the reason why I used floats.

tgrospic · 2022-08-22T09:15:26Z

casper/src/main/scala/coop/rchain/casper/blocks/proposer/BlockCreator.scala

+    val bondsMap          = preState.fringeBondsMap
+    val blockNum          = preState.justifications.map(_.blockNum).max + 1
+    val creatorsPk        = id.publicKey
+    val creatorsId        = ByteString.copyFrom(creatorsPk.bytes)


Please use extension to convert to ByteString.

tgrospic

LGTM

nzpr

LGTM

stanislavlyalin · 2022-08-25T17:36:41Z

bors merge

bors · 2022-08-25T18:06:09Z

Build succeeded:

bors build finished

Removed commented code

998942a

stanislavlyalin linked an issue Aug 19, 2022 that may be closed by this pull request

Refactor attestations implementation #3805

Closed

stanislavlyalin self-assigned this Aug 19, 2022

stanislavlyalin added Priority-Low GoodToHave labels Aug 19, 2022

stanislavlyalin added this to the Block merge (Hard Fork 2) milestone Aug 19, 2022

stanislavlyalin added 6 commits August 19, 2022 14:12

Fixed typo

e0a4d3a

Simplified deploys creation

777c541

Refactoring: introduced hasDeploys for block

58754cd

Refactoring: simplified using of msgMap(_).seen

b993889

Introduced functions to check stake is enough to avoid using of magic…

3966737

… constants 2 and 3

Simplified usage of BlockCreator.create

37c83c6

stanislavlyalin force-pushed the refactor-attestations branch from d92a26e to 37c83c6 Compare August 19, 2022 14:31

stanislavlyalin commented Aug 19, 2022

View reviewed changes

stanislavlyalin requested review from nzpr and tgrospic August 19, 2022 19:31

nzpr reviewed Aug 20, 2022

View reviewed changes

tgrospic reviewed Aug 22, 2022

View reviewed changes

stanislavlyalin added 4 commits August 23, 2022 10:09

Refactored Stake object

c60eb11

Refactoring: finalization is extracted from preState

7e72a2b

Refactoring: fixed review issues

e7a274b

Fixed creation of OptionT

1c628fd

stanislavlyalin marked this pull request as ready for review August 23, 2022 07:42

stanislavlyalin requested review from nzpr and tgrospic August 23, 2022 07:44

tgrospic approved these changes Aug 23, 2022

View reviewed changes

nzpr approved these changes Aug 25, 2022

View reviewed changes

bors bot merged commit ae983fc into rchain:dev Aug 25, 2022

stanislavlyalin deleted the refactor-attestations branch August 26, 2022 12:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attestations refactoring #3806

Attestations refactoring #3806

stanislavlyalin commented Aug 19, 2022 •

edited

Loading

stanislavlyalin Aug 19, 2022

nzpr Aug 20, 2022

tgrospic Aug 22, 2022

stanislavlyalin Aug 19, 2022

stanislavlyalin Aug 23, 2022

stanislavlyalin Aug 19, 2022

nzpr Aug 20, 2022 •

edited

Loading

tgrospic Aug 22, 2022

stanislavlyalin Aug 23, 2022

stanislavlyalin Aug 19, 2022

stanislavlyalin Aug 19, 2022

nzpr Aug 20, 2022

nzpr Aug 20, 2022

nzpr Aug 20, 2022

tgrospic Aug 22, 2022

tgrospic Aug 22, 2022

tgrospic Aug 22, 2022

tgrospic left a comment

nzpr left a comment •

edited

Loading

stanislavlyalin commented Aug 25, 2022

bors bot commented Aug 25, 2022

		preState <- MultiParentCasper.getPreStateForNewBlock[F]
		preState <- MultiParentCasper.getPreStateForNewBlock[F]

Attestations refactoring #3806

Attestations refactoring #3806

Conversation

stanislavlyalin commented Aug 19, 2022 • edited Loading

Overview

Please make sure that this PR:

Bors cheat-sheet:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nzpr Aug 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgrospic left a comment

Choose a reason for hiding this comment

nzpr left a comment • edited Loading

Choose a reason for hiding this comment

stanislavlyalin commented Aug 25, 2022

bors bot commented Aug 25, 2022

stanislavlyalin commented Aug 19, 2022 •

edited

Loading

nzpr Aug 20, 2022 •

edited

Loading

nzpr left a comment •

edited

Loading