Skip to content

Refuse values that do not fit their database column - #2568

Merged
thesimplekid merged 1 commit into
cashubtc:mainfrom
crodas:fix/sql-integer-overflow-simple
Oct 1, 2026
Merged

thesimplekid merged 1 commit into
cashubtc:mainfrom
crodas:fix/sql-integer-overflow-simple

Conversation

@crodas

@crodas crodas commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

Description

This is a simpler solution for #2425 before #2473 is matured.

Every integer column is signed, and the bind path narrowed with an unchecked cast, so a value past the signed maximum wrapped to a negative value and was committed, while Amount::to_i64 erased an oversized amount into NULL. Narrowing is checked now and refuses the write instead.

Postgres declared most of those columns int4 and truncated silently, storing an expiry past 2038 as a date already gone, so we widened them to int8. The balance query casts its aggregate back because a wider sum returns numeric. Storing amounts at their full u64 width stays a separate change.


Notes to the reviewers


Suggested CHANGELOG Updates

CHANGED

ADDED

REMOVED

FIXED


Checklist

  • I followed the code style guidelines
  • I ran just quick-check before committing
  • If the Wallet API was modified (added/removed/changed), I have reflected those changes in the FFI bindings (crates/cdk-ffi)

@crodas crodas self-assigned this Sep 17, 2026
@github-project-automation github-project-automation Bot moved this to Backlog in CDK Sep 17, 2026
@crodas
crodas marked this pull request as ready for review September 17, 2026 17:06
@crodas

crodas commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator Author

@cdk-bot review

@comwanga

Copy link
Copy Markdown
Contributor

Could we retain the public Amount<()>::to_i64, Amount<()>::from_i64, and Amount<CurrencyUnit>::to_i64 methods here?

Moving the SQL persistence paths to checked i64::try_from(...) conversions addresses the unsafe narrowing from #2425, but removing these methods also turns the database-integrity fix into a public cashu::Amount API change.

This seems especially worth preserving because the split-out direction discussed on #2473 explicitly called for retaining the public Amount::to_i64() methods while fixing the checked database conversions separately. from_i64 is likewise unrelated to the persistence bug.

Unless removing these helpers is an intentional breaking API change, keeping them and their boundary tests would preserve downstream compatibility without affecting the database fix.

@crodas
crodas force-pushed the fix/sql-integer-overflow-simple branch from c7a7d64 to 08e5f8a Compare September 18, 2026 19:16
@crodas

crodas commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator Author

@cdk-bot review

@cdk-bot cdk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified findings approved for disclosure:

  • Wallet mint_quote reads now hard-fail on legacy negative amounts instead of returning amount: None (low) - see inline comment

Comment thread crates/cdk-sql-common/src/wallet/mod.rs
@crodas

crodas commented Sep 19, 2026

Copy link
Copy Markdown
Collaborator Author

@cdk-bot review

@j-kon j-kon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • No critical, warning, or nit-level issues found.
    • The migration safely widens integer columns to BIGINT for Postgres without altering immutable past migrations.
    • Replacing implicit truncations with checked try_from conversions ensures out-of-range database writes fail fast at the persistence layer rather than silently corrupting state.
    • Commit 08e5f8a4 properly restored the public Amount::to_i64 and Amount::from_i64 helpers along with thorough boundary unit tests, preserving API stability.

Summary

Reviewed database widening migrations, SQL parameter bindings, checked numeric conversions in cdk-sql-common and cdk-postgres, and the restored public Amount helpers. The changes prevent silent data truncation and keep public API backwards-compatible.

Verdict

APPROVE

@thesimplekid thesimplekid left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the things that give me most pause about the other PR is the database migration so the motivation of this PR was to avoid that and just error on anything we cannot store instead of silently wrapping and leave a migration to a follow up.

@github-project-automation github-project-automation Bot moved this from Backlog to In progress in CDK Sep 22, 2026
@crodas
crodas force-pushed the fix/sql-integer-overflow-simple branch from 08e5f8a to ea8720a Compare September 30, 2026 20:22
@crodas

crodas commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator Author

@cdk-bot review

Use checked integer conversions when binding SQL values so out-of-range
values return an error instead of being truncated or stored as NULL.
Preserve NULL for absent optional values.

Check PostgreSQL INT2 and INT4 bounds and decode INT2 values as i16.
Keep existing column widths; widening columns and supporting the full
u64 amount range remain separate work.
@thesimplekid
thesimplekid force-pushed the fix/sql-integer-overflow-simple branch from ea8720a to 4089d6c Compare October 1, 2026 07:34
@thesimplekid
thesimplekid merged commit 4089d6c into cashubtc:main Oct 1, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants