Change the state machine used for processing new blocks #20

abacabadabacaba · 2020-09-11T23:03:22Z

Now a newly added block that is not yet trusted is stored in untrustedHead. After the challenge period is over, it can be moved to head. Fixes #13, #15.

ailisp · 2020-09-11T23:30:46Z

nearbridge/contracts/NearBridge.sol

-    BlockInfo public backupHead;
+
+    // The most recently added block. May still be in its challenge period, so should not be trusted.
+    BlockInfo public untrustedHead;


Great to make a difference between head and untrustedHead!

ailisp

Looks good, need bump version in package.json if we want to use this immediately. verify_near_headers test failed because it simply try to submit all blocks during a period, so refused by "not sufficiently new" check, either change test or remove some blocks for now

abacabadabacaba · 2020-09-11T23:56:26Z

I fixed verify_near_headers, but it still won't pass cli tests because there is new constructor parameter.

MaksymZavershynskyi

This logic makes contract more complex by adding an additional public method. Instead we can make head and backupHead private and add an additional method getTrustedHead() which returns either head or backupHead depending on whether validAfter has passed.

abacabadabacaba · 2020-09-12T00:17:04Z

This logic makes contract more complex by adding an additional public method. Instead we can make head and backupHead private and add an additional method getTrustedHead() which returns either head or backupHead depending on whether validAfter has passed.

I think that the logic is now less complex. There is head which is trusted. A new method is needed to move untrustedHead to head when the challenge period is over without pushing a new block. This also makes this block available to blockHashes and blockMerkleRoots. It is possible to add an extra method, but it won't make things less complex, because there should be still a way to commit a block so that NearProver can use it (it uses blockMerkleRoots).

Or I can just make commitBlock internal, so a block will only become available when another block is submitted on top of it.

Overall, this code needs a lot of refactoring, but for now I try not to change it too much.

MaksymZavershynskyi · 2020-09-12T00:22:21Z

This also makes this block available to blockHashes and blockMerkleRoots. It is possible to add an extra method, but it won't make things less complex, because there should be still a way to commit a block so that NearProver can use it (it uses blockMerkleRoots).

We could add trustedMerkleRoots method that filter the last merkle root if the header has not passed the challenge period yet. By adding commitBlock we are delegating complexity to the contract user who now need to call an extra method before being able to use this contract.

Or I can just make commitBlock internal, so a block will only become available when another block is submitted on top of it.

This will make header that passed challenge period not useful until the next header is submitted. Ideally, we want header to be useful as soon as it passes the challenge period.

abacabadabacaba · 2020-09-12T15:19:37Z

We could add trustedMerkleRoots method that filter the last merkle root if the header has not passed the challenge period yet.

That's not a good API design. "Trusted" should be the default (and indeed the only) option.

Anyway, I changed the API to return the last block as soon as the challenge period is over.

MaksymZavershynskyi

Looks good. There are some suggestions to add comments. We also need to add PR for rainbow-bridge-cli and rainbow-bridge-lib to make the tests pass. Sorry for the delay.

nearbridge/contracts/NearBridge.sol

MaksymZavershynskyi · 2020-09-14T23:15:14Z

nearbridge/contracts/NearBridge.sol

+    // Address of the account which submitted the last block.
+    address lastSubmitter;
+    // End of challenge period, or zero if there is no block to be challenged.
+    uint lastValidAt;


Please add a comment saying something like: "when it is zero untrustedHead, untrustedHeadIsFromNextEpoch, untrustedNextBlockProducers should be considered to be not set".

nearbridge/contracts/NearBridge.sol

abacabadabacaba · 2020-09-15T17:39:06Z

@nearmax I addressed the comments and also fixed another bug, is everything OK now? Will make a PR for near-bridge-cli and -lib a bit later. I can't merge this PR because of CI.

abacabadabacaba · 2020-09-15T17:58:55Z

Created pull requests: Near-One/rainbow-bridge#348, near/rainbow-bridge-lib#14.

MaksymZavershynskyi

Approving, assuming https://github.com/near/rainbow-bridge-sol/pull/20/files?file-filters%5B%5D=.sol#diff-b55dd2aebd3038c032230f5c9710455dR212 is the only non-comment change since previous review.

MaksymZavershynskyi · 2020-09-15T20:24:10Z

nearbridge/contracts/NearBridge.sol

        } else {
-            // The new block is from the next epoch.
-            _checkBp(nearBlock, nextBlockProducers);
+            revert("NearBridge: Epoch id of the block is not valid");


Was this the additional bug that you fixed? It is hard to see because of the forced push.

The bug that I fixed was that I added a check to checkBlockProducerSignatureInHead that signatureIndex is valid.

Are you talking about signatureIndex < untrustedBlockProducers.bpsLength check? Because it seems !untrustedApprovals[signatureIndex].none check does not fix vulnerabilities.

ailisp

Looks good!

MaksymZavershynskyi · 2020-09-22T16:55:46Z

dist.sh

@@ -15,6 +15,7 @@ do
  # Get contract name without extension and without directories.
  contract_name="${filename%.*}"
  node_modules/.bin/truffle-flattener "./contracts/${contract_name}.sol" > "dist/${contract_name}.full.sol"
+  sed -i '/^\/\/ SPDX-License-Identifier:/d' "dist/${contract_name}.full.sol"


Could we add a comment explaining what is this for?

This was because of NomicFoundation/truffle-flattener#55. Will add a comment.

MaksymZavershynskyi · 2020-09-22T16:56:33Z

nearbridge/contracts/NearBridge.sol

@@ -75,26 +82,27 @@ contract NearBridge is INearBridge {
        bytes32 blockHash
    );

-    constructor(Ed25519 ed, uint256 _lockEthAmount, uint256 _lockDuration) public {
+    constructor(Ed25519 ed, uint256 _lockEthAmount, uint256 _lockDuration, uint256 _replaceDuration) public {


This PR might be a good opportunity to make sure we follow Solidity style guide: https://solidity.readthedocs.io/en/v0.6.0/style-guide.html#avoiding-naming-collisions

MaksymZavershynskyi · 2020-09-22T16:57:58Z

nearbridge/contracts/NearBridge.sol

        balanceOf[msg.sender] = balanceOf[msg.sender].sub(lockEthAmount);
        msg.sender.transfer(lockEthAmount);
    }

-    function challenge(address payable receiver, uint256 signatureIndex) public {
-        require(block.timestamp < head.validAfter, "Lock period already passed");
+    function challenge(address payable receiver, uint256 signatureIndex) override public {


AFAIU override is only useful when contract inherits from another contract, not from an interface.

In Solidity 0.6, override is mandatory when inheriting from an interface. Without it, it just won't compile.

MaksymZavershynskyi · 2020-09-22T17:14:21Z

nearbridge/contracts/NearBridge.sol

+            res.currentHeight = head.height;
+            res.nextTimestamp = untrustedHead.timestamp;
+            res.nextValidAt = lastValidAt;
+            res.numBlockProducers =


It is unclear why do we not return numBlockProducers when there is an untrusted head.

We do return the number of block producers when there is an untrusted head. This number is used by the watchdog to determine the number of signatures to check.

MaksymZavershynskyi · 2020-09-22T17:14:40Z

nearbridge/contracts/NearBridge.sol

-    function addLightClientBlock(bytes memory data) public payable {
+    struct BridgeState {
+        uint currentHeight;
+        uint nextTimestamp;


Could we please add comments explaining each field?

MaksymZavershynskyi · 2020-09-22T17:22:47Z

nearbridge/contracts/NearBridge.sol

        } else {
-            // The new block is from the next epoch.
-            _checkBp(nearBlock, nextBlockProducers);
+            revert("NearBridge: Epoch id of the block is not valid");


Are you talking about signatureIndex < untrustedBlockProducers.bpsLength check? Because it seems !untrustedApprovals[signatureIndex].none check does not fix vulnerabilities.

MaksymZavershynskyi · 2020-09-22T17:24:32Z

nearbridge/contracts/NearBridge.sol

        }

-        // 6. If next_bps is not none, sha256(borsh(next_bps)) corresponds to the next_bp_hash in inner_lite.
-        if (!nearBlock.next_bps.none) {
+        // Check that the new block is signed by more than 2/3 of the validators.


The numbers in the comments were useful because they corresponded to the spec: https://nomicon.io/ChainSpec/LightClient.html

…. Now a newly added block that is not yet trusted is stored in untrustedHead. After the challenge period is over, it can be moved to head. Fixes #13, #15.

MaksymZavershynskyi

Looks good

MaksymZavershynskyi · 2020-10-06T01:16:22Z

I have tested this change locally, so I am going to merge it and publish the package.

abacabadabacaba requested review from MaksymZavershynskyi and ailisp September 11, 2020 23:03

ailisp reviewed Sep 11, 2020

View reviewed changes

abacabadabacaba force-pushed the fix-bridge-state-machine branch from 0bfa9af to 6bd6a89 Compare September 11, 2020 23:55

MaksymZavershynskyi suggested changes Sep 12, 2020

View reviewed changes

abacabadabacaba force-pushed the fix-bridge-state-machine branch from 6bd6a89 to b9e4819 Compare September 12, 2020 15:12

abacabadabacaba requested a review from MaksymZavershynskyi September 12, 2020 15:20

MaksymZavershynskyi approved these changes Sep 14, 2020

View reviewed changes

abacabadabacaba force-pushed the fix-bridge-state-machine branch from b9e4819 to 8a889fa Compare September 15, 2020 17:36

MaksymZavershynskyi approved these changes Sep 15, 2020

View reviewed changes

abacabadabacaba force-pushed the fix-bridge-state-machine branch 2 times, most recently from 1263476 to baa0a8e Compare September 20, 2020 21:37

MaksymZavershynskyi mentioned this pull request Sep 20, 2020

Merge new NearOnEthClient state machine design #23

Closed

abacabadabacaba force-pushed the fix-bridge-state-machine branch from baa0a8e to f4ad769 Compare September 21, 2020 17:15

abacabadabacaba requested review from ailisp and MaksymZavershynskyi September 21, 2020 18:23

ailisp approved these changes Sep 22, 2020

View reviewed changes

MaksymZavershynskyi suggested changes Sep 22, 2020

View reviewed changes

Bump version, change the state machine used for processing new blocks…

ea88dc6

…. Now a newly added block that is not yet trusted is stored in untrustedHead. After the challenge period is over, it can be moved to head. Fixes #13, #15.

abacabadabacaba force-pushed the fix-bridge-state-machine branch from f4ad769 to ea88dc6 Compare September 22, 2020 21:27

MaksymZavershynskyi approved these changes Sep 22, 2020

View reviewed changes

CHANGELOG

c357eca

MaksymZavershynskyi merged commit d89751f into master Oct 6, 2020

abacabadabacaba deleted the fix-bridge-state-machine branch October 6, 2020 10:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change the state machine used for processing new blocks #20

Change the state machine used for processing new blocks #20

abacabadabacaba commented Sep 11, 2020

ailisp Sep 11, 2020

ailisp left a comment

abacabadabacaba commented Sep 11, 2020

MaksymZavershynskyi left a comment

abacabadabacaba commented Sep 12, 2020

MaksymZavershynskyi commented Sep 12, 2020

abacabadabacaba commented Sep 12, 2020 •

edited

MaksymZavershynskyi left a comment

MaksymZavershynskyi Sep 14, 2020

abacabadabacaba commented Sep 15, 2020

abacabadabacaba commented Sep 15, 2020

MaksymZavershynskyi left a comment

MaksymZavershynskyi Sep 15, 2020

abacabadabacaba Sep 15, 2020

MaksymZavershynskyi Sep 22, 2020

ailisp left a comment

MaksymZavershynskyi Sep 22, 2020

abacabadabacaba Sep 22, 2020

MaksymZavershynskyi Sep 22, 2020

MaksymZavershynskyi Sep 22, 2020

abacabadabacaba Sep 22, 2020

MaksymZavershynskyi Sep 22, 2020

abacabadabacaba Sep 22, 2020

MaksymZavershynskyi Sep 22, 2020

MaksymZavershynskyi Sep 22, 2020

MaksymZavershynskyi Sep 22, 2020

MaksymZavershynskyi left a comment

MaksymZavershynskyi commented Oct 6, 2020

Change the state machine used for processing new blocks #20

Change the state machine used for processing new blocks #20

Conversation

abacabadabacaba commented Sep 11, 2020

Choose a reason for hiding this comment

ailisp left a comment

Choose a reason for hiding this comment

abacabadabacaba commented Sep 11, 2020

MaksymZavershynskyi left a comment

Choose a reason for hiding this comment

abacabadabacaba commented Sep 12, 2020

MaksymZavershynskyi commented Sep 12, 2020

abacabadabacaba commented Sep 12, 2020 • edited

MaksymZavershynskyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abacabadabacaba commented Sep 15, 2020

abacabadabacaba commented Sep 15, 2020

MaksymZavershynskyi 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

ailisp 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaksymZavershynskyi left a comment

Choose a reason for hiding this comment

MaksymZavershynskyi commented Oct 6, 2020

abacabadabacaba commented Sep 12, 2020 •

edited