Uh oh!
There was an error while loading. Please reload this page.
Error instead of panicking when source account sequence number is at i64::MAX - #2681
Error instead of panicking when source account sequence number is at i64::MAX#2681Galmanus wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR prevents integer-overflow panics when incrementing Stellar account sequence numbers by introducing a checked “next sequence number” helper and propagating a dedicated overflow error through CLI commands.
Changes:
- Added
next_sequence_number()helper +SequenceNumberOverflowerror and regression tests. - Replaced
sequence + 1increments across config/tx/contract commands with checked increment + error propagation. - Wired
SequenceNumberOverflowinto relevant error enums for transparent bubbling.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/soroban-cli/src/utils.rs | Adds checked sequence increment helper, overflow error type, and tests. |
| cmd/soroban-cli/src/config/mod.rs | Uses checked increment when deriving the next account sequence number; adds error variant. |
| cmd/soroban-cli/src/commands/tx/update/sequence_number/next.rs | Switches to checked increment and adds overflow error variant. |
| cmd/soroban-cli/src/commands/contract/upload.rs | Switches to checked increment and adds overflow error variant. |
| cmd/soroban-cli/src/commands/contract/restore.rs | Switches to checked increment and adds overflow error variant. |
| cmd/soroban-cli/src/commands/contract/invoke.rs | Switches to checked increment and adds overflow error variant. |
| cmd/soroban-cli/src/commands/contract/extend.rs | Switches to checked increment and adds overflow error variant. |
| cmd/soroban-cli/src/commands/contract/deploy/wasm.rs | Switches to checked increment and adds overflow error variant. |
| cmd/soroban-cli/src/commands/contract/deploy/asset.rs | Switches to checked increment and adds overflow error variant. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// point must surface as an error instead of an integer-overflow panic. | ||
| #[derive(thiserror::Error, Debug)] | ||
| #[error("account sequence number is at the maximum value ({0}); no further transactions can be submitted from this account")] | ||
| pub struct SequenceNumberOverflow(pub i64); |
There was a problem hiding this comment.
Good instinct on API surface, but SequenceNumberOverflow can't be pub(crate) here. It's the #[from] field type in eight pre-existing pub enum Error variants (contract invoke/upload/extend/restore, deploy wasm + asset, tx update sequence-number next, and config) and the error type of next_sequence_number. Narrowing the struct while those enums stay pub trips the private_interfaces lint, and .cargo/config.toml sets [build] rustflags = ["-Dwarnings", ...], so that fails the build. Downgrading the eight command Error enums to match isn't desirable — they're established public API. So the struct stays pub. I have narrowed next_sequence_number to pub(crate) though — see the other thread.
| /// # Errors | ||
| /// | ||
| /// Returns [`SequenceNumberOverflow`] when `current` is `i64::MAX`. | ||
| pub fn next_sequence_number(current: i64) -> Result<i64, SequenceNumberOverflow> { |
There was a problem hiding this comment.
Done in 4b1dbab — made next_sequence_numberpub(crate); all its callers are in-crate (config, the contract commands, tx::args), verified it still compiles under -D warnings. The SequenceNumberOverflow type has to stay pub (it's the #[from] field in eight pub Error variants — see the sibling comment), but the fn doesn't.
…i64::MAX Any account can reach `i64::MAX` sequence with a single `bump_sequence` operation. After that, every command that builds a transaction for the account computed `sequence + 1`, which panics with "attempt to add with overflow" on debug builds and wraps to `i64::MIN` on release builds. Add `utils::next_sequence_number` (checked add with a descriptive error) and use it at every site that increments an account sequence number: `config::Args::next_sequence_number`, `contract invoke` (simulation and final build), `contract upload`, `contract extend`, `contract restore`, `contract deploy` (wasm and asset), and `tx update sequence-number next`. `Assembled::bump_seq_num` is left as-is: it has no in-repo callers and changing it would break its public `-> Self` signature. Fixesstellar#2417
Copilot review: all callers are in-crate (config, the contract commands, and tx::args), so the helper doesn't need to be part of the public API. The SequenceNumberOverflow type stays pub because it is the #[from] field in eight pub Error variants (see the sibling thread).
4b1dbab to
f1eafe1Compare| /// point must surface as an error instead of an integer-overflow panic. | ||
| #[derive(thiserror::Error, Debug)] | ||
| #[error("account sequence number is at the maximum value ({0}); no further transactions can be submitted from this account")] | ||
| pub struct SequenceNumberOverflow(pub i64); |
| + 1) | ||
| .into()) | ||
| .0; | ||
| Ok(crate::utils::next_sequence_number(seq_num)?.into()) |
Fixes#2417.
Any account can reach
i64::MAXsequence with a singlebump_sequenceoperation. After that, every command that builds a transaction for the account computedsequence + 1, which panics withattempt to add with overflowon debug builds and wraps toi64::MINon release builds.The issue reports the
tx newpath (config::Args::next_sequence_number), but the same unchecked increment exists at 8 more sites.Fix
Add
utils::next_sequence_number(checked add with a descriptive error):and use it at every site that increments an account sequence number:
config::Args::next_sequence_number(the issue's repro path)contract invoke(simulation and final tx build)contract uploadcontract extendcontract restorecontract deploy(wasm and asset)tx update sequence-number nextNot changed:
Assembled::bump_seq_num— it has no in-repo callers and changing it would break its public-> Selfsignature; it can be handled separately if desired.Tests
test_next_sequence_number— ordinary increments, includingi64::MAX - 1 → i64::MAX.test_next_sequence_number_at_max_errors_instead_of_overflowing—i64::MAXyields the error instead of overflowing.