Skip to content

fix: amino JSON signing for MsgSetRoutingIsmDomain#157

Merged
mbreithecker merged 8 commits intobcp-innovations:mainfrom
rootulp:rootulp/reproduce-amino-json-signing-bug
Feb 11, 2026
Merged

fix: amino JSON signing for MsgSetRoutingIsmDomain#157
mbreithecker merged 8 commits intobcp-innovations:mainfrom
rootulp:rootulp/reproduce-amino-json-signing-bug

Conversation

@rootulp
Copy link
Copy Markdown
Contributor

@rootulp rootulp commented Feb 9, 2026

Closes #156

Summary

  • Fix Amino JSON signing bug where multisig transactions containing MsgSetRoutingIsmDomain fail with "unknown protobuf field" for the nested Route message
  • Add proto_init.go to register types.proto before tx.proto, ensuring Route resolves correctly
  • Add a regression test that verifies GetSignBytes succeeds for MsgSetRoutingIsmDomain

Root Cause

Go runs init() functions in alphabetical file order. Since tx.pb.go sorts before types.pb.go ('x' < 'y'), tx.proto's file descriptor was registered first. tx.proto references the Route message from types.proto, but types.proto wasn't in the registry yet, so Route became a placeholder with 0 fields. RejectUnknownFields used this broken placeholder and rejected all Route fields as unknown.

Fix

proto_init.go sorts before tx.pb.go ('p' < 't') and registers types.proto early. When tx.pb.go's init() subsequently processes tx.proto, Route resolves correctly from the registry with its 2 fields.

Context

See celestia-app#6541 for the original report. This blocked Celestia's multisig signers from submitting MsgSetRoutingIsmDomain transactions.

Test plan

  • go test ./x/core/01_interchain_security/types/ -run TestAminoJSONSigningMsgSetRoutingIsmDomain -v passes
  • All 28 existing specs in the package still pass

🤖 Generated with Claude Code

rootulp and others added 2 commits February 9, 2026 14:28
Add a unit test that reproduces the bug where Amino JSON signing fails
for MsgSetRoutingIsmDomain with "unknown protobuf field" because the
nested Route message's field descriptors are not resolvable via
gogoproto.HybridResolver. Also add a TxConfig() accessor to simapp.App
for future integration-level amino signing tests.

Closes bcp-innovations#156

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Go runs init() functions in alphabetical file order. Since tx.pb.go
sorts before types.pb.go, tx.proto's file descriptor was registered
first. tx.proto references the Route message from types.proto, but
types.proto wasn't in the registry yet, so Route became a placeholder
with 0 fields. This broke Amino JSON signing because RejectUnknownFields
saw the 0-field placeholder and rejected all Route fields as unknown.

Add proto_init.go which sorts before tx.pb.go ('p' < 't') and registers
types.proto early. When tx.pb.go's init() subsequently processes
tx.proto, it resolves Route correctly from the registry.

Closes bcp-innovations#156

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@rootulp rootulp changed the title test: reproduce Amino JSON signing bug for MsgSetRoutingIsmDomain fix(ism): fix Amino JSON signing for MsgSetRoutingIsmDomain Feb 9, 2026
@rootulp
Copy link
Copy Markdown
Contributor Author

rootulp commented Feb 9, 2026

IMO this was super hacky so I researched two alternatives:

  1. Rename types.proto to something that sorts before tx.proto — e.g. models.proto or domain.proto ('d' < 't', 'm' < 't'). This would fix the ordering naturally but requires regenerating all protobuf code and updating all import paths.
  2. Move Route into tx.proto — eliminates the cross-file dependency entirely, but scatters the data model.

@rootulp
Copy link
Copy Markdown
Contributor Author

rootulp commented Feb 9, 2026

The same bug is likely also occurring for warp tx.proto and types.proto because tx.proto imports RemoteRouter which is defined in types.proto but that will be registered after tx.proto.

MsgEnrollRemoteRouter and MsgRemoteTransfer likely can't be signed by multisigs via Amino either.

@rootulp
Copy link
Copy Markdown
Contributor Author

rootulp commented Feb 9, 2026

TBH I think this implementation is hacky and doesn't resolve it for all modules. Going to opt for

Rename types.proto to something that sorts before tx.proto — e.g. models.proto or domain.proto ('d' < 't', 'm' < 't'). This would fix the ordering naturally but requires regenerating all protobuf code and updating all import paths.

@rootulp
Copy link
Copy Markdown
Contributor Author

rootulp commented Feb 11, 2026

We should probably do the proto init thing for x/warp too.

@rootulp rootulp changed the title fix(ism): fix Amino JSON signing for MsgSetRoutingIsmDomain fix: amino JSON signing for MsgSetRoutingIsmDomain Feb 11, 2026
@rootulp
Copy link
Copy Markdown
Contributor Author

rootulp commented Feb 11, 2026

Updated the PR title. This PR actually fixes amino JSON signing for multiple messages but that would be a long PR title.

@rootulp rootulp marked this pull request as ready for review February 11, 2026 14:15
@rootulp
Copy link
Copy Markdown
Contributor Author

rootulp commented Feb 11, 2026

I just read the contributor guidelines and this prob deserves a CHANGELOG.

@mbreithecker
Copy link
Copy Markdown
Member

Hey, this deserves a Changelog, but will add this later.

@mbreithecker mbreithecker merged commit 0afaa4a into bcp-innovations:main Feb 11, 2026
1 check passed
mbreithecker pushed a commit that referenced this pull request Feb 11, 2026
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

(cherry picked from commit 0afaa4a)
mbreithecker added a commit that referenced this pull request Feb 11, 2026
@rootulp
Copy link
Copy Markdown
Contributor Author

rootulp commented Feb 11, 2026

I added a changelog in a787d3a

@rootulp rootulp deleted the rootulp/reproduce-amino-json-signing-bug branch February 11, 2026 18:02
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.

Tx with MsgSetRoutingIsmDomain and multiple signers hits "unknown protobuf field"

2 participants