feat(toolkit): exclude historical balance DBs from lite snapshot#121
feat(toolkit): exclude historical balance DBs from lite snapshot#121halibobo1205 wants to merge 2 commits intodevelopfrom
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
codeant-ai
bot
added
the
size:M
|
CodeAnt AI finished reviewing your PR. |
halibobo1205
force-pushed
the
feat/toolkit_db_lite_opt
branch
from
0f1467c to
eaba17c
Compare
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
size:XL
|
CodeAnt AI Incremental review completed. |
|
@CodeAnt-AI: review |
|
CodeAnt AI is running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
size:XL
Sequence DiagramThis PR updates the DbLite toolkit so that lite snapshot history trimming also removes balance and account trace data for trimmed blocks, while merges from backup restore account trace history from the start height to keep historical balance lookups working. sequenceDiagram
participant Operator
participant DbLite toolkit
participant Block and transaction DBs
participant Balance trace DB
participant Account trace DB
Operator->>DbLite toolkit: Run lite split and trim history after snapshot
DbLite toolkit->>Block and transaction DBs: Identify and remove blocks beyond snapshot range
DbLite toolkit->>Balance trace DB: Load balance traces for trimmed blocks
DbLite toolkit->>Account trace DB: Delete account trace entries for affected addresses
DbLite toolkit->>Balance trace DB: Delete balance trace entries for trimmed blocks
Operator->>DbLite toolkit: Merge backup history into lite database
DbLite toolkit->>Block and transaction DBs: Copy archived block and transaction data
DbLite toolkit->>Account trace DB: Copy account trace entries from history start
Generated by CodeAnt AI |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
|
|
||
| @Test | ||
| public void testToolsWithTrimHistory() throws InterruptedException, IOException { | ||
| testTools("ROCKSDB", 1, true, true); |
There was a problem hiding this comment.
Suggestion: This test is named to validate trim-history behavior, but passing true for the third argument drives the merge-back path (snapshotMaxNum > historyMaxNum) instead of the trim path (trimExtraHistory). As written, it does not cover the intended scenario and can miss regressions in history trimming with balance lookup enabled. Pass false so the test actually exercises trim-history logic. [logic error]
Severity Level: Major ⚠️
- ⚠️ DbLite CLI history-trim with balance lookup untested.
- ⚠️ Lite snapshot rebuild regressions may bypass CI detection.| testTools("ROCKSDB", 1, true, true); | |
| testTools("ROCKSDB", 1, false, true); |
Steps of Reproduction ✅
1. Open `plugins/src/test/java/org/tron/plugins/rocksdb/DbLiteWithHistoryRocksDbTest.java`
and see `testToolsWithTrimHistory()` at lines 9–12 calling `testTools("ROCKSDB", 1, true,
true);` (Read output confirms this exact call).
2. Open `plugins/src/test/java/org/tron/plugins/DbLiteTest.java` and inspect the Javadoc
for the 4‑arg overload at lines 96–101 (Grep output): it states `advanceSnapshot=true`
makes `snapshotMaxNum > historyMaxNum` and "exercises mergeBak2Database", while
`advanceSnapshot=false` makes `historyMaxNum > snapshotMaxNum` and "exercises
trimExtraHistory".
3. In the same file, locate the 4‑arg method `testTools(String dbType, int
checkpointVersion, boolean advanceSnapshot, boolean historyBalanceLookup)` at line 102
(Grep) and its log statement at line 104; when
`DbLiteWithHistoryRocksDbTest.testToolsWithTrimHistory()` runs, it always invokes this
overload with `advanceSnapshot=true, historyBalanceLookup=true`, so the executed scenario
is the merge path, not the trimExtraHistory path described in the Javadoc.
4. Check other callers via Grep: `DbLiteRocksDbTest` and the LevelDB tests only call the
2‑arg `testTools(dbType, checkpointVersion)` (lines 90–93), which hard‑codes
`advanceSnapshot=false, historyBalanceLookup=false`; thus no test ever invokes `testTools`
with `advanceSnapshot=false` and `historyBalanceLookup=true`. Therefore, despite its name
(`testToolsWithTrimHistory`), the new RocksDB test never exercises the "trim history with
balance lookup enabled" scenario, and history‑trimming logic under
`historyBalanceLookup=true` remains untested.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** plugins/src/test/java/org/tron/plugins/rocksdb/DbLiteWithHistoryRocksDbTest.java
**Line:** 11:11
**Comment:**
*Logic Error: This test is named to validate trim-history behavior, but passing `true` for the third argument drives the merge-back path (`snapshotMaxNum > historyMaxNum`) instead of the trim path (`trimExtraHistory`). As written, it does not cover the intended scenario and can miss regressions in history trimming with balance lookup enabled. Pass `false` so the test actually exercises trim-history logic.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.There was a problem hiding this comment.
Valid point. advanceSnapshot=true exercises the merge path, not trim — the test name is misleading. Changed to testTools("ROCKSDB", 1, false, true) so it actually covers the v1 trim + historyBalance scenario. Fixed in the next commit.
| @@ -137,11 +152,13 @@ public void testTools(String dbType, int checkpointVersion) | |||
| String.format("rename snapshot to %s failed", | |||
| Paths.get(dbPath, databaseDir))); | |||
| } | |||
| // start and validate the snapshot | |||
| startApp(); | |||
| generateSomeTransactions(checkpointVersion == 1 ? 18 : 6); | |||
| // stop the node | |||
| shutdown(); | |||
| if (advanceSnapshot) { | |||
| // start and validate the snapshot, producing blocks beyond history | |||
| startApp(); | |||
| generateSomeTransactions(checkpointVersion == 1 ? 18 : 6); | |||
There was a problem hiding this comment.
Suggestion: The block-generation durations for advanceSnapshot=true are inverted for checkpoint version 2, which makes history grow more than snapshot and violates the method's own contract (snapshotMaxNum > historyMaxNum). This causes the test to exercise the wrong merge path and can miss regressions in the intended branch. Use the same "history shorter, snapshot longer" timing strategy regardless of checkpoint version. [logic error]
Severity Level: Major ⚠️
- ⚠️ RocksDB checkpoint v2 mergeBak2Database path never exercised.
- ⚠️ trimExtraHistory branch redundantly tested for checkpoint version two.
- ⚠️ Checkpoint v2 merge regressions may pass unnoticed in tests.| generateSomeTransactions(advanceSnapshot ? 6 : 18); | |
| generateSomeTransactions(18); |
Steps of Reproduction ✅
1. Run the unit test `testToolsWithRocksDB` in
`plugins/src/test/java/org/tron/plugins/rocksdb/DbLiteRocksDbV2Test.java:9-12`, which
calls `testTools("ROCKSDB", 2, true, false)` on `DbLiteTest`.
2. Inside `DbLiteTest.testTools`
(`plugins/src/test/java/org/tron/plugins/DbLiteTest.java:102-169`), after the initial
18-second block generation and snapshot split, the line at 136 executes
`generateSomeTransactions(advanceSnapshot ? (checkpointVersion == 1 ? 6 : 18) : 18);`
which, for `advanceSnapshot=true` and `checkpointVersion=2`, generates 18 seconds of
additional blocks before the history split (making `historyMaxNum` significantly larger).
3. Later in the same method, after renaming `snapshot` to the live database directory, the
`if (advanceSnapshot)` block at lines 36–41 (diff line 158 in the hunk) runs
`generateSomeTransactions(checkpointVersion == 1 ? 18 : 6);`, which for
`checkpointVersion=2` generates only 6 seconds of blocks on the snapshot, so at merge time
`snapshotMaxNum < historyMaxNum` contrary to the method's Javadoc contract for
`advanceSnapshot=true`.
4. When `cli.execute(argsForMerge)` is called in `DbLiteTest.testTools`
(`plugins/src/test/java/common/org/tron/plugins/DbLite.java:460-519`),
`checkAndGetBlockNumInfo` computes a `BlockNumInfo` with `snapshotMaxNum < historyMaxNum`,
causing `trimExtraHistory` (`DbLite.java:55-52, lines 520-599`) to execute and
`mergeBak2Database` (`DbLite.java:54-64, lines 573-599`) to early-return with "Ignore
merging the bak data"; thus `DbLiteRocksDbV2Test` unintentionally exercises the trim path
again instead of the intended `mergeBak2Database` path for checkpoint version 2.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** plugins/src/test/java/org/tron/plugins/DbLiteTest.java
**Line:** 136:158
**Comment:**
*Logic Error: The block-generation durations for `advanceSnapshot=true` are inverted for checkpoint version 2, which makes history grow more than snapshot and violates the method's own contract (`snapshotMaxNum > historyMaxNum`). This causes the test to exercise the wrong merge path and can miss regressions in the intended branch. Use the same "history shorter, snapshot longer" timing strategy regardless of checkpoint version.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.There was a problem hiding this comment.
This is a pre-existing behavior, not introduced by this PR. The original code already had checkpointVersion == 1 ? 6 : 18 / checkpointVersion == 1 ? 18 : 6 timing. This PR only wrapped it in the advanceSnapshot conditional without changing the logic. Can be addressed separately if needed.
| DBInterface balanceTraceDb = DbTool.getDB(liteDir, BALANCE_TRACE_DB_NAME); | ||
| DBInterface accountTraceDb = DbTool.getDB(liteDir, ACCOUNT_TRACE_DB_NAME); | ||
|
|
||
| ProgressBar.wrap(LongStream.rangeClosed(start, end) | ||
| .boxed() | ||
| .sorted((a, b) -> Long.compare(b, a)), "trimHistory").forEach(n -> { | ||
| .sorted((a, b) -> Long.compare(b, a)), "trimHistory") | ||
| .map(ByteArray::fromLong).forEach(n -> { | ||
| try { | ||
| byte[] blockIdHash = blockIndexDb.get(ByteArray.fromLong(n)); | ||
| byte[] blockIdHash = blockIndexDb.get(n); | ||
| Protocol.Block block = Protocol.Block.parseFrom(blockDb.get(blockIdHash)); | ||
| // delete transactions | ||
| for (Protocol.Transaction e : block.getTransactionsList()) { | ||
| transDb.delete(DBUtils.getTransactionId(e).getBytes()); |
There was a problem hiding this comment.
Suggestion: During history trimming, transaction IDs are removed from trans and transactionRetStore, but transactionHistoryStore entries for the same trimmed blocks are left behind. This leaves stale transaction info queryable by transaction ID after the corresponding blocks are deleted, so delete transaction-history entries for each removed block transaction as well. [logic error]
Severity Level: Major ⚠️
- ❌ getTransactionInfoById returns info for trimmed-block transactions.
- ⚠️ Lite node exposes inconsistent transaction and block history.
- ⚠️ History-based tooling may misinterpret node's canonical chain.| DBInterface balanceTraceDb = DbTool.getDB(liteDir, BALANCE_TRACE_DB_NAME); | |
| DBInterface accountTraceDb = DbTool.getDB(liteDir, ACCOUNT_TRACE_DB_NAME); | |
| ProgressBar.wrap(LongStream.rangeClosed(start, end) | |
| .boxed() | |
| .sorted((a, b) -> Long.compare(b, a)), "trimHistory").forEach(n -> { | |
| .sorted((a, b) -> Long.compare(b, a)), "trimHistory") | |
| .map(ByteArray::fromLong).forEach(n -> { | |
| try { | |
| byte[] blockIdHash = blockIndexDb.get(ByteArray.fromLong(n)); | |
| byte[] blockIdHash = blockIndexDb.get(n); | |
| Protocol.Block block = Protocol.Block.parseFrom(blockDb.get(blockIdHash)); | |
| // delete transactions | |
| for (Protocol.Transaction e : block.getTransactionsList()) { | |
| transDb.delete(DBUtils.getTransactionId(e).getBytes()); | |
| DBInterface transactionHistoryDb = DbTool.getDB(liteDir, TRANSACTION_HISTORY_DB_NAME); | |
| DBInterface balanceTraceDb = DbTool.getDB(liteDir, BALANCE_TRACE_DB_NAME); | |
| DBInterface accountTraceDb = DbTool.getDB(liteDir, ACCOUNT_TRACE_DB_NAME); | |
| ProgressBar.wrap(LongStream.rangeClosed(start, end) | |
| .boxed() | |
| .sorted((a, b) -> Long.compare(b, a)), "trimHistory") | |
| .map(ByteArray::fromLong).forEach(n -> { | |
| try { | |
| byte[] blockIdHash = blockIndexDb.get(n); | |
| Protocol.Block block = Protocol.Block.parseFrom(blockDb.get(blockIdHash)); | |
| // delete transactions | |
| for (Protocol.Transaction e : block.getTransactionsList()) { | |
| byte[] txId = DBUtils.getTransactionId(e).getBytes(); | |
| transDb.delete(txId); | |
| transactionHistoryDb.delete(txId); |
Steps of Reproduction ✅
1. Enable transaction history persistence so `TransactionHistoryStore` is used (its `put`
method checks `CommonParameter.getInstance().getStorage().getTransactionHistorySwitch()`
in `chainbase/src/main/java/org/tron/core/store/TransactionHistoryStore.java:27-32`). Run
a full node so that transactions and their history are written into
`transactionHistoryStore`.
2. Use the DbLite toolkit to create a snapshot and then, after producing additional
blocks, create a history dataset so that `historyMaxNum > snapshotMaxNum`. This pattern is
exercised by `testTools(..., advanceSnapshot=false, historyBalanceLookup=false)` in
`plugins/src/test/java/org/tron/plugins/DbLiteTest.java:23-60`, which generates blocks,
creates a snapshot, generates more blocks, then creates history.
3. Merge history into the lite database by invoking the DbLite CLI with `-o merge` (wired
to `DbLite.call()` at `plugins/src/main/java/common/org/tron/plugins/DbLite.java:119-149`,
which dispatches to `completeHistoryData()` at `DbLite.java:225-260`). In
`completeHistoryData`, archive DBs including `"transactionHistoryStore"` are backed up and
then copied from `historyDir` into `liteDir` (`archiveDbs` list at `DbLite.java:65-72`,
`backupArchiveDbs` at `DbLite.java:497-505`, `copyHistory2Database` at
`DbLite.java:508-512`).
4. When `historyMaxNum > snapshotMaxNum`, `completeHistoryData` calls
`trimExtraHistory(liteDir, blockNumInfo)` at `DbLite.java:244`. Inside `trimExtraHistory`
(`DbLite.java:514-571`), for each block in `[start, end]` it deletes transactions from
`transDb` and per-block results from `TRANSACTION_RET_DB_NAME` via `tranRetDb.delete(n)`
at `DbLite.java:541-546` and removes the corresponding blocks and indices, but it never
touches `TRANSACTION_HISTORY_DB_NAME`. After this, `mergeBak2Database`
(`DbLite.java:573-609`) merges backed-up DBs; for `TRANSACTION_HISTORY_DB_NAME` it copies
all entries from the backup without filtering by block (see `archiveDbs.stream()` and
conditional seek at `DbLite.java:592-603`). As a result, `transactionHistoryStore` retains
entries for transactions whose blocks were trimmed. A client calling
`Wallet.getTransactionInfoById(transactionId)` in
`framework/src/main/java/org/tron/core/Wallet.java:1888-1894` will find no result in
`TransactionRetStore` (entry deleted) but a non-null `TransactionInfoCapsule` from
`TransactionHistoryStore.get(transactionId.toByteArray())`, even though the corresponding
block and trans mapping were removed, leaving transaction-by-id queries returning
information for blocks that no longer exist in the trimmed lite history.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** plugins/src/main/java/common/org/tron/plugins/DbLite.java
**Line:** 531:543
**Comment:**
*Logic Error: During history trimming, transaction IDs are removed from `trans` and `transactionRetStore`, but `transactionHistoryStore` entries for the same trimmed blocks are left behind. This leaves stale transaction info queryable by transaction ID after the corresponding blocks are deleted, so delete transaction-history entries for each removed block transaction as well.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.There was a problem hiding this comment.
Not applicable. transactionHistoryStore is deprecated and no longer written to, so there is no stale data to clean up.
|
CodeAnt AI finished running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
@codex: review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eaba17c211
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (TRANS_DB_NAME.equals(dbName) || TRANSACTION_HISTORY_DB_NAME.equals(dbName) | ||
| || ACCOUNT_TRACE_DB_NAME.equals(dbName)) { | ||
| iterator.seekToFirst(); |
There was a problem hiding this comment.
mergeBak2Database computes head = historyMaxNum + 1 to restore only backup data newer than the imported history, but this branch forces account-trace to seekToFirst() and copies the entire DB. When snapshotMaxNum > historyMaxNum (the merge-back path) and history balance lookup is enabled, this reintroduces snapshot-side account-trace entries for overlapping heights, so historical balance queries can read forked/stale values for affected addresses. Account-trace rows need height filtering (e.g., decode the block number suffix and skip <= historyMaxNum) instead of unconditional full replay.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is not an issue. The merge path runs when snapshotMaxNum > historyMaxNum. The backup (snapshot) and liteDir (history) are from the same chain, so for blocks <= historyMaxNum, account-trace entries are identical — put is idempotent. For blocks > historyMaxNum, only the snapshot has data, which is correctly merged back. There is no fork scenario in normal DbLite usage.
halibobo1205
force-pushed
the
feat/toolkit_db_lite_opt
branch
from
eaba17c to
3885da2
Compare
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
size:M
|
CodeAnt AI Incremental review completed. |
…#6631) * feat(protocol): add protoLint script for enum validation * feat(protocol): optimize protoLint performance and caching * fix(proto): resolve gradle implicit task dependency warning * docs(protocol): clarify enum discriminator in protoLint * build(protocol): harden proto lint buf config * docs: update comment to reflect dynamic include path derivation
halibobo1205
force-pushed
the
feat/toolkit_db_lite_opt
branch
from
3885da2 to
a03adc7
Compare
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
size:L
|
CodeAnt AI Incremental review completed. |
What Changed Exclude balance-trace and account-trace from lite snapshots. Since historical balance lookup is off by default, most nodes see no difference in snapshot size; only nodes with the feature enabled benefit from smaller snapshots. Impact ✅ Smaller lite snapshots when historical balance lookup is enabled ✅ No impact for default-configured nodes close tronprotocol#6597
halibobo1205
force-pushed
the
feat/toolkit_db_lite_opt
branch
from
a03adc7 to
4f4f8dd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f4f8dd10e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| DBInterface balanceTraceDb = DbTool.getDB(liteDir, BALANCE_TRACE_DB_NAME); | ||
| DBInterface accountTraceDb = DbTool.getDB(liteDir, ACCOUNT_TRACE_DB_NAME); |
There was a problem hiding this comment.
When merging with a history dataset that does not contain balance-trace/account-trace (e.g., datasets generated before this change), trimExtraHistory opens these DBs via DbTool.getDB(liteDir, ...), which defaults to LevelDB if engine.properties is absent. In a RocksDB node this creates new trace DB directories tagged as LEVELDB, and subsequent node startup fails engine validation (Cannot open LEVELDB database with ROCKSDB engine). This is a regression introduced by eagerly opening these optional stores without inheriting the existing node engine.
Useful? React with 👍 / 👎.
2 participants
User description
What does this PR do?
Why are these changes required?
This PR has been tested by:
Follow up
Extra details
CodeAnt-AI Description
Keep historical balance lookups working after lite snapshot trimming
What Changed
Impact
✅ Reliable historical balance lookups after snapshot trimming✅ Fewer stale trace records after lite database rebuilds✅ Safer history cleanup when balance lookup is enabled🔄 Retrigger CodeAnt AI Review
Details
💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.