Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 26 minutes and 50 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds defensive error propagation and validation: block-body decode errors are emitted and skipped by indexers; signature decoding returns errors instead of panicking; ECIES and secp256k1 now validate public-key coordinates; gas estimation includes authorization list; many tests added for these failure modes. Changes
Sequence Diagram(s)sequenceDiagram
participant Iterator as Chain Iterator
participant Decoder as Block Body Decoder
participant Queue as Result Queue
participant Indexer as Index Processor
rect rgba(220, 53, 69, 0.5)
Note over Iterator,Indexer: Parallel iteration emits errors instead of aborting
Iterator->>Decoder: Decode block body (parallel)
alt decode succeeds
Decoder-->>Queue: emit blockTxHashes{num, txs, err=nil}
else decode fails
Decoder-->>Queue: emit blockTxHashes{num, nil, err}
end
Queue->>Indexer: dequeue item
alt item.err == nil
Indexer->>Indexer: write/delete tx index entries
else item.err != nil
Indexer->>Indexer: log warning and skip block
end
end
sequenceDiagram
participant Caller as Caller
participant ECIES as ECIES Layer
participant Validator as Point Validator
participant Crypto as Crypto Ops
rect rgba(0, 123, 255, 0.5)
Note over Caller,Crypto: ECIES public-key validation during GenerateShared/Decrypt
Caller->>ECIES: GenerateShared(pub) / Decrypt(ct)
ECIES->>Validator: ensure pub.X and pub.Y non-nil
alt coords invalid
Validator-->>ECIES: ErrInvalidPublicKey
ECIES-->>Caller: error
else coords present
Validator->>Crypto: verify point is on curve
alt off-curve
Crypto-->>ECIES: ErrInvalidPublicKey
ECIES-->>Caller: error
else on-curve
Crypto->>Crypto: perform ECDH/decrypt
Crypto-->>Caller: success
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
core/rawdb/chain_iterator_test.go (1)
210-262: Add a companion test forIndexTransactionswith a missing body.The new test only exercises the unindex path. The indexing path has equivalent new error-skip logic (chain_iterator.go lines 221-224) but is uncovered. A symmetric test would catch regressions there and document the (arguably surprising) behavior that tail advances past a block whose txs were never indexed.
🧪 Suggested sibling test
func TestIndexTransactionsMissingBody(t *testing.T) { chainDb := NewMemoryDatabase() var txs []*types.Transaction to := common.BytesToAddress([]byte{0x11}) block := types.NewBlock(&types.Header{Number: big.NewInt(0)}, nil, nil, nil, newHasher()) WriteBlock(chainDb, block) WriteCanonicalHash(chainDb, block.Hash(), block.NumberU64()) for i := uint64(1); i <= 10; i++ { tx := types.NewTx(&types.LegacyTx{ Nonce: i, GasPrice: big.NewInt(11111), Gas: 1111, To: &to, Value: big.NewInt(111), Data: []byte{0x11, 0x11, 0x11}, }) txs = append(txs, tx) block = types.NewBlock(&types.Header{Number: big.NewInt(int64(i))}, []*types.Transaction{tx}, nil, nil, newHasher()) WriteBlock(chainDb, block) WriteCanonicalHash(chainDb, block.Hash(), block.NumberU64()) } missingBlock := uint64(5) DeleteBody(chainDb, ReadCanonicalHash(chainDb, missingBlock), missingBlock) IndexTransactions(chainDb, 0, 11, nil) if tail := ReadTxIndexTail(chainDb); tail == nil || *tail != 0 { t.Fatalf("tx index tail mismatch: got %v, want 0", tail) } for i := 1; i <= 10; i++ { number := ReadTxLookupEntry(chainDb, txs[i-1].Hash()) if uint64(i) == missingBlock { if number != nil { t.Fatalf("tx for missing-body block %d should not be indexed", i) } continue } if number == nil { t.Fatalf("tx index %d missing after indexing", i) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/rawdb/chain_iterator_test.go` around lines 210 - 262, Add a symmetric test exercising IndexTransactions when a block body is missing: create TestIndexTransactionsMissingBody mirroring TestUnindexTransactionsMissingBody, build and write blocks/txs, call DeleteBody(ReadCanonicalHash(...), missingBlock) before IndexTransactions(chainDb, 0, 11, nil), then assert ReadTxIndexTail(chainDb) remains 0 and that ReadTxLookupEntry(chainDb, tx.Hash()) is nil for the missing block and present for others; this will cover the skip logic in IndexTransactions and document the tail behavior.core/types/transaction_signing_test.go (1)
140-158: Test exercises the new non-panic path.The wrapped
func(){ defer recover() … }()pattern is a bit heavy since the whole point of the change is that no panic should occur — a straight call witht.Fatalonerr == nilwould be equivalent. Not a blocker, leaving as-is is fine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/types/transaction_signing_test.go` around lines 140 - 158, The test TestSignatureValuesError currently wraps the call to tx.WithSignature in an anonymous func with a defer/recover to guard against panics; since the change ensures no panic should occur, remove the anonymous wrapper and defer/recover, call tx.WithSignature(signer, invalidSig) directly, and assert err != nil (t.Fatal when nil) and t.Logf the error; locate this behavior in TestSignatureValuesError where tx := NewTransaction(...) and signer := HomesteadSigner{} and replace the wrapped invocation of tx.WithSignature with a direct call and the existing error assertion.crypto/secp256k1/secp256_test.go (1)
220-253: Add regression cases for modulo aliases of valid points.The new checks are meant to reject coordinates outside
[0, P). Please include cases likeGx + P,Gy + P, and negative aliases such asGx - P/Gy - P; those are closer to the previous bypass because they reduce to a valid generator coordinate moduloP.Suggested test additions
func TestIsOnCurveCoordinateRange(t *testing.T) { curve := S256() @@ if curve.IsOnCurve(pPlus7, curve.Gy) { t.Fatal("x = P+7 should not be on curve") } + + xPlusP := new(big.Int).Add(curve.Gx, curve.P) + if curve.IsOnCurve(xPlusP, curve.Gy) { + t.Fatal("x = Gx+P should not be on curve") + } + + yPlusP := new(big.Int).Add(curve.Gy, curve.P) + if curve.IsOnCurve(curve.Gx, yPlusP) { + t.Fatal("y = Gy+P should not be on curve") + } + + xMinusP := new(big.Int).Sub(curve.Gx, curve.P) + if curve.IsOnCurve(xMinusP, curve.Gy) { + t.Fatal("x = Gx-P should not be on curve") + } + + yMinusP := new(big.Int).Sub(curve.Gy, curve.P) + if curve.IsOnCurve(curve.Gx, yMinusP) { + t.Fatal("y = Gy-P should not be on curve") + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypto/secp256k1/secp256_test.go` around lines 220 - 253, Extend TestIsOnCurveCoordinateRange to add regression checks that coordinates which are congruent modulo P to valid generator coordinates are rejected: construct values like new(big.Int).Add(curve.Gx, curve.P), new(big.Int).Add(curve.Gy, curve.P), new(big.Int).Sub(curve.Gx, curve.P) and new(big.Int).Sub(curve.Gy, curve.P) (and any negatives like big.NewInt(-1).Add(...)/Sub(...) as needed) and assert curve.IsOnCurve(...) returns false for each; reference the existing test harness (TestIsOnCurveCoordinateRange) and the curve fields/methods (S256(), curve.IsOnCurve, curve.Gx, curve.Gy, curve.P) when adding these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/rawdb/chain_iterator.go`:
- Around line 221-224: indexTransactions currently swallows delivery.err (when a
block body is missing) and advances lastNum / writes WriteTxIndexTail, creating
a permanent index hole; change the behavior so that when delivery.err != nil you
do NOT advance lastNum or write the tail—either return an error to abort the
indexing batch (preserving prior behavior and allowing maintainTxIndex to retry)
or record the delivery.number in a “skipped” set and skip advancing the tail
until those numbers are resolved; update the logic around WriteTxIndexTail,
indexTransactions, and any callers (e.g., maintainTxIndex and ReadTxLookupEntry
expectations) so the tail is only advanced for successfully indexed blocks and
missing/corrupt bodies trigger retry rather than silent omission.
In `@crypto/secp256k1/ext.h`:
- Around line 112-115: The code currently only decodes field elements via
secp256k1_fe_set_b32 and then constructs a group element with
secp256k1_ge_set_xy, but it does not check curve membership; add an explicit
validation call to secp256k1_ge_is_valid_var immediately after
secp256k1_ge_set_xy and return 0 (failure) if it returns false, before any use
of the point (e.g., before calling secp256k1_ecmult_const), mirroring the
pattern used in eckey_impl.h.
---
Nitpick comments:
In `@core/rawdb/chain_iterator_test.go`:
- Around line 210-262: Add a symmetric test exercising IndexTransactions when a
block body is missing: create TestIndexTransactionsMissingBody mirroring
TestUnindexTransactionsMissingBody, build and write blocks/txs, call
DeleteBody(ReadCanonicalHash(...), missingBlock) before
IndexTransactions(chainDb, 0, 11, nil), then assert ReadTxIndexTail(chainDb)
remains 0 and that ReadTxLookupEntry(chainDb, tx.Hash()) is nil for the missing
block and present for others; this will cover the skip logic in
IndexTransactions and document the tail behavior.
In `@core/types/transaction_signing_test.go`:
- Around line 140-158: The test TestSignatureValuesError currently wraps the
call to tx.WithSignature in an anonymous func with a defer/recover to guard
against panics; since the change ensures no panic should occur, remove the
anonymous wrapper and defer/recover, call tx.WithSignature(signer, invalidSig)
directly, and assert err != nil (t.Fatal when nil) and t.Logf the error; locate
this behavior in TestSignatureValuesError where tx := NewTransaction(...) and
signer := HomesteadSigner{} and replace the wrapped invocation of
tx.WithSignature with a direct call and the existing error assertion.
In `@crypto/secp256k1/secp256_test.go`:
- Around line 220-253: Extend TestIsOnCurveCoordinateRange to add regression
checks that coordinates which are congruent modulo P to valid generator
coordinates are rejected: construct values like new(big.Int).Add(curve.Gx,
curve.P), new(big.Int).Add(curve.Gy, curve.P), new(big.Int).Sub(curve.Gx,
curve.P) and new(big.Int).Sub(curve.Gy, curve.P) (and any negatives like
big.NewInt(-1).Add(...)/Sub(...) as needed) and assert curve.IsOnCurve(...)
returns false for each; reference the existing test harness
(TestIsOnCurveCoordinateRange) and the curve fields/methods (S256(),
curve.IsOnCurve, curve.Gx, curve.Gy, curve.P) when adding these assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3e2db0f-2971-4a47-91c5-7631b283fd0c
📒 Files selected for processing (16)
core/rawdb/chain_iterator.gocore/rawdb/chain_iterator_test.gocore/types/transaction.gocore/types/transaction_signing.gocore/types/transaction_signing_test.gocore/types/transaction_test.gocore/types/tx_setcode.gocrypto/ecies/ecies.gocrypto/ecies/ecies_test.gocrypto/kzg4844/kzg4844_ckzg_cgo.gocrypto/secp256k1/curve.gocrypto/secp256k1/ext.hcrypto/secp256k1/secp256_test.goeth/protocols/snap/sync.gointernal/ethapi/transaction_args.gop2p/rlpx/rlpx_oracle_poc_test.go
- Updated `TestSignatureValuesError` to use table-driven tests for better readability and maintainability. - Introduced `TestS256RejectsCoordinatesOutsideField` to validate that coordinates outside the secp256k1 curve are correctly rejected. - Modified `SigToPub` and `DecompressPubkey` functions to return public keys with the correct curve type. - Added checks in `IsOnCurve` to ensure coordinates are within valid bounds.
…ests - Introduced `TestUnindexTransactionsCorruptBody` to validate behavior when unindexing transactions with a corrupt body. - Added `TestSignSetCode` to ensure correct signing of set code transactions, verifying address, nonce, and signature values. - Enhanced existing tests for clarity and maintainability.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
core/types/transaction_signing_test.go (1)
140-165: Assert the specific error, not just non-nil.The test only checks that
WithSignaturereturns a non-nil error for invalid signature lengths. Consider asserting the concrete sentinel (e.g. the length-mismatch error returned by the updateddecodeSignature) so a regression that swaps this for an unrelated error (e.g. chain-id validation) is still caught.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/types/transaction_signing_test.go` around lines 140 - 165, The test TestSignatureValuesError should assert the concrete sentinel error from decodeSignature for invalid signature lengths instead of only checking non-nil; modify the subtests to compare the returned error from tx.WithSignature(signer, tt.sig) using errors.Is(err, ErrInvalidSignatureLength) (or the exact sentinel exported by decodeSignature) and fail the test if it does not match, referencing WithSignature, decodeSignature, and the sentinel ErrInvalidSignatureLength so regressions that return different errors are caught.crypto/crypto_test.go (1)
95-119: Good boundary coverage; consider one extra case.The test nicely pins down the new out-of-field range guards. Two small additions that would strengthen it without much cost:
- A nil-coordinate case (
curve.IsOnCurve(nil, params.Gy)and vice versa) to lock in the nil guard atcrypto/signature_nocgo.goline 166.- A coordinate pair that is inside the field but not on the curve (e.g.
(Gx, Gy+1)), to ensure the delegatedKoblitzCurve.IsOnCurvepath still rejects non-curve points after wrapping.🧪 Proposed additional assertions
pPlus7 := new(big.Int).Add(params.P, big.NewInt(7)) if curve.IsOnCurve(pPlus7, params.Gy) { t.Fatal("x = P+7 should not be on curve") } + + if curve.IsOnCurve(nil, params.Gy) || curve.IsOnCurve(params.Gx, nil) { + t.Fatal("nil coordinates should not be on curve") + } + + offCurveY := new(big.Int).Add(params.Gy, big.NewInt(1)) + if curve.IsOnCurve(params.Gx, offCurveY) { + t.Fatal("(Gx, Gy+1) should not be on curve") + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypto/crypto_test.go` around lines 95 - 119, Add two extra assertions to TestS256RejectsCoordinatesOutsideField: call curve.IsOnCurve(nil, params.Gy) and curve.IsOnCurve(params.Gx, nil) to exercise the nil-coordinate guard in the IsOnCurve implementation, and add a case for a point inside the field but not on the curve using pBadY := new(big.Int).Add(params.Gy, big.NewInt(1)) then assert curve.IsOnCurve(params.Gx, pBadY) returns false to ensure the delegated KoblitzCurve.IsOnCurve path rejects non-curve points; locate these changes in the TestS256RejectsCoordinatesOutsideField test near references to S256(), params.Gx, params.Gy, and the IsOnCurve calls.crypto/signature_nocgo.go (1)
157-176: DefensiveIsOnCurvewrapper looks correct; minor performance optimizations available.The nil/range guards correctly prevent panics and out-of-field coordinates from reaching
KoblitzCurve.IsOnCurve, and correspond to the test matrix added incrypto/crypto_test.go. A couple of small, optional improvements:
btcec.Params().Pis fetched four times per call. SincebtCurveembeds*btcec.KoblitzCurve, which itself embeds*elliptic.CurveParams, the prime is reachable ascurve.P, avoiding the package-level function call.S256()returns a newbtCurveinstance by value on every call; caching one instance in a package-level var would eliminate re-boxing on hot paths.♻️ Optional tidy-up
-// S256 returns an instance of the secp256k1 curve. -func S256() elliptic.Curve { - return btCurve{btcec.S256()} -} - -type btCurve struct { - *btcec.KoblitzCurve -} - -func (curve btCurve) IsOnCurve(x, y *big.Int) bool { - if x == nil || y == nil { - return false - } - if x.Sign() < 0 || x.Cmp(btcec.Params().P) >= 0 { - return false - } - if y.Sign() < 0 || y.Cmp(btcec.Params().P) >= 0 { - return false - } - return curve.KoblitzCurve.IsOnCurve(x, y) -} +type btCurve struct { + *btcec.KoblitzCurve +} + +var theCurve = btCurve{btcec.S256()} + +// S256 returns an instance of the secp256k1 curve. +func S256() elliptic.Curve { + return theCurve +} + +func (curve btCurve) IsOnCurve(x, y *big.Int) bool { + if x == nil || y == nil { + return false + } + p := curve.P + if x.Sign() < 0 || x.Cmp(p) >= 0 { + return false + } + if y.Sign() < 0 || y.Cmp(p) >= 0 { + return false + } + return curve.KoblitzCurve.IsOnCurve(x, y) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypto/signature_nocgo.go` around lines 157 - 176, The IsOnCurve wrapper currently calls btcec.Params().P repeatedly and re-boxes KoblitzCurve in S256; change IsOnCurve to use the embedded curve's prime via curve.P (instead of btcec.Params().P) to avoid repeated package-level calls, and make S256 return a shared package-level btCurve instance (cache a single btCurve value and return it) to avoid allocating a new btCurve on each call; refer to the btCurve type, its IsOnCurve method, and the S256 function when applying these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core/types/transaction_signing_test.go`:
- Around line 140-165: The test TestSignatureValuesError should assert the
concrete sentinel error from decodeSignature for invalid signature lengths
instead of only checking non-nil; modify the subtests to compare the returned
error from tx.WithSignature(signer, tt.sig) using errors.Is(err,
ErrInvalidSignatureLength) (or the exact sentinel exported by decodeSignature)
and fail the test if it does not match, referencing WithSignature,
decodeSignature, and the sentinel ErrInvalidSignatureLength so regressions that
return different errors are caught.
In `@crypto/crypto_test.go`:
- Around line 95-119: Add two extra assertions to
TestS256RejectsCoordinatesOutsideField: call curve.IsOnCurve(nil, params.Gy) and
curve.IsOnCurve(params.Gx, nil) to exercise the nil-coordinate guard in the
IsOnCurve implementation, and add a case for a point inside the field but not on
the curve using pBadY := new(big.Int).Add(params.Gy, big.NewInt(1)) then assert
curve.IsOnCurve(params.Gx, pBadY) returns false to ensure the delegated
KoblitzCurve.IsOnCurve path rejects non-curve points; locate these changes in
the TestS256RejectsCoordinatesOutsideField test near references to S256(),
params.Gx, params.Gy, and the IsOnCurve calls.
In `@crypto/signature_nocgo.go`:
- Around line 157-176: The IsOnCurve wrapper currently calls btcec.Params().P
repeatedly and re-boxes KoblitzCurve in S256; change IsOnCurve to use the
embedded curve's prime via curve.P (instead of btcec.Params().P) to avoid
repeated package-level calls, and make S256 return a shared package-level
btCurve instance (cache a single btCurve value and return it) to avoid
allocating a new btCurve on each call; refer to the btCurve type, its IsOnCurve
method, and the S256 function when applying these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b77f2068-75f8-4f39-8966-844b8c9a209e
📒 Files selected for processing (7)
core/rawdb/chain_iterator_test.gocore/types/transaction_signing_test.gocrypto/crypto_test.gocrypto/ecies/ecies_test.gocrypto/signature_nocgo.goeth/protocols/snap/sync_test.gointernal/ethapi/api_morph_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- crypto/ecies/ecies_test.go
- core/rawdb/chain_iterator_test.go
- Introduced `TestIndexTransactionsMissingBody` to validate the behavior of transaction indexing when a block body is missing. - Enhanced logging in `indexTransactions` to clarify handling of missing or corrupt block bodies. - Ensured that the transaction index tail does not advance past the missing block, maintaining correct indexing behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/rawdb/chain_iterator.go`:
- Around line 311-317: The code advances nextNum before checking delivery.err
which lets the tail skip past blocks whose tx lookup entries were not deleted;
move the advancement so nextNum is only updated after successful unindexing:
perform the err check first (using delivery from queue.PopItem() /
blockTxHashes), if delivery.err != nil log a clearer message that the block was
skipped and its tx lookup entries were NOT deleted (include delivery.number and
delivery.err), then continue; only after calling DeleteTxLookupEntries(batch,
delivery.hashes) update nextNum = delivery.number + 1 so the tail only advances
when deletions actually occurred.
- Around line 219-224: The loop currently does "continue" when delivery.err !=
nil which leaves lastNum unchanged and stalls the priority queue; instead abort
this indexing run so the persisted tail reflects only fully-indexed blocks: when
delivery.err != nil, flush/commit the current batch (same place you call
WriteTxIndexTail(batch, lastNum)) and return from the function (or otherwise
break out of the outer routine) rather than continue; modify the error branch
around queue.PopItem()/delivery (blockTxHashes, delivery.err, lastNum) to ensure
you persist the current batch/tail and exit so maintainTxIndex will retry the
range cleanly instead of leaving queued lower blocks unprocessed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aa8f9864-53b0-4760-bd7d-d3765a87a606
📒 Files selected for processing (3)
core/rawdb/chain_iterator.gocore/rawdb/chain_iterator_test.gocrypto/secp256k1/ext.h
✅ Files skipped from review due to trivial changes (1)
- core/rawdb/chain_iterator_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- crypto/secp256k1/ext.h
- indexTransactions: replace continue with flush+return on body error so maintainTxIndex can retry the range from the correct tail rather than silently stalling with lower blocks never indexed - unindexTransactions: improve log message to state that tx lookup entries were NOT deleted for the skipped block (structural behavior unchanged) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary by CodeRabbit
Bug Fixes
Tests