fix(evm): ten bugs from a self-review pass over the network driver - #2231
Open
atharrva01 wants to merge 12 commits into
Open
fix(evm): ten bugs from a self-review pass over the network driver#2231atharrva01 wants to merge 12 commits into
atharrva01 wants to merge 12 commits into
Conversation
Needed to build this branch locally; already open separately as LFDT-Panurus#2228 and will drop out here once that merges and this branch rebases. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
The finality watcher polled StatusByAnchor until either the anchor appeared or the timeout expired, and on timeout it always reported Invalid. If the chain was unreachable for the whole window, every poll errored and got skipped, so the loop reached the timeout having never actually observed the ledger, and reported Invalid anyway. The shared ttx listener maps Invalid straight to a deleted transaction, so a connectivity outage on the reading side could make a transaction that actually committed look failed, and its tokens would be dropped from local bookkeeping. The watcher now tracks whether any poll in the window actually reached the chain, valid or not. Only then does an absent anchor at the timeout mean Invalid. If every attempt errored, it reports OnError instead, the same signal the interface already defines for "the finality event could not be delivered." The transaction stays Pending rather than being marked deleted, and the driver's existing recovery sweep picks it up again later with a fresh read. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
The default block tag is finalized, and reading at that tag has a real time-to-finality of roughly 13 minutes (design §7.2). The default finality timeout was 5 minutes, so a deployment running on defaults alone would time out on every single transaction, valid or not, and report it Invalid before the chain could ever finalize it. The design already documents this exact constraint (§7.5: "any deployment must configure finality.timeout above ... the chain's finality"), but nothing enforced it. DefaultFinalityTimeout is now 20 minutes, with real margin over the ~13 minute floor rather than sitting at its edge. Validate also now rejects a finalized-tag configuration whose timeout is shorter than that floor, so a deployer who explicitly sets an unsafe combination gets a startup error instead of every transaction silently failing later. The floor applies only to the finalized tag; safe and latest resolve on their own, faster schedules and are not validated here. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
The watcher recorded a new version as seen before calling the handler that actually applies it, and the handler had no way to report failure at all (UpdateHandler returned nothing). If applying a version failed for any reason, the watcher had already moved past it: the next poll only looks at what changed since the last seen version, so a failed reload was silently never retried, and the node kept serving stale public parameters with nothing left to notice the gap. UpdateHandler now returns an error, and the watcher only advances past a version once its handler actually succeeds; a failure is logged and the same version is retried on the next poll. applyPublicParams collects and returns the combined error of every TMS that failed to update, so a partial failure is visible to the watcher rather than swallowed. Retrying the whole batch is safe: updating a TMS with parameters it already holds is a no-op, so a TMS that already succeeded is not disturbed by covering it again. Covered at the watcher, where the actual defect lived: a new test drives a handler that fails twice then succeeds and asserts the same version is retried rather than skipped, and that seen only advances on the eventual success. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
ChainProvider.PublicParams read the parameter bytes and the version as two separate, unsynchronised calls. An endorsed setup delta landing between them could tear the pair: bytes from before the update, version from after, or the reverse. The contract checks both fields together against what it currently holds and reverts StalePublicParams on a mismatch, so a torn read here did not corrupt state, but it turned a purely local race in this function into a doomed, gas-spending transaction the contract was always going to reject. The version is now read before and after the bytes, and the whole read is retried if it moved: version and bytes only ever change together, in the same transaction, so two matching reads bracketing the bytes read is proof nothing landed in between. The retry is bounded (three attempts) so a pathological chain that never settles fails with an error instead of spinning. TestWatcherSurvivesAFailedRead needed a related fix: it modelled the version as the raw RPC call count, which does not hold once PublicParams reads the version twice per attempt, two call counts a few lines apart would themselves look torn. Rewritten to use the same stable chain-state double the other watcher tests already use. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
SetupPublicParams checked only Network.endorsement, the field tests inject a stub into directly. In production nothing ever sets that field: the driver wires the per-TMS endorsement factory through endorsementFor instead, keyed by an already-built management service. SetupPublicParams never has one of those to hand it, that is the entire reason it takes a bare TMSID rather than a management service, so it could never reach the factory at all. Every call failed with "no endorsement service configured" on any real deployment, and first-time setup of a namespace, the one thing this method exists to make possible, could not work. Nothing caught this: the shared ppsetup view exercises the real production path, but the EVM integration suite bootstraps and updates parameters through its own harness-side submitter instead, bypassing this method entirely, so the gap was invisible to every existing test. Network now also carries endorsementForID, a TMSID-keyed counterpart to endorsementFor, and SetupPublicParams resolves through that instead. The driver installs it in installEndorsement next to the existing factory: both ultimately call the same per-TMS ServiceFactory.ForTMS, one entered from a management service, the other from the id alone. New tests cover what nothing did before: SetupPublicParams resolving through the id-based factory and reaching it with the requested TMS, and a failed endorsement collection not broadcasting. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
…nonce allocation NonceManager split allocation and recovery into two separate critical sections: Next handed out a nonce and released the lock, and a failed Submit later called Reset, which walked the whole sequence back to whatever eth_getTransactionCount(pending) currently showed. That call only reflects transactions the node has actually seen. A different, concurrent Submit that had already allocated a higher nonce but not yet reached SendRawTransaction was invisible to it, and Reset would hand that same nonce to a third caller, producing two transactions racing for one nonce. Every path that called Reset already treated its own failure as certain proof the transaction never reached the chain: gas estimation and fee suggestion are read-only, signing is local, and a rejected broadcast is documented as never judged by the chain. So walking back to the chain's view was never actually necessary, it just happened to be how the recovery was implemented, and that implementation was what raced. NonceManager.Next and Reset are replaced by WithNonce, which holds the lock for the whole allocate-and-use step. The sequence advances only if the callback succeeds; on failure the nonce is simply left where it was, with no round trip to the chain, and nothing else could have been mid allocation while the callback ran. Submitter.Submit now runs its entire body inside that callback. New tests cover the failure path directly (a failed attempt does not advance the sequence and needs no re-sync) and drive many goroutines with a mix of successes and failures to check every successful attempt still gets a distinct nonce. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
…discarding it silently Driver.New runs once per (network, channel) the node is configured for, and installEndorsement, called from it, builds a fresh per-network ServiceFactory and tries to register this node's endorsement responder every time. That registration used a sync.Once scoped to the whole Driver, not to a network, so on a node configured to endorse for more than one EVM network, only the first network's registration ever ran. Every later network's factory, key and allowlist were silently discarded, with nothing logged to say so. The reason a straight per-network fix is not possible: FSC routes an incoming session to a responder by the initiating view's Go type alone, with no notion of "this responder, but only for network X". Only one Responder can ever be registered for endorsement.Initiator across a process's lifetime, so whichever network's factory happens to win the race is baked into it permanently, EIP-712 domain, chain client and all. Routing a second network's requests through it would not fail cleanly, it would validate against the right TMS but sign and read against the wrong chain. registerEndorser now tracks which network it registered for and refuses, loudly, when a different network also wants to endorse: an operator gets a clear error naming both networks instead of a request that silently never gets answered. The same network registering twice (a network rebuilt over a node's life) and a network that never wanted to endorse in the first place both remain unaffected. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
…t registration Two comments, on the Allowlist field and on Authorizer itself, promised that an empty allowlist would default to "the TMS network's nodes, resolved at config load in Week 5". That resolution was never built. Authorizer.NewAuthorizer is deliberately fail-closed and rejects an empty allowlist outright, which is the right call for authorization, but nothing upstream of it ever supplied the promised default, so an operator who left Allowlist unset trusting the documented behavior got a node that came up looking healthy and silently never registered as an endorser, the failure logged as an error easy to miss during wiring rather than surfaced as the startup failure it should have been. Validate now rejects an endorser.enabled configuration with no allowlist, matching this file's own stated philosophy that a bad configuration should be a startup error, not a surprise later. Both comments are corrected to describe the actual, intentional fail-closed behavior instead of a fallback that does not exist. The integration harness is unaffected: it already builds the allowlist itself from every node in the TMS rather than relying on the driver to do it, which is what the documented default was supposed to be doing. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
…gree Broadcast checked only that an envelope carried a delta at all, never that the envelope's own anchor and the anchor baked into that delta actually named the same transaction. Under the normal flow they always agree, since RequestApproval and SetupPublicParams both derive the envelope's anchor and the delta's anchor from the same value, but Broadcast has no way to know how the envelope it was actually handed was built. A mismatch here is not just a theoretical validation gap. The chain only ever looks at the delta's anchor: that is what applyStateDelta checks for replay, what the digest covers, what StateCommitted is emitted for. The local side tracks the transaction by the envelope's anchor instead, finality listeners and the ttx store are keyed on it. If the two ever diverged, the transaction would apply and commit on chain under one anchor while everything local kept waiting on a different one, and after the finality timeout wrongly report a transaction that actually succeeded as failed, the same failure shape HIGH #1 fixed, just reachable through a construction bug instead of a chain-read one. Broadcast now parses the envelope's anchor and compares it against the delta's before spending any gas, and refuses if they disagree. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
…ted tags FinalityConfig.BlockTag's own comment said state is read at finalized or safe, but Validate has accepted latest as a third legal value since it was introduced for the local, instant-mining test harness. The comment now names all three and repeats, next to the field itself, what BlockTagLatest's own doc comment already says: it carries no reorg protection and is only appropriate for a local chain. No behavior changes; latest was already accepted before this commit. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
LFDT-Panurus#2180 enabled containedctx and fixed the ttx occurrence. The evm module is a separate Go module and was not linted in that pass, so make lint has been failing on it since. Neither field can be dropped. Ledger.ctx exists because driver.GetStateFnc passes no context to GetState, and fakeContext.ctx exists because it implements view.Context, whose Context() method has to return one. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Went through the evm driver directory end to end looking for correctness bugs and fixed everything that verified against the actual code, six that could produce a wrong result and four smaller ones. One commit per fix, each with its own tests.
High:
Medium/low:
Also bumped the module to fabric-smart-client v0.17.0, needed to build this branch (dupes #2228, will collapse on rebase).
Tested with go build, go vet, gofmt, golangci-lint on every changed package, and go test -race across the whole module including the anvil e2e test.