-
Notifications
You must be signed in to change notification settings - Fork 20.3k
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.
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
core/beacon: eth/catalyst: Implement Kiln-v2 spec #24506
Conversation
f7b069e
to
876ca42
Compare
b951e9c
to
3abd590
Compare
758fd24
to
2215ec2
Compare
} | ||
} | ||
// If the head block is already in our canonical chain, the beacon client is | ||
// probably resyncing. Ignore the update. | ||
if rawdb.ReadCanonicalHash(api.eth.ChainDb(), block.NumberU64()) == update.HeadBlockHash { | ||
log.Warn("Ignoring beacon update to old head", "number", block.NumberU64(), "hash", update.HeadBlockHash, "age", common.PrettyAge(time.Unix(int64(block.Time()), 0)), "have", api.eth.BlockChain().CurrentBlock().NumberU64()) | ||
return beacon.ForkChoiceResponse{Status: beacon.VALID}, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you accidentally removed this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise it's funny that we make a log and don't do anything else :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need to prepare the payload if that was required from us
This can happen if another node has not proposed a block in their slot and we have to propose on a previous block.
And if we don't have the block we do the else if. I can rewrite the logic a bit and downgrade the log message though
eth/catalyst/api.go
Outdated
log.Trace("Engine API request received", "method", "ExecutePayload", "number", params.Number, "hash", params.BlockHash) | ||
block, err := beacon.ExecutableDataToBlock(params) | ||
if err != nil { | ||
return api.invalid(), err | ||
log.Warn("Invalid params", "params", params, "error", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This warning message will appear without context and will be very herd to make sense of. Please refactor to Invalid NewPayload parameter
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, would be nice to see what gets serialized to the user logs? Unsure if the printout will be meaningful. Also worth a thought whether it could be attacked by feeding arbitrary text in certain fields. We've paid out bounties before for attacks trying to hijack the console/logs with "please update from here" texts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will look like this
ERROR[03-15|17:09:00.841] Invalid params params="{ParentHash:0xce7282d56c42a1618d9c04b2bad59cb3d68992023558ca59e7479bc3985f9c18 FeeRecipient:0x0000000000000000000000000000000000000000 StateRoot:0x9ec2649ca8cc97458bb8cdbb2e81bf3adf648cf1c41d0f9ecf2dc308c92f8632 ReceiptsRoot:0xae1b7b64ba6ae97588440bdafc6e33c31ef41f9e64a6e31ce9ce8828cb007254 LogsBloom:[0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0] Random:0xb18b0ece16bedc80897c5cc27101002130f1606a8e85b490f42b5408fac660b4 Number:55303 GasLimit:8000000 GasUsed:126000 Timestamp:1647360540 ExtraData:[] BaseFeePerGas:+1770307273 BlockHash:0xc7b7ba871a2628dbc765ace5183af24fc9915ec453375821833665c9536f1041 Transactions:[[2 248 121 131 20 105 202 131 1 66 151 132 3 90 16 83 132 230 80 101 73 130 82 8 148 94 253 170 39 243 217 53 55 243 50 79 222 26 242 15 238 201 4 88 39 137 1 189 51 14 8 106 136 0 0 128 192 128 160 106 43 138 33 247 103 191 182 120 29 178 181 39 158 229 53 246 101 251 60 32 185 192 40 242 3 60 238 155 145 31 181 160 92 137 205 99 225 205 27 39 12 172 168 94 224 191 232 159 231 2 73 171 104 180 135 228 238 99 125 157 229 39 224 7] [2 248 121 131 20 105 202 131 1 66 152 132 3 90 16 83 132 230 80 101 73 130 82 8 148 115 48 200 192 188 240 94 13 182 110 162 233 160 130 1 50 10 80 45 93 137 1 189 51 14 8 106 136 0 0 128 192 1 160 66 163 115 182 112 141 253 151 44 216 54 82 159 192 26 144 147 164 79 61 219 83 134 41 233 175 22 230 57 25 224 51 160 35 129 20 209 159 234 229 59 37 254 112 223 161 11 115 40 235 209 112 187 43 127 159 10 244 188 33 30 17 54 234 250] [2 248 121 131 20 105 202 131 1 66 153 132 3 90 16 83 132 230 80 101 73 130 82 8 148 58 243 235 91 110 20 22 47 172 15 169 211 63 210 176 209 46 244 207 182 137 1 189 51 14 8 106 136 0 0 128 192 128 160 255 196 55 230 193 136 194 71 94 113 49 169 68 134 14 59 237 157 26 153 186 192 9 47 16 179 203 159 199 162 67 34 160 39 200 146 44 151 125 120 31 1 45 221 165 117 88 111 22 141 199 85 137 43 240 200 217 210 114 190 19 12 195 159 57] [2 248 121 131 20 105 202 131 1 66 154 132 3 90 16 83 132 230 80 101 73 130 82 8 148 138 148 71 104 70 127 229 104 224 236 124 84 32 155 93 188 66 42 55 222 137 1 189 51 14 8 106 136 0 0 128 192 128 160 53 98 58 111 216 130 84 12 53 129 247 223 195 146 122 178 224 100 139 248 60 199 144 221 127 104 189 16 101 165 217 219 160 106 70 92 184 191 38 32 214 106 96 58 233 205 123 147 143 42 63 141 200 214 109 23 195 95 209 96 21 81 99 192 47] [2 248 121 131 20 105 202 131 1 66 155 132 3 90 16 83 132 230 80 101 73 130 82 8 148 115 183 117 134 199 86 187 193 180 154 32 93 57 86 217 99 163 81 208 240 137 1 189 51 14 8 106 136 0 0 128 192 1 160 81 228 156 177 236 85 14 113 241 31 247 98 58 23 167 59 235 212 254 46 60 53 23 196 64 70 186 111 249 239 128 249 160 114 150 84 180 33 13 79 72 210 239 143 202 203 11 97 34 225 59 126 134 171 46 177 133 136 197 142 191 235 113 35 19] [2 248 121 131 20 105 202 131 1 66 156 132 3 90 16 83 132 230 80 101 73 130 82 8 148 8 131 49 113 199 38 155 196 131 91 182 195 73 250 149 6 136 88 168 115 137 1 189 51 14 8 106 136 0 0 128 192 128 160 171 253 84 7 109 239 216 5 212 247 231 9 190 24 249 245 212 27 14 148 237 239 176 242 240 78 240 124 82 115 36 140 160 18 1 68 162 14 168 240 255 11 215 79 127 38 196 207 50 237 223 35 227 244 248 247 14 167 16 117 151 205 19 140 159]]}" error="blockhash mismatch, want c7b7ba871a2628dbc765ace5183af24fc9915ec453375821833665c9536f1041, got 4939674c8df87bbf80d2a6fbe337cc54c63d92bd016e734cf2c53f719a39bfe2"
``` its a pretty scary message for warn, I'll downgrade it to debug. But this was extremely handy when we debugged kiln
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few nitpicks, but I think otherwise it's ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
* core/beacon: eth/catalyst: updated engine api to new version * core: implement exchangeTransitionConfig * core/beacon: prevRandao instead of Random * eth/catalyst: Fix ExchangeTransitionConfig, add test * eth/catalyst: stop external miners on TTD reached * node: implement --authrpc.vhosts flag * core: allow for config override on non-mainnet networks * eth/catalyst: fix peters comments * eth/catalyst: make stop remote sealer more explicit * eth/catalyst: add log output * cmd/utils: rename authrpc.host to authrpc.addr * eth/catalyst: disable the disabling of the miner * eth: core: remove notion of terminal pow block * eth: les: more of peters nitpicks
* core/beacon: eth/catalyst: updated engine api to new version * core: implement exchangeTransitionConfig * core/beacon: prevRandao instead of Random * eth/catalyst: Fix ExchangeTransitionConfig, add test * eth/catalyst: stop external miners on TTD reached * node: implement --authrpc.vhosts flag * core: allow for config override on non-mainnet networks * eth/catalyst: fix peters comments * eth/catalyst: make stop remote sealer more explicit * eth/catalyst: add log output * cmd/utils: rename authrpc.host to authrpc.addr * eth/catalyst: disable the disabling of the miner * eth: core: remove notion of terminal pow block * eth: les: more of peters nitpicks
* core/beacon: eth/catalyst: updated engine api to new version * core: implement exchangeTransitionConfig * core/beacon: prevRandao instead of Random * eth/catalyst: Fix ExchangeTransitionConfig, add test * eth/catalyst: stop external miners on TTD reached * node: implement --authrpc.vhosts flag * core: allow for config override on non-mainnet networks * eth/catalyst: fix peters comments * eth/catalyst: make stop remote sealer more explicit * eth/catalyst: add log output * cmd/utils: rename authrpc.host to authrpc.addr * eth/catalyst: disable the disabling of the miner * eth: core: remove notion of terminal pow block * eth: les: more of peters nitpicks
This PR implements the newest merge spec
It includes the following changes:
random -> prevRandao
engine_exchangeTransitionConfigurationV1
--authrpc.vhosts
flagTerminalBlock