Prefetch Transactions from Pool & PIP-66 Back#2031
Conversation
Code ReviewBug found in consensus/bor/bor.go at line 1080 The delay calculation delay = time.Until(parentTime) computes time until the PARENT block timestamp. Since the parent block has already been mined, its timestamp is in the past, making this a negative duration. When time.After(delay) is called with a negative duration at line 1102 ( Lines 1101 to 1103 in 0077186 The old Seal logic correctly waited until header.GetActualTime() (the new block target time, which is in the future): Lines 1367 to 1370 in 0077186 Suggested fix at line 1080 ( Lines 1078 to 1082 in 0077186 Note: The test at line 1741 passes because it sets genesis time 3 seconds in the future, which makes parentTime positive. In production, the parent block is always in the past. Checked for bugs and CLAUDE.md compliance. |
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (79.90%) 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 @@
## develop #2031 +/- ##
===========================================
+ Coverage 50.15% 50.34% +0.18%
===========================================
Files 871 871
Lines 150614 150917 +303
===========================================
+ Hits 75546 75973 +427
+ Misses 70023 69889 -134
- Partials 5045 5055 +10
... and 18 files with indirect coverage changes
🚀 New features to boost your workflow:
|
The delay calculation Design IntentThe Rio fork allows early block announcement to improve block propagation speed. The primary producer (succession == 0) can produce and announce blocks as soon as Key timing relationships:
Why negative delays are correctIn production, when the parent block is in the past:
|
cffls
left a comment
There was a problem hiding this comment.
This PR is quite large to review, and it essentially contains two features. If possible, could you split it into two, one is about PIP-66 and the other is txns prefetch?
Yeah, I do also agree it got big. But is important to point that the Transaction Prefetching benefits and partially depends on PIP-66. So it would be a bit hard trying to split those changes now. What I'll try to do is provided a summary of snippets related to the PIP-66 below. 1. Early Announcement CheckThis section introduces a new and more flexible way to check early announcements too early because of the introduced Lines 405 to 438 in 6acb9e1 2. Accounting the Announcement Time into timeout for span checkAs we discussed on a chat, this address the original issue we got when introduction PIP-66. Which were considering the announcement time on the TTL instead of block time itself. Lines 496 to 508 in 6acb9e1 3. Wait on
|
Code ReviewIssue 1/2: Ignored Error from SubGasFile: Issue: Ignored error return The error returned from
While the first Suggested fix: Check the error return and handle appropriately: if err := totalGasPool.SubGas(result.TotalGasUsed); err != nil {
// Log and continue, or return early if gas exhaustion should stop prefetching
log.Debug("Prefetch gas pool exhausted", "err", err)
return
}Issue 2/2: Data Race on w.extraFile: Issue: Race condition The When Suggested fix: Either:
|
|
The latest Claude review pointed two issues, second one I agree and fixed. The first one is a false positive. |
Code ReviewFound 2 issues in the new code: 1. Ignored error from
|
|
Code ReviewNo issues found. Checked for bugs and CLAUDE.md compliance. Review details:
Notes on items verified as false positives:
|
* enabling pip-66 back and wait moved from seal to prepare * bring subsecond extra time * prefetch from pool * more prefetch metrics * address early announcement for rio blocks * separate worker cache metrics * optinal flag to disable prefetch * prefetch gas limit flag * tests and lint * small fixes * remove logs * address duplicates * small fix on integration test * prefetch coverage histogram and fmt * verify headers coverage and bor test resiliency * address duplicates * duplicates and lint * addressing comments * small fix * address comments * fix push tx for rpc nodes * rename meter and intermediateroot prefetch * address lint * address succession number check * fixing concurrency issues and tests for prefetch state being thrown away by gc * make lint * e2e worker tests * benchmark tests * worker tests fixed and interrupt watch while waiting * remove parallel from tests


Description
SealtoPreparephaseKey Changes
1. State Prefetching from Transaction Pool
miner/worker.go,core/state_prefetcher.go2. Configuration Flags
--miner.prefetch: Enable/disable transaction pool prefetching (default: disabled)--miner.prefetch.gaslimit.percent: Percentage of block gas limit to be used as internal limit of prefetched transactions.3. Enhanced Metrics
miner/worker.go,core/state/reader.go,core/blockchain.goworker/chain/...)hit_from_prefetch: Cache hits attributed to prefetchprefetch_used_unique: Unique accounts loaded by prefetch and used during processingTechnical Details
Prefetch Flow
commitWorkis called, two parallel operations start:preparephase)Results
From a local test with
polyclirunning bothrandomandUniswapmode we observed that many storages that never were prefetched are now being prefetched and also a slightly increase on accounts being early prefetchedWorker Commit Transactions Time
Instance 1 has prefetch enabled, results in ms
PS: The second instance is the one with prefetchFromPool activated
Changes
Breaking changes
Please complete this section if any breaking changes have been made, otherwise delete it
Nodes audience
In case this PR includes changes that must be applied only to a subset of nodes, please specify how you handled it (e.g. by adding a flag with a default value...)
Checklist
Cross repository changes
Testing
Manual tests
Please complete this section with the steps you performed if you ran manual tests for this functionality, otherwise delete it
Additional comments
Please post additional comments in this section if you have them, otherwise delete it