JayDDee / cpuminer-opt

Optimized multi algo CPU miner
Other
774 stars 544 forks source link

alllium algo is broken since v3.18.0 #342

Closed YetAnotherRussian closed 2 years ago

YetAnotherRussian commented 2 years ago

low diff shares only (tested on pool https://www.nlpool.nl with avx/avx2 win builds). Warning: don't test on p2p pool from miningpoolstats.stream (that pool seems to be not maintained - front-end is broken)

Works OK in v3.17.1

JayDDee commented 2 years ago

Reproduced Windows only, SSE2 build works, suspect it's AES that's the problem, groestl specifically. The only change in Groestl was to use the _lrotl builtin function instead of the localy defined double-shift-or for bit rotations. Seems it doesn't work on Windows, I only tested it on Linux initially.

I think it's a size problem. There are three intrinsics defined by Intel: int _rotl, long _lrotl, and __int64 _rotl64. _rotl64 popped an undefined error on linux so I used _lrotl which wotked for 64 bit rotate. However the definition for _lrotl is ambiguous, both 32 and 64 bit rotates, likely due to Windows definition of long. I've run accross problems with sizing with printf due to the different definition of long.

Maybe _lrotl64 works on Windows, otherwise it's back 3 instruction rotates.

JayDDee commented 2 years ago

I found rolq and rold, maybe these are defined by GCC, but they remove the size ambiguity. They work on Linux, need to test Windows.

JayDDee commented 2 years ago

Windows test passed. I'm in no hurry for a new release, nothing else is in it yet. Algos that use AES including all the X* algos should use v3.17.1 in the meantime.

YetAnotherRussian commented 2 years ago

I'm in no hurry for a new release

Well, it could be a good idea to add an alias (incl. --help output) for scrypt:1048576. Something like scrypt-n2 or scryptn2 (scrypt2/scrypt²/scrypt^2 are all bad variants), really easy addition (I see your great progress for this algo).

Also I see something interesting here (with allium, but the algo should not matter I guess):

image

But --no-extranonce is used. I see it is actually disabled on the pool side (as compared to another miner, where it is intentionally enabled). So, the correct way is not to print that line, or print something like "Extranonce subscription is supported, but disabled" (or just "Extranonce subscription is disabled").

YetAnotherRussian commented 2 years ago

Seems that --benchmark option was affected in 3.18/3.18.1 as it has a small issue (which cannot be reproduced in 3.17.1):

cpuminer-zen3.exe -t 1 --cpu-affinity 1 -a sha256d --benchmark

v. 3.18.1: image

v. 3.18 (negative values not fixed): image

So, bench mode spams a lot @ start (sometimes it's 5-10 lines, sometimes up to 100 lines)

JayDDee commented 2 years ago

Benchmark was a tradeoff. I'd rather it work for a CPU mineable algo like scryptn2. Prior to v3.18.0 benchmark was unusable on scryptn2.The issue is the difference in hashrate, sha256d is the fastest and scryptn2 the slowest. What you're seeing is the miner trying to adjust it's timing to the algo. It isn't pretty but to supress it means adding code that will run every loop to check if the hash rate is sane before outputting it. I don't like adding code to the loop that is only needed at startup.

"scryptn2" is already defined as an alias but it's not listed in the help. I haven't decided whether I want to add it.

Extranonce is weird. The log you're seeing is because stratum_subscribe always parses the extranonce rgardless of the option. Later there is an extranonce_subscribe that is bypassed if the option is disabled. I don't want to mess with that without fully understanding it. I also see no reason to use--no-extranonce, it just works when it's needed.

YetAnotherRussian commented 2 years ago

I also see no reason to use--no-extranonce, it just works when it's needed.

I don't know why but widely available p2pools require that to be disabled

JayDDee commented 2 years ago

I also see no reason to use--no-extranonce, it just works when it's needed.

I don't know why but widely available p2pools require that to be disabled

But the log means it works, at least the first part works. The server even tells us what the size is for part 2. If the option is disabled the second part is skipped. What happens on p2pool if option is enabled? I'm thinking the option only applies to extranonce2 but the name isn't specific.

JayDDee commented 2 years ago

One thing about extranonce2 is it does nothing until you run out of nonces. When that happens the miner increments extranonce2 to generate it's own work to allow it to continue mining. Without extranonce2 it just stops mining until the server sends new work. If extranonce2 truly needs to be disabled it should be indicated in the stratum_subscribe response which would have generated warnings or errors.

Edit: if you want to seen extranonce2 in action just connect to a sha256d pool and let it for a couple of minutes all threads. Everytime the miner runs out of nonces you will see a log. Even a CPU needs extranonce for sha256d. Without extranonce the miner would stop mining until a new job is received from the pool.

Edit: if you haven't already you should read https://en.bitcoin.it/wiki/Stratum_mining_protocol, particualrly the mining.subscribe & mining.extranonce.subscribe methods.

JayDDee commented 2 years ago

I found a way to reduce the benchmark spam without affecting normal mining. Simply don't log the Total hashrate if it's zero.

I can't reproduce the spam but the fix seems to work well. I never see zero total hashrate. The first one or 2 logged are still invalid but they are just nominal values used to help tuning the scan time.

JayDDee commented 2 years ago

So, the correct way is not to print that line, or print something like "Extranonce subscription is supported, but disabled" (or just "Extranonce subscription is disabled").

I have to disagree with this. The log is just reporting the server's response to mining.subscribe which isn't part of the extranonce option. There are checks in the handler and any errors would have been logged. Instead the "success" log was produced. The log IMO is perfectly legitimate.

Edit: I don't like the help description for no-extranonce. It doesn't really disable support, only it's use. I may change it to "diisables extranonce subscribe".

JayDDee commented 2 years ago

Regarding including scryptn2 my concerns are the same as yespower.

scrypt:N is the generic form that can be used by any scrypt variation so must be included.

scrypt (default) has a legacy that means scrypt:1024 but is a specific variation. There are many coins that use this variation.

scryptn2 means scrypt:1048576, another specific variation that few coins use. Will it survive?

If I include one variation should I include them all? Is it fair to the coins that use a variation without a custom alias?

I don't like making decisions when the right answer isn't clear so this one will probably remain status quo.

YetAnotherRussian commented 2 years ago

If I include one variation should I include them all? Is it fair to the coins that use a variation without a custom alias?

Don't know. The're coins where some schoolboys even did not manage to copy&paste yespower key (yespower_params.pers) correctly, and third-party forks do support these "algos". I don't think you should, as you may turn your miner to an "algo graveyard" in the future. As for configurable algos, I personally miss the ability to tune argon2d the way yespower/scrypt are made, so any argon2d16000 or argon2d1234567890 could be used w/o changes in code. But the same question is actual for those:

Will it survive?

Lots of algos like this, this or this are old or popular but require some extra work to be added (not that low amount of work), and, again:

Will it survive?

No one knows.

JayDDee commented 2 years ago

I was thinking a bit more about your extranonce concerns. Using the same name for 2 unrelated features is unfortunate because it leads to confusion.

The first part is not optional and results in the log you noted if successful, but an error exit otherwise. An error is fatal regardless the no-extranonce option.

The second part is what extends the nonce range for high hash rate algos. This part is optional and can be disabled with --no-extranonce.

If a pool does not support extranonce subscribe and the featire is not disabled in the miner it will probably fail with an error, hopefully one that is informative for the user. A successful extranonce subscribe is usually silent with no positive confirmation.

I don't think such a log is required since extranonce subscribe is so widespread. However, I could add a log when the feature is disabled. It may help avoid some of the confusion caused by the first log.

It would be good to confirm how p2pools really work. Do they require --no-extranonce? What happens if --no-extranonce isn't used?

YetAnotherRussian commented 2 years ago

Hypothetically: https://github.com/trexminer/T-Rex/issues/23

As for now (2021), I cannot confirm that pool does not work with cpuminer-opt w/o --no-extranonce option, but it still has that option in example cli settings. Without " --no-extranonce" I'm still getting:

image

Here http://p2p-spb.xyz:5213/static/ I can see the option set for sgminer as well. Here http://p2p-spb.xyz:9552/static/ - for cpuminer-opt.

This one https://pool.freshgrlc.net/ is supposedly p2p, but I can't see "--no-extranonce" there.

Goooooogling the problem gives some info that "some pools ignore miner connection if extranonce subscription request is sent",

UPD.: I've also reproduced rare extranonce issues with fastpool.xyz and another miner (srbminer) (cpuminer-opt does not support this algo), so I can say issues do happen with this feature. Sometimes.

JayDDee commented 2 years ago

I don't see any issues with extranonce. no-extranonce option seems to work as required and no one has reported any problems.

It seems like your only concern is the log and that it uses the word extranonce. I have changed nothing in how extranonce subscribe is handled, and I don't intend to. The only question is the log and whether I should just remove it. Is that what your'e suggesting?

JayDDee commented 2 years ago

V3.18.2 should fix the initial issue with Allium. In addition zero hash rate logs will be suppressed in benchmark mode. No changes were made to either extranonce logging or its operation. The confusion is due to the ambiguous terminology in the specification.

JayDDee commented 2 years ago

No problems reported.