feat: a sequential batch TSS keysign scheduler for EVM chain #4427
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces a batched TSS keysign system to improve EVM chain outbound performance. It refactors signing workflows to use per-nonce digest caching, batch multiple keysigns into a single TSS operation, and eliminates height-based scheduling in favor of a nonce-driven approach with stale-block detection. Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as EVM Scheduler
participant Signer as Base Signer
participant Batch as Batch Manager
participant TSS as TSS Service
participant Cache as Signature Cache
Scheduler->>Signer: PrepareForKeysign(zetaHeight, nextNonce)
Signer->>Signer: Check stale blocks
Signer->>Signer: Clean stale keysign info
Signer-->>Scheduler: Ready: bool
alt Batch ready to sign
Scheduler->>Signer: GetKeysignBatch(batchNumber)
Signer->>Batch: Collect digests in nonce range
Batch-->>Signer: TSSKeysignBatch
Signer->>Signer: SignBatch(batch)
Signer->>Signer: Compute keysignHeight via Cantor pairing
Signer->>TSS: SignBatch(digests, height)
TSS->>Cache: Store signatures
Signer->>Signer: AddBatchSignatures(batch, sigs)
else Waiting for signatures
Signer->>Cache: GetSignatureOrAddDigest(nonce, digest)
Cache-->>Signer: (sig [65]byte, found bool)
alt Found in cache
Signer-->>Scheduler: Success
else Not found
Signer-->>Scheduler: ErrWaitForSignature
end
end
sequenceDiagram
participant Client as EVM Client
participant Outbound as OutboundData
participant TSS as Signer (TSS)
participant Batch as Batch Signing
rect rgb(200, 200, 255)
note over Client,TSS: Old Flow: Height-based scheduling
Client->>Outbound: NewOutboundData(ctx, cctx, height, logger)
Outbound-->>Client: OutboundData with height field
Client->>TSS: Sign(ctx, data, ..., height)
TSS->>TSS: Per-nonce TSS keysign
end
rect rgb(200, 255, 200)
note over Client,Batch: New Flow: Batch and digest-based
Client->>Outbound: NewOutboundData(ctx, cctx, logger)
Outbound-->>Client: OutboundData (height from ObservedExternalHeight)
Client->>TSS: GetSignatureOrAddDigest(nonce, digest)
alt Signature cached
TSS-->>Client: (sig, true)
else Waiting on batch
TSS-->>Client: (empty, false)
Client->>Batch: PrepareForKeysign()
Batch->>Batch: Accumulate nonces
Batch->>TSS: SignBatch(digests[])
TSS->>TSS: Single TSS keysign for batch
Batch->>Batch: Cache all signatures
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
!!!WARNING!!! Be very careful about using Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203 Pay extra attention to the way |
ws4charlie
changed the title
…e into one method
…t-eth-tss-batch-sign
ws4charlie
changed the title
ws4charlie
added
zetaclient
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #4427 +/- ##
===========================================
+ Coverage 64.79% 64.94% +0.15%
===========================================
Files 469 472 +3
Lines 28547 28781 +234
===========================================
+ Hits 18497 18692 +195
- Misses 9029 9061 +32
- Partials 1021 1028 +7
🚀 New features to boost your workflow:
|
ws4charlie
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
zetaclient/chains/evm/signer/signer_admin_test.go (1)
209-225: Fix negative test to actually use the modified nonceIn
signMigrateERC20CustodyFundsCmd - should fail if keysign fails,txDataOtheris created and its nonce incremented, but the call still passestxData. As written, the test will not exercise the cache‑miss path you intend and may spuriously pass depending on prior state.Consider updating the call to use
txDataOther:- // Call signWhitelistERC20Cmd - tx, err := evmSigner.signMigrateERC20CustodyFundsCmd(ctx, txData, params) + // Call signMigrateERC20CustodyFundsCmd with modified nonce (no cached signature) + tx, err := evmSigner.signMigrateERC20CustodyFundsCmd(ctx, &txDataOther, params)This matches the pattern used in the other
keysign failssubtests.
🧹 Nitpick comments (12)
zetaclient/chains/zrepo/zrepo.go (1)
68-70: Add godoc comment for public method.The implementation is clean and follows the delegation pattern correctly. However, public methods should include godoc comments per Go conventions.
+// GetBlockHeight returns the current block height from the underlying zetacore client. func (repo *ZetaRepo) GetBlockHeight(ctx context.Context) (int64, error) { return repo.client.GetBlockHeight(ctx) }zetaclient/chains/zrepo/client.go (1)
34-34: Add godoc comment for interface method.The method signature is clean and appropriate. However, exported interface methods should include godoc comments per Go conventions to describe their purpose and behavior.
+ // GetBlockHeight returns the current block height from zetacore. GetBlockHeight(context.Context) (int64, error)pkg/math/pairing_test.go (1)
9-35: Consider adding edge case tests.The test coverage is good and includes boundary cases with
MaxPairValue. However, consider adding tests for additional edge cases to ensure robustness:{name: "test 4", x: 925478314, y: 91456237, z: 517077941108709313}, {name: "test 5", x: MaxPairValue, y: MaxPairValue, z: 9223372032559808512}, + {name: "test 6 - zeros", x: 0, y: 0, z: 0}, + {name: "test 7 - boundary x", x: MaxPairValue, y: 0, z: 4611686017353646080}, + {name: "test 8 - boundary y", x: 0, y: MaxPairValue, z: 4611686018427387904}, }These cases verify:
- Minimum value handling (0, 0)
- Asymmetric boundary behavior when one coordinate is MaxPairValue and the other is 0
zetaclient/dry/dry.go (1)
119-121: Consider returningfalseinstead of panicking for this read-only query.
IsSignatureCachedis a read-only method that queries signature cache state without triggering signing or mutating external state. In dry mode, it would be more ergonomic to returnfalse(indicating no cached signature) rather than panicking, allowing dry-mode code paths to execute further without special handling.Compare with other dry-mode clients where non-mutating getters (e.g.,
PubKey()at line 106) return valid values rather than panicking.Apply this diff if you wish to make dry-mode code more permissive for read-only queries:
-func (*TSSClient) IsSignatureCached(int64, [][]byte) bool { - panic(MsgUnreachable) -} +func (*TSSClient) IsSignatureCached(int64, [][]byte) bool { + return false +}pkg/math/pairing.go (1)
22-46: Document input constraints or add defensive validation forCantorUnpair.Lines 45-46 cast
big.Inttouint32with a#nosecdirective, assuming the result is always in range. While this is safe whenzwas produced byCantorPair(x, y)withx, y ≤ MaxPairValue, there's no runtime validation to ensure the invariant holds.Since the comment at line 21 indicates this function is "currently only used for unit tests," the risk is low. However, for production-grade code, consider either:
Adding a defensive check before the cast:
if x.Uint64() > math.MaxUint32 || y.Uint64() > math.MaxUint32 { panic("CantorUnpair: input out of valid range") }Strengthening the documentation to clearly state preconditions:
// CantorUnpair inverts CantorPair. // Precondition: z must be a value previously produced by CantorPair(x, y) // where x, y ≤ MaxPairValue. Violating this precondition results in undefined behavior.zetaclient/chains/evm/signer/signer_test.go (2)
141-260: Sign tests now accurately cover available vs. missing signature flows*The refactored tests for
TryProcessOutbound,BroadcastOutbound, and the variousSign*methods now:
- Use
NewOutboundData(ctx, cctx, logger)to build realistictxData.- Drive the happy path by pre-populating the signer’s cache with a matching digest/signature.
- Exercise the
ErrWaitForSignaturepath via a modified nonce that changes the digest.This closely matches the new digest-based, per-nonce signing model and gives solid coverage of both success and “no signature yet” behaviors.
263-376: Digest helpers and mockSignature align well with batch TSS signing, but mirror production logicThe helper functions that reconstruct the digests and
mockSignaturenicely simulate the real batch TSS flow by:
- Building transactions with the same parameters used in production (chain ID, target contract, receiver, amount, gas, nonce).
- Hashing via the EVM client signer and feeding digests into the signer's cache and a synthetic
TSSKeysignBatch.- Calling
SignBatchand thenAddBatchSignaturesto populate signatures before tests invokeSign*.Two minor considerations:
- Because these helpers duplicate the production digest construction, any future change in contract ABI or tx shape will require updates in both places; if this starts to drift, consider centralizing digest construction behind a shared helper used by both code and tests.
- Ensure that the ABI method selectors (
"withdraw","onReceive","onRevert") and parameter ordering exactly match the protocol-contracts definitions to avoid brittle tests tightly bound to a specific contract version.Overall, the approach is sound and provides realistic test wiring into the batch-signing pipeline.
zetaclient/chains/base/signer.go (1)
36-45: Signer TSS nonce tracking is correct; consider lowering log verbosityThe new
tssKeysignInfoMapandnextTSSNoncefields are properly initialized inNewSigner, andSetNextTSSNonce:
- Uses the signer mutex, so updates are thread-safe.
- Enforces a strictly increasing
nextTSSNonce, which matches expected TSS nonce semantics.- Updates the
NextTSSNoncegauge with a per-chain label, keeping metrics in sync.Given that
SetNextTSSNoncemay be called frequently under load, logging every update atInfocould become noisy in production. You may wish to reduce this toDebug(similar toMarkOutbound) or gate it behind a sampled/log-on-change pattern if logs become too chatty.Also applies to: 63-74, 197-207
zetaclient/chains/evm/evm.go (1)
197-271: Sequential batch keysign loop is well structured; consider guarding interval misconfigThe
scheduleKeysignlogic ties together:
- Stale block skipping via
IsStaleBlockEvent.- A single
nextTSSNonceas the deterministic starting point.PrepareForKeysignto gate onOutboundScheduleIntervaland pending nonces.- Sequential batch signing where:
- Only unsigned batches are sent to
SignBatch.- Advancement to the next batch occurs only when the current batch hits
IsEnd().One minor robustness improvement:
PrepareForKeysignperformszetaHeight % scheduleInterval, assumingscheduleInterval > 0. If chain params are ever misconfigured to 0, this will panic. A small defensive check like:if scheduleInterval <= 0 { return false, fmt.Errorf("invalid outbound schedule interval: %d", scheduleInterval) }would make failures explicit and easier to diagnose.
zetaclient/chains/evm/signer/signer.go (1)
176-211: Digest‑cache–driven Sign flow and ErrWaitForSignature handling are sound
Signnow:
- Builds the tx as before.
- Computes the digest via
Signer().Hash(tx).- Uses
GetSignatureOrAddDigestto either:
- Return a cached signature, or
- Register the digest and return
ErrWaitForSignature.
TryProcessOutboundspecial‑casesErrWaitForSignatureby returning silently, allowing the scheduler to retry onceSignBatchhas populated signatures. This is a clean separation between digest registration, TSS batch signing, and broadcast, and avoids noisy logging on the expected “first attempt” miss.It may be worth adding a very low‑level debug log on the
ErrWaitForSignaturepath if you later need to trace digest registration behavior under load.Also applies to: 338-352
zetaclient/chains/base/signer_batch_sign.go (2)
49-88: PrepareForKeysign logic is sound; guard against zero interval configuration
PrepareForKeysigncorrectly:
- Triggers only on heights that are multiples of
scheduleInterval.- Fetches pending nonces, prunes stale keysign info via
removeKeysignInfo(nonceLow).- Short‑circuits when there are no pending CCTXs (
nonceLow >= nonceHigh) or the TSS nonce is already at/above the pending high watermark.One robustness improvement would be to explicitly handle
scheduleInterval <= 0before doingzetaHeight % scheduleInterval, returning an error instead of panicking if chain params are misconfigured.
90-120: Batch readiness and collection enforce the intended no‑gaps invariantThe combination of:
isBatchReadyToSign, which checks overlap between[NonceLow, NonceHigh)and the batch’s nonce range, andcollectKeysignBatch, which:
- Sorts all known nonces,
- Collects digests within the batch’s range,
- Rejects empty, non‑sequential batches or those missing
untilNonce,ensures that batches are only signed when there is a contiguous span of digests up to the required nonce.
GetKeysignBatch’s use ofretry.DoWithBackoffto wait briefly for missing digests is also a pragmatic trade‑off between responsiveness and avoiding partial batches.You might consider differentiating between transient “waiting for digests” errors and unexpected internal errors to avoid retrying the latter, but this is not strictly necessary.
Also applies to: 188-217, 220-260
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (28)
changelog.md(1 hunks)cmd/zetae2e/local/performance.go(1 hunks)pkg/math/pairing.go(1 hunks)pkg/math/pairing_test.go(1 hunks)zetaclient/chains/base/signer.go(6 hunks)zetaclient/chains/base/signer_batch_info.go(1 hunks)zetaclient/chains/base/signer_batch_sign.go(1 hunks)zetaclient/chains/evm/evm.go(5 hunks)zetaclient/chains/evm/observer/outbound.go(1 hunks)zetaclient/chains/evm/signer/outbound_data.go(1 hunks)zetaclient/chains/evm/signer/outbound_data_test.go(1 hunks)zetaclient/chains/evm/signer/sign.go(1 hunks)zetaclient/chains/evm/signer/sign_test.go(12 hunks)zetaclient/chains/evm/signer/signer.go(7 hunks)zetaclient/chains/evm/signer/signer_admin.go(0 hunks)zetaclient/chains/evm/signer/signer_admin_test.go(15 hunks)zetaclient/chains/evm/signer/signer_test.go(3 hunks)zetaclient/chains/evm/signer/v2_sign.go(0 hunks)zetaclient/chains/tssrepo/client.go(1 hunks)zetaclient/chains/zrepo/client.go(1 hunks)zetaclient/chains/zrepo/zrepo.go(1 hunks)zetaclient/dry/dry.go(1 hunks)zetaclient/metrics/metrics.go(1 hunks)zetaclient/mode/chaos/generate/sample.json(1 hunks)zetaclient/mode/chaos/generated.go(2 hunks)zetaclient/testutils/mocks/tss.go(1 hunks)zetaclient/testutils/mocks/zetacore.go(2 hunks)zetaclient/tss/service.go(1 hunks)
💤 Files with no reviewable changes (2)
- zetaclient/chains/evm/signer/v2_sign.go
- zetaclient/chains/evm/signer/signer_admin.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Files:
zetaclient/chains/evm/signer/outbound_data.gozetaclient/dry/dry.gozetaclient/chains/tssrepo/client.gocmd/zetae2e/local/performance.gozetaclient/tss/service.gozetaclient/mode/chaos/generated.gozetaclient/chains/zrepo/zrepo.gozetaclient/chains/evm/signer/signer_test.gozetaclient/chains/zrepo/client.gozetaclient/testutils/mocks/tss.gozetaclient/chains/evm/signer/signer_admin_test.gozetaclient/chains/evm/signer/sign_test.gopkg/math/pairing.gozetaclient/metrics/metrics.gozetaclient/chains/evm/observer/outbound.gozetaclient/testutils/mocks/zetacore.gozetaclient/chains/evm/signer/outbound_data_test.gozetaclient/chains/base/signer.gozetaclient/chains/base/signer_batch_info.gopkg/math/pairing_test.gozetaclient/chains/evm/evm.gozetaclient/chains/evm/signer/signer.gozetaclient/chains/evm/signer/sign.gozetaclient/chains/base/signer_batch_sign.go
🧠 Learnings (33)
📓 Common learnings
Learnt from: gartnera
Repo: zeta-chain/node PR: 3632
File: zetaclient/chains/solana/signer/signer.go:304-304
Timestamp: 2025-03-04T22:39:58.395Z
Learning: The Solana signer implementation in zetaclient/chains/solana/signer/signer.go has limited test coverage, particularly for the transaction broadcasting logic with fallback scenarios. Adding this coverage has been acknowledged as a potential future improvement outside the scope of immediate fixes.
📚 Learning: 2025-09-18T14:13:53.345Z
Learnt from: lumtis
Repo: zeta-chain/node PR: 4200
File: changelog.md:16-16
Timestamp: 2025-09-18T14:13:53.345Z
Learning: The LastBlockHeight state variable in the ZetaChain codebase was never used since the beginning of the project, making its removal a cleanup/refactor rather than a functional breaking change.
Applied to files:
zetaclient/mode/chaos/generate/sample.jsonzetaclient/chains/evm/signer/outbound_data.gozetaclient/chains/zrepo/zrepo.gozetaclient/chains/zrepo/client.gozetaclient/testutils/mocks/zetacore.gozetaclient/chains/evm/signer/sign.go
📚 Learning: 2025-11-09T17:07:25.025Z
Learnt from: kingpinXD
Repo: zeta-chain/node PR: 4419
File: server/testnet.go:288-292
Timestamp: 2025-11-09T17:07:25.025Z
Learning: In the ZetaChain node testnet command (server/testnet.go), when setting StartHeight for validator signing info to `app.LastBlockHeight() - 1`, it's safe to assume LastBlockHeight() >= 1 because the testnet command is designed to fork from existing node data. For fresh initialization with no blocks, users would use the regular `zetacored start` command instead.
Applied to files:
zetaclient/mode/chaos/generate/sample.jsonzetaclient/chains/evm/signer/outbound_data.gozetaclient/mode/chaos/generated.gozetaclient/chains/zrepo/zrepo.gozetaclient/chains/zrepo/client.gozetaclient/testutils/mocks/zetacore.gozetaclient/chains/evm/signer/signer.gozetaclient/chains/evm/signer/sign.go
📚 Learning: 2025-01-14T03:39:29.764Z
Learnt from: ws4charlie
Repo: zeta-chain/node PR: 3306
File: zetaclient/chains/bitcoin/signer/sign.go:225-233
Timestamp: 2025-01-14T03:39:29.764Z
Learning: The signature hash calculation in Bitcoin TSS transactions using `txscript.NewCannedPrevOutputFetcher([]byte{}, 0)` with correct input amounts has been proven to work reliably in production, despite theoretical concerns about empty previous output fetcher.
Applied to files:
zetaclient/dry/dry.gozetaclient/chains/evm/signer/signer_test.gozetaclient/chains/evm/signer/signer_admin_test.gozetaclient/chains/evm/signer/sign_test.gozetaclient/chains/evm/signer/signer.go
📚 Learning: 2025-05-30T16:31:30.275Z
Learnt from: skosito
Repo: zeta-chain/node PR: 3939
File: go.mod:52-52
Timestamp: 2025-05-30T16:31:30.275Z
Learning: The ethermint dependency updates in the zeta-chain/node repository are typically moves between feature branches and main branch of the same fork, not breaking API changes. CI status should be verified before assuming compilation issues.
Applied to files:
changelog.md
📚 Learning: 2025-02-07T19:33:35.631Z
Learnt from: kingpinXD
Repo: zeta-chain/node PR: 3503
File: e2e/runner/require.go:0-0
Timestamp: 2025-02-07T19:33:35.631Z
Learning: In e2e tests for the zeta-chain/node repository, minimize computation and avoid redundant checks that are already covered by unit tests to ensure faster execution. For example, ballot sorting is verified in unit tests, so e2e tests can safely assume the order.
Applied to files:
changelog.md
📚 Learning: 2024-10-31T16:23:17.250Z
Learnt from: gartnera
Repo: zeta-chain/node PR: 3071
File: e2e/e2etests/test_stress_eth_withdraw.go:41-42
Timestamp: 2024-10-31T16:23:17.250Z
Learning: In `e2e/e2etests/test_stress_eth_withdraw.go`, since some goroutines may fail, we should use a mutex to append durations of successful withdrawals for accurate latency reporting.
Applied to files:
cmd/zetae2e/local/performance.go
📚 Learning: 2024-10-31T16:21:47.362Z
Learnt from: gartnera
Repo: zeta-chain/node PR: 3071
File: e2e/e2etests/test_stress_eth_withdraw.go:0-0
Timestamp: 2024-10-31T16:21:47.362Z
Learning: In `e2e/e2etests/test_stress_eth_withdraw.go`, when handling the error returned by `stats.Describe` in the `TestStressEtherWithdraw` function, log the error instead of failing the test.
Applied to files:
cmd/zetae2e/local/performance.go
📚 Learning: 2024-11-01T17:07:59.584Z
Learnt from: gartnera
Repo: zeta-chain/node PR: 3079
File: e2e/e2etests/test_pause_erc20_custody.go:65-68
Timestamp: 2024-11-01T17:07:59.584Z
Learning: In the file `e2e/e2etests/test_pause_erc20_custody.go`, prefer verbosity and avoid extracting common event handling logic into helper functions to enhance readability and ease future refactoring.
Applied to files:
cmd/zetae2e/local/performance.gozetaclient/chains/evm/signer/signer_test.gozetaclient/chains/evm/signer/signer_admin_test.gozetaclient/chains/evm/signer/sign_test.go
📚 Learning: 2025-08-06T01:54:04.100Z
Learnt from: kingpinXD
Repo: zeta-chain/node PR: 4064
File: cmd/zetae2e/local/solana.go:0-0
Timestamp: 2025-08-06T01:54:04.100Z
Learning: The `CheckSolanaTSSBalance()` method in e2e/runner has been refactored to not return an error. Instead, it uses internal assertions (require statements) to fail the test immediately if balance checks fail. This is part of a broader refactoring pattern in the E2E runner where balance check methods were changed from error-returning to assertion-based approaches.
Applied to files:
cmd/zetae2e/local/performance.gozetaclient/chains/evm/signer/signer_test.gozetaclient/chains/evm/signer/sign_test.go
📚 Learning: 2025-07-28T18:08:13.883Z
Learnt from: ws4charlie
Repo: zeta-chain/node PR: 4053
File: e2e/e2etests/test_bitcoin_std_deposit.go:42-42
Timestamp: 2025-07-28T18:08:13.883Z
Learning: In Bitcoin deposit E2E tests for the ZetaChain project, `DepositBTCWithAmount` is preferred over `DepositBTCWithExactAmount` because depositing exact amounts to a ZEVM receiver is complex. The tests use rough amounts and then calculate the actual received amount from the raw Bitcoin transaction to verify balance changes, making the tests more robust and less flaky.
Applied to files:
cmd/zetae2e/local/performance.go
📚 Learning: 2024-07-04T23:46:38.428Z
Learnt from: ws4charlie
Repo: zeta-chain/node PR: 2411
File: zetaclient/orchestrator/orchestrator.go:192-217
Timestamp: 2024-07-04T23:46:38.428Z
Learning: The `GetUpdatedSigner` method in `zetaclient/orchestrator/orchestrator.go` is covered by unit tests in `zetaclient/orchestrator/chain_activate_test.go` and `zetaclient/orchestrator/orchestrator_test.go`.
Applied to files:
zetaclient/tss/service.gozetaclient/mode/chaos/generated.gozetaclient/chains/evm/signer/signer_test.gozetaclient/chains/evm/signer/signer_admin_test.gozetaclient/chains/evm/signer/sign_test.gozetaclient/chains/evm/signer/outbound_data_test.gozetaclient/chains/base/signer.gozetaclient/chains/evm/signer/signer.gozetaclient/chains/evm/signer/sign.gozetaclient/chains/base/signer_batch_sign.go
📚 Learning: 2024-10-31T16:19:26.038Z
Learnt from: gartnera
Repo: zeta-chain/node PR: 3071
File: e2e/e2etests/test_stress_eth_withdraw.go:58-59
Timestamp: 2024-10-31T16:19:26.038Z
Learning: In the Go code within `e2e/utils/zetacore.go`, the function `WaitCctxMinedByInboundHash` does not return an error.
Applied to files:
zetaclient/mode/chaos/generated.go
📚 Learning: 2025-01-22T22:46:58.820Z
Learnt from: gartnera
Repo: zeta-chain/node PR: 3395
File: .github/workflows/reusable-sim.yml:29-30
Timestamp: 2025-01-22T22:46:58.820Z
Learning: The zeta-chain/node repository uses Go version >= 1.22. Do not suggest downgrading to earlier versions like 1.21.
Applied to files:
zetaclient/chains/zrepo/zrepo.go
📚 Learning: 2025-03-04T22:39:58.395Z
Learnt from: gartnera
Repo: zeta-chain/node PR: 3632
File: zetaclient/chains/solana/signer/signer.go:304-304
Timestamp: 2025-03-04T22:39:58.395Z
Learning: The Solana signer implementation in zetaclient/chains/solana/signer/signer.go has limited test coverage, particularly for the transaction broadcasting logic with fallback scenarios. Adding this coverage has been acknowledged as a potential future improvement outside the scope of immediate fixes.
Applied to files:
zetaclient/chains/evm/signer/signer_test.gozetaclient/chains/evm/signer/signer_admin_test.gozetaclient/chains/evm/signer/sign_test.gozetaclient/chains/evm/signer/outbound_data_test.gozetaclient/chains/base/signer.gozetaclient/chains/base/signer_batch_info.gozetaclient/chains/evm/evm.gozetaclient/chains/evm/signer/signer.gozetaclient/chains/evm/signer/sign.gozetaclient/chains/base/signer_batch_sign.go
📚 Learning: 2024-07-05T00:02:31.446Z
Learnt from: ws4charlie
Repo: zeta-chain/node PR: 2411
File: zetaclient/orchestrator/chain_activate.go:184-247
Timestamp: 2024-07-05T00:02:31.446Z
Learning: The `CreateSignerObserverBTC` function in `zetaclient/orchestrator/chain_activate.go` is covered by unit tests in `zetaclient/orchestrator/chain_activate_test.go`.
Applied to files:
zetaclient/chains/evm/signer/signer_test.gozetaclient/chains/evm/signer/signer_admin_test.gozetaclient/chains/evm/signer/sign_test.gozetaclient/chains/evm/signer/outbound_data_test.gozetaclient/chains/base/signer.gozetaclient/chains/base/signer_batch_info.gozetaclient/chains/evm/signer/signer.gozetaclient/chains/evm/signer/sign.gozetaclient/chains/base/signer_batch_sign.go
📚 Learning: 2024-07-05T00:02:36.493Z
Learnt from: ws4charlie
Repo: zeta-chain/node PR: 2411
File: zetaclient/orchestrator/chain_activate.go:116-181
Timestamp: 2024-07-05T00:02:36.493Z
Learning: The `CreateSignerObserverEVM` function in `zetaclient/orchestrator/chain_activate.go` is covered by unit tests in `zetaclient/orchestrator/chain_activate_test.go`.
Applied to files:
zetaclient/chains/evm/signer/signer_test.gozetaclient/chains/evm/signer/signer_admin_test.gozetaclient/chains/evm/signer/sign_test.gozetaclient/chains/evm/observer/outbound.gozetaclient/chains/evm/signer/outbound_data_test.gozetaclient/chains/base/signer.gozetaclient/chains/evm/evm.gozetaclient/chains/evm/signer/signer.gozetaclient/chains/evm/signer/sign.gozetaclient/chains/base/signer_batch_sign.go
📚 Learning: 2024-10-08T15:34:48.217Z
Learnt from: ws4charlie
Repo: zeta-chain/node PR: 2907
File: zetaclient/chains/bitcoin/observer/outbound_test.go:81-82
Timestamp: 2024-10-08T15:34:48.217Z
Learning: In the `mineTxNSetNonceMark` function within `bitcoin/observer/outbound_test.go`, it's acceptable to hardcode the chain ID, as the tests do not require varying chain IDs.
Applied to files:
zetaclient/chains/evm/signer/signer_test.gozetaclient/chains/evm/signer/sign_test.gozetaclient/chains/evm/observer/outbound.gozetaclient/chains/evm/signer/outbound_data_test.gozetaclient/chains/base/signer.go
📚 Learning: 2025-01-13T23:23:35.853Z
Learnt from: ws4charlie
Repo: zeta-chain/node PR: 3306
File: zetaclient/chains/bitcoin/signer/sign.go:155-211
Timestamp: 2025-01-13T23:23:35.853Z
Learning: Amount validation for Bitcoin outbound transactions, including checks for gas price, recipient address, and dust threshold, is performed in NewOutboundData before the transaction signing process begins.
Applied to files:
zetaclient/chains/evm/signer/signer_test.gozetaclient/chains/evm/signer/signer_admin_test.gozetaclient/chains/evm/signer/sign_test.gozetaclient/chains/evm/signer/outbound_data_test.go
📚 Learning: 2025-01-13T23:23:35.853Z
Learnt from: ws4charlie
Repo: zeta-chain/node PR: 3306
File: zetaclient/chains/bitcoin/signer/sign.go:155-211
Timestamp: 2025-01-13T23:23:35.853Z
Learning: Bitcoin outbound transaction validation in NewOutboundData includes checks for:
1. Valid CCTX
2. Gas token only
3. Non-negative fee rate
4. Valid receiver address format and type
5. Dust amount threshold
6. Compliance requirements
Applied to files:
zetaclient/chains/evm/signer/signer_test.gozetaclient/chains/evm/signer/sign_test.gozetaclient/chains/evm/signer/outbound_data_test.gozetaclient/chains/evm/signer/signer.go
📚 Learning: 2025-02-06T16:29:58.925Z
Learnt from: gartnera
Repo: zeta-chain/node PR: 3485
File: e2e/utils/sui/signer.go:24-35
Timestamp: 2025-02-06T16:29:58.925Z
Learning: The Sui blockchain requires signatures in [R || S || V] format, which is best generated using go-ethereum's crypto.Sign function that takes an ecdsa.PrivateKey.
Applied to files:
zetaclient/testutils/mocks/tss.gozetaclient/chains/evm/signer/signer_admin_test.gozetaclient/chains/evm/signer/sign_test.gozetaclient/chains/evm/signer/signer.go
📚 Learning: 2025-09-15T13:42:17.594Z
Learnt from: lumtis
Repo: zeta-chain/node PR: 4199
File: zetaclient/chains/evm/signer/signer_admin.go:25-26
Timestamp: 2025-09-15T13:42:17.594Z
Learning: In PR 4199, the CLI references to CmdMigrateTssFunds in x/crosschain/client/cli/ files are intentionally not updated as they are out of scope for this specific refactor focused on removing ERC20 custody messages.
Applied to files:
zetaclient/chains/evm/signer/signer_admin_test.gozetaclient/chains/evm/signer/sign.go
📚 Learning: 2024-11-06T21:11:34.420Z
Learnt from: gartnera
Repo: zeta-chain/node PR: 3105
File: cmd/zetae2e/local/bitcoin.go:8-8
Timestamp: 2024-11-06T21:11:34.420Z
Learning: In the `e2e` codebase, it's acceptable to use `github.com/stretchr/testify/require` outside of test files.
Applied to files:
zetaclient/chains/evm/signer/signer_admin_test.go
📚 Learning: 2024-10-10T18:54:33.554Z
Learnt from: ws4charlie
Repo: zeta-chain/node PR: 2987
File: testutil/sample/memo.go:0-0
Timestamp: 2024-10-10T18:54:33.554Z
Learning: In the `testutil/sample/memo.go` file, unchecked type assertions in the `ABIPack` function are acceptable because it is designed for unit tests and simplicity.
Applied to files:
zetaclient/chains/evm/signer/signer_admin_test.gopkg/math/pairing_test.go
📚 Learning: 2024-11-06T21:10:14.301Z
Learnt from: gartnera
Repo: zeta-chain/node PR: 3105
File: e2e/runner/setup_bitcoin.go:51-69
Timestamp: 2024-11-06T21:10:14.301Z
Learning: In test code (`e2e/runner/setup_bitcoin.go`), adding security measures for private key handling in the `GetBtcAddress` method is not required.
Applied to files:
zetaclient/chains/evm/signer/signer_admin_test.go
📚 Learning: 2024-07-04T23:47:56.072Z
Learnt from: ws4charlie
Repo: zeta-chain/node PR: 2411
File: zetaclient/orchestrator/orchestrator.go:222-237
Timestamp: 2024-07-04T23:47:56.072Z
Learning: The `GetUpdatedChainObserver` method in the `Orchestrator` class is covered by unit tests in `zetaclient/orchestrator/orchestrator_test.go` and `zetaclient/orchestrator/chain_activate_test.go`.
Applied to files:
zetaclient/chains/evm/signer/sign_test.go
📚 Learning: 2025-01-21T04:39:26.779Z
Learnt from: ws4charlie
Repo: zeta-chain/node PR: 3381
File: zetaclient/chains/bitcoin/signer/sign.go:71-74
Timestamp: 2025-01-21T04:39:26.779Z
Learning: In Bitcoin ZRC20 token context, `txData.txSize` represents the gas limit and is used to determine if a transaction is a ZRC20 'withdraw' operation that should be charged less fee by comparing it with `common.BtcOutboundBytesWithdrawer`.
Applied to files:
zetaclient/chains/evm/signer/sign_test.gozetaclient/chains/evm/signer/sign.go
📚 Learning: 2025-01-14T03:37:18.430Z
Learnt from: ws4charlie
Repo: zeta-chain/node PR: 3306
File: pkg/math/integer_test.go:0-0
Timestamp: 2025-01-14T03:37:18.430Z
Learning: Use big.Int for handling large number calculations and edge cases to avoid integer overflow issues.
Applied to files:
pkg/math/pairing.go
📚 Learning: 2025-02-04T06:03:54.382Z
Learnt from: ws4charlie
Repo: zeta-chain/node PR: 3461
File: x/observer/migrations/v10/migrate.go:29-34
Timestamp: 2025-02-04T06:03:54.382Z
Learning: In the observer module's ChainParams struct, the Confirmation field is a value type (not a pointer), so nil checks are not needed.
Applied to files:
zetaclient/chains/evm/observer/outbound.go
📚 Learning: 2025-02-04T06:12:41.760Z
Learnt from: ws4charlie
Repo: zeta-chain/node PR: 3461
File: x/observer/types/chain_params.go:59-62
Timestamp: 2025-02-04T06:12:41.760Z
Learning: The legacy `ConfirmationCount` field in the ChainParams struct is planned to be deprecated after a full upgrade to the new confirmation count system that uses SafeInboundCount, FastInboundCount, SafeOutboundCount, and FastOutboundCount fields.
Applied to files:
zetaclient/chains/evm/observer/outbound.go
📚 Learning: 2024-08-01T18:08:13.681Z
Learnt from: kingpinXD
Repo: zeta-chain/node PR: 2615
File: x/crosschain/keeper/msg_server_vote_outbound_tx_test.go:472-472
Timestamp: 2024-08-01T18:08:13.681Z
Learning: The `SaveFailedOutbound` function in `x/crosschain/keeper/msg_server_vote_outbound_tx.go` requires a string argument for the error message.
Applied to files:
zetaclient/chains/evm/observer/outbound.go
📚 Learning: 2024-10-30T17:56:16.341Z
Learnt from: gartnera
Repo: zeta-chain/node PR: 3070
File: cmd/zetae2e/init.go:0-0
Timestamp: 2024-10-30T17:56:16.341Z
Learning: In code reviews for Go files like `cmd/zetae2e/init.go` in the ZetaChain project, avoid suggesting unrelated refactoring. Focus comments on changes relevant to the PR objectives.
Applied to files:
zetaclient/chains/evm/evm.gozetaclient/chains/evm/signer/sign.go
📚 Learning: 2024-10-10T19:29:59.300Z
Learnt from: ws4charlie
Repo: zeta-chain/node PR: 2987
File: pkg/memo/fields_v0_test.go:270-320
Timestamp: 2024-10-10T19:29:59.300Z
Learning: In the `FieldsV0` struct of the `memo` package, `RevertAddress` may be left empty when `CallOnRevert` is set to true.
Applied to files:
zetaclient/chains/evm/signer/sign.go
🧬 Code graph analysis (14)
zetaclient/chains/evm/signer/outbound_data.go (1)
testutil/sample/crosschain.go (1)
InboundParams(176-191)
zetaclient/dry/dry.go (1)
zetaclient/chains/tssrepo/client.go (1)
TSSClient(13-31)
zetaclient/tss/service.go (2)
zetaclient/tss/crypto.go (1)
PubKey(28-31)zetaclient/tss/config.go (1)
Version(20-20)
zetaclient/chains/evm/signer/signer_test.go (1)
zetaclient/chains/evm/signer/outbound_data.go (1)
NewOutboundData(53-155)
zetaclient/chains/evm/signer/signer_admin_test.go (3)
zetaclient/chains/evm/signer/outbound_data.go (2)
NewOutboundData(53-155)OutboundData(22-49)zetaclient/chains/evm/signer/signer.go (2)
Signer(62-76)ErrWaitForSignature(47-47)testutil/sample/crypto.go (2)
EthAddress(89-91)Hash(189-191)
zetaclient/chains/evm/signer/sign_test.go (4)
zetaclient/chains/evm/signer/outbound_data.go (2)
NewOutboundData(53-155)OutboundData(22-49)zetaclient/chains/evm/signer/signer.go (2)
Signer(62-76)ErrWaitForSignature(47-47)zetaclient/chains/base/signer.go (1)
Signer(21-47)zetaclient/chains/base/signer_batch_info.go (2)
NewTSSKeysignBatch(48-52)NewTSSKeysignInfo(28-33)
zetaclient/testutils/mocks/zetacore.go (2)
zetaclient/maintenance/client.go (1)
ZetacoreClient(12-20)zetaclient/orchestrator/contextupdater.go (1)
ZetacoreClient(18-32)
zetaclient/chains/evm/signer/outbound_data_test.go (1)
zetaclient/chains/evm/signer/outbound_data.go (1)
NewOutboundData(53-155)
zetaclient/chains/base/signer.go (2)
zetaclient/chains/base/signer_batch_info.go (1)
TSSKeysignInfo(19-25)zetaclient/metrics/metrics.go (2)
NextTSSNonce(83-87)Info(119-123)
zetaclient/chains/base/signer_batch_info.go (1)
pkg/math/pairing.go (2)
MaxPairValue(10-10)CantorPair(15-18)
pkg/math/pairing_test.go (1)
pkg/math/pairing.go (3)
MaxPairValue(10-10)CantorPair(15-18)CantorUnpair(22-47)
zetaclient/chains/evm/evm.go (3)
pkg/scheduler/opts.go (1)
BlockTicker(47-49)zetaclient/chains/base/signer.go (1)
OutboundIDFromCCTX(215-218)zetaclient/chains/base/signer_batch_info.go (1)
NonceToBatchNumber(149-151)
zetaclient/chains/evm/signer/signer.go (3)
zetaclient/chains/base/signer.go (1)
Signer(21-47)zetaclient/metrics/metrics.go (1)
NextTSSNonce(83-87)zetaclient/chains/evm/signer/outbound_data.go (1)
NewOutboundData(53-155)
zetaclient/chains/base/signer_batch_sign.go (6)
zetaclient/chains/base/signer.go (1)
Signer(21-47)zetaclient/chains/zrepo/zrepo.go (1)
ZetaRepo(44-50)pkg/scheduler/context.go (1)
BlockFromContextWithDelay(27-41)zetaclient/chains/base/signer_batch_info.go (5)
TSSKeysignBatch(36-45)NonceToBatchNumber(149-151)NewTSSKeysignInfo(28-33)BatchNumberToRange(155-157)NewTSSKeysignBatch(48-52)pkg/retry/retry.go (2)
Retry(147-157)DoWithBackoff(62-82)zetaclient/logs/fields.go (1)
FieldNonce(23-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Cursor Bugbot
- GitHub Check: test-sim-import-export / sim
- GitHub Check: test-sim-nondeterminism / sim
- GitHub Check: test-sim-after-import / sim
- GitHub Check: test-sim-fullappsimulation / sim
- GitHub Check: lint
- GitHub Check: gosec
- GitHub Check: build-and-test
- GitHub Check: build
- GitHub Check: analyze (go)
🔇 Additional comments (24)
changelog.md (1)
7-8: LGTM!The changelog entry accurately describes the performance improvement feature introduced in this PR.
cmd/zetae2e/local/performance.go (1)
167-172: LGTM! Correct timing measurement.Moving the timer start to after the deposit operation ensures that the performance test measures only the withdraw test duration, not the setup overhead. This provides more accurate performance metrics.
zetaclient/testutils/mocks/zetacore.go (1)
171-196: LGTM!The parameter renaming from
ctxto_a0is a standard mockery convention for generated mock code. No functional changes.zetaclient/chains/evm/observer/outbound.go (1)
136-136: LGTM! Helpful clarification.The comment appropriately documents that the
continueKeysignflag is no longer used with the batch keysign implementation. This provides valuable context for future maintainers.zetaclient/mode/chaos/generate/sample.json (1)
96-96: LGTM!The
GetBlockHeightentry is consistent with other chaos mode configuration entries and properly supports the new method added to the ZetacoreClient interface.zetaclient/chains/tssrepo/client.go (1)
30-30: LGTM!The interface addition is clean and follows Go conventions. The signature clearly expresses intent: query whether signatures for given digests on a specific chain are already cached.
zetaclient/chains/evm/signer/outbound_data.go (1)
143-143: LGTM! Height sourcing aligns with PR objectives.Using
cctx.InboundParams.ObservedExternalHeightfor the outbound data height is semantically appropriate and decouples outbound processing from the real ZetaChain height, consistent with the PR's architectural goal of using deterministic height values.zetaclient/mode/chaos/generated.go (1)
96-108: Generated code follows established patterns.This is generated code (as noted in line 1). The new
GetBlockHeightandIsSignatureCachedwrappers follow the same patterns as other methods in the file, suggesting the generator is functioning correctly.Also applies to: 1651-1659
zetaclient/chains/evm/signer/sign.go (1)
45-52: LGTM! Height parameter consistently removed across all signing functions.All seven signing functions have been consistently updated to remove the
heightparameter fromsigner.Sign()calls. This mechanical refactoring aligns with the PR's objective to decouple signing from ZetaChain block height and move toward deterministic, nonce-based scheduling.Also applies to: 91-98, 108-115, 125-132, 160-167, 193-200, 209-216
zetaclient/testutils/mocks/tss.go (1)
113-115: LGTM! Mock implementation returns safe default.The mock returns
false, indicating no cached signature, which is a sensible default for tests. Tests requiring cached signature behavior can use dependency injection or test setup to override this behavior if needed.zetaclient/metrics/metrics.go (1)
82-87: NextTSSNonce metric definition looks consistent and low-riskThe new
NextTSSNoncegauge follows the existing per-chain metric pattern (namespace, label set, help text) and should integrate cleanly with the signer logic using it.zetaclient/chains/evm/signer/outbound_data_test.go (1)
20-22: Updated NewOutboundData usage aligns with new constructor signatureSwitching
newOutboundtoNewOutboundData(ctx, cctx, logger)correctly matches the updated API and keeps the test semantics intact.zetaclient/chains/evm/signer/signer_admin_test.go (4)
3-15: Tests correctly exercise digest-based signing flowUsing
NewOutboundDatawith a test logger plus the new digest helpers keeps tests aligned with production construction of admin txs and the newErrWaitForSignature/ cache semantics. This is a solid pattern for the other admin command tests in this file as well.Also applies to: 26-29
34-37: Consistent pre‑seeding of signatures for happy‑path testsPrecomputing the digest via the helper and then calling
mockSignaturebefore invoking the signer ensures tests remain deterministic while still going through the new cache‑firstSignpath. This is a clean way to validate each admin command without coupling to internal signer fields.Also applies to: 59-62, 79-82, 98-101, 130-133, 187-190, 246-249, 265-268, 323-326
152-161: Negative paths for missing signatures validate ErrWaitForSignatureCloning
txData, bumping the nonce, and then calling the command withtxDataOtheris an effective way to simulate a digest that has not yet been signed and assertErrWaitForSignatureis plumbed correctly. This nicely exercises the digest cache miss path without touching internal state.Also applies to: 294-305, 339-348
351-436: Digest helpers closely mirror on‑chain encodingThe helpers correctly:
- Use
erc20custody.ERC20CustodyMetaData.GetAbi()and pack the expected function selectors (whitelist,withdraw,pause/unpause).- Rebuild txs with
toChainID,to, appropriateamount(zero ortxData.amount), gas, and nonce.- Derive the digest via
signer.evmClient.Signer().Hash(tx).Bytes().This should keep test digests in lock‑step with production behavior as contracts evolve.
zetaclient/chains/evm/evm.go (2)
58-67: Decoupled block subscriptions for CCTX and keysignCreating separate
WatchNewBlockssubscriptions for CCTX scheduling and TSS keysign (cctxBlockChanvskeysignBlockChan) and wiring them viascheduler.BlockTickercleanly separates the two concerns. This should prevent keysign backpressure from delaying CCTX scheduling and vice versa, while keeping error handling straightforward.Also applies to: 101-106
121-195: Nonce‑driven CCTX scheduling looks correct and simplerThe new
scheduleCCTXflow is easy to reason about:
- Stale events are filtered via
signer.IsStaleBlockEvent, avoiding reprocessing old block notifications.- Chain params are refreshed each time, keeping
OutboundScheduleLookaheadand addresses current.nextTSSNoncefrom the EVM account is used to:
- Vote and advance already‑processed CCTXs (
TssNonce < nextTSSNonce) viaVoteOutboundIfConfirmed.- Only spawn
TryProcessOutboundfor nonces at or beyond the current TSS nonce.- The
lookahead/outboundScheduleLookBackchecks guard against pathological nonce gaps.This is a solid improvement over height‑based gating.
zetaclient/chains/evm/signer/signer.go (1)
42-48: ErrWaitForSignature and NextTSSNonce integrate cleanly with base signer stateDefining
ErrWaitForSignatureand usingNextTSSNonceto:
- Query
NonceAton the TSS account, and- Feed
SetNextTSSNoncefor metrics/trackingprovides a clear contract between scheduling and the signer. The shared
zeroValueconstant is also appropriate for zero‑ETH outbounds as long as it remains treated as immutable throughout the codebase.Also applies to: 163-174
zetaclient/chains/base/signer_batch_info.go (3)
11-45: Batch metadata and accessors are straightforward and coherent
TSSKeysignInfoandTSSKeysignBatchcleanly capture the minimal state needed for batching:
nonceLow/nonceHighare updated only viaAddKeysignInfo.- Accessors (
BatchNumber,Digests,NonceLow,NonceHigh) keep call sites readable and encapsulate the mapping between nonces and batches.This provides a solid foundation for the higher‑level batching logic.
Also applies to: 47-73
74-113: Sequentiality, end‑of‑batch detection, and nonce↔batch mapping look correct
IsSequential’slen(digests) == nonceHigh - nonceLow + 1check is a simple and effective way to guard against gaps in the collected digests.IsEndusingBatchNumberToRangecorrectly determines whether a batch has reached its upper nonce bound.NonceToBatchNumberandBatchNumberToRangeimplement the intended 0–9, 10–19, ... mapping and align with the comments.These helpers make the invariants around batching explicit and easy to audit.
Also applies to: 141-157
119-139: Cantor‑based keysign height computation is reasonable; bounds checks are appropriateUsing
CantorPair(batchNumber+1, chainID)to derive a uniqueKeysignHeightper (batch, chain) pair is a neat choice, and the guards againstbatchNumber >= MaxPairValueand invalidchainIDprovide a clear failure mode rather than silent overflow.This should keep TSS requests uniquely identifiable across chains.
zetaclient/chains/base/signer_batch_sign.go (2)
27-47: Stale block detection using live height vs event height is appropriate
IsStaleBlockEvent:
- Extracts the block event from context.
- Compares its height with
zetaRepo.GetBlockHeight(ctx).Skipping events where
block.Height < zetaHeightis a sensible way to avoid processing backlogged events once the client has caught up, which is important now that keysign is tied to real‑time height.
151-181: No issues identified—code is correct as written.Verification confirms:
- Go 1.22.5 in go.mod satisfies the 1.21+ requirement for the
minbuiltin (line 214)- RWMutex discipline is correct across all four functions:
GetSignatureOrAddDigestandAddBatchSignaturesappropriately use exclusive locks, whilecollectKeysignBatchcorrectly uses read locks, andremoveKeysignInfocorrectly uses exclusive locks for deletion
| txEtherDeposit := r.DepositEtherDeployer() | ||
| r.WaitForMinedCCTX(txEtherDeposit) | ||
|
|
||
| r.Logger.Print("🏃 starting Ethereum withdraw performance tests") |
There was a problem hiding this comment.
This is because the r.WaitForMinedCCTX may take minutes when there is a lot of pending deposits to process during stress test, which leads to inaccurate outbound stress testing time.
lumtis
left a comment
lumtis
left a comment
There was a problem hiding this comment.
I don't see any issues in the change, it's typically what will nee to be tested manually on testnet with stress tests
I might think it would be good to track this one as well for a security review.
| } | ||
|
|
||
| // PrepareForKeysign checks if it's time to sign and clean up stale keysign information. | ||
| func (s *Signer) PrepareForKeysign( |
There was a problem hiding this comment.
This can be broken down into two functions
- Function that only reads and validates
- Function that writes , which is the
s.removeKeysignInfo(nonceLow)section .
|
|
||
| // check if it's the time to perform keysign | ||
| interval := e.observer.ChainParams().OutboundScheduleInterval | ||
| shouldSign, err := e.signer.PrepareForKeysign(ctx, zetaRepo, nextTSSNonce, zetaHeight, interval) |
There was a problem hiding this comment.
We can add Pending Nonces as an argument to this function and KeySignBatch
| slices.Sort(allNonces) | ||
|
|
||
| // collect digests within the batch's range | ||
| for _, nonce := range allNonces { |
There was a problem hiding this comment.
can we loop batch low to batch high , and pick the nonces from the map ?
| } | ||
|
|
||
| // AddKeysignInfo adds one TSS keysign information to the batch. | ||
| func (b *TSSKeysignBatch) AddKeysignInfo(nonce uint64, info TSSKeysignInfo) { |
There was a problem hiding this comment.
nit : this seems to be called by nonces in acesding order , maybe we can just set to high directly for every call.
But this is fine too , and a bit more robust , if we need to call it from other locations in the future
Description
Remaining work:
This PR implements a sequential batch TSS keysign scheduler for EVM chain, improving outbound speed by
4~5X.Decouple CCTX process goroutine
scheduleCCTXandTSSkeysign scheduler goroutinescheduleKeysign.Use an artificial (deterministic) height instead of real ZetaChain height to create TSS keysign request.
This improves outbound performance by
2X.Schedule
TSSkeysign for batched digests instead of only one single digest. Reduced total keysign requests number from multiple to only one (per chain).Schedule
TSSkeysign by nonce (batched) sequentially without waiting intervals, replacing the existing interval based logiczeta_height % interval == cctx_nonce % interval.This improves outbound performance by
2~3XThe
ethwithdraw stress test result before:The result after:
Closes #4436
How Has This Been Tested?
Note
Cursor Bugbot is generating a summary for commit cdf2b23. Configure here.