Fix spurious reporting of baker double signing

CHANGELOG.md

@@ -2,6 +2,9 @@
 
 ## Unreleased changes
 
+- Fix a bug where receiving a duplicate of an invalid block could be spuriously reported as double
+  signing.
+
 ## 6.0.1
 
 - Remove configuration option `no-rpc-server` and environment variable

concordium-consensus/src/Concordium/KonsensusV1/Consensus/Blocks.hs

@@ -278,16 +278,25 @@ receiveBlockKnownParent parent pendingBlock = do
             let blockWitness = toBlockSignatureWitness (pbBlock pendingBlock)
             -- Check if we've already seen a signed block from this baker in this round.
             use (roundBakerExistingBlock (blockRound pendingBlock) (blockBaker pendingBlock)) >>= \case
-                Just w -> do
-                    -- If the baker has already signed a block in this round then we flag it.
-                    logEvent Konsensus LLDebug $
-                        "Baker "
-                            <> show (blockBaker pendingBlock)
-                            <> " signed multiple blocks in round "
-                            <> show (blockRound pendingBlock)
-                            <> "."
-                    flag (DuplicateBlock w blockWitness)
-                    return $ BlockResultDoubleSign verifiedBlock
+                Just w
+                    | bswBlockHash w == pbHash -> do
+                        -- This case is uncommon, as duplicates are typically detected earlier, but
+                        -- it can happen if:
+                        --   1. the block bypasses de-duplication (i.e. as a direct message), and
+                        --   2. the block is invalid, but has a valid baker signature.
+                        -- We thus check if the block is actually distinct so that we do not
+                        -- spuriously report it as a double signing.
+                        return BlockResultDuplicate
+                    | otherwise -> do
+                        -- If the baker has already signed a block in this round then we flag it.
+                        logEvent Konsensus LLDebug $
+                            "Baker "
+                                <> show (blockBaker pendingBlock)
+                                <> " signed multiple blocks in round "
+                                <> show (blockRound pendingBlock)
+                                <> "."
+                        flag (DuplicateBlock w blockWitness)
+                        return $ BlockResultDoubleSign verifiedBlock
                 Nothing -> do
                     -- If the baker has not already signed a block in this round, we record that
                     -- it has now and return control to the caller for retransmitting the block.

concordium-consensus/src/Concordium/KonsensusV1/Types.hs

@@ -1352,11 +1352,12 @@ unsafeGetBlockKnownHash ts sbHash = do
 -- |Nominally, a proof that a baker signed a block in a particular round and epoch.
 -- For now, though, we do not include any information in the witness since we do not provide it to
 -- any external parties.
-data BlockSignatureWitness = BlockSignatureWitness
+newtype BlockSignatureWitness = BlockSignatureWitness {bswBlockHash :: BlockHash}
+    deriving (Eq, Show)
 
 -- |Derive a 'BlockSignatureWitness' from a signed block.
 toBlockSignatureWitness :: SignedBlock -> BlockSignatureWitness
-toBlockSignatureWitness _ = BlockSignatureWitness
+toBlockSignatureWitness = BlockSignatureWitness . getHash
 
 -- |A proof that contains the 'Epoch' for a 'QuorumCertificate'
 -- has been checked for a particular 'Round'.

concordium-consensus/tests/consensus/ConcordiumTests/KonsensusV1/Consensus/Blocks.hs

@@ -724,6 +724,14 @@ testReceiveDuplicate = runTestMonad noBaker testTime genesisData $ do
     res <- uponReceivingBlock $ signedPB testBB1
     liftIO $ res `shouldBe` BlockResultDuplicate
 
+-- |Receive an invalid block twice.
+testReceiveInvalidDuplicate :: Assertion
+testReceiveInvalidDuplicate = runTestMonad noBaker testTime genesisData $ do
+    let badBlock = signedPB (testBB1{bbStateHash = StateHashV0 minBound})
+    succeedReceiveBlockFailExecute badBlock
+    res <- uponReceivingBlock $ badBlock
+    liftIO $ res `shouldBe` BlockResultDuplicate
+
 testReceiveStale :: Assertion
 testReceiveStale = runTestMonad noBaker testTime genesisData $ do
     mapM_ (succeedReceiveBlock . signedPB) [testBB2', testBB3', testBB4']
@@ -1253,6 +1261,7 @@ tests = describe "KonsensusV1.Consensus.Blocks" $ do
         it "receive 4 blocks reordered, multiple epochs" testReceive4Reordered
         it "skip round 1, receive rounds 2,3,4" testReceiveWithTimeout
         it "receive duplicate block" testReceiveDuplicate
+        it "receive invalid duplicate block" testReceiveInvalidDuplicate
         it "receive stale round" testReceiveStale
         it "epoch transition" testReceiveEpoch
         it "block dies on old branch" testReceiveBlockDies