Repository navigation
Millisecond-resolution close times (MS_CLOSE_TIME, vnext) - #5423
Conversation
8165b71 to
7de3592
Compare
0f86bce to
e76c393
Compare
bboston7
left a comment
There was a problem hiding this comment.
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::scheduleLoadGenerationtruncates 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::processbreaks due to a truncation to 0.
There might be more, but those are the ones I found. I think we should either:
- 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.
- 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. |
78788ea to
7650fb9
Compare
2f1b4fb to
9ef9b5c
Compare
There was a problem hiding this comment.
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, definescloseTimeMsas auint32remainder in[0, 999], but this stores the full epoch time in milliseconds. Current timestamps therefore truncate, andhasValidCloseTimeimmediately rejects the resulting value, so v29 nomination cannot work with the documented pin. Either update the submodule to the later full-uint64schema (03cbf40…) or rework the conversions, signatures, and tests to use a remainder.
Integrate PR stellar#5423 before parallel transaction validation. Carry full Unix milliseconds in consensus values while retaining whole-second application semantics for transaction time bounds, sequence age and upgrade scheduling. Enable the schema and protocol gate in normal builds. Preserve overlay-v2 early proposal preparation with precise timestamps and teach the Rust overlay to extract tx-set hashes from the new StellarValue arms. Port the upgrade replay and subsecond tests to overlay v2 and refresh protocol-28 metadata.
Integrate PR stellar#5423 before parallel transaction validation. Carry full Unix milliseconds in consensus values while retaining whole-second application semantics for transaction time bounds, sequence age and upgrade scheduling. Enable the schema and protocol gate in normal builds. Preserve overlay-v2 early proposal preparation with precise timestamps and teach the Rust overlay to extract tx-set hashes from the new StellarValue arms. Port the upgrade replay and subsecond tests to overlay v2 and refresh protocol-28 metadata.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The implementation conflicts with the pinned XDR field semantics, and two tests construct non-advancing ledger close times.
Review effort: Balanced
Findings: 2
Open (6)
closeTimeMsin the pinned XDR is only the[0, 999]remainder, not milliseconds since epoch.… The pinned stellar-xdr commit definessignedMsValue.closeTimeMsas auint32millisecond… Apply tx2 in the T+1 ledger to preserve monotonic close times · New Include tx in the T+1 ledger to avoid duplicate close times · New This derives the future-time bound using the local LCL's protocol. A pre-upgrade node is explicitly… This sentence should say “our peers are sending us”; the repeated “our” is a grammatical typo.
13e889c to
aac34ce
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Production startup no longer verifies that linked C++ and Rust XDR schemas are compatible.
Review effort: Balanced
Findings: 3
Open (7)
Re-enable startup XDR identity check and resolve feature mismatch · NewcloseTimeMsin the pinned XDR is only the[0, 999]remainder, not milliseconds since epoch.… The pinned stellar-xdr commit definessignedMsValue.closeTimeMsas auint32millisecond… Include tx in the T+1 ledger to avoid duplicate close times Apply tx2 in the T+1 ledger to preserve monotonic close times This derives the future-time bound using the local LCL's protocol. A pre-upgrade node is explicitly… This sentence should say “our peers are sending us”; the repeated “our” is a grammatical typo.
aac34ce to
36fa0b7
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The implementation treats the XDR millisecond-remainder field as a full epoch timestamp, causing truncation and invalid SCP values.
5 open findings
Re-enable startup XDR identity check and resolve feature mismatchcloseTimeMsin the pinned XDR is only the[0, 999]remainder, not milliseconds since epoch.… The pinned stellar-xdr commit definessignedMsValue.closeTimeMsas auint32millisecond… This derives the future-time bound using the local LCL's protocol. A pre-upgrade node is explicitly… This sentence should say “our peers are sending us”; the repeated “our” is a grammatical typo.
2 resolved since last review
🧠 Review effort: Balanced
stellar#5423 moved it to MAX_SUPPORTED_PROTOCOL_VERSION in src/util/ProtocolVersion.h, so the check found no assignment in Config.cpp. Fall back to Config.cpp for older revisions and follow both files when looking for the last bump.
stellar#5423 moved it to MAX_SUPPORTED_PROTOCOL_VERSION in src/util/ProtocolVersion.h, so the script found no assignment in Config.cpp. Fall back to Config.cpp for older revisions and follow both files when looking for the bump commit.
Build 3640 (0e49751aa) includes stellar/stellar-core#5423, so ledgers after the protocol 30 upgrade now use the millisecond close-time StellarValue arms. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Build 3640 (0e49751aa) includes stellar/stellar-core#5423, so ledgers after the protocol 30 upgrade now use the millisecond close-time StellarValue arms. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* Shift soroban-env window to P30 with CAP-0084 pins go-stellar-sdk replaced to sisuresh/go @ 50ebbb65; -prev now 28.0.1, -curr git-pinned to rs-soroban-env#1700 head with next; stellar-xdr git-pinned with cap_0084_muxed_contract. * methods: drop EventInfo.InSuccessfulContractCall Removed from protocols/rpc in the pinned go-stellar-sdk (#5995). * Repin CAP-0084 host window to merged upstream heads -prev now tracks core's protocol-29 stand-in (rs-soroban-env cb32250d, no crates.io release exists for it); -curr moves off the rs-soroban-env fork PR to its merged commit 169890d5 (protocol 30 by default, no `next` feature needed). stellar-xdr repinned to a9c9750e to match curr's own pin. go-stellar-sdk fork pin bumped to the PR #6015 head (still unmerged upstream). * ci: add P30 integration leg, copy P28 upgrade fixtures Drops the oldest P27 leg per the one-stanza-per-protocol convention. No -vnext core artifact exists yet for CAP-0084, so the P30 leg's core image is left unset. Upgrade fixtures copied from P28 unchanged since CAP-0084 doesn't touch ConfigSettings. * ci: test protocols 29 and 30 against merged stellar-core 73f2bdd1 The host window is now 29/30, so the P28 leg becomes P29 (with p29 upgrade fixtures copied from p28, as p30's were). Both legs use stellar-core 73f2bdd1's published artifacts. Host blocks declare their versions so check-dependencies.bash can find them. * ci: add apt.stellar.org unstable for unreleased core packages stellar-core 73f2bdd1's packages are only in the unstable component. * Repin go-stellar-sdk to the go-stellar-sdk#6015 head (663a89a1) #6015 is now the single P30 SDK PR: XDR regenerated from stellar-xdr 4f524bba with CAP-0084, CAP-0087 and CAP-0088, plus ingest/ledger NodeID/Signature handling for every signed StellarValue arm. 663a89a1 is 174b61f0 with go-stellar-sdk main (6c5e720f: #6006, #6014, #6017) merged in, so the replace does not drop fixes RPC would otherwise get from main. No compile breaks beyond the EventInfo.InSuccessfulContractCall removal already in this branch: RPC uses storage.ConnectOptions but none of the cache APIs #6017 removed. go mod tidy drops hashicorp/golang-lru, which only the removed on-disk cache needed. The replace stays allow-listed in .golangci.yml until #6015 merges. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Repin the soroban host window to the real P29 host and P30 main - soroban-env-host/simulation-prev: cb32250d (core's p29 stand-in) -> rs-soroban-env v29.0.0 (a721944b), stellar-core's src/rust/soroban/p29 in v29.0.0 and master, pinned by git so it builds with the same dependency tree as core's p29 host. - soroban-env-host/simulation-curr: 169890d5 (CAP-0084 only) -> rs-soroban-env main 8abed902, with CAP-0084 and the CAP-0087 ML-DSA host functions ungated (#1737, #1744, #1745, #1746). This is the p30 host stellar-core #5478 pins; core master's p30 (fddfd567) predates ML-DSA. To be repinned to core's final p30 submodule once core #5455/#5478 merge. - stellar-xdr: a9c9750e -> 28f34cf5, curr's own pin (XDR from stellar-xdr 4f524bba, as in the Go SDK). CAP-0084, CAP-0087 and CAP-0088 are ungated there, so the cap_0084_muxed_contract feature goes away, and xdr2json can now decode CAP-0088 StellarValue arms (MS_CLOSE_TIME was a disabled feature at a9c9750e). The new ml-dsa dependency tree resolves to the versions in core's p30 dep-tree, except hybrid-array 0.4.10 (core: 0.4.15; 0.4.11+ needs typenum 1.20 and the lockfile has 1.18) and ctutils 0.4.3 (core: 0.4.2). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J5dN6jqdJt52ve9XWyeggH * methods: test serving CAP-0088 millisecond close time ledgers Ingests a protocol 30 ledger whose StellarValue uses STELLAR_VALUE_SIGNED_MS (and, separately, STELLAR_VALUE_EMPTY_TX_SET_MS) through the writers the ingestion service uses (ledger, transactions, events, fee windows, commit) and checks what getLatestLedger, getLedgers (XDR and JSON), getTransaction, getEvents and getFeeStats serve: every close time is the whole-second closeTime (closeTimeMs / 1000), the XDR is byte-identical, and xdr2json keeps the arm and closeTimeMs. RPC never reads StellarValue.ext, lcValueSignature or the node ID, so the only real requirement is that both XDR stacks decode the new arms. The JSON check fails on the previous rs-stellar-xdr pin (a9c9750e, MS_CLOSE_TIME gated off): getLedgers returns "xdr_to_json() failed: couldn't read LedgerCloseMeta: xdr value invalid". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * ci: run P29 on stellar-core 29.0.0 and P30 on core master's vnext build - P29: stellar-core 29.0.0 (4eb833373), as on protocol-next. Its p29 host is rs-soroban-env v29.0.0 (a721944b), which soroban-env-host-prev now pins, instead of master 73f2bdd1 with the cb32250d stand-in. - P30: vnext build of master 88d2431d7 (merge of v29.0.0 into master), whose p30 host is fddfd567: CAP-0084 plus the v29.0.0 fixes. It differs from soroban-env-host-curr (8abed902) only by the ML-DSA commits. No core build with CAP-0087 (#5455/#5478) or CAP-0088 (#5423) is published yet. Its deb is only in the apt.stellar.org unstable component, which integration-tests.yml already adds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * integrationtest: drop the P28 upgrade fixtures No CI leg runs protocol 28 any more (P29 and P30 only), and the harness picks <name>.p<protocol>.xdr for the protocol under test. Following the docker/README convention of moving the oldest fixtures to the new protocol, this leaves p29 and p30, which are identical to the old p28 ones. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * scripts: fix check-dependencies for the P29/P30 window and SDK replaces The dependency sanity check failed on this branch for two reasons that were not real mismatches: - It looked up `stellar-xdr@<protocol>`, assuming the crate's major version tracks the protocol. The protocol 29 and 30 hosts both use stellar-xdr 28.0.0 (crates.io for prev, git 28f34cf5 for curr), so there was no match and the Rust XDR revision came out empty. Each host's stellar-xdr now comes from that host's own dependency tree, and the comparison with core's dep-trees/p<N>-expect.txt uses the host for the same protocol rather than always the newest. - It derived the Go SDK commit from the required go-stellar-sdk version and fetched it from stellar/go-stellar-sdk, ignoring the replace directive. It now reads xdr/xdr_commit_generated.txt from the module the build actually uses (go list -m -f {{.Dir}}), which also handles tags. On this branch the check now passes: Go (#6015) and the curr host's rs-stellar-xdr both use stellar-xdr 4f524bba. It still reports, without failing, that core master's p30 uses rs-stellar-xdr a9c9750e. The updated script also passes against protocol-next's configuration. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Add Protocol 30 CHANGELOG entries Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * ci: run the P30 leg on stellar-core master after protocol 30 landed stellar/stellar-core#5478 made protocol 30 the current protocol on master, with CAP-0084 and CAP-0087, and its p30 host is rs-soroban-env 8abed902, the same as soroban-env-host-curr. Move the P30 leg from the pre-#5478 vnext build (88d2431d7) to master's ba6a4e6e3 build. In the same change, give testnet.p30 and unlimited.p30 the 95 contract cost params core now requires at protocol 30. The 9 new ML-DSA entries (86-94) are core's v30 defaults from updateCpuCostParamsEntryForV30 / updateMemCostParamsEntryForV30; every other entry is unchanged. The .xdr files are re-encoded from the .json with stellar-xdr encode --type ConfigUpgradeSet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J5dN6jqdJt52ve9XWyeggH * Require the merged go-stellar-sdk Protocol 30 commit stellar/go-stellar-sdk#6015 merged into protocol-next as 990270d3, so drop the replace that pointed at the sisuresh/go fork (663a89a1) and require the upstream commit. This also picks up the fixes made in #6015 after 663a89a1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Run the P30 leg on core 3640, which includes CAP-0088 Build 3640 (0e49751aa) includes stellar/stellar-core#5423, so ledgers after the protocol 30 upgrade now use the millisecond close-time StellarValue arms. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Run the P30 leg on the stellar-core 30.0.0 release candidate 30.0.0-3642.0e49751aa is built from the same commit as build 3640, so this changes the package under test, not the code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Correct how core's p30 host differs from -curr Core's p30 host also takes stellar-xdr 30.0.0 from crates.io, not only soroban-wasmi. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Trim comments and the CHANGELOG, drop the CAP-0088 unit test The P30 integration leg runs on the stellar-core 30.0.0 release candidate, so its ledgers use the CAP-0088 arms, and the integration tests already fetch them with getLedgers and getTransaction in JSON. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: tamirms <tamirms@gmail.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>



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
minSeqAgeor 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_SETchange. 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_SETas invalid, preventing it from properly replaying SCP messages. This is very minor. In the P28 upgrade, we won't actually start sendingSTELLAR_VALUE_EMPTY_TX_SETmessage 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
clang-formatv8.0.0 (viamake formator the Visual Studio extension)