Txs patches - #2173
Conversation
…ppwBMGGagNF57Zq8qvD'. Fixed 'cryto.Verify' internals according to the XEdDSA spec for X25519. For more info visit https://signal.org/docs/specifications/xeddsa/#curve25519.
There was a problem hiding this comment.
🟡 Not ready to approve
There are a few correctness/clarity issues in protocol-facing feature ID annotation, error context strings, and a mainnet abnormal-tx cleanup path that can unnecessarily re-initialize a large map.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR strengthens transaction/order verification and state-application correctness by (1) normalizing legacy padded Ethereum EIP-712 order signatures while preserving original bytes for feature-gated validation, (2) adding mainnet-driven regression tests and state “patching” hooks for known abnormal transactions, and (3) tightening/augmenting Ride invocation balance validation behavior in specific mainnet-height conditions.
Changes:
- Normalize padded (129-byte) protobuf EIP-712 order signatures to standard 65-byte form while keeping original bytes for deterministic-finality-era rejection.
- Add regression tests for real mainnet exchange/order verification and padded Ethereum order signatures.
- Add special-case mainnet abnormal-tx snapshot application and layered balance-change validation for Ride invocations.
File summaries
| File | Description |
|---|---|
| pkg/state/verifier_test.go | Adds exchange transaction verification test using real mainnet JSON. |
| pkg/state/transaction_checker.go | Extends order feature gating to include deterministic finality checks for Ethereum order signature length. |
| pkg/state/transaction_checker_test.go | Adds regression test for padded Ethereum order signature behavior under feature activation combinations. |
| pkg/state/exclusions.go | Introduces abnormal mainnet transaction snapshot data + lazy init/cleanup for patch lookup. |
| pkg/state/appender.go | Refactors append flow, introduces abnormal-tx patch application path, and adjusts fee counting / address indexing flow. |
| pkg/settings/features.go | Adds DeterministicFinality feature constant and metadata. |
| pkg/ride/tree_evaluation_test.go | Adjusts Ride test env setup to include explicit height. |
| pkg/ride/functions_proto.go | Adds mainnet-height conditional behavior to validate intermediate balances “scala-like” during invocations. |
| pkg/ride/environment.go | Implements layered balance-change tracking and intermediate effective-balance validation helpers. |
| pkg/ride/diff_state.go | Adds iterator helper for changed WAVES-balance accounts. |
| pkg/proto/types.go | Preserves original EIP-712 signature bytes on Ethereum orders and uses them for protobuf serialization. |
| pkg/proto/transactions_with_proofs.go | Ensures orig EIP-712 signature bytes are set during JSON unmarshal for compatibility. |
| pkg/proto/protobuf_converters.go | Adds padded-signature normalization + stores original protobuf signature bytes in Ethereum orders. |
| pkg/proto/proto_test.go | Adds unit tests for padded-signature normalization helper. |
| pkg/proto/eth_signer.go | Standardizes Ethereum signature length constants. |
| pkg/proto/eth_crypto.go | Refactors signature validation and adds stronger checks before public key recovery. |
| pkg/crypto/crypto.go | Extends Ed25519 verification to support specific non-canonical s values (XEdDSA-related). |
| pkg/crypto/crypto_test.go | Adds a mainnet-derived verification test case and improves test structure. |
| .golangci-strict.yml | Excludes pkg/state/exclusions.go from strict linting. |
Review details
Suppressed comments (1)
pkg/state/transaction_checker.go:878
- This wrapper message refers to “metamask feature checks”, but the called helper also applies deterministic finality checks now. Updating the context string will make errors less confusing to debug.
if errO2 := checkOrderWithFeatures(o2, metamaskActivated, deterministicFinalityActivated); errO2 != nil {
return nil, errors.Wrap(errO2, "order2 metamask feature checks failed")
}
- Files reviewed: 19/19 changed files
- Comments generated: 6
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| DeterministicFinality // 25 Deterministic Finality and RideV9 | ||
| InvokeExpression // 25 |
| func tryGetAbnormalMainnetTxPatch(txIDBytes []byte, blockHeight proto.Height) (abnormalTxInfo, bool, error) { | ||
| if blockHeight == nextHeightAfterLastAbnormalTxMainnet { | ||
| cleanAbnormalTxsMainnet() // clean unnecessary map | ||
| } |
| if errO1 := checkOrderWithFeatures(o1, metamaskActivated, deterministicFinalityActivated); errO1 != nil { | ||
| return nil, errors.Wrap(errO1, "order1 metamask feature checks failed") | ||
| } |
| r, s := data[:doubledParamSize], data[doubledParamSize:doubledSigSize-1] | ||
| rc, sc := r[ethereumSignatureParamSize:], s[ethereumSignatureParamSize:] // cut left unnecessary part | ||
| return bytes.Join([][]byte{rc, sc, data[doubledSigSize-1:]}, nil) |
There was a problem hiding this comment.
🟡 Not ready to approve
The abnormal mainnet patch cleanup path can re-initialize the large patch map at the cleanup height, defeating cleanup and potentially keeping unnecessary memory live, and there are a few misleading/incorrect messages/comments that should be corrected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (6)
pkg/state/appender.go:608
- The abnormal-tx patch logic includes height == nextHeightAfterLastAbnormalTxMainnet in the lookup range and calls cleanAbnormalTxsMainnet() inside tryGetAbnormalMainnetTxPatch(). If a node starts syncing from exactly nextHeightAfterLastAbnormalTxMainnet, the map will be cleaned and then immediately re-initialized by getAbnormalTxMainnet() in the same call path, defeating the intended cleanup and keeping the large map alive for the rest of the process lifetime (cleaner sync.Once prevents future cleanup). Limit patch lookup to <= lastAbnormalTxsMainnetHeight and perform cleanup separately at height == nextHeightAfterLastAbnormalTxMainnet without calling getAbnormalTxMainnet().
// handle some abnormal transactions in mainnet
if !params.validatingUtx && a.settings.AddressSchemeCharacter == proto.MainNetScheme &&
blockHeight >= firstAbnormalTxsMainnetHeight &&
blockHeight <= nextHeightAfterLastAbnormalTxMainnet {
txPatch, ok, err := tryGetAbnormalMainnetTxPatch(txIDBytes, blockHeight)
pkg/state/appender.go:633
- tryGetAbnormalMainnetTxPatch() currently performs cleanup based on blockHeight, but it still calls getAbnormalTxMainnet() afterwards, which may reinitialize the map right after cleaning (especially when starting from nextHeightAfterLastAbnormalTxMainnet). With cleanup moved to doAppendTx(), this helper should only perform the ID conversion and lookup.
func tryGetAbnormalMainnetTxPatch(txIDBytes []byte, blockHeight proto.Height) (abnormalTxInfo, bool, error) {
if blockHeight == nextHeightAfterLastAbnormalTxMainnet {
cleanAbnormalTxsMainnet() // clean unnecessary map
}
pkg/proto/protobuf_converters.go:696
- The comment for transformToStandardSig() says the padded form is "65 bytes format with r and s padded", but the code actually handles a 129-byte encoding (r[64] + s[64] + v[1]). Updating the comment to match the actual on-wire format will reduce confusion for future maintenance.
// transformToStandardSig transforms signature from protobuf message to standard 65 bytes format
// (r[32] + s[32] + v[1]) if it is in 65 bytes format with r and s values padded with 32 bytes prefix data.
// This transformation is required due to the bug in Scala implementation, which allows
// signature values to be padded with 32 bytes prefix data.
// Padded signatures are disallowed since deterministic finality feature (25) activation.
pkg/state/transaction_checker.go:878
- checkOrderWithFeatures() now enforces both pre-MetaMask constraints and (optionally) DeterministicFinality-related signature-length constraints, but the wrapped error context still says "metamask feature checks failed". This makes failures from deterministic-finality checks misleading; consider using a more general wrapper message.
if errO1 := checkOrderWithFeatures(o1, metamaskActivated, deterministicFinalityActivated); errO1 != nil {
return nil, errors.Wrap(errO1, "order1 metamask feature checks failed")
}
if errO2 := checkOrderWithFeatures(o2, metamaskActivated, deterministicFinalityActivated); errO2 != nil {
return nil, errors.Wrap(errO2, "order2 metamask feature checks failed")
}
pkg/state/verifier_test.go:189
- Subtest name has a typo: "verify_first_oder" -> "verify_first_order".
t.Run("verify_first_oder", func(t *testing.T) {
pkg/settings/features.go:34
- Feature ID comments became inconsistent after inserting DeterministicFinality: InvokeExpression is no longer ID 25 (it becomes 26 via iota). Keeping the numeric comments accurate helps avoid accidental feature-ID mismatches.
ecrecoverFix // 24, intentionally package private
DeterministicFinality // 25 Deterministic Finality and RideV9
InvokeExpression // 25
)
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
At least one newly added test asserts an outdated error message prefix and will fail after the transaction checker’s wrap-message change.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
pkg/crypto/crypto.go:536
- newScalarNonCanonicalReducedBytes reduces x mod l via SetUniformBytes, so the subsequent SetCanonicalBytes on ss.Bytes() is redundant and the inline comment about checking "or has no excess bits" is misleading (excess-bits check is enforced earlier in Verify). Consider doing an explicit excess-bits check here (defense in depth) and returning the reduced scalar directly.
ssBytesCanonical := ss.Bytes() // returns canonical reduced by l form
ss, err = ss.SetCanonicalBytes(ssBytesCanonical) // sets x and check that x is reduced by l or has no excess bits
if err != nil {
- Files reviewed: 19/19 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
It introduces a concrete cryptographic verification flaw (non-canonical Ed25519 scalar handling) and a rollback/reorg correctness risk in abnormal-tx patch lifecycle that must be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
pkg/state/exclusions.go:74
- abnormalTxsMainnetInitializer/cleaner use sync.Once, but cleanAbnormalTxsMainnet() permanently nils the map without resetting the initializer. If the node rolls back/reorgs to a height within the abnormal-txs range after cleanup (rollback is supported in state), the patch map will never be rebuilt in this process and abnormal tx patches will silently stop applying.
//nolint:gochecknoglobals // special case
var (
abnormalTxsMainnetInitializer sync.Once
abnormalTxsMainnetCleaner sync.Once
abnormalTxsMainnet map[crypto.Digest]abnormalTxInfo
- Files reviewed: 19/19 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| func newScalarNonCanonicalReducedBytes(x []byte) (*edwards.Scalar, error) { | ||
| const ( | ||
| reducedScalarSize = 32 | ||
| uniformScalarSize = 64 | ||
| ) | ||
| if len(x) != reducedScalarSize { | ||
| return nil, errors.New("newScalarNonCanonicalReducedBytes: invalid scalar length") | ||
| } | ||
| // expand it to perform operations by `mod l` inside. | ||
| var wide [uniformScalarSize]byte | ||
| copy(wide[:reducedScalarSize], x) | ||
| ss, err := edwards.NewScalar().SetUniformBytes(wide[:]) // sets x mod l | ||
| if err != nil { | ||
| return nil, fmt.Errorf("newScalarNonCanonicalReducedBytes: %w", err) | ||
| } | ||
| ssBytesCanonical := ss.Bytes() // returns canonical reduced by l form | ||
| ss, err = ss.SetCanonicalBytes(ssBytesCanonical) // sets x and check that x is reduced by l or has no excess bits | ||
| if err != nil { | ||
| return nil, fmt.Errorf("newScalarNonCanonicalReducedBytes: %w", err) | ||
| } | ||
| return ss, nil | ||
| } |
| ecrecoverFix // 24, intentionally package private | ||
| InvokeExpression // 25 | ||
| DeterministicFinality // 25 Deterministic Finality and RideV9 | ||
| InvokeExpression // 26 |
There was a problem hiding this comment.
InvokeExpression and DeterministicFinality marked as not implemented, so they can't affect state database.
There was a problem hiding this comment.
🟡 Not ready to approve
The abnormal-tx patch cleanup uses sync.Once in a way that can prevent re-applying patches after rollback/rescan within the same process, which is a correctness risk for node operation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
pkg/state/exclusions.go:74
- cleanAbnormalTxsMainnet() uses sync.Once to permanently nil out abnormalTxsMainnet, but getAbnormalTxMainnet() will never re-initialize it after cleanup because abnormalTxsMainnetInitializer has already fired. If the node rolls back below lastAbnormalTxsMainnetHeight (or tests re-enter that height range in the same process), patches will silently stop applying. Consider either not cleaning at all, or resetting the initializer/cleaner so the map can be rebuilt on demand after a rollback/rescan.
func cleanAbnormalTxsMainnet() {
abnormalTxsMainnetCleaner.Do(func() { abnormalTxsMainnet = nil })
}
pkg/state/transaction_checker_test.go:654
- The commented-out DeterministicFinality test still expects the old wrapper prefix ("order2 metamask feature checks failed"), but the production code now wraps with "order2 features checks failed". If this block is uncommented later, it will fail due to the stale expected string.
const expErrStr = "order2 metamask feature checks failed: " +
"invalid original EIP-712 signature length for ethereum order: got 129, want 65"
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
It introduces (1) an Ed25519 fallback path that can accept excess-bit scalars contrary to the stated XEdDSA rule, and (2) a sync.Once-based cleanup that prevents re-initializing abnormal-tx patches after rollback/reprocessing.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
pkg/crypto/crypto.go:525
- newScalarNonCanonicalReducedBytes() is used as a fallback when SetCanonicalBytes() fails, but the current implementation reduces any 32-byte value mod l via SetUniformBytes() without first rejecting scalars with excess bits. This contradicts the function comment (“rejecting s if it has excess bits…”) and can unintentionally accept signatures that should be rejected (malleability/compat behavior change). Add an explicit excess-bits check (top 3 bits of x[31]) before reducing mod l, and drop the redundant SetCanonicalBytes() round-trip.
if len(x) != reducedScalarSize {
return nil, errors.New("newScalarNonCanonicalReducedBytes: invalid scalar length")
}
// expand it to perform operations by `mod l` inside.
var wide [uniformScalarSize]byte
- Files reviewed: 19/19 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| func cleanAbnormalTxsMainnet() { | ||
| abnormalTxsMainnetCleaner.Do(func() { abnormalTxsMainnet = nil }) | ||
| } |
There was a problem hiding this comment.
🟡 Human review recommended
It changes consensus-critical transaction application and Ride invocation validation logic (including mainnet height-gated behavior), which warrants careful human review despite added tests.
Review details
Suppressed comments (1)
pkg/crypto/crypto.go:533
newScalarNonCanonicalReducedBytes()documentation/comments say it rejects scalars with “excess bits”, but the function currently does not enforce that constraint itself (it only reduces viaSetUniformBytesand then canonicalizes the reduced result). This makes the function’s behavior rely on external callers doing the excess-bit check and is misleading for future reuse; please either add an explicit excess-bits guard here or adjust the comment to match actual behavior.
ssBytesCanonical := ss.Bytes() // returns canonical reduced by l form
ss, err = ss.SetCanonicalBytes(ssBytesCanonical) // sets x and check that x is reduced by l or has no excess bits
if err != nil {
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Human review recommended
It changes core consensus-adjacent validation paths (crypto verification, state appending patches, Ride invoke balance validation) and should receive careful human review despite only minor code-level nits found.
Review details
Suppressed comments (4)
pkg/crypto/crypto.go:535
- In newScalarNonCanonicalReducedBytes(), ss is already reduced/canonical after SetUniformBytes(wide[:]); calling SetCanonicalBytes(ss.Bytes()) cannot fail and makes the error branch and comment misleading. Consider returning ss directly to keep the intent clear and avoid dead code.
ssBytesCanonical := ss.Bytes() // returns canonical reduced by l form
ss, err = ss.SetCanonicalBytes(ssBytesCanonical) // sets x and check that x is reduced by l or has no excess bits
if err != nil {
return nil, fmt.Errorf("newScalarNonCanonicalReducedBytes: %w", err)
}
pkg/state/transaction_checker_test.go:654
- Leaving the DeterministicFinalityActivated subtest commented out makes intended post-activation behavior unclear. Keep it as a skipped test so it becomes active automatically once the feature is implemented/enabled in tests.
// TODO: uncomment when DeterministicFinality feature will be set to implemented.
/*
t.Run("DeterministicFinalityActivated", func(t *testing.T) {
const expErrStr = "order2 features checks failed: " +
"invalid original EIP-712 signature length for ethereum order: got 129, want 65"
pkg/state/transaction_checker_test.go:645
- This subtest name duplicates the one used inside the "NoRideV6" group, which makes test output harder to interpret. Rename it to reflect that RideV6 is activated in this branch.
t.Run("NoDeterministicFinalityActivation", func(t *testing.T) {
pkg/state/transaction_checker_test.go:643
- Leaving an entire subtest commented out makes it easy to forget and harder to see intended coverage. Prefer keeping the subtest in place and skipping it until DeterministicFinality is implemented.
This issue also appears on line 650 of the same file.
// TODO: uncomment when DeterministicFinality feature will be set to implemented.
/*
t.Run("DeterministicFinalityActivated", func(t *testing.T) {
to, info := createEnv(t, settings.DeterministicFinality)
_, err := to.tc.checkExchangeWithProofs(tx, info)
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
This pull request introduces several important improvements and fixes related to Ethereum signature handling, cryptographic verification, and protobuf order conversion, as well as some general code quality enhancements. The main focus is on correcting how padded Ethereum signatures are processed, improving signature verification robustness, and ensuring compatibility with legacy and current systems.
Ethereum signature handling and bugfixes:
transformToStandardSigto process and normalize padded Ethereum signatures (129 bytes with extra prefix data) to the standard 65-byte format, fixing a bug in protobuf order conversion and addressing legacy compatibility issues. (pkg/proto/protobuf_converters.go,pkg/proto/types.go,pkg/proto/transactions_with_proofs.go) [1] [2] [3] [4] [5] [6]validateEthereumSignatureRSfor internal use and improved error handling in public key recovery. (pkg/proto/eth_crypto.go) [1] [2] [3] [4] [5]EthereumSignatureLengthfor clarity and maintainability. (pkg/proto/eth_signer.go,pkg/proto/eth_crypto.go) [1] [2] [3] [4]Ed25519 signature verification improvements:
svalues according to the XEdDSA spec, increasing robustness and compatibility with signatures not fully reduced by the group order. (pkg/crypto/crypto.go)pkg/crypto/crypto_test.go) [1] [2] [3]Other improvements and code quality:
pkg/state/exclusions.goand performed minor import ordering and code cleanup. (.golangci-strict.yml,pkg/crypto/crypto.go,pkg/proto/eth_crypto.go,pkg/proto/protobuf_converters.go,pkg/ride/diff_state.go,pkg/ride/environment.go) [1] [2] [3] [4] [5] [6] [7]These changes address critical edge cases in signature processing, improve test coverage, and increase the reliability and maintainability of the codebase.