Skip to content

Conversation

@reez
Copy link
Collaborator

@reez reez commented Oct 13, 2025

Description

Propagates invalid genesis hashes instead of panicking.

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@reez
Copy link
Collaborator Author

reez commented Oct 27, 2025

upstream electrum never trys to construct a BlockHash (stops at deserializing into raw bytes) like we are trying to do here in bdk-ffi, so I'm proposing this extra check here (and not panic) in the case the parse fails, while uncommon for electrum to pass something bad back to us we should treat this sort of failure as a recoverable error (surfacing the error) instead of crashing.

I can add a test, but hopefully the above comment makes sense, feel free to ask me anything else related to this though.

just rebased PR now, fyi is all @thunderbiscuit

@rustaceanrob
Copy link
Collaborator

Although it will probably never happen, good to protect against a malicious server

ACK 2578420

Copy link
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

ACK 2578420. Just needs a rebase.

@reez
Copy link
Collaborator Author

reez commented Oct 29, 2025

rebased

@thunderbiscuit thunderbiscuit merged commit 9b996e1 into bitcoindevkit:master Oct 29, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants