fix: validate tx graph finality

[Kukks]

TxGraph::new checked structure and node count but never that a node is actually broadcastable. It now also requires a zero locktime and a final sequence on every chunk.

The check sits in the chunk-ingestion loop, so it covers every chunk before any graph assembly rather than only the reachable nodes. The operator-side builder already emits exactly these values, so well-formed graphs are unaffected.

Test plan

• Four new unit tests: accepts a final graph, rejects a non-zero locktime, rejects a non-final sequence, and rejects a non-final sequence on a non-root chunk. The reject cases assert the message names both the offending value and the txid.
• Commenting the check out makes all three reject tests fail while the accept case still passes.
• cargo test --workspace --exclude e2e-tests: 279 passed, 0 failed (baseline 275/0 — delta is exactly the four new tests). cargo clippy --all-targets --all-features clean.

Summary by CodeRabbit

• Bug Fixes
  • Transaction graph construction now rejects chunks with unsupported lock times or input sequences.
  • Invalid non-root graph chunks are detected before graph construction, improving validation and reliability.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 4eb324b0-6522-424a-9ec2-80dd5e31238a

📥 Commits

Reviewing files that changed from the base of the PR and between 59818f8 and 58a906d.

📒 Files selected for processing (1)

• ark-core/src/tx_graph.rs

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

Walkthrough

TxGraph::new now validates transaction finality before graph construction. It rejects non-zero lock times and input sequences other than Sequence::MAX. Tests cover valid graphs and invalid root and child chunks.

Changes

Transaction graph finality

Layer / File(s)	Summary
Finality validation during graph construction ark-core/src/tx_graph.rs	TxGraph::new validates each transaction before insertion. Validation rejects non-zero lock times and non-final input sequences with transaction-specific errors.
Finality validation tests ark-core/src/tx_graph.rs	Tests cover accepted final graphs and rejection of invalid lock times and sequences in root and child chunks.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: 🔵 Low · up to 58a90

The PR rejects transaction graphs with non-zero locktime or non-final input sequences before signing, improving admission safety without changing well-formed graphs. Merge is reasonable with explicit owner awareness that external graph producers must emit the stricter values or risk bounded availability failures in signing or recovery flows.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 1 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: validating transaction graph finality during construction.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 2

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/tx-validation-hardening

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Kukks]

CI red here is pre-existing and unrelated to this change — flagging so it isn't mistaken for a regression.

All three failures (clippy, wasm_ubuntu, wasm_macos) stop at dependency resolution before any code is compiled:

error: failed to select a version for the requirement `secp256k1 = "^0.32.0-beta.2"`
  version 0.32.0-beta.2 is yanked
required by package `ark-client v0.10.1`

Cargo.lock is gitignored (.gitignore:8), so CI always resolves fresh and now picks up the yank. The previous ci.yml run was 2026-08-18 and was green, so the yank landed in the interim and this PR is simply the first run since — any PR opened today fails identically.

Locally, with the repo's documented workaround from docs/cargo-minimal-lock.md (cp Cargo-minimal.lock Cargo.lock):

• cargo test -p ark-core — 96 passed, 0 failed (baseline 92, delta is exactly the four new tests)
• cargo clippy -p ark-core --all-targets -- -D warnings — clean
• cargo clippy --all-targets --all-features — clean across the workspace

Unblocking CI needs either a secp256k1 bump or a committed lockfile, which felt out of scope for a security fix. Happy to do it separately if you'd like it folded in.

[arkana-ai-bot]

Review — fix: validate tx graph finality

Reviewed at head 58a906d. Scope: ark-core/src/tx_graph.rs (117 added lines, 0 deleted). Cross-repo impact checked against arkd, ts-sdk, go-sdk, dotnet-sdk.

Protocol / correctness

validate_finality logic is sound. lock_time == 0 and every input.sequence == Sequence::MAX (0xFFFFFFFF) is the strictest possible finality condition — it guarantees tree nodes are unconditionally broadcastable regardless of chain state and explicitly opts out of both BIP68 relative timelocks and RBF. Confirmed correct for the protocol: arkd/pkg/ark-lib/tree/builder.go:228 constructs every VTXO-tree node with psbt.New(…, 3, 0, []uint32{wire.MaxTxInSequenceNum}), so well-formed operator graphs will always pass.

Early validation is the right placement. The check sits inside the chunks loop before chunks_by_txid.insert, so every chunk is screened before graph assembly begins. A malformed node deep in the tree cannot survive by being unreachable in the traversal.

Conservative lock_time rejection. A tx with lock_time != 0 but all sequences at MAX would be immediately broadcastable under Bitcoin consensus (MAX sequence disables lock_time). This PR rejects it anyway — which is intentional defence-in-depth: if a signing bug later produces a non-MAX sequence, a non-zero lock_time that was previously masked would suddenly make the graph unbroadcastable exactly when needed. The restriction is appropriate here; the error message will guide operators to the offending tx.

Minor observations (no changes required)

Zero-input chunk passes silently (tx_graph.rs, new validate_finality): the for input in &tx.unsigned_tx.input loop is a no-op when the input list is empty, so a hypothetical input-less chunk would pass finality checks. Such a tx would be consensus-invalid and would never be produced by the operator builder, so this is not actionable — just worth being aware of.

Cross-SDK inconsistency: ts-sdk/src/tree/validation.ts (validateVtxoTxGraph) and dotnet-sdk/NArk.Core/Batches/TreeValidator.cs (ValidateVtxoTxGraph) do not have equivalent finality checks. This is not a regression introduced here, but it means TS/dotnet clients currently accept graphs with non-zero locktimes or non-MAX sequences. Whether to mirror the check in those SDKs is a separate decision.

Tests

Four tests covering accept, reject-locktime, reject-sequence-on-root, and reject-sequence-on-non-root-chunk — the happy path and all three rejection branches. Error message assertions verify both the offending value and the txid, which is the right granularity. Mutation test described in the PR body (commenting out the check breaks the three reject tests, passes the accept test) is a good sanity signal.

Danger false-positive: CI flagged "source changed with no test changes" — 95 lines of new tests are clearly present. The warning can be ignored.

Verdict

Implementation is correct, tests are thorough, cross-repo impact is nil (private method, no API surface change). No protocol paths (VTXO handling, signing, forfeit, round lifecycle, unilateral exit) are modified. No changes requested.

[bonomat]

thank you

[bonomat]

Can you rebase to get the CI green please?