feat: embed commit-sealing timings in extra data - #2277
Conversation
…ra data Embed the producer's commit-sealing timings -- the same elapsed and finalize values logged by "Commit new sealing work" -- into the block's BlockExtraData, mirroring the existing TimeNano feature so the values can be read directly from the chain instead of scraped from logs. - Add SealElapsedNano and SealFinalizeNano optional fields to BlockExtraData, plus GetSealTimings getter and SetSealTimings setter. - Capture and embed the timings in miner commit() after FinalizeAndAssemble, gated on the Placeholder fork, rebuilding the block before sealing. The existing log line is left unchanged. - Presence-validate both fields in verifyHeader (errMissingSealTimings); values are per-producer and non-deterministic so only presence is checked, as with TimeNano and the Giugliano fields.
|
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (60.71%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## feature/timenano-placeholder-fork #2277 +/- ##
=====================================================================
- Coverage 55.23% 55.23% -0.01%
=====================================================================
Files 912 912
Lines 165883 165936 +53
=====================================================================
+ Hits 91631 91659 +28
- Misses 68790 68809 +19
- Partials 5462 5468 +6
... and 22 files with indirect coverage changes
🚀 New features to boost your workflow:
|
|
codegenie review |
🧞 Codegenie ReviewReviewed all 9 hunks (4 deep, 5 normal); no skipped or failed hunks. One critical correctness defect: the Hampi-gated block rebuild in CoverageReviewed 9/9 hunks.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧞 Codegenie Review
Reviewed all 9 hunks (4 deep, 5 normal); no skipped or failed hunks. One critical correctness defect: the Hampi-gated block rebuild in miner/worker.go commit() reconstructs the block from env.txs, which does not include the bor state-sync transaction appended inside finalizeAndAssemble, producing a block whose TxHash and ReceiptHash are mutually inconsistent on sprint-start blocks. One contract-consistency question: the new post-Hampi errMissingSealTimings presence rule is enforced for every header, but the fields are written on only one of the miner's block-assembly paths — please confirm the other paths never seal bor blocks post-Hampi. Two low-severity test-coverage gaps: the finalizeNano == nil disjunct of the new consensus guard and the pre-Austin branch / nil-optional-field boundary of SetSealTimings are unpinned.
Reviewed 9/9 hunks.
Coverage levels: deep 4, normal 5, light 0, skip 0.
Inline findings included in the review body:
-
⚪ Low: Hampi seal-timings presence rule is enforced globally but written on only one block-assembly path (
consensus/bor/bor.go:503)
Post-Hampi,verifyHeaderrejects any header whose decodedBlockExtraDatalacks either seal-timing field:if c.config.IsHampi(header.Number) { if header.GetTimeNano(c.chainConfig) == nil { return errMissingTimeNano } elapsedNano, finalizeNano := header.GetSealTimings(c.chainConfig) if elapsedNano == nil || finalizeNano == nil { return errMissingSealTimings } }
This rule applies to every header, but the fields are written on only one assembly path.
SetSealTimingshas exactly one non-test caller,worker.commit()atminer/worker.go:3168. The second assembly site,generateWork()atminer/worker.go:2387(reached fromworker.go:1190viagetWorkChand fromminer/payload_building.go:227/:262), assembles a header with no seal-timings write:var block *types.Block block, work.receipts, _, err = w.engine.FinalizeAndAssemble(w.chain, work.header, work.state, &body, work.receipts) if err != nil { return &newPayloadResult{err: err} }
miner/pipeline.gosimilarly constructs blocks at lines 192, 439 and 793 with no mention ofSetSealTimings.Impact: The header contract changes so that writer and validator coverage must match. If a block produced through
generateWork/getSealingBlockor the pipelined commit path is ever sealed and broadcast on a Hampi-enabled bor network, every verifying peer rejects it witherrMissingSealTimings. Whether those paths can seal a bor block post-Hampi was not verified; please confirm the intended producer paths for this contract.Suggested fix: Write the seal timings on a path shared by all bor block assembly (e.g. inside bor's
FinalizeAndAssemble, alongside whereTimeNanois set), or replicate theIsHampi-gatedSetSealTimingsplus block rebuild ingenerateWorkand the pipelined commit path. If those paths are payload/dev-only and never seal bor blocks, document that explicitly next to the new check.Suggested test: Drive the
getSealingBlock/generateWorkpath with a Hampi-enabled bor chain config and assertbor.verifyHeaderdoes not returnerrMissingSealTimingsfor the resulting header; add the same assertion for the pipelined commit path.
— codegenie v0.5.6 (a662388fde) · View Workflow Job
| if w.chainConfig.Bor != nil && w.chainConfig.Bor.IsHampi(env.header.Number) { | ||
| if err := env.header.SetSealTimings(w.chainConfig, uint64(time.Since(start).Nanoseconds()), uint64(finalizeDuration.Nanoseconds())); err != nil { | ||
| return err | ||
| } | ||
| block = types.NewBlock(env.header, &types.Body{Transactions: env.txs}, env.receipts, trie.NewStackTrie(nil)) |
There was a problem hiding this comment.
The Hampi-gated rebuild reconstructs the block from env.txs, but env.txs does not contain the bor state-sync transaction that finalizeAndAssemble appends to its own local body. The receipts, however, do include the state-sync receipt because they are returned and assigned to env.receipts.
if w.chainConfig.Bor != nil && w.chainConfig.Bor.IsHampi(env.header.Number) {
if err := env.header.SetSealTimings(w.chainConfig, uint64(time.Since(start).Nanoseconds()), uint64(finalizeDuration.Nanoseconds())); err != nil {
return err
}
block = types.NewBlock(env.header, &types.Body{Transactions: env.txs}, env.receipts, trie.NewStackTrie(nil))
}The transaction list handed to FinalizeAndAssemble is a fresh literal, so the callee's append is never visible to the caller:
block, env.receipts, commitTime, err = w.engine.FinalizeAndAssemble(w.chain, env.header, env.state, &types.Body{
Transactions: env.txs,
}, env.receipts)Inside consensus/bor/bor.go finalizeAndAssemble, the state-sync transaction is appended to the local body and its receipt is folded into the returned receipts:
if len(stateSyncData) > 0 && c.config != nil && c.config.IsMadhugiri(big.NewInt(int64(headerNumber))) {
stateSyncTx := types.NewTx(&types.StateSyncTx{StateSyncData: stateSyncData})
body.Transactions = append(body.Transactions, stateSyncTx)
receipts = insertStateSyncTransactionAndCalculateReceipt(stateSyncTx, header, body, state, receipts)
}
block := types.NewBlock(header, body, receipts, trie.NewStackTrie(nil))params/config.go contains configs where HampiBlock and MadhugiriBlock are both active (e.g. both big.NewInt(0)), so the Hampi-gated rebuild runs on Madhugiri sprint-start blocks.
Impact: On a sprint-start block with pending state-sync data, the sealed block omits the StateSyncTx from its body while its ReceiptHash is derived from receipts that include the state-sync receipt. The TxHash/ReceiptHash pair is unreproducible by any verifier and the state-sync transaction is lost from the canonical body, so every such block is rejected by peers and the producer misses its slots. This is not covered by the PR intent, which describes the rebuild only as refreshing Extra ("the block built above carries the pre-timing Extra and must be rebuilt before it is handed off for sealing").
Suggested fix: Rebuild from the body actually returned by FinalizeAndAssemble rather than from env.txs:
block = types.NewBlock(env.header, block.Body(), env.receipts, trie.NewStackTrie(nil))Alternatively, have FinalizeAndAssemble return the final transaction list and reassign env.txs, or set the timings on the header before assembly / use a header-only re-hash path so no rebuild is needed.
Suggested test: With a chain config enabling both Hampi and Madhugiri, produce a sprint-start block with pending state-sync events and assert the committed block's body contains the StateSyncTx, that len(block.Transactions()) matches the receipts count, and that bor verification of the sealed block succeeds.
| chainConfig := ¶ms.ChainConfig{ | ||
| ChainID: big.NewInt(137), | ||
| CancunBlock: cancunBlock, | ||
| Bor: ¶ms.BorConfig{ | ||
| AustinBlock: big.NewInt(100), | ||
| HampiBlock: big.NewInt(200), | ||
| }, | ||
| } | ||
|
|
||
| // Distinctive vanity and seal bytes to confirm they survive the rewrite. | ||
| vanity := bytes.Repeat([]byte{0xab}, ExtraVanityLength) | ||
| seal := bytes.Repeat([]byte{0xcd}, ExtraSealLength) | ||
|
|
||
| gasTarget := uint64(15000000) | ||
| bfcd := uint64(64) | ||
| timeNano := uint64(1700000000_000_000_000) + 123456789 | ||
| encoded, err := EncodeBlockExtraData(chainConfig, big.NewInt(200), nil, &gasTarget, &bfcd, &timeNano) | ||
| if err != nil { | ||
| t.Fatalf("failed to encode BlockExtraData: %v", err) | ||
| } | ||
|
|
||
| extra := append(append(append([]byte{}, vanity...), encoded...), seal...) | ||
| header := &Header{Number: big.NewInt(200), Extra: extra} |
There was a problem hiding this comment.
TestSetSealTimings fixes a single configuration — Austin active, all three earlier optional fields non-nil — leaving two live boundaries of SetSealTimings unverified:
chainConfig := ¶ms.ChainConfig{
ChainID: big.NewInt(137),
CancunBlock: cancunBlock,
Bor: ¶ms.BorConfig{
AustinBlock: big.NewInt(100),
HampiBlock: big.NewInt(200),
},
}
encoded, err := EncodeBlockExtraData(chainConfig, big.NewInt(200), nil, &gasTarget, &bfcd, &timeNano)
header := &Header{Number: big.NewInt(200), Extra: extra}
if err := header.SetSealTimings(chainConfig, elapsedNano, finalizeNano); err != nil {- Pre-Austin branch is never executed. With
AustinBlock: 100and header number200, only theBlockExtraDataPostAustinbranch runs. Theelsebranch decodes intoBlockExtraData, whereTxDependencysits ahead of the optional run and must survive the in-placeExtrarewrite:
} else {
var blockExtraData BlockExtraData
if err := rlp.DecodeBytes(h.Extra[ExtraVanityLength:len(h.Extra)-ExtraSealLength], &blockExtraData); err != nil {
return fmt.Errorf("decode block extra data: %w", err)
}
blockExtraData.SealElapsedNano = &elapsedNano
blockExtraData.SealFinalizeNano = &finalizeNano
blockExtraDataBytes, err = rlp.EncodeToBytes(&blockExtraData)
}The sole production caller (miner/worker.go:3168) is IsHampi-gated but does not itself guarantee Austin, so this branch ships untested.
- The nil-vs-zero boundary is unexercised.
rlp:"optional"only trims a trailing run of zero values:
type BlockExtraDataPostAustin struct {
ValidatorBytes []byte
GasTarget *uint64 `rlp:"optional"`
BaseFeeChangeDenominator *uint64 `rlp:"optional"`
TimeNano *uint64 `rlp:"optional"`
SealElapsedNano *uint64 `rlp:"optional"`
SealFinalizeNano *uint64 `rlp:"optional"`
}Setting the two new trailing fields when an earlier optional pointer is nil forces that earlier field onto the wire as an empty value, which decodes back as a non-nil zero pointer — GetTimeNano/GasTarget go from nil to 0. The fixture always passes non-nil values, so this is never observed.
Impact: SetSealTimings rewrites chain-visible header Extra in place, and consensus/bor/bor.go:503 presence-validates TimeNano and both seal timings post-Hampi. A future edit to the untested branch or to struct field order would not be caught.
Suggested fix: Add subtests TestSetSealTimings/pre_austin_preserves_tx_dependency (pre-Austin config with non-empty TxDependency, asserting TxDependency, ValidatorBytes, vanity and seal all survive) and TestSetSealTimings/nil_time_nano_promotion (extra encoded with timeNano == nil, asserting the resulting GetTimeNano/DecodeBlockExtraData semantics explicitly — either still nil, or documented as 0).
| func TestVerifyHeader_HampiMissingSealTimings(t *testing.T) { | ||
| t.Parallel() | ||
| s := newHampiVerifySetup(t, true) | ||
|
|
||
| gasTarget := uint64(15_000_000) | ||
| bfcd := uint64(64) | ||
| timeNano := uint64(1_700_000_000_000_000_000) | ||
| extra := buildBlockExtraBytes(&types.BlockExtraData{ | ||
| GasTarget: &gasTarget, | ||
| BaseFeeChangeDenominator: &bfcd, | ||
| TimeNano: &timeNano, | ||
| }) | ||
| h := s.makeSignedChild(t, extra, big.NewInt(params.InitialBaseFee)) | ||
|
|
||
| chain := newRawDBChain(s.db, s.cfg, h, nil, nil) | ||
| require.ErrorIs(t, s.b.verifyHeader(chain, h, nil), errMissingSealTimings) |
There was a problem hiding this comment.
The new consensus guard is a two-term disjunction, but no test makes the second term the deciding condition:
elapsedNano, finalizeNano := header.GetSealTimings(c.chainConfig)
if elapsedNano == nil || finalizeNano == nil {
return errMissingSealTimings
}TestVerifyHeader_HampiTimeNanoPresent sets both fields; TestVerifyHeader_HampiMissingSealTimings sets neither, so elapsedNano == nil short-circuits first:
extra := buildBlockExtraBytes(&types.BlockExtraData{
GasTarget: &gasTarget,
BaseFeeChangeDenominator: &bfcd,
TimeNano: &timeNano,
})
h := s.makeSignedChild(t, extra, big.NewInt(params.InitialBaseFee))
chain := newRawDBChain(s.db, s.cfg, h, nil, nil)
require.ErrorIs(t, s.b.verifyHeader(chain, h, nil), errMissingSealTimings)Both fields are RLP optional-tail entries:
SealElapsedNano *uint64 `rlp:"optional"`
SealFinalizeNano *uint64 `rlp:"optional"`so a peer-suppliable header that encodes SealElapsedNano and omits the trailing SealFinalizeNano is valid RLP and decodes to elapsed != nil, finalize == nil. GetSealTimings forwards the decoded pointers verbatim without normalization. Tree-wide searches for SealElapsedNano/SealFinalizeNano/errMissingSealTimings found only the both-set and neither-set cases (consensus/bor/bor_test.go:6086-6103, :6106-6122, core/types/block_test.go:901-904).
Impact: Dropping the finalizeNano == nil term (or flipping || to &&) would still pass the entire suite while loosening a fork-gated header validation rule — a partially populated header would be accepted by a patched node and rejected by others. The shipped guard is correct today; this is a test-coverage gap on a chain-split-class check.
Suggested fix: Add a third case (ideally a table-driven subtest sharing newHampiVerifySetup) that builds extra with GasTarget, BaseFeeChangeDenominator, TimeNano and SealElapsedNano only, omitting SealFinalizeNano:
extra := buildBlockExtraBytes(&types.BlockExtraData{
GasTarget: &gasTarget,
BaseFeeChangeDenominator: &bfcd,
TimeNano: &timeNano,
SealElapsedNano: &elapsedNano,
})
h := s.makeSignedChild(t, extra, big.NewInt(params.InitialBaseFee))
require.ErrorIs(t, s.b.verifyHeader(newRawDBChain(s.db, s.cfg, h, nil, nil), h, nil), errMissingSealTimings)2 participants
Summary
Embed the producer's commit-sealing timings -- the same elapsed and finalize values logged by "Commit new sealing work" -- into the block's BlockExtraData, mirroring the existing TimeNano feature so the values can be read directly from the chain instead of scraped from logs.
Executed tests
<what was actually run beyond CI's standard unit / integration / e2e gates: kurtosis scenarios, chaos runs, manual checks against Amoy / mainnet RPCs, devnet upgrades, etc. Include output or pointers to where the run lives.>
Rollout notes
<consensus-affecting? requires coordinated upgrade? backwards-compatible? operator-facing change?>