fix(nns-recovery): choose DFINITY-owned node as one with highest certification share height #6554
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pierugo-dfinity
added
the
CI_ALL_BAZEL_TARGETS
frankdavid
reviewed
Sep 1, 2025
andrewbattat
approved these changes
Sep 2, 2025
|
The test is still flaky, and we are actively trying to find the cause. In the meantime, let us merge this PR, which should fix the original source of flakiness. |
pierugo-dfinity
deleted the
pierugo/nns-recovery/highest-certification-share-height-node
branch
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 4, 2025
The NNS recovery test is flaky. #6554 fixed a first edge-case and this one should fix a second one. In some cases, the DFINITY-owned node would be fixed twice: once by `ic-recovery` and once by the `guestos-recovery-upgrader`/`guestos-recovery-engine`. This is not a problem per se, the bug is a "test" bug: when simulating the actions of node providers, we overwrite `BOOT_ARGS_A` in `/boot/boot_args` [here](https://github.com/dfinity/ic/pull/6606/files#diff-c26ca29faacddc5919f46b7b6d2d7c503af940fe2e7b14038964accb17d0bebdL256). This works fine if the node is currently using partition A. But the DFINITY-owned node has already upgraded as part of `ic-recovery` and thus is using partition B. This led to its state being wiped and not be able to make consensus progress. As a short-term flakiness fix, this PR does not run the simulated NP actions on the DFINITY-owned node anymore, but the functionality can be introduced more carefully in the future as part of the effort of testing more recovery scenarios.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The introduction of the end-to-end NNS recovery test (64da037) is very flaky. The reason is that in the case where the DFINITY-owned node is lagging behind (which happens when it is part of the faulty nodes), it could happen that its artifact pool has a finalization height which is lower than the highest certification share height across the subnet. In that case, we ask the
ICReplaystep of the recovery to replay until the latter but it will only replay until the highest finalized block.Indeed, the recovery tool assumes that the node we download the state from (including the consensus artifact pools) from contains all artifacts (except certifications and certification shares) up to the highest certification share height. If this is not the case, we could download those artifacts from the relevant node. This is something that we take note of (CON-1580) and will look into in the future by allowing for example to download the artifacts from a different node than the state.
Note: this is not specific to NNS recovery, it is also the case in application subnet recoveries. In app subnet system tests, we avoid this edge-case by downloading the state from the highest certification height node. This PR thus does the same thing to fix the flakiness short-term. Once CON-1580 is implemented, we will go back to selecting the DFINITY-owned node randomly.
Note 2: This edge-case never happened in production in app subnet recoveries because the DFINITY-owned node always had up-to-date artifacts.