Skip to content

fix: address review feedback from @UdjinM6 on #7303#5

Closed
thepastaclaw wants to merge 1 commit into
knst:refactor-spork-net-processing-v2from
thepastaclaw:reviewbot-pr7303-udjin-spork-signer
Closed

fix: address review feedback from @UdjinM6 on #7303#5
thepastaclaw wants to merge 1 commit into
knst:refactor-spork-net-processing-v2from
thepastaclaw:reviewbot-pr7303-udjin-spork-signer

Conversation

@thepastaclaw
Copy link
Copy Markdown

Addresses 2 review comment(s) from @UdjinM6 on dashpay#7303.

Review comments addressed:

Original PR: dashpay#7303


Automated fix by thepastaclaw


🤖 This was generated by an automated review bot.
Don't want automated PRs or comments on your code? You can opt out by replying here or messaging @PastaPastaPasta on Slack — we'll make sure the bot skips your PRs/repos going forward.

…r info in logs

- Replace IsSporkValid() with GetValidSporkSigner() returning std::optional<CKeyID>
  so the recovered signer key flows directly into ProcessSpork. This drops the
  ECDSA RecoverCompact() count per inbound spork from three to one (was: once in
  IsSporkValid, twice in ProcessSpork via has_value() + value()).
- ProcessSpork now takes the validated CKeyID as a parameter, removing the
  unreachable defensive check that was added in 6a4ee52 to replace the original
  assert.
- ProcessSpork takes a peer_log_suffix string_view so the "SPORK -- hash:..."
  seen/updated/new-signer/new log lines include peer info again (was lost when
  ProcessSpork stopped taking NodeId).
- Tighten ActiveSporks() doc comment to describe what it returns rather than
  what it "handles".

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@thepastaclaw
Copy link
Copy Markdown
Author

Hi @knst — this PR applies the follow-up for @UdjinM6's review feedback on dashpay#7303. Could you take a look when you have a chance?

@knst
Copy link
Copy Markdown
Owner

knst commented May 14, 2026

dashpay#7303 already includes both Udjin's patches and merged @thepastaclaw

@knst knst closed this May 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants