stellar-archivist: support empty generalized txsets in scan --verify #5867
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in stellar-archivist scan --verify where it was emitting spurious errors when encountering empty generalized transaction sets (v1). The code previously only handled v0 transaction sets, but the GeneralizedTransactionSet feature has been in Stellar Core's XDR for years.
Key Changes:
- Extended
VerifyTransactionHistoryEntryto handle both v0 and v1 transaction set versions - Added support for detecting empty v1 transaction sets (both sequential and parallel variants)
- Added integration tests that verify against live testnet and futurenet archives
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| historyarchive/verify.go | Added v1 GeneralizedTransactionSet support in verification, including helpers to construct empty sequential/parallel txsets and logic to detect them |
| historyarchive/archive_test.go | Added integration tests that scan and verify the last 100 ledgers from testnet and futurenet archives |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
graydon
force-pushed
the
archivist-scan-generalized-txsets
branch
from
a46efe7 to
028d6f0
Compare
historyarchive/archive_test.go
Outdated
| assert.NoError(t, err, "scanning last 100 checkpoints failed") | ||
| } | ||
|
|
||
| func TestScanVerifyLast100Testnet(t *testing.T) { |
There was a problem hiding this comment.
If you prefer a fixed test target, let me know and I can switch to that instead.
Yes, that might be ideal as having the test rely on remote resources could lead to random instability. Are these tests meant to validate those deployments in some way also by chance?
Maybe can use mock from GetTestMockArchive paired with some fake generated archive data similar to howTestGetLedgers wires up?
There was a problem hiding this comment.
mocking archive contents in a way that'll test --verify is way beyond the ability of the existing mocks -- they just fill the files with random data and are only sufficient to test parts of archivist that check for "some file existing", not like the actual content of it.
but I can download a specific sequence of checkpoints from futurenet and check that in as testdata. it's not too big, just a few MiB.
graydon
force-pushed
the
archivist-scan-generalized-txsets
branch
4 times, most recently
from
90e94be to
cdc0b23
Compare
graydon
force-pushed
the
archivist-scan-generalized-txsets
branch
from
cdc0b23 to
2b26c49
Compare
2 participants
PR Checklist
PR Structure
otherwise).
services/friendbot, orallordocif the changes are broad or impact manypackages.
Thoroughness
.mdfiles, etc... affected by this change). Take a look in the
docsfolder for a given service,like this one.
Release planning
CHANGELOG.mdwithin the component folder structure. For example, if I changed horizon, then I updated (services/horizon/CHANGELOG.md. I add a new line item describing the change and reference to this PR. If I don't update a CHANGELOG, I acknowledge this PR's change may not be mentioned in future release notes.semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
What
@sisuresh noticed recently that
stellar-archivist scan --verifyreports a bunch of spurious errors when confronted with empty transaction sets. This is because the code in it doesn't know about GeneralizedTransactionSet at all -- a feature that's been in core's XDR for years now! -- and the handling of empty txsets requires some special code that knows about them.I also added a test here to scan with verification the last 100 ledgers of the testnet and futurenet archives. This doesn't actually reproduce the error that's being fixed here. In theory I can go find one specific checkpoint in the archive which exhibits it and check that in too, but I thought it might be better to get some "moving signal" about whenever there's breakage between one of the archives and archivist in the future. If you prefer a fixed test target, let me know and I can switch to that instead.
This is a pure bug fix / patch. It changes nothing user-facing besides "not emitting spurious errors that it currently emits".
Why
It was emitting spurious errors.
Known limitations
N/A