Skip to content

Conversation

@wolf31o2
Copy link
Member

@wolf31o2 wolf31o2 commented Nov 30, 2025

Summary by cubic

Shifted transaction validity handling to the database layer to ensure consistent state updates and simplify ledger logic. Invalid transactions now consume collateral UTxOs and skip certificates, datums, and protocol updates.

  • Refactors
    • Persist tx validity via a new Transaction.Valid field.
    • UTxO consumption moved to DB: valid uses Consumed; invalid uses Collateral.
    • Certificates and datum storage run only for valid transactions.
    • Protocol parameter updates gated by tx.IsValid().
    • Removed UtxoConsume and related ledger helpers; invalid-tx path stripped from ledger/delta.go.
    • Improved UTxO-not-found logging with hash/index context.

Written for commit a37f6a6. Summary will update automatically on new commits.

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Improved handling of invalid transactions throughout the system, ensuring proper validity checks are applied during processing.
    • Enhanced protocol parameter update logic to account for transaction validity status.
  • Chores

    • Refined transaction data model for better validity state tracking.
    • Removed deprecated UTXO handling logic.

✏️ Tip: You can customize this high-level summary in your review settings.

@wolf31o2 wolf31o2 requested a review from a team as a code owner November 30, 2025 01:18
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 30, 2025

📝 Walkthrough

Walkthrough

This PR refactors transaction validity handling across the database and ledger layers. A Valid bool field is added to the Transaction struct, and invalid transaction processing is consolidated into the standard transaction path. Special handling for invalid transactions—including UTxO consumption marking and separate record creation—is removed. Certificate processing and protocol parameter updates now execute only when transactions are valid. The public UtxoConsume method and the consumeUtxo helper function are deleted, eliminating intermediate transaction state management in favor of a unified validity-aware processing flow.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • database/plugin/metadata/sqlite/transaction.go: Major refactoring with expanded certificate handling logic, multiple new conditional branches (pool, registration, delegation, etc.), and validity-based input selection logic—requires careful tracing of all certificate type paths.
  • Removal of public/internal methods: UtxoConsume and consumeUtxo deletion requires verification that no orphaned call sites remain across the codebase.
  • ledger/delta.go: Elimination of invalid transaction special-case handling—needs validation that the removal doesn't break transaction recording or state consistency.
  • Multi-layer coordination: Changes span database models, plugin metadata handling, database layer, and ledger state, requiring cross-layer verification of consistency and data flow.

Possibly related PRs

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main refactoring objective—moving transaction validity checks into the database layer, which is reflected across multiple files in the changeset.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch refactor/transaction-validity

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fdbe2a4 and a37f6a6.

📒 Files selected for processing (6)
  • database/models/transaction.go (1 hunks)
  • database/plugin/metadata/sqlite/transaction.go (4 hunks)
  • database/transaction.go (1 hunks)
  • database/utxo.go (0 hunks)
  • ledger/delta.go (0 hunks)
  • ledger/state.go (0 hunks)
💤 Files with no reviewable changes (3)
  • ledger/state.go
  • database/utxo.go
  • ledger/delta.go
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 975
File: database/models/transaction.go:21-26
Timestamp: 2025-10-24T21:56:15.978Z
Learning: In database/models/transaction.go, the Transaction struct uses a dual foreign-key pattern for UTXO associations: Outputs []Utxo uses TransactionID→ID (where UTXOs were created), and Inputs []Utxo uses SpentAtTxId→Hash (where UTXOs were consumed). Explicit GORM tags `gorm:"foreignKey:SpentAtTxId;references:Hash"` for Inputs and `gorm:"foreignKey:TransactionID;references:ID"` for Outputs prevent association conflicts and preserve UTXO provenance.
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 977
File: database/plugin/metadata/sqlite/transaction.go:71-81
Timestamp: 2025-10-26T14:12:53.587Z
Learning: In Babbage-era transactions (gouroboros ledger), the Produced() method behavior depends on transaction validity: for valid transactions (IsValid() == true), Produced() returns all transaction outputs; for invalid transactions (IsValid() == false), Produced() returns only the collateral return UTXO (if non-nil) with index len(t.Outputs()). The collateral return Output is set to t.CollateralReturn() in the returned Utxo struct.
📚 Learning: 2025-10-24T21:56:15.978Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 975
File: database/models/transaction.go:21-26
Timestamp: 2025-10-24T21:56:15.978Z
Learning: In database/models/transaction.go, the Transaction struct uses a dual foreign-key pattern for UTXO associations: Outputs []Utxo uses TransactionID→ID (where UTXOs were created), and Inputs []Utxo uses SpentAtTxId→Hash (where UTXOs were consumed). Explicit GORM tags `gorm:"foreignKey:SpentAtTxId;references:Hash"` for Inputs and `gorm:"foreignKey:TransactionID;references:ID"` for Outputs prevent association conflicts and preserve UTXO provenance.

Applied to files:

  • database/models/transaction.go
  • database/plugin/metadata/sqlite/transaction.go
📚 Learning: 2025-10-26T14:12:53.587Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 977
File: database/plugin/metadata/sqlite/transaction.go:71-81
Timestamp: 2025-10-26T14:12:53.587Z
Learning: In Babbage-era transactions (gouroboros ledger), the Produced() method behavior depends on transaction validity: for valid transactions (IsValid() == true), Produced() returns all transaction outputs; for invalid transactions (IsValid() == false), Produced() returns only the collateral return UTXO (if non-nil) with index len(t.Outputs()). The collateral return Output is set to t.CollateralReturn() in the returned Utxo struct.

Applied to files:

  • database/plugin/metadata/sqlite/transaction.go
📚 Learning: 2025-10-22T20:13:35.972Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T20:13:35.972Z
Learning: In utxorpc code paths, always operate on github.com/blinklabs-io/gouroboros/ledger/common.Block. Use database/models.Block only as the storage type and call models.Block.Decode() to obtain a ledger/common.Block before further processing.

Applied to files:

  • database/plugin/metadata/sqlite/transaction.go
🧬 Code graph analysis (1)
database/plugin/metadata/sqlite/transaction.go (7)
database/models/pool.go (10)
  • ErrPoolNotFound (24-24)
  • Pool (26-38)
  • PoolRegistration (44-60)
  • PoolRegistration (62-64)
  • PoolRegistrationOwner (66-71)
  • PoolRegistrationOwner (73-75)
  • PoolRegistrationRelay (77-85)
  • PoolRegistrationRelay (87-89)
  • PoolRetirement (91-98)
  • PoolRetirement (100-102)
database/types/types.go (2)
  • Uint64 (54-54)
  • Rat (25-27)
database/models/account.go (15)
  • StakeRegistration (107-113)
  • StakeRegistration (115-117)
  • Account (27-35)
  • StakeDelegation (84-90)
  • StakeDelegation (92-94)
  • StakeRegistrationDelegation (119-126)
  • StakeRegistrationDelegation (128-130)
  • Registration (72-78)
  • Registration (80-82)
  • StakeVoteRegistrationDelegation (145-153)
  • StakeVoteRegistrationDelegation (155-157)
  • VoteRegistrationDelegation (171-178)
  • VoteRegistrationDelegation (180-182)
  • VoteDelegation (159-165)
  • VoteDelegation (167-169)
database/models/epoch.go (2)
  • Epoch (17-27)
  • Epoch (29-31)
database/models/drep.go (7)
  • RegistrationDrep (45-53)
  • RegistrationDrep (55-57)
  • DeregistrationDrep (33-39)
  • DeregistrationDrep (41-43)
  • UpdateDrep (59-66)
  • UpdateDrep (68-70)
  • Drep (19-27)
database/models/auth_committee_hot.go (2)
  • AuthCommitteeHot (17-22)
  • AuthCommitteeHot (24-26)
database/models/resign_committee_cold.go (2)
  • ResignCommitteeCold (17-23)
  • ResignCommitteeCold (25-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
  • GitHub Check: cubic · AI code reviewer
  • GitHub Check: go-test (1.24.x, macos-latest)
  • GitHub Check: go-test (1.25.x, windows-latest)
  • GitHub Check: go-test (1.25.x, macos-latest)
  • GitHub Check: go-test (1.24.x, windows-latest)
  • GitHub Check: nilaway
  • GitHub Check: Analyze (go)
🔇 Additional comments (6)
database/transaction.go (1)

62-62: LGTM: Protocol parameter updates correctly guarded by transaction validity.

This ensures that only valid transactions can update protocol parameters, which aligns with Cardano ledger rules where invalid transactions do not affect on-chain state beyond consuming collateral.

database/models/transaction.go (1)

34-34: LGTM: Valid field addition supports validity-aware processing.

The new Valid field enables the database layer to track transaction validity, supporting the refactored processing logic where invalid transactions consume collateral and skip certificate processing.

database/plugin/metadata/sqlite/transaction.go (4)

132-132: LGTM: Transaction validity correctly captured from ledger.

The Valid field is properly populated from tx.IsValid(), preserving the transaction's validity state in the database.


372-771: LGTM: Certificate processing correctly guarded by transaction validity.

Wrapping all certificate processing in if tx.IsValid() is correct - invalid transactions should not modify stake pool registrations, delegations, governance state, or any other on-chain state managed by certificates. Only collateral consumption should occur for invalid transactions.

The comprehensive certificate handling covers all major certificate types with consistent error handling patterns. The deposit tracking (lines 376-384) and the warning for missing deposits on deposit-bearing certificates demonstrates good defensive coding.


323-363: Validity-based input consumption correctly aligns with Cardano ledger semantics across all eras.

This consolidates UTxO consumption into a single path by selecting the appropriate input set based on transaction validity:

  • Valid transactions consume regular inputs (tx.Consumed())
  • Invalid transactions consume collateral inputs (tx.Collateral())

Verification confirms this works correctly across all supported eras:

  • Babbage and later (with collateral support): Invalid transactions consume only collateral inputs; valid transactions consume regular inputs
  • Byron through Shelley (pre-collateral): tx.IsValid() is always true, tx.Collateral() returns an empty slice, so the logic correctly consumes all inputs
  • CollateralReturn is handled separately and correctly tracked at lines 138-144

The implementation is sound and requires no changes.


768-770: <!-- [scratchpad_start]
[task verification_1/1 CONCLUSION_READY]
Have sufficient evidence to conclude verification.

[observations]

  • Collateral return is part of tx.Produced() (line 140)
  • Collateral return is stored in tmpTx.CollateralReturn (line 142)
  • Collateral inputs (used FOR collateral) are separate from collateral return (output FROM failed tx)
  • The code explicitly handles collateral return in the output loop

[key_insight]
The collateral return IS included in tx.Produced(). So:

  • For VALID transactions: storeTransactionDatums stores datums from all outputs (including any collateral return)
  • For INVALID transactions: storeTransactionDatums is NOT called, so NO datums are stored

The question is whether collateral return datums should be stored for invalid transactions.

[analysis]
Looking at Cardano protocol:

  • Collateral return is a UTxO returned to the sender when script fails
  • Collateral return can have an inline datum (it's just a UTxO)
  • However, the review comment's concern is valid - should we store datums for invalid transaction outputs?

Current behavior (with IsValid() guard):

  • Datums stored only for valid transactions
  • Collateral return datums NOT stored for invalid transactions

This could be intentional because:

  1. Collateral returns are less important to track (already in the UTxO table with CBOR)
  2. Most downstream queries probably don't need to search for datums from collateral returns
  3. The separate datum table might be for important datums used in script execution

The review comment appropriately asks for confirmation. This is a potentially subtle behavior change that could affect clients.

[done]
Verification complete. The concern is valid but the behavior appears intentional. Need to flag for developer to confirm intent.


</function_calls>

Based on my verification, I can now provide the rewritten review comment.

Confirm that datum storage exclusion for invalid transactions is intentional.

The storeTransactionDatums call is guarded by tx.IsValid() (inside the guard block at line 372), meaning datums—including those from the collateral return output—are only stored for valid transactions. This appears to be intentional per the refactoring commit ("move transaction validity checks to database"), but please confirm:

  1. Collateral return outputs can have inline datums in Cardano; should these datums be stored in the datum table for invalid transactions?
  2. Are there downstream queries or chain validation logic that depend on datums from invalid transaction outputs?

If datums from collateral returns should be stored regardless of transaction validity, consider moving the storeTransactionDatums call outside the tx.IsValid() guard, or adding a separate call to store only collateral return datums for invalid transactions.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 6 files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants