contracts: modify checkpoint contract and rewrite unittests

contracts/registrar/contract/registrar.sol

@@ -6,22 +6,6 @@ pragma solidity ^0.4.24;
  * @dev Implementation of the blockchain checkpoint information registrar.
  */
 contract Registrar {
-    /*
-        Definitions
-    */
-
-    // Checkpoint represents a set of post-processed trie roots (CHT and BloomTrie)
-    // associated with the appropriate section head hash.
-    //
-    // It is used to start light syncing from this checkpoint
-    // and avoid downloading the entire header chain while still being able to securely
-    // access old headers/logs.
-    struct Checkpoint {
-        bytes32 sectionHead;
-        bytes32 chtRoot;
-        bytes32 bloomTrieRoot;
-    }
-
     /*
         Modifiers
     */
@@ -39,13 +23,17 @@ contract Registrar {
     */
 
     // NewCheckpointEvent is emitted when new checkpoint is registered.
-    event NewCheckpointEvent(uint indexed index, bytes32 sectionHead, bytes32 chtRoot, bytes32 bloomTrieRoot);
+    // Grantor indicates the people register the checkpoint.
+    // We use checkpoint hash instead of the full checkpoint to make the transaction cheaper.
+    event NewCheckpointEvent(uint indexed index, address grantor, bytes32 checkpointHash);
 
     // AddAdminEvent is emitted when new address is accepted as admin.
-    event AddAdminEvent(address addr);
+    // Grantor indicates who authorizes the add admin operation.
+    event AddAdminEvent(address addr, address grantor, string description);
 
     // RemoveAdminEvent is emitted when an admin is removed.
-    event RemoveAdminEvent(address addr);
+    // Grantor indicates who authorizes the remove admin operation.
+    event RemoveAdminEvent(address addr, address grantor, string reason);
 
     /*
         Public Functions
@@ -63,36 +51,37 @@ contract Registrar {
     /**
      * @dev Get latest stable checkpoint information.
      * @return section index
-     * @return section head
+     * @return checkpoint hash
-     * @return cht root hash
-     * @return bloom trie root hash
      */
     function GetLatestCheckpoint()
     view
     public
-    returns(uint, bytes32, bytes32, bytes32) {
+    returns(uint, bytes32) {
-        (bytes32 sectionHead, bytes32 chtRoot, bytes32 bloomRoot) = GetCheckpoint(latest);
+        bytes32 hash = GetCheckpoint(latest);
-        return (latest, sectionHead, chtRoot, bloomRoot);
+        return (latest, hash);
     }
 
     /**
      * @dev Get a stable checkpoint information with specified section index.
      * @param _sectionIndex section index
-     * @return section head
+     * @return checkpoint hash
-     * @return cht root hash
-     * @return bloom trie root hash
      */
     function GetCheckpoint(uint _sectionIndex)
     view
     public
-    returns(bytes32, bytes32, bytes32)
+    returns(bytes32)
     {
-        Checkpoint memory checkpoint = checkpoints[_sectionIndex];
+        return checkpoints[_sectionIndex];
-        return (checkpoint.sectionHead, checkpoint.chtRoot, checkpoint.bloomTrieRoot);
     }
 
     /**
      * @dev Set stable checkpoint information.
+     * Checkpoint represents a set of post-processed trie roots (CHT and BloomTrie)
+     * associated with the appropriate section head hash.
+     *
+     * It is used to start light syncing from this checkpoint
+     * and avoid downloading the entire header chain while still being able to securely
+     * access old headers/logs.
      *
      * Note we trust the given information here provided by foundation,
      * need a trust less version for future.
@@ -113,7 +102,8 @@ contract Registrar {
     returns(bool)
     {
         // Ensure the checkpoint information provided is strictly continuous with previous one.
-        if (_sectionIndex != latest + 1 && latest != 0) {
+        // But the latest checkpoint modification is allowed.
+        if (_sectionIndex != latest && _sectionIndex != latest + 1 && latest != 0) {
             return false;
         }
         // Ensure the checkpoint is stable enough to be registered.
@@ -121,14 +111,10 @@ contract Registrar {
             return false;
         }
 
-        checkpoints[_sectionIndex] = Checkpoint({
+        checkpoints[_sectionIndex] = keccak256(abi.encodePacked(_sectionHead, _chtRoot, _bloomTrieRoot));
-            sectionHead:   _sectionHead,
-            chtRoot:       _chtRoot,
-            bloomTrieRoot: _bloomTrieRoot
-        });
         latest = _sectionIndex;
 
-        emit NewCheckpointEvent(_sectionIndex, _sectionHead, _chtRoot, _bloomTrieRoot);
+        emit NewCheckpointEvent(_sectionIndex, msg.sender, checkpoints[_sectionIndex]);
     }
 
     /**
@@ -136,7 +122,7 @@ contract Registrar {
      * @param _addr specified new admin address.
      * @return indicator whether add new admin successfully
      */
-    function AddAdmin(address _addr)
+    function AddAdmin(address _addr, string _description)
     OnlyAuthorized
     public
     returns(bool)
@@ -148,7 +134,7 @@ contract Registrar {
         admins[_addr] = 1;
         adminList.push(_addr);
 
-        emit AddAdminEvent(_addr);
+        emit AddAdminEvent(_addr, msg.sender, _description);
         return true;
     }
 
@@ -157,7 +143,7 @@ contract Registrar {
      * @param _addr specified admin address to remove.
      * @return indicator whether remove admin successfully
      */
-    function RemoveAdmin(address _addr)
+    function RemoveAdmin(address _addr, string _reason)
     OnlyAuthorized
     public
     returns(bool)
@@ -179,7 +165,7 @@ contract Registrar {
             }
         }
 
-        emit RemoveAdminEvent(_addr);
+        emit RemoveAdminEvent(_addr, msg.sender, _reason);
         return true;
     }
 
@@ -211,7 +197,7 @@ contract Registrar {
     address[] adminList;
 
     // Registered checkpoint information
-    mapping(uint => Checkpoint) checkpoints;
+    mapping(uint => bytes32) checkpoints;
 
     // Latest stored section id
     // Note all registered checkpoint information should continuous with previous one.
@@ -221,6 +207,8 @@ contract Registrar {
     uint constant sectionSize = 32768;
 
     // The number of confirmations needed before a checkpoint can be registered.
-    uint constant confirmations = 10000;
+    // We have to make sure the checkpoint registered will not be invalid due to
+    // chain reorg.
+    uint constant confirmations = 500;
 }
 

contracts/registrar/registrar.go

@@ -30,11 +30,11 @@ import (
 )
 
 var (
-	// registrar contract address for mainnet or test chain.
+	// registrar contract address for mainnet and testnet.
 	RegistrarAddr = map[common.Hash]common.Address{
 		params.MainnetGenesisHash: common.HexToAddress(""),
 		params.TestnetGenesisHash: common.HexToAddress(""),
-		params.RinkebyGenesisHash: common.HexToAddress("0x3b934494985d17bcb49557671e1bc8ec32cccdd5"),
+		params.RinkebyGenesisHash: common.HexToAddress("0xe3f2686a5d0c56a2d853c19c46b173a755263be8"),
 	}
 )
 
@@ -42,7 +42,7 @@ var errEventNotFound = errors.New("contract event not found")
 
 const (
 	sectionSize            = 32768 // The frequency for creating a checkpoint
-	checkpointConfirmation = 10000 // The number of confirmations needed before a checkpoint becoming stable.
+	checkpointConfirmation = 500   // The number of confirmations needed before a checkpoint can be accepted
 )
 
 type Registrar struct {

contracts/registrar/registrar_test.go

@@ -17,9 +17,11 @@
 package registrar
 
 import (
+	"errors"
 	"math/big"
 	"reflect"
 	"testing"
+	"time"
 
 	"github.com/ethereum/go-ethereum/accounts/abi/bind"
 	"github.com/ethereum/go-ethereum/accounts/abi/bind/backends"
@@ -43,101 +45,209 @@ var (
 	}
 )
 
+// validateOperation executes the operation, watches and delivers all events fired by the backend and ensures the
+// correctness by assert function.
+func validateOperation(t *testing.T, c *contract.Contract, backend *backends.SimulatedBackend, operation func(),
+	assert func(<-chan *contract.ContractNewCheckpointEvent, <-chan *contract.ContractAddAdminEvent, <-chan *contract.ContractRemoveAdminEvent) error, opName string) {
+	// Watch all events and deliver them to assert function
+	var (
+		sink1 = make(chan *contract.ContractNewCheckpointEvent)
+		sink2 = make(chan *contract.ContractAddAdminEvent)
+		sink3 = make(chan *contract.ContractRemoveAdminEvent)
+	)
+	sub1, _ := c.WatchNewCheckpointEvent(nil, sink1, nil)
+	sub2, _ := c.WatchAddAdminEvent(nil, sink2)
+	sub3, _ := c.WatchRemoveAdminEvent(nil, sink3)
+	defer func() {
+		// Close all subscribers
+		sub1.Unsubscribe()
+		sub2.Unsubscribe()
+		sub3.Unsubscribe()
+	}()
+	operation()
+
+	// flush pending block
+	backend.Commit()
+	if err := assert(sink1, sink2, sink3); err != nil {
+		t.Errorf("operation {%s} failed, err %s", opName, err)
+	}
+}
+
+// validateEvents checks that the correct number of contract events
+// fired by contract backend.
+func validateEvents(target int, sink interface{}) bool {
+	chanval := reflect.ValueOf(sink)
+	chantyp := chanval.Type()
+	if chantyp.Kind() != reflect.Chan || chantyp.ChanDir()&reflect.RecvDir == 0 {
+		return false
+	}
+	count := 0
+	timeout := time.After(1 * time.Second)
+	cases := []reflect.SelectCase{{Chan: chanval, Dir: reflect.SelectRecv}, {Chan: reflect.ValueOf(timeout), Dir: reflect.SelectRecv}}
+	for {
+		chose, _, _ := reflect.Select(cases)
+		if chose == 1 {
+			// Not enough event received
+			return false
+		}
+		count += 1
+		if count == target {
+			break
+		}
+	}
+	done := time.After(50 * time.Millisecond)
+	cases = cases[:1]
+	cases = append(cases, reflect.SelectCase{Chan: reflect.ValueOf(done), Dir: reflect.SelectRecv})
+	chose, _, _ := reflect.Select(cases)
+	// If chose equal 0, it means receiving redundant events.
+	return chose == 1
+}
+
+// Tests contract administrator managements.
 func TestAdminManagement(t *testing.T) {
 	var (
-		adminCandidate  = common.HexToAddress("0x123")
+		adminCandidate  = common.HexToAddress("0xdead")
-		adminCandidate2 = common.HexToAddress("0x456")
+		adminCandidate2 = common.HexToAddress("0xbeef")
 	)
 
 	// Deploy registrar contract
 	transactOpts := bind.NewKeyedTransactor(key)
 	contractBackend := backends.NewSimulatedBackend(core.GenesisAlloc{addr: {Balance: big.NewInt(1000000000)}})
-	_, _, contract, err := contract.DeployContract(transactOpts, contractBackend, nil)
+	_, _, c, err := contract.DeployContract(transactOpts, contractBackend, nil)
 	if err != nil {
 		t.Error("deploy registrar contract failed", err)
 	}
 	contractBackend.Commit()
 
 	// Test AddAdmin function
-	contract.AddAdmin(transactOpts, addr) // Contract should ignore the duplicate registration
+	validateOperation(t, c, contractBackend, func() {
-	contract.AddAdmin(transactOpts, adminCandidate)
+		for _, a := range []common.Address{addr, adminCandidate, adminCandidate2} {
-	contract.AddAdmin(transactOpts, adminCandidate2)
+			c.AddAdmin(transactOpts, a, "")
-	contractBackend.Commit()
+		}
-	adminList, err := contract.GetAllAdmin(nil)
+	}, func(sink1 <-chan *contract.ContractNewCheckpointEvent, sink2 <-chan *contract.ContractAddAdminEvent, sink3 <-chan *contract.ContractRemoveAdminEvent) error {
-	if err != nil {
+		adminList, err := c.GetAllAdmin(nil)
-		t.Error("fetch admin list failed", err)
+		if err != nil {
-	}
+			return errors.New("get admin list failed")
-	if !reflect.DeepEqual(adminList, []common.Address{addr, adminCandidate, adminCandidate2}) {
+		}
-		t.Error("expect the returned admin list contain 3 address")
+		if !reflect.DeepEqual(adminList, []common.Address{addr, adminCandidate, adminCandidate2}) {
-	}
+			return errors.New("add admin failed")
+		}
+		if !validateEvents(2, sink2) {
+			return errors.New("receive incorrect number of events")
+		}
+		return nil
+	}, "add admin")
 
-	// Test RemoveAdmin function (remove at the middle)
+	// Test Remove admin function
-	contract.RemoveAdmin(transactOpts, adminCandidate)
+	validateOperation(t, c, contractBackend, func() {
-	contractBackend.Commit()
+		c.RemoveAdmin(transactOpts, adminCandidate, "")
-	adminList, err = contract.GetAllAdmin(nil)
+	}, func(events <-chan *contract.ContractNewCheckpointEvent, events2 <-chan *contract.ContractAddAdminEvent, events3 <-chan *contract.ContractRemoveAdminEvent) error {
-	if err != nil {
+		adminList, err := c.GetAllAdmin(nil)
-		t.Error("fetch admin list failed", err)
+		if err != nil {
-	}
+			return errors.New("get admin list failed")
-	if !reflect.DeepEqual(adminList, []common.Address{addr, adminCandidate2}) {
+		}
-		t.Error("expect the returned admin list contain 3 address")
+		if !reflect.DeepEqual(adminList, []common.Address{addr, adminCandidate2}) {
-	}
+			return errors.New("remove admin failed")
+		}
+		if !validateEvents(1, events3) {
+			return errors.New("receive incorrect number of events")
+		}
+		return nil
+	}, "remove admin at middle")
 
 	// Test RemoveAdmin function (remove at the head)
-	contract.RemoveAdmin(transactOpts, addr)
+	validateOperation(t, c, contractBackend, func() {
-	contractBackend.Commit()
+		c.RemoveAdmin(transactOpts, addr, "")
-	adminList, err = contract.GetAllAdmin(nil)
+	}, func(events <-chan *contract.ContractNewCheckpointEvent, events2 <-chan *contract.ContractAddAdminEvent, events3 <-chan *contract.ContractRemoveAdminEvent) error {
-	if err != nil {
+		adminList, err := c.GetAllAdmin(nil)
-		t.Error("fetch admin list failed", err)
+		if err != nil {
-	}
+			return errors.New("get admin list failed")
-	if !reflect.DeepEqual(adminList, []common.Address{adminCandidate2}) {
+		}
-		t.Error("expect the returned admin list contain 3 address")
+		if !reflect.DeepEqual(adminList, []common.Address{adminCandidate2}) {
-	}
+			return errors.New("remove admin failed")
+		}
+		if !validateEvents(1, events3) {
+			return errors.New("receive incorrect number of events")
+		}
+		return nil
+	}, "remove admin at head")
 
 	// Test unauthorized operation
-	contract.AddAdmin(transactOpts, adminCandidate)
+	validateOperation(t, c, contractBackend, func() {
-	contractBackend.Commit()
+		c.AddAdmin(transactOpts, adminCandidate, "")
-	adminList, err = contract.GetAllAdmin(nil)
+	}, func(events <-chan *contract.ContractNewCheckpointEvent, events2 <-chan *contract.ContractAddAdminEvent, events3 <-chan *contract.ContractRemoveAdminEvent) error {
-	if err != nil {
+		adminList, err := c.GetAllAdmin(nil)
-		t.Error("fetch admin list failed", err)
+		if err != nil {
-	}
+			return errors.New("get admin list failed")
-	if !reflect.DeepEqual(adminList, []common.Address{adminCandidate2}) {
+		}
-		t.Error("expect the returned admin list contain 3 address")
+		if !reflect.DeepEqual(adminList, []common.Address{adminCandidate2}) {
-	}
+			return errors.New("unauthorized operation should be banned")
+		}
+		return nil
+	}, "unauthorized operation")
 }
 
+// Tests checkpoint managements.
 func TestCheckpointRegister(t *testing.T) {
 	// Deploy registrar contract
 	transactOpts := bind.NewKeyedTransactor(key)
 	contractBackend := backends.NewSimulatedBackend(core.GenesisAlloc{addr: {Balance: big.NewInt(1000000000)}})
-	_, _, contract, err := contract.DeployContract(transactOpts, contractBackend, []common.Address{addr})
+	_, _, c, err := contract.DeployContract(transactOpts, contractBackend, []common.Address{addr})
 	if err != nil {
 		t.Error("deploy registrar contract failed", err)
 	}
 	contractBackend.Commit()
 
-	// Register an unstable checkpoint
+	// Register unstable checkpoint
-	contract.SetCheckpoint(transactOpts, big.NewInt(int64(trustedCheckpoint.SectionIdx)), trustedCheckpoint.SectionHead,
+	validateOperation(t, c, contractBackend, func() {
-		trustedCheckpoint.ChtRoot, trustedCheckpoint.BloomTrieRoot)
+		c.SetCheckpoint(transactOpts, big.NewInt(int64(trustedCheckpoint.SectionIdx)), trustedCheckpoint.SectionHead,
-	contractBackend.Commit()
+			trustedCheckpoint.ChtRoot, trustedCheckpoint.BloomTrieRoot)
-	head, chtRoot, bloomTrieRoot, err := contract.GetCheckpoint(nil, big.NewInt(int64(trustedCheckpoint.SectionIdx)))
+	}, func(events <-chan *contract.ContractNewCheckpointEvent, events2 <-chan *contract.ContractAddAdminEvent, events3 <-chan *contract.ContractRemoveAdminEvent) error {
-	if err != nil {
+		hash, err := c.GetCheckpoint(nil, big.NewInt(int64(trustedCheckpoint.SectionIdx)))
-		t.Error("fetch checkpoint failed", err)
+		if err != nil {
-	}
+			return errors.New("get checkpoint failed")
-	if head != emptyHash || chtRoot != emptyHash || bloomTrieRoot != emptyHash {
+		}
-		t.Error("the unstable checkpoint is not allowed to be registered")
+		if hash != emptyHash {
-	}
+			return errors.New("unstable checkpoint should be banned")
+		}
+		return nil
+	}, "register unstable checkpoint")
 
 	// Register a stable checkpoint
-	contractBackend.ShiftBlocks(sectionSize + checkpointConfirmation)
+	validateOperation(t, c, contractBackend, func() {
-	contract.SetCheckpoint(transactOpts, big.NewInt(int64(trustedCheckpoint.SectionIdx)), trustedCheckpoint.SectionHead,
+		contractBackend.ShiftBlocks(sectionSize + checkpointConfirmation)
-		trustedCheckpoint.ChtRoot, trustedCheckpoint.BloomTrieRoot)
+		c.SetCheckpoint(transactOpts, big.NewInt(int64(trustedCheckpoint.SectionIdx)), trustedCheckpoint.SectionHead,
-	contractBackend.Commit()
+			trustedCheckpoint.ChtRoot, trustedCheckpoint.BloomTrieRoot)
-	head, chtRoot, bloomTrieRoot, err = contract.GetCheckpoint(nil, big.NewInt(int64(trustedCheckpoint.SectionIdx)))
+	}, func(events <-chan *contract.ContractNewCheckpointEvent, events2 <-chan *contract.ContractAddAdminEvent, events3 <-chan *contract.ContractRemoveAdminEvent) error {
-	if err != nil {
+		hash, err := c.GetCheckpoint(nil, big.NewInt(int64(trustedCheckpoint.SectionIdx)))
-		t.Error("fetch checkpoint failed", err)
+		if err != nil {
-	}
+			return errors.New("get checkpoint failed")
-	if !reflect.DeepEqual(head[:], trustedCheckpoint.SectionHead.Bytes()) || !reflect.DeepEqual(chtRoot[:], trustedCheckpoint.ChtRoot.Bytes()) ||
+		}
-		!reflect.DeepEqual(bloomTrieRoot[:], trustedCheckpoint.BloomTrieRoot.Bytes()) {
+		if common.Hash(hash).Hex() != crypto.Keccak256Hash(trustedCheckpoint.SectionHead.Bytes(), trustedCheckpoint.ChtRoot.Bytes(), trustedCheckpoint.BloomTrieRoot.Bytes()).Hex() {
-		t.Error("expect the returned checkpoint should be same with the given one")
+			return errors.New("register stable checkpoint failed")
-	}
+		}
+		if !validateEvents(1, events) {
+			return errors.New("receive incorrect number of events")
+		}
+		return nil
+	}, "register stable checkpoint")
+
+	// Modify the latest checkpoint
+	validateOperation(t, c, contractBackend, func() {
+		trustedCheckpoint.SectionHead = common.HexToHash("dead")
+		c.SetCheckpoint(transactOpts, big.NewInt(int64(trustedCheckpoint.SectionIdx)), trustedCheckpoint.SectionHead,
+			trustedCheckpoint.ChtRoot, trustedCheckpoint.BloomTrieRoot)
+	}, func(events <-chan *contract.ContractNewCheckpointEvent, events2 <-chan *contract.ContractAddAdminEvent, events3 <-chan *contract.ContractRemoveAdminEvent) error {
+		hash, err := c.GetCheckpoint(nil, big.NewInt(int64(trustedCheckpoint.SectionIdx)))
+		if err != nil {
+			return errors.New("get checkpoint failed")
+		}
+		if common.Hash(hash).Hex() != crypto.Keccak256Hash(trustedCheckpoint.SectionHead.Bytes(), trustedCheckpoint.ChtRoot.Bytes(), trustedCheckpoint.BloomTrieRoot.Bytes()).Hex() {
+			return errors.New("register stable checkpoint failed")
+		}
+		if !validateEvents(1, events) {
+			return errors.New("receive incorrect number of events")
+		}
+		return nil
+	}, "modify latest checkpoint")
 }

les/server.go

@@ -136,7 +136,7 @@ func (s *LesServer) Start(srvr *p2p.Server) {
 	s.privateKey = srvr.PrivateKey
 	s.protocolManager.blockLoop()
 	if s.registrar != nil {
-		s.checkpointLoop()
+		go s.checkpointLoop()
 	}
 }
 
@@ -154,10 +154,9 @@ func (s *LesServer) Stop() {
 		<-s.protocolManager.noMorePeers
 	}()
 	s.protocolManager.Stop()
-	close(s.quitSync)
 }
 
-// checkpointLoop starts a standalone goroutine to watch new checkpoint event and upgrades local's stable checkpoint.
+// checkpointLoop starts a standalone goroutine to watch new checkpoint event and updates local's stable checkpoint.
 func (s *LesServer) checkpointLoop() (err error) {
 	sink := make(chan *contract.ContractNewCheckpointEvent)
 	sub, err := s.registrar.WatchNewCheckpointEvent(sink)
@@ -171,19 +170,12 @@ func (s *LesServer) checkpointLoop() (err error) {
 	for {
 		select {
 		case event := <-sink:
-			// New stable checkpoint received
+			// Note several duplicate events can be received because of latest checkpoint modification is allowed.
-			// Note several duplicate events can be received due to chain reorg, just track the first arrive one.
+			// Always update local checkpoint when the section index is not less than the local one.
-			if event.Index.Uint64() > s.stableCheckpoint.SectionIdx {
+			// todo(rjl493456442) update local checkpoint
-				checkpoint := &light.TrustedCheckpoint{
+			if event.Index.Uint64() >= s.stableCheckpoint.SectionIdx {
-					SectionIdx:    event.Index.Uint64(),
+				log.Info("update checkpoint", "section", event.Index, "hash", common.Hash(event.CheckpointHash).Hex(),
-					SectionHead:   common.Hash(event.SectionHead),
+					"grantor", event.Grantor.Hex())
-					ChtRoot:       common.Hash(event.ChtRoot),
-					BloomTrieRoot: common.Hash(event.BloomTrieRoot),
-				}
-				light.WriteTrustedCheckpoint(s.protocolManager.chainDb, checkpoint)
-				s.stableCheckpoint = checkpoint
-				log.Info("update checkpoint", "section", checkpoint.SectionIdx, "head", checkpoint.SectionHead.Hex(),
-					"chtRoot", checkpoint.ChtRoot.Hex(), "bloomTrieRoot", checkpoint.BloomTrieRoot.Hex())
 			}
 
 		case <-s.quitSync: