Skip to content

Conversation

TheCharlatan
Copy link
Contributor

I might be missing a reason for having the default constructor, but it is not used anywhere and trying to use one of its methods will cause a segfault.

It is not used and trying to use one of its methods will cause a
segfault.
@theuni
Copy link
Contributor

theuni commented Jun 12, 2024

Concept ACK. @TheCharlatan and I discussed this and couldn't find a user of the default ctor.

Barely any functions check for a valid m_minisketch before using it. So rather than adding the checks, we figured it made sense to just eliminate the possibility.

As far as I can see, without this ctor, the only way to hit a null m_minisketch would be a moved-from object, which is usually assumed to be undefined unless otherwise specified.

@sipa
Copy link
Collaborator

sipa commented May 13, 2025

ACK 5cc7d20

@sipa sipa merged commit ea8f66b into bitcoin-core:master May 13, 2025
fanquake added a commit to fanquake/bitcoin that referenced this pull request May 13, 2025
ea8f66b1ea Merge bitcoin-core/minisketch#89: header: Delete default Minisketch constructor
fc24958646 Merge bitcoin-core/minisketch#94: misc: trim trailing whitespace
485b16e7ff misc: trim trailing whitespace
5cc7d20866 header: Delete default Minisketch constructor

git-subtree-dir: src/minisketch
git-subtree-split: ea8f66b1eabb0388d221daaa4706120a5083da0c
fanquake added a commit to bitcoin/bitcoin that referenced this pull request May 16, 2025
bf25a09 Squashed 'src/minisketch/' changes from d1e6bb8bbf..ea8f66b1ea (fanquake)

Pull request description:

  Includes:
  * bitcoin-core/minisketch#89
  * bitcoin-core/minisketch#94 (in #32482)

ACKs for top commit:
  maflcko:
    lgtm ACK 46b533d
  willcl-ark:
    ACK 46b533d

Tree-SHA512: 7655aa480381c711fd3397fc7031be4619d5bd67f9d68f7e69f5f903734e776b97f7a2384257b365fc4e1f504b9c97acfd4a355840d1064d4c2c2f259070f4ef
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