fix(store)!: charge gaskv iterator elements exactly once - #26739
fix(store)!: charge gaskv iterator elements exactly once#26739SnowingFox wants to merge 2 commits into
Conversation
|
@greptile review |
Greptile SummaryThe pull request corrects
Confidence Score: 4/5The implementation appears safe to merge after the non-blocking changelog attribution is brought into the repository's required format. The iterator change and regression test consistently implement one per-byte charge per visited element; the only accepted concern is the changelog entry's missing component tag and incorrect link target. Files Needing Attention: store/CHANGELOG.md Important Files Changed
Reviews (1): Last reviewed commit: "fix(store): charge gaskv iterator elemen..." | Re-trigger Greptile |
|
|
||
| ### Bug Fixes | ||
|
|
||
| * [#15854](https://github.com/cosmos/cosmos-sdk/issues/15854) Fix `gaskv` iterator metering the first iterated element's gas twice, so each key/value pair is charged exactly once. |
There was a problem hiding this comment.
Use the required changelog attribution
This entry omits the required (store) tag and links issue #15854 instead of this pull request, making its attribution inconsistent with the repository's changelog format.
| * [#15854](https://github.com/cosmos/cosmos-sdk/issues/15854) Fix `gaskv` iterator metering the first iterated element's gas twice, so each key/value pair is charged exactly once. | |
| * (store) [#26739](https://github.com/cosmos/cosmos-sdk/pull/26739) Fix `gaskv` iterator metering the first iterated element's gas twice, so each key/value pair is charged exactly once. |
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
should be * [#26739](https://github.com/cosmos/cosmos-sdk/pull/26739) Fix... under state-breaking section
There was a problem hiding this comment.
You're right. This changes externally observable gas consumption, so it belongs under API Breaking, and the entry should reference this PR rather than the issue. My previous (store)-tag requirement was incorrect for this changelog.
| * [#15854](https://github.com/cosmos/cosmos-sdk/issues/15854) Fix `gaskv` iterator metering the first iterated element's gas twice, so each key/value pair is charged exactly once. | |
| * [#26739](https://github.com/cosmos/cosmos-sdk/pull/26739) Fix `gaskv` iterator metering the first iterated element's gas twice, so each key/value pair is charged exactly once. |
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
There was a problem hiding this comment.
You're right that this should reference PR #26739 rather than issue #15854, and the gas-accounting change belongs in the state-breaking/API-breaking category. store/CHANGELOG.md currently names that stanza API Breaking and does not have a separate State Breaking section, so the corrected entry there is:
* [#26739](https://github.com/cosmos/cosmos-sdk/pull/26739) Fix `gaskv` iterator metering the first iterated element's gas twice, so each key/value pair is charged exactly once.
I’ll withdraw the (store)-tag requirement from my earlier comment.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #26739 +/- ##
==========================================
+ Coverage 64.99% 65.26% +0.27%
==========================================
Files 785 830 +45
Lines 55399 58580 +3181
==========================================
+ Hits 36006 38235 +2229
- Misses 19393 20345 +952
🚀 New features to boost your workflow:
|
Summary
Fixes the
gaskviterator gas-accounting inaccuracy from #15854.On current
main,GStore.iteratormeters the first key/value pair when the iterator is created (seek to first), andgasIterator.Nextmeters the current pair again before advancing:Iterating over N elements therefore consumes
(N+1)per-byte charges: the first element is charged twice and the accounting for the last element is shifted by oneNext()step.Fix
Advance the underlying iterator before metering, so
consumeSeekGasmeters the pair the iterator has just moved to, and meters nothing per-byte when it advances past the last element:Each key/value pair is now charged exactly once: the first when the iterator is created, and every subsequent pair by the
Next()call that lands on it.Testing
TestGasKVStoreIteratorChargesEachElementOnce, which iterates N keys and asserts the total gas equalsN * ReadCostPerByte * len(key+value) + (N+1) * IterNextCostFlat. It fails onmain(off by one per-byte charge) and passes with this change.TestGasKVStoreIteratorfor the corrected accounting.Closes #15854