Stockfish: Atomic Illegal Moves

[sabfrompc]

I noticed that the GUI I was using started to show some weird lines after a couple of days of analyzing a position where it looked like white made nonexistant moves in the variations. Something like "... 10. Na3 b5 11. 11. 11. 11. g3 Nd5 12. Bg2 ..." and then would go back to normal after this for a while. Sometimes there would be another string of nonexistant moves later in the variation, and sometimes there wouldn't. I decided to try to solve this by running stockfish solely from command prompt. I started up stockfish and ran the following commands:

Setoption name UCI_Variant value ATOMIC //This should probably be ignored as it I wasn't sure if the variant was capitalized or not. As it wasn't a valid option it didn't appear to change anything, but I am leaving it in just to be safe. Even if this was the cause, it would still be a bug if mistyping something before the analysis started messed things up in this way.
Setoption name UCI_Variant value atomic
Setoption name MultiPV value 4
position fen rnb1kb1r/p1pp1ppp/4p3/8/8/2P1P3/PP1P1PPP/RNBK1B1R w kq - 0 7
go infinite

I took a screenshot incase anything odd happened later on:
https://i.imgur.com/3UOSxeF.png
For reference's sake, here's a picture of what that position looks like:
https://i.imgur.com/ejus5Zh.png //5. Qd4 Bd6 6. Kd1 Qh4! is another interesting line for anyone curious

After a few days of analysis, the terminal looked like this:
https://i.imgur.com/BmsOHkn.png
If we look at the evaluations that were given out we can see something odd is going on. I have attempted to translate stockfish's moves into algebraic notation, but I might have made a typo somewhere, which is why I am including a picture as well.

multiPV 1 146 cp: 7. h4 d5 8. ??? //moving some invisible piece on b4 to b5, it must be a pawn, rook or queen, this is followed by moving an invisible piece on c4 by black.
multiPV 2 146 cp: 7. d4 Nc6 8. b4 d5 9. f4 f5 10. Ba6 Bxa6 11. b5 Rc8 12. Ba3 Nb4 13 Bxb4 Be7 14. Na3 c6 15. Rb1 ??? //moving some invisible piece on g5 to f4, it must be a pawn, bishop or queen
multiPV 3 185 cp: 7. d4 ??? //moving some invisible piece on c5 to b4, it must be a pawn, bishop or queen
multiPV 4 88 cp: 7. e4 Nc6 8. Na3 Na5 9. Ba6 d5 10. f4 Nb3 11. axb3 e5 12. d4 Be7 13. f5 f6 14. dxe5 O-O 15. e5 Re8 16. Bb7 c6 17. Re1 d4 18. c4 a5 19. Bxa8 d3 20. Bd2 Bd6 21. Re3 g6 22. Bc3 fxf5 23. h3 Bb8 24. e6 d2 25. e7 h5 26. Rd3 a4 27. Ra3 h4 28. Re3 //no illegal moves in this line strangely enough

Obviously stockfish outputting illegal moves would explain the GUI's odd behavior. I decided to close stockfish, and typed the following:

exit //which apparently wasn't a valid command, so I then typed 'quit'
quit

This returned something else I found strange:
https://i.imgur.com/S1xz6Rv.png
If we look at the analysis returned here, we see the following:

multiPV 1 146 cp: 7. h4 d5 8. ??? //again, moving some invisible piece on b4 to b5, it must be a pawn, rook or queen, this is followed by moving an invisible piece on c4 by black. This is the same line as listed earlier.
multiPV 2 146 cp: 7. d4 Nc6 8. b4 d5 9. f4 f5 10. Ba6 Bxa6 11. b5 Rc8 12. Ba3 Nb4 13 Bxb4 Be7 14. Na3 c6 15. Rb1 ??? //again, moving some invisible piece on g5 to f4, it must be a pawn, bishop or queen. This is the same line as listed earlier.
multiPV 3 mate: 7. a4 //really odd how this is not only not a check, or a mate, but that it comes third in the list. This line was not listed as one of the lines earlier before I typed 'quit'.
multiPV 4 185 cp: 7. b1a3 ??? //moving some invisible piece on c5 to b4, it must be a pawn, bishop or queen. This line was not listed as one of the lines earlier before I typed 'quit'.

I did some searching and found that this was similar to what was written about in this post on lichess's forums:
https://lichess.org/forum/team-atomic-chess-theoreticians/bug-using-stockfish-on-windows-with-comrades-gui
Perhaps the user there just didn't realize that the engine was still outputting illegal moves as it is a little harder to see in the command prompt? I wanted to try and get it working with winboard to test it there as well, since user Luelinks claimed to have success with that, but didn't have much success getting the two to work together...

[ianfab]

Considering the issues in https://github.com/ddugovic/Stockfish/issues/425 it might be that there are more hidden bugs in atomic chess that are only found after long analysis. We should maybe add more assertions for atomic chess which could help in identifying these issues. Just like in the issue I mentioned, finding a position where the bug can be reproduced in a shorter time helps a lot. The output in your screenshots suggest that do_move and undo_move are not always able to restore the original position, but this is just a guess.

[benwh1]

@ianfab That seems like a good idea - I'm running more atomic searches right now and ran into another segfault earlier today on the current version, after #425 was fixed.

[sabfrompc]

Well, I can't say what exactly is causing it, but all I know is that deep analysis in atomic causes it to output invalid moves in pretty much every instance. As it's always fairly deep I would suspect an overflow of some sort, but again, I have little to no knowledge of how stockfish actually works. Glad to know that others who are more technically minded are aware of this and on the case. If there's anything else I could do to help, I'd be happy to, but I'm not really sure what that would be...

In case it's relevant this was compiled by MinGW on a windows 8 machine then run on a windows 7 machine with: "make profile-build ARCH=x86-64-modern COMP=mingw" I didn't bother using "strip stockfish.exe" as the file was already fairly small.

[ianfab]

@sabfrompc A thorough bug report as in this case already helps a lot. If you by chance encounter a position where a bug occurs sooner, please let us know.

The only overflow I can currently think of would be the move count. This might indeed be an issue, since the maximum move count could be higher than in standard chess, because adjadcent kings can avoid making a position illegal where one or both kings are attacked. However, if you compiled Stockfish with the default set of variants (including crazyhouse) this can not be the cause of the issue, because we double the maximum move count if compiled with crazyhouse.

Nevertheless I would still be interested if someone knows of an upper or lower bound on the maximum number of moves in a legal atomic chess position (in standard chess I think it was about 230). To be on the safe side, we should maybe increase the value not only if compiled with crazyhouse, but also for atomic and horde.

Regarding this issue, could you maybe recompile Stockfish with debug=yes (after a make clean) and rerun it (preferably with a debugger) on a position where you found issues? There might be a failed assertion that would hint at where to start the investigation.

[alwey]

Perhaps there is an instabilty.
Atomic start position startpos (recent Stockfish 9cc849b):

go perft 4:
...
d2d3: 11959
...
Nodes searched: 197281

Enter position after move d2d3:

position fen rnbqkbnr/pppppppp/8/8/8/3P4/PPP1PPPP/RNBQKBNR b KQkq - 0 1
go perft 3
...
Nodes searched: 11963

BTW cutechess has no tests for atomic in its test suite. I noticed this some weeks ago and prepared some tests to fill the void.

cutechess tst_board has a (startpos) perft 4 value of 197326 with split value d2d3: 11963 for 3 plies at startpos and the same value for 3 plies from position after move d2d3. FWIW.

[ddugovic]

432 is also a mess at the moment... it really should be possible to run ./stockfish bench all with Fast = false in position.cpp. I spent some time this morning trying to fix both the assertion code and all the bugs... sigh.

(╯° °）╯︵ ┻━┻

[sabfrompc]

@ianfab I will try to do that in the morning if you can tell me what debugger to use (assuming you don't just mean setting debug=true and leave it at that), and let you know how it turns out. Don't expect anything too soon though, as it takes a while for this to happen in my experience even though it happens in pretty much all positions eventually.

[ianfab]

@sabfrompc I usually use dbg, but any debugger should do. I would just like to have a partly or full stacktrace in case there is a failed assertion. That, or even only the message of the failed assertion, would already help a lot.

[ddugovic]

Thanks @alwey . Using latest master my perft numbers are:

go perft 4:
...
d2d3: 11963
...
Nodes searched: 197326

Enter position after move d2d3:

position fen rnbqkbnr/pppppppp/8/8/8/3P4/PPP1PPPP/RNBQKBNR b KQkq - 0 1
go perft 3
...
Nodes searched: 11963

[sabfrompc]

Well, i tried, but I couldn't get things working with dbg... As such I have decided to try to find a position where this issue occurs in a much more timely manner, and I think I have one:

fen: 3r2kr/1p1R4/8/8/1P5p/4P3/P1P2PP1/R5K1 b - - 0 20

I tried this position several times, with slightly different settings, all of which resulted in the issue occuring in under 30 minutes with 64 bit stockfish 231017 both via commandline and with an atomic GUI. Granted this depends on your computer, but the one I tested on was using was a laptop that isn't too great by modern standards. I tried it with 1 thread and 2 threads. I would recommend using 2 threads as it produces it more quickly than it should based on my understanding of how threading works. It produces the issue in around 1/3-1/4 of the time, well less than 1/2 at the very least. The issue had always occurred by the time that the engine recognizes that the position is a forced mate in all lines from my 4-5 tests. Interestingly, at a lower depth, I saw it go from saying a line was a mate in 25, to saying it was a mate in 33 at the next depth with that position with 1 thread during one of my tests.

[benwh1]

I ran that position with debugging and got the same en passant assertion fail as in #437. I removed that assert and ran it again and got a different one a few lines later:

position.cpp:1240: void Position::do_move(Move, StateInfo&, bool): Assertion `piece_on(capsq) == make_piece(them, PAWN)' failed.
Aborted (core dumped)

Commented out that block of 5 assertions to see if anything else would happen, and noticed something strange. Running the engine before, I got an evaluation of about -10.71 at depth 30, but with those assertions removed I got -17.74 and it took much longer to reach depth 30.

[benwh1]

With the 5 assertions mentioned in the previous comment removed, I got another en passant-related assert failure after about 1 hour:

position.cpp:1613: void Position::undo_move(Move): Assertion `to == st->previous->epSquare' failed.
Aborted (core dumped)

[ddugovic]

As you suggested in the other issue, this is reproducible (I changed Threads to 2):

setoption name UCI_Variant value atomic
setoption name Threads value 2
position fen 3r2kr/1p1R4/8/8/1P5p/4P3/P1P2PP1/R5K1 b - - 0 20
go infinite

resulting in:

info depth 25 seldepth 43 multipv 1 score cp -843 upperbound nodes 47380260 nps 1669200 hashfull 999 tbhits 0 time 28385 pv h8h6 d7g7
info depth 25 seldepth 43 multipv 1 score cp -851 upperbound nodes 53552369 nps 1668298 hashfull 999 tbhits 0 time 32100 pv h8h6 d7g7
info depth 25 seldepth 43 multipv 1 score cp -862 upperbound nodes 61196993 nps 1670725 hashfull 999 tbhits 0 time 36629 pv h8h6 d7g7
stockfish: position.cpp:1237: void Position::do_move(Move, StateInfo&, bool): Assertion `to == st->epSquare' failed.

[ddugovic]

Ah, I think I see it now (having added a bit of logging code)...

A player in check may win by explosion. However, a "killer" en passant move may only win if it is a legal move. I saw a position similar to 4k3/4R3/8/8/7p/8/6PK/8 b - - 0 1 and Stockfish illegally executed h4g3ep# (the assertion failed because epSquare == SQ_NONE).

[ianfab]

@ddugovic Do I understand correctly that your fix would solve the issues for all positions mentioned in this thread?

[ddugovic]

My fix corrects:

1. Killer pawn capture checkmate move must capture a piece
2. Killer en passant pawn capture checkmate move must capture a piece

I was so fixated on the latter of these that I failed to consider:
a) Pawn capture checkmate must capture an opponent's piece
b) En passant pawn capture checkmate must capture an opponent's pawn and to must be the en passant square (something apparently @official-stockfish checks by generating a full list of LEGAL moves)

[ddugovic]

@ianfab I think edbe9ef is what we need? But pseudo-legal move validation is complicated, and it's challenging to test all combinations of player-in-check-delivers-mate (both legal and illegal) so if anyone wants to code review, that might help.

[ddugovic]

Looking at Position::capture gives me an idea 995bec73 . I think that'll do it... this (entire branch) increases ./stockfish bench atomic 16 1 10 from 217913 to 217914.

[ddugovic]

The more closely I inspect this issue the more sources of potential error I discover...

In theory a killer move (for example, [X]c4d5) could be legal in some positions (c4xd5#; Bc4d5) but not others (Nc4xd5#; c4d5). I assume this is more of a problem for multi-threaded searches than for single-threaded searches. I think b50ae33 fixes that (copying code from later in the same function) however pseudo-legal move validation isn't simple to test.

[ddugovic]

It seems in my testing, pseudo-legal move validation doesn't need to handle "capture puts player in check" which is validated by Position::legal.

[ianfab]

@ddugovic I think that everything that is taken into account in move generation should also be handled in pseudo-legal move validation, and everything else should be considered only in legal move validation.

[ddugovic]

@ianfab Good point. For standard chess (and I think exactly the same for atomic chess), legal move validation does not validate:

1. En passant square
2. Castling
3. King evasion moves
4. Unpinned pieces (or pinned pieces moving/capturing along the pin)

however 1 & 2 are checked by pseudo-legal move validation (which uses move generation):

  // Use a slower but simpler function for uncommon cases
#ifdef CRAZYHOUSE
  if (type_of(m) != NORMAL && type_of(m) != DROP)
#else
  if (type_of(m) != NORMAL)
#endif
      return MoveList<LEGAL>(*this).contains(m);

and I think pseudo-legal move validation in e3c5478 checks 3 & 4.

[ddugovic]

OK... looking again at this, atomic move generation only excludes self-explode captures when generating captures, but apparently not for other phases which happen to be captures! So perhaps it makes sense to remove:

      if (Type == CAPTURES)
          target &= ~pos.attacks_from<KING>(pos.square<KING>(us));

and to allow all captures to pass pseudo_legal, not "all other than self-explode captures". On the other hand, that seems to double the bench number...?

[ianfab]

@ddugovic In my opinion it makes sense to do all validation that is trivial in move generation, since it improves performance, but to avoid complex validation that can be written in a much more simple way in legal move validation. Couldn't we simply replace

      if (Type == CAPTURES)
          target &= ~pos.attacks_from<KING>(pos.square<KING>(us));

by
target &= ~(pos.attacks_from<KING>(pos.square<KING>(us)) & pos.pieces(~us));
and add a similar change in evasion generation?

I have not worked much with the atomic move generation and validation code yet, but if I have time, I will study that code over the weekend to be able to give more feedback.

[ddugovic]

Interesting, I'll explore that move generation improvement (which sounds reasonable based on what sounds like a reasonable assumption about how target is used).

[ddugovic]

Another factor to consider is that for non-normal moves, pseudo_legal delegates to legal, so (for promotion capture moves, and moves with an illegal en passant square) this condition may need to be tested first.

[ianfab]

@ddugovic If move generation excludes such cases, then there should not be an issue with return MoveList<LEGAL>(*this).contains(m); as only the moves that are generated are passed to legal and not the move that is to be tested for pseudo-legality. And the generated moves should always be pseudo-legal (i.e., ready to be tested by legal), otherwise it is a bug.

[ddugovic]

@ianfab I agree... so I removed that pseudo_legal check and (with some logging and assertion code added) am troubleshooting why "king takes pawn" is occurring (FEN 3R4/4k3/4P3/1p2r3/1P4P1/7p/P4P1K/8 b - - 6 33) from the originally reported position (Threads = 2)...

#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007ffff6f1bf5d in __GI_abort () at abort.c:90
#2  0x00007ffff6f11f17 in __assert_fail_base (fmt=<optimized out>, 
    assertion=assertion@entry=0x5555555f33b8 "!pos.is_atomic() || !pos.capture(move) || !(pos.attacks_from<KING>(to_sq(move)) & pos.square<KING>(pos.side_to_move()))", file=file@entry=0x5555555f306c "search.cpp", line=line@entry=1084, 
    function=function@entry=0x5555555f3880 <Value (anonymous namespace)::search<((anonymous namespace)::NodeType)0>(Position&, Search::Stack*, Value, Value, Depth, bool, bool)::__PRETTY_FUNCTION__> "Value {anonymous}::search(Position&, Search::Stack*, Value, Value, Depth, bool, bool) [with {anonymous}::NodeType NT = (<unnamed>::NodeType)0]") at assert.c:92
#3  0x00007ffff6f11fc2 in __GI___assert_fail (
    assertion=assertion@entry=0x5555555f33b8 "!pos.is_atomic() || !pos.capture(move) || !(pos.attacks_from<KING>(to_sq(move)) & pos.square<KING>(pos.side_to_move()))", file=file@entry=0x5555555f306c "search.cpp", line=line@entry=1084, 
    function=function@entry=0x5555555f3880 <Value (anonymous namespace)::search<((anonymous namespace)::NodeType)0>(Position&, Search::Stack*, Value, Value, Depth, bool, bool)::__PRETTY_FUNCTION__> "Value {anonymous}::search(Position&, Search::Stack*, Value, Value, Depth, bool, bool) [with {anonymous}::NodeType NT = (<unnamed>::NodeType)0]") at assert.c:101
#4  0x00005555555b73f6 in (anonymous namespace)::search<(<unnamed>::NodeType)0>(Position &, Search::Stack *, Value, Value, Depth, bool, bool) (
    pos=..., ss=ss@entry=0x7ffff5acfd90, alpha=alpha@entry=-2264, beta=beta@entry=-2263, depth=depth@entry=6, cutNode=cutNode@entry=true, 
    skipEarlyPruning=false) at search.cpp:1084
#5  0x00005555555b7979 in (anonymous namespace)::search<(<unnamed>::NodeType)0>(Position &, Search::Stack *, Value, Value, Depth, bool, bool) (
    pos=..., ss=ss@entry=0x7ffff5acfd60, alpha=alpha@entry=2263, beta=beta@entry=2264, depth=depth@entry=6, cutNode=cutNode@entry=true, 
    skipEarlyPruning=false) at search.cpp:1295
#6  0x00005555555b7979 in (anonymous namespace)::search<(<unnamed>::NodeType)0>(Position &, Search::Stack *, Value, Value, Depth, bool, bool) (
    pos=..., ss=ss@entry=0x7ffff5acfd30, alpha=-2264, beta=-2263, depth=depth@entry=9, cutNode=false, skipEarlyPruning=false) at search.cpp:1295
#7  0x00005555555b74af in (anonymous namespace)::search<(<unnamed>::NodeType)0>(Position &, Search::Stack *, Value, Value, Depth, bool, bool) (
    pos=..., ss=ss@entry=0x7ffff5acfd00, alpha=alpha@entry=2263, beta=beta@entry=2264, depth=depth@entry=9, cutNode=cutNode@entry=true, 
    skipEarlyPruning=false) at search.cpp:1307
#8  0x00005555555b7979 in (anonymous namespace)::search<(<unnamed>::NodeType)0>(Position &, Search::Stack *, Value, Value, Depth, bool, bool) (
    pos=..., ss=ss@entry=0x7ffff5acfcd0, alpha=-2264, beta=-2263, depth=depth@entry=12, cutNode=true, skipEarlyPruning=false) at search.cpp:1295
#9  0x00005555555b74af in (anonymous namespace)::search<(<unnamed>::NodeType)0>(Position &, Search::Stack *, Value, Value, Depth, bool, bool) (
    pos=..., ss=ss@entry=0x7ffff5acfca0, alpha=2263, beta=2264, depth=depth@entry=12, cutNode=false, skipEarlyPruning=false) at search.cpp:1307
#10 0x00005555555b74af in (anonymous namespace)::search<(<unnamed>::NodeType)0>(Position &, Search::Stack *, Value, Value, Depth, bool, bool) (
    pos=..., ss=ss@entry=0x7ffff5acfc70, alpha=alpha@entry=-2264, beta=beta@entry=-2263, depth=depth@entry=13, cutNode=cutNode@entry=true, 
    skipEarlyPruning=false) at search.cpp:1307
#11 0x00005555555b7979 in (anonymous namespace)::search<(<unnamed>::NodeType)0>(Position &, Search::Stack *, Value, Value, Depth, bool, bool) (
    pos=..., ss=ss@entry=0x7ffff5acfc40, alpha=alpha@entry=2263, beta=beta@entry=2264, depth=depth@entry=15, cutNode=cutNode@entry=true, 
    skipEarlyPruning=false) at search.cpp:1295
#12 0x00005555555b7979 in (anonymous namespace)::search<(<unnamed>::NodeType)0>(Position &, Search::Stack *, Value, Value, Depth, bool, bool) (
    pos=..., ss=ss@entry=0x7ffff5acfc10, alpha=-2264, beta=-2263, depth=depth@entry=18, cutNode=false, skipEarlyPruning=false) at search.cpp:1295
#13 0x00005555555b74af in (anonymous namespace)::search<(<unnamed>::NodeType)0>(Position &, Search::Stack *, Value, Value, Depth, bool, bool) (
    pos=..., ss=ss@entry=0x7ffff5acfbe0, alpha=2263, beta=2264, depth=depth@entry=19, cutNode=true, skipEarlyPruning=false) at search.cpp:1307
#14 0x00005555555b74af in (anonymous namespace)::search<(<unnamed>::NodeType)0>(Position &, Search::Stack *, Value, Value, Depth, bool, bool) (
    pos=..., ss=ss@entry=0x7ffff5acfbb0, alpha=-2264, beta=-2263, depth=depth@entry=20, cutNode=false, skipEarlyPruning=false) at search.cpp:1307

[ddugovic]

Nevermind, I think I removed the wrong test (from pseudo_legal; the corresponding test in legal has already been converted into an assertion).

[ianfab]

@ddugovic Could you please give an example of how to reproduce the issue with current master, and if you already understand the issue, give a brief explanation of what the bug is about? Since we mixed the discussion about the bug with discussions about refactoring, I am now a bit confused what really has to be fixed.

[ddugovic]

@ianfab There is at least one bug and multiple possible sources of error, in addition to the desire to de-duplicate (and hopefully simplify) code:

1. Killer/TT moves must be pseudo-legal according to how chess pieces move (
   e2896c9) for all piece types and for all move types
2. Pseudo-legal moves cannot self-explode
3. En passant square cannot be adjacent to own king

setoption name UCI_Variant value atomic
setoption name Threads value 2
position fen 3r2kr/1p1R4/8/8/1P5p/4P3/P1P2PP1/R5K1 b - - 0 20
go infinite

[ianfab]

@ddugovic Thanks. I also would like to simplify the code and remove redundant checks, but I think we should do that after fixing the bug(s) in order to avoid complicating matters.

I tried to understand the reason for the failed assertion in your example and found that returning true too early in pseudo_legal for moves that capture the opponent's king is the issue. E.g., when we are not allowed to capture en passant because the opponent did not advance his pawn, but we get an en passant move from TT that would capture the king with this en passant move, then we return true too early.

I have written a minimal fix for this bug where I tried to avoid touching the existing code in order to make clear what is changed. The point of this patch is to do the exact same validation for king captures as for other moves except that we can ignore checks. In order to make debugging simpler, I used an example that quickly (~2s) exposes the bug:

setoption name UCI_Variant value atomic
position fen 4k3/1p3R2/8/8/1P2P2p/8/2Pr1PPK/3R4 w - -
go infinite

Edit: With this fixed, I do not get any failed assertions in both examples. Point 1 should be fixed by this and 2 and 3 can currently not cause issues, since there is a redundant check in legal.

[ddugovic]

Ah, I suppose that is a simpler fix than my approach of "do not make pseudo_legal responsible for checks discovered by blast moves".

3 doesn't immediately cause an error, however for normal chess Stockfish doesn't set an e.p. square when the move itself is illegal. On the other hand, checking for such legality is not nearly this simple as that doesn't consider checkmate or self-discovered check.

I'd still like to finish testing & merging 125cd1e .

[ianfab]

@ddugovic For me the point of 125cd1e is not entirely clear. Due to the redundant check in legal I do not see what it fixes, except from maybe improving performance as illegal moves are rejected earlier. In principle I think moving the self-explode check to move generation and pseudo-legal move validation instead of doing it in legal validation is good, but as I said I would like to separate refactoring from bugfixing, and your commit message suggests that it fixes this issue, but if I am not mistaken it does not (at least the example I mentioned still gives the same failed assertion).

[ddugovic]

@ianfab Thanks, your reasoning makes sense. OK, we can defer refactoring until after the bug is fixed per your recommendation.

[ddugovic]

The point of 125cd1e is twofold:

1. Follow your earlier refactoring advice that move generation and pseudo_legal apply similar rules
2. Avoid pseudo-random side effect in MovePicker constructor which increments stage due to ttMove being a not-pseudo-legal move

[ianfab]

@ddugovic So should I prepare a fix similar to the minimal one? And is this currently the only known bug in atomic move generation/validation?

I am open to refactoring once the bug is fixed, since redundant and inconsisent validation makes the code harder to read and less efficient, and simplifications are anyway welcome.

[ddugovic]

@ianfab I think so? The following is also anomalous but I can't prove it to be a move generation bug. In the final benchmark position, if the move generator generates a non-pseudo-legal move (Kxa6) then the bench number increases, irrespective of whether that move is later removed from the move list!

./stockfish bench atomic 16 1 1
...
Position: 9/9
info depth 1 seldepth 1 multipv 1 score cp 54 nodes 12 nps 12000 tbhits 0 time 1 pv a7b6

I think a reasonable output would contain nodes 3 as if illegal move Kxa6 weren't generated. But maybe that unexpected side effect increases Elo somehow, so maybe it's a feature rather than a bug.

[ianfab]

@ddugovic I am now testing https://github.com/ianfab/Stockfish/commit/612fb1bd5b47a839ddf3e245bf1237c443d034a1. I have run both test positions to high depths (see commands below) and have not observed any issues.

setoption name UCI_Variant value atomic
setoption name Threads value 2
position fen 3r2kr/1p1R4/8/8/1P5p/4P3/P1P2PP1/R5K1 b - - 0 20
go depth 40

setoption name UCI_Variant value atomic
position fen 4k3/1p3R2/8/8/1P2P2p/8/2Pr1PPK/3R4 w - -
go depth 125



md5-db203d7da60c3e8f8fec67d9ed0c08a5



setoption name UCI_Variant value atomic
position fen 8/k7/P7/8/8/6p1/2p3N1/2K5 b - - 0 1
d
go perft 1
go depth 1
info string variant atomic startpos rnbqkbnr/pppppppp/8/8/8/8/PPPPPPPP/RNBQKBNR w KQkq - 0 1

 +---+---+---+---+---+---+---+---+
 |   |   |   |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+
 | k |   |   |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+
 | P |   |   |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+
 |   |   |   |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+
 |   |   |   |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+
 |   |   |   |   |   |   | p |   |
 +---+---+---+---+---+---+---+---+
 |   |   | p |   |   |   | N |   |
 +---+---+---+---+---+---+---+---+
 |   |   | K |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+

Fen: 8/k7/P7/8/8/6p1/2p3N1/2K5 b - - 0 1
Key: 21EFA4AE4B1BE447
Checkers: 
a7b8: 1
a7b6: 1
a7a8: 1

Nodes searched: 3

info depth 1 seldepth 1 multipv 1 score cp 54 nodes 12 nps 12000 tbhits 0 time 1 pv a7b6
bestmove a7b6



md5-8220f3693e3b88f927c294a06ed2997c



setoption name UCI_Variant value atomic
position fen 8/8/P7/k7/8/6p1/2p3N1/2K5 b - - 0 1
d
go perft 1
go depth 1
info string variant atomic startpos rnbqkbnr/pppppppp/8/8/8/8/PPPPPPPP/RNBQKBNR w KQkq - 0 1

 +---+---+---+---+---+---+---+---+
 |   |   |   |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+
 |   |   |   |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+
 | P |   |   |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+
 | k |   |   |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+
 |   |   |   |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+
 |   |   |   |   |   |   | p |   |
 +---+---+---+---+---+---+---+---+
 |   |   | p |   |   |   | N |   |
 +---+---+---+---+---+---+---+---+
 |   |   | K |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+

Fen: 8/8/P7/k7/8/6p1/2p3N1/2K5 b - - 0 1
Key: A395D027C7DE740D
Checkers: 
a5a4: 1
a5b4: 1
a5b5: 1
a5b6: 1

Nodes searched: 4

info depth 1 seldepth 1 multipv 1 score cp 54 nodes 4 nps 4000 tbhits 0 time 1 pv a5b6
bestmove a5b6

[ddugovic]

@ianfab Excellent point - still, generating Kxa6 affects the ordering in which moves are picked due to MovePicker constructor using a pseudo-illegal ttMove.

With 125cd1e I see perft 1 ordered (Kb6, Kb8, Ka8) as below; without it I see perft 1 ordered (Kb8, Kb6, Ka8) as in your example

Stockfish 191117 64 BMI2 by T. Romstad, M. Costalba, J. Kiiski, G. Linscott
setoption name UCI_Variant value atomic
position fen 8/k7/P7/8/8/6p1/2p3N1/2K5 b - - 0 1
d
go perft 1
go depth 1
info string variant atomic startpos rnbqkbnr/pppppppp/8/8/8/8/PPPPPPPP/RNBQKBNR w KQkq - 0 1

 +---+---+---+---+---+---+---+---+
 |   |   |   |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+
 | k |   |   |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+
 | P |   |   |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+
 |   |   |   |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+
 |   |   |   |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+
 |   |   |   |   |   |   | p |   |
 +---+---+---+---+---+---+---+---+
 |   |   | p |   |   |   | N |   |
 +---+---+---+---+---+---+---+---+
 |   |   | K |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+

Fen: 8/k7/P7/8/8/6p1/2p3N1/2K5 b - - 0 1
Key: 21EFA4AE4B1BE447
Checkers: 
a7b6: 1
a7b8: 1
a7a8: 1

Nodes searched: 3

info depth 1 seldepth 1 multipv 1 score cp 54 nodes 3 nps 3000 tbhits 0 time 1 pv a7b6
bestmove a7b6

a6+ doesn't affect the move ordering - here I substitute a bishop for a pawn, with 125cd1e:

Stockfish 191117 64 BMI2 by T. Romstad, M. Costalba, J. Kiiski, G. Linscott
setoption name UCI_Variant value atomic
position fen 8/k7/B7/8/8/6p1/2p3N1/2K5 b - - 0 1
d
go perft 1
go depth 1info string variant atomic startpos rnbqkbnr/pppppppp/8/8/8/8/PPPPPPPP/RNBQKBNR w KQkq - 0 1

 +---+---+---+---+---+---+---+---+
 |   |   |   |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+
 | k |   |   |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+
 | B |   |   |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+
 |   |   |   |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+
 |   |   |   |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+
 |   |   |   |   |   |   | p |   |
 +---+---+---+---+---+---+---+---+
 |   |   | p |   |   |   | N |   |
 +---+---+---+---+---+---+---+---+
 |   |   | K |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+

Fen: 8/k7/B7/8/8/6p1/2p3N1/2K5 b - - 0 1
Key: B08DC4A7F9EA0A06
Checkers: 
a7b6: 1
a7b8: 1
a7a8: 1

Nodes searched: 3

and master:

Stockfish 191117 64 BMI2 by T. Romstad, M. Costalba, J. Kiiski, G. Linscott
setoption name UCI_Variant value atomic
position fen 8/k7/B7/8/8/6p1/2p3N1/2K5 b - - 0 1
d
go perft 1
go depth 1info string variant atomic startpos rnbqkbnr/pppppppp/8/8/8/8/PPPPPPPP/RNBQKBNR w KQkq - 0 1

 +---+---+---+---+---+---+---+---+
 |   |   |   |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+
 | k |   |   |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+
 | B |   |   |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+
 |   |   |   |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+
 |   |   |   |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+
 |   |   |   |   |   |   | p |   |
 +---+---+---+---+---+---+---+---+
 |   |   | p |   |   |   | N |   |
 +---+---+---+---+---+---+---+---+
 |   |   | K |   |   |   |   |   |
 +---+---+---+---+---+---+---+---+

Fen: 8/k7/B7/8/8/6p1/2p3N1/2K5 b - - 0 1
Key: B08DC4A7F9EA0A06
Checkers: 
a7b8: 1
a7b6: 1
a7a8: 1

Nodes searched: 3

[ianfab]

@ddugovic I did not mean to say that I do not see a point in removing this strange behavior. It is just not a bug per se as it does not result in illegal moves being played, so fixing the bug(s) first has higher priority for me. However, it would make sense to exclude self-explode moves in move generation and pseudo-legal move validation, since it is relatively easy to implement and improves both clarity and performance (in terms of nps, not necessarily measurably Elo).

[ddugovic]

@ianfab Certainly I'll merge a3417a7 as that doesn't conflict with removing this strange behavior.

I'll also rebase 125cd1e (which I think can be tested at [-10, 5] even though it adds code) as there is some point in trying to remove the strange behavior.