Remove unused fields from Bank

[brooksprumo]

Problem

Bank (and snapshots) contains unused fields. I'm going to be adding a new snapshot version (and serde style) (see #21604 for info), and have a large PR for that (see PR #22467), but wanted to have as many small, self-contained PRs as possible. Keeping the unused fields in the new snapshot version seems unnecessary.

Summary of Changes

Remove unused fields from Bank.

Questions

• Should I also bump the snapshot version to 1.2.1? If yes, should I retain snapshot version 1.2.0?
  • A: Nope.

Testing

Manual compatibility testing is required.

	created by pr	created by v1.8
loaded by pr	works [1]	works [2]
loaded by 1.8	works [3]	works [4]

[1]: Tested via the tests in the code and CI; plus running a validator, creating snapshots, and restarting from them
[2]: Tested by bootstrapping a validator running this PR's code, and downloading a snapshot from the cluster running v1.8
[3]: Running a validator with this PR's code, then creating a snapshot with this new version. Rebuilding the validator with v1.8, and starting up with the new-version's snapshot. Ensure the node connects to mnb. The validator has been running for 2 hours as of me typing this.
[4]: This is the current working impl, so it's presumed good

[codecov]

Codecov Report

Merging #22491 (6a3cc7e) into master (e291342) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #22491   +/-   ##
=======================================
  Coverage    81.1%    81.1%           
=======================================
  Files         556      556           
  Lines      150655   150638   -17     
=======================================
+ Hits       122268   122294   +26     
+ Misses      28387    28344   -43     

[ryoqun]

[ ] Should I also bump the snapshot version to 1.2.1? If yes, should I retain snapshot version 1.2.0?

(i posted more general question at #21604; answering this question specifically)

i don't think you need to increment snapshot version as long as there is no compatibility concerns. moreover, if we increment the version, i think older node version causes error if it encountered unknown (newer) version (even like with patch version increment). or is the current snapshot version check code clever enough, striving to the semver?

[brooksprumo]

moreover, if we increment the version, i think older node version causes error if it encountered unknown (newer) version (even like with patch version increment).

Right! Yes, I'd be planning on backporting so that older node versions would know about newer snapshot versions.

or is the current snapshot version check code clever enough, striving to the semver?

I was assuming semver-like, yes. But maybe that's not necessary/overkill.

[brooksprumo]

@ryoqun Also, and more generally, is this PR (as is) safe? In other words, is it OK to remove these unused fields from the Bank?

[brooksprumo]

@ryoqun @mvines Following up to see if this PR needs anything else. TIA!

[carllin]

This seems fine, serialization into and out of the snapshot format should remain unchanged

[brooksprumo]

@ryoqun Thanks in advance for taking another look. I wanted to make sure there weren't any corner cases I had overlooked. (Esp anything related to if the whole Bank struct is hashed and compared across the cluster, but I don't think that happens.)

[ryoqun]

@ryoqun Also, and more generally, is this PR (as is) safe? In other words, is it OK to remove these unused fields from the Bank?

this should be safe. there's little test coverage in snapshot binary compatibility across branches. so, manual tedious testing of loading older/newer snapshot with older/newer node might be needed.

also, as you might already know, bank serialization is factored out of Bank itself for versioning. so, it should be safe as far as i look the rust code changes itself.

[ryoqun]

(sorry for late replay...)

or is the current snapshot version check code clever enough, striving to the semver?

I was assuming semver-like, yes. But maybe that's not necessary/overkill.

yeah, i think it's overkill. we just copied the version number from the validator node version at the time. let's things keep simple as much as possible.

[brooksprumo]

@ryoqun

there's little test coverage in snapshot binary compatibility across branches. so, manual tedious testing of loading older/newer snapshot with older/newer node might be needed.

Done. I've performed manual testing and put the results in the PR description. Is that enough testing?

also, as you might already know, bank serialization is factored out of Bank itself for versioning. so, it should be safe as far as i look the rust code changes itself.

👍

[ryoqun]

lgtm!