Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add withdrawals to PayloadIdentifier to avoid collisions #5321

Merged
merged 5 commits into from
Apr 11, 2023
Merged

Add withdrawals to PayloadIdentifier to avoid collisions #5321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9f45ac6
Add withdrawals to PayloadIdentifier to avoid collisions
siladu Apr 10, 2023
9c13cb4
javadoc
siladu Apr 10, 2023
806c013
changelog
siladu Apr 10, 2023
0035242
Sort list to make sure it's deterministic and add more tests
siladu Apr 10, 2023
78c20f7
Update withdrawls engine APIs to have unique validatorIndex
jframe Apr 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
- Make QBFT validator smart contract mode work with london fork [#5249](https://github.com/hyperledger/besu/issues/5249)
- Try to connect to EthStats server by default with ssl followed by non-ssl. [#5301](https://github.com/hyperledger/besu/pull/5301)
- Allow --miner-extra-data to be used in Proof-of-Stake block production [#5291](https://github.com/hyperledger/besu/pull/5291)
- Add withdrawals to payloadId calculation to avoid collisions [#5321](https://github.com/hyperledger/besu/pull/5321)

### Download Links

Expand Down
4 changes: 2 additions & 2 deletions ...tests/tests/src/test/resources/jsonrpc/engine/test-cases/11_shanghai_prepare_payload.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
},
{
"index": "0x1",
"validatorIndex": "0x0",
"validatorIndex": "0x1",
"address": "0xfe3b557e8fb62b89f4916b721be55ceb828dbd73",
"amount": "0x2"
}
Expand All @@ -39,7 +39,7 @@
"latestValidHash": "0x3559e851470f6e7bbed1db474980683e8c315bfce99b2a6ef47c057c04de7858",
"validationError": null
},
"payloadId": "0x0065bd32d4f95410"
"payloadId": "0x0065bd329c251cd9"
}
},
"statusCode" : 200
Expand Down
46 changes: 46 additions & 0 deletions ...ests/src/test/resources/jsonrpc/engine/test-cases/12_shanghai_prepare_payload_replay.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
{
"request": {
"jsonrpc": "2.0",
"method": "engine_forkchoiceUpdatedV2",
"params": [
{
"headBlockHash": "0x3559e851470f6e7bbed1db474980683e8c315bfce99b2a6ef47c057c04de7858",
"safeBlockHash": "0x3559e851470f6e7bbed1db474980683e8c315bfce99b2a6ef47c057c04de7858",
"finalizedBlockHash": "0x0000000000000000000000000000000000000000000000000000000000000000"
},
{
"timestamp": "0x10",
"prevRandao": "0x0000000000000000000000000000000000000000000000000000000000000000",
"suggestedFeeRecipient": "0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b",
"withdrawals": [
{
"index": "0x0",
"validatorIndex": "0x0",
"address": "0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b",
"amount": "0x3"
},
{
"index": "0x1",
"validatorIndex": "0x1",
"address": "0xfe3b557e8fb62b89f4916b721be55ceb828dbd73",
"amount": "0x4"
}
]
}
],
"id": 67
},
"response": {
"jsonrpc": "2.0",
"id": 67,
"result": {
"payloadStatus": {
"status": "VALID",
"latestValidHash": "0x3559e851470f6e7bbed1db474980683e8c315bfce99b2a6ef47c057c04de7858",
"validationError": null
},
"payloadId": "0x0065bd329c251d59"
}
},
"statusCode" : 200
}
12 changes: 6 additions & 6 deletions ...e/test-cases/12_shanghai_get_payload.json → ...e/test-cases/13_shanghai_get_payload.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"jsonrpc": "2.0",
"method": "engine_getPayloadV2",
"params": [
"0x0065bd32d4f95410"
"0x0065bd329c251d59"
],
"id": 67
},
Expand All @@ -14,7 +14,7 @@
"executionPayload": {
"parentHash": "0x3559e851470f6e7bbed1db474980683e8c315bfce99b2a6ef47c057c04de7858",
"feeRecipient": "0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b",
"stateRoot": "0x7a6f2e03f2348dc75731e6e767c97a88da50430748a030874e27f7c3fac3d49d",
"stateRoot": "0xc8c8e840369eac89a610bfe2ec21fcdee4c9c43bec4876f0129fcd4b5311f6dd",
"logsBloom": "0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
"prevRandao": "0x0000000000000000000000000000000000000000000000000000000000000000",
"gasLimit": "0x1c9c380",
Expand All @@ -28,17 +28,17 @@
"index": "0x0",
"validatorIndex": "0x0",
"address": "0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b",
"amount": "0x1"
"amount": "0x3"
},
{
"index": "0x1",
"validatorIndex": "0x0",
"validatorIndex": "0x1",
"address": "0xfe3b557e8fb62b89f4916b721be55ceb828dbd73",
"amount": "0x2"
"amount": "0x4"
}
],
"blockNumber": "0x2",
"blockHash": "0x5dec96e86ccd61362959cc73a0992b033fcde6f01d7467393363538c74c5260d",
"blockHash": "0x4a7b0e390d320a07150daa9c3b9dddfc6b7f6ca7d7a2971ef2f53ac1f29f24dc",
"receiptsRoot": "0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421"
},
"blockValue": "0x0"
Expand Down
0 ...lid_null_withdrawals_execute_payload.json → ...lid_null_withdrawals_execute_payload.json
View file
Open in desktop
File renamed without changes.
6 changes: 3 additions & 3 deletions ...st-cases/14_shanghai_execute_payload.json → ...st-cases/15_shanghai_execute_payload.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,13 @@
},
{
"index": "0x1",
"validatorIndex": "0x0",
"validatorIndex": "0x1",
"address": "0xfe3b557e8fb62b89f4916b721be55ceb828dbd73",
"amount": "0x2"
}
],
"blockNumber": "0x2",
"blockHash": "0x5dec96e86ccd61362959cc73a0992b033fcde6f01d7467393363538c74c5260d",
"blockHash": "0x4f88d512a0045bc6d447ba74a18eac0ed2ebb8d9faca325f5f55b2ca84be0705",
"receiptsRoot": "0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421"
}
],
Expand All @@ -41,7 +41,7 @@
"id": 67,
"result": {
"status": "VALID",
"latestValidHash": "0x5dec96e86ccd61362959cc73a0992b033fcde6f01d7467393363538c74c5260d",
"latestValidHash": "0x4f88d512a0045bc6d447ba74a18eac0ed2ebb8d9faca325f5f55b2ca84be0705",
"validationError": null
}
},
Expand Down
28 changes: 0 additions & 28 deletions ...sts/tests/src/test/resources/jsonrpc/engine/test-cases/15_shanghai_update_forkchoice.json
View file
Open in desktop

This file was deleted.

28 changes: 28 additions & 0 deletions ...sts/tests/src/test/resources/jsonrpc/engine/test-cases/16_shanghai_update_forkchoice.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
{
"request": {
"jsonrpc": "2.0",
"method": "engine_forkchoiceUpdatedV2",
"params": [
{
"headBlockHash": "0x4f88d512a0045bc6d447ba74a18eac0ed2ebb8d9faca325f5f55b2ca84be0705",
"safeBlockHash": "0x4f88d512a0045bc6d447ba74a18eac0ed2ebb8d9faca325f5f55b2ca84be0705",
"finalizedBlockHash": "0x4f88d512a0045bc6d447ba74a18eac0ed2ebb8d9faca325f5f55b2ca84be0705"
},
null
],
"id": 67
},
"response": {
"jsonrpc": "2.0",
"id": 67,
"result": {
"payloadStatus": {
"status": "VALID",
"latestValidHash": "0x4f88d512a0045bc6d447ba74a18eac0ed2ebb8d9faca325f5f55b2ca84be0705",
"validationError": null
},
"payloadId": null
}
},
"statusCode": 200
}
0 ..._withdrawals_address1_latest_balance.json → ..._withdrawals_address1_latest_balance.json
View file
Open in desktop
File renamed without changes.
0 ..._withdrawals_address2_latest_balance.json → ..._withdrawals_address2_latest_balance.json
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion ...ge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ public PayloadIdentifier preparePayload(

final PayloadIdentifier payloadIdentifier =
PayloadIdentifier.forPayloadParams(
parentHeader.getBlockHash(), timestamp, prevRandao, feeRecipient);
parentHeader.getBlockHash(), timestamp, prevRandao, feeRecipient, withdrawals);

if (blockCreationTask.containsKey(payloadIdentifier)) {
LOG.debug(
Expand Down
23 changes: 20 additions & 3 deletions ...e/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/PayloadIdentifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,12 @@
import org.hyperledger.besu.datatypes.Address;
import org.hyperledger.besu.datatypes.Hash;
import org.hyperledger.besu.datatypes.Quantity;
import org.hyperledger.besu.ethereum.core.Withdrawal;

import java.math.BigInteger;
import java.util.Comparator;
import java.util.List;
import java.util.Optional;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonValue;
Expand Down Expand Up @@ -50,24 +54,37 @@ public PayloadIdentifier(final Long payloadId) {
}

/**
* Create payload identifier for payload params.
* Create payload identifier for payload params. This is a deterministic hash of all payload
* parameters that aims to avoid collisions
*
* @param parentHash the parent hash
* @param timestamp the timestamp
* @param prevRandao the prev randao
* @param feeRecipient the fee recipient
* @param withdrawals the withdrawals
* @return the payload identifier
*/
public static PayloadIdentifier forPayloadParams(
final Hash parentHash,
final Long timestamp,
final Bytes32 prevRandao,
final Address feeRecipient) {
final Address feeRecipient,
final Optional<List<Withdrawal>> withdrawals) {

return new PayloadIdentifier(
timestamp
^ ((long) parentHash.toHexString().hashCode()) << 8
^ ((long) prevRandao.toHexString().hashCode()) << 16
^ ((long) feeRecipient.toHexString().hashCode()) << 24);
^ ((long) feeRecipient.toHexString().hashCode()) << 24
^ (long)
withdrawals
.map(
ws ->
ws.stream()
.sorted(Comparator.comparing(Withdrawal::getIndex))
.map(Withdrawal::hashCode)
.reduce(1, (a, b) -> a ^ (b * 31)))
Comment on lines +83 to +86
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

.orElse(0));
}

@Override
Expand Down
91 changes: 90 additions & 1 deletion ...c/test/java/org/hyperledger/besu/consensus/merge/blockcreation/PayloadIdentifierTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,19 @@
*/
package org.hyperledger.besu.consensus.merge.blockcreation;

import static java.util.Collections.emptyList;
import static org.assertj.core.api.AssertionsForClassTypes.assertThat;

import org.hyperledger.besu.datatypes.Address;
import org.hyperledger.besu.datatypes.GWei;
import org.hyperledger.besu.datatypes.Hash;
import org.hyperledger.besu.ethereum.core.Withdrawal;

import java.util.List;
import java.util.Optional;

import org.apache.tuweni.bytes.Bytes32;
import org.apache.tuweni.units.bigints.UInt64;
import org.junit.Test;

public class PayloadIdentifierTest {
Expand All @@ -43,8 +48,92 @@ public void serializesToEvenHexRepresentation() {
public void conversionCoverage() {
var idTest =
PayloadIdentifier.forPayloadParams(
Hash.ZERO, 1337L, Bytes32.random(), Address.fromHexString("0x42"));
Hash.ZERO, 1337L, Bytes32.random(), Address.fromHexString("0x42"), Optional.empty());
assertThat(new PayloadIdentifier(idTest.getAsBigInteger().longValue())).isEqualTo(idTest);
assertThat(new PayloadIdentifier(idTest.getAsBigInteger().longValue())).isEqualTo(idTest);
}

@Test
public void differentWithdrawalAmountsYieldDifferentHash() {
final List<Withdrawal> withdrawals1 =
List.of(
new Withdrawal(
UInt64.valueOf(100),
UInt64.valueOf(1000),
Address.fromHexString("0x1"),
GWei.of(1)),
new Withdrawal(
UInt64.valueOf(200),
UInt64.valueOf(2000),
Address.fromHexString("0x2"),
GWei.of(2)));
final List<Withdrawal> withdrawals2 =
List.of(
new Withdrawal(
UInt64.valueOf(100),
UInt64.valueOf(1000),
Address.fromHexString("0x1"),
GWei.of(3)),
new Withdrawal(
UInt64.valueOf(200),
UInt64.valueOf(2000),
Address.fromHexString("0x2"),
GWei.of(4)));
final Bytes32 prevRandao = Bytes32.random();
var idForWithdrawals1 =
PayloadIdentifier.forPayloadParams(
Hash.ZERO, 1337L, prevRandao, Address.fromHexString("0x42"), Optional.of(withdrawals1));
var idForWithdrawals2 =
PayloadIdentifier.forPayloadParams(
Hash.ZERO, 1337L, prevRandao, Address.fromHexString("0x42"), Optional.of(withdrawals2));
assertThat(idForWithdrawals1).isNotEqualTo(idForWithdrawals2);
}

@Test
public void differentOrderedWithdrawalsYieldSameHash() {
final List<Withdrawal> withdrawals1 =
List.of(
new Withdrawal(
UInt64.valueOf(100),
UInt64.valueOf(1000),
Address.fromHexString("0x1"),
GWei.of(1)),
new Withdrawal(
UInt64.valueOf(200),
UInt64.valueOf(2000),
Address.fromHexString("0x2"),
GWei.of(2)));
final List<Withdrawal> withdrawals2 =
List.of(
new Withdrawal(
UInt64.valueOf(200),
UInt64.valueOf(2000),
Address.fromHexString("0x2"),
GWei.of(2)),
new Withdrawal(
UInt64.valueOf(100),
UInt64.valueOf(1000),
Address.fromHexString("0x1"),
GWei.of(1)));
final Bytes32 prevRandao = Bytes32.random();
var idForWithdrawals1 =
PayloadIdentifier.forPayloadParams(
Hash.ZERO, 1337L, prevRandao, Address.fromHexString("0x42"), Optional.of(withdrawals1));
var idForWithdrawals2 =
PayloadIdentifier.forPayloadParams(
Hash.ZERO, 1337L, prevRandao, Address.fromHexString("0x42"), Optional.of(withdrawals2));
assertThat(idForWithdrawals1).isEqualTo(idForWithdrawals2);
}

@Test
public void emptyOptionalAndEmptyListWithdrawalsYieldDifferentHash() {
final Bytes32 prevRandao = Bytes32.random();
var idForWithdrawals1 =
PayloadIdentifier.forPayloadParams(
Hash.ZERO, 1337L, prevRandao, Address.fromHexString("0x42"), Optional.empty());
var idForWithdrawals2 =
PayloadIdentifier.forPayloadParams(
Hash.ZERO, 1337L, prevRandao, Address.fromHexString("0x42"), Optional.of(emptyList()));
assertThat(idForWithdrawals1).isNotEqualTo(idForWithdrawals2);
}
}
Loading