fix: reset extranonce2 only if clean_jobs == true - #1694
fix: reset extranonce2 only if clean_jobs == true#1694blackmennewstyle wants to merge 8 commits into
Conversation
This PR improves the extranonce2 reset. It is only performed if clean_jobs == true. The previous code was triggering a cascade of duplicate shares after a varDiff change: - mining.set_difficulty - mining.notify with clean_jobs is set to false The behavior is described here: bitaxeorg#1587 (comment)
|
out of interest, where did you see this duplicated share burst? On what pools and modes. I have never seen this, an dups after vardiff, that doesn't really makes sense to me. You should see maybe some lowDiff Shares on a vardiff change if the pool is very strict on workerdiff changes. CK accepts the old diff until the a new template arrives. |
blackmennewstyle
changed the title
Literally all stratum software using I provided the reason multiple times with examples when i opened my bug: #1587 (comment) where i tracked it all down to the culprit and it is specifically the
It is triggered by the naive reset of
With my patch over two days of mining, even on |
IMHO extranonce2 should never be reset. Until it overflows it would require more than 100 years 24/7 operation or so. And even then, it really doesn't matter, the counter would wrap around and starts again at 0. |
|
After 2 hours on bitcoin.cedric-crispin.com: 7 hours in: |
My I actually have someone who is getting duplicated shares currently: In I saw in the ESP-Miner source code, it was not that value which was expected apparently in order to be classified as When i check mine, they are all gone with the patch. You are getting duplicated each time the |
Better if else structure, more pleasing for the eyes and logical code.
It was from memory, could be 80 seconds as well. I blame the time dilution when watching logs. |
I ran it for 20+ hours and not a single duplicate share. In that time it must have had 1000 or so varDiffs, so not sure what's different between my setup and this report? |
In I saw in the ESP-Miner source code, it was not that value which was expected apparently in order to be classified as |
|
What's the format of a share reject message? As there's no formal format, there's a best effort in parsing them. Take a look at the test cases to see what's supported: |
format should be: |
The format is not supported, but if the parser fails it will report I added support for this format in #1701, so if something is rejected then at least the proper message is shown. |
Thank you very much. However don't make such affirmation i can easily refute, in my initial bug report made nearly two months ago - #1587 (comment) - I provided tremendous amount of data including my own LOG. Pay attention to the following: My format was perfectly handled and the rejected shares were reported untill Please let's fix the bug, it will make a lot of people happy and |
I'm not implying there is no issue. However, on my hardware, connecting to the same stratum, I get no duplicates. So, something is different and this is apparently not a bug that's happening on every device. This makes me believe there is something else at hand, and resetting Having another look at your logs from #1595, why does the pool send the same work twice? It's the exact same template, but well over 2 minutes later? This makes the miner start mining work that was already covered. The miner is deterministic, if you send the same job, it covers the same search space. |
|
for me this als looks like a pool problem, not a bitaxe firmware problem. NTDH |
Such a weird question honestly. It's how The problem is totally resolved if |
It doesn't have to rebroadcast. A new vardiff will be in effect from the new job, there's no need to enforce this by resending the blocktemplate. Just send a new difficulty and wait until the node gives you a new template.
Don't send it. That, or make new work: adjust To put it differently: extranonce2 is controlled by the miner, nothing a pool can do should be affect that. |
|
I went and checked the miningcore source on this. if (connection.Context.ApplyPendingDifficulty())
{
await connection.NotifyAsync(BitcoinStratumMethods.SetDifficulty, new object[] { connection.Context.Difficulty });
await connection.NotifyAsync(BitcoinStratumMethods.MiningNotify, currentJobParams);
}So the mechanism @blackmennewstyle described is real — I was wrong to doubt that a vardiff change triggers a notify. It does. That said, @mutatrum is right that the behaviour is genuinely weird. Re-sending a byte-for-byte identical job (same ntime, same coinbase, same merkle) is pointless — the new difficulty applies from the next job anyway, so there is no need to enforce it with a resend. If it wants to push work it should make it actually new work (roll ntime, bump extranonce, change the scriptsig, anything), otherwise just send the difficulty and wait for the node to hand over a new template. But the part I would hang the fix on is mutatrum's other point: extranonce2 is the miner's to control, and nothing a pool sends should make us reset it. Tracing @mutatrum you are the eagle eye here — does that match how you read the create_jobs flow, or am I missing something? |
|
Correct. If When The assumption here is that when a The nuance here - and proper fix - is making both sides better:
Only resetting I've created #1731. Please test and let us know. |
|
Just like i was suspected, you just did not want to accept my PR, you simply wanted to do your own fix, you just had to say it. As long as the issue is fixed, i'm happy honestly, i will certainly not disturb a dev who always to be the only one who fixes things lol |
Could be said about you too? That you jst wanted ro land a PR? Why don't you unterstand that there is no need to reset the extranonce in your suggested way if the pool does his job right? mining-core obviously does some weird stuff which no other pool does and this is what you don't want to differentiate. Mutatrum patched it in the right way and you come up with more conspiracies and unappropriate forwards like in your whole conversation trying to tell one of the main devs he doesn't know how mining works. |
The other mining software, including ASIC closed-source mining software, does not have that issue, they handle duplicate Maybe also do not talk for the other pools (plenty of them actually send duplicate job as well), it's nothing uncommon and maybe also do not talk for Now if you think you are better than anyone in the space, good on you, i'm very happy for you but i honestly could not care less of what you think about me, i'm trying to find a solution to a problem, which i never experienced before with other mining software. |
I'm sorry you got that impression, I'm not here to steal your glory. It's fine if you take the code of 1731 and apply the diff to your PR so we can merge it under your name, or as co-author, I don't care. The only thing I care about is that the issue raised is fixed in a proper way. Let me add this, as I do care: I go out of my way to always make sure the correct authors are attributed to every PR that gets submitted. Contributions from external developers are super valuable, and they do a lot of great work. On every major release we have there are a few first-time contributors, and that makes me happy. So, please don't feel discouraged to not raise a new issue or PR. This was a difficult issue because for whatever reason it was not reproducible on my side, about a nuanced piece of logic and the perseverance on both sides made the firmware better for it. |
I can report that after nearly 12 hours of mining with your fix which ignores duplicate I am closing my Apologize as well for my Thank you very much and sorry if i make you waste some of your precious time. Keep it up the great job 👍🏽 |
Excellent! Did you see it handle the duplicate
Haha, all good. Franc-parler 🤜🤛 Dutch bluntness |
Yes i saw the yellow message about them in the log ;) |
4 participants
This PR improves the extranonce2 reset.
It is only performed if clean_jobs == true.
The previous code was triggering a cascade of duplicate shares after a varDiff change:
cleanJobset to false #1587 (comment)