Avoid wrongful TT/history updates when unwinding the search during a thread stop

[AndyGrant]

Bench 1511354

Probcut does not have the same logic that exists in the primary move loop during pvsearch, which guards against acting on invalid scores from child nodes. With this PR, it does. The solution, and the comment, are copy-pasted from the other location.

STC non-reg: https://tests.stockfishchess.org/tests/view/66fd0c2e86d5ee47d953b9af
LLR: 2.93 (-2.94,2.94) <-1.75,0.25>
Total: 67136 W: 17581 L: 17398 D: 32157
Ptnml(0-2): 215, 7478, 18033, 7593, 249

LTC non-reg: https://tests.stockfishchess.org/tests/view/66feec5686d5ee47d953bb4f
LLR: 2.94 (-2.94,2.94) <-1.75,0.25>
Total: 350766 W: 88748 L: 88862 D: 173156
Ptnml(0-2): 211, 36626, 101794, 36570, 182

[github-actions]

clang-format 18 needs to be run on this PR.
If you do not have clang-format installed, the maintainer will run it when merging.
For the exact version please see https://packages.ubuntu.com/noble/clang-format-18.

(execution 11229608985 / attempt 1)

[vondele]

This kind of patch needs testing on fishtest. IMO as an Elo gainer.

[AndyGrant]

/s? Elo gainer for a bug fix? Best you could ask for is non-reg.

[cj5716]

I think a non-reg here is enough imo. Especially when it is to do with obeying go nodes more strictly and accurately

[vondele]

It doesn't change the behavior of go nodes in any significant way, it is not an exact bound by design.

[cj5716]

Oh, I see. Would it be better for us to follow the go nodes command exactly, together with this change? Or would it be preferred to stick to what we currently have

[AndyGrant]

Well if you do aim to have go nodes be an exact bound, which I believe is quite readily doable, then you create the conditions to be able to perfectly reproduce any game played on 1-thread, so long as you have a PGN containing the node counters ( openbench-cutechess / Fastchess ), and the original settings. Which is a very valuable tool for debugging, or inspecting things.

... but that was just musings in the PR message. Fixing the bug of storing a results which "cannot be trusted" is the primary goal. Any time the search stops midway on 1-thread ( hitting upper bound on time / nodes ), you dump bad data into the TT. Similarly, on an N-thread test, the remaining N-1 helper threads will also risk dumping this bad data as the N-1 helper threads unwind their search stacks.

[AndyGrant]

STC non-reg: https://tests.stockfishchess.org/tests/view/66fd0c2e86d5ee47d953b9af
LLR: 2.93 (-2.94,2.94) <-1.75,0.25>
Total: 67136 W: 17581 L: 17398 D: 32157
Ptnml(0-2): 215, 7478, 18033, 7593, 249

Run an LTC if desired, although I do believe that just a single STC non-reg has long been the standard for bugfixes in this project.

[ddobbelaere]

Correct me if I'm wrong, but at the moment we don't care much about "correctness" of TT values either, with hash collisions a very likely thing to happen by design. We do this because using a better hash function loses Elo.

[AndyGrant]

Sure. You can safely store some garbage into the TT and not have the world fall apart. However, this exact bug is addressed in the main loop as well. The main loop case is more important, because its not just the TT, but also has a chance to swap the root move.

Would you rather have an engine filled with bugs, that is exactly 3500 elo. Or an engine without bugs, which is still 3500 elo. I frankly don't care if this gets merged -- its clear that there is some ideological issue at play, hence Pere's meltdown. You guys are welcome to keep your bugs if you want to. In the future, when I have bugfixes I learn of through my work on Torch, I'll just pass them off to another SF-dev and keep my name out of it.

[AndyGrant]

LTC non-reg: https://tests.stockfishchess.org/tests/view/66feec5686d5ee47d953bb4f
LLR: 2.94 (-2.94,2.94) <-1.75,0.25>
Total: 350766 W: 88748 L: 88862 D: 173156
Ptnml(0-2): 211, 36626, 101794, 36570, 182