Skip to content

Conversation

cuiweixie
Copy link
Contributor

@cuiweixie cuiweixie commented Sep 22, 2025

Summary by CodeRabbit

  • Refactor
    • Simplified internal prefix handling using a more reliable standard approach, reducing custom logic.
  • Bug Fixes
    • Added explicit validation when deriving keys with prefixes, preventing silent inconsistencies and surfacing issues earlier.
  • Chores
    • Removed unused internal helper code related to prefix processing.

No user-facing behavior changes are expected under normal conditions; overall robustness and maintainability are improved.

@cuiweixie
Copy link
Contributor Author

cuiweixie commented Sep 22, 2025

#25357
@aljo242

Copy link
Contributor

coderabbitai bot commented Sep 22, 2025

📝 Walkthrough

Walkthrough

Replaced a custom prefix-stripping helper in PrefixIterator.Key with bytes.CutPrefix, adding an explicit presence check and panic on mismatch. Removed the unused helper. External behavior remains unchanged when the prefix is valid.

Changes

Cohort / File(s) Summary
Prefix handling in PrefixIterator.Key
store/prefix/store.go
Switched from custom stripPrefix to bytes.CutPrefix in Key(); added ok check and panic if prefix not found; removed stripPrefix helper.

Sequence Diagram(s)

sequenceDiagram
    participant C as Caller
    participant PI as PrefixIterator
    participant B as bytes.CutPrefix

    C->>PI: Key()
    PI->>B: CutPrefix(key, prefix)
    B-->>PI: (rest, ok)
    alt prefix present
        PI-->>C: return rest
    else prefix missing
        PI-->>C: panic("should not happen")
    end
    note over PI: Explicit validation replaces implicit assumption
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly identifies the primary change (switching to bytes.CutPrefix) and therefore relates to the main changeset, but it contains a spelling error ("libary") and awkward phrasing ("to use", "func") that reduce clarity and polish.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 9171179 and 70e20f0.

📒 Files selected for processing (1)
  • store/prefix/store.go (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Summary
🔇 Additional comments (2)
store/prefix/store.go (2)

166-169: LGTM! Good refactor to use standard library function.

The refactor to use bytes.CutPrefix is a good improvement that leverages the standard library. The explicit check for prefix presence with a panic on mismatch maintains the expected invariant while providing clearer error handling than the previous custom stripPrefix helper.


166-169: Panic is unreachable — prefix invariant holds.

newPrefixIterator sets pi.valid = parent.Valid() && bytes.HasPrefix(parent.Key(), prefix), and Next() clears pi.valid whenever iter becomes invalid or the key no longer has the prefix. Key() requires pi.valid before calling iter.Key(), so bytes.CutPrefix cannot fail under normal (single-threaded) iterator usage.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@aljo242
Copy link
Contributor

aljo242 commented Sep 22, 2025

can you test and benchmark this

key = stripPrefix(key, pi.prefix)
key, ok := bytes.CutPrefix(key, pi.prefix)
if !ok {
panic("should not happen")

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods Warning

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
@cuiweixie
Copy link
Contributor Author

can you test and benchmark this

 store git:(bytes.CutPrefix) go test ./...
?       cosmossdk.io/store      [no test files]
ok      cosmossdk.io/store/cache        (cached)
ok      cosmossdk.io/store/cachekv      (cached)
ok      cosmossdk.io/store/cachekv/internal     (cached)
ok      cosmossdk.io/store/cachemulti   (cached)
ok      cosmossdk.io/store/dbadapter    (cached)
ok      cosmossdk.io/store/gaskv        (cached)
ok      cosmossdk.io/store/iavl (cached)
ok      cosmossdk.io/store/internal/conv        (cached)
?       cosmossdk.io/store/internal/kv  [no test files]
ok      cosmossdk.io/store/internal/maps        0.010s
ok      cosmossdk.io/store/internal/proofs      (cached)
?       cosmossdk.io/store/internal/tree        [no test files]
ok      cosmossdk.io/store/listenkv     0.019s
ok      cosmossdk.io/store/mem  (cached)
?       cosmossdk.io/store/metrics      [no test files]
?       cosmossdk.io/store/mock [no test files]
ok      cosmossdk.io/store/prefix       0.021s
ok      cosmossdk.io/store/pruning      (cached)
ok      cosmossdk.io/store/pruning/types        (cached)
ok      cosmossdk.io/store/rootmulti    47.202s
ok      cosmossdk.io/store/snapshots    (cached)
ok      cosmossdk.io/store/snapshots/types      (cached)
ok      cosmossdk.io/store/streaming    (cached)
?       cosmossdk.io/store/streaming/abci       [no test files]
?       cosmossdk.io/store/streaming/abci/examples/file [no test files]
ok      cosmossdk.io/store/tracekv      0.019s
ok      cosmossdk.io/store/transient    (cached)
ok      cosmossdk.io/store/types        (cached)
?       cosmossdk.io/store/wrapper      [no test files]

benchmark:

package main

import (
	"bytes"
	"testing"
)

// 方法1:手动实现
func stripPrefix(key, prefix []byte) []byte {
	if len(key) < len(prefix) || !bytes.Equal(key[:len(prefix)], prefix) {
		panic("should not happen")
	}
	return key[len(prefix):]
}

// 方法2:使用 bytes.CutPrefix
func stripPrefixBuiltin(key, prefix []byte) []byte {
	result, ok := bytes.CutPrefix(key, prefix)
	if !ok {
		panic("should not happen")
	}
	return result
}

// Benchmark 测试
func BenchmarkStripPrefix(b *testing.B) {
	key := []byte("prefix_some_long_key_data_here")
	prefix := []byte("prefix_")

	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		_ = stripPrefix(key, prefix)
	}
}

func BenchmarkStripPrefixBuiltin(b *testing.B) {
	key := []byte("prefix_some_long_key_data_here")
	prefix := []byte("prefix_")

	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		_ = stripPrefixBuiltin(key, prefix)
	}
}

// 测试不同长度的 prefix
func BenchmarkStripPrefixShort(b *testing.B) {
	key := []byte("a_some_long_key_data_here")
	prefix := []byte("a_")

	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		_ = stripPrefix(key, prefix)
	}
}

func BenchmarkStripPrefixBuiltinShort(b *testing.B) {
	key := []byte("a_some_long_key_data_here")
	prefix := []byte("a_")

	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		_ = stripPrefixBuiltin(key, prefix)
	}
}

func BenchmarkStripPrefixLong(b *testing.B) {
	key := []byte("very_long_prefix_here_some_long_key_data_here")
	prefix := []byte("very_long_prefix_here_")

	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		_ = stripPrefix(key, prefix)
	}
}

func BenchmarkStripPrefixBuiltinLong(b *testing.B) {
	key := []byte("very_long_prefix_here_some_long_key_data_here")
	prefix := []byte("very_long_prefix_here_")

	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		_ = stripPrefixBuiltin(key, prefix)
	}
}

BenchmarkStripPrefix
BenchmarkStripPrefix-10                	505638128	         2.277 ns/op
BenchmarkStripPrefixBuiltin
BenchmarkStripPrefixBuiltin-10         	512019571	         2.286 ns/op
BenchmarkStripPrefixShort
BenchmarkStripPrefixShort-10           	478129240	         2.518 ns/op
BenchmarkStripPrefixBuiltinShort
BenchmarkStripPrefixBuiltinShort-10    	484700299	         2.421 ns/op
BenchmarkStripPrefixLong
BenchmarkStripPrefixLong-10            	371604224	         3.245 ns/op
BenchmarkStripPrefixBuiltinLong
BenchmarkStripPrefixBuiltinLong-10     	369011395	         3.267 ns/op
PASS

it's almost equal. code logic is almost equal too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants