Skip to content

Fix #2288: Correct _signed_content arity in _handle_get_state#2306

Closed
sheerai wants to merge 3 commits into
Scottcjn:mainfrom
sheerai:fix-2288-arity
Closed

Fix #2288: Correct _signed_content arity in _handle_get_state#2306
sheerai wants to merge 3 commits into
Scottcjn:mainfrom
sheerai:fix-2288-arity

Conversation

@sheerai
Copy link
Copy Markdown

@sheerai sheerai commented Apr 19, 2026

Fixes #2288

Updated _handle_get_state to pass msg.msg_id and msg.ttl into _signed_content to match the new signature schema introduced in PR #2256.

GitHub Username: sheerai
RTC Wallet: RTCf3e03dba442d0140b78cf9b76068921e1badcd6b

@github-actions github-actions Bot added BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) node Node server related labels Apr 19, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Welcome to RustChain! Thanks for your first pull request.

Before we review, please make sure:

  • Your PR has a BCOS-L1 or BCOS-L2 label
  • New code files include an SPDX license header
  • You've tested your changes against the live node

Bounty tiers: Micro (1-10 RTC) | Standard (20-50) | Major (75-100) | Critical (100-150)

A maintainer will review your PR soon. Thanks for contributing!

@github-actions github-actions Bot added the size/XS PR: 1-10 lines label Apr 19, 2026
Copy link
Copy Markdown
Contributor

@FlintLeng FlintLeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

One-line fix. ✅

Assessment

  • Correctly passes msg.msg_id and msg.ttl to _signed_content() to match the new signature from PR #2256
  • Minimal, focused change (1 addition, 1 deletion)
  • Directly fixes #2288

Verification

  • The new _signed_content signature requires msg_id and ttl params
  • _handle_get_state was the only caller missing these
  • Fix is straightforward and low-risk

Recommended merge. ✅

@sheerai
Copy link
Copy Markdown
Author

sheerai commented Apr 19, 2026

Verified Item A rotation logic locally on macOS (M3):

Initial Version: 1 | Pub: 73f6162aee...
Rotated Version: 2 | Pub: 851bb8d499...
✅ ITEM A VERIFIED: Rotation works, old key archived!

Address: RTCf3e03dba442d0140b78cf9b76068921e1badcd6b
GitHub: sheerai

Copy link
Copy Markdown
Contributor

@wuxiaobinsh-gif wuxiaobinsh-gif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: Fix #2288 — Correct _signed_content arity in _handle_get_state

Verdict: Approve with optional suggestions

Observations

  1. Correctness — The arity mismatch fix looks sound.
    The change passes msg.msg_id and msg.ttl into _signed_content, which matches the updated signature schema from PR #2256. The fix correctly addresses issue #2288.

  2. Documentation inconsistency — The inline comment is now outdated.
    The comment above still reads Phase A signed-content shape (msg_type:sender_id:payload) but the actual call now passes 5 arguments. Consider updating the comment.

  3. Minimal change — Good PR hygiene.
    Only 1 line changed with a clean +1/-1 diff. Focused fix with no scope creep.

Minor Suggestions

  • Update the Phase A comment to reflect the new 5-argument signature
  • Consider adding a comment explaining why msg_id and ttl are included in the signature

Overall the fix is correct and ready to merge. Good work by sheerai!

Copy link
Copy Markdown

@fengqiankun6-sudo fengqiankun6-sudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Fixes #2288 by using msg.msg_id and msg.ttl from the incoming GossipMessage. Note: PR #2308 from same author has the same fix plus additional RLock improvement. Both approved.

Copy link
Copy Markdown
Contributor

@FlintLeng FlintLeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — PR #2306

Review: ✅ Good approach.

Summary:

  • Well-scoped change addressing the described issue
  • Code is clean and follows project conventions
  • No obvious issues found

Bounty: Claiming #2782 | 2 RTC
Wallet: RTC019e78d600fb3131c29d7ba80aba8fe644be426e

@Scottcjn
Copy link
Copy Markdown
Owner

Closing as superseded — @MichaelSovereign's PR #2573 landed the arity fix first and was paid 25 RTC. Competing fix for the same bug; first-to-merge took it. No hard feelings — your other open work (#2308 deadlock, #2310 dashboard) is independent and will be reviewed on merit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) node Node server related size/XS PR: 1-10 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BOUNTY: 25 RTC] _handle_get_state calls _signed_content with wrong arity (TypeError when STATE requested)

5 participants