Repository navigation
feat(burn)!: write burn claim proofs with a burn output proof - #145
Conversation
Adopt the burn claim proof shape from tari-project/tari#8054, which proves the burn output was mined (against the block's `block_output_mr`) instead of the burn kernel. - Fetch `/generate_burn_output_proof?commitment=` instead of `/generate_kernel_merkle_proof`. - Reject a proof whose output does not hash to the expected burn output hash. - Use `tari_sidechain::{BurnClaimProof, CompleteClaimBurnProof}` instead of local mirrors, and fill the now-required `mined_in_epoch` from the proof's block height. - Point the tari crates at `../tari` path deps until the change is released. BREAKING CHANGE: the burn proof file format changes and needs a base node that serves `/generate_burn_output_proof`.
sdbondi
left a comment
There was a problem hiding this comment.
Posted by Claude.
Comment only — 0 blocking, 2 non-blocking, 1 nit · head 1b77a51
Own PR, and CI is red only because the ../tari path deps don't exist on the runner (cargo metadata fails to load the manifest), so no approve click. Path deps not raised, per the draft note.
Checked against tari at 3792c5008: BurnClaimProof/CompleteClaimBurnProof field shapes, GenerateBurnOutputProofResponse { proof }, the node handler's hex commitment query param, and OutputHashPreimageExt::hash (documented as matching TransactionOutput::hash, so comparing against the scanner-stored output_hash is sound). mined_in_epoch is computed identically to the console wallet's transaction_event_handler.rs (consensus_constants(height).block_height_to_epoch(height)). Locally cargo test -p minotari burn (12 pass) and clippy on minotari are clean.
Non-blocking
- The output-hash guard is untested —
minotari/src/tasks/burn_proof_worker.rs:115. It's the one new piece of logic in the worker, and nothing exercises either branch; the tests only coverassemble_complete_proof. Pulling the check into a small pure fn (fn check_output_proof(expected: &FixedHash, proof: &BurnOutputProof) -> Result<()>) would make both the match and the mismatch path a two-line test each. The test fixture already builds anOutputHashPreimagefromTransactionOutput::default(), so the expected hash is one.hash()away. - Pruned-node
410 GONEretries forever atwarn—burn_proof_worker.rs:95. The description notes a pruned node can only prove burns within its horizon; once a burn is outside it, the worker logs the same warning every 60s indefinitely and the proof never completes. Not a regression in kind (the kernel endpoint had the same failure mode), but the new endpoint gives a distinct status for it — worth either surfacing it distinctly ("use an archival node") or at least noting it in theburn_proofs_dir/burn-fundsdocs.
Nits
DbBurnProofstill carrieskernel_excess*,sender_offset_public_key,kernel_fee,kernel_lock_height, andget_pending_burn_proofsstill selects them, though nothing reads them now. Fine to keep the columns to avoid a migration as stated, but a one-line comment on the struct saying they're vestigial would stop the next reader wondering what uses them. Thepending_merklestatus name is similarly stale.
Sound
- Removal of the local mirror structs in favour of
tari_sidechaintypes; the JSON round-trip test covers the new requiredmined_in_epoch. - Rejecting a mismatched proof and retrying rather than writing a useless file.
BurnProofWorker::newbuilding oneConsensusManagerup front rather than per proof.- Doc/openapi wording updates are consistent across all touched sites.
- Move the output hash check into check_output_proof and test both branches. - Log a 410 GONE from the base node as an error that points at an archival node, instead of the generic retry warning. - Note the vestigial burn_proofs fields and the stale pending_merkle status name, and document the pruning limit on burn_proofs_dir.
|
Posted by Claude. Addressed in 2079d7b:
|
sdbondi
left a comment
There was a problem hiding this comment.
Posted by Claude.
Comment only — 0 blocking, 0 non-blocking, 0 nits · 3 resolved since 1b77a51 (2 non-blocking, 1 nit) · head 2079d7b
Own PR, and CI is still red only on the ../tari path deps, so this is a comment rather than an approval. On the code, nothing stands.
Resolved
- Output-hash guard untested: now
check_output_proof(burn_proof_worker.rs:162), with a match test and a mismatch test. The matching test derives the expected hash from the fixture's preimage, so it exercises the realOutputHashPreimageExt::hashpath. - Pruned-node
410retries silently atwarn:is_pruned_block_errordowncasts toHttpError::ServerError { status: GONE }. Verified this works:send_requestreturnsHttpErrorfor any non-2xx, andget_burn_output_proofconverts it with a bare?, so the downcast target is not wrapped in context.reqwest-retry's default policy does not retry 410, so the error reaches the worker straight away. It logs aterror!with an "use an archival node" hint, and theburn_proofs_dirdoc now carries the same caveat. The test covers GONE, NOT_FOUND and a non-HTTP error. - Vestigial DB fields /
pending_merklename: now documented onDbBurnProofand the status constant.
cargo test -p minotari burn_proof_worker (9 pass) and clippy on minotari are clean locally at this head.
brianp
left a comment
There was a problem hiding this comment.
Reviewed against tari#8054 and the published 6.0.1-pre.1 crates. The contract lines up: hex `commitment` query param, `BlockBodyPruned -> 410 GONE`, and the `mined_in_epoch` computation is identical to the console wallet's. `cargo test -p minotari burn` (15 passed) and `cargo clippy -p minotari --all-targets` are clean locally, CI green.
Non-blocking observations, none introduced by this diff:
- Downstream shape mismatch: Universe's `ootle/claim.rs` and tari-ootle's `MinotariBurnClaimProof` still parse the old shape (`commitment`/`encoded_merkle_proof`/`kernel`). Until both ship the new type, a proof file written by this build is not claimable. Worth a line in Breaking Changes so the bump order is explicit. `BurnProofWorker::new` gaining `Network` also breaks Universe's call site (compile-time, easy).
- Pruned branch log: the new `error!` arm drops the `error:% = e` field the `warn!` arm has, so the node's 410 body never gets logged on the one path where it is most useful. Also re-fetches every 60s forever with no backoff; a `pruned` status would stop that, but fine to defer.
- Epoch calc untested: the only new arithmetic (`consensus_constants(height).block_height_to_epoch(height)`) sits behind the HTTP call and every test threads a literal epoch in. Passing the `ConsensusManager` into `assemble_complete_proof` would let one LocalNet test pin it.
- Pre-existing and out of scope: no confirmation-depth gate before finalising (reorg leaves a stale proof + epoch), `block_hash`/`block_height` from the node are not cross-checked against the wallet's own scanned output row, and `write_proof_file` is not atomic relative to `mark_burn_proof_complete`.
🤖 Generated with Claude Code
…the epoch calculation
|
Posted by Claude. Thanks @brianp. Addressed in 556ff28:
I didn't change the out-of-scope items (confirmation depth, block hash cross-check, atomic write). |
Description
Adopts the burn claim proof shape from tari-project/tari#8054: the proof shows the burn output was mined (against
block_output_mr) instead of the kernel.BurnProofWorkerfetches/generate_burn_output_proof?commitment=<hex>instead of/generate_kernel_merkle_proof.CompleteClaimBurnProof/BurnClaimProof/AbridgedTransactionKernelmirrors are replaced by thetari_sidechaintypes.mined_in_epoch(now required) is computed from the proof's block height using the network's consensus constants.BurnProofWorker::newnow takes theNetwork.openapi.jsonare updated.The tari crates are bumped to
6.0.1-pre.1, which includes tari-project/tari#8054.The
burn_proofstable still stores the kernel fields and sender offset key. They are no longer used to build the proof file but are left in place to avoid a migration.Motivation and Context
Lets Ootle bind a burn claim to the burn output's features (
sidechain_id,claim_public_key), which the kernel cannot carry.How Has This Been Tested?
cargo test -p minotari burnpasses, including updatedassemble_complete_prooftests and a JSON round-trip of the complete proof.cargo clippy -p minotari --all-targetsis clean.Breaking Changes
output_proofreplacescommitment,encoded_merkle_proof,kernelandsender_offset_public_key, andmined_in_epochis required.ootle/claim.rs) and tari-ootle (MinotariBurnClaimProof) must ship the newtari_sidechaintype first or together.BurnProofWorker::newtakes aNetwork, which breaks Universe's call site at compile time./generate_burn_output_proof. A pruned node can only prove burns within its pruning horizon.