TransactionInputs::read_from skips all three invariants that TransactionInputs::new enforces
- 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.