0xMiden / 0xMiden/protocol

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

Đang mở
#3,535 2 bình luận 0 reaction 0 người được giao Xem trên GitHub
Ngôn ngữ chính
Rust
Star
132
Fork
167
Merge trung bình
1 ngày 23 giờ
Pull request đã merge (30 ngày)
110

Mô tả

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

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.