0xMiden / 0xMiden/protocol

TransactionInputs::read_from skips all three invariants that TransactionInputs::new enforces

Open
#3,535 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
132
Forks
167
Avg merge
1d 23h
Merged PRs (30d)
110

Description

### Summary

`TransactionInputs::new` enforces three invariants; `TransactionInputs::read_from` enforces none of them. Same class as #3494 (`BlockSignatures`) and #3533 (`PartialStorage`): a validating constructor exists, but the `Deserializable` impl builds the struct straight from the deserialized fields.

`new` checks (`crates/miden-protocol/src/transaction/inputs/mod.rs:77`):

1. `blockchain.chain_length() == block_header.block_num()`
2. `blockchain.peaks().hash_peaks() == block_header.chain_commitment()`
3. every `InputNote::Authenticated` proof validates against its block header, via `validate_is_in_block`

`read_from` in the same file reads the fields and returns `Ok(TransactionInputs { .. })`, calling none of them.

### Reproduction

Two individually valid values taken at different chain heights, spliced so that the header and the partial blockchain disagree:

```
early: header=1 chain_len=1
late : header=3 chain_len=3

new() -> Err("partial blockchain has length 1 which does not match block number 3")
read_from -> Ok (header=3, chain_len=1)
```

Test (passes against next at 3486370)

```rust
use miden_protocol::transaction::TransactionInputs;
use miden_protocol::utils::serde::{Deserializable, Serializable};
use miden_testing::{Auth, MockChain};

#[test]
fn read_from_accepts_what_new_rejects() -> anyhow::Result<()> {
let mut builder = MockChain::builder();
let account = builder.add_existing_wallet(Auth::IncrNonce)?;
let mut chain = builder.build()?;

chain.prove_next_block()?;
let early_block = chain.latest_block_header().block_num();
chain.prove_next_block()?;
chain.prove_next_block()?;
let late_block = chain.latest_block_header().block_num();

let early = chain.get_transaction_inputs_at(early_block, &account, &[], &[])?;
let late = chain.get_transaction_inputs_at(late_block, &account, &[], &[])?;

// `new` rejects the mismatched pair.
assert!(
TransactionInputs::new(
early.account().clone(),
late.block_header().clone(),
early.blockchain().clone(),
early.input_notes().clone(),
)
.is_err()
);

// The same mismatch, assembled as bytes in the field order `read_from` expects.
let early_bytes = early.to_bytes();
let account_len = early.account().to_bytes().len();
let early_header_len = early.block_header().to_bytes().len();

let mut spliced = Vec::new();
spliced.extend_from_slice(&early_bytes[..account_len]);
spliced.extend_from_slice(&late.block_header().to_bytes());
spliced.extend_from_slice(&early_bytes[account_len + early_header_len..]);

let decoded = TransactionInputs::read_from_bytes(&spliced).unwrap();
assert_ne!(
u32::from(decoded.block_header().block_num()),
decoded.blockchain().chain_length() as u32,
"deserialized an inconsistent chain view"
);

Ok(())
}
```

### Severity

Lower than it first looks, and I would rather say so than overstate it. The transaction kernel prologue independently authenticates note inclusion (`authenticate_note` in `prologue.masm`, `ERR_PROLOGUE_NOTE_AUTHENTICATION_FAILED`), so a forged `TransactionInputs` fails during execution rather than yielding a valid proof. The Rust-side check is defense in depth, not the boundary — which is why I am filing this publicly rather than as an advisory. Re-triage if you disagree.

What it does mean is that any component consuming serialized `TransactionInputs` — delegated proving being the obvious one — can hold a value whose invariants no constructor would allow, with the failure surfacing later and less legibly than it should.

### Why this one and not its siblings

I checked the neighbours while I was in here. `ProvenBlock`, `SignedBlock` and `ProposedBatch` also build directly in `read_from`, but each has a documented `new_unchecked` / `new_unverified`, so bypassing validation there is a deliberate, named choice. `TransactionInputs` has no such escape hatch, which is what makes this look like an oversight rather than a decision.

### Fix

Have `read_from` run the same checks as `new`. Happy to send a PR with the regression test if this can be assigned to me.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.