Do not skip non-recapture ttMove when in check #3199

syzygy1 · 2020-10-27T22:45:55Z

The qsearch() MovePicker incorrectly skips a non-recapture ttMove when in check (if depth <= DEPTH_QS_RECAPTURES). This is clearly not intended and can cause qsearch() to return a mate score when there is no mate. (Incorrect mate scores reported to the user should be extremely rare though, if possible at all.)

The bug seems to have been present in hidden form since this patch:
cad300c
Hidden, because the skipped ttMove would still be played when it was generated.

The bug was "activated" by this patch (and detected by @joergoster in #3171 and #3198):
6596f0e

This PR fixes the bug by not skipping the non-recapture ttMove when in check.

Passed non-regression STC:
https://tests.stockfishchess.org/tests/view/5f9867ea6a2c112b60691b10
LLR: 2.98 (-2.94,2.94) {-1.25,0.25}
Total: 27112 W: 2943 L: 2842 D: 21327
Ptnml(0-2): 127, 2170, 8878, 2237, 144

Passed non-regression LTC:
https://tests.stockfishchess.org/tests/view/5f9967326a2c112b60691bb0
LLR: 2.99 (-2.94,2.94) {-0.75,0.25}
Total: 18392 W: 807 L: 738 D: 16847
Ptnml(0-2): 9, 655, 7802, 718, 12

Bench: 3870606

If we are in check, we should not throw out the ttMove just because it is not a recapture.

syzygy1 · 2020-10-27T22:48:58Z

I think this still needs LTC, but I'd first like to know if we want to fix it like this or revert the patch that activated the bug.

syzygy1 · 2020-10-27T22:53:31Z

AppVeyor reports an engine bench different from the reference bench, but it seems to me I have correctly specified the new bench. What do I do wrong?

No need to split the long line in two, as this piece of code contains even longer lines. Bench: 3817613

syzygy1 · 2020-10-27T23:03:17Z

Looks like I've figured out the problem.

vondele · 2020-10-28T06:06:11Z

thanks for the analysis, I think this is the better fix, so please run an LTC test.

joergoster · 2020-10-28T06:35:09Z

I can confirm that with this fix applied the assert doesn't fire.

@vondele Please note, as I already pointed out in #3171 this is only one part of the bugfix.
This commit 843a961 also triggers the assert.

vondele · 2020-10-28T06:40:34Z

yes, we need to fix the other bit as well. We can deal with it separately, however. I have been wondering if even #3196 is related (but this PR alone doesn't fix it).

joergoster · 2020-10-28T07:07:53Z

@vondele Started a test with a fix on top of Ronald's fix.

syzygy1 · 2020-10-28T09:09:24Z

I see @vondele is also running a test, so I should probably not start LTC for now.

vondele · 2020-10-28T09:22:00Z

@syzygy1 I think might be useful nevertheless, at least that will be enough to fix part 1 of this regression. Seems like the additional pruning in qsearch might be more tricky to fix without Elo loss.

I'm getting more and more convinced that #3196 is a bug that results from this. At least the assert is triggered quickly on that case, and I don't see the wrong scores in the first round of testing (2000 searches on the input I provided in that issue) with the combined two patches in place.

syzygy1 · 2020-10-28T12:40:31Z

@vondele That would be a very welcome side effect and doesn't seem impossible. I have started an LTC test. Bench is wrong again, but we already know that it compiles (and I see no good space to remove just to fix the commit message ;)).

(Well, I could negate the ||s back into &&s as it was before. Not sure why I changed that.)

Stay closer to the previous code (without changing the logic). Bench: 3870606

syzygy1 · 2020-10-28T22:04:15Z

LTC passed.

I have made the code a bit nicer (and fixed bench). Feel free to reformat to your liking.

This patch reverts code (started as 843a961) that incorrectly prunes moves if in check, and adds an assert to make sure no wrong mate scores are given in the future. Initially discussed in official-stockfish#3171 (pointed out by joergoster) and later in official-stockfish#3199 and official-stockfish#3198. This PR effectively closes official-stockfish#3171 It is also likely fixes official-stockfish#3196 where this causes user visible incorrect TB scores, which probably result from these incorrect mate scores. Just adding the additional code that checks for being inCheck failed in multiple variants: https://tests.stockfishchess.org/tests/view/5f9be2f76a2c112b60691cfe https://tests.stockfishchess.org/tests/view/5f6b5b62c7759d4ee307d006 https://tests.stockfishchess.org/tests/view/5f9a78856a2c112b60691c3f https://tests.stockfishchess.org/tests/view/5f99f4ab6a2c112b60691bfc https://tests.stockfishchess.org/tests/view/5f992f346a2c112b60691b89 https://tests.stockfishchess.org/tests/view/5f9918796a2c112b60691b80 However, as the fixed code doesn't pass Elo-gaining bounds wrt. the revert: https://tests.stockfishchess.org/tests/view/5f9a7e8a6a2c112b60691c46 this patch justs reverts to the correct state. Bench: 4151139

Only do countermove based pruning in qsearch if we already have a move with a better score than a TB loss. This patch fixes a bug (started as 843a961) that incorrectly prunes moves if in check, and adds an assert to make sure no wrong mate scores are given in the future. It replaces a no-op moveCount check with a check for bestValue. Initially discussed in official-stockfish#3171 and later in official-stockfish#3199, official-stockfish#3198 and official-stockfish#3210. This PR effectively closes official-stockfish#3171 It also likely fixes official-stockfish#3196 where this causes user visible incorrect TB scores, which probably result from these incorrect mate scores. Passed STC and LTC non-regression tests. https://tests.stockfishchess.org/tests/view/5f9ef8dabca9bf35bae7f648 LLR: 2.93 (-2.94,2.94) {-1.25,0.25} Total: 21672 W: 2339 L: 2230 D: 17103 Ptnml(0-2): 126, 1689, 7083, 1826, 112 https://tests.stockfishchess.org/tests/view/5f9f0caebca9bf35bae7f666 LLR: 2.97 (-2.94,2.94) {-0.75,0.25} Total: 33152 W: 1551 L: 1485 D: 30116 Ptnml(0-2): 27, 1308, 13832, 1390, 19 closes official-stockfish#3214 Bench: 3625915

The qsearch() MovePicker incorrectly skips a non-recapture ttMove when in check (if depth <= DEPTH_QS_RECAPTURES). This is clearly not intended and can cause qsearch() to return a mate score when there is no mate. Introduced in cad300c and 6596f0e, as observed by joergoster in official-stockfish#3171 and official-stockfish#3198. This PR fixes the bug by not skipping the non-recapture ttMove when in check. Passed non-regression STC: https://tests.stockfishchess.org/tests/view/5f9867ea6a2c112b60691b10 LLR: 2.98 (-2.94,2.94) {-1.25,0.25} Total: 27112 W: 2943 L: 2842 D: 21327 Ptnml(0-2): 127, 2170, 8878, 2237, 144 Passed non-regression LTC: https://tests.stockfishchess.org/tests/view/5f9967326a2c112b60691bb0 LLR: 2.99 (-2.94,2.94) {-0.75,0.25} Total: 18392 W: 807 L: 738 D: 16847 Ptnml(0-2): 9, 655, 7802, 718, 12 closes official-stockfish#3199 closes official-stockfish#3198 Bench: 3870606

Fix bug in qsearch MovePicker

26e7818

If we are in check, we should not throw out the ttMove just because it is not a recapture.

Minor reformatting

4b69c7a

No need to split the long line in two, as this piece of code contains even longer lines. Bench: 3817613

joergoster mentioned this pull request Oct 28, 2020

Revert "Always remember the ttMove" #3198

Closed

Merge branch 'master' into mp_bug

af2da97

vondele mentioned this pull request Oct 28, 2020

Segmentation fault #3171

Closed

Fix bench and formatting

ad4f771

Stay closer to the previous code (without changing the logic). Bench: 3870606

vondele self-assigned this Oct 28, 2020

vondele added the to be merged Will be merged shortly label Oct 28, 2020

vondele closed this in 0f6c08c Oct 28, 2020

vondele mentioned this pull request Nov 1, 2020

Revert incorrect pruning in qsearch #3210

Closed

joergoster mentioned this pull request Nov 2, 2020

Only do countermove based pruning in qsearch #3214

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not skip non-recapture ttMove when in check #3199

Do not skip non-recapture ttMove when in check #3199

syzygy1 commented Oct 27, 2020 •

edited

syzygy1 commented Oct 27, 2020

syzygy1 commented Oct 27, 2020

syzygy1 commented Oct 27, 2020

vondele commented Oct 28, 2020

joergoster commented Oct 28, 2020

vondele commented Oct 28, 2020

joergoster commented Oct 28, 2020

syzygy1 commented Oct 28, 2020

vondele commented Oct 28, 2020

syzygy1 commented Oct 28, 2020

syzygy1 commented Oct 28, 2020

Do not skip non-recapture ttMove when in check #3199

Do not skip non-recapture ttMove when in check #3199

Conversation

syzygy1 commented Oct 27, 2020 • edited

syzygy1 commented Oct 27, 2020

syzygy1 commented Oct 27, 2020

syzygy1 commented Oct 27, 2020

vondele commented Oct 28, 2020

joergoster commented Oct 28, 2020

vondele commented Oct 28, 2020

joergoster commented Oct 28, 2020

syzygy1 commented Oct 28, 2020

vondele commented Oct 28, 2020

syzygy1 commented Oct 28, 2020

syzygy1 commented Oct 28, 2020

syzygy1 commented Oct 27, 2020 •

edited