Suggested Electra test case

[michaelsproul]

We just found another consensus bug in Lighthouse's v1.5.0-alpha8 implementation that is not covered by existing tests. I'm not sure if it would be covered in the tests of more recent alphas, but figured this was worth reporting.

The bug we had is described here: sigp/lighthouse#6496 (comment)

A test case would involve:

• One or more validators pre-Electra with balance > min_activation_balance, compounding withdrawal credentials, and an activation epoch after Electra.
• A fork upgrade from this pre-Electra state to post-Electra.

Lighthouse was double-crediting these validators, as they were being processed as part of the pre-activation processing, and then (mistakenly) reprocessed as part of the processing of validators with compounding withdrawal credentials.

[sauliusgrigaitis]

Grandine also had a few cases that were not covered by spec tests.

[mkalinin]

@sauliusgrigaitis could you please describe those cases?

[nflaig]

In Lodestar we found a edge case on Mekong which was caused by running multiple epoch transitions due to a reorg. This caused a mismatch between the validators in state and our pubkey2index cache. More details can be found here ChainSafe/lodestar#7284.

This bug only affected a single node so we're pretty lucky to find it on Mekong, I would guess other clients manage their pubkey<>index mapping in a similar way (ie. shared between states) and it might be worth reviewing this edge case.

[sauliusgrigaitis]

@sauliusgrigaitis could you please describe those cases?

@weekday-grandine-io could you describe these cases in detail?

[weekday-grandine-io]

Prior to Electra, valid top-up deposits may be processed out of order.
A block may contain multiple deposits with the same pubkey, in which case the later ones are top-up deposits¹.
Grandine has an optimization that combines such deposits into a single object².
Starting with Electra, the order of deposits is recorded in BeaconState.pending_deposits.
The optimization in Grandine was incorrectly adapted to Electra.

The bug in Grandine was triggered by block 11617 in Mekong devnet 0.
Its post-state should contain pending deposits with the following pubkeys:

0x88e56f25cb821468620f2ddd4167cf174021ea033dca285241e094eb630b067a878a3c682c0cd3ee74203b1d01e7fdc9
0x898623ec7e5c9c761bfaf4a2d15f3e2dba5cb3afcf393dba35b9c3a0cb731bee6a28888a843698bac47710a877054309
0xaad541099cd9371ebb7d69c3e187e7cdc2cef4a9d41c05c106f78d767ff58bd0ee8736a53b4918614c83028d71206c14
0x83300b9567f1a829d157ff7c0f58c8452582a565733037b3cc28f77adafe645a63674258a84a67aee7d22bd83fdafaad
0xb704b109d9d50ee2199b48aa79c5d6ee20cec15e0da73a9a3337b7298400926f71d52eb0176d1e0fff106f1bf59d1da3
0x91537ca0c42448056690c954c8de704a13175ff02ea7ea9b1d8f2aa7b9d16330aa748b9fc7029a4ab729ba1af3d147df
0xb9719fdf6a97dacbde502c2bbce300b0a69b058ae855f98f72295b740fed3c06d02b1222d00ba481296709b25fce80a1
0xac4b792670965fdb774d2944fe9053b81fed1248711f45bb7be7fb1877708e9dabd25db902b58cf08f9df90a9c715778
0x907bd327d54cd071d8b9c23c5fb06efbc60ca4bcac6c92d6ca5dde5848aecbb85c646ba8a2ebc3de56723b2beadae8db
0x94ad5155aff93b5a940ee56b3dad8ebd477ef369ebdad01dfec9d64330718fdc01f3bc7feb65542efb34bffdbc94c70c
0x88e56f25cb821468620f2ddd4167cf174021ea033dca285241e094eb630b067a878a3c682c0cd3ee74203b1d01e7fdc9
0x91537ca0c42448056690c954c8de704a13175ff02ea7ea9b1d8f2aa7b9d16330aa748b9fc7029a4ab729ba1af3d147df

The bug in Grandine caused deposits to be added to BeaconState.pending_deposits in an incorrect order:

0x88e56f25cb821468620f2ddd4167cf174021ea033dca285241e094eb630b067a878a3c682c0cd3ee74203b1d01e7fdc9
0x88e56f25cb821468620f2ddd4167cf174021ea033dca285241e094eb630b067a878a3c682c0cd3ee74203b1d01e7fdc9
0x898623ec7e5c9c761bfaf4a2d15f3e2dba5cb3afcf393dba35b9c3a0cb731bee6a28888a843698bac47710a877054309
0xaad541099cd9371ebb7d69c3e187e7cdc2cef4a9d41c05c106f78d767ff58bd0ee8736a53b4918614c83028d71206c14
0x83300b9567f1a829d157ff7c0f58c8452582a565733037b3cc28f77adafe645a63674258a84a67aee7d22bd83fdafaad
0xb704b109d9d50ee2199b48aa79c5d6ee20cec15e0da73a9a3337b7298400926f71d52eb0176d1e0fff106f1bf59d1da3
0x91537ca0c42448056690c954c8de704a13175ff02ea7ea9b1d8f2aa7b9d16330aa748b9fc7029a4ab729ba1af3d147df
0x91537ca0c42448056690c954c8de704a13175ff02ea7ea9b1d8f2aa7b9d16330aa748b9fc7029a4ab729ba1af3d147df
0xb9719fdf6a97dacbde502c2bbce300b0a69b058ae855f98f72295b740fed3c06d02b1222d00ba481296709b25fce80a1
0xac4b792670965fdb774d2944fe9053b81fed1248711f45bb7be7fb1877708e9dabd25db902b58cf08f9df90a9c715778
0x907bd327d54cd071d8b9c23c5fb06efbc60ca4bcac6c92d6ca5dde5848aecbb85c646ba8a2ebc3de56723b2beadae8db
0x94ad5155aff93b5a940ee56b3dad8ebd477ef369ebdad01dfec9d64330718fdc01f3bc7feb65542efb34bffdbc94c70c

Notice that deposits with pubkeys 0x88e5…fdc9 and 0x9153…47df are bundled together.

There was another similar bug that did not get triggered in Mekong.
If a block contained multiple deposits with the same pubkey, Grandine would add them to BeaconState.pending_deposits with the same withdrawal credentials.
Doing so is incorrect because top-up deposits are allowed to contain any withdrawal credentials.

Hope this helps.
I wasn't the one who found the bug.
@povi may be able to provide more information.

Footnotes

1. The first one may be a top-up deposit too if the pubkey is already in BeaconState.validators. ↩

2. This is done to minimize updates to BeaconState. They can be costly because lists in BeaconState are represented by deeply nested binary trees. ↩

[mkalinin]

There are two more deposit test scenarios to be covered:

• Multiple transitions to the same epoch that causes new validator creation, can be implemented as sanity block tests where blocks n and n+1 are children of block n - 1, Add test verifying validator creation on two forks #4112
• Multiple deposit requests in the same block with same pubkey and different withdrawal credentials interleaved with deposit requests with another set of pubkeys, Add test with multiple deposit requests with same pubkey but different withdrawal credentials #4085

[mkalinin]

Closing this issue as all the cases described in this thread are now covered with tests. Thanks everyone for reporting those edge cases!