Millisecond-resolution close times (MS_CLOSE_TIME, vnext)

[SirTyson]

Description

Adds a MS component to CloseTime.

Currently, block close times must be in whole second increments, making non-whole second and subsecond block times impossible. For example, we cannot achieve a block time of 2.5 seconds, as the block time which we use for our basis of the next ledger trigger timer is rounded.

This is intended to unblock our latency experiments, but should be robust enough for a protocol release if we so choose. To keep things simple, I've left the current closeTime field that same (it still holds the whole second component of closeTime) and added an additional field for the ms component of close time. This means downstream consumers of closeTime won't be broken with the upgrade, and don't necessarily even need to ingest the ms times if they don't need it.

Additionally, for everything user-facing with a time component, I've left the interface the same and always round down the block time to the nearest whole second. Specifically, minSeqAge, transaction time bounds, claimable balance predicates, upgrade scheduling, and soroban ledger timestamps all round down to the nearest whole number. At the protocol level, this doesn't break anything, and I've added unit tests for subsecond ledgers, where back to back ledgers round to the same whole second value. While I'm sure we have performance issues, the protocol itself supports subsecond ledgers.

On the application side, I'm 99% sure we don't break anything. It's possible that an implementation of minSeqAge or transaction bounds could use currTime + 1 as a proxy for the next ledger, but this does not seem like a correct use case. I think from a protocol perspective, it's fine to release this as is, then add MS resolution to the transaction interface if anyone actually cares.

There is one complexity around SCP values during the upgrade, which is also present in the STELLAR_VALUE_EMPTY_TX_SET change. Basically, whenever StellarValueType changes via a protocol upgrade, it's challenging to properly check if an SCP message is valid. You can't just look at the LCL's protocol version, as it's possible the node is slow and it's peers have already completed the upgrade and are sending valid future slot messages. The same is true in the inverse, if you're past the protocol upgrade, you still might receive/relay messages from older nodes that are actually valid for pre-upgrade slots. "millisecond close time upgrade boundary" tests this case, where a node loses sync on the upgrade boundary and needs to replay the upgrade via SCP messages.

Note that there is a potential "bug" (maybe) in the current protocol 28 upgrade path. An out of sync node will drop any future slot values with STELLAR_VALUE_EMPTY_TX_SET as invalid, preventing it from properly replaying SCP messages. This is very minor. In the P28 upgrade, we won't actually start sending STELLAR_VALUE_EMPTY_TX_SET message types. Even if we did, it only affects out of sync nodes, which would just lose sync and catchup via regular history replay without issue. This change it's a little worse, since we're guarenteed to start using the new message type immediately on the upgrade boundary, but it is still quite minor.

The XDR changes (stellar/stellar-xdr#316) have merged; the submodule is pinned to the canonical commit (96cbfc3).

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

[Copilot]

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

[bboston7]

I think this is largely correct. The SCP adjacent changes (signature checking, value validation, pending envelopes, etc) all look correct to me.

The only thing that stood out to me is that there's some weird behavior with sub-second ledger close times. There are parts of the code where expected close times get rounded down, and a 0 value is hard to reason about. Examples include:

• getUpperBoundCloseTimeOffset. I think this might have weird impacts on determining whether transactions have expired or not, but I'm not sure.
• LoadGenerator::scheduleLoadGeneration truncates sub-second close times to 0, which might break some internal accounting or checks. I'm also not sure about this one.
• I think the query window calculation in Peer::process breaks due to a truncation to 0.

There might be more, but those are the ones I found. I think we should either:

1. reject sub-second close times in the config parser. That would still allow targeted unit tests to use sub-second close times, but would prevent us from accidentally configuring supercluster to use them without fixing these issues first.
2. Audit the codebase for all places where sub-second close times round down to 0 and verify whether it's OK or needs special handling.

[bboston7]

Why not just isMsCloseTimeStellarValue in this conditional? Doesn't getCloseTimeMs return 0 whenever isMsCloseTimeStellarValue is false?

[SirTyson]

I think this is largely correct. The SCP adjacent changes (signature checking, value validation, pending envelopes, etc) all look correct to me.

The only thing that stood out to me is that there's some weird behavior with sub-second ledger close times. There are parts of the code where expected close times get rounded down, and a 0 value is hard to reason about. Examples include:

• getUpperBoundCloseTimeOffset. I think this might have weird impacts on determining whether transactions have expired or not, but I'm not sure.
• LoadGenerator::scheduleLoadGeneration truncates sub-second close times to 0, which might break some internal accounting or checks. I'm also not sure about this one.
• I think the query window calculation in Peer::process breaks due to a truncation to 0.

There might be more, but those are the ones I found. I think we should either:

1. reject sub-second close times in the config parser. That would still allow targeted unit tests to use sub-second close times, but would prevent us from accidentally configuring supercluster to use them without fixing these issues first.
2. Audit the codebase for all places where sub-second close times round down to 0 and verify whether it's OK or needs special handling.

Thanks, I've added unit tests and fixed the cases you've found. I think for this initial PR, I'm most concerned about sub-second correctness of protocol itself. As in, is the CAP spec sufficient or is there some sort of TX application/observable artifact that we need to potentially address for sub-second ledger. I think we've addressed and tested all of those features well.

For non-protocol breaking changes, I'm a little less concerned. I image there are a lot of things broken with sub second ledgers beyond second assumptions, so I think we should definitely maintain the ability to test this in SSC. Production has a strict lower bound of 4 second ledgers, so we're safe even from an accidental closeTime change. For SSC, I'd rather we try our best to fix what we can spot then just let it break during the test, rather than hardcode our own minimum bound for testing.

[Copilot]

Pull request overview

Copilot reviewed 68 out of 885 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 68 out of 885 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/herder/HerderSCPDriver.cpp:559

• Use “are” here: “it's possible our peers are sending us…”
  src/herder/HerderImpl.cpp:2936
• The stated XDR pin, 96cbfc3, defines closeTimeMs as a uint32 remainder in [0, 999], but this stores the full epoch time in milliseconds. Current timestamps therefore truncate, and hasValidCloseTime immediately rejects the resulting value, so v29 nomination cannot work with the documented pin. Either update the submodule to the later full-uint64 schema (03cbf40…) or rework the conversions, signatures, and tests to use a remainder.

[tacticalnoot]

Superseded by the consolidated hardening handoff below: #5423 (comment)

Preserved invariant: predecessor-dependent semantics must wait for the canonical predecessor rather than mutable local-LCL knowledge.

[tacticalnoot]

Deferred follow-up; the active merge gate is the consolidated predecessor-known/unknown handoff: #5423 (comment)

Preserved separate concern: if minSeqAge ever gains millisecond semantics, Core must have canonically preserved the exact relative-time origin before that activation; discarded precision cannot be reconstructed later.

[tacticalnoot]

Superseded by the consolidated hardening handoff below: #5423 (comment)

The performance tangent remains intentionally out of scope for this correctness gate.

[tacticalnoot]

Tyler — the ConsensusTime direction is materially cleaner; separating protocol precision from whole-second application semantics removed a lot of ambiguity.

One merge gate still survives the exact-head recheck: future-slot validation must not reject from predecessor facts the node does not know yet. PendingEnvelopes already defers beyond LCL+1, but deserializeAndValidateStellarValue() can still reject future protocol-gated values from the local LCL, and checkCloseTime() can evaluate a first-ms future value at stale precision.

Smallest proof: predecessor known → validate exactly; predecessor unknown + binary-supported format → preserve/defer predecessor-dependent rejection. One permutation test across P27→P28 with future _MS + empty-tx-set values, arbitrary delivery order, restart, and catchup should converge to the same final accept/reject result.

Would you be up for making that monotone predecessor-known/unknown test the merge gate?