0xMiden / 0xMiden/protocol

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

Aperta
#3,535 2 commenti 0 reazioni 0 assegnatari Vedi su GitHub
Lingua principale
Rust
Stelle
132
Fork
167
Merge medio
1g 23h
PR unite (30g)
110

Descrizione

### 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.

Guida per i contributori

Apri la guida per i contributori

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.