fix(jsonrpc): post log/block filters for reorg-applied blocks#13
Closed
0xbigapple wants to merge 3 commits into
Closed
fix(jsonrpc): post log/block filters for reorg-applied blocks#130xbigapple wants to merge 3 commits into
0xbigapple wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughManager's fork-switch exception recovery now re-enqueues block and logs filters for the re-applied branch via a new ChangesFork-switch filter re-enqueuing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
framework/src/main/java/org/tron/core/db/Manager.java (1)
2288-2298: ⚡ Quick winExtract the FULL-node filter posting into a shared helper.
This replays the same
postBlockFilter(...)/postLogsFilter(..., false, false)pair already embedded inblockTrigger(...)atframework/src/main/java/org/tron/core/db/Manager.java:1423-1426. Keeping two copies of that contract is how the reorg path drifted in the first place; a small shared helper would keep the normal-extend and switch-fork paths aligned.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/main/java/org/tron/core/db/Manager.java` around lines 2288 - 2298, Extract the duplicate FULL-node filter posting into a new private helper (e.g., postFullNodeFilters or postFullStreamFilters) that accepts a BlockCapsule and encapsulates the two calls postBlockFilter(blockCapsule, false) and postLogsFilter(blockCapsule, false, false); then replace the inline pairs in both reApplyLogsFilter(List<KhaosBlock> newBranch) and the blockTrigger(...) path with a call to this new helper (preserving the surrounding CommonParameter.getInstance().isJsonRpcHttpFullNodeEnable() check and any boolean context).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/src/test/java/org/tron/core/db/ManagerTest.java`:
- Around line 1551-1624: The test switchForkShouldPostFullNodeFilterForNewBranch
sets CommonParameter.getInstance().jsonRpcHttpFullNodeEnable = true but never
restores it; wrap the test body in try/finally (or capture the original boolean
before changing) and in the finally block reset
CommonParameter.getInstance().jsonRpcHttpFullNodeEnable back to its original
value so subsequent tests are unaffected; ensure the finally executes even on
exceptions so the flag is always restored.
---
Nitpick comments:
In `@framework/src/main/java/org/tron/core/db/Manager.java`:
- Around line 2288-2298: Extract the duplicate FULL-node filter posting into a
new private helper (e.g., postFullNodeFilters or postFullStreamFilters) that
accepts a BlockCapsule and encapsulates the two calls
postBlockFilter(blockCapsule, false) and postLogsFilter(blockCapsule, false,
false); then replace the inline pairs in both reApplyLogsFilter(List<KhaosBlock>
newBranch) and the blockTrigger(...) path with a call to this new helper
(preserving the surrounding
CommonParameter.getInstance().isJsonRpcHttpFullNodeEnable() check and any
boolean context).
🪄 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: a2ca6477-a955-4965-b10f-90f3ea5d02cf
📒 Files selected for processing (2)
framework/src/main/java/org/tron/core/db/Manager.javaframework/src/test/java/org/tron/core/db/ManagerTest.java
Comment on lines
+1551
to
+1624
| public void switchForkShouldPostFullNodeFilterForNewBranch() throws Exception { | ||
| CommonParameter.getInstance().jsonRpcHttpFullNodeEnable = true; | ||
| // filterProcessLoop only starts when isJsonRpcFilterEnabled() held at Manager.init() time; it | ||
| // was false then, so filterCapsuleQueue is produce-only here and fully observable. | ||
|
|
||
| // bootstrap a head with a known witness | ||
| String key = PublicMethod.getRandomPrivateKey(); | ||
| byte[] privateKey = ByteArray.fromHexString(key); | ||
| final ECKey ecKey = ECKey.fromPrivate(privateKey); | ||
| byte[] address = ecKey.getAddress(); | ||
| ByteString addressByte = ByteString.copyFrom(address); | ||
| chainManager.getAccountStore().put(addressByte.toByteArray(), | ||
| new AccountCapsule(Protocol.Account.newBuilder().setAddress(addressByte).build())); | ||
| WitnessCapsule witnessCapsule = new WitnessCapsule(addressByte); | ||
| chainManager.getWitnessScheduleStore().saveActiveWitnesses(new ArrayList<>()); | ||
| chainManager.addWitness(addressByte); | ||
| chainManager.getWitnessStore().put(address, witnessCapsule); | ||
| Block block = blockGenerate.getSignedBlock( | ||
| witnessCapsule.getAddress(), 1533529947843L, privateKey); | ||
| dbManager.pushBlock(new BlockCapsule(block)); | ||
|
|
||
| Map<ByteString, String> keys = addTestWitnessAndAccount(); | ||
| keys.put(addressByte, key); | ||
|
|
||
| // fund an owner; transfers go owner -> witness 'address' (an existing account) | ||
| ECKey ownerKey = new ECKey(Utils.getRandom()); | ||
| byte[] owner = ownerKey.getAddress(); | ||
| AccountCapsule ownerAccount = new AccountCapsule( | ||
| Protocol.Account.newBuilder().setAddress(ByteString.copyFrom(owner)).build()); | ||
| ownerAccount.setBalance(1_000_000_000L); | ||
| chainManager.getAccountStore().put(owner, ownerAccount); | ||
|
|
||
| long t = 1533529947843L; | ||
| long base = chainManager.getDynamicPropertiesStore().getLatestBlockHeaderNumber(); | ||
|
|
||
| // common ancestor P (empty) — fork point and tapos reference | ||
| BlockCapsule p = createTestBlockCapsule(t + 3000, base + 1, | ||
| chainManager.getDynamicPropertiesStore().getLatestBlockHeaderHash().getByteString(), keys); | ||
| dbManager.pushBlock(p); | ||
|
|
||
| long expiration = t + 1_000_000L; | ||
| BlockingQueue<FilterTriggerCapsule> queue = | ||
| ReflectUtils.getFieldValue(dbManager, "filterCapsuleQueue"); | ||
| queue.clear(); | ||
|
|
||
| // old branch: A carries a transfer; applied via the normal extend path | ||
| BlockCapsule a = blockWithTransfer(t + 6000, base + 2, p.getBlockId().getByteString(), keys, | ||
| transfer(owner, address, 1L, p, expiration)); | ||
| dbManager.pushBlock(a); | ||
| Assert.assertEquals("control: head should be A after normal extend", | ||
| a.getBlockId(), chainManager.getDynamicPropertiesStore().getLatestBlockHeaderHash()); | ||
| Assert.assertTrue("control: normal-path block A's logs must reach FULL stream (added)", | ||
| hasFullLogsFilter(queue, a, false)); | ||
|
|
||
| // heavier competing branch P -> B1 -> B2, each carrying a transfer, to force switchFork | ||
| BlockCapsule b1 = blockWithTransfer(t + 6001, base + 2, p.getBlockId().getByteString(), keys, | ||
| transfer(owner, address, 2L, p, expiration)); | ||
| dbManager.pushBlock(b1); // num <= head -> kept in khaosDb, no switch yet | ||
| BlockCapsule b2 = blockWithTransfer(t + 9000, base + 3, b1.getBlockId().getByteString(), keys, | ||
| transfer(owner, address, 3L, p, expiration)); | ||
| dbManager.pushBlock(b2); // num > head & parent != head -> triggers switchFork | ||
|
|
||
| Assert.assertEquals("reorg must switch the canonical head to the competing branch (B2)", | ||
| b2.getBlockId(), chainManager.getDynamicPropertiesStore().getLatestBlockHeaderHash()); | ||
|
|
||
| // reorg withdraws the orphaned old-branch logs (removed=true) | ||
| Assert.assertTrue("reorg: orphaned block A's logs must be withdrawn (removed=true)", | ||
| hasFullLogsFilter(queue, a, true)); | ||
| // the fix: new canonical blocks' logs are delivered (added, i.e. removed=false) | ||
| Assert.assertTrue("reorg: new canonical block B1's logs must reach FULL stream (added)", | ||
| hasFullLogsFilter(queue, b1, false)); | ||
| Assert.assertTrue("reorg: new canonical block B2's logs must reach FULL stream (added)", | ||
| hasFullLogsFilter(queue, b2, false)); | ||
| } |
There was a problem hiding this comment.
Restore the FULL-node flag in a finally block.
This test mutates the global CommonParameter singleton and never puts it back. If another test runs afterward in the same JVM, it can start seeing extra FULL-node filter postings and fail nondeterministically.
Suggested fix
`@Test`
public void switchForkShouldPostFullNodeFilterForNewBranch() throws Exception {
- CommonParameter.getInstance().jsonRpcHttpFullNodeEnable = true;
+ boolean originalFullNodeEnable = CommonParameter.getInstance().jsonRpcHttpFullNodeEnable;
+ CommonParameter.getInstance().jsonRpcHttpFullNodeEnable = true;
// filterProcessLoop only starts when isJsonRpcFilterEnabled() held at Manager.init() time; it
// was false then, so filterCapsuleQueue is produce-only here and fully observable.
+ BlockingQueue<FilterTriggerCapsule> queue =
+ ReflectUtils.getFieldValue(dbManager, "filterCapsuleQueue");
+ try {
- // bootstrap a head with a known witness
- String key = PublicMethod.getRandomPrivateKey();
- byte[] privateKey = ByteArray.fromHexString(key);
- final ECKey ecKey = ECKey.fromPrivate(privateKey);
- byte[] address = ecKey.getAddress();
- ByteString addressByte = ByteString.copyFrom(address);
- chainManager.getAccountStore().put(addressByte.toByteArray(),
- new AccountCapsule(Protocol.Account.newBuilder().setAddress(addressByte).build()));
- WitnessCapsule witnessCapsule = new WitnessCapsule(addressByte);
- chainManager.getWitnessScheduleStore().saveActiveWitnesses(new ArrayList<>());
- chainManager.addWitness(addressByte);
- chainManager.getWitnessStore().put(address, witnessCapsule);
- Block block = blockGenerate.getSignedBlock(
- witnessCapsule.getAddress(), 1533529947843L, privateKey);
- dbManager.pushBlock(new BlockCapsule(block));
+ // existing test body
+ } finally {
+ CommonParameter.getInstance().jsonRpcHttpFullNodeEnable = originalFullNodeEnable;
+ queue.clear();
+ }
- ...
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/src/test/java/org/tron/core/db/ManagerTest.java` around lines 1551
- 1624, The test switchForkShouldPostFullNodeFilterForNewBranch sets
CommonParameter.getInstance().jsonRpcHttpFullNodeEnable = true but never
restores it; wrap the test body in try/finally (or capture the original boolean
before changing) and in the finally block reset
CommonParameter.getInstance().jsonRpcHttpFullNodeEnable back to its original
value so subsequent tests are unaffected; ensure the finally executes even on
exceptions so the flag is always restored.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
0xbigapple
changed the title
…er elements
- Restore topicToByteArray for LogFilter topics, guard with (0x)?[0-9a-fA-F]{63,64}$ so the stripped zero is padded back by ByteArray.fromHexString, while non-hex or wrong-length input still gets a clean -32602.
- LogFilter: validate element types before the (String) cast in the address array and nested topic array loops.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Adds
reApplyLogsFilter(List<KhaosBlock>), invoked afterswitchForkhas fully applied thenew branch (oldest-first), mirroring
blockTrigger's jsonrpc FULL handling so reorg'd blocksreach the FULL stream like the normal extend path.
Why are these changes required?
On a reorg,
pushBlockcallsswitchFork()and returns early, never reachingblockTrigger()— the only place that posts the "added" FULL filters (postBlockFilter/postLogsFilter(block, false, false)). The rollback side withdraws the orphaned branch'slogs (
reOrgLogsFilter,removed=true), but nothing re-posts the new branch's logs:withdraw old, never add new, violating reorg semantics.
So dApps using
eth_newFilter+eth_getFilterChangesmiss every log in blocks applied viaswitchFork.eth_getLogs(canonical-chain snapshot) is unaffected, which is why the gap iseasy to overlook.
This PR has been tested by:
Follow up
(switch-back) path are dropped on the same early-return; out of scope, known limitations.
Extra details
removed=trueapplies to the log filter (eth_newFilter) only. The block filter(
eth_newBlockFilter) is a block-hash stream with noremovedconcept — per Ethereumsemantics it is not withdrawn on a reorg, so the rollback side posts no block filter and
only the new branch gets an added block filter.
postBlockFilter+postLogsFilter).Summary by cubic
Fixes missed FULL-node JSON-RPC filters after reorgs and hardens
eth_newFiltertopic/address parsing. After fork switches, we now emit block and log filters for the new canonical branch.reApplyLogsFilter(List<KhaosBlock>)afterswitchForkto post FULL block/log filters for the new branch (oldest-first); includes a reorg test that withdraws orphaned logs and delivers new ones with correctremovedflags. Documented the method and call site for clarity.-32602.Written for commit b2e3f7e. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests