Repository navigation
Update blocks informing logic - #638
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
6c30c3d to
83aba1d
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the PowPeg node’s “inform BTC headers to the Bridge” flow so that subsequent receiveHeaders calls avoid re-sending headers the Bridge already knows, improving behavior across forks and repeated update cycles.
Changes:
- Adjust
BtcToRskClient.updateBridgeBtcBlockchain()to skip headers already known by the Bridge (viaisBlockHashInformedToBridge) and send only the next missing batch. - Extend test mocks (
SimpleFederatorSupport) to track “known” headers via both the registered chain and priorsendReceiveHeaderscalls. - Update/extend tests to validate deep-fork behavior and successive update calls that progressively inform the remaining headers.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/main/java/co/rsk/federate/BtcToRskClient.java | Adds logic to find the first unknown header and only inform missing headers to the Bridge. |
| src/test/java/co/rsk/federate/BtcToRskClientTest.java | Updates deep-fork expectations and adds a test covering repeated calls informing headers in batches. |
| src/test/java/co/rsk/federate/mock/SimpleFederatorSupport.java | Implements isBlockHashInformedToBridge in the test support to reflect Bridge-known header behavior. |
83aba1d to
fcde92e
Compare
62ae86c to
a20b483
Compare
|
|
||
| boolean shouldUpdateBridge = federatorBtcBlockchainBestChainHeight > bridgeBtcBlockchainBestChainHeight; | ||
| if (!shouldUpdateBridge) { | ||
| return 0; |
There was a problem hiding this comment.
Maybe log here? "Bridge is already up to date with BTC best chain {}", height
There was a problem hiding this comment.
Or even better, move the log from below to before this if
| totalHeadersToSendToBridge.add(currentBlockHeader); | ||
| currentBlock = bitcoinWrapper.getBlock(currentBlockHeader.getPrevBlockHash()); | ||
| } | ||
| if (totalHeadersToSendToBridge.isEmpty()) { |
There was a problem hiding this comment.
I understand this could never happen, it was already checked above. As long as we have a test that covers the case where there are no blocks to inform to the Bridge it's probably safe to remove this.
| // Only send the headers that are not already known to the Bridge | ||
| int startIndex = findFirstUnknownHeader(totalHeadersToSendToBridge); | ||
| int totalHeadersToSendToBridgeCount = totalHeadersToSendToBridge.size(); | ||
| if (startIndex == totalHeadersToSendToBridgeCount) { |
There was a problem hiding this comment.
Same here I believe, or is there a case I'm missing?
| Federation federation = TestUtils.createP2shP2wshErpFederation(MAINNET_BTC_PARAMS, 20); | ||
| setUpActiveFedClient(federation); | ||
|
|
||
| // the Bridge has maxAmountOfHeadersToSend headers informed past the common ancestor, | ||
| // meaning we have chainHeight - lastInformedHeader = 130 headers to inform. | ||
| markHeadersAsInformed(1, lastInformedHeader); | ||
| int firstCallFirstHeaderToSend = lastInformedHeader + 1; | ||
| markHeadersAsNotInformed(firstCallFirstHeaderToSend, chainHeight); | ||
|
|
2c360dd to
8c9b243
Compare
8c9b243 to
c3db5d0
Compare
| private CoinbaseInformation getCoinbaseInformation(Sha256Hash blockHash) throws IOException { | ||
| return btcToRskActiveFedClientFileStorage.read(MAINNET_PARAMS).getData().getCoinbaseInformationMap().get(blockHash); | ||
| } | ||
|
|
There was a problem hiding this comment.
just moved from below
1f41c29 to
b1d8ae7
Compare
b1d8ae7 to
2f008dd
Compare
| // the Bridge has maxAmountOfHeadersToSend headers informed past the common ancestor, | ||
| // meaning we have chainHeight - lastInformedHeader = 130 headers to inform. | ||
| markHeadersAsInformed(1, lastInformedHeader); | ||
| int firstCallFirstHeaderToSend = lastInformedHeader + 1; | ||
| markHeadersAsNotInformed(firstCallFirstHeaderToSend, chainHeight); | ||
|
|
| int chainHeight = 230; | ||
| int maxAmountOfHeadersToSend = 100; // builder default | ||
| int lastInformedHeader = 100; |
|
|
||
| // setup fed best chain | ||
| int chainHeight = 230; | ||
| int maxAmountOfHeadersToSend = 100; // builder default |
|
757e8d9 to
20e0c7d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/main/java/co/rsk/federate/BtcToRskClient.java:448
amountOfHeadersToSendis read directly from config with no validation. If it is configured as 0 (or negative),tobecomesfrom, resulting in an emptyheadersToSendToBridgelist. That leads to callingsendReceiveHeaderswith an empty array (which dereferencesheaders[0]inFederatorSupport.sendReceiveHeaders) and also throws here when loggingheadersToSendToBridge.get(0)/get(size-1).
int to = Math.min(from + amountOfHeadersToSend, totalHeadersToSendToBridge.size());
List<Block> headersToSendToBridge = totalHeadersToSendToBridge.subList(from, to);
federatorSupport.sendReceiveHeaders(headersToSendToBridge.toArray(new Block[]{}));



No description provided.