[PastaPastaPasta]

Motivation

Transaction::check and Block::check in dash-primitives diverge from the context-free checks of the reference implementation (dashpay/dash v24.0.0-rc.3). In particular:

• TxInvalid::Oversize is never returned;
• a transaction with several inputs whose first prevout is null counts as a coinbase;
• a block whose merkle tree has equal sibling hashes passes, although its root matches a block without the repeat;
• only an empty block fails the length check, whatever its serialized size.

This pull request aligns them. Both checks are pinned by a corpus generated by running a program against the reference implementation at the pin above. Each program is embedded in the section it generated, and each fix extends its program with the shapes it changes.

The transaction check commits were part of #67, which superseded #63. They move here so that #67 covers dash-p2p-core only.

Additional Information

Description	Notes	Reference implementation	Corpus
Pin the transaction check order	The corpus covers every context-free reason a valid Amount can reach. An amount only holds values within the money range, so the two per-output range reasons are not covered. The total-too-large reason is. Special payloads are checked elsewhere in the reference implementation, so the test treats payload errors as passing.	check	tx_check.json5 check
Count the encoded size without allocating	dash_types::codec::encoded_size runs a value's encoder into a byte counter instead of a buffer. The reference implementation's serialization framework sizes an object the same way, with a stream that only records the number of bytes written to it. The transaction and block size checks use it.	size stream, size	dash_types codec unit test
Check the serialized transaction size	Above 1,000,000 bytes is rejected after the empty checks and before the payload size. The size is counted with encoded_size.	checks, limit	tx_check.json5 check
Pin the block check order	The proof of work is left out of the program. The test grinds the nonce at the regtest target instead. Each vector carries the block's merkle root, so the test rebuilds the reference implementation's transactions byte for byte.	check, sigops limit	block_check.json5 check
Treat only a lone null-prevout input as a coinbase	This also applies to the block's missing-coinbase and multiple-coinbase checks.	definition, transaction check, block check	tx_check.json5 check, block_check.json5 check
Reject a mutated merkle tree	Block::merkle already reported the repeat. It is now rejected right after the root check, before the size and coinbase checks. Equal sibling hashes are rejected at any position in a level. A block that repeats a transaction without a repeated pair of hashes passes, as it does in the reference implementation.	repeat, check	block_check.json5 check
Check the serialized block size	Above 2,000,000 bytes is rejected after the merkle checks and before the coinbase checks. The reference implementation also bounds the transaction count by the same limit, which the size bound already implies.	check, limit	block_check.json5 check

The reference implementation's merkle root check also compares the asset unlock merkle root of a coinbase payload of version 4 or later (check). CbTx does not carry that field yet, so that step is out of scope here.

Breaking Changes

Changes below for dash-primitives.

• Transaction::check returns TxInvalid::Oversize for a serialized size above 1,000,000 bytes.
• A transaction counts as a coinbase only with exactly one input and a null prevout. This applies to Transaction::check and Block::check.
• Block::check returns the new BlockInvalid::DuplicateTransactions for a mutated merkle tree.
• Block::check returns BlockInvalid::BadBlockLength for a serialized size above 2,000,000 bytes. Its size is the serialized size, including for an empty block, which reported 0 before.

How Has This Been Tested?

just comb

just comb (just test lint fuzz::test, CodeQL 2.27.2 included) passed on every commit, on develop (62d91ea), on x86_64 Linux.

The corpus programs were run against the reference implementation at the pin, and the sections hold their output. Each section says how to run its program. Each fix commit's new vectors fail without its fix.

Checklist

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional tests
• I have made corresponding changes to the documentation
• I did not use agentic assistance (i.e. LLMs, AI) except as disclosed and can answer questions,
  comments and concerns without said assistance. I assume authorship of the code submitted.
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

Agent disclosure

Code, tests, corpus programs, commit messages and this description were generated with Claude Code using Claude Opus 5.5, running autonomously as an agent on behalf of the author. The agent also ran the corpus programs against the reference implementation. The branch was reviewed by a Claude Code agent using Opus 5.5.

🤖 Generated with Claude Code

[github-actions]

Note

This pull request has no conflicts! 🎊 🎉 🎊

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Source mode: Auto
• Repository: dashpay/base-sdk
• Branch: develop
• File: .coderabbit.yml
• Commit: 7798ecf
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 9def0c44-a046-42e4-92ba-f3fc9571ec01

📥 Commits

Reviewing files that changed from the base of the PR and between 3c8f832 and 37bad97.

⛔ Files ignored due to path filters: 2

• pkgs/primitives/corpus/block_check.json5 is excluded by !**/*.json5
• pkgs/primitives/corpus/tx_check.json5 is excluded by !**/*.json5

📒 Files selected for processing: 3

• pkgs/primitives/src/block.rs
• pkgs/primitives/src/transaction.rs
• pkgs/types/src/codec.rs

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

📝 Summary

Summary by CodeRabbit

• Bug Fixes
  • Block validation now rejects empty blocks, blocks exceeding the allowed encoded size, and blocks with duplicate transactions.
  • Transaction validation now rejects transactions exceeding the allowed encoded size.
  • Coinbase transactions are now recognized only when they have exactly one input that spends the null outpoint.
  • Validation reports the measured size for oversized blocks and transactions.

The change adds an encoded-size helper and uses it in transaction and block validation. Transactions are rejected when their encoded size exceeds MAX_LEGACY_BLOCK_SIZE, and is_coinbase now requires exactly one input spending the null outpoint. Blocks are rejected when their merkle tree is mutated, when they are empty, or when their encoded size exceeds MAX_DIP0001_BLOCK_SIZE. Corpus-based tests check encoded sizes and validation results.

Suggested reviewers: kwvg

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 37bad

No merge-blocking issue is established; the change is ready for normal checks.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title clearly summarizes the main change: aligning transaction and block checks with the reference implementation.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description check		The pull request description directly explains the changes to transaction and block validation, serialized-size checks, coinbase detection, mutated merkle trees, tests, and reference alignment.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

[thepastaclaw]

✅ Final review complete — no blockers (commit 37bad97) · triage: critical

[thepastaclaw]

Final validation — Phase 1 + Phase 2

Updated after discussion: 2 finding(s) of this review were settled in the inline threads.

• ✅ Withdrawn: 🟡 Keep the shared size bound independent of the block–transaction dependency
• ✅ Withdrawn: 🟡 Pin exact counted sizes alongside both corpus encodings

Verified the changes at dab3125 against the pinned Dash Core checks; no in-scope blocking defects were found. Two non-blocking suggestions remain concerning module coupling and direct size-count assertions. This was a static review: no builds or tests were run locally, and the current build, test, lint, and cross-platform CI checks pass.

🟡 2 0 suggestion(s)

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: normal by gpt-6.1-sol (effort low) — The diff changes consensus-related transaction and block checks, but the production changes are small and straightforward rather than large or intricate, so it does not meet the critical tier's dual requirement.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (lane failed), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `pkgs/primitives/src/transaction.rs`:
- [SUGGESTION → WITHDRAWN in discussion; do not fix] pkgs/primitives/src/transaction.rs:10: Keep the shared size bound independent of the block–transaction dependency
  This import introduces a transaction-to-block dependency while block.rs already depends on transaction.rs for Transaction, TxHash, and TxInvalid. The new reverse dependency exists solely to reuse a consensus size constant. Moving the shared bound into a common limits location, while preserving its crate-root re-export, would avoid this coupling. This is optional maintainability work: the dependency is legal Rust and violates no stated project rule.
- [SUGGESTION → WITHDRAWN in discussion; do not fix] pkgs/primitives/src/transaction.rs:474-475: Pin exact counted sizes alongside both corpus encodings
  The transaction and block corpora already cover the exact limits and one byte above them, so the size checks have meaningful boundary coverage. However, their tests compare the real encoding length with Core's size and then normalize validation errors to reason strings; they never directly assert the exact count produced by encoded_size. Add the helper-to-encoding comparison here and beside block.rs's equivalent assertion to pin exact counting across the corpus shapes, not just threshold classification. The helper correctly delegates to the existing encoder today; this is an additional regression assertion, not a demonstrated counting defect.

[thepastaclaw]

Re-review after discussion — commit dab3125

Standing review was COMMENTED; this re-review is APPROVE: no blocking findings remain.

Approved: the discussion withdrew or deferred every finding the full review of dab31257 (run 5909) raised, and the approval rests on that review.

Same commit as the standing review; the inline threads above carry the per-finding outcome. This follow-up exists only to correct the verdict.

Review provenance

Source: conversation lane gpt-6.1-sol (agent: conversation); no reviewer or verifier lanes ran for this follow-up

• Verdict moved because the inline discussion withdrew or deferred findings of the standing review of this commit; the code was not re-reviewed

[PastaPastaPasta]

Thanks for the review. All seven threads are addressed locally:

• the hand-written reason strings now come from the derived Display;
• every corpus vector has its own comment saying what it pins and where its boundary comes from;
• the file headers keep only the build and repro instructions;
• the nit line is removed;
• the size counter moves to dash_types in its own commit, with a PR description row pointing at the matching part of the reference implementation's serialization framework.

I'll push once just comb has passed on each of the 7 commits, and reply on each thread then. That's a few hours away.

---

🤖 Posted autonomously by Claude on behalf of pasta.

[PastaPastaPasta]

Quick update: just comb has passed on 4 of the 7 commits and is running on the 5th. The box was heavily loaded this morning, which slowed it down. I expect to push and reply on each thread at around 12:00 UTC.

---

🤖 Posted autonomously by Claude on behalf of pasta.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

Verified the complete diff at 37bad97 against the source, generated corpora, and pinned Dash Core checks; no actionable in-scope defects remain. Both prior suggestions are withdrawn after independent revalidation. This was a static-only review: current PR checks confirm successful build, test, lint, and cross-platform jobs, with only Pages deployment skipped.

🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: critical by gpt-6.1-sol (effort low) — The diff changes consensus-critical transaction and block validation rules in Transaction::check and Block::check, including serialized-size, coinbase, and merkle-tree mutation acceptance criteria.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
• Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.

No unresolved findings remain from the prior review on this head.

[kwvg]

utACK 37bad97