refactor: Better separation between Cycles and NominalCycles

[dsarlis]

This PR is a first step towards implementing what's sketched out in #9063.

Specifically, in this PR we make some cycles metrics private to ReplicatedState so that we force callers to always use APIs that require specific types to be passed in as well as convert some other APIs to accept NominalCycles instead of simply Cycles. This allows the removal of the functions that allow direct conversion from Cycles to NominalCycles and wherever this is needed we can convert from the raw amount (this is mostly tests and one case in CyclesAccountManager which will be further improved in a follow-up).

Follow-up PR(s) will implement the remaining parts of the solution, namely a new type that captures both the real amount and the one to update metrics and further locking down access patterns via public APIs with explicit types on them.

[github-actions]

Run on af9b853 URL: https://github.com/dfinity/ic/actions/runs/22856388293

[dsarlis]

@basvandijk Seems there were some Java exceptions when running all tests, can you restart the job?

[basvandijk]

Seems bazel-test-all failed because of a clippy error: (sorry about color codes, I had to copy/paste this via my phone since I'm on the run)

�[1m�[91merror�[0m�[1m: unused import: `Cycles`�[0m
  �[1m�[94m--> �[0mrs/replicated_state/src/replicated_state.rs:29:38
   �[1m�[94m|�[0m
�[1m�[94m29�[0m �[1m�[94m|�[0m     AccumulatedPriority, CanisterId, Cycles, NumBytes, SubnetId, Time,
   �[1m�[94m|�[0m                                      �[1m�[91m^^^^^^�[0m
   �[1m�[94m|�[0m
   �[1m�[94m= �[0m�[1mnote�[0m: `-D unused-imports` implied by `-D warnings`
   �[1m�[94m= �[0m�[1mhelp�[0m: to override `-D warnings` add `#[allow(unused_imports)]`

�[1m�[91merror�[0m�[1m: aborting due to 1 previous error�[0m

But strange the cargo clippyjob didn't fail with the same error.

[dsarlis]

Seems bazel-test-all failed because of a clippy error: (sorry about color codes, I had to copy/paste this via my phone since I'm on the run)

�[1m�[91merror�[0m�[1m: unused import: `Cycles`�[0m
  �[1m�[94m--> �[0mrs/replicated_state/src/replicated_state.rs:29:38
   �[1m�[94m|�[0m
�[1m�[94m29�[0m �[1m�[94m|�[0m     AccumulatedPriority, CanisterId, Cycles, NumBytes, SubnetId, Time,
   �[1m�[94m|�[0m                                      �[1m�[91m^^^^^^�[0m
   �[1m�[94m|�[0m
   �[1m�[94m= �[0m�[1mnote�[0m: `-D unused-imports` implied by `-D warnings`
   �[1m�[94m= �[0m�[1mhelp�[0m: to override `-D warnings` add `#[allow(unused_imports)]`

�[1m�[91merror�[0m�[1m: aborting due to 1 previous error�[0m

But strange the cargo clippyjob didn't fail with the same error.

Hmm, it seems that Cycles are only used with [#cfg(debug_assertions)] after my changes so it rightfully complains. Perhaps, the conditional usage is what confused things with cargo clippy? But it shouldn't have been the case.

Btw, I am not sure how to easily fish out the actual error messages from the usually quite long output. There were indeed about 50 or so java exceptions that seemed to be what the issue was about (also given that I saw nothing else fail).

[basvandijk]

Btw, I am not sure how to easily fish out the actual error messages from the usually quite long output. There were indeed about 50 or so java exceptions that seemed to be what the issue was about (also given that I saw nothing else fail).

Yeah good point. It would be great if we could make BuildBuddy public. I'll bring it up in our next IDX meeting.

[alin-at-dfinity]

Just some nitpicks / questions, otherwise LGTM.

Review dismissed by automation script.

[dsarlis]

@alin-at-dfinity Assuming the approval meant that there are no other concerns. Since I merged master and fixed some stuff after merging, your approval is required again. Please also kickstart CI and add the PR to the merge queue if no more concerns.

[alin-at-dfinity]

Apologies for the late response, got carried away with other stuff. Triggered presubmit tests and auto-merge, let's see if it just works. I'll check back later.

Review dismissed by automation script.

[dsarlis]

@alin-at-dfinity I am not sure CI was started properly. You should see a message like this one if it's properly triggered.

[github-actions]

Run on c8dd263 URL: https://github.com/dfinity/ic/actions/runs/22940358522

[github-actions]

Run on c8dd263 URL: https://github.com/dfinity/ic/actions/runs/22940358522

[github-actions]

Run on c8dd263 URL: https://github.com/dfinity/ic/actions/runs/22942189740

[alin-at-dfinity]

Sorry about all that, I believe I got it all figured out now (with a lot of handholding).

[dsarlis]

No worries. I see you started 3 of them -- at least we'll have some fault tolerance in case we hit some flaky tests :)

[alin-at-dfinity]

Problem is, for the first couple of them I used the GitHub commit URL instead of just the commit SHA. And couldn't understand why they kept failing.

Review dismissed by automation script.

[dsarlis]

@alin-at-dfinity I had to fix some things after merging master which contained some latest changes. Please restart things when you get a chance.

[github-actions]

Run on 5c6ed24 URL: https://github.com/dfinity/ic/actions/runs/22952574021