Skip to content

Conversation

@ryanio
Copy link
Contributor

@ryanio ryanio commented Feb 11, 2022

Closes #1703

Updates (from @holgerd77):

  • Adds a preMerge HF to Common

@codecov
Copy link

codecov bot commented Feb 11, 2022

Codecov Report

Merging #1712 (9ec298b) into master (1876890) will decrease coverage by 0.05%.
The diff coverage is 22.22%.

Impacted file tree graph

Flag Coverage Δ
block 85.57% <ø> (ø)
blockchain 83.28% <ø> (ø)
client 71.91% <19.23%> (-0.24%) ⬇️
common 93.90% <100.00%> (+0.01%) ⬆️
devp2p 82.63% <ø> (+0.13%) ⬆️
ethash 90.76% <ø> (ø)
trie 86.18% <ø> (ø)
tx 89.94% <ø> (ø)
util 92.62% <ø> (ø)
vm 81.19% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@ryanio
Copy link
Contributor Author

ryanio commented Feb 11, 2022

Thanks to @g11tech for letting me know about this file for Lodestar pre/post merge testing: https://github.com/ChainSafe/lodestar/blob/master/packages/lodestar/test/sim/merge-interop.test.ts

This branch successfully passes that file with just a few modifications (summarized below, or as seen in this commit on my fork):


test/sim/merge-interop.test.ts:L42

-const terminalTotalDifficultyPreMerge = 20;
+const terminalTotalDifficultyPreMerge = 10;

The comment above this line says // 10 ttd / 2 difficulty per block = 5 blocks * 5 sec = 25 sec and the test would finish with "Merge not completed", possibly the calculation was not updated for difficulty of 20? Took too long for us to get there. 10 worked.

in test/sim/merge-interop.test.ts:L79 inside gethProc.stdout.on("data", (chunk) => {, add:

      if (str.includes("Please enter the 0x-prefixed private key to unlock")) {
        gethProc.stdin.write("0x45a915e4d060149eb4365960e6a7a45f334393093061116b197e3240065ff2d8\n");
      }

We don't currently accept pkey via command line startup args, so this writes the account pkey to the ethereumjs process when requested. This is needed for unlocking the signer account for signing PoA blocks pre-merge.


kiln/ethereumjs/common-setup.sh

#!/bin/bash -x

echo $TTD
echo $DATA_DIR
echo $scriptDir
echo $EL_BINARY_DIR

env TTD=$TTD envsubst < $scriptDir/genesisPre.tmpl > $DATA_DIR/genesis.json
echo "45a915e4d060149eb4365960e6a7a45f334393093061116b197e3240065ff2d8" > $DATA_DIR/sk.json
echo "12345678" > $DATA_DIR/password.txt
pubKey="0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b"

kiln/ethereumjs/genesisPre.tmpl
kiln/ethereumjs/genesisPost.tmpl
copy over same files from kiln/geth folder


kiln/ethereumjs/post-merge.sh

#!/bin/bash -x

scriptDir=$(dirname $0)
. $scriptDir/common-setup.sh

$EL_BINARY_DIR/ethereumjs --datadir $DATA_DIR --gethGenesis $DATA_DIR/genesis.json --saveReceipts --rpc --ws --rpcEngine --rpcEnginePort=8545 --loglevel=debug --rpcDebug

kiln/ethereumjs/pre-merge.sh

#!/bin/bash -x

scriptDir=$(dirname $0)
. $scriptDir/common-setup.sh

$EL_BINARY_DIR/ethereumjs --datadir $DATA_DIR --gethGenesis $DATA_DIR/genesis.json --saveReceipts --rpc --ws --rpcEngine --rpcEnginePort=8545 --unlock $pubKey --mine --loglevel=debug --rpcDebug

Then make sure this branch (merge-kiln) is built in the monorepo, and I used this command in lodestar/packages/lodestar:

EL_BINARY_DIR=/path_to/ethereumjs-monorepo/node_modules/.bin EL_SCRIPT_DIR=kiln/ethereumjs EL_PORT=8545 ../../node_modules/.bin/mocha test/sim/merge-interop.test.ts

Results:

✓ Send stub payloads to EL (6192ms)
✓ Post-merge, run for a few blocks (77867ms)
✓ Pre-merge, run for a few blocks (77926ms)
3 passing (3m)

We don't currently have support for eth_sendTransaction endpoint (we only have eth_sendSignedTransaction) so we can't support TX_SCENARIOS=simple yet, but that would be interesting to try to make sure also works! We could sign the txs and use the signed endpoint to test. (or add some support for eth_sendTransaction when using a "from" account in --unlock, for local testing purposes, and not for production)

@ryanio ryanio mentioned this pull request Feb 11, 2022
11 tasks
try {
await findBlock(toBuffer(parentHash), this.validBlocks, this.chain)
} catch (error: any) {
// TODO if we can't find the parent, return ACCEPTED when optimistic sync is supported
Copy link
Contributor

Choose a reason for hiding this comment

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

I think 🤔 ACCEPTED is to be provided when payload doesn't extend canonical chain and can't be fully validated (because of no parent linkage available with the client yet or may be something else too), else SYNCING.

@g11tech
Copy link
Contributor

g11tech commented Feb 11, 2022

looks 🔥 🧱 ! 🚀

Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

LGTM! ❤️

@ryanio
Copy link
Contributor Author

ryanio commented Feb 25, 2022

thanks for the review @g11tech, I will merge this into master since it currently works with devnet-4 and will start a new PR for kiln v2 updates

@ryanio ryanio merged commit 6f3da02 into master Feb 25, 2022
@ryanio ryanio deleted the merge-kiln branch February 25, 2022 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Client: Kiln🔥🧱 Tracking Issue

4 participants