Skip to content

Conversation

fjahr
Copy link
Contributor

@fjahr fjahr commented Apr 28, 2023

Since #27483 was merged the modinv() body is just one line calling pythons own implementation of pow(). We can just remove the function as it doesn't seem to add any value. Additionally the comment in the function is now outdated and the test is only testing two ways of doing modular inverse but both using python's pow() function.

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 28, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK theStack
Concept ACK sipa, kevkevinpal

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #27507 (lint: stop ignoring LIEF imports by fanquake)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot added the Tests label Apr 28, 2023
@sipa
Copy link
Member

sipa commented Apr 28, 2023

Concept ACK

@kevkevinpal
Copy link
Contributor

Concept ACK also did a grep for modinv in /test and looks like you got all them

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

ACK dc14ba0

@fanquake fanquake merged commit be0325c into bitcoin:master May 1, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 1, 2023
stratospher added a commit to stratospher/bitcoin that referenced this pull request Jun 9, 2023
remove util also since unit tests there were removed in bitcoin#27538

Co-authored-by: theStack <sebastian.falbesoner@gmail.com>
@stratospher
Copy link
Contributor

we'd need to remove util from TEST_FRAMEWORK_MODULES in test_runner too. i've included this in #24005 where anyways TEST_FRAMEWORK_MODULES has to be touched.

stratospher added a commit to stratospher/bitcoin that referenced this pull request Jun 28, 2023
remove util also since unit tests there were removed in bitcoin#27538

Co-authored-by: theStack <sebastian.falbesoner@gmail.com>
stratospher added a commit to stratospher/bitcoin that referenced this pull request Jun 29, 2023
remove util also since unit tests there were removed in bitcoin#27538

Co-authored-by: theStack <sebastian.falbesoner@gmail.com>
stratospher added a commit to stratospher/bitcoin that referenced this pull request Jun 29, 2023
remove util also since unit tests there were removed in bitcoin#27538

Co-authored-by: theStack <sebastian.falbesoner@gmail.com>
kwvg added a commit to kwvg/dash that referenced this pull request Feb 14, 2024
kwvg added a commit to kwvg/dash that referenced this pull request Feb 14, 2024
kwvg added a commit to kwvg/dash that referenced this pull request Feb 14, 2024
kwvg added a commit to kwvg/dash that referenced this pull request Feb 15, 2024
@bitcoin bitcoin locked and limited conversation to collaborators Jun 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants