Align blocks difficulty

src/main/java/co/rsk/federate/signing/hsm/advanceblockchain/ConfirmedBlocksProvider.java

@@ -34,43 +34,46 @@ public ConfirmedBlocksProvider(
     }
 
     public List<Block> getConfirmedBlocks(Keccak256 startingPoint) {
-        List<Block> potentialBlocks = new ArrayList<>();
-        List<Block> confirmedBlocks = new ArrayList<>();
-        BigInteger accumulatedDifficulty = BigInteger.ZERO;
-
         Block initialBlock = blockStore.getBlockByHash(startingPoint.getBytes());
+        long initialBlockNumber = initialBlock.getNumber();
         Block bestBlock = blockStore.getBestBlock();
         logger.trace(
             "[getConfirmedBlocks] Initial block height is {} and RSK best block height {}. Using HSM version {}, difficulty target {}, difficulty cap {}, sending max {} elements",
-            initialBlock.getNumber(),
+            initialBlockNumber,
             bestBlock.getNumber(),
             hsmVersion,
             minimumAccumulatedDifficulty,
             difficultyCap,
             maximumElementsToSendHSM
         );
 
-        int lastIndexToConfirmBlock = 0;
-        Block blockToProcess = blockStore.getChainBlockByNumber(initialBlock.getNumber() + 1);
+        List<Block> blocksInWindow = new ArrayList<>();
+        int proofBlocksCount = 0;
+        BigInteger accumulatedDifficulty = BigInteger.ZERO;
+        List<Block> confirmedBlocks = new ArrayList<>();
+
+        Block blockToProcess = blockStore.getChainBlockByNumber(initialBlockNumber + 1);
         while (blockToProcess != null && confirmedBlocks.size() < maximumElementsToSendHSM) {
-            potentialBlocks.add(blockToProcess);
-            BigInteger difficultyToConsider = getBlockDifficultyToConsider(blockToProcess);
-            accumulatedDifficulty = accumulatedDifficulty.add(difficultyToConsider);
+            blocksInWindow.add(blockToProcess);
+            BigInteger totalDifficulty = getBlockTotalDifficulty(blockToProcess, initialBlockNumber);
+            accumulatedDifficulty = accumulatedDifficulty.add(totalDifficulty);
 
-            if (accumulatedDifficulty.compareTo(minimumAccumulatedDifficulty) >= 0) { // Enough difficulty accumulated
+            boolean enoughDifficulty = accumulatedDifficulty.compareTo(minimumAccumulatedDifficulty) >= 0;
+            if (enoughDifficulty) {
                 logger.trace(
                     "[getConfirmedBlocks] Accumulated enough difficulty {} with {} blocks",
                     accumulatedDifficulty,
-                    potentialBlocks.size()
+                    blocksInWindow.size()
                 );
 
-                // The first block was confirmed. Add it to confirm, subtract its difficulty from the accumulated and from the potentials list
-                Block confirmedBlock = potentialBlocks.get(0);
+                // The block was confirmed. Add it to confirmed blocks list,
+                // subtract its difficulty from the accumulated and remove it from the proof blocks list
+                Block confirmedBlock = blocksInWindow.get(0);
                 confirmedBlocks.add(confirmedBlock);
-                BigInteger confirmedBlockDifficultyToConsider = getBlockDifficultyToConsider(confirmedBlock);
-                accumulatedDifficulty = accumulatedDifficulty.subtract(confirmedBlockDifficultyToConsider);
-                potentialBlocks.remove(confirmedBlock);
-                lastIndexToConfirmBlock = potentialBlocks.size();
+                BigInteger confirmedBlockTotalDifficulty = getBlockTotalDifficulty(confirmedBlock, initialBlockNumber);
+                accumulatedDifficulty = accumulatedDifficulty.subtract(confirmedBlockTotalDifficulty);
+                blocksInWindow.remove(confirmedBlock);
+                proofBlocksCount = blocksInWindow.size();
 
                 logger.trace(
                     "[getConfirmedBlocks] Confirmed block {} (height {})",
@@ -85,33 +88,30 @@ public List<Block> getConfirmedBlocks(Keccak256 startingPoint) {
         if (confirmedBlocks.isEmpty()) {
             return confirmedBlocks;
         }
-        // Adding the proof of the confirmed elements from the potential elements
-        potentialBlocks = potentialBlocks.subList(0, lastIndexToConfirmBlock);
-        confirmedBlocks.addAll(potentialBlocks);
-        logger.debug("[getConfirmedBlocks] Added {} extra blocks as proof", potentialBlocks.size());
+        // Adding the proof of the confirmed elements from the blocks remaining in the window
+        blocksInWindow = blocksInWindow.subList(0, proofBlocksCount);
+        confirmedBlocks.addAll(blocksInWindow);
+        logger.debug("[getConfirmedBlocks] Added {} extra blocks as proof", blocksInWindow.size());
 
         return confirmedBlocks;
     }
 
-    protected BigInteger getBlockDifficultyToConsider(Block block) {
+    protected BigInteger getBlockTotalDifficulty(Block block, long uncleHeightThreshold) {
         logger.trace(
-            "[getBlockDifficultyToConsider] Get difficulty for block {} at height {}",
+            "[getBlockTotalDifficulty] Get total difficulty for block {} at height {}",
             block.getHash(),
             block.getNumber()
         );
 
-        BigInteger blockTotalDifficulty = block.getDifficulty().asBigInteger();
-
-        BigInteger blockDifficultyToConsider = difficultyCap.min(blockTotalDifficulty);
-        BigInteger unclesDifficultyToConsider = block.getUncleList().stream()
+        BigInteger blockDifficulty = difficultyCap.min(block.getDifficulty().asBigInteger());
+        // Each block uncle is sent to the HSM as a brother of the respective canonical block
+        // it shares a parent with, which is part of the set being sent only when the
+        // original block's uncle is above the HSM best block.
+        // So only those uncles can be delivered as brothers and counted.
+        BigInteger unclesDifficulty = block.getUncleList().stream()
+            .filter(uncle -> uncle.getNumber() > uncleHeightThreshold)
             .map(uncle -> difficultyCap.min(uncle.getDifficulty().asBigInteger()))
             .reduce(BigInteger.ZERO, BigInteger::add);
-
-        logger.trace(
-            "[getBlockDifficultyToConsider] Block difficulty {}, considering {}",
-            blockTotalDifficulty,
-            blockDifficultyToConsider
-        );
-        return blockDifficultyToConsider.add(unclesDifficultyToConsider);
+        return blockDifficulty.add(unclesDifficulty);
     }
 }

src/main/java/co/rsk/federate/signing/hsm/advanceblockchain/HSMBookkeepingService.java

@@ -154,24 +154,27 @@ protected void informConfirmedBlockHeaders() {
                 hsmCurrentBestBlock.getNumber()
             );
 
-            List<Block> blocks = this.confirmedBlocksProvider.getConfirmedBlocks(hsmCurrentBestBlock.getHash());
-            if (blocks.isEmpty()) {
+            List<Block> confirmedBlocks = this.confirmedBlocksProvider.getConfirmedBlocks(hsmCurrentBestBlock.getHash());
+            if (confirmedBlocks.isEmpty()) {
                 logger.debug("[informConfirmedBlockHeaders] No new block headers to inform");
                 logger.info("[informConfirmedBlockHeaders] Finished HSM bookkeeping process");
                 informing = false;
                 return;
             }
 
+            int confirmedBlocksSize = confirmedBlocks.size();
+            Block firstConfirmedBlock = confirmedBlocks.get(0);
+            Block lastConfirmedBlock = confirmedBlocks.get(confirmedBlocksSize - 1);
             logger.debug(
                     "[informConfirmedBlockHeaders] Going to inform {} block headers. From block number {} with hash {} to block number {} with hash {}",
-                    blocks.size(),
-                    blocks.get(0).getNumber(),
-                    blocks.get(0).getHash(),
-                    blocks.get(blocks.size() - 1).getNumber(),
-                    blocks.get(blocks.size() - 1).getHash()
+                    confirmedBlocksSize,
+                    firstConfirmedBlock.getNumber(),
+                    firstConfirmedBlock.getHash(),
+                    lastConfirmedBlock.getNumber(),
+                    lastConfirmedBlock.getHash()
             );
 
-            hsmBookkeepingClient.advanceBlockchain(blocks);
+            hsmBookkeepingClient.advanceBlockchain(confirmedBlocks);
             hsmCurrentBestBlock = getHsmBestBlock();
             logger.debug(
                 "[informConfirmedBlockHeaders] HSM best block after informing {} (height: {})",

src/main/java/co/rsk/federate/signing/hsm/advanceblockchain/HsmBookkeepingClientImpl.java

@@ -43,7 +43,7 @@ public class HsmBookkeepingClientImpl implements HSMBookkeepingClient {
     private static final Logger logger = LoggerFactory.getLogger(HsmBookkeepingClientImpl.class);
     private final HSMClientProtocol hsmClientProtocol;
     private final HSMVersion hsmVersion;
-    private int maxChunkSize = 10;  // DEFAULT VALUE
+    private int maxChunkSize = 10; // DEFAULT VALUE
     private boolean isStopped = false;
 
     public HsmBookkeepingClientImpl(HSMClientProtocol hsmClientProtocol) throws HSMClientException {
@@ -126,9 +126,12 @@ private void addBrothersToPayload(ObjectNode payload, List<String[]> brothers) {
         payload.set(BROTHERS.getFieldName(), brothersFieldData);
     }
 
-    private List<String[]> getBrothers(String[] blockHeaderChunk, AdvanceBlockchainMessage message)
-        throws HSMBlockchainBookkeepingRelatedException {
+    private List<String[]> getBrothers(
+        String[] blockHeaderChunk,
+        AdvanceBlockchainMessage message
+    ) throws HSMBlockchainBookkeepingRelatedException {
         List<String[]> brothers = new ArrayList<>();
+
         for (String blockHeader : blockHeaderChunk) {
             brothers.add(message.getParsedBrothers(blockHeader));
         }
@@ -163,16 +166,19 @@ public void updateAncestorBlock(UpdateAncestorBlockMessage updateAncestorBlockMe
     }
 
     @Override
-    public void advanceBlockchain(List<Block> blocks) throws HSMClientException {
-        AdvanceBlockchainMessage message = new AdvanceBlockchainMessage(blocks);
+    public void advanceBlockchain(List<Block> confirmedBlocks) throws HSMClientException {
+        String advanceBlockchain = ADVANCE_BLOCKCHAIN.getCommand();
+
+        AdvanceBlockchainMessage message = new AdvanceBlockchainMessage(confirmedBlocks);
         List<String> blockHeaders = message.getParsedBlockHeaders();
-        validateHSMStateAndBlockHeaders(blockHeaders, ADVANCE_BLOCKCHAIN.getCommand());
+        validateHSMStateAndBlockHeaders(blockHeaders, advanceBlockchain);
         List<String[]> blockHeadersChunks = getChunks(blockHeaders.toArray(new String[]{}), maxChunkSize, false);
 
-        logger.trace("[advanceBlockchain] Going to send {} headers in {} chunks.", blockHeaders.size(), blockHeadersChunks.size());
-        for (int i = 0; i < blockHeadersChunks.size(); i++) {
+        int blockHeadersChunksSize = blockHeadersChunks.size();
+        logger.trace("[advanceBlockchain] Going to send {} headers in {} chunks.", blockHeaders.size(), blockHeadersChunksSize);
+        for (int i = 0; i < blockHeadersChunksSize; i++) {
             String[] blockHeaderChunk = blockHeadersChunks.get(i);
-            ObjectNode payload = this.hsmClientProtocol.buildCommand(ADVANCE_BLOCKCHAIN.getCommand(), hsmVersion);
+            ObjectNode payload = this.hsmClientProtocol.buildCommand(advanceBlockchain, hsmVersion);
             addBlocksToPayload(payload, blockHeaderChunk);
 
             List<String[]> brothers = getBrothers(blockHeaderChunk, message);

src/main/java/co/rsk/federate/signing/hsm/message/AdvanceBlockchainMessage.java

@@ -14,48 +14,49 @@ public class AdvanceBlockchainMessage {
     protected static final int BROTHERS_LIMIT_PER_BLOCK_HEADER = 10;
     private final List<ParsedHeader> parsedHeaders;
 
-    public AdvanceBlockchainMessage(List<Block> blocks) {
-        this.parsedHeaders = parseHeadersAndBrothers(blocks);
+    public AdvanceBlockchainMessage(List<Block> confirmedBlocks) {
+        this.parsedHeaders = parseHeadersAndBrothers(confirmedBlocks);
     }
 
     public List<String> getParsedBlockHeaders() {
-        return this.parsedHeaders.stream().map(ParsedHeader::getBlockHeader).collect(Collectors.toList());
+        return this.parsedHeaders.stream().map(ParsedHeader::getBlockHeader).toList();
     }
 
     public String[] getParsedBrothers(String blockHeader) throws HSMBlockchainBookkeepingRelatedException {
         return this.parsedHeaders.stream()
             .filter(header -> header.getBlockHeader().equals(blockHeader))
             .findFirst()
             .map(ParsedHeader::getBrothers)
-            .orElseThrow(() -> new HSMBlockchainBookkeepingRelatedException("Error while trying to get brothers for block header. Could not find header: " + blockHeader));
+            .orElseThrow(
+                () -> new HSMBlockchainBookkeepingRelatedException("Error while trying to get brothers for block header. Could not find header " + blockHeader)
+            );
     }
 
-    private List<ParsedHeader> parseHeadersAndBrothers(List<Block> blocks) {
-        Map<Keccak256, List<BlockHeader>> brothersByParentHash = groupBrothersByParentHash(blocks);
+    private List<ParsedHeader> parseHeadersAndBrothers(List<Block> confirmedBlocks) {
+        Map<Keccak256, List<BlockHeader>> brothersByParentHash = groupBrothersByParentHash(confirmedBlocks);
 
-        return blocks.stream()
+        return confirmedBlocks.stream()
             .sorted(Comparator.comparingLong(Block::getNumber).reversed()) // sort blocks from latest to oldest
             .map(block -> new ParsedHeader(
                 block.getHeader(),
-                filterBrothers(brothersByParentHash.getOrDefault(block.getParentHash(), Collections.emptyList()))
-            )).collect(Collectors.toList());
+                capAmountOfBrothers(brothersByParentHash.getOrDefault(block.getParentHash(), Collections.emptyList()))
+            )).toList();
     }
 
-    private Map<Keccak256, List<BlockHeader>> groupBrothersByParentHash(List<Block> blocks) {
-        return blocks.stream()
+    private Map<Keccak256, List<BlockHeader>> groupBrothersByParentHash(List<Block> confirmedBlocks) {
+        return confirmedBlocks.stream()
             .skip(1) // Skip the oldest block (index 0) because its uncles doesn't belong to this set of blocks
             .flatMap(block -> block.getUncleList().stream())
             .collect(Collectors.groupingBy(BlockHeader::getParentHash));
     }
 
-    private List<BlockHeader> filterBrothers(List<BlockHeader> brothers) {
+    private List<BlockHeader> capAmountOfBrothers(List<BlockHeader> brothers) {
         if (brothers.size() <= BROTHERS_LIMIT_PER_BLOCK_HEADER) {
             return brothers;
         }
         return brothers.stream()
-            .sorted((brother1, brother2) ->
-                brother2.getDifficulty().asBigInteger().compareTo(brother1.getDifficulty().asBigInteger()))
+            .sorted((brother1, brother2) -> brother2.getDifficulty().compareTo(brother1.getDifficulty()))
             .limit(BROTHERS_LIMIT_PER_BLOCK_HEADER)
-            .collect(Collectors.toList());
+            .toList();
     }
 }

src/test/java/co/rsk/federate/signing/hsm/advanceblockchain/ConfirmedBlocksProviderTest.java

@@ -182,7 +182,7 @@ void getConfirmedBlocks_belowDifficultyCap_ok() {
     }
 
     @Test
-    void getBlockDifficultyToConsider_considersUnclesAndCapDifficulty() {
+    void getBlockTotalDifficulty_considersUnclesAndCapDifficulty() {
         // arrange
         Block block = buildBlockWithUncles();
 
@@ -196,16 +196,48 @@ void getBlockDifficultyToConsider_considersUnclesAndCapDifficulty() {
         );
 
         // act
-        BigInteger consideredDifficulty = confirmedBlocksProvider.getBlockDifficultyToConsider(block);
+        int bestBlockHeight = 1;
+        BigInteger totalDifficulty = confirmedBlocksProvider.getBlockTotalDifficulty(block, bestBlockHeight);
 
         // assert
         // Pow HSM considers brothers difficulty
         // 7000000000000000000001 difficulty round to 7000000000000000000000 from block 4
         // + 1000000000000000000000 difficulty from block 2 (uncle)
         // + 8000000000000000000000 difficulty round to 7000000000000000000000 from block 3 (uncle)
         // = 15000000000000000000000 considered difficulty
-        BigInteger expectedConsideredDifficulty = new BigInteger("15000000000000000000000");
-        assertEquals(expectedConsideredDifficulty, consideredDifficulty);
+        BigInteger expectedTotalDifficulty = new BigInteger("15000000000000000000000");
+        assertEquals(expectedTotalDifficulty, totalDifficulty);
+    }
+
+    @Test
+    void getBlockTotalDifficulty_ignoresUnclesNotAboveHsmBestBlock() {
+        // arrange
+        Block block = buildBlockWithUncles();
+
+        // build blocks provider for Pow HSM
+        ConfirmedBlocksProvider confirmedBlocksProvider = new ConfirmedBlocksProvider(
+            BigInteger.valueOf(160),
+            100,
+            mock(BlockStore.class),
+            MAINNET.getDifficultyCap(),
+            hsmVersion
+        );
+
+        // act
+        // With the HSM best block at height 2, the uncle at height 2 is a sibling of the HSM best
+        // block: its canonical sibling is not part of the set being sent, so it cannot be delivered
+        // as a brother and its difficulty must be ignored. Only the uncle at height 3 is counted.
+        int bestBlockHeight = 2;
+        BigInteger totalDifficulty = confirmedBlocksProvider.getBlockTotalDifficulty(block, bestBlockHeight);
+
+        // assert
+        // Pow HSM considers brothers difficulty
+        // 7000000000000000000001 difficulty round to 7000000000000000000000 from block 4
+        // + 8000000000000000000000 difficulty round to 7000000000000000000000 from block 3 (uncle)
+        // block 2 (uncle at height 2) is NOT counted because it cannot be sent as a brother
+        // = 14000000000000000000000 considered difficulty
+        BigInteger expectedTotalDifficulty = new BigInteger("14000000000000000000000");
+        assertEquals(expectedTotalDifficulty, totalDifficulty);
     }
 
     private Block buildBlockWithUncles() {