-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Disable in-wallet miner for win/macos Travis/Gitian builds #2778
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but test is currently failing.
Did you try to figure out which part exactly is causing the false-positives? I see that you removed the whole block assembler code (miner.cpp) which then required touching tests. Shouldn't it be enough to remove |
Excluded It's weird though, I'm pretty sure that when I tried smth like that earlier I had like 10-ish detections -that's why I started excluding more and more code and finally decided to split builds. Well, anyway... Dropped additional builds Travis now (should be ok-ish to ignore one single EDIT: also rebased and renamed |
LGTM, I'm however wondering why it's needed to use the new configure option in all win+mac builds on Travis? Wouldn't it be enough to only use it on one of the builds to just make sure it builds/links? In the end it's only important to use it in Gitian builds later. |
It's basically to make sure that all Gitian builds will/should succeed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I guess it's better to be on the safe side here instead of realizing that gitian fails AFTER we tag a release :)
utACK
I added the backport-candidate label for 13.x, just in case we decide to do a 13.3 release |
This should lower AV false-positive score for our win/macos release binaries quite a bit.
Before:
https://www.virustotal.com/#/file/f0b6f5833a177d0254ce874a8cdf5b317e648fc05e53db1b6444d0f19f807d9b/detection (17)
After:
https://www.virustotal.com/#/file/30046d2a7bd29b3441df2591ba38f53c559ba8613064e3ed597881da91bcab41/detection (4)
Bitcoin
master
(for comparison):https://www.virustotal.com/#/file/1feaf335225525bb492a74929c5bdeb48772fcd4cca02a20d1bc846d906eadf2/detection (6)
Not sure what else could be removed to make every AV happy (suggestions are welcome!) but this PR resolves issues with most major AVs already, so imo it's good enough even if there is nothing else we can do to improve it further.
Downsides: