fix: return proper synthetic tx in eth_getBlockByNumber RPC
#3634
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a changelog entry for PR [3634] that specifies the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RPC_Server
participant TxParser
Client->>RPC_Server: Request block by number (eth_getBlockByNumber)
RPC_Server->>TxParser: Retrieve tx block result
TxParser->>TxParser: Select last synthetic transaction from txs.Txs
TxParser->>RPC_Server: Return block with accurate synthetic tx
RPC_Server->>Client: Deliver block with correct synthetic tx data
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
skosito
changed the title
…enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3634 +/- ##
========================================
Coverage 64.56% 64.56%
========================================
Files 469 469
Lines 32772 32772
========================================
Hits 21159 21159
Misses 10648 10648
Partials 965 965 |
skosito
changed the title
eth_getBlockByNumber RPC
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rpc/types/events.go (1)
276-277: The fix addresses the symptom, but consider investigating the root cause.The change correctly modifies the behavior to select the last transaction from the array instead of the first one, which should fix the immediate issue with the RPC returning incorrect transactions. However, the TODO comment indicates that there's still a need to understand why synthetic transaction events are in reversed order.
Consider investigating the core issue in event emission to potentially address this at the source rather than adjusting the consumption pattern.
You could add more detailed documentation about this behavior by expanding the TODO comment:
- // TODO: check why when there are multiple synthetic txs events are in reversed order + // TODO: check why when there are multiple synthetic txs events are in reversed order. + // For now, we're taking the last transaction instead of the first one to return + // the correct synthetic transaction rather than gas payment deposits.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
changelog.md(1 hunks)precompiles/bank/method_test.go(1 hunks)precompiles/staking/staking_test.go(1 hunks)rpc/types/events.go(1 hunks)x/crosschain/keeper/utils_test.go(1 hunks)x/fungible/keeper/deposits_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- precompiles/staking/staking_test.go
- x/fungible/keeper/deposits_test.go
- x/crosschain/keeper/utils_test.go
- precompiles/bank/method_test.go
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code, point out issues relative to ...
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
rpc/types/events.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: start-e2e-test / e2e
🔇 Additional comments (2)
changelog.md (1)
46-46: The changelog entry accurately reflects the fix implemented.This entry properly documents the change that addresses the issue with the
eth_getBlockByNumberRPC now returning the correct synthetic transaction instead of gas payment deposits.rpc/types/events.go (1)
276-277:❓ Verification inconclusive
Verify the fix works across different node configurations.
Since this change affects how transactions are selected from events, it's important to ensure that this behavior is consistent across different node configurations and blockchain environments.
🏁 Script executed:
#!/bin/bash # Test the behavior of eth_getBlockByNumber RPC with multiple synthetic transactions # 1. Find a recent block with multiple synthetic transactions rg --json -e "multiple synthetic txs" rpc/types/events.go | grep "TODO" # 2. Look for any test cases or implementations that might be affected fd -e go -H "eth_getBlockByNumber" --exec grep -l "ParseTxBlockResult" {} \;Length of output: 442
ACTION: Confirm Consistent Transaction Ordering Across Node Configurations
The change in
rpc/types/events.go(lines 276–277) now selects the last transaction (parsedTx := txs.Txs[len(txs.Txs)-1]) when handling multiple synthetic transactions. The shell script outputs confirm the presence of the TODO comment referencing potential reverse order issues and that no additional references (like tests aroundParseTxBlockResult) were found.
- Verify that this transaction selection logic produces consistent results on various node configurations and blockchain environments.
- Suggestion: If the affected scenario isn’t already covered by automated tests, consider adding tests to simulate different synthetic transaction orders to ensure production-grade stability.
Description
no need to emit gas payment events
it appears that eth_getBlockByNumber that blosckout use for indexing is indexing gas payment deposit instead of proper tx
2 solutions here, both can be implemented or one:
add noEthereumTxEvent to deposit zrc20, and for both usages of pay gas set true, skipping emitting these events, is there a use case for events to be here?
it seems in get block by number, order of events is reversed, so simply taking last works on these txs at least, looks like a bare minimum fix, without changing how events are emitted, so there will still be redundant data
How Has This Been Tested?
Summary by CodeRabbit