Use read only LedgerTxn

[sisuresh]

Description

Resolves #3716

Checklist

• Reviewed the contributing document
• Rebased on top of master (no merge commits)
• Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
• Compiles
• Ran all tests
• If change impacts performance, include supporting evidence per the performance document

[marta-lokhova]

I think the overall idea to only use read-only LedgerTxns outside of the write phase is a good one. I'm reluctant about this particular change though given the current state of LedgerTxn interface. Specifically,

• Right now we have shouldUpdateLastModified flag which I think doesn't actually make sense in the context of RO txs. The problem here is that by allowing callers to set this flag, we potentially modify internal state of LedgerTxn, which is a footgun, as it might invalidate LedgerTxn state in read-only mode.
• On the other hand, the current master doesn't have this problem, as LedgerTxn ltx(app.getLedgerTxnRoot()); encapsulates all the internal state.
• The reason we sort of tolerated this poor interface in the past is because using RO LedgerTxn is beneficial in code paths where we create thousands of LedgerTxns (like overlay). For things like upgrades and nomination, there probably isn't any perf benefit.
• Thinking about why we have some many LedgerTxns throughout the code, I'm wondering: will these changes clash with what @dmkozh is working on, i.e. stop using LedgerTxn for network config retrieval? IIUC, the reason we have so many RO LedgerTxns in the code now is to support Soroban config loading in v20.

If we still want to move towards RO LedgerTxns everywhere, we should make sure we have a safe interface first. We have #3800 to clean up the API, but I think we can harden things even further by forbidding commits in RO mode?

[dmkozh]

Thinking about why we have some many LedgerTxns throughout the code, I'm wondering: will these changes clash with what @dmkozh is working on, i.e. stop using LedgerTxn for network config retrieval? IIUC, the reason we have so many RO LedgerTxns in the code now is to support Soroban config loading in v20.

Given that this PR is really small, I think we don't need to block it on my changes. Also some of ltxs are also used for something else besides the config getter. But if we're going to do a larger scale refactoring, then I'd say let's wait for my changes.

[sisuresh]

Yeah we can hold off on this and any refactoring since it's just a perf change and not high priority. I had a few minutes before standup so decided to do this, but we have other issues that are higher priority so we shouldn't be spending more time on this.