Skip to content

fix(evm): make public-parameters setup actually endorsable - #2416

Merged
Effi-S merged 1 commit into
LFDT-Panurus:mainfrom
atharrva01:fix/2412-setup-endorsement
Oct 7, 2026
Merged

Effi-S merged 1 commit into
LFDT-Panurus:mainfrom
atharrva01:fix/2412-setup-endorsement

Conversation

@atharrva01

Copy link
Copy Markdown
Contributor

Part of #2412 (items 1, 2, 6, 8).

SetupPublicParams sent raw public-parameters bytes through the same validator a token request goes through, which can never succeed since those bytes aren't a marshalled token.Request. Adds a SetupDeltaFactory that validates and signs a setup delta directly, without needing an existing TMS (needed for first-time setup, where there is no TMS yet to resolve a validator from).

Two related bugs came out of the same review round:

  • the delta's public-parameters CAS check reads the chain at latest, but the local watcher was polling at the finality tag, so every ordinary approval got refused for the whole finalization window after a setup update. The watcher now reads at latest too.
  • NewApprovedEnvelope never set Namespace, so an envelope built through it can't be broadcast. RequestApproval and SetupPublicParams now both go through it instead of duplicating the struct literal (which is how the two silently diverged in the first place).

Also adds a startup check that an endorsing node's own address is actually one of the configured endorsers, instead of starting cleanly and silently signing endorsements nobody's registry recognizes.

Test plan

  • go build / go vet clean for x/token/services/network/evm and the two SDK wiring packages
  • go test ./... green, including new tests for the setup dispatch, the watcher block tag, and the endorser address check
  • docs updated (network-ethereum.md, network-ethereum-internals.md, network-ethereum-deployment.md)

Comment thread x/token/services/network/evm/endorsement/responder.go
@atharrva01

Copy link
Copy Markdown
Contributor Author

hi @Effi-S, pushed the fix, PTAL

@Effi-S Effi-S left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @atharrva01,
Thank you for the work here.
Please squash to 1 commit and rebase.

Comment thread x/token/services/network/evm/endorsement/delta.go
Comment thread x/token/services/network/evm/driver.go Outdated
Comment thread x/token/services/network/evm/endorsement/responder.go
@atharrva01
atharrva01 force-pushed the fix/2412-setup-endorsement branch from c685e6d to a52a922 Compare September 29, 2026 18:45
@atharrva01

Copy link
Copy Markdown
Contributor Author

hi @Effi-S, pushed fixes for the watcher gating and the boilerplate nit, and dug into the setup-hash question, replied with what I found. Also squashed to one commit. PTAL

@Effi-S Effi-S left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@atharrva01 - Looking Good, Some minor changes.

Comment thread x/token/services/network/evm/endorsement/delta.go Outdated
Comment thread x/token/services/network/evm/endorsement/delta.go
Comment thread docs/services/network-ethereum-internals.md Outdated
Comment thread x/token/services/network/evm/pp/provider.go Outdated
@Effi-S Effi-S added this to the Q4/26 milestone Oct 1, 2026
@atharrva01
atharrva01 force-pushed the fix/2412-setup-endorsement branch from a52a922 to d4a6176 Compare October 2, 2026 07:11
@atharrva01
atharrva01 requested a review from Effi-S October 2, 2026 07:19

@Effi-S Effi-S left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Low:

bind only checks delta.Anchor and delta.Validate(). It ignores req.Kind. For a
KindSetup request the initiator sent PublicParamsRaw itself, so it can cheaply assert both
delta.IsSetup and delta.SetupParameters == req.PublicParamsRaw with no validator — unlike
TokenRequestHash for approvals, which the comment correctly explains it cannot reproduce.

As written, a quorum that returned a delta with different SetupParameters (or a non-setup
delta IsSetup=false) would still pass bind, assemble, and be broadcast — the node would
commit public parameters it did not request, or silently commit nothing. This needs a malicious
or buggy quorum (full quorum compromise is already out of scope), so it is defense-in-depth, not
an exploitable hole. But it is a request-local, validator-free check the setup path uniquely
allows, and worth adding.

Nit:

  • responder.go endorse: factory = f stores a concrete *SetupDeltaFactory into the
    deltaBuilder interface. If a future setupFactoryFor ever returned (nil, nil),
    factory.Build would panic on a typed-nil. Current production/test wiring never does this.
  • A node with Endorser.Enabled=true that fails to register (e.g. the "one endorsing network
    per node" refusal in registerEndorser) still gets latest reads on its watcher, taking
    reorg exposure without the endorsement benefit. Harmless and self-healing; edge case only.

Comment thread x/token/services/network/evm/endorsement/delta.go
@atharrva01
atharrva01 force-pushed the fix/2412-setup-endorsement branch from d4a6176 to c8937b5 Compare October 4, 2026 13:34
@atharrva01
atharrva01 requested a review from Effi-S October 4, 2026 13:41
@atharrva01

Copy link
Copy Markdown
Contributor Author

@Effi-S , PTAL

Comment thread x/token/services/network/evm/endorsement/responder.go Outdated
Comment thread x/token/services/network/evm/driver.go
Comment thread x/token/services/network/evm/endorsement/delta.go
@atharrva01
atharrva01 force-pushed the fix/2412-setup-endorsement branch from c8937b5 to c9609a8 Compare October 7, 2026 09:27
@atharrva01

Copy link
Copy Markdown
Contributor Author

hi @Effi-S, pushed c9609a8 (squashed to one commit, rebased onto main). endorse() now has an explicit KindApproval case with an error default, the watcher takes the endorsing namespace's config when namespaces share a TokenState so a slower non-endorsing one can't stretch the poll interval, and SetupDeltaFactory.Build checks the anchor before the validator and the chain read. Added tests for the last two. PTAL

@atharrva01
atharrva01 requested a review from Effi-S October 7, 2026 09:28
Comment thread x/token/services/network/evm/driver.go
Comment thread x/token/services/network/evm/endorsement/messages.go
Comment thread x/token/services/network/evm/endorsement/initiator.go
@AkramBitar

Copy link
Copy Markdown
Contributor

@atharrva01

Could you please fix the lint ?

Regards,
Akram

SetupPublicParams sent raw public-parameters bytes through the same
validator used for token requests, which can never succeed because
those bytes aren't a marshalled token.Request. Adds a SetupDeltaFactory
that validates and signs a setup delta directly, without needing an
existing TMS.

Two related bugs came out of the same review round:

- the delta's public-parameters CAS check reads the chain at latest,
  but the local watcher was polling at the finality tag, so every
  approval got refused for the whole finalization window after a
  setup update. The watcher now reads at latest too, but only for a
  contract this node actually endorses against: a non-endorsing node
  has no lockstep requirement with the CAS check, so reading at latest
  for it only added reorg exposure for no benefit.
- NewApprovedEnvelope never set Namespace, so an envelope built
  through it can't be broadcast. RequestApproval and SetupPublicParams
  now both go through it instead of duplicating the struct literal.

Also:
- adds a startup check that an endorsing node's own address is
  actually one of the configured endorsers, instead of silently
  signing endorsements nobody recognizes
- guards the KindSetup dispatch against a nil setupFactoryFor, so a
  Responder built without one errors instead of panicking
- shares one Build+error path between the two branches of endorse's
  request-kind switch instead of duplicating it

Verified the setup delta's TokenRequestHash (SHA256 of the raw public
parameters, since a setup has no token request to hash) never reaches
ttxfinality's hash comparison: the only production caller of
SetupPublicParams (integration/token/fungible/views/ppsetup) uses its
own finality listener that only checks status, not the hash, and
never registers the transaction with ttxDB, so the generic recovery
path that would compare hashes never sees a setup anchor.

Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
@atharrva01
atharrva01 force-pushed the fix/2412-setup-endorsement branch from c9609a8 to f3bdfb8 Compare October 7, 2026 12:51
@atharrva01

Copy link
Copy Markdown
Contributor Author

pushed f3bdfb8 (still one commit). @Effi-S: the watcher now takes the shortest pollInterval among endorsing namespaces sharing a TokenState, Validate rejects a request carrying the other kind's field, and bind guards a nil request. Tests added for the first two. @AkramBitar: the lint failure was a test of mine tripping testifylint (assert vs require), fixed, golangci-lint is clean locally on the evm module. The CI run also printed govulncheck findings in grpc and libp2p, in code this PR doesn't touch, so those would need a dependency bump separately. PTAL

@atharrva01
atharrva01 requested a review from Effi-S October 7, 2026 12:52
@Effi-S
Effi-S merged commit e58b277 into LFDT-Panurus:main Oct 7, 2026
211 checks passed
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