Add withdrawals to PayloadIdentifier to avoid collisions

[siladu]

We have come across a case where certain network conditions cause nimbus to replay an engine_forkchoiceUpdatedV2 (FCU) request with payloadParameters, except with a small change to the Withdrawal amounts (since the validator balances have changed between requests).

This ultimately results in a missed proposal.

No error log present on besu side. Here is the error you would see on nimbus:

WRN 2023-04-09 00:59:12.717+00:00 Execution client did not return correct withdrawals topics="beacval" withdrawals_from_cl_len=16 withdrawals_from_el_len=16 

withdrawals_from_cl="@[
(index: 2265686, validator_index: 351739, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1814587), 
(index: 2265687, validator_index: 351740, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1767132), 
(index: 2265688, validator_index: 351741, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1778701), 
(index: 2265689, validator_index: 351742, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1806524), 
(index: 2265690, validator_index: 351743, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1730569), 
(index: 2265691, validator_index: 351744, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1809174), 
(index: 2265692, validator_index: 351745, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1820438), 
(index: 2265693, validator_index: 351746, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1804021), 
(index: 2265694, validator_index: 351747, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1802687), 
(index: 2265695, validator_index: 351748, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1786983), 
(index: 2265696, validator_index: 351749, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1793804), 
(index: 2265697, validator_index: 351750, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1764766), 
(index: 2265698, validator_index: 351751, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1847616), 
(index: 2265699, validator_index: 351752, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1797693), 
(index: 2265700, validator_index: 351753, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1746126), 
(index: 2265701, validator_index: 351754, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1804971)]" 

withdrawals_from_el="@[
(index: 2265686, validator_index: 351739, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1803288), 
(index: 2265687, validator_index: 351740, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1755833), 
(index: 2265688, validator_index: 351741, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1767402), 
(index: 2265689, validator_index: 351742, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1795225), 
(index: 2265690, validator_index: 351743, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1719270), 
(index: 2265691, validator_index: 351744, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1797875), 
(index: 2265692, validator_index: 351745, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1811708), 
(index: 2265693, validator_index: 351746, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1792722), 
(index: 2265694, validator_index: 351747, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1791388), 
(index: 2265695, validator_index: 351748, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1775684), 
(index: 2265696, validator_index: 351749, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1782505), 
(index: 2265697, validator_index: 351750, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1753467), 
(index: 2265698, validator_index: 351751, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1838886), 
(index: 2265699, validator_index: 351752, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1786394), 
(index: 2265700, validator_index: 351753, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1734827), 
(index: 2265701, validator_index: 351754, address: (data: [65, 191, 37, 252, 140, 82, 210, 146, 189, 102, 211, 188, 236, 216, 169, 25, 236, 185, 239, 136]), amount: 1793672)]"

ERR 2023-04-09 00:59:12.727+00:00 Cannot create block for proposal 

Here's how besu behaviour leads to this mismatch:

1. Receive FCU(1) with payloadParameter instruction to build a block including withdrawals
2. Generate payloadId from the payloadParameters, caches it and responds with the payloadId to the CL.
3. Receive FCU(2) identical to FCU(1) other than Withdrawal amounts
4. Generate colliding payloadId
5. Find payloadId in the cache so would hit the "Block proposal for the same payload id {} already present, nothing to do" log 6. Respond with the same payloadId to CL.
6. Receive getPayload request relating to FCU(2) and respond with block cached against FCU(1).

Eth R&D Discord thread with more details here: https://discord.com/channels/595666850260713488/892088344438255616/1093963476948496455

Note, I have not noticed any other CL behaving like this, all FCU replays I've seen have the same Withdrawal amounts.

Note geth added withdrawals to payloadId
ethereum/go-ethereum#26554

It appears nethermind may have behave the same as besu and therefore also have this bug (in combination with nimbus):
https://github.com/NethermindEth/nethermind/blob/4407c97970e4e392d014f5fdfb5f7773244530d4/src/Nethermind/Nethermind.Merge.Plugin/BlockProduction/PayloadPreparationService.cs#L77-L89
https://github.com/NethermindEth/nethermind/blob/e4a91624f63ad9f97cc93762da0969f250ab62fc/src/Nethermind/Nethermind.Merge.Plugin/BlockProduction/PayloadPreparationService.cs#L226-L235

It's not clear from the spec whether FCU(2) should be different to FCU(1) but it seems reasonable for besu to include withdrawals in the payloadId anyway.

[github-actions]

• I thought about documentation and added the doc-change-required label to this PR if updates are required.
• I have considered running ./gradlew acceptanceTestNonMainnet locally if my PR affects non-mainnet modules.
• I thought about the changelog and included a changelog update if required.

[garyschulte]

great place for a clarifying comment. it isn't clear whether this is determistic or not at first glance.

[siladu]

Which bit should I clarify?

This line attempts to create a deterministic hash and avoid collisions, which I believe is the same as the rest of the method.

I think the forPayloadParams method maybe needs a comment about strategy for creating the hash, but I'm missing the original context. This line is a continuation of the above code I guess.

I'm also concerned about the even numbers used in the bitshifting of the other hashes, e.g. (long) parentHash.toHexString().hashCode()) << 8
I found collisions when I did something similar with withdrawals.

[garyschulte]

It might be wise to sort before mapping the list, on the off chance that the order is different but the contents the same.

[siladu]

Withdrawals should be in the same order, but good idea to sort the list just in case as we can't guarantee it at this level. Have committed a fix for that.

[fab-10]

the current implementation is basically a custom made hash function, that try to spread hash codes of the input parameters across all the available 8 bytes, but not sure how much collision resistant it is, so we can probably just use a standard hash function that.

[garyschulte]

I have a couple questions about the actual Payload Identifier creation, but LGTM. This is a nice way to partition these requests without having to make a value judgement on either of them.

[jframe]

LGTM

[garyschulte]

👍

[garyschulte]

LGTM 👍