return cached signature on regression request#1
Conversation
WalkthroughA new mechanism for caching and pruning recently signed messages was added to the consensus state, using a Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/chain/state.rs(8 hunks)src/privval.rs(1 hunks)src/session.rs(4 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
src/chain/state.rs
[error] 1-350: cargo fmt check failed due to code formatting differences. Please run 'cargo fmt' to fix formatting issues.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-push-image
🔇 Additional comments (7)
src/privval.rs (1)
24-24: LGTM! Clone derivation enables signature caching.The addition of the
Clonetrait toSignableMsgis necessary to support the new signature caching mechanism. This allows signable messages to be stored and retrieved from the cache without ownership issues.src/session.rs (3)
131-133: LGTM! Early return optimization for cached signatures.The early return when a cached signature is found is an effective optimization that prevents redundant signing operations. The logic properly checks for existing signatures before proceeding with the signing process.
166-166: LGTM! Proper signature caching after successful signing.The signature is correctly cached after successful signing and before returning the response. The use of
clone()is appropriate here since the signable message needs to be stored in the cache while also being returned to the caller.
231-242: LGTM! Well-structured caching helper methods.The private helper methods are well-designed:
check_existing_sign: Properly extracts consensus state and queries the cachesave_sign: Correctly stores the signature with appropriate lockingBoth methods handle the mutex locking appropriately and extract the necessary state information for cache operations.
src/chain/state.rs (3)
30-30: LGTM! Appropriate data structure for signature caching.The
BTreeMap<(Height, Round, i8), SignableMsg>is a good choice for caching signatures:
- The key tuple
(Height, Round, i8)uniquely identifies consensus states- BTreeMap provides efficient lookups and ordered iteration for pruning
- SignableMsg values can now be cloned thanks to the trait derivation
53-53: LGTM! Proper field initialization.The
last_signsfield is correctly initialized as an empty BTreeMap in both constructor methods (load_stateandwrite_initial_state).Also applies to: 180-180
214-221: LGTM! Efficient cache operations with pruning.The cache methods are well-implemented:
get_last_vote: Usescloned()to return an owned copy without affecting the cachesave_sign: Implements intelligent pruning by retaining only entries within 100 heights of the current heightThe pruning logic
height.value() < 100 + key.0.value()effectively keeps recent signatures while preventing unbounded memory growth.
| #[test] | ||
| fn test_save_sign() { | ||
| let mut state = State { | ||
| consensus_state: state!(1, 1, 0, None), | ||
| state_file_path: EXAMPLE_PATH.into(), | ||
| last_signs: BTreeMap::new(), | ||
| }; | ||
|
|
||
| state.save_sign(Height::from(1 as u32), Round::from(1 as u16), 0, SignableMsg::Proposal(Proposal{ | ||
| msg_type: Type::Proposal, | ||
| pol_round: None, | ||
| block_id: None, | ||
| timestamp: None, | ||
| signature: None, | ||
| height: Height::from(1 as u32), | ||
| round: Round::from(1 as u16), | ||
| })); | ||
|
|
||
| assert_eq!(state.last_signs.len(), 1); | ||
| state.save_sign(Height::from(100 as u32), Round::from(1 as u16), 0, SignableMsg::Proposal(Proposal{ | ||
| msg_type: Type::Proposal, | ||
| pol_round: None, | ||
| block_id: None, | ||
| timestamp: None, | ||
| signature: None, | ||
| height: Height::from(100 as u32), | ||
| round: Round::from(1 as u16), | ||
| })); | ||
| assert_eq!(state.last_signs.len(), 2); | ||
|
|
||
| state.save_sign(Height::from(101 as u32), Round::from(1 as u16), 0, SignableMsg::Proposal(Proposal{ | ||
| msg_type: Type::Proposal, | ||
| pol_round: None, | ||
| block_id: None, | ||
| timestamp: None, | ||
| signature: None, | ||
| height: Height::from(101 as u32), | ||
| round: Round::from(1 as u16), | ||
| })); | ||
| assert_eq!(state.last_signs.len(), 2); | ||
| } |
There was a problem hiding this comment.
Fix code formatting issues.
The test code demonstrates the pruning functionality well, but there are formatting issues that need to be addressed.
Based on the pipeline failure, please run cargo fmt to fix the formatting issues:
cargo fmtThe test logic itself is sound - it verifies that:
- Signatures are added to the cache
- Pruning occurs when height advances beyond the 100-height window
🤖 Prompt for AI Agents
In src/chain/state.rs around lines 333 to 373, the test_save_sign function has
formatting issues causing pipeline failures. Run the Rust formatter by executing
`cargo fmt` in the project root to automatically fix indentation, spacing, and
line breaks according to Rust style guidelines. This will resolve the formatting
problems without altering the test logic.
|
I think this should be done with more cares like the signing block is exactly same or not |
2 participants
Summary by CodeRabbit