Attestations implementation #3791

nzpr · 2022-07-28T11:14:34Z

Overview

This PR adds implementation for attestation messages, which are the same block messages, but with pre state == post state and empty list of deploys.
The logic of block creation remains the same - there is an attempt to create block after each incoming block replayed.
But when there are no deploys to put in a block (no user deploys, no system deploys) - attestation is created.

I'm not sure yet how to enable testing for this. Thinking about more refactoring.

Closes #3249

Notes

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.

tgrospic · 2022-07-29T11:28:19Z

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

+  ): F[BlockCreatorResult] = {
+    val creatorsPk    = id.publicKey
+    val blockData     = BlockData(blockNum, creatorsPk, seqNum)
+    val shouldPropose = deploys.nonEmpty || toSlash.nonEmpty || changeEpoch


We need to split this logic in multiple functions. First to detect conditions and data for the next block and after that the block creation itself.
This is also important to correct handling of invalid blocks (consensus related and replay related) and also for easier testing.
Create function should ALWAYS create a block.

It looks to me that you want this function to be just "packageBlock".
If we go to deep refactoring, I think that there should not be even create block function which is special for propose. It should be the same thing for propose and replaying block.

shouldPropose can be calculated by the caller so that logic of checking for conditions is not mixed with the main logic of creation blocks.

We can leave it as is in this PR and fix it later.

tgrospic · 2022-07-29T11:29:49Z

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

-                signedBlock = validatorIdentity.signBlock(unsignedBlock)
-                _           <- Span[F].mark("block-signed")
+    postState.map {
+      case None                                                            => BlockCreatorResult.noNewDeploys


Block creator status should be removed, see comments above.

How do you want to signal about the result of a propose? What should be the result? Are you thinking to store it in Proposer state?

Checking the condition to propose or not should not be inside of the function doing the main logic. So if conditions are not satisfied propose should not be called.

We can leave it as is in this PR and fix it later.

tgrospic · 2022-07-29T11:32:28Z

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

+        nextBlockNum      = preState.justifications.map(_.blockNum).max + 1
+        parentHashes      = preState.justifications.map(_.blockHash)
+        finalBonds        = preState.fringeBondsMap
+        offenders         = preState.justifications.filter(_.validationFailed).map(_.sender)


This data should be extracted and used in block validation. From parents state expected data should be created and be used as a basis for validation. Block metadata will store expected data and invalid parts like invalid block number will be store separately. This is to make DB consistent even with invalid blocks.

tgrospic · 2022-07-29T11:37:38Z

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

+        finalBonds        = preState.fringeBondsMap
+        offenders         = preState.justifications.filter(_.validationFailed).map(_.sender)
+        // slashing
+        preStateBonds <- RuntimeManager[F].computeBonds(preStateHash.toByteString)


We cannot use bonds from PoS here, previous code is wrong in doing this. PoS bonds should only be used in epoch change to add new validators to final bonds and to remove rejected from final bonds.

Bonds on final state + bonds from consensus (parents) together must be used to validate and generate new blocks. Bonds from final state defines all active validators and bonds from consensus defines who is rejected in non-finalized blocks until the new bonds is finalized.

For now just use bonds from pre-state.

I'm not sure wdy mean - For now just use bonds from pre-state. I'm doing exactly that, don't I?

It's fine for now until finalizer have support for invalid blocks.

Situation that I have in mind is just before epoch change is finalized and fringe bonds map contains old validators and pre-state bonds contains new validators. When old validator is slashed it may not be visible to pre-state bonds, I'm not sure how this will be handled in PoS. Maybe I'm missing something now.

tgrospic · 2022-07-29T11:59:21Z

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

+            !newStateTransition && attestationStake * 3 < preStateBonds.values.toList.sum * 2
+          }
+        }
+        suppressAttestation <- nothingToFinalize ||^ waitingForSupermajorityToAttest
        // push dummy deploy if needed


What is the purpose of dummy deploys?

There is a dev mode in the node when you can specify to put deploy in each block, signed with specific key. So you can create blocks without external orchestration, pretending that there is always deploy in the pool.

From our standup discussion, we should later move this in a separate debugging code so it's not mixed with main block creation logic.

tgrospic · 2022-08-03T08:24:49Z

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


-    def createBlock(validatorIdentity: ValidatorIdentity): F[BlockCreatorResult] =
+    def createBlock(


I'd rather have BlockCreator with methods to create all necessary stuff to create blocks then Proposer.

tgrospic · 2022-08-03T08:26:27Z

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

    checkActiveValidator: ValidatorIdentity => F[Boolean],
-    createBlock: ValidatorIdentity => F[BlockCreatorResult],
+    createBlock: (ParentsMergedState, ValidatorIdentity) => F[BlockCreatorResult],
    validateBlock: BlockMessage => F[ValidBlockProcessing],
    proposeEffect: BlockMessage => F[Unit],
    validator: ValidatorIdentity


Why to have all these methods accepted separately? Proposer can just work with instance like BlockCreator to initiate block creator execution.
Take a look how easily can be tested code with dependencies with Mockito library.

Refactor API tests using mocks #3787

We now have 3 different things about Proposer which is confusing. For example ProposerInstance doesn't create Proposer instance, it just accepting it as an argument.

I'd like to see clear hierarchy of dependencies. BlockCreator can have methods with the logic to prepare each step in block creation process and Proposer can be the worker how is responsible to call the final method from BlockCreator in the right moment and/or from the right place.

tgrospic

LGTM

tgrospic · 2022-08-09T07:22:03Z

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

-                signedBlock = validatorIdentity.signBlock(unsignedBlock)
-                _           <- Span[F].mark("block-signed")
+    postState.map {
+      case None                                                            => BlockCreatorResult.noNewDeploys


Checking the condition to propose or not should not be inside of the function doing the main logic. So if conditions are not satisfied propose should not be called.

We can leave it as is in this PR and fix it later.

tgrospic · 2022-08-09T07:25:59Z

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

+  ): F[BlockCreatorResult] = {
+    val creatorsPk    = id.publicKey
+    val blockData     = BlockData(blockNum, creatorsPk, seqNum)
+    val shouldPropose = deploys.nonEmpty || toSlash.nonEmpty || changeEpoch


shouldPropose can be calculated by the caller so that logic of checking for conditions is not mixed with the main logic of creation blocks.

We can leave it as is in this PR and fix it later.

tgrospic · 2022-08-09T07:27:51Z

bors try

bors · 2022-08-09T08:10:52Z

try

Build failed:

bors build finished

stanislavlyalin

What is Attestations? Without reading PR description and especially issue description is very hard to understand it is just messages. Maybe we should provide it in PR's title or add some information from issue in PR's description. I would like to see some sequential diagram how attestations works and why it is needed. Why we cannot close block without attestations and what is "closing block"?

In general, it would be very good to have description in Wiki how blockchain works in examples and with terms. I think now we have good understanding of LLBM and can document it. I want to do it because want to understand this things. And I can do it from sources. We can create issue for that. @tgrospic, what do you think?

Few last commits looks unrelated to this PR.

stanislavlyalin · 2022-08-11T08:52:02Z

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

+      preState          <- MultiParentCasper.getPreStateForNewBlock[F]
+      creatorsPk        = validatorIdOpt.get.publicKey
+      creatorsId        = ByteString.copyFrom(creatorsPk.bytes)
+      creatorsLatestOpt = preState.justifications.find(_.sender == creatorsId)
+      nextSeqNum        = creatorsLatestOpt.map(_.seqNum + 1).getOrElse(0L)
+      nextBlockNum      = preState.justifications.map(_.blockNum).max + 1
+      createBlockResult <- BlockCreator(validatorIdOpt.get, shardName).create(
+                            preState.preStateHash,
+                            preState.justifications.map(_.blockHash),
+                            preState.fringeBondsMap,
+                            preState.rejectedDeploys,
+                            nextBlockNum,
+                            nextSeqNum,
+                            deployDatums.map(_.sig),
+                            Set(),
+                            false,
+                            true


Preparation of calling create looks very similar in all cases. Consider to hide some preparation logic inside create (e.g. extracting needed parameters from preState) or introduce additional preparation function.

stanislavlyalin · 2022-08-11T08:54:40Z

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

+//        // push dummy deploy if needed
+//        p <- BlockDagStorage[F].pooledDeploys
+//        _ <- dummyDeployOpt
+//              .traverse {
+//                case (privateKey, term) =>
+//                  val deployData = ConstructDeploy.sourceDeployNow(
+//                    source = term,
+//                    sec = privateKey,
+//                    vabn = nextBlockNum - 1,
+//                    shardId = shardId
+//                  )
+//                  BlockDagStorage[F].addDeploy(deployData)
+//              }
+//              .whenA(p.isEmpty)


Why do you leave this commented code? If it is really needed it would be good to add TODO comment.

…ooled storage is empty

stanislavlyalin · 2022-08-16T14:21:11Z

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

+              newBlocks.exists(b => b.state.systemDeploys.nonEmpty || b.state.deploys.nonEmpty)
+            val attestationStake =
+              preStateBonds.filterKeys(newBlocks.map(_.sender).toSet).values.toList.sum
+            !newStateTransition && attestationStake * 3 < preStateBonds.values.toList.sum * 2


I don't like that numbers 2 and 3 (stake threshold) hardcoded here and in other places (e.g. in Finalizer). Better to move it to utility function or to shared constant or to configuration.

stanislavlyalin · 2022-08-16T14:24:20Z

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

+          val newlySeen = creatorsLatestOpt
+            .map { prev =>
+              prev.justifications.flatMap(dag.dagMessageState.msgMap(_).seen) --
+                parentHashes.flatMap(dag.dagMessageState.msgMap(_).seen)


This block of code looks similar to forming conflictSet. Maybe better to extract
val msgMap = dag.dagMessageState.msgMap and use in both cases or to extract whole parentHashes.flatMap(dag.dagMessageState.msgMap(_).seen).

stanislavlyalin · 2022-08-16T14:26:22Z

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

+            .getOrElse(Set())
+          newlySeen.toList.traverse(BlockStore[F].getUnsafe).map { newBlocks =>
+            val newStateTransition =
+              newBlocks.exists(b => b.state.systemDeploys.nonEmpty || b.state.deploys.nonEmpty)


Consider to introduce function hasDeploys(block: BlockMessage) and used here and in creation of nothingToFinalize.

stanislavlyalin · 2022-08-16T14:27:39Z

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

+          OptionT
+            .fromOption(pooledOk.nonEmpty.guard[Option].as(pooledOk))


OptionT.whenF(pooledOk.nonEmpty)(pooledOk.pure) looks more natural for me. @tgrospic, it such replacement correct?

Yes, this also makes sense.

Another solution is to not even go to OptionT, but just use Option. In this case it's not necessary to mention pooledOk twice.
Option(pooledOk).filter(_.nonEmpty)

stanislavlyalin · 2022-08-16T14:28:15Z

casper/src/main/scala/coop/rchain/casper/dag/BlockDagKeyValueStorage.scala

+  ): F[Unit] = {
+    val newLPF = dbPruneFringe(newState, childMap)
+    val curLPF = dbPruneFringe(curState, childMap)
+    val toPune = (newState.msgMap.between(newLPF, curLPF) ++ curLPF).map(_.id)


toPune - looks like typo.

stanislavlyalin

👍

nzpr · 2022-08-18T04:46:30Z

bors merge

bors · 2022-08-18T05:12:08Z

Build succeeded:

bors build finished

nzpr force-pushed the attest branch 2 times, most recently from 78c04a9 to 8b4f482 Compare July 28, 2022 12:06

nzpr marked this pull request as ready for review July 28, 2022 15:28

nzpr requested review from tgrospic and stanislavlyalin July 28, 2022 15:28

nzpr assigned stanislavlyalin and tgrospic Jul 28, 2022

nzpr added the enhancement label Jul 28, 2022

nzpr added this to the Block merge (Hard Fork 2) milestone Jul 28, 2022

nzpr requested a review from hilltracer July 28, 2022 17:48

tgrospic reviewed Jul 29, 2022

View reviewed changes

nzpr force-pushed the attest branch 2 times, most recently from 5752e82 to bdda594 Compare August 2, 2022 17:38

tgrospic reviewed Aug 3, 2022

View reviewed changes

nzpr force-pushed the attest branch 4 times, most recently from b50e06d to be81bfd Compare August 8, 2022 09:05

tgrospic approved these changes Aug 9, 2022

View reviewed changes

bors bot added a commit that referenced this pull request Aug 9, 2022

Try #3791:

cced879

tgrospic removed their assignment Aug 11, 2022

stanislavlyalin reviewed Aug 11, 2022

View reviewed changes

nzpr added 3 commits August 12, 2022 10:33

Attestations implementation

4a14c5e

Suppress more atteststions

0d80996

adjust test

3801e9a

nzpr added 5 commits August 12, 2022 10:33

Fix bug in pre state merge

0eff5d5

Fix bug in proposer. Put in block rejections on finalization

2afe8c9

Fix bug in Merge Scope

845419b

Fix bug. Pool dummy deploy when nothing to put in a block, but when p…

8ea234c

…ooled storage is empty

Prune merging index

5370120

nzpr force-pushed the attest branch from be81bfd to 5370120 Compare August 12, 2022 07:36

nzpr mentioned this pull request Aug 16, 2022

Unable to consume results of system deploy investigation. #3804

Closed

stanislavlyalin requested changes Aug 16, 2022

View reviewed changes

stanislavlyalin approved these changes Aug 17, 2022

View reviewed changes

stanislavlyalin mentioned this pull request Aug 17, 2022

Refactor attestations implementation #3805

Closed

bors bot merged commit 641ad00 into rchain:dev Aug 18, 2022

stanislavlyalin mentioned this pull request Aug 19, 2022

Attestations refactoring #3806

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attestations implementation #3791

Attestations implementation #3791

nzpr commented Jul 28, 2022 •

edited

Loading

tgrospic Jul 29, 2022

nzpr Jul 30, 2022

tgrospic Aug 9, 2022

tgrospic Jul 29, 2022

nzpr Aug 2, 2022

tgrospic Aug 9, 2022

tgrospic Jul 29, 2022

tgrospic Jul 29, 2022

nzpr Jul 29, 2022

tgrospic Jul 29, 2022

tgrospic Jul 29, 2022

nzpr Jul 29, 2022

tgrospic Jul 29, 2022

tgrospic Aug 3, 2022

tgrospic Aug 3, 2022

tgrospic left a comment

tgrospic Aug 9, 2022

tgrospic Aug 9, 2022

tgrospic commented Aug 9, 2022

bors bot commented Aug 9, 2022

stanislavlyalin left a comment

stanislavlyalin Aug 11, 2022

stanislavlyalin Aug 11, 2022

stanislavlyalin Aug 16, 2022

stanislavlyalin Aug 16, 2022

stanislavlyalin Aug 16, 2022

stanislavlyalin Aug 16, 2022

tgrospic Aug 17, 2022

stanislavlyalin Aug 16, 2022

stanislavlyalin left a comment

nzpr commented Aug 18, 2022

bors bot commented Aug 18, 2022


		def createBlock(validatorIdentity: ValidatorIdentity): F[BlockCreatorResult] =
		def createBlock(

		OptionT
		.fromOption(pooledOk.nonEmpty.guard[Option].as(pooledOk))

Attestations implementation #3791

Attestations implementation #3791

Conversation

nzpr commented Jul 28, 2022 • edited Loading

Overview

Notes

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgrospic commented Aug 9, 2022

bors bot commented Aug 9, 2022

try

stanislavlyalin left a 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

stanislavlyalin left a comment

Choose a reason for hiding this comment

nzpr commented Aug 18, 2022

bors bot commented Aug 18, 2022

nzpr commented Jul 28, 2022 •

edited

Loading