Skip to content

Reject Ironwood commitments for v5 transactions#513

Merged
ebfull merged 1 commit into
feat/ironwoodfrom
ebfull/fallible-commitment
Jun 26, 2026
Merged

Reject Ironwood commitments for v5 transactions#513
ebfull merged 1 commit into
feat/ironwoodfrom
ebfull/fallible-commitment

Conversation

@ebfull

@ebfull ebfull commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Previously commitment_format ignored tx_version for the Ironwood pool, silently committing an Ironwood bundle under v6 personalization even when the caller requested TxVersion::V5. That should instead fail because it's unrepresentable.

This makes authorizing_commitment and the hash_bundle_*_empty helpers fallible, mirroring commitment, and threads CommitmentError through the internal hash_bundle_*_data helpers. We add a CommitmentError::InvalidTransactionVersion variant and return it from the commitment APIs.

Previously `commitment_format` ignored `tx_version` for the Ironwood
pool, silently committing an Ironwood bundle under v6 personalization
even when the caller requested `TxVersion::V5`. Add a
`CommitmentError::InvalidTransactionVersion` variant and return it from
the commitment APIs instead.

This makes `authorizing_commitment` and the `hash_bundle_*_empty`
helpers fallible, mirroring `commitment`, and threads
`CommitmentError` through the internal `hash_bundle_*_data` helpers.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@TalDerei TalDerei left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah the fallibility here seems good, no objection.

@ValarDragon ValarDragon left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works for me!

As a caller I've seen no realistic risk of this though. The real risk we have in calling, is confusion over which V5 Orchard choice you do in commitments, because they are all equivalent:

  • Pre NU6_2 , V5
  • NU6_2, V5
  • NU6_3 V5

@ebfull

ebfull commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator Author

This isn't a big deal, it's just something I wish we would have done in #510 in retrospect. I also expect it to be identified in audits; it was identified by zkao.

Technically, this is fixing a case where we actually return an incorrect value for an unrepresentable input. The cases you mention are all possible mistakes the caller can make, but at least the library cannot produce completely invalid results in those cases.

Probably the weirder examples are ones like @TalDerei suggested to me, such as OrchardPreNu6_2 with v6, which aren't possible in practice (due to separate consensus restrictions) but still have coherent and specified results in the digest computation.

@TalDerei

TalDerei commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

it was identified by zkao

forgot to mention, v12 didn't surface anything new (only some duplicative informational findings that are outside our threat model).

@ebfull ebfull merged commit 30c4ea2 into feat/ironwood Jun 26, 2026
16 checks passed
@ebfull ebfull deleted the ebfull/fallible-commitment branch June 26, 2026 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants