Repository navigation
fix(evm): make public-parameters setup actually endorsable - #2416
Conversation
|
hi @Effi-S, pushed the fix, PTAL |
Effi-S
left a comment
There was a problem hiding this comment.
Hi @atharrva01,
Thank you for the work here.
Please squash to 1 commit and rebase.
c685e6d to
a52a922
Compare
|
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
left a comment
There was a problem hiding this comment.
@atharrva01 - Looking Good, Some minor changes.
a52a922 to
d4a6176
Compare
Effi-S
left a comment
There was a problem hiding this comment.
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.goendorse:factory = fstores a concrete*SetupDeltaFactoryinto the
deltaBuilderinterface. If a futuresetupFactoryForever returned(nil, nil),
factory.Buildwould panic on a typed-nil. Current production/test wiring never does this.- A node with
Endorser.Enabled=truethat fails to register (e.g. the "one endorsing network
per node" refusal inregisterEndorser) still getslatestreads on its watcher, taking
reorg exposure without the endorsement benefit. Harmless and self-healing; edge case only.
d4a6176 to
c8937b5
Compare
|
@Effi-S , PTAL |
c8937b5 to
c9609a8
Compare
|
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 |
|
Could you please fix the lint ? Regards, |
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>
c9609a8 to
f3bdfb8
Compare
|
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 |
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:
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