feat: nullify PlatformState.addressBooks

[anthony-swirldslabs]

Description:
Core goal of this fix:

• Migrate data from PlatformState.[previous]AddressBook fields to RosterService states
• Nullify these PlatformState fields

The goal is accomplished via the RosterService and PlatformState schemas' migrate()/restart() methods. The rest of the changes are unit tests for this modification and necessary plumbing due to some class renaming/code moving.

The rest of the system always uses the RosterRetriever methods to fetch the active/previous rosters, and these methods prioritize reading from the RosterService states. Once they are populated, the RosterRetriever will never read data from the PlatformState fields anymore.

This is the next step required for deprecating the AddressBook fields in the PlatformState. Hopefully, in the next release, we'll be able to physically remove these fields from the state altogether.

Related issue(s):

Fixes #17150

Notes for reviewer:
All tests should pass.

Checklist

• Documented (Code comments, README, etc.)
• Tested (unit, integration, etc.)

[tinker-michaelj]

⚠️ Class ids for states depend on the version of their registering schema, so changing this to 0.59.0 will break deserialization of production states. (I.e., the 0.59 software would not be able to load a state created by 0.58 because it would not have a registered serializer for the old RosterService class ids.)

[anthony-swirldslabs]

I recognize that this may be a problem. However, I've literally followed your own recommendation to rename/change the version number in the schema class to implement this. Since the solution that you originally recommended is problematic, and I lack any prior experience in this particular area, could you please suggest another solution for this?

My wild guess would be to keep the existing V0540 schemas intact and add extra V0590 schema classes that would implement the new migration/restart logic. However, this is just a wild guess as I've never done this before, and I'm unsure if I'd still have to modify these old schemas too to remove any unnecessary logic that I may move to the new V0590 schemas.

What should I do here w.r.t. the versions? Please advise.

[edward-swirldslabs]

for the platform owned code.

[tinker-michaelj]

⚠️ This is the branch that is supposed to put the roster from the final on-disk config.txt into the RosterService state.

But with the above migrate() implementation, getActiveRoster() will no longer return null at the upgrade boundary when this restart() method is called.

So instead of adopting the last 0.58 candidate roster before the upgrade boundary, we will keep using the roster from the start of the 0.58 release.

[anthony-swirldslabs]

There doesn't need to be a "final on-disk config.txt" when the system is ready to enable the new Roster lifecycle in full. Instead, you will want to populate the candidate roster in the RosterService states to allow this code to follow the normal upgrade branch a bit below at line 182 in order to adopt it. The migrate() above will simply ensure that the RosterService states have the current/previous rosters populated, and the regular upgrade branch will handle the adoption of the candidate roster using the normal code path as it's supposed to happen with the new Roster lifecycle.

I believe you'll be able to remove this if (rosterStore.getActiveRoster() == null) branch here altogether because this situation can(/should) never happen.

In other words, you don't need to write any special code to handle this literally one-time event when we switch to the new Roster lifecycle. You can use the regular upgrade/adopt code path that will work from then on. This requires less complexity and fewer special cases that we need to implement/test/support.

[tinker-michaelj]

As written this will break state deserialization; and even if that is fixed, will prevent adoption of the final 0.58 candidate roster. (See comments inline.)

But I don't actually understand why we need this PR at all.

Once two things happen:

1. There are zero consumers of the PlatformState address book fields; and,
2. This issue @mhess-swl created is done

Then we can make useRosterLifecycle=true by default and delete all code paths that only run when it is false. The result should be exactly as described in your goal for this change.

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.06% (target: -1.00%)	✅ 85.71%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (70c450e)	96090	68316	71.10%
Head commit (2eb160a)	96063 (-27)	68359 (+43)	71.16% (+0.06%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#17282)	98	84	85.71%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

_{Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more}

[anthony-swirldslabs]

@tinker-michaelj :

I don't actually understand why we need this PR at all.

This PR aims to decouple the parallel work streams that the Services and Platform teams are working on.

The Services are mainly tasked with finishing the implementation of the new Roster lifecycle (replacing the usage of the config.txt with the candidate roster, using all the new network configuration code/data structures that you've created, etc.) and then finally flipping the flag which protects your new code (or removing the old code altogether - whichever works best for you.)

The Platform team is mainly concerned with deprecating the PlatformState.addressBook fields and starting using the RosterService states for storing current/previous rosters, which is completely independent of the new Roster lifecycle launch/the value of the flag, and therefore shouldn't depend on your work.

This PR belongs to this latter area.

[codecov]

Codecov Report

Attention: Patch coverage is 82.65306% with 17 lines in your changes missing coverage. Please review.

Project coverage is 67.30%. Comparing base (70c450e) to head (2eb160a).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
...era/node/app/roster/schemas/V0590RosterSchema.java	78.08%	13 Missing and 3 partials ⚠️
...wirlds/platform/state/PlatformMerkleStateRoot.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #17282      +/-   ##
============================================
+ Coverage     67.24%   67.30%   +0.06%     
- Complexity    21991    21995       +4     
============================================
  Files          2583     2582       -1     
  Lines         96307    96280      -27     
  Branches      10054    10054              
============================================
+ Hits          64763    64804      +41     
+ Misses        27839    27769      -70     
- Partials       3705     3707       +2     

Files with missing lines	Coverage Δ
...-app/src/main/java/com/hedera/node/app/Hedera.java	52.29% <100.00%> (ø)
...a/node/app/blocks/impl/BlockStreamManagerImpl.java	94.01% <100.00%> (ø)
.../node/app/records/impl/BlockRecordManagerImpl.java	81.53% <100.00%> (ø)
...java/com/hedera/node/app/roster/RosterService.java	100.00% <100.00%> (ø)
...ode/app/roster/schemas/RosterTransplantSchema.java	96.55% <ø> (ø)
...s/platform/state/service/PlatformStateService.java	100.00% <100.00%> (ø)
...form/state/service/ReadablePlatformStateStore.java	76.92% <ø> (ø)
...form/state/service/WritablePlatformStateStore.java	98.85% <100.00%> (ø)
...tate/service/schemas/V0590PlatformStateSchema.java	91.42% <100.00%> (ø)
...platform/state/snapshot/SignedStateFileReader.java	72.54% <100.00%> (ø)
... and 2 more

... and 19 files with indirect coverage changes

[anthony-swirldslabs]

This PR is temporarily put on hold because @tinker-michaelj claims that he and his team are working towards the same goal on their end and have the code fully tested, and are about to release it next week. We'll close the PR if/when that happens.