Conversation
📝 WalkthroughWalkthroughThis PR disables unprotected Ethereum transactions across all Moonbeam, Moonbase, and Moonriver runtime configurations by setting Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ 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 |
Coverage Report@@ Coverage Diff @@
## master elois/forbid-unprotected-txs +/- ##
===============================================================
Coverage 77.10% 77.10% 0.00%
Files 389 389
Lines 76943 76943
===============================================================
Hits 59323 59323
Misses 17620 17620
|
librelois
added
B7-runtimenoteworthy
WASM runtime size check:Compared to target branchMoonbase runtime: 2120 KB (no changes) ✅ Moonbeam runtime: 2240 KB (no changes) ✅ Moonriver runtime: 2240 KB (no changes) ✅ Compared to latest release (runtime-4201)Moonbase runtime: 2120 KB (+188 KB compared to latest release) Moonbeam runtime: 2240 KB (+212 KB compared to latest release) Moonriver runtime: 2240 KB (+212 KB compared to latest release) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/suites/dev/moonbase/test-eth-tx/test-test-tx-nonce.ts (1)
143-144:⚠️ Potential issue | 🟡 MinorDuplicate test
id: "T01"within suiteD011304— second test should be"T02".Lines 135 and 143 both declare
id: "T01"inside the samedescribeSuite. The commit message "fix test D011304T01" targeted this suite, but only updatedtxnType; the ID collision was left unresolved. Depending on the moonwall test runner, the duplicate ID may cause one test to be silently skipped or both results to be misreported.🐛 Proposed fix
it({ - id: "T01", + id: "T02", title: "should increment to 1",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/dev/moonbase/test-eth-tx/test-test-tx-nonce.ts` around lines 143 - 144, Within the describeSuite for D011304 there are two tests using the same id value "T01"; locate the second it({ id: "T01", ... }) inside that describeSuite and change its id to "T02" so test IDs are unique (ensure the entry inside the it call that sets id is updated; e.g., modify the second occurrence of id: "T01" to id: "T02" in the test block that also updated txnType).runtime/moonriver/src/lib.rs (1)
217-226:⚠️ Potential issue | 🟡 MinorThe
spec_versionwas incremented for this breaking change, but not in the same PR.
AllowUnprotectedTxs = falseis a breaking behavioral change (PR#3677, Feb 20), rejecting legacy Ethereum transactions that were previously valid. Thespec_versionwas incremented from 4200 to 4300, but this occurred in a separate PR (#3657, Feb 5) for unrelated runtime changes (frontier upgrades, EIP support, XCM fixes). The AllowUnprotectedTxs change itself did not trigger a spec_version bump. While the final state correctly reflects version 4300, best practice would align the spec_version increment with the PR introducing this breaking behavioral change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/moonriver/src/lib.rs` around lines 217 - 226, The spec_version bump must be applied in the same PR that introduced the breaking change AllowUnprotectedTxs (PR `#3677`); update the RuntimeVersion constant named VERSION by incrementing the spec_version field (RuntimeVersion::spec_version) so that the PR that introduced AllowUnprotectedTxs contains the version change (e.g., bump 4300 -> 4301) and include that updated VERSION in the same commit that toggles AllowUnprotectedTxs to ensure the breaking change and version bump are co-located.
🧹 Nitpick comments (5)
test/suites/dev/moonbase/test-precompile/test-precompile-xtokens.ts (1)
254-255: Nit: inline fee style diverges from the other three tests in the same file.T01, T03, and T05 all use an intermediate
gasPricevariable before computingfees. Keeping a consistent style makes the pattern easier to scan.♻️ Optional style alignment
- const fees = receipt.gasUsed * receipt.effectiveGasPrice; + const gasPrice = receipt.effectiveGasPrice; + const fees = receipt.gasUsed * gasPrice;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/dev/moonbase/test-precompile/test-precompile-xtokens.ts` around lines 254 - 255, The assertion computes fees inline; to match T01/T03/T05, introduce an intermediate gasPrice (or effectiveGasPrice) variable from receipt.effectiveGasPrice before computing fees and then use fees = receipt.gasUsed * gasPrice, so update the lines around the receipt usage (the `fees` calculation and subsequent expect) to use the intermediate `gasPrice` variable for consistent style with the other tests.test/suites/dev/moonbase/test-eth-tx/test-test-tx-nonce.ts (1)
151-165:txnType: "eip1559"change is correct; consider preserving receipt assertion.The switch from
"legacy"to"eip1559"correctly aligns the test with the newAllowUnprotectedTxs = falseruntime policy.The AI summary notes that the original code also validated the transaction receipt and asserted the block contained exactly one transaction. Dropping those checks means a silent EVM revert (e.g., out-of-gas on the
incr()call withgasLimit: 21000) would still satisfy the nonce assertion, masking a real failure.♻️ Suggested improvement: re-add receipt check
- await context.createBlock( + const { result } = await context.createBlock( context.createTxn!({ privateKey: BALTATHAR_PRIVATE_KEY, to: incrementorAddress, data, value: 0n, gasLimit: 21000, txnType: "eip1559", }) ); + expect(result?.successful, "transaction should succeed").toBe(true); expect(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/dev/moonbase/test-eth-tx/test-test-tx-nonce.ts` around lines 151 - 165, The nonce assertion was kept but the test dropped the transaction receipt and block-transaction-count checks, which can mask failures; update the test around context.createBlock(context.createTxn!({...})) to also fetch and assert the transaction receipt (from the returned tx hash or block) and assert the created block contains exactly one transaction (use the block/receipt from context.createBlock or context.viem().getTransactionReceipt and context.viem().getBlock) before checking getTransactionCount for BALTATHAR_ADDRESS, ensuring the incr() call actually executed (refer to context.createBlock, context.createTxn, BALTATHAR_PRIVATE_KEY, incrementorAddress, and getTransactionCount).runtime/moonbase/src/lib.rs (1)
694-697: Core change LGTM — ensurespec_version(andtransaction_version) is bumped before on-chain deployment.Rejecting previously-valid unprotected transactions is a consensus-breaking change. Without a
spec_versionincrement, tooling cannot detect the semantic boundary, and any in-flight unprotected transactions in the mempool at upgrade time will be silently dropped rather than clearly rejected. Consider also bumpingtransaction_versionto signal the change to signing clients. This applies equally to the moonbeam and moonriver runtimes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/moonbase/src/lib.rs` around lines 694 - 697, Bump the runtime `spec_version` (and also `transaction_version`) in the runtime metadata before on-chain deployment to signal the consensus change caused by disallowing unprotected transactions; update the constants where `spec_version` and `transaction_version` are defined (e.g., the runtime version struct or constants that return RuntimeVersion) so tooling and signing clients detect the breaking change and in-flight mempool unprotected transactions are rejected rather than silently dropped; apply the same change in both moonbeam and moonriver runtime modules.test/suites/dev/moonbase/test-balance/test-balance-transfer.ts (1)
94-96: Weakened fee assertion in T03 — consider asserting the exact deduction.With
maxPriorityFeePerGas: 0nandmaxFeePerGas: GENESIS_BASE_FEE, the effective gas price equalsGENESIS_BASE_FEE(no tip above base fee), so the total deduction is deterministic.toBeGreaterThan(512n)only proves the transfer occurred; it doesn't validate fee accounting. T08 uses an exact fee assertion for the same EIP-1559 configuration.♻️ Suggested fix
- expect(balanceBefore - balanceAfter).toBeGreaterThan(512n); + expect(balanceBefore - balanceAfter).toBe(512n + 21000n * GENESIS_BASE_FEE);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/dev/moonbase/test-balance/test-balance-transfer.ts` around lines 94 - 96, The test currently only checks balanceBefore - balanceAfter > 512n, which is too weak; instead compute the exact expected deduction using the transaction's value and gas used times GENESIS_BASE_FEE and assert equality. After sending the transfer, fetch the receipt (e.g., via context.viem().getTransactionReceipt or from the send call), read receipt.gasUsed, compute expectedDeduction = transferValue + receipt.gasUsed * GENESIS_BASE_FEE (use the same transfer amount used in the test), then replace the loose expect with expect(balanceBefore - balanceAfter).toEqual(expectedDeduction) referencing balanceBefore, balanceAfter, CHARLETH_ADDRESS, GENESIS_BASE_FEE and the receipt.gasUsed value.runtime/moonriver/src/lib.rs (1)
695-698:AllowUnprotectedTxs = false— correct and aligns with PR intent.This prevents legacy (pre-EIP-155, no chain ID) Ethereum transactions from passing
validate_self_contained. XCM-originated Ethereum transactions (dispatched viapallet_ethereum_xcm) go throughValidatedTransaction::applydirectly and are unaffected by this flag, so XCM flows remain intact.Optionally, since other boolean configs in this file use
ConstBool<N>directly (e.g.ConstBool<true>inpallet_async_backing::Config), theparameter_types!indirection could be replaced:♻️ Optional: use
ConstBooldirectly for consistency-parameter_types! { - pub const PostBlockAndTxnHashes: PostLogContent = PostLogContent::BlockAndTxnHashes; - pub const AllowUnprotectedTxs: bool = false; -} +parameter_types! { + pub const PostBlockAndTxnHashes: PostLogContent = PostLogContent::BlockAndTxnHashes; +} impl pallet_ethereum::Config for Runtime { type StateRoot = ...; type PostLogContent = PostBlockAndTxnHashes; type ExtraDataLength = ConstU32<30>; - type AllowUnprotectedTxs = AllowUnprotectedTxs; + type AllowUnprotectedTxs = ConstBool<false>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/moonriver/src/lib.rs` around lines 695 - 698, The current parameter_types! block defines AllowUnprotectedTxs as a bool constant set to false; to match the other boolean config patterns in this file (e.g. the ConstBool<true> used in pallet_async_backing::Config) and remove the indirection, replace usages of the AllowUnprotectedTxs parameter with the direct ConstBool<false> type and remove the AllowUnprotectedTxs parameter from the parameter_types! block (keeping PostBlockAndTxnHashes as-is); search for the identifier AllowUnprotectedTxs in the crate and update the bound to ConstBool<false> so the config types are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@runtime/moonriver/src/lib.rs`:
- Around line 217-226: The spec_version bump must be applied in the same PR that
introduced the breaking change AllowUnprotectedTxs (PR `#3677`); update the
RuntimeVersion constant named VERSION by incrementing the spec_version field
(RuntimeVersion::spec_version) so that the PR that introduced
AllowUnprotectedTxs contains the version change (e.g., bump 4300 -> 4301) and
include that updated VERSION in the same commit that toggles AllowUnprotectedTxs
to ensure the breaking change and version bump are co-located.
In `@test/suites/dev/moonbase/test-eth-tx/test-test-tx-nonce.ts`:
- Around line 143-144: Within the describeSuite for D011304 there are two tests
using the same id value "T01"; locate the second it({ id: "T01", ... }) inside
that describeSuite and change its id to "T02" so test IDs are unique (ensure the
entry inside the it call that sets id is updated; e.g., modify the second
occurrence of id: "T01" to id: "T02" in the test block that also updated
txnType).
---
Nitpick comments:
In `@runtime/moonbase/src/lib.rs`:
- Around line 694-697: Bump the runtime `spec_version` (and also
`transaction_version`) in the runtime metadata before on-chain deployment to
signal the consensus change caused by disallowing unprotected transactions;
update the constants where `spec_version` and `transaction_version` are defined
(e.g., the runtime version struct or constants that return RuntimeVersion) so
tooling and signing clients detect the breaking change and in-flight mempool
unprotected transactions are rejected rather than silently dropped; apply the
same change in both moonbeam and moonriver runtime modules.
In `@runtime/moonriver/src/lib.rs`:
- Around line 695-698: The current parameter_types! block defines
AllowUnprotectedTxs as a bool constant set to false; to match the other boolean
config patterns in this file (e.g. the ConstBool<true> used in
pallet_async_backing::Config) and remove the indirection, replace usages of the
AllowUnprotectedTxs parameter with the direct ConstBool<false> type and remove
the AllowUnprotectedTxs parameter from the parameter_types! block (keeping
PostBlockAndTxnHashes as-is); search for the identifier AllowUnprotectedTxs in
the crate and update the bound to ConstBool<false> so the config types are
consistent.
In `@test/suites/dev/moonbase/test-balance/test-balance-transfer.ts`:
- Around line 94-96: The test currently only checks balanceBefore - balanceAfter
> 512n, which is too weak; instead compute the exact expected deduction using
the transaction's value and gas used times GENESIS_BASE_FEE and assert equality.
After sending the transfer, fetch the receipt (e.g., via
context.viem().getTransactionReceipt or from the send call), read
receipt.gasUsed, compute expectedDeduction = transferValue + receipt.gasUsed *
GENESIS_BASE_FEE (use the same transfer amount used in the test), then replace
the loose expect with expect(balanceBefore -
balanceAfter).toEqual(expectedDeduction) referencing balanceBefore,
balanceAfter, CHARLETH_ADDRESS, GENESIS_BASE_FEE and the receipt.gasUsed value.
In `@test/suites/dev/moonbase/test-eth-tx/test-test-tx-nonce.ts`:
- Around line 151-165: The nonce assertion was kept but the test dropped the
transaction receipt and block-transaction-count checks, which can mask failures;
update the test around context.createBlock(context.createTxn!({...})) to also
fetch and assert the transaction receipt (from the returned tx hash or block)
and assert the created block contains exactly one transaction (use the
block/receipt from context.createBlock or context.viem().getTransactionReceipt
and context.viem().getBlock) before checking getTransactionCount for
BALTATHAR_ADDRESS, ensuring the incr() call actually executed (refer to
context.createBlock, context.createTxn, BALTATHAR_PRIVATE_KEY,
incrementorAddress, and getTransactionCount).
In `@test/suites/dev/moonbase/test-precompile/test-precompile-xtokens.ts`:
- Around line 254-255: The assertion computes fees inline; to match T01/T03/T05,
introduce an intermediate gasPrice (or effectiveGasPrice) variable from
receipt.effectiveGasPrice before computing fees and then use fees =
receipt.gasUsed * gasPrice, so update the lines around the receipt usage (the
`fees` calculation and subsequent expect) to use the intermediate `gasPrice`
variable for consistent style with the other tests.
* forbid unprotected txs * Tests must not use legacy txs without chain id * fix test D011304T01
1 participant
Ethereum transactions without chain ID are now rejected
What does it do?
Forbid ethereum transactions without chain ID
What important points should reviewers know?
Is there something left for follow-up PRs?
What alternative implementations were considered?
Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?
What value does it bring to the blockchain users?